fix(studio-server): bound preview variable insertion scans - #3715
Conversation
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at exact head eb6dca04d0d3c00da0c8fc7c1902910f244764f0. No blockers.
The scan at packages/studio-server/src/helpers/previewVariables.ts:17-24 is linear and behavior-equivalent. For each of three fixed-priority prefixes it performs one literal/anchored search and, only on a match, one indexOf for the first >; a first unmatched prefix with no later > cannot hide a closable later prefix of that kind. The head → html → leading-doctype → prepend priority and the existing permissive boundaries (<headless>, > inside quotes) are explicitly pinned at previewVariables.test.ts:7-35.
All three route branches use the extracted helper at routes/preview.ts:386, :411, and :494. The JSON < escaping is byte-identical at previewVariables.ts:10-13, and the main/sub-composition ETag salts remain computed from the raw variable query before any injection (routes/preview.ts:327-336, :457-476), so cache separation is unchanged.
Independent exact-head verification:
- 60/60 helper + preview-route tests passed
- changed-file formatting passed
- complete old/new output matched across 300,017 generated and adversarial HTML inputs, including 100k unterminated head/html prefixes and the single-closer variants
Nit: packages/studio-server/src/helpers/mediaProxyPreview.ts:114 still says it mirrors injectPreviewVariables “in routes/preview.ts”; this extraction moved the implementation to helpers/previewVariables.ts. Updating that breadcrumb would keep the refactor’s ownership record accurate.
CI had no failures; JavaScript CodeQL, Windows, and remaining required jobs were still running and remain landing gates.
Verdict: APPROVE
Reasoning: The extracted helper preserves every insertion and cache/escaping contract while replacing repeated regex rescans with a bounded constant number of linear passes.
— Magi
|
Updated the stale mediaProxyPreview.ts reference in 7f6b9e6 to point to helpers/previewVariables.ts. This revision only changes that comment; lint, format and signed commit hooks passed. Requesting exact-head re-review while fresh CI runs. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-pinned at exact head 7f6b9e6a8b114f5345d56099a83fd853eb7881d7 after verifying the post-approval delta.
eb6dca04d0...7f6b9e6a8b is exactly one commit and one comment line: packages/studio-server/src/helpers/mediaProxyPreview.ts:114 now points to helpers/previewVariables.ts. Runtime and test blobs are unchanged, and git diff --check is clean. This closes the prior nit without changing the approved behavior.
Fresh CI remains the landing gate.
Verdict: APPROVE
Reasoning: The only new-head delta is the requested breadcrumb correction; the previously reviewed implementation is byte-identical.
— Magi
2 participants
Fixes CodeQL alert #743: preview variable injection repeatedly rescans unterminated
<heador<htmlprefixes while searching for a closing>.Find the first candidate prefix, then search once for its closing
>. If that first prefix cannot close, no later prefix of the same kind can close either. Preserve the head → html → leading doctype → prepend priority, existing prefix/case semantics, insertion position and script-value escaping. Move the existing injection function into an internal helper so its boundary behavior can be tested directly; all three preview call sites use the same helper.Validation: both original regexes exceeded a two-second subprocess timeout on 100k repeated unterminated prefixes. All 60 injection-helper and preview-route tests pass, covering long malformed tags, fallback order, case/attributes, script breakout escaping, main/sub-composition previews and ETags. Complete old/new output matched across 100k generated inputs. Parser/core/lint and Studio-server builds, server typecheck, lint and format pass.