override and update deps with security issues #1075
Conversation
🦋 Changeset detectedLatest commit: 72beed9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| typescript: 5.9.3 | ||
| esbuild: 0.25.4 | ||
|
|
||
| overrides: |
There was a problem hiding this comment.
Not a big fan of overrides...
Can we figure out if we depend on them at runtime?
There was a problem hiding this comment.
qs and body-parser are for express which is used by the express-dev wrapper and unlikely to ever be used by a consumer. brace-expansion is a deeply nested dependency of @node-minify/core which is used to minify JS in the server bundle.
These overrides are 1 patch version above the version that is currently depended on because those patches were released solely to resolve the security issue in their respective libraries.
| ], | ||
| "dependencies": { | ||
| "@ast-grep/napi": "^0.40.0", | ||
| "@aws-sdk/client-cloudfront": "3.398.0", |
There was a problem hiding this comment.
BTW, is this exact pinning really necessary? GHSA-6475-r3vj-m8vf
There was a problem hiding this comment.
Aws has proven to be not very good at respecting semver, they had some big breaking changes at times (that broke thing for a few versions and was fixed after)
I think we should keep pinning, but just update the SDK and ensure that everything works, maybe in another PR
| "cookie": "^1.0.2", | ||
| "esbuild": "catalog:", | ||
| "express": "^5.1.0", | ||
| "express": "^5.2.1", |
There was a problem hiding this comment.
I think express should be moved to the dev dependencies.
And because it's only used in dev, we should not have to override any package for security reason (qs here) WDYT?
|
|
||
| overrides: | ||
| "qs@6.14.0": ^6.14.1 | ||
| "brace-expansion@2.0.1": ^2.0.2 |
There was a problem hiding this comment.
brace-expansion is a deeply nested dependency of @node-minify/core which is used to minify JS in the server bundle.
IIUC correctly, this is used to minify JSON only. I don't think there is much to minimize in JSON files.
So what we could do instead is to drop the dep and use parse followed by stringify(o, null, 0).
@conico974 anything I miss here?
| overrides: | ||
| "qs@6.14.0": ^6.14.1 | ||
| "brace-expansion@2.0.1": ^2.0.2 | ||
| "nanoid@3.3.7": ^3.3.8 |
There was a problem hiding this comment.
-> % pnpm -F @opennextjs/aws why nanoid
Legend: production dependency, optional only, dev only
@opennextjs/aws@3.9.11 /Users/vberchet/code/work/aws/packages/open-next
dependencies:
next 16.0.10
└─┬ postcss 8.4.31
└── nanoid 3.3.7
I would say that no override is necessary here.
postcss is build time.
IMO the best thing to do here is ask Next to update postcss if not done in their latest version?
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Changes look good to me, beside the overrides.
I have several concern with overrides:
- Even if the bump is small, we end up with non tested code. I have verified quite a few times that whatever is non tested doesn't work
- If we override 9 with 10 but the original package update to 11 then we will still be pinned at 10
So I think we should strive to avoid overrides unless there is a critical reason not to. I don't think there is in this (see my inline comments)
|
No worries, I'll close this if there isn't much appetite for it. For context, at work we have a policy of removing issues flagged by security tools from our apps wherever possible (we don't use OpenNext, but if we did, these would be flagged by one of our tools and we'd be asked to do something about them), so it's common to override deeply nested dependencies if there's a patched non-breaking version.
I don't think this would be the case because the override is only for applying to a specific version of the dependency - if they upgrade, the override wouldn't apply anymore as the version is different to the one it's set to apply to. |
Oh right, I missed that |
4 participants
This PR bumps / overrides certain deps to address the following security issues flagged in a Snyk scan.
It also bumps the pnpm version used in the pipelines so that catalog overrides are respected.