Skip to content

3.5.0 OpenAPI3 - enumerated query params are not validated propertly #794

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

Closed
kgarlikowski opened this issue Jan 3, 2018 · 3 comments · Fixed by #802
Closed

3.5.0 OpenAPI3 - enumerated query params are not validated propertly #794

kgarlikowski opened this issue Jan 3, 2018 · 3 comments · Fixed by #802
Assignees
Milestone

Comments

@kgarlikowski
Copy link
Contributor

Version

  • vert.x core: 3.5.0
  • vert.x web: 3.5.0

Context

I encountered an exception which looks suspicious while trying to force my query parameters to be properly validated.

Example api definition:

      parameters:
        - name: param1
          in: query
          required: true
          schema:
            type: string
            enum: ["foo", "bar"]

when sending request with query parameters ?param1=foo&param1=baz it passes validation but should not. Opposite order (?param1=baz&param1=foo) ends up in ValidationException as expected.

I've narrowed it down to an issue with EnumTypeValidator and provided testcases showcasing the problem: https://github.com/vert-x3/vertx-web/compare/master...kgarlikowski:open-api-validation-issues?expand=1. The testcase should_not_pass_disallowed_values_opposite_order fails as it does not produce ValidationException.

It is caused by ParameterTypeValidator only validating first value from the whole list of query parameters.

  default RequestParameter isValidCollection(List<String> value) throws ValidationException {
    return this.isValid(value.get(0));
  }
@slinkydeveloper slinkydeveloper self-assigned this Jan 3, 2018
@slinkydeveloper slinkydeveloper added the component/api-contract/bug Bugs for vertx-web-api-contract label Jan 3, 2018
@slinkydeveloper slinkydeveloper added this to the 3.5.1 milestone Jan 3, 2018
@slinkydeveloper
Copy link
Member

slinkydeveloper commented Jan 3, 2018

The parameter you have defined should be an array. If you pass two parameters with the same name, only first parameter is validated, the second one is discarded. If you want both parameters, you should define it as:

parameters:
- name: param1
  in: query
  required: true
  schema:
    type: array
    items:
      type: string
      enum: ["foo", "bar"]

As default for OAS 3, your parameter will have style: form and explode: true: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#style-examples

@kgarlikowski
Copy link
Contributor Author

Actually my initial idea was to have it as just enum - not array - and if case anybody sends more than one value it will not validate as API defines expectation of a single value.
I've noticed that it's not the case, it only validates first value. In the end - is this a correct approach?

I've also tested your suggestion and it does not work exactly as you described - with in: query the style is correctly defaults to form but explode is being set to false. Only when explicitly stated explode: true it validates all of the parameters. It looks like a bug in swagger-parser: swagger-api/swagger-parser#569

@slinkydeveloper
Copy link
Member

When you send more than one parameter with the same name (formally an array with style form exploded) it should throw a ValidationException, but it doesn't. This is a bug and I will fix that 😄

The new version of swagger-parser will be included in 3.5.1 of vertx-web-api-contract, and it has this fix swagger-api/swagger-parser#569

slinkydeveloper added a commit that referenced this issue Jan 4, 2018
slinkydeveloper added a commit that referenced this issue Jan 9, 2018
@slinkydeveloper slinkydeveloper mentioned this issue Jan 12, 2018
21 tasks
@pmlopes pmlopes removed the component/api-contract/bug Bugs for vertx-web-api-contract label Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants