fix(bundle): content-address bundled script filenames for stable SRI#725
Merged
fix(bundle): content-address bundled script filenames for stable SRI#725
Conversation
Bundled script public URLs were hashed from the upstream src, so the filename stayed constant across deploys even when the fetched content (or proxy rewrites) changed. Long-cached JS then got served against a new integrity hash in fresh HTML, breaking SRI on the second deploy. Derive the public filename from a sha256 of the final bundled bytes so content changes always flip the URL, and drop the cached integrity hash (it is now computed fresh from cached content each build). Resolves #724
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds centralized helpers to sanitize hash-derived filenames and build asset URLs from Nuxt Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/transform.test.ts (1)
683-720:⚠️ Potential issue | 🟡 MinorReset cache mocks after this scenario to avoid order-dependent tests.
This test leaves the storage mocks configured as a fresh cache hit with
cachedContent. The following top-level tests then inherit that state and their951c324253eef4b3.jssnapshots depend on this test running first.🧪 Proposed isolation fix
- beforeEach(() => { + const resetBundleStorageMocks = () => { // Reset all mocks for bundleStorage mockBundleStorage.getItem.mockReset() mockBundleStorage.setItem.mockReset() mockBundleStorage.getItemRaw.mockReset() mockBundleStorage.setItemRaw.mockReset() mockBundleStorage.hasItem.mockReset() vi.clearAllMocks() - }) + } + + beforeEach(resetBundleStorageMocks) + afterEach(resetBundleStorageMocks)Also add
afterEachto the Vitest import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/transform.test.ts` around lines 683 - 720, This test leaves shared mocks in a "fresh cache" state and causes order-dependent failures; add an afterEach cleanup that resets all Vitest mocks and clears any shared renderedScript state. Specifically, import afterEach from Vitest alongside vi, then add afterEach(() => { vi.resetAllMocks(); mockBundleStorage.hasItem.mockReset?.(); mockBundleStorage.getItem.mockReset?.(); mockBundleStorage.getItemRaw.mockReset?.(); renderedScript?.clear?.(); }); so mockBundleStorage methods and the renderedScript Map are reset between tests and no test order dependency remains.
🧹 Nitpick comments (2)
test/unit/transform.test.ts (1)
1190-1240: Assert the exact hashes, not just the hash shape.These tests would still pass if the filename/integrity were any 16-hex value or any
sha384-*string. Since this PR is specifically about deriving both from final bytes, compare against the digest ofcachedContent/scriptContent.🧪 Proposed stronger assertions
- // Integrity is computed fresh from cached content, yielding a deterministic SRI hash. - expect(code).toMatch(/integrity: 'sha384-[A-Za-z0-9+/=]+'/) + const expectedIntegrity = createHash('sha384').update(cachedContent).digest('base64') + expect(code).toContain(`integrity: 'sha384-${expectedIntegrity}'`) @@ - expect(code).not.toContain('/_scripts/assets/beacon.min.js') - expect(code).toMatch(/\/_scripts\/assets\/[a-f0-9]{16}\.js/) + const expectedFilename = createHash('sha256').update(scriptContent).digest('hex').slice(0, 16) + expect(code).not.toContain('/_scripts/assets/beacon.min.js') + expect(code).toContain(`/_scripts/assets/${expectedFilename}.js`)Add this import near the top of the test file:
+import { createHash } from 'node:crypto'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/transform.test.ts` around lines 1190 - 1240, The tests currently only assert hash shape; instead compute the actual content digests from the final bytes and assert exact values: for the first case derive the expected SRI (sha384) from cachedContent (the Buffer assigned to cachedContent) and assert transform(...) output contains that exact integrity string; for the second case compute the expected 16-hex content filename from scriptContent (the Buffer assigned to scriptContent) and assert transform(...) output contains that exact filename pattern rather than any 16-hex placeholder. Locate uses of transform, cachedContent, scriptContent, mockBundleStorage and the mocked fetch to obtain the bytes to hash and replace the loose regex assertions with exact equality checks against the computed digests.packages/script/src/plugins/transform.ts (1)
142-176: Include all rewrite inputs in the cache identity.
cacheKeyonly hashesproxyRewrites, but the cached bytes also depend onsdkPatches,skipApiRewrites, andneutralizeCanvasat Line 166. Changing any of those while the cache is fresh can reuse stale rewritten JS. The sharedbundle-meta:${filename}timestamp also lets one variant refresh another variant’s expiry.♻️ Proposed cache identity adjustment
- const proxyRewritesHash = proxyRewrites?.length ? `-${ohash(proxyRewrites)}` : '' - const cacheKey = proxyRewrites?.length ? `bundle-proxy:${filename.replace('.js', `${proxyRewritesHash}.js`)}` : `bundle:${filename}` - const shouldUseCache = !forceDownload && await storage.hasItem(cacheKey) && !(await isCacheExpired(storage, filename, cacheMaxAge)) + const rewriteOptionsHash = proxyRewrites?.length + ? `-${ohash({ proxyRewrites, sdkPatches, skipApiRewrites, neutralizeCanvas })}` + : '' + const cacheKey = proxyRewrites?.length ? `bundle-proxy:${filename.replace(/\.js$/, `${rewriteOptionsHash}.js`)}` : `bundle:${filename}` + const metaFilename = cacheKey.replace(/^bundle(?:-proxy)?:/, '') + const shouldUseCache = !forceDownload && await storage.hasItem(cacheKey) && !(await isCacheExpired(storage, metaFilename, cacheMaxAge)) @@ - await storage.setItem(`bundle-meta:${filename}`, { + await storage.setItem(`bundle-meta:${metaFilename}`, { timestamp: Date.now(), src, - filename, + filename: metaFilename, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/script/src/plugins/transform.ts` around lines 142 - 176, The cache key currently only includes proxyRewritesHash (proxyRewritesHash, cacheKey) while the stored bundle bytes also depend on sdkPatches, skipApiRewrites, and neutralizeCanvas used by rewriteScriptUrlsAST; update the cache identity to incorporate a hash of proxyRewrites + sdkPatches + skipApiRewrites + neutralizeCanvas (or a combined optionsHash) so cacheKey uniquely represents all rewrite inputs, and ensure metadata is stored per-variant (use the same variant-aware key instead of bundle-meta:${filename}) so timestamp/expiry is tied to the exact cacheKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/unit/transform.test.ts`:
- Around line 683-720: This test leaves shared mocks in a "fresh cache" state
and causes order-dependent failures; add an afterEach cleanup that resets all
Vitest mocks and clears any shared renderedScript state. Specifically, import
afterEach from Vitest alongside vi, then add afterEach(() => {
vi.resetAllMocks(); mockBundleStorage.hasItem.mockReset?.();
mockBundleStorage.getItem.mockReset?.();
mockBundleStorage.getItemRaw.mockReset?.(); renderedScript?.clear?.(); }); so
mockBundleStorage methods and the renderedScript Map are reset between tests and
no test order dependency remains.
---
Nitpick comments:
In `@packages/script/src/plugins/transform.ts`:
- Around line 142-176: The cache key currently only includes proxyRewritesHash
(proxyRewritesHash, cacheKey) while the stored bundle bytes also depend on
sdkPatches, skipApiRewrites, and neutralizeCanvas used by rewriteScriptUrlsAST;
update the cache identity to incorporate a hash of proxyRewrites + sdkPatches +
skipApiRewrites + neutralizeCanvas (or a combined optionsHash) so cacheKey
uniquely represents all rewrite inputs, and ensure metadata is stored
per-variant (use the same variant-aware key instead of bundle-meta:${filename})
so timestamp/expiry is tied to the exact cacheKey.
In `@test/unit/transform.test.ts`:
- Around line 1190-1240: The tests currently only assert hash shape; instead
compute the actual content digests from the final bytes and assert exact values:
for the first case derive the expected SRI (sha384) from cachedContent (the
Buffer assigned to cachedContent) and assert transform(...) output contains that
exact integrity string; for the second case compute the expected 16-hex content
filename from scriptContent (the Buffer assigned to scriptContent) and assert
transform(...) output contains that exact filename pattern rather than any
16-hex placeholder. Locate uses of transform, cachedContent, scriptContent,
mockBundleStorage and the mocked fetch to obtain the bytes to hash and replace
the loose regex assertions with exact equality checks against the computed
digests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47cbfdcb-a74b-49d9-9d18-9ba07a066c42
📒 Files selected for processing (2)
packages/script/src/plugins/transform.tstest/unit/transform.test.ts
Guards against regression of URL-hash-only filenames producing SRI mismatches when upstream content changes between deployments.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/bundle-two-deploy-repro.test.ts`:
- Around line 4-8: The test currently only checks that an integrity string
starting with "sha384-" was emitted; instead compute the actual SHA-384 SRI of
the served bundle bytes and assert exact equality. In the test that uses
NuxtScriptBundleTransformer (the block around the existing integrity check), get
the emitted script content (the response body or the bundle string used to
generate the tag), compute the base64 SHA-384 digest (e.g., using Node's crypto:
createHash('sha384').update(content).digest('base64')) and compare that value
prefixed with "sha384-" to the integrity attribute parsed from the generated tag
(the variable currently asserted for presence). Replace the loose
regex/assertion with an equality assertion between the computed
"sha384-<base64>" and the integrity attribute so the hash matches the actual
bytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3963638f-c0c3-48dd-b86d-2f98a29a63b1
📒 Files selected for processing (1)
test/unit/bundle-two-deploy-repro.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Resolves #724
❓ Type of change
📚 Description
Bundled script public URLs were hashed from the upstream
src, so the filename stayed constant across deploys even when the fetched content (or proxy rewrites) changed. Long-cached JS at the unchanged URL then got served against a new integrity hash in fresh HTML, breaking SRI on the second deploy (notably reproducible with AdSense + bundle + proxy).The bundler now derives the public filename from a sha256 of the final bundled bytes, so any content change flips the URL and keeps SRI consistent. Cached integrity hashes are dropped from bundle metadata since integrity is now computed fresh from cached content on each build.