fix(security): verify CDN script integrity before inlining - #3822
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
The raw-byte check is correctly placed before TextDecoder, and the fixture digests independently match the exact cdnjs/npm bytes. Strongest-algorithm selection, multiple same-strength hashes, unsupported-only metadata, reserved options, URL-safe base64, and mismatch fallback otherwise match the browser behavior I checked.
Blocker — packages/producer/src/services/htmlCompiler.ts:1169: preserve case-insensitive SRI algorithm tokens. The SRI contract defines a supported algorithm token by its ASCII-lowercased value, and Chromium accepts SHA384-…. This regex recognizes only lowercase names. Consequently a valid uppercase or mixed-case token produces no hashes, reaches if (!strongest) return true, and disables verification. I reproduced the concrete bypass with an uppercase SHA-384 digest for trusted bytes and a fetch returning window.compromised=true;: the compiler reported compromisedInlined: true and removed the external tag, while Chromium rejects the same mismatched response. Normalize the parsed algorithm (or make matching case-insensitive and lowercase the capture) before strongest selection, and add the mixed-case mismatch regression.
Focused evidence: bun test packages/producer/src/services/htmlCompiler.test.ts — 134 pass; exact CDN SHA-384 digests and npm byte equality both verified.
Verdict: REQUEST CHANGES
Reasoning: The nine pinned lowercase tags are correct, but the new shared verifier has a standards-valid case variant that bypasses the integrity check and inlines modified code.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Correction to my prior review evidence: my first browser probe showed that a matching uppercase token executed, but did not exercise an uppercase mismatch. The author correctly called out that gap. I reran the exact case without request interception using HeadlessChrome/152.0.7977.42: lowercase mismatch → blocked; SHA384 mismatch → altered script executed; sHa384 mismatch → altered script executed.
The compiler-side casing fix at packages/producer/src/services/htmlCompiler.ts:1169-1180 is correct and closes the original bypass: mixed-case supported algorithms are normalized for strongest-selection and comparison, and the new tests discriminate valid from tampered bytes.
Blocker — integrity mismatch still falls through to an executable browser retry. At htmlCompiler.ts:1211-1213, a mismatch becomes a rejected Promise.allSettled item; htmlCompiler.ts:1234-1240 handles every rejection identically by warning and leaving the original external tag in the document. For lowercase metadata Chrome blocks that fallback. For the standards-valid mixed-case metadata the compiler now rejects, Chrome 152 ignores the token and executes the altered response. The exact attack path is therefore: compiler detects tampered bytes → preserves src + mixed-case integrity → render browser fetches the same tampered URL → browser executes it.
Treat a verified integrity mismatch differently from a download/HTTP failure: fail compilation, or remove/replace the executable tag so the result cannot run remote bytes. Failing compilation is the clearest contract. Keep the existing availability fallback for ordinary fetch failures if desired; a successful fetch whose bytes violate declared integrity is not an availability failure. Update the unit assertion at htmlCompiler.test.ts:356-360, which currently pins the unsafe retained-tag behavior for every mismatch, and add the mixed-case browser mismatch control if that browser behavior remains part of the rationale.
Fresh focused evidence: bun test packages/producer/src/services/htmlCompiler.test.ts — 134 pass; direct browser probe output: lowercaseMismatch blocked, uppercaseMismatch altered, mixedCaseMismatch altered. CI is still running on this head.
— Magi
Verdict: REQUEST CHANGES
Reasoning: Algorithm normalization is fixed, but the mismatch recovery path hands execution back to a browser that demonstrably does not enforce the same valid metadata spelling, so tampered CDN code can still run.
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at exact 1728c44524749c9c8f605ac25bc3d41caf3a3ea8. The remote/nested metadata model is coherent: scriptItems carries integrity/CORS, both producer and core use the same reconstruction helper, exact-URL root duplicates are strengthened, later unpinned references retain a pin, and distinct non-empty requirements fail explicitly. The prior mixed-case fallback blocker is also closed.
Blocker — an unpinned local script can be inlined before a later nested pin is discovered. packages/core/src/compiler/htmlBundler.ts:865-899 inlines every relative root script before sub-compositions are parsed, without checking integrity. When the child pin is processed later at htmlBundler.ts:941-963, ensureExternalScriptTag adds a protected external tag but cannot retract the already-emitted inline chunk. I reproduced this with root <script src="local.js">, a child tag for the same local.js carrying the SHA-384 of trusted bytes, and altered bytes on disk. bundleToSingleHtml returned both alteredInline: true and protectedExternal: true: the browser executes the altered inline code before it ever verifies the protected duplicate.
The same ordering exists within collected children/templates: an unpinned relative item pushes bytes at htmlBundler.ts:954-959 (or hoistExternalScript at htmlBundler.ts:743-750), while a later pinned sibling only creates/strengthens an external tag. The new protected-local-template test covers a pin seen first; it does not cover an unpinned-first/pinned-later requirement.
Collect and merge the integrity requirement for each resolved local script URL before inlining any bytes, then either verify those bytes before inlining or keep one protected external tag. Conflict detection also needs to happen before a chunk for that URL is committed. Please pin both root-unpinned + child-pinned local bytes and sibling/template unpinned-first + pinned-later; mismatched bytes must never appear in inline output.
Fresh focused suites are otherwise green: bun test htmlBundler.test.ts inlineSubCompositions.test.ts htmlCompiler.test.ts → 225 pass, 0 fail. Native CI is still running on this head.
— Magi
Verdict: REQUEST CHANGES
Reasoning: CDN requirements now survive reconstruction, but the core bundler can still execute unverified local bytes when an integrity requirement arrives after an earlier inline decision.
miguel-heygen
left a comment
There was a problem hiding this comment.
Final re-review at exact db10320fc04326b51a22cfb49daf59280106fccb. All three prior security blockers are closed.
- Supported SRI tokens are parsed case-insensitively and canonicalized to lowercase before browser fallback; raw fetched bytes are checked before decoding. A verified mismatch raises through
Promise.allSettledand aborts compilation, while ordinary network/HTTP failures retain their existing external-tag fallback. scriptItemscarries integrity/CORS through nested and template extraction. Producer and core reconstruct through the same helper; an exact-URL child pin strengthens existing root duplicates, later unpinned references retain it, and distinct non-empty requirements fail before output.- The core bundler now defers every local inline decision until sub-composition/template requirements are collected. Resolved-path comparison joins
local.js/./local.js; root, sibling, and template unpinned-first cases remain external once any later pin exists. My exact altered-file probe now returnsalteredInline: false, with every surviving alias tag carrying canonicalsha384-…. Unprotected chunks retain their original document/chunk order. - The nine alert mappings and CDN SHA-384 values were independently verified earlier against exact cdnjs/npm bytes. No PR-ref code-scanning alerts or unresolved review threads remain.
Fresh focused evidence: bun test packages/core/src/compiler/htmlBundler.test.ts packages/core/src/compiler/inlineSubCompositions.test.ts packages/producer/src/services/htmlCompiler.test.ts → 228 pass, 0 fail. Every applicable native check is now green, including JavaScript CodeQL, full Test, build/type/lint/Fallow, producer unit/integration, catalog/preview, Windows, perf, and all regression shards.
Non-blocking compatibility note: resolved local aliases remain as separate protected external tags, preserving the pre-existing duplicate execution count rather than silently changing authored behavior.
— Magi
Verdict: APPROVE
Reasoning: Integrity requirements now survive every direct, nested, template, duplicate, alias, and local-inline boundary audited; mismatches cannot return executable fallback HTML, and all focused and native evidence is green.
CDN-backed compositions could execute changed third-party scripts, and producer compilation fetched and inlined scripts without checking their
integrityattribute. Pin the existing GSAP 3.12.2 and Three.js r128 bytes in nine compositions and verify supported SRI digests before decoding or inlining downloaded scripts. Library versions and script contents stay unchanged. A verified integrity mismatch aborts compilation, producing no executable fallback HTML. Ordinary network/HTTP failures retain the existing fallback behavior. Nested and template composition hoisting carries integrity/CORS attributes into both preview and producer output. A child pin strengthens existing same-URL root tags; conflicting nonempty integrity strings fail explicitly rather than discarding a requirement. Protected local scripts remain external for browser verification.Alert mapping:
requestAnimationFramewarning when processing GSAP script in nested compositions #402: four liquid-glass registry blocks using Three.js r128.Validation: CDN bytes independently match npm release artifacts. Full workspace build, producer typecheck, 135 compiler tests and 2,711 core tests, the full producer Bun unit lane, and Fallow pass. The new regression fails the original compiler. Real Chromium executes both exact library payloads and blocks modified responses; the actual compiler accepts/rejects the same payloads under Node and Bun. All five producer fixtures pass CLI lint/check. All four registry blocks pass lint, runtime, layout, and motion with their bundled assets and library dependencies preserved; the full check remains red only for pre-existing contrast findings, reproduced on both baseline and fixed HTML.
Local broader-suite limitations: the Vitest producer unit lane has 663 passing tests, one skip, and one unchanged audio loudness failure (3.9 LUFS difference versus a 3 LUFS limit), reproduced with the original compiler. Integration Vitest has 77 passing tests; the Bun integration runner reaches cross-worker tests that require beginframe capture, while this macOS browser selects screenshot capture. These are not claimed green; native CI must pass before merge. No workflow or threshold changes.
Nested validation exercises real bundleToSingleHtml and compileForRender paths, including an unpinned root duplicate, valid versus tampered CDN bytes, duplicate pin retention, conflicting pins, and protected local template scripts. The nested tampering regression fails the previous head. Full workspace build, core/producer types, and change-scoped Fallow pass.
Local script inline decisions are deferred until nested/template requirements are collected. Resolved-path matching covers aliases such as local.js and ./local.js; root, sibling, and template unpinned-first regressions all fail the earlier implementation and pass now. Emitted supported algorithm tokens are normalized to lowercase for browser enforcement. Unprotected script chunk order is retained.