Skip to content

fix(core): bound timing compiler opening tag scans - #3719

Merged
jrusso1020 merged 2 commits into
mainfrom
fix/security-timing-tag-scans
Sep 5, 2026
Merged

fix(core): bound timing compiler opening tag scans#3719
jrusso1020 merged 2 commits into
mainfrom
fix/security-timing-tag-scans

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

What

Fix CodeQL #164 and #165: repeated opening tag prefixes without a closing > cause quadratic regex work in timing compilation. Apply the same bounded scans to the related audio/composition discovery and both ID-targeted duration writers.

Why

The four discovery matchers each exceeded two seconds on 100,000 unclosed prefixes. Independent review also reproduced the same failure through injectDurations and clampDurations with only 1,000 repeated ID-bearing prefixes. All four public timing APIs need bounded opening-tag searches.

How

An internal iterator finds a prefix and searches once for its closer, advancing beyond complete spans and stopping when no closer remains. Replacements assemble slices from the original pass input. ID-targeted writers advance a separate escaped-ID cursor across complete spans, retaining the old greedy ID choice, overlapping matches and IDs containing >.

Preserve separate video/audio passes, independent ID counters, resolution ordering, mixed-media source order, case matching, substring id= matching, first-closer behavior and inert-region restoration. This changes scan complexity, not parsing rules. Two files changed; no public API or workflow changes.

Test plan

  • All 273 compiler tests pass, including 15 new cases for long incomplete suffixes across all public consumers, ID/order behavior, delimiter-bearing IDs and existing quoted-> behavior.
  • 100,000 generated inputs produced identical complete results for compilation, extraction, duration injection and clamping against main.
  • Long unclosed-prefix probes complete promptly, including 100,000 ID-bearing prefixes through both duration writers.
  • Parser/core/lint builds, core/runtime types, lint, formatting and complexity audit pass.
  • Renewed Magi review and fresh CI/CodeQL remain landing gates. Verify main-branch closure of both alerts after merge.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at exact head 5bc19a3570da54942388305525cc5c11d87a5cf3.

The new iterator at packages/core/src/compiler/timingCompiler.ts:157-181 is behavior-equivalent and linear for the four migrated streams. It yields the same prefix→first-> spans as the old regexes, advances on the original pass input so replacement text is not rescanned, and preserves separate video/audio passes plus mixed-media source order. I independently matched all four old/new span streams across 300,009 generated/adversarial inputs each; 266/266 compiler tests pass.

Blocker — two exported opening-tag writers in the same file still use the quadratic form. injectDurations at timingCompiler.ts:302-325 and clampDurations at :374-389 each retain (<[^>]*id=["']…["'][^>]*>). They accept the same composition HTML and have the same unclosed-opening-tag precondition this PR removes from compile/extract.

This is reachable through the public APIs, not a theoretical grep match. With only 1,000 repetitions of <video id="target" and one matching resolution, both exact-head calls exceeded a 3-second subprocess timeout. The isolated remaining regex took 5.17 seconds at that size. That is the original failure class at two sibling sites, while the migrated iterator handles 100,000 prefixes in 0.19–1.07 ms in my probe.

Please route both ID-targeted writers through the bounded tag iterator (preserving their current substring-id= and first-> behavior) and add long-unclosed-prefix witnesses for injectDurations and clampDurations. A safe shape is to iterate complete <…> spans linearly, perform the escaped ID match only inside each bounded tag, and reuse replaceOpeningTags; avoid moving the overlapping [^>]* lookahead into the prefix regex.

CI has no completed failure; JavaScript CodeQL, Windows, build/typecheck, and regression jobs are still running.

Verdict: REQUEST CHANGES
Reasoning: The shared iterator is correct, but two public sibling paths retain—and independently reproduce—the same multi-second opening-tag scan this PR is intended to eliminate.

— Magi

@jrusso1020

Copy link
Copy Markdown
Collaborator Author

Addressed the duration-writer scope blocker in 513ec8b. Both injectDurations and clampDurations now use bounded opening spans plus an advancing escaped-ID cursor. The ID-specific helper preserves substring matching, the greedy last matching ID (including overlaps), and IDs containing >; simply testing IDs inside the first-> span would have regressed that last case.

All 273 compiler tests pass, including long unclosed ID-bearing prefixes through both writers. The 100,000-prefix writer probes take about 0.5–0.6 ms locally. Complete compile/extract/inject/clamp outputs match main across 100,000 generated inputs, including delimiter and regex-metacharacter IDs. Core/runtime types, formatting, lint, complexity audit and signed hooks pass. PR description rewritten for final scope; renewed review and fresh CI/CodeQL requested.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at exact head 513ec8bc7cb57fe3e03e14751dd6793eca7e98ab. The prior duration-writer scope blocker is closed; no new blockers.

replaceIdTags at packages/core/src/compiler/timingCompiler.ts:184-208 reproduces the old matcher’s non-obvious selection rules without overlapping whole-tag scans:

  • candidate.index < end requires the matching id= substring to begin before the first >, as the old leading [^>]* did.
  • Advancing idPattern.lastIndex by one permits overlaps, while retaining the last eligible candidateEnd reproduces the old greedy prefix’s last matching ID.
  • candidateEnd <= lastClosing plus the closer search after targetEnd preserves IDs containing >: the ID literal may cross the iterator’s initial span, then the replacement ends at the first > after its closing quote.
  • cursor prevents iterator spans swallowed by that extended replacement from being emitted twice; the independent candidate cursor advances each match once.

Both injectDurations and clampDurations now use this single helper, so every public timing-compiler opening-tag writer is on a bounded scan. Repeated resolutions still apply sequentially to the previous result, matching the old outer loop.

Independent exact-head verification:

  • 273/273 compiler tests passed
  • changed-file formatting passed
  • old/new replacement output matched across 250,012 generated/adversarial strings for each of eight IDs, covering overlaps, substring data-id, mismatched quotes, empty/metacharacter IDs, and </> inside IDs
  • 100k unclosed prefixes completed in 1.77 ms for a normal ID and 25.02 ms for a delimiter-bearing ID; both remain linear, versus both public APIs timing out beyond 3 seconds at only 1,000 prefixes on the prior head

Fresh CI has no current-head completed failure; JavaScript CodeQL, Windows, build/typecheck, and regression jobs remain landing gates. Older cancelled runs are surfaced as failures by gh pr checks but are not from this head’s live run.

Verdict: APPROVE
Reasoning: The two-cursor helper closes the remaining public quadratic paths while preserving the old greedy, overlapping, substring, and delimiter-bearing ID semantics exactly.

— Magi

@jrusso1020
jrusso1020 merged commit b94b5bd into main Sep 5, 2026
59 of 87 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-timing-tag-scans branch September 5, 2026 22:02
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.

2 participants