fix(deps): resolve brace-expansion advisory — previous override pinned a still-vulnerable version - #331
Conversation
…d a still-vulnerable version GHSA-mh99-v99m-4gvg (CVE-2026-14257, high) affects brace-expansion <=5.0.7; 5.0.8 is the only patched release. The override added in #325 pinned brace-expansion to ^2.1.2, which is inside the affected range, so `bun audit` stayed red on main and blocked every open PR. Two changes: - Drop the unused `glob` dependency from @tpsdev-ai/pi-tps-mail. glob was declared in the package's initial commit (#271) and is imported nowhere in the repo; it was the sole path to brace-expansion, so removing it removes the vulnerable subtree at the root. - Remove the `brace-expansion` entry from the root overrides block entirely rather than re-pinning it to ^5.0.8. Bumping the override was considered and rejected. brace-expansion 3.x replaced the CJS default export with a named `expand` export, while glob@10 -> minimatch@9 still default-imports it, so a forced 5.x resolution throws "(0 , brace_expansion_1.default) is not a function" on any brace pattern while non-brace patterns keep working. Carrying that override would convert a future loud, blocking audit failure into a quiet correctness bug that ships. A red audit cannot be ignored; a silent runtime break can. The ws, shell-quote and js-yaml overrides are untouched — those clear live advisories on packages actually present in the tree. bun audit: 1 high -> 0 vulnerabilities. Dependency tree 131 -> 99 packages. Tests unchanged vs main: 1113 pass, 5 fail (all 5 pre-existing on main and environmental), 3 skip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
523eace to
d64b00b
Compare
tps-kern
left a comment
There was a problem hiding this comment.
Architectural review — APPROVED.
The previous fix was not a fix. PR #325 pinned brace-expansion to ^2.1.2, which is inside the vulnerable range (<=5.0.7). The override looked resolved for weeks but the audit stayed red — the only thing that caught it. This is the key finding and it's worth a postmortem: a forced resolution that pins to a version still inside the advisory range is a silent non-fix. The audit being a required CI gate is what surfaced it.
The fix is correct — remove the root cause, not patch the symptom.
glob was declared in pi-tps-mail's initial commit and imported nowhere. Removing it drops brace-expansion from the tree entirely (131 → 99 packages). The lockfile confirms 0 occurrences after a clean install. Test counts are byte-identical to main baseline (1113 pass, 5 fail, 3 skip — all 5 pre-existing and environmental). If glob were actually imported, the build or tests would break. They don't.
Judgment call 1 — don't bump to ^5.0.8:
Correct. brace-expansion 3.x dropped the CJS default export. minimatch@9 default-imports it and calls .default(pattern). With 5.0.8, non-brace patterns work but brace patterns throw TypeError at runtime — a silent, input-dependent break strictly worse than the advisory. The PR body provides concrete reproduction evidence. Bumping would trade a loud blocking audit for a quiet correctness bug.
Judgment call 2 — remove the override rather than keep a correct floor:
Approve. With glob gone, a ^5.0.8 override is inert today but dangerous tomorrow: if a future dependency pulls in a minimatch@9-shaped consumer, the override converts a loud blocking audit failure into the quiet runtime TypeError demonstrated above. A red audit is unignorable; a silent break on brace patterns is not. Removing the override favors loud failure over silent failure — the right direction for a security-adjacent decision.
The three remaining overrides (ws: ^8.21.0, shell-quote: ^1.9.0, js-yaml: ^4.3.0) are preserved and still effective.
CI: 11/11 green, including Dependency Audit (which was red on main). This unblocks every open CLI PR.
Ship it.
tps-sherlock
left a comment
There was a problem hiding this comment.
Security Review: cli#331 — brace-expansion advisory: removal, not bump 🔥
Verdict: APPROVE. Both judgment calls are correct. The prior "fix" was a false resolution, and the removal is the right posture.
The prior fix that never was
PR #325 pinned brace-expansion to ^2.1.2 via an override. ^2.1.2 is inside the affected range (≤5.0.7). The advisory was never resolved — the audit stayed red for weeks. Nothing caught it except the audit itself. This is a textbook example of why a red audit is a better signal than a green audit achieved through a wrong override.
Judgment call 1: Not bumping to ^5.0.8
Correct. brace-expansion 3.x dropped the CJS default export. minimatch@9 default-imports it. A forced ^5.0.8 would install cleanly (the package exists at that version) but throw TypeError at runtime — and ONLY on brace patterns. This is a silent, input-dependent break. Strictly worse than a red audit that can't be ignored.
The dependency chain is: glob → minimatch@9 → brace-expansion. minimatch@9 does import braceExpansion from "brace-expansion" — a default import that resolves to undefined when the CJS default export is removed. The TypeError only fires when minimatch actually calls braceExpansion(), which only happens on brace patterns. A test suite that doesn't exercise brace patterns would pass. Production would break on the first glob with {a,b} syntax.
Judgment call 2: Removing the override rather than keeping a correct floor
Correct, and I'll explain why in detail since you asked.
The override was "brace-expansion": "^2.1.2". With glob gone, it's inert today — no package in the tree depends on brace-expansion. But it wouldn't stay inert. A future dependency that pulls in minimatch@9 (or any other brace-expansion consumer) would silently resolve to ^2.1.2 — the override — which is inside the advisory range. The audit would stay green (the override satisfies the semver check) while the vulnerability is present.
A pinned floor at a genuinely safe version (e.g., ^5.0.8) would be worse: it would make the audit green while introducing the CJS runtime break described above. There IS no compatible version of brace-expansion that is both outside the advisory range AND compatible with minimatch@9's default import.
The red audit is the correct signal here. It says: "this dependency has a vulnerability and we can't safely upgrade it because the fixed versions break the consumer." An override that papers over that signal is a trap.
The right posture: Remove the unused dependency (glob), remove the inert override, and let the audit stay red if a future dependency re-introduces the chain. A red audit cannot be ignored; a wrong override can.
The glob removal
glob was declared as a dependency of pi-tps-mail in its initial commit and imported nowhere in the repo. Flint verified zero import sites independently. The lockfile confirms the full chain is gone: glob → minimatch → brace-expansion → balanced-match, plus all transitive dependencies (foreground-child, cross-spawn, jackspeak, @isaacs/cliui, @pkgjs/parseargs, minipass, lru-cache, path-scurry, package-json-from-dist, path-key, shebang-command, shebang-regex, which, isexe, eastasianwidth, color-convert, color-name, and all the -cjs aliases). 131 → 99 packages.
Verification
- Audit clean under wiped
node_modulesAND under--frozen-lockfile(what CI runs) brace-expansionabsent from the lockfile- Tests 1113 pass / 5 fail / 3 skip, byte-identical to main baseline, all 5 pre-existing and environmental
- 11 of 11 CI checks green
This is the correct fix: remove the unused dependency, remove the inert override, let the audit be the honest signal it is.
tps-kern
left a comment
There was a problem hiding this comment.
Architecture review — APPROVED
The previous fix that never was
PR #325 pinned brace-expansion to ^2.1.2 — inside the affected range (<=5.0.7). The override looked like a fix and wasn't. This is the most important finding: the audit staying red was the only thing that caught it. Worth a process note: an override pin should be validated against the advisory's actual patched range, not just assumed to resolve it.
Removal over bump — correct call
The judgment call to remove the override rather than bump to ^5.0.8 is the right one. The incompatibility is real and demonstrated: minimatch@9 default-imports brace-expansion, and 5.0.8 dropped the CJS default export. A forced ^5.0.8 would install clean and throw TypeError on brace patterns at runtime — a silent, input-dependent break. Worse than the advisory.
With glob removed, nothing in the tree consumes brace-expansion. An inert override at ^5.0.8 would be harmless today but would become a trap if a future dependency pulls in a minimatch@9-shaped consumer: it would convert a loud, blocking audit failure into a quiet correctness bug. A red audit forces action; a silent runtime error on brace patterns might not be noticed until production.
Removing the override means: if brace-expansion re-enters the tree via a new dependency, the audit catches it. That's the correct failure mode — loud and blocking, not quiet and wrong.
glob removal — verified unused
The diff removes glob from packages/pi-tps-mail/package.json and its transitive subtree from bun.lock (131 to 99 packages). The PR body states zero import sites, verified independently. The GitHub code search confirms: "glob" appears in docs, config files, and as a string concept, but no import/require of the glob package in any source or test file. The lockfile diff shows the full transitive subtree (glob, minimatch, brace-expansion, balanced-match, foreground-child, jackspeak, path-scurry, etc.) cleanly removed.
Other overrides untouched — correct
ws, shell-quote, js-yaml overrides remain. All three clear live advisories on packages actually present in the tree. Verified: the diff only removes the brace-expansion line from the overrides block.
Test results
1113 pass / 5 fail / 3 skip — byte-identical to main baseline. The 5 failures are pre-existing and environmental (sandboxed process spawn, SSH reachability timeout). 11/11 CI checks green (the pre-existing Dependency Audit failure is on main too, and this PR fixes it by removing the vulnerable package).
Ship it.
tps-sherlock
left a comment
There was a problem hiding this comment.
Review — removal is correct, both judgment calls are right.
The previous fix that never was
PR #325 pinned brace-expansion to ^2.1.2 — inside the affected range (<=5.0.7). ^2.1.2 resolves to 2.1.2, which is vulnerable. The override looked like a fix, audit stayed red, and nothing caught it for weeks. This is the real finding: a forced resolution that doesn't actually resolve the advisory is a silent false-positive suppressor, and the only thing that caught it was the audit refusing to go green.
Removal, not bump — correct
Bumping to ^5.0.8 would be wrong. brace-expansion 3.x dropped the CJS default export; minimatch@9 default-imports it. Forcing 5.0.8 under minimatch@9 installs cleanly and then throws TypeError: (0 , brace_expansion_1.default) is not a function — but only on brace patterns. Non-brace patterns keep working. A silent, input-dependent runtime break is strictly worse than the advisory.
Override removed rather than kept as floor — correct call
With glob gone, nothing in the tree depends on brace-expansion. The lockfile confirms: every removed package (@isaacs/cliui, balanced-match, cross-spawn, foreground-child, jackspeak, lru-cache, minimatch, minipass, path-scurry, which, etc.) is a transitive of glob. No other path to brace-expansion exists.
A ^5.0.8 override would be inert today but would not stay inert. If a future dependency pulls in minimatch@9, the override converts a loud, blocking audit failure into the silent correctness bug above. A forced resolution can break silently; a red audit cannot be ignored. The override removal is the safer posture.
glob removal — verified
Removed from packages/pi-tps-mail/package.json. The PR body states zero import sites in the repo. The lockfile confirms it was the sole path to the entire vulnerable subtree. 131 to 99 packages.
Test baseline
1113 pass / 5 fail / 3 skip, byte-identical to main. All 5 failures pre-existing and environmental. 11/11 CI green.
✅ Approved.
The headline: the previous override pinned to a still-vulnerable version
bun audithas been red onmain, and sincebun auditis a required CI job(
.github/workflows/test.yml), it blocks every open PR.GHSA-mh99-v99m-4gvg
(CVE-2026-14257, high, CVSS 7.5) affects brace-expansion
<=5.0.7, and5.0.8 is the only patched release — there is no patched 2.x, 3.x or 4.x line.
PR #325 added
"brace-expansion": "^2.1.2"to the rootoverridesblock as theremedy.
2.1.2is inside the affected range, so the override never cleared theadvisory — it just moved the resolution to another vulnerable version. That is
the most important thing to take away from this PR: the earlier fix looked like a
fix and was not one, and nothing caught it except the audit staying red.
Why the fix is not simply bumping the override to
^5.0.8brace-expansion 3.x replaced the CJS default export with a named
expandexport:2.1.2entry point doesmodule.exports = expandTop(default-style export)5.0.8entry point doesexports.expand = expandwith__esModule: true, andships no default export
glob@10.5.0depends onminimatch@9.0.9, which declaresbrace-expansion: ^2.0.2and imports it as a default (
__importDefault(require("brace-expansion")), thenbrace_expansion_1.default(pattern)).Forcing 5.0.8 under minimatch@9 installs cleanly and appears fine, then throws at
runtime on the first brace pattern:
Non-brace patterns keep working, so this is a silent, input-dependent break — a
worse failure mode than the advisory it was meant to fix.
minimatch@10is thefirst release that declares
brace-expansion: ^5.0.5and uses the named-export API.What this PR does
globdependency from@tpsdev-ai/pi-tps-mail.globwasdeclared in that package's initial commit (feat(pi-tps-mail): publishable extension for TPS mail watcher (ops-lnvc) #271) and is imported nowhere in the
repository — no
import/requireof it exists in any source, test, or script.It was the sole path to
brace-expansion, so removing it removes the vulnerablesubtree at the root instead of papering over it with a forced resolution.
brace-expansionentry from the rootoverridesblock entirely,rather than re-pinning it to
^5.0.8. Reasoning below.The
ws,shell-quoteandjs-yamloverrides are untouched — those clear liveadvisories on packages actually present in the tree, and all three remain effective
(
ws@8.21.0,shell-quote@1.10.0,js-yaml@4.3.0).Why the override is removed rather than corrected to
^5.0.8A forced resolution can break silently; a red audit cannot be ignored.
With
globgone, nothing in the tree depends onbrace-expansion, so a^5.0.8override would be inert today. But it would not stay inert: if a future dependency
pulls in a
minimatch@9-shaped consumer, the override converts what would have beena loud, blocking
bun auditfailure into the quiet correctness bug demonstratedabove — wrong results only on brace patterns, in production. A red audit blocks every
PR until someone deals with it, which is exactly how this problem surfaced. Keeping
the entry as a "correct floor" only helps if the floor is compatible with whatever
consumer arrives, and the incompatibility above shows we cannot assume that.
Verification
bun auditbefore (onmain):bun auditafter:Dependency tree: 131 -> 99 packages installed.
brace-expansiondoes notreappear in
bun.lockafter a cleanrm -rf node_modules && bun install(0occurrences in the lockfile).
Build:
bun run buildsucceeds from a clean tree.Lint:
bun run lintpasses (exit 0).Tests — run per package, and compared against a pristine
maincheckout on thesame machine:
mainbaselinepackages/agentpackages/clipackages/pi-tps-mailZero regressions. The 5 failures are identical by name on both branches and are
pre-existing on
main— all environmental (sandboxed process spawn and an SSHreachability probe that times out), unrelated to dependencies:
get > runs verify and returns live valuerunCommandUnderNono() > warns and falls back when nono not on PATH (non-strict)type coercion > string coercion — rejects emptyrunVerify — nonzero exit > false command returns nonzero_exitvalidateSshReachable > reports full ssh command in error for debuggingAll five pass in CI, confirming they are local-sandbox artifacts. The 3
pi-tps-mailskips are hard-codedit.skipin source and are unchanged frommain.Note that the root
testscript chains packages with&&, so the pre-existingpackages/clifailures short-circuit it andpackages/pi-tps-mailnever runs underbun run test. Each package was run individually to get the counts above.