Skip to content

fix: bound parser nesting and recursive AST traversal - #78

Open
thaibaole89 wants to merge 1 commit into
micromatch:masterfrom
thaibaole89:fix/ast-depth-guard
Open

thaibaole89 wants to merge 1 commit into
micromatch:masterfrom
thaibaole89:fix/ast-depth-guard

Conversation

@thaibaole89

Copy link
Copy Markdown

Deeply nested patterns below the existing character limit can overflow the recursive AST walkers. This change rejects excessive nesting with a controlled SyntaxError before the engine exhausts its stack.

  • Cap parser brace/parenthesis nesting and recursive compile/expand/stringify traversal at 100.
  • Apply the traversal bound to direct AST nodes trees, which bypass parsing.
  • Document the bound in README and its .verb source. Parser container nesting and walker call depth have distinct counts at the boundary.
  • Preserve ordinary alternatives, ranges, escapes and parsed AST behavior. No version/dependency change.

Addresses #70 / GHSA-vfj7-8cjw-p6xm. Related to #77, which proposes an opt-in maxDepth option; this proposal provides a mandatory default bound for existing callers that pass no option. Maintainer input on the threshold/API is welcome.

Validation:

  • New regressions against unchanged upstream: 12 fail / 2 pass.
  • Builder: npm test on Node 24.14.1, 908 pass, including 14 new cases.
  • Independent reviewer: full upstream suite on actual Node 8.3.0, 908 pass; new tests 14/14 pass. Removing the shared guard causes 12 failures; individual compile/expand/stringify guards cause 2 failures each, and parser guards 1 failure.
  • A 512KB-stack child process checks 4000-level strings; direct AST and 100/101 boundaries are exercised. Ordinary syntax and 99 nested containers retain expected results.

This bounds parser and AST nodes recursion depth; it does not cap expansion cardinality or claim safety for every malformed AST field/getter/parent structure or resource-exhaustion input. For example, a malformed array-valued text node can still reach unguarded flattening; the parser does not emit that shape. No general CPU/memory or regex-safety claim is made.

AI assistance: OpenAI Codex was used to prepare the patch, tests and independent review.

cwaits6 added a commit to cwaits6/two42 that referenced this pull request Oct 9, 2026
braces 3.0.3 (CVE-2026-93687) is reached only at lint time through
eslint-plugin-next -> fast-glob -> micromatch, on glob patterns this repo
writes itself, and upstream has three fix PRs open (micromatch/braces#78,
#79, #82). Leave the finding visible so it clears on its own when the
patched release reaches the lockfile.

@ulises-jeremias ulises-jeremias left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I independently tested the current head 97308a0 with the upstream test suite on Node 22.22.0: npm test passes all 908 tests. This includes the new nesting/depth cases. GitHub currently reports no CI checks for this PR, and no npm release is available yet, so consumers cannot adopt it as a supported dependency update until it is merged and published.

@ActiveInAI

Copy link
Copy Markdown

We tested an additional local guard against the published 3.0.3 package while working on the existing stack exhaustion advisory. This feedback is intended to complement the current PR rather than duplicate it.

A parser-only nesting cap does not cover the public AST entry points:

const braces = require('braces');
let ast = { type: 'text', value: 'x' };
for (let i = 0; i < 20000; i++) ast = { type: 'root', nodes: [ast] };
braces.compile(ast); // RangeError with the unpatched package on Node 24.19.0

Our local patch checks nodes iteratively before compile, expand and stringify, as well as limiting parser brace/parenthesis nesting. An ancestry set catches child cycles while allowing shared subtrees: a root whose two children are the same text node must still compile to xx. A global visited set alone would reject that valid case.

The guard runs before the existing recursive walkers and throws a controlled SyntaxError. We used an explicit fixed depth budget of 128. Range/cardinality limits remain separate; this is not a claim to validate all malformed AST fields or prevent every expansion resource-exhaustion case.

Validation: 37 tests passed, 0 failed on Node 24.19.0, covering the existing compile/expand/parse fixtures and three new regression tests for deep strings, deep/cyclic/shared ASTs, and ordinary escape/range behaviour. The fixture harness uses Node's test runner with the existing Mocha-style globals; this is not a claim that the complete Mocha suite or the Node 8 compatibility matrix was run.

On this Node 24 runtime, the balanced 4000-level string case did not reproduce a stack overflow at the default stack size. The direct AST case above did, so we kept that distinction in the evidence.

OziinG added a commit to EVNSolution/clever-driver-app that referenced this pull request Oct 11, 2026
…dings instead of staying red until an upstream release that has no date

npm audit only sees registry versions, so the installed source patches could never
turn the gate green and every new advisory would hide behind the permanent failure.
The gate now runs the same npm audit and fails on everything except the two listed
GHSA ids, and accepts those only while --check verifies the installed patches and
the allowlist has not expired.

Constraint: braces 3.0.3 and node-forge 1.4.0 have no patched release (checked 2026-10-11); micromatch/braces#78 and digitalbazaar/forge#1152 are still open.
Rejected: --audit-level=critical | hides every high finding, not just the two reviewed ones
Rejected: waiting for upstream | braces has had no release since 2024-05 and no maintainer response on the fix PR
Rejected: replacing the dependencies | both are internal to Expo CLI; a fork would be heavier than the patches
Confidence: high
Scope-risk: narrow
Directive: remove an allowlist entry as soon as a supported fix release exists; renew expiresOn only with a new review note.
Tested: node --test scripts/security (34 pass), node scripts/audit-allowlist.mjs locally (npm audit high 20 all resolved to the two advisories, exit 0), expo lint, git diff --check
Not-tested: this gate on CI Node 20.19.4 / npm 10.8.2 (pushed for CI)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
OziinG added a commit to EVNSolution/clever-driver-app that referenced this pull request Oct 11, 2026
…dings instead of staying red until an upstream release that has no date

npm audit only sees registry versions, so the installed source patches could never
turn the gate green and every new advisory would hide behind the permanent failure.
The gate now runs the same npm audit and fails on everything except the two listed
GHSA ids, and accepts those only while --check verifies the installed patches and
the allowlist has not expired.

Constraint: braces 3.0.3 and node-forge 1.4.0 have no patched release (checked 2026-10-11); micromatch/braces#78 and digitalbazaar/forge#1152 are still open.
Rejected: --audit-level=critical | hides every high finding, not just the two reviewed ones
Rejected: waiting for upstream | braces has had no release since 2024-05 and no maintainer response on the fix PR
Rejected: replacing the dependencies | both are internal to Expo CLI; a fork would be heavier than the patches
Confidence: high
Scope-risk: narrow
Directive: remove an allowlist entry as soon as a supported fix release exists; renew expiresOn only with a new review note.
Tested: node --test scripts/security (34 pass), node scripts/audit-allowlist.mjs locally (npm audit high 20 all resolved to the two advisories, exit 0), expo lint, git diff --check
Not-tested: this gate on CI Node 20.19.4 / npm 10.8.2 (pushed for CI)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants