search

APIDevTools / swagger-parser

Swagger 2.0 and OpenAPI 3.0 parser/validator
https://apitools.dev/swagger-parser
MIT License
1.09k stars 154 forks source link

fix: Bump z-schema 4.2.3 → 5.0.1 for ReDoS fix in validator #166

Closed nvenegas closed 3 years ago

nvenegas commented 3 years ago

WIP because the earliest version of z-schema that would have the required bump to validator is 5.0.1. See https://github.com/zaggino/z-schema/pull/265

Creating this early just to see if any tests fail with the 5.x line of z-schema. Note, however, that on master there is one failing test (see CI CD #28)

  68 passing (12s)
  1 failing

  1) Real-world APIs
       26) akeneo.comakeneo.comakeneo.com:
     MissingPointerError: Token "parent" does not exist.

I get the same single failure in this branch

nvenegas commented 3 years ago

5.0.0 of z-schema is the next release after 4.2.3 (see https://www.npmjs.com/package/z-schema?activeTab=versions) but I'm unsure what parts of the API changed. I'll ask in the other PR

nvenegas commented 3 years ago

@JamesMessinger Can you please help me figure out what else I'd need to do to get this (future) version bump released? e.g., getting master green again, release process, etc.

JamesMessinger commented 3 years ago

@nicolasv - As long as all tests pass (run npm run clean && npm run build && npm test) you should be good to submit your PR. That'll kick-off our CI workflow, which will make sure the tests pass on all OSes and all supported versions of Node. Assuming that passes, we can merge the PR and publish the new version.

nvenegas commented 3 years ago

5.0.0 of z-schema is the next release after 4.2.3 (see https://www.npmjs.com/package/z-schema?activeTab=versions) but I'm unsure what parts of the API changed. I'll ask in the other PR

From the other PR

dropped node 6 and a change in default behaviour of breakOnFirstError v4.2.3...v5.0.0

nvenegas commented 3 years ago

As long as all tests pass… you should be good to submit your PR.

@JamesMessinger After upgrading to 5.0.1 of z-schema, I still get the single failure in the 26) akeneo.comakeneo.comakeneo.com test case that I also get on master

That'll kick-off our CI workflow, which will make sure the tests pass on all OSes and all supported versions of Node. Assuming that passes, we can merge the PR and publish the new version.

I noticed that npm run build modifies the online/js/bundle.* files — should I also commit those files to this PR or does CI handle that?

seriousme commented 3 years ago

As long as all tests pass… you should be good to submit your PR.

@JamesMessinger After upgrading to 5.0.1 of z-schema, I still get the single failure in the 26) akeneo.comakeneo.comakeneo.com test case that I also get on master

This is what #165 fixes ;-)

That'll kick-off our CI workflow, which will make sure the tests pass on all OSes and all supported versions of Node. Assuming that passes, we can merge the PR and publish the new version.

I noticed that npm run build modifies the online/js/bundle.* files — should I also commit those files to this PR or does CI handle that?

bradfordayers commented 3 years ago

@nicolasv Are you planning on committing this soon? Our company was hoping for the exact change you're making as it will fix a vulnerability found with the z-schema@4.2.3.

kibertoad commented 3 years ago

@JamesMessinger Could you please review this?

nvenegas commented 3 years ago

@nicolasv Are you planning on committing this soon? Our company was hoping for the exact change you're making as it will fix a vulnerability found with the z-schema@4.2.3.

I've been waiting on some more review plus potentially #165 being merged 🤞

bradfordayers commented 3 years ago

@JamesMessinger can you take a look at https://github.com/APIDevTools/swagger-parser/pull/165 to see if that can get committed? This PR https://github.com/APIDevTools/swagger-parser/pull/166 is dependent on that as well as a review.

abdulgit2021 commented 3 years ago

@JamesMessinger can you please take a look at both #165 and this PR #166 and merge them if they are fine.. this helps fix a vulnerability.

philsturgeon commented 3 years ago

Merged #170 first.

kibertoad commented 3 years ago

@philsturgeon Any chance you could release a new version?

jonrober-80 commented 3 years ago

Is there an update on when a version containing this fix will be released? Like others, we are also tracking it as it fixes a vulnerability. @philsturgeon @JamesMessinger

philsturgeon commented 3 years ago

When #173 is unblocked through #178 or #175 or whichever PR makes the build pass we can release. Cannot release without tests passing.