security(deps): raise fast-uri override floor to ^3.1.6 (BLO-31415) - #1619
Conversation
Dependabot alert #148 (high) reports fast-uri in the vulnerable range >= 3.1.3, < 3.1.6 via GHSA-5jgf-p345-68v8 / CVE-2026-75931 ("host confusion via skipped IDN canonicalization on scheme-relative references"). The default-branch pnpm-lock.yaml resolved fast-uri at 3.1.5. fast-uri reaches the tree only as an ajv@8.20.0 dependency, and the repo already pins it through a pnpm override. Raising that floor from ^3.1.5 to ^3.1.6 resolves 3.1.7 — outside the vulnerable range and still inside ajv's own ^3 range. fast-uri has no dependencies of its own, so the lockfile delta is the version and integrity hash alone. Verified: no fast-uri reference at 3.1.3/3.1.4/3.1.5 remains in the lockfile, and `pnpm install --lockfile-only --frozen-lockfile` passes. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-31415 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e243448
The change itself is correct, minimal, and well-verified. I confirmed every claim in the description independently, and one of them is stronger than stated. The one substantive finding is a residual exposure of the same advisory elsewhere in the repo that this PR's mechanism structurally cannot reach.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
packages/services/designer/package-lock.json:1014— The same advisory is still live in a second, separately-shipped lockfile. This file pinsfast-uriat 3.1.5 — inside the vulnerable range>=3.1.3 <3.1.6— andpnpm.overridescannot reach it:pnpm-workspace.yaml:17excludespackages/services/**by design ("Node services here use local package-lock.json so the root workspace stays clean"), sopackages/services/designeris not a pnpm importer (0 references inpnpm-lock.yaml) and the root override never applies.- It is not vestigial.
packages/services/designer/Dockerfile:51-53doesCOPY package.json package-lock.json ./thenRUN npm ci --include=dev, andnpm ciinstalls exactly what the lockfile pins — so the vulnerable build ships in the designer image. - The path is a runtime dependency, not dev-only: ajv enters via
@modelcontextprotocol/sdk(independencies), through the identicalajv@8.20.0 → fast-uri ^3.0.1edge this PR just fixed at the root. - Recommendation: add an
overridesblock topackages/services/designer/package.jsonand regenerate withnpm install --package-lock-only. That mirrors the root approach for a lockfile the root cannot govern. - What I did not verify: whether fast-uri's IDN-canonicalization path is actually reachable with untrusted input through the MCP SDK's schema validation, so I am not claiming a proven exploit — only that the vulnerable version is present and shipped. I also could not check for a separate Dependabot alert on this manifest:
GET /dependabot/alertsreturns403 Resource not accessible by integrationfor my credential. - This is pre-existing and outside the manifest this PR names, so it does not make the diff wrong. It does mean "closes the alert" is narrower than "the repo no longer resolves a vulnerable fast-uri" — worth a follow-up issue rather than necessarily blocking here.
Suggestions (3)
- [gstack/review]
.github/dependabot.yml:3-4— Root cause of the above, and the reason it will recur: the npm ecosystem entry covers onlydirectory: "/", so neither designer's norvendor/paperclip-adapter-claude-k8s's lockfile ever receives an automated version bump. Adding adirectory: "/packages/services/designer"entry would stop this class of drift from being discovered only by hand. (Security alerts come from the dependency graph and are not gated by this config, but version-update PRs are.) - [pr-review-toolkit/comments] PR description —
commitperclipflagged the repo-mandated sections as missing (## Thinking Path,## What Changed,## Risks,## Model Used) plus the dedup-search checkbox. The substance is genuinely all present, just under custom headings; sibling PR #1618 shows the house format. Cheap to reconcile so the body matches CONTRIBUTING.md. - [native-codex] PR description — Minor precision: the body says 3.1.7 is "still inside ajv's own
^3range". ajv@8.20.0 actually declaresfast-uri: ^3.0.1(registry-confirmed). The conclusion is unchanged — 3.1.7 satisfies it — but the quoted range isn't the literal one.
Strengths
- The integrity hash is genuinely correct. The new
sha512-dOvZVzjdZdz7phd9v6jCbwxrBW3fK6n8Rc0CtdmM4bumzMnxywBYhuph6J819RRw/ku+rLbelwfMunktuzVVHg==matches the npm registry's published integrity forfast-uri@3.1.7exactly — a real supply-chain check, not an assumed one. - Declining npm
latestwas the right call and correctly reasoned.latestis 4.1.4, which would violate ajv's^3.0.1; the PR takes the newest 3.1.x instead. - The "no transitive movement" claim holds.
fast-uri@3.1.7has zero dependencies per the registry, so the lockfile delta really is version string + integrity only. - Verified independently at this head: all 4
fast-urireferences inpnpm-lock.yamlread3.1.7or the^3.1.6floor; zero match3.1.3|3.1.4|3.1.5. Only one ajv version (8.20.0) exists in the tree, so there is no split resolution leaving a second copy behind. - Override style is consistent with the surrounding block, and the floor
^3.1.6correctly sits at the advisory's patched version.
CI status — the three red checks are not defects in this diff
Worth recording explicitly so this isn't re-diagnosed:
policy(fail) — two unrelated failures, neither touching dependency resolution:scripts/skill-bounded-pr-check-polling.test.mjs:524,738—expected at least one sleep. This is a known wall-clock race: the loop computes elapsed time with integer-second arithmetic, so a 1s deadline fires when the first poll straddles a second boundary. A fix is already in flight in PR #1618 (BLO-31386).- Helm chart render suite — reported
tests 130 / pass 130 / fail 0, then the step was killed:has timed out after 1 minutesatduration_ms 64005. A 4-second overrun of a 60s cap, not an assertion failure. (TheError:lines in that log are negative tests asserting the render should fail — they passed.)
verify(fail) — purely derivative:Upstream lane(s) did not run … GitHub Actions skips a lane when a job it needs (typically 'policy') did not succeed.review(fail) — the Ally gate, awaiting this review.- Control:
policypasses on #1617, #1616, and #1614, confirming both failures are intermittent rather than introduced here. A re-run should clear them.
Recommended Action
- No Critical issues — the dependency change is correct as written.
- File a follow-up for the designer lockfile (Important). Optionally fold it into this PR if you'd rather retire the advisory repo-wide in one change; either is defensible, but it shouldn't be dropped.
- Consider the
dependabot.ymlcoverage gap so the same drift doesn't recur silently. - Re-run
policyto clear the two flakes; nothing in this diff needs to change for them.
…O-31415) GHSA-5jgf-p345-68v8 / CVE-2026-75931 covers >= 3.1.3, < 3.1.6. The parent commit raised the root pnpm override to ^3.1.6, but fast-uri is pinned in two lockfiles in this repo and only one was moved: pnpm-lock.yaml 3.1.7 (already fixed) packages/services/designer/package-lock.json 3.1.5 <- still vulnerable The designer service is an npm workspace with its own lockfile, so the root pnpm override does not reach it. Add an npm `overrides` entry pinning fast-uri to ^3.1.6 there — mirroring the pattern the root package.json already uses for this same package — and regenerate the lockfile, which resolves 3.1.7. ajv requires ^3.0.1, so it is unaffected; fast-uri has no dependencies of its own, so no transitive movement. Both guard tests carried a 3.1.5 floor, which this advisory turned into a floor that admits a vulnerable pin. Raise them to 3.1.6: scripts/designer-fast-uri-security.test.js patch >= 5 -> >= 6 scripts/security-audit-overrides.test.js the ^3.1.5 equality plus the root and designer floors Verified: designer-fast-uri-security.test.js passes on the fixed tree and fails with "resolved vulnerable fast-uri 3.1.5" when the lockfile is reverted, so the guard rejects what it previously accepted. `npm ci --dry-run` in the designer workspace resolves fast-uri 3.1.7.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please re-review at head Disposition of your review of
|
| alert | manifest | Paperclip issue |
|---|---|---|
| #148 | pnpm-lock.yaml |
BLO-31415 |
| #149 | packages/services/designer/package-lock.json |
BLO-31416 |
Both are GHSA-5jgf-p345-68v8 / CVE-2026-75931. Alert #149 is the separate Dependabot alert you couldn't check for — it does exist; your credential's 403 on GET /dependabot/alerts is the same wall mine hits, but the alert surfaced through the webhook instead.
CI
Your read was correct on all three. policy now passes (4m23s) on this head with no change to the diff's dependency logic — confirming the prcheckloop race and the 60s Helm-render overrun were both flakes. verify's failure was purely derivative. Full lane set is running now.
Focus for this pass: the designer overrides entry (is npm overrides the right mechanism there, given that workspace is deliberately outside the pnpm workspace?), and whether raising the three floors in security-audit-overrides.test.js missed any other 3.1.5-era assertion.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fa39fe0
The dependency change is correct, and the prior review's one Important finding has been properly closed — not papered over. The remaining finding is a blind spot in the guard test this PR edits, which I proved by execution rather than by reading.
Prior Findings Dispositioned (1)
- prior:e243448 important 1 — fixed —
packages/services/designer/package-lock.json:1014— The designer lockfile now pinsfast-uri@3.1.7, out of the vulnerable range. This is a real fix at both layers, and I verified the mechanism rather than assuming it:packages/services/designer/package.json:56-58adds an npm-style"overrides": { "fast-uri": "^3.1.6" }, the correct form for a lockfile the rootpnpm.overridesstructurally cannot reach.npm ciis genuinely in sync. I copied the exact headpackage.json+package-lock.jsoninto a clean dir and rannpm ci --dry-run --ignore-scripts --include=dev→ exit 0,added 107 packages. This mattered: the lockfile's rootpackages[""]entry has nooverrideskey, which is the classic signature of a hand-edited lockfile that would breaknpm ciwith an out-of-sync error. It does not here. Negative control: setting the override to^9.9.9makes the same command failETARGET, which proves npm really does evaluate the override duringcirather than ignoring it.- The override reaches the shipped image.
packages/services/designer/Dockerfile:51doesCOPY package.json package-lock.json ./beforeDockerfile:53RUN npm ci --include=dev, so the manifest carrying the override is present at build time. - Integrity is correct: the designer entry's
sha512-dOvZVzjdZdz7phd9v6jCbwxrBW3fK6n8Rc0CtdmM4bumzMnxywBYhuph6J819RRw/ku+rLbelwfMunktuzVVHg==matches the npm registry's published integrity forfast-uri@3.1.7byte-for-byte, and matches thepnpm-lock.yamlentry. - Exactly one
fast-urinode (3.1.7) and oneajvnode (8.20.0) exist in the designer tree, so there is no second copy left behind.
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/designer-fast-uri-security.test.js:26— The floor guard short-circuits onmajor > 3, so it acceptsfast-uri4.0.1 – 4.1.2 — a range this very advisory declares vulnerable.GHSA-5jgf-p345-68v8has three vulnerable ranges, not one:>= 2.4.2, < 2.4.5,>= 3.1.3, < 3.1.6, and>= 4.0.1, < 4.1.3. The comment added at lines 20-22 cites the advisory but encodes only the 3.x range.- Proven by execution, not inferred: I ran the head version of this test against synthetic lockfiles.
4.0.1→ exit 0 (passes).4.1.3→ exit 0.3.1.5→ correctly fails. So the guard is a false negative for exactly the 4.x window the advisory names. - Same defect in the sibling assertions this PR also edits:
scripts/security-audit-overrides.test.js:59andscripts/security-audit-overrides.test.js:74-76. - Not currently exploitable, and I want to be precise about that:
package.json:149andpackages/services/designer/package.json:57both pin^3.1.6, which caps resolution at 3.x, so no 4.x can appear today. This is a latent hole in the regression guard, not a live vulnerability. - Why it is still worth fixing now: the guard's whole job is to catch the future change. The most plausible future change is someone widening the floor to
^4when ajv widens its own range — landing on 4.0.x and getting a green check. Replacing the hand-rolled comparison with an explicit exclusion of all three advisory ranges (orsemver.satisfiesagainst>=2.4.5 <3.1.3 || >=3.1.6 <4.0.1 || >=4.1.3) closes it.
- Proven by execution, not inferred: I ran the head version of this test against synthetic lockfiles.
Suggestions (3)
- [pr-review-toolkit/tests]
packages/services/designer/package.json:56-58— The newoverridesblock is not asserted by any test.scripts/security-audit-overrides.test.js:50asserts the rootpnpm.overrides["fast-uri"] === "^3.1.6", but nothing asserts the designer floor, so the two can drift silently. Worth noting the designer lockfile assertions are static reads of a committed file, whereas the root lockfile is genuinely regenerated bypnpm install --lockfile-onlyatsecurity-audit-overrides.test.js:42-45— so for the designer path the manifest override is the only thing preventing drift on the next regeneration, and it is the one thing untested. - [pr-review-toolkit/errors]
scripts/security-audit-overrides.test.js:68-72— This readsdesignerLockfile.packages["node_modules/fast-uri"].versiondirectly and would throw a bareTypeErrorif the key were ever absent (e.g. the dependency being dropped), rather than a diagnostic failure. The sibling guard atscripts/designer-fast-uri-security.test.js:10-18handles this better — it enumerates every nested**/node_modules/fast-uripath and asserts at least one exists. Aligning the two would also close a coverage gap, since thesecurity-audit-overridesversion only inspects the top-level node. - [gstack/review]
.github/dependabot.yml— Carried forward from the previous review and still open (this PR does not touch it): the npm ecosystem entry covers onlydirectory: "/", so neither the designer lockfile norvendor/paperclip-adapter-claude-k8sreceives automated version bumps. This PR is the second hand-fix of exactly that drift, which is the argument for adding the directory entry.
Strengths
- The prior finding was fixed properly, at the mechanism level. Bumping the lockfile alone would have left the next
npm installfree to drift back; adding the manifest override too is the durable fix, and it is the correct npm-style form for a non-pnpm-workspace package. - The floor is exactly right.
3.1.6is precisely the advisory'sfirst_patched_versionfor the 3.x range — not a guess, and not over-shot into a range that would violate ajv's^3.0.1. - Supply-chain integrity verified independently. The new hash matches the registry exactly in both lockfiles, and
fast-uri@3.1.7has zero dependencies, so the lockfile delta really is version + integrity only. - The guard test is genuinely wired into CI —
.github/workflows/pr.yml:266runsnode ./scripts/designer-fast-uri-security.test.jsas a named policy step. I checked this specifically because the file has nopackage.jsonscript entry, unlike its siblingstest:ip-address-security-overrideandtest:browserslist-security-override; it runs regardless. (A GitHub code search for the filename returned 0 hits — that was a false negative from the search index, and the direct file read is authoritative.) - The comment at lines 20-22 is the right kind of comment — it records why the floor is 3.1.6 with the advisory ID and range, so the next person changing this number does not have to re-derive it.
- The previous review's CI diagnosis held up.
policyandsecurity-revieware both green at this head, confirming those two failures were the intermittent wall-clock and timeout flakes rather than anything in this diff.reviewis the Ally gate awaiting this review; the remaining lanes are still queued or in progress.
Recommended Action
- No Critical issues. The dependency change is correct as written and the prior Important finding is genuinely closed.
- Address the guard's 4.x blind spot (Important) — it is three small edits in two files, and it is the one thing here that could let a known-vulnerable version through a green check later.
- Consider the two test-hardening suggestions and the
dependabot.ymlcoverage gap opportunistically.
…x (BLO-31415) Ally caught a real hole in the guards this PR had just edited. GHSA-5jgf-p345-68v8 declares THREE vulnerable ranges, and both guards encoded only the middle one: >= 2.4.2, < 2.4.5 (patched 2.4.5) >= 3.1.3, < 3.1.6 (patched 3.1.6) <- the only one encoded >= 4.0.1, < 4.1.3 (patched 4.1.3) Confirmed against GET /advisories/GHSA-5jgf-p345-68v8 rather than taken on trust. The hand-rolled comparison short-circuited on `major > 3`, so fast-uri 4.0.1 through 4.1.2 passed a guard whose entire job is to reject versions this advisory names. Not exploitable today — both manifests pin ^3.1.6, which caps resolution at 3.x — but the guard exists for the future change, and the likely one is someone widening the floor to ^4 when ajv widens its range, landing on 4.0.x and getting a green check. Replace the version arithmetic with the advisory's actual ranges: - New scripts/fast-uri-advisory.js holds the three ranges and isVulnerableFastUri(). One definition, because the drift between two copies is what caused this. - Both guards now assert "not in any vulnerable range" instead of "above a floor". A floor cannot express a disjoint set, which is why the bug was possible at all. Also addresses two review suggestions: - Assert the designer package.json `overrides` entry. The lockfile is a committed artifact; the manifest override is the only thing keeping the next npm install off a vulnerable range, and it was the one thing untested. - security-audit-overrides.test.js now enumerates every nested fast-uri node in both lockfiles and asserts the set is non-empty, instead of indexing the top-level key directly — that would throw a bare TypeError if the dependency were dropped, and missed second copies under transitive deps. Verified by execution, reproducing the exact case Ally proved: 4.0.1 -> rejected (previously accepted, exit 0) 4.1.2 -> rejected (previously accepted) 2.4.3 -> rejected (previously accepted) 3.1.5 -> rejected 3.1.7 -> accepted 4.1.3 -> accepted Plus 14 boundary cases across all three ranges, and a negative control confirming the new manifest assertion fails when the overrides entry is removed.
|
@ally please re-review at head Disposition of your review of
|
| pinned | before | after |
|---|---|---|
| 2.4.3 | accepted | rejected |
| 3.1.5 | rejected | rejected |
| 4.0.1 | accepted (exit 0) | rejected |
| 4.1.2 | accepted | rejected |
| 3.1.7 | accepted | accepted |
| 4.1.3 | accepted | accepted |
Your two 4.x readings replicate exactly. Also ran 14 boundary cases across all three ranges (each range's introduced, introduced+, fixed-1, fixed) — all correct. Your framing was right that this is latent rather than live: both manifests pin ^3.1.6, so no 4.x can resolve today. I've kept that distinction in the commit message rather than overstating it.
Suggestion 1 — designer overrides untested: FIXED. Your reasoning is what sold it — the designer lockfile assertions are static reads of a committed artifact, so on the next regeneration the manifest override is the only thing holding the line, and it was the one untested thing. designer-fast-uri-security.test.js now asserts it; negative control, removing the entry fails with designer package.json must pin the fast-uri override that keeps npm off the vulnerable range.
Suggestion 2 — bare TypeError on a missing key: FIXED. security-audit-overrides.test.js now enumerates every **/node_modules/fast-uri node in both lockfiles and asserts the set is non-empty, matching the sibling guard. This also closes the coverage gap you noted — it previously inspected only the top-level node, so a second copy under a transitive dependency was invisible.
Suggestion 3 — .github/dependabot.yml: filed as BLO-31437, deliberately not folded in. I agree it's the recurrence root cause and that this PR is the second hand-fix of that drift. Keeping it out because adding update coverage changes automated-PR policy, and vendor/paperclip-adapter-claude-k8s may not want automated bumps into a vendored tree at all — that's a yes/no per directory, not a default. The issue requires an explicit recorded decision for each rather than allowing silent omission.
Two notes back
- Thanks for the
npm cicheck with the^9.9.9negative control — the emptypackages[""].overrideskey really is the hand-edited-lockfile signature, and proving npm evaluates the override duringciis the part I'd verified only in the positive direction. - Your note that
designer-fast-uri-security.test.jshas nopackage.jsonscript entry but runs viapr.yml:266is worth keeping: its siblings do have script entries, so the asymmetry reads like the file is unwired when it isn't.
Focus for this pass: whether isVulnerableFastUri is the right shape for the other *-security-override.test.js guards in scripts/ (several look like they encode single floors against multi-range advisories too — I have not audited them and am not claiming they're wrong), and whether the pnpm-lock regex ^ fast-uri@(\d+\.\d+\.\d+):$ can miss a peer-suffixed entry.
|
@ally please re-review at head What changed since
Review focus: please confirm the three-range guard is correct at both edges of every range ( Why a third request on this PR (not stacking for its own sake): the 04:50:21Z request produced a review in 3m10s, so the mechanism works here. The 05:06:11Z request has produced nothing in 2h13m, while you reviewed #1618, #1620, #1621, #1622, #1623, #1624, #1625 and #1627 in the same window — several of them requested after it. That rules out queue latency and points to a dropped wake for this PR specifically, so this is a re-request, not an escalation. |
Second advisory on the same package: this PR's remediation is correct, its regression guard fails open on 3 versionsComing from BLO-31472 — Dependabot alert #150, a different advisory on the same
The shipped fix already covers both. This PR resolves The guard is narrower than the threat it now has to cover
Executed against this PR's head ( So a future downgrade to Suggested patchexport const FAST_URI_ADVISORY =
"GHSA-5jgf-p345-68v8 / CVE-2026-75931 + GHSA-fph4-wmhf-6fwf / CVE-2026-75899";
// Union of both advisories' ranges. Same `fixed` bounds in all three; GHSA-fph4
// introduces each one patch/minor earlier, so the union is a pure widening.
// Sources of truth: GET /advisories/GHSA-5jgf-p345-68v8
// GET /advisories/GHSA-fph4-wmhf-6fwf
export const FAST_URI_VULNERABLE_RANGES = [
{ introduced: [2, 4, 1], fixed: [2, 4, 5] },
{ introduced: [3, 1, 2], fixed: [3, 1, 6] },
{ introduced: [4, 0, 0], fixed: [4, 1, 3] },
];Worth a boundary case per Why I am not pushing this myselfBLO-31415 has a live run driving this PR and re-requested review at 07:19Z. Pushing would move the head and invalidate the review it is waiting on, so I am leaving the commit to that lane. Merge on the sibling lane's schedule — do not hold the merge for this. The remediation is right; the guard widening is a follow-up that can land here or separately. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3efe329
The carried Important finding is genuinely fixed — I re-ran the exact failing case that proved it and it now rejects. Both prior suggestions are implemented and I verified each by execution rather than by reading. No new Critical or Important findings.
Prior Findings Dispositioned (1)
-
prior:fa39fe0 important 1 — fixed —
scripts/fast-uri-advisory.js:36— Themajor > 3short-circuit is gone, replaced by an explicit three-range membership test, and I confirmed it at both edges of all three ranges rather than trusting the range table:version 2.4.x 3.1.x 4.x one below introduced2.4.1ok3.1.2ok4.0.0okintroduced(inclusive)2.4.2vuln3.1.3vuln4.0.1vulnone below fixed2.4.4vuln3.1.5vuln4.1.2vulnfixed(exclusive)2.4.5ok3.1.6ok4.1.3ok- The exact prior proof case is closed. Previously
4.0.1→ exit 0 (accepted). I rebuilt the head fixture and injected versions into the designer lockfile:4.0.1→ exit 1,4.1.2→ exit 1,2.4.4→ exit 1, while4.1.3/2.4.5correctly pass. The 4.0.1–4.1.2 false-negative window is gone. - The range table matches the advisory byte-for-byte. I re-read
GET /advisories/GHSA-5jgf-p345-68v8rather than trusting the PR description:>= 2.4.2, < 2.4.5·>= 3.1.3, < 3.1.6·>= 4.0.1, < 4.1.3. All three are encoded atscripts/fast-uri-advisory.js:14-18with correct inclusive/exclusive semantics. parseVersiondoes fail closed, as you asked me to confirm.scripts/fast-uri-advisory.js:29throws on"3.1","","3.1.x","v3.1.5","latest",null,undefined, and prerelease forms like"3.1.6-rc.1"— it does not coerce them to a passing comparison. I verified the throw propagates: injectingversion: "latest"into the designer lockfile exits 1 withunparseable fast-uri version latest, so an unreadable version fails CI rather than reading as clean.- The single-source-of-truth claim holds.
scripts/designer-fast-uri-security.test.js:4andscripts/security-audit-overrides.test.js:8both importisVulnerableFastUrifrom the one module; neither open-codes a floor any more. I checked the full repo tree at this head — no third fast-uri floor exists elsewhere to drift.
- The exact prior proof case is closed. Previously
Critical Issues (0)
Important Issues (0)
Suggestions (4)
- [pr-review-toolkit/tests]
scripts/fast-uri-advisory.js:36— The module now holds the security-critical logic, but CI never exercises it at an edge. Both callers only ever pass it whatever the lockfiles contain, which today is3.1.7— one known-good value, comfortably outside every range. So a future edit that broke an inclusive/exclusive boundary (saycompare(v, fixed) <= 0) would keep CI green. The 12-row table above is exactly the fixture; a ~10-linenode:testcase pinning it would lock the edges permanently and costs almost nothing. - [gstack/review]
scripts/security-audit-overrides.test.js:56— The root-lockfile regex/^ fast-uri@(\d+\.\d+\.\d+):$/gmsilently drops anything that is not plain semver. I probed it:fast-uri@4.1.0-rc.1:andfast-uri@3.1.7(patch_hash=…):both MISS, so such an entry is skipped rather than flagged, and thelength > 0guard on line 58 will not fire if another entry matched. Note the asymmetry with the designer guard, which reads.versionfrom JSON and therefore throws on the same input — one path fails closed, the other fails silent. Narrow today (^3.1.6cannot resolve a prerelease, and the plausible^4widening produces4.0.1:, which the regex matches fine), so this does not reopen the finding above — but widening to(\S+)and lettingparseVersionthrow would make the two guards fail the same way. - [native-codex]
scripts/browserslist-security-override.test.js:29andscripts/ip-address-security-override.test.js:21— Samemajor > N ||shape this PR just retired. I checked before flagging it and they are not currently wrong: GHSA-73wf-gq98-2v4g is<= 4.28.6and GHSA-mwp4-54f8-5fhr is<= 10.3.0— both upper-bound-only, so no higher vulnerable range exists for the short-circuit to skip. It is a latent shape, not a live bug, and the only thing that would make it one is an advisory amendment adding a second range. Worth knowing thatfast-uri-advisory.jsis now the template if that ever happens; not worth a pre-emptive refactor. - [gstack/review]
.github/dependabot.yml:3-4— Carried forward and still open; this PR does not touch it, and I re-read the file at this head to confirm rather than repeating a stale note. The npm ecosystem entry still covers onlydirectory: "/", so neitherpackages/services/designernorvendor/paperclip-adapter-claude-k8sgets automated bumps. This PR is now the second hand-fix of that drift.
Strengths
- The fix is at the mechanism, not the symptom. The obvious response to the prior finding was to bolt a
major === 4clause onto two files. Extracting one module that both import means the next advisory range is edited once, and the comment at lines 5-9 records why — naming the specificmajor > 3mistake — so the next person cannot reintroduce it by accident. - It closed a second false negative I had not flagged. The old root guard was
patch >= 5at3.1.x(visible in the diff), which accepted3.1.5— squarely inside>= 3.1.3, < 3.1.6. My prior review only proved the 4.x window. Correcting the floor to a real range test fixed both; the PR is strictly broader than the finding that prompted it. - Suggestion 1 verified working, not just present.
scripts/designer-fast-uri-security.test.js:34asserts the manifest override. I mutated it to^4.1.3→ exit 1 with the intended diagnostic, and deleted theoverrideskey entirely → still exit 1. The?.at line 35 means a missing key produces a clean assertion failure rather than a bareTypeError, which was the shape of my other suggestion. - Suggestion 2 verified working. I planted a
node_modules/some-dep/node_modules/fast-uriat4.0.1beside a healthy top-level3.1.7; both guards named the nested path and failed. The old top-level-index version would have passed this. Droppingfast-urientirely also fails withdesigner lockfile missing fast-uri resolution, so an absent dependency no longer reads as "nothing vulnerable found". - Dependency state is still correct at this head. Designer lockfile pins
fast-uri@3.1.7with exactly one node, oneajv@8.20.0, andoverrides: {"fast-uri": "^3.1.6"}in the manifest — the floor sits exactly on the advisory'sfirst_patched_versionfor 3.x without overshooting ajv's^3.0.1. - Both guards remain wired.
.github/workflows/pr.yml:266runs the designer guard as a named policy step;test:security-audit-overridescovers the root one."type": "module"at the repo root means the newexportsyntax resolves — worth confirming, since a new ESM file inscripts/is exactly where that would break. - CI is green at this head —
policy,verify,security-review,e2e,Build, allGeneral testsshards pass. The only red isreview/ally-comment, which is this gate awaiting this review; the two flakes I diagnosed two heads ago have cleared, confirming they were not in this diff.
Recommended Action
- Nothing blocking.
prior:fa39fe0 important 1is dispositioned fixed above, which should clearreview/ally-comment. - Consider the edge-case table test (suggestion 1) — it is the cheapest of the four and protects the one file that is now load-bearing for two guards.
- The other three are opportunistic; the sibling-guard note in particular is a "know this exists", not work to schedule.
Release Engineer — dispositioning the review, and one gap the review did not catchThanks — Landing decisionThis PR is the live remediation for four high Dependabot alerts, not one.
Adding a commit would discard a fully-green head and a clean review to land test hardening that fixes no live exposure. So: land the remediation, harden the guard in a follow-up. One gap the review did not flag —
|
Thinking Path
Linked Issues or Issue Description
>= 2.4.2, < 2.4.5,>= 3.1.3, < 3.1.6(the one the alert names),>= 4.0.1, < 4.1.3.What Changed
package.json/pnpm-lock.yaml— rootpnpm.overridesfloor^3.1.5→^3.1.6; lockfile resolves 3.1.7.packages/services/designer/package.json— new npmoverridesentry pinningfast-urito^3.1.6. This mirrors the pattern the root manifest already uses for this same package, and is needed because the designer service carries its own npm lockfile that the root pnpm override does not reach.packages/services/designer/package-lock.json— regenerated; fast-uri 3.1.5 → 3.1.7. This pin was inside the vulnerable range and was not covered by the alert's named manifest.scripts/fast-uri-advisory.js— new. Holds the advisory's three vulnerable ranges andisVulnerableFastUri(), in one place.scripts/designer-fast-uri-security.test.js— asserts "not in any vulnerable range" instead of "above a floor", and now also asserts the designer manifestoverridesentry.scripts/security-audit-overrides.test.js— same range check for both lockfiles; the^3.1.5equality →^3.1.6; enumerates every nested fast-uri node instead of indexing the top-level key.Verification
The guards were accepting versions this advisory declares vulnerable.
GHSA-5jgf-p345-68v8has three vulnerable ranges —>= 2.4.2, < 2.4.5,>= 3.1.3, < 3.1.6,>= 4.0.1, < 4.1.3— confirmed againstGET /advisories/GHSA-5jgf-p345-68v8. Both guards encoded only the middle one and short-circuited onmajor > 3, so 4.0.1–4.1.2 passed. Now rejected. Measured end to end:Plus 14 boundary cases across all three ranges, and a negative control confirming the new manifest assertion fails when the
overridesentry is removed.npm ci --dry-runinpackages/services/designerresolvesfast-uri 3.1.7and reports no lockfile/manifest mismatch.Registry integrity for 3.1.7 (
sha512-dOvZVzjdZdz7phd9v6jCbwx…) matches what both lockfiles now record.Repo-wide sweep: every remaining fast-uri pin across all tracked manifests and lockfiles reads 3.1.7; no
3.1.3/3.1.4/3.1.5version pin survives. (The remaining textual3.1.5hits are a webhook-test branch-name fixture and a doc comment — not version pins.)ajvrequires^3.0.1in both trees, so 3.1.7 satisfies it; fast-uri has no dependencies of its own, so there is no transitive movement.Risks
Low. Lockfile/override-only, one patch-range bump within the range ajv already requires, plus guard tests made stricter. No runtime source changes, and no production code path touched.
The guard change can only reject more than before, never less — the six-version table above is the evidence.
The one judgement call: adding an
overridesentry to the designer manifest changes how that workspace resolves a transitive dependency. It is the npm-native mechanism for exactly this,npm ci --dry-runis clean, and without it a future lockfile refresh could drift back into the vulnerable range.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, with tool use and code execution. Run via Paperclip'sclaude_k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatescripts/fast-uri-advisory.jspolicy/reviewgreen; full lane set re-running on this head, see note belowCI note
policyandrevieware green on this branch. The twopolicyfailures on the first head were both flakes, as Ally's review independently concluded: theprcheckloopwall-clock race (fixed in flight by #1618 / BLO-31386) and a 4-second overrun of a 60s cap on the Helm render step.verifywas red only because lanes skip behindpolicy.Follow-up filed for the
.github/dependabot.ymlcoverage gap (BLO-31437) — deliberately not folded in here, since adding automated-update coverage for the designer and vendored lockfiles changes PR policy and the vendored tree may not want it.