Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance the OpenAPI plugin to accept user-provided values for parameters #17339

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

artem-smotrakov
Copy link
Contributor

This is a patch for #17315

Main changes:

  • Added parameter_values_location configuration option for the OpenAPI plugin. The option allows to set a path to a YAML file which contains context-specific parameter values.
  • Added ParameterValues class which can load context-specific parameter values from a YAML file. The file contains a mapping { path, parameter name } -> list of values . Currently, it doesn't use an HTTP method but it can be easily updated. For in: body parameter type, a user can specify a JSON payload, but it's not currently possible to specify a value for a particular field in the JSON payload.
  • Updated ParameterHandler to use context-specific parameter values. If a user provided custom parameter values, then the class prefers them, and tries to enumerate all possible combinations of the specified parameters.
  • Fixed a possible null-dereference in get_uri() method.
  • Added a couple of tests.

I am testing the feature on a couple of applications but would like to start a review now to get feedback earlier.

@artem-smotrakov
Copy link
Contributor Author

I have tested it with a couple of applications - it worked well.

Copy link
Owner

@andresriancho andresriancho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Loads parameter values from YAML.
:param string: Definition of parameter values in YAML.
"""
content = yaml.load(string)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User might enter an invalid YAML, that case should be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll add a try-except block to catch YAMLError and wrap the exception in to ParameterValueParsingError.

h = ('This option sets a path to a YAML file which contains parameter values'
' which should be used in testing API endpoints. If no parameter values are provided,'
' the plugin tries to guess them.')
o = opt_factory('parameter_values_location', self._parameter_values_location, d, INPUT_FILE, help=h)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the YAML format validation should be done here? That could be done by changing the INPUT_FILE type to YAML_INPUT_FILE and creating the logic for handling that.

It should be fairly easy, take a look at:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that may be nice. Although input_file_option.py doesn't look that simple at first glance :) Okay, I'll see what I can do here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, input_file_option.py doesn't look simple, but yaml_file_option.py should be simple :-)

Take a look at the other files in the same directory, those will give you ideas on how simple the inputs can actually be.

h = ('This option sets a path to a YAML file which contains parameter values'
' which should be used in testing API endpoints. If no parameter values are provided,'
' the plugin tries to guess them.')
o = opt_factory('parameter_values_location', self._parameter_values_location, d, INPUT_FILE, help=h)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter_values_location should be changed to parameter_values_file (that is how input files are usually named)

o = opt_factory('output_file', self._output_file_name, d, OUTPUT_FILE)

o = opt_factory('output_file', self._file_name, d, OUTPUT_FILE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem. Then, custom_spec_location should be probably renamed to custom_spec_file

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

- 1234567
- name: birth-date
values:
- 2000-01-02
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this feature requires its own documentation in https://github.com/andresriancho/w3af/blob/develop/doc/sphinx/scan-rest-apis.rst

I would make the example in get_long_desc shorter, and reference users to the documentation with something like: "For more information and examples about this feature read the framework documentation at https://docs.w3af.org"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll update both description and docs. In fact, the docs need to be updated with descriptions of the recent changes/features. I hope I can find time to do that.

@andresriancho
Copy link
Owner

I need to work harder on merging your PR. If I don't do that I'll end up in a branch merge nightmare.

Sorry for not being more responsive with these PRs. I was on vacations. Will try to merge all these amazing things you've been working on to develop as soon as the comments are solved.

@artem-smotrakov
Copy link
Contributor Author

Thanks for your comments. Let's work on them one by one, and I'll resolve possible conflicts which may appear. Then, it should be easier to merge them to develop.

@artem-smotrakov
Copy link
Contributor Author

I have updated the patch:

  • Renamed configuration parameters in the crawl.open_api plugin.
  • Added YAML_INPUT_FILE option type.
  • Updated docs and description for the crawl.open_api plugin.
  • The crawl.open_api plugin now expects an invalid YAML file.
  • Added TestOpenAPICustomParameterValues for the new configuration option.
  • Added TestOpenAPICustomSpec for custom_spec_file configuration option.

@artem-smotrakov
Copy link
Contributor Author

@andresriancho When you have some time, could you please take a look at this pull request? I have also updated other pull requests and added several comments - looking for a review. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants