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(deps): override body-parser and bump express COMPASS-8288) #6226

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

mabaasit
Copy link
Contributor

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the fix label Sep 11, 2024
@mabaasit mabaasit added the no release notes Fix or feature not for release notes label Sep 11, 2024
package.json Outdated
@@ -95,6 +95,7 @@
"scripts"
],
"overrides": {
"mongodb-client-encryption": "^6.1.0"
"mongodb-client-encryption": "^6.1.0",
"body-parser": "^1.20.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fwiw, in order to bump a transitive dependency, you don't usually need to put the nested dependency in overrides, unless the intermediate dependency (express in this case, I guess) has a dependency specification that conflicts with the new version.

It should be enough here to update our package-lock.json, i.e. you can remove this line, run npm install and the package-lock.json should still point to 1.20.3 without issues. That's going to simplify things a bit once our dependencies start depending on body-parser 2.x, for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think it should be enough to bump express in this case and update package-lock with install, express has an exact version specified for body-parser and express version change should've bumped it in package lock too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this PR is to address the vulnerability in body-parser 1.20.2 (added COMPASS-8288).

With bump in express, the package lock does update it correctly for compass-web package. But we also have express in compass-oidc and it resolves to body-parser version 1.20.2.

Running npm install with npm update express correctly updates the deps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool – opened mongodb-js/mongosh#2166 for the mongosh equivalent 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aaaaah, finally got it, I was assuming this whole time that it's only due to express here having the "older" version than the one in oidc-plugin that the error is being triggered. Thanks for clarifying!

@addaleax
Copy link
Collaborator

Oh, also: Is this to fix some specific issue? If yes, we'll probably need the same for mongosh

@mcasimir
Copy link
Collaborator

mcasimir commented Sep 11, 2024

Can we add a ticket link on the PR and some more context on the description, so that whoever will have to deal with this in future can get some context from the git history / blame at least.

@mabaasit mabaasit changed the title fix(deps): override body-parser and bump express fix(deps): override body-parser and bump express COMPASS-8288) Sep 11, 2024
@mabaasit mabaasit merged commit b75d06a into main Sep 11, 2024
19 of 26 checks passed
@mabaasit mabaasit deleted the bump-express branch September 11, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants