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

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

Conversation

nvenegas
Copy link

@nvenegas nvenegas commented May 5, 2021

WIP because the earliest version of z-schema that would have the required bump to validator is 5.0.1. See zaggino/z-schema#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
Copy link
Author

nvenegas commented May 5, 2021

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
Copy link
Author

nvenegas commented May 5, 2021

@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
Copy link
Member

@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.

WIP because the earliest version of z-schema that would have the required
bump to validator is 5.0.1. See zaggino/z-schema#265
@nvenegas nvenegas force-pushed the nvenegas/z-schema-validator-redos-fix branch from e5c45ad to 3fde413 Compare May 11, 2021 23:20
@nvenegas nvenegas changed the title WIP fix: Bump z-schema 4.2.3 → 5.0.0 for ReDoS fix in validator fix: Bump z-schema 4.2.3 → 5.0.1 for ReDoS fix in validator May 11, 2021
@nvenegas
Copy link
Author

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 nvenegas marked this pull request as ready for review May 11, 2021 23:23
@nvenegas
Copy link
Author

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

@JamesMessinger Could you please review this?

@nvenegas
Copy link
Author

nvenegas commented Jun 4, 2021

@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
Copy link

@JamesMessinger can you take a look at #165 to see if that can get committed? This PR #166 is dependent on that as well as a review.

@abdulgit2021
Copy link

@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
Copy link
Member

Merged #170 first.

@kibertoad
Copy link
Contributor

@philsturgeon Any chance you could release a new version?

@jonrober-80
Copy link

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
Copy link
Member

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

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.

8 participants