Skip to content

fix(markup): remove exponential backtracking from reference validation - #439

Open
Wenjix wants to merge 1 commit into
hypit-ai:mainfrom
Wenjix:security/markup-reference-redos
Open

Wenjix wants to merge 1 commit into
hypit-ai:mainfrom
Wenjix:security/markup-reference-redos

Conversation

@Wenjix

@Wenjix Wenjix commented Oct 27, 2026 •

Copy link
Copy Markdown

Closes #447

Summary

One ReDoS finding from a security review of markup parsing:

  • 57 (medium, CWE-1333): packages/markup/src/syntax.ts validated dotted reference names with ^[A-Za-z_][A-Za-z0-9_.:-]*(?:\.[A-Za-z_][A-Za-z0-9_.:-]*)*$. . appears both inside the segment character class and as the separator, so on a malformed dotted name the regex engine tries exponentially many partitions — measured ~330 ms at 25 trailing dots, doubling per dot; ~40 dots is effectively a parse-thread hang. Any .svml file can reach this.
  • The grammar is now strict ident(.ident)* — ^[A-Za-z_][A-Za-z0-9_:-]*(?:\.[A-Za-z_][A-Za-z0-9_:-]*)*$ — . only separates, so each character has exactly one consumption path and matching is linear.
  • Side effect worth noting: the old pattern also accepted a., a..b and x.y. (trailing/consecutive dots). Those are now rejected, which is a correctness fix as well as a security one — no test or fixture relied on the loose form.

Behavior relative to main

Input main this PR
camera.lens accepted accepted
a.b.b.b…b! (25 dots + bad tail) ~330 ms, grows 2× per dot <1 ms, rejected
a. / a..b / x.y. accepted rejected

Red / green

packages/markup/test/syntax.test.ts (new): parses a normal dotted reference, rejects empty/trailing segments, and rejects a long malformed input — the last fails by timeout on main and returns immediately here.

Verification

  • node --import tsx --test packages/markup/test/syntax.test.ts — 16 pass.
  • pnpm check (tsc -p tsconfig.json --noEmit) — clean.
  • Adversarial input timed before/after: 326 ms → sub-millisecond.

Merge Danger

  • Reference names ending in or containing repeated dots (a., a..b) were previously admitted and are now validation errors.

Checklist

  • The why is documented in this description.
  • Regression test added; fails (by timeout) on main, passes here.
  • TypeScript check clean.

Generated with Devin

The reference attribute regex admitted `.` both inside segments and as
the repeated group separator, so a malformed dotted path could be
partitioned exponentially many ways before rejection. A 40-segment
reference took minutes on the synchronous parse thread.

Require `.` strictly between segments: each character now has exactly
one consumption path. This also rejects previously tolerated empty or
trailing segments (`a.`, `a..b`), which could never resolve to a record.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 27, 2026 06:15

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

SanHsien added a commit to SanHsien/hypit that referenced this pull request Oct 28, 2026
…i#447-hypit-ai#456; advance watermarks

PRs hypit-ai#442, hypit-ai#443 and hypit-ai#444 were ported ahead of upstream (d5afdfe, 541ab59,
5dc763c) because the fork has the same defect and it is reachable from
outside input. The rest are recorded with the fork-side evidence and a
re-review trigger in docs/fork/DECISIONS.md. Release track unchanged at
v0.3.1; watermarks PR 455, issue 456.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

bug(markup): exponential backtracking in reference-name validation

2 participants