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

Change AJV allErrors default and support user setting #955

Merged
merged 3 commits into from
Aug 31, 2024

Conversation

mdmower-csnw
Copy link
Contributor

@mdmower-csnw mdmower-csnw commented Aug 30, 2024

BREAKING CHANGE: Request and response validation stops after the first failure. Only one error will be reported even when multiple may exist. This follows best practices from AJV:

To report all validation errors (only recommended in development), option allErrors can be set in express-openapi-validator request and response validation options. For example:

app.use(
  OpenApiValidator.middleware({
    apiSpec: 'path/to/openapi.json',
    validateRequests: {
      allErrors: true,
    },
    validateResponses: {
      allErrors: true,
    },
  })
);

AJV security recommendations advise that allErrors should not be set to true in production (more details in #954). Update createAjv() so that it does not set allErrors by default and add support for letting users define allErrors in request and response validation options.

Added tests to ensure that the number of errors reported (when multiple are expected) is correct based on how allErrors is set (or unset).

The allErrors configuration for OpenAPISchemaValidator is not changed by this commit since that validation is for trusted content.

Fixes #954

AJV recommends setting option `allErrors` to `false` in production.
pdate `createAjv()` to respect the user's setting. Avoid introducing a
breaking change by defaulting to `true` when not defined by the user.

Add tests:
1. Make sure `AjvOptions` sets the value appropriately based on whether
   the end user defined `allErrors` or not.
2. When validating requests, make sure the number of errors reported
   (when multiple occur) is 1 when `allErrors` is `false`.

The `allErrors` configuration for OpenAPISchemaValidator is not changed
by this commit since that validation is for trusted content.

Fixes cdimascio#954
@mdmower-csnw
Copy link
Contributor Author

@cdimascio - couple of questions for this PR:

  1. I opted to not introduce a breaking change by defaulting allErrors to true if not defined by the end user. Do you agree with this, or would you prefer to follow AJV's recommendation and default it to false?
  2. Should I open a 2nd PR for the oas3.1 branch? Or do you plan to keep merging master into it over time?

@cdimascio
Copy link
Owner

@mdmower-csnw thanks for highlighting this and providing a fix. the ajv default is described as:

allErrors

Check all rules collecting all errors. Default is to return after the first error.

see ref

As a result, let's default to false following the security guidance and best practice

- Do not set allErrors by default **breaking change**
@mdmower-csnw
Copy link
Contributor Author

Thanks for the quick response @cdimascio . Changes made and PR description updated.

@mdmower-csnw mdmower-csnw changed the title Support setting allErrors for AJV validation Change AJV allErrors default and support user setting Aug 31, 2024
@cdimascio
Copy link
Owner

cdimascio commented Aug 31, 2024

@mdmower-csnw apologies for the multiple followups.

since this parameter allErrors impacts AJV behaviors, it should be available on both the request and response validator options i.e. validateRequests and validateResponses, rather than on root option object. These option objects configure each independently.

export interface OpenApiValidatorOpts {
  apiSpec: DeepImmutable<OpenAPIV3.Document> | string;
  validateApiSpec?: boolean;
  validateResponses?: boolean | ValidateResponseOpts;  // <--- In these opts
  validateRequests?: boolean | ValidateRequestOpts;  // <--- and these opts

Also, if you're up for it, it will be fantastic if you can also add the new option usages notes to the docs here

- Allow allErrors to be set on requests and responses independently
@mdmower-csnw
Copy link
Contributor Author

Thanks @cdimascio, that makes sense. I've moved the option to request and response validation options and added tests for each. A PR for related documentation is at cdimascio/express-openapi-validator-documentation#1 .

@cdimascio
Copy link
Owner

Thank you!

@cdimascio cdimascio merged commit 392f1dd into cdimascio:master Aug 31, 2024
5 checks passed
@mdmower-csnw
Copy link
Contributor Author

@cdimascio - could you check on the change history entry for this? https://github.com/cdimascio/express-openapi-validator/blob/master/CHANGE_HISTORY.md#2024-08-31 It doesn't look like the breaking change was recorded correctly.

Also, if this was released as version 5.3.4, the versions in the documentation could use an update: cdimascio/express-openapi-validator-documentation#1 (review)

@cdimascio
Copy link
Owner

Thanks will take a look

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.

Support configuring AJV allErrors to follow their recommended security practices
2 participants