fix: backport GHSA-mh99-v99m-4gvg - #129
Conversation
|
@juliangruber this looks to have landed cleanly, but I'll be doing a bit more cleanup and whatnot - I would appreciate knowing though that you're happy with landing this and if there's any particular changes you'd like to see for v1 etc specifically. Assuming this is good after cleanup, I'll try to backport to v2 and v3 as well |
91b1773 to
0bb3193
Compare
|
that backport does not actually fix the issue, assuming something got left out that shouldn't have been |
juliangruber
left a comment
There was a problem hiding this comment.
same requests as in v2 PR
|
When this v1 version will be released? |
|
It needs to pass review first |
|
thank got. we need this fix! |
juliangruber
left a comment
There was a problem hiding this comment.
Overview
Backports the CVE-2026-14257 fix from the v5 line onto v1. Three changes:
expandbecomes iterative - consumes top-level brace groups left to right threading an accumulator, instead of recursing onm.postonce per group. Removes the stack overflow at ~2,700 chained groups.- New
maxLengthoption (default 4,000,000) enforced in a newcombinehelper, bounding total output characters. - New
test/cve.jswith three regression tests.
The port is faithful to upstream src/index.ts, correctly de-TypeScripted, and kept ES5 (var throughout), which matters since v1 still declares an ie/8..latest testling matrix. All 193 existing tests pass, and I diffed base-vs-PR output across all 147 test/cases.txt entries: no regressions.
The approach is sound. A few things want addressing before merge.
Undocumented behavior change: ${...} no longer halts expansion
Base returned the whole string literally on /\$$/. The PR now continues expanding the tail. Seven inputs change behavior:
"${x}{a,b}" base: ["${x}{a,b}"] pr: ["${x}a","${x}b"]
"a${b}{c,d}e" base: ["a${b}{c,d}e"] pr: ["a${b}ce","a${b}de"]
"x${y}z{1..3}" base: ["x${y}z{1..3}"] pr: ["x${y}z1","x${y}z2","x${y}z3"]
"{a,b}${c}{d,e}" base: ["a${c}{d,e}", ...] pr: ["a${c}d","a${c}e","b${c}d","b${c}e"]
This is a fix, not a regression. I checked against bash (echo ${x}{a,b} gives a b) and against published 5.0.8, and the new behavior matches both. But it is a semantic change landing on a legacy line with very large downstream reach, and test/dollar.js misses it entirely: its three cases all use ${a,b} shapes where nothing expandable follows, so they pass either way.
Please add the covering case and call the change out in the PR body / release notes:
t.deepEqual(expand('${x}{a,b}'), ['${x}a', '${x}b']);max default diverges from upstream, so the bound is characters-only
Upstream defaults max to EXPANSION_MAX = 100_000. Line 82 here keeps v1's Infinity. Because combine only tracks characters, zero-length expansions never advance the counter and out.length >= max never trips:
expand('{,}'.repeat(28)) // every expansion is '', so `length` stays 0To be clear this is pre-existing and the PR improves it - on the base v1 the same input dies at 22 groups, on this branch it survives to 26. But the header comment claims memory "stays flat no matter how many brace groups are chained", and that is not true for this shape. Passing max explicitly closes it entirely ({max: 100000} handles 100 groups in ~57ms).
Keeping Infinity looks deliberate, since tests 188/191 assert "default is unbounded". If so, worth either softening that comment to say the v1 bound is characters-only, or adding a cheap out.length guard in combine that is independent of max. Flagging it as a decision rather than a defect.
Minor
concat-mapis now an unused dependency. Line 1 still hasvar concatMap = require('concat-map'), but the PR removed its only call site (N = concatMap(n, ...)). Dropping the require and thedependenciesentry is a free supply-chain reduction for the v1 line. (identityon line 98 is also dead, but that predates this PR.)- Comments reference
EXPANSION_MAX, which does not exist onv1. Lines 12-20, copied verbatim from upstream. The rationale "100k results hittingEXPANSION_MAXmeasure ~1M characters" does not parse for a v1 reader, since there is no such constant on this branch. Needs rewording for the backport. - Typo from the mechanical
consttovarconversion. Line 212 reads "keeps the native stack depth varant"; upstream says "constant". Worth re-reading the other ported comments for the same collateral. - TypeScript and c8 leftovers in a plain-JS tape project.
/* c8 ignore start|stop */on lines 157, 161, 286, 300, plus then[0] === undefined/n[2] !== undefinedguards that exist only to satisfytsc.v1has no c8 and no compiler, so these are inert noise, and theif (n[0] === undefined || n[1] === undefined)early return is unreachable in JS. - Missing trailing newlines on both
index.jsandtest/cve.js(\ No newline at end of fileappears twice in the diff). var fs = require('fs')intest/cve.jsline 3 is unused.test/cve.jsruntime. The 50,000-group stack test plus the three{a,b}repeat loops add real wall time. Fine, just noting CI time grows.t.doesNotThrow(() => { ... })wrapping assertions means an inner assertion failure surfaces as a confusing throw rather than a clean tape failure.
Test coverage
Good regression coverage for the two vectors being fixed, and the "bound is a single accumulator, not groups * maxLength" loop is a nice touch. Gaps:
- No test for the
${x}{a,b}behavior change above - No test pinning the zero-length-expansion case, if you decide to address it
Happy to re-review once these are in.
version 1 has backport juliangruber/brace-expansion#129 version 2 has backport juliangruber/brace-expansion#130 but npm audit with GHSA-mh99-v99m-4gvg not take that into account yet !
No description provided.