chore(deps): refresh fast-uri, qs and browserslist to clear npm audit - #2249
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Correct the security assessment that inaccurately describes qs as dev-only.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refreshes transitive lockfile resolutions to patched versions without manifest changes.
Changes:
- Updates root
fast-uriandqs. - Updates TUI
browserslistand related data packages. - Incorrectly characterizes production-reachable
qsas dev-only.
File summaries
| File | Description |
|---|---|
package-lock.json |
Refreshes patched dependencies; the qs reachability assessment needs correction. |
clients/tui/package-lock.json |
Refreshes browserslist and its transitives. |
Review details
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- clients/tui/package-lock.json: Generated file
- Files reviewed: 0/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Closes #2244 Closes #2225 All three outstanding advisories were stale lockfile resolutions, not upward-blocked pins: every fixed version already sits inside the range its declaring parent asks for, so refreshing the lock entry is the whole fix. fast-uri 3.1.5 -> 3.1.7 ajv@8.18.0 asks for ^3.0.1 (root) qs 6.15.3 -> 6.16.0 express@5.2.1 asks for ^6.14.0 (root) browserslist 4.28.2 -> 4.28.9 via @babel/core (clients/tui) No `overrides` entry is added, and no manifest changes. AGENTS.md's rule is that a transitive is pinned with `overrides` rather than with `npm audit fix` -- it does not call for a pin where none is needed. A permanent pin here would buy nothing and would later hold a package back: `fast-uri: ^3.1.6` forbids fast-uri 4.x for as long as it stands, including after ajv moves to it. The monthly refresh sweep and the daily alert sweep are what catch a regression. `npm audit` is clean in all five installs (root, web, cli, tui, launcher). Reachability, assessed rather than assumed: - fast-uri is the one that ships. ajv is a root runtime dependency, and Vite pre-bundles it into the published `clients/web/dist`, so the vulnerable code was inlined into the SPA rather than merely resolved at install time. Its input is attacker-influenced: `schemaUtils.ts` compiles the `outputSchema` a server under test supplies, and ajv resolves that schema's `$id`/`$ref` through fast-uri (`ajv/dist/runtime/uri.js`). Impact is bounded well below the advisory headlines, though: both SSRF advisories need a consumer that fetches the parsed URI, and ajv never performs a network request. The realistic worst case is the host-confusion pair mis-normalizing a crafted `$id`, giving a wrong or failed validation of one tool's output. - qs is installed in production, but nothing shipped runs it. express is a root devDependency, but that is not its only path: `npm ls express --omit=dev` shows it reaching a production install through @modelcontextprotocol/server-legacy@2.0.0 (a root runtime dependency, also via express-rate-limit) and through @modelcontextprotocol/ext-apps -> @modelcontextprotocol/sdk@1.30.0. So qs is present in every user install. What holds is that no shipped module ever instantiates it: nothing in `core/`, `clients/*/src` or `clients/web/server` calls `express()`, the web backend is Hono, and server-legacy is imported only from `test-servers/src`, which the root `files` list does not publish. Both advisories need express to parse an attacker-supplied query string, which requires a live express app. - browserslist is dev tooling. It arrives in the clients/tui install through eslint-plugin-react-hooks -> @babel/core, is reached only by lint, and is in no published bundle. The root install already resolved a patched 4.28.8 from the same plugin at the same version, which is what identified tui's copy as a stale lock rather than a constrained one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YahVxMTGpigLbZBh1JGPDr Signed-off-by: cliffhall <cliff@futurescale.com>
9fec0f4 to
1018d81
Compare
Response to Copilot review round 1One finding, no inline comments. Copilot is right and I was wrong — fixed in ✅ "Correct the security assessment that inaccurately describes
|
There was a problem hiding this comment.
🟢 Approval recommended
All reviewed lockfile updates are compatible, and no unresolved issues remain.
Review details
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- clients/tui/package-lock.json: Generated file
- Files reviewed: 0/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The lockfile-only updates address the advisories with no unresolved issues.
Review details
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- clients/tui/package-lock.json: Generated file
- Files reviewed: 0/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
Both lockfile updates are verified, and no unresolved issues remain.
Review details
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- clients/tui/package-lock.json: Generated file
- Files reviewed: 0/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Closes #2244
Closes #2225
All three outstanding advisories turned out to be stale lockfile resolutions, not upward-blocked pins. Every fixed version already sits inside the range its declaring parent asks for, so refreshing the lock entry is the whole fix:
fast-uriajv@8.18.0→^3.0.1qsexpress@5.2.1→^6.14.0browserslistclients/tui@babel/core→ (unconstrained above 4.28.7)Two lockfiles changed. No manifest changed, nothing moved between manifests, and no client re-declares a root-owned package.
Why no
overridesentryBoth #2244 and #2225 proposed an
overridespin, andAGENTS.mddoes say to pin a transitive withoverridesrather than withnpm audit fix. That rule is about how to pin when a pin is needed — it does not call for one where the declared range already admits the fix.A pin here would buy nothing and would cost something later:
"fast-uri": "^3.1.6"forbidsfast-uri@4.xfor as long as it stands, including afterajvmoves to it, and an override is applied by npm with no upward-bound check and no signal when it becomes obsolete. The three lock entries were refreshed with targetednpm update <pkg>calls rather thannpm audit fix, so nothing was silently downgraded — the diff is the fixed versions plus their in-family transitives (caniuse-lite,electron-to-chromium,node-releases,update-browserslist-db,baseline-browser-mapping) and nothing else.Regression cover is the two sweeps that just landed in this milestone (#2232, #2239, #2243) plus the release-time
npm audit --audit-level=highreport. npm never downgrades an existing lock entry, so the refreshed resolutions hold across an ordinarynpm install.Reachability — assessed, not assumed
The issue asked for this to be established rather than taken from the advisory headline.
fast-uriis the one that ships, and its impact is well below its headline.ajvis a root runtime dependency, and Vite pre-bundles it into the publishedclients/web/dist(getViteDevOptimizeDeps().includenames bothajvand@modelcontextprotocol/client/validators/ajv), so the vulnerable code was inlined into the shipped SPA, not merely resolved at install time. The input is attacker-influenced:schemaUtils.tscompiles theoutputSchemaa server under test supplies, and ajv resolves that schema's$id/$refthrough fast-uri (ajv/dist/runtime/uri.js). But both SSRF advisories (GHSA-f65p-4m7j-42xc, GHSA-fph4-wmhf-6fwf) require a consumer that fetches the parsed URI, and ajv never performs a network request. The realistic worst case is the host-confusion pair (GHSA-5jgf-p345-68v8, GHSA-jqff-g426-hqxp) mis-normalizing a crafted$id, yielding a wrong or failed validation of one tool's output. Real, worth fixing before the release build, not user-facing SSRF.qsis installed in production, but nothing shipped runs it. (Corrected during review — my first pass called this dev-only, and that was wrong.)expressis a rootdevDependency, but that is not its only path into the tree.npm ls express --omit=devshows it reaching a production install two other ways:@modelcontextprotocol/server-legacyis a root runtime dependency, soqsis present in every user install — a real supply-chain footprint, not nothing. What does hold is that no shipped code ever instantiates it: nothing incore/,clients/*/srcorclients/web/servercallsexpress(), the web backend is Hono, andserver-legacyis imported only fromtest-servers/src, which the rootfileslist does not publish. Both advisories (GHSA-x5fp-wj9c-mxmx, GHSA-4mjr-xmp4-gh2g) require express to parse an attacker-supplied query string, which needs a live express app. So: in the tree, not on any executed path.browserslistis dev tooling, as #2225 established: it arrives in theclients/tuiinstall througheslint-plugin-react-hooks→@babel/core, is reached only by lint, and is in no published bundle. What identified it as a stale lock rather than a constrained one is that the root install already resolved a patched4.28.8from the same plugin at the same version — same range, newer resolution — so no pin was ever required, only a refresh.Verification
npm auditis clean in all five installs, both with and without dev dependencies (10/10):npm run formatclean;npm run local:gatepasses.No UI change, so no screenshots.
On #2225
#2244 is a superset of #2225 — same
browserslistfinding, same tui install, same chain. This PR satisfies every box in #2225's "Done when" (dev audit clean in all five,--omit=devclean in all five, gate passes); it just reaches the last one without theoverridesentry that issue proposed, for the reason above. Closing both.🤖 Generated with Claude Code
https://claude.ai/code/session_01YahVxMTGpigLbZBh1JGPDr