fix(studio): match style attributes with explicit quote boundaries - #3712
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at exact head 456e5272225206b7316a1985720552323fb0f912.
The changed matcher at packages/studio/src/utils/htmlEditor.ts:32-40 is equivalence-correct: the alternatives preserve leftmost/first-match selection, full matched span, quote, empty/multiline value, and therefore the exact replacement boundary. I independently compared normalized match {index, full, quote, value} across 200,015 generated/adversarial inputs with no mismatch. The 51 focused tests pass at this head.
Blocker — the same quote-matcher contract remains in two live Studio writers. packages/studio/src/utils/sourcePatcher.ts:187 still applies /\bstyle=(["'])([\s\S]*?)\1/ to owner-authored source reached through applyPatch/applyPatchByTarget, and packages/studio/src/components/editor/colorGradingScopePatch.ts:2 retains the same wildcard/backreference form for data-color-grading on source passed to patchMediaColorGradingInHtml. The function fixed here has no in-repo caller beyond its public export, while those two paths are invoked by Studio hooks/components, so this clears the two named CodeQL locations without applying the chosen explicit-quote-boundary invariant to the active sibling paths.
I am not claiming a demonstrated ReDoS—the PR is appropriately explicit that the scanner input did not reproduce one. The issue is scope completeness: these sites have the same unbounded HTML precondition and the same ambiguous regex shape, and relying on CodeQL to trace only the exported helper makes the rule depend on analyzer reachability. Please convert both siblings to explicit single/double-quote alternatives and add the corresponding long/repeated-opener regression cases.
CI has no failures; JavaScript CodeQL and Windows checks were still pending when reviewed.
Verdict: REQUEST CHANGES
Reasoning: The proposed matcher is correct, but two active sibling mutation paths still implement the exact wildcard/backreference contract this security cleanup is replacing.
— Magi
|
Addressed the requested sibling-path coverage in 4995be1. The source patcher and media color-grading writer now use explicit quote-delimited alternatives while retaining their own word/whitespace boundaries and case rules. Added long, multiline, empty and repeated-opener cases through ID/selector edits and grading replacement/removal. All 60 focused tests, Studio build/typecheck, lint/format and signed hooks pass; 150k additional complete-output comparisons across the three active entry points matched. Requesting re-review; fresh CI and CodeQL remain required before merge. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at exact head 4995be16c3b8f4543d012ace9a9c2c4cfc5bdee6. The prior scope blocker is closed; no new blockers.
packages/studio/src/utils/sourcePatcher.ts:187-190 now preserves its case-sensitive \bstyle= contract while selecting the active alternative's quote/value, including empty values. packages/studio/src/components/editor/colorGradingScopePatch.ts:2 preserves its leading-whitespace and case-insensitive contract; because this writer only needs the full matched span, removing the captures cannot affect replacement or removal. Both remain first-match, delimiter-bounded expressions.
The added public-path cases at sourcePatcher.test.ts:11-24 exercise both ID and selector edits, while colorGradingScopePatch.test.ts:5-23 exercises both replacement and removal across empty, multiline, long, and repeated-opener values. A repo-wide scan confirms the wildcard/backreference HTML-attribute form is gone from Studio production code.
Independent exact-head verification:
- all 60 focused tests passed
- formatting passed on all six changed files
- old/new match span and applicable quote/value captures were identical for 250,020 generated/adversarial inputs independently for each of the three matchers
Fresh CI had no failures; JavaScript CodeQL, Windows, and remaining required jobs were still running and remain landing gates.
Verdict: APPROVE
Reasoning: Both active sibling paths now share the explicit quote-boundary invariant without losing their distinct boundary/case semantics, and the observable mutations remain equivalent under focused and independent comparison.
— Magi
Addresses CodeQL alerts #168 and #169 in Studio style matching, plus the same quote-matching pattern in active inline-style and media color-grading writers. Replace broad wildcard/backreference matchers with explicit single-quoted and double-quoted alternatives whose contents cannot consume their closing delimiter.
Preserve each writer's existing prefix/boundary and case rules, first-match selection, quote/value handling, multiline/empty values, and exact source replacement. Existing permissive behavior for malformed attributes remains unchanged. The scanner's repeated-opener example did not reproduce a slowdown locally; this structurally simplifies the patterns without claiming a demonstrated exploit.
Validation: all 60 focused HTML-editor, source-patcher and color-grading tests pass. Cases cover both quote styles, opposite quotes in values, multiline/empty values, duplicates, case/spacing rules, malformed tags, 100k-character values and 10k repeated openers. Complete outputs matched across 100k generated HTML-editor inputs and another 150k comparisons across ID, selector and color-grading edits. Full workspace build passed for the initial change; Studio build/typecheck, lint/format and signed hooks passed after the sibling changes.