Skip to content

fix(sri): resolve hashes using cdnURL at runtime instead of build-time #615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maxmaxme
Copy link

@maxmaxme maxmaxme commented Apr 3, 2025

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR fixes a bug in Subresource Integrity (SRI) generation when using dynamic CDN URLs.

Currently, nuxt-security uses the cdnURL value at build time to construct the keys for the #sri-hashes map. If the same build is deployed to different environments with different CDN base URLs, the computed src / href at runtime does not match any entry in the #sri-hashes, and integrity is not applied.

This change defers the use of cdnURL to runtime, stripping it from the resource URL before matching it against the #sri-hashes map. This makes SRI compatible with multi-environment deployments using different CDN base URLs set via runtime config (NUXT_APP_CDN_URL).

Before:

  • #sri-hashes keys were generated using cdnURL at build time
  • runtime matching failed if cdnURL changed

After:

  • #sri-hashes keys are built without cdnURL
  • at runtime, actual resource URLs are normalized by removing cdnURL before lookup

This allows a single build to work reliably across multiple environments with different CDN domains.

Checklist:

  • My change requires a change to the documentation. (no)
  • I have updated the documentation accordingly. (no behavior changed)
  • I have added tests to cover my changes (already exists but fails even in the main branch)

Copy link

vercel bot commented Apr 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuxt-security ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 5, 2025 3:57pm

@Baroshem
Copy link
Owner

@vejja

Could you take a look at it? I think you have more experience in the SRI part :)

@vejja
Copy link
Collaborator

vejja commented May 5, 2025

Sorry for the delay
This looks alright in principle.
The only things that would make it even more brilliant are:

  • there used to be an extra check for whether cdnURL was provided with a trailing slash or not. I can remember it arose because of one user reporting the issue. This check has now disappeared, would it still work irrespective of the trailing slash?
  • would it be possible to include some basic tests in the test suite to check that it works?

@maxmaxme let me know what you think on these 2

@maxmaxme
Copy link
Author

maxmaxme commented May 5, 2025

@vejja, hi!

  1. Yes, a trailing slash in the cdnURL breaks integrity. I deleted it.
  2. Would be nice. Added a test

@vejja
Copy link
Collaborator

vejja commented May 5, 2025

Thanks @maxmaxme
We have another open PR that touches a different aspect of cdnURL : PR #622 - would you mind checking whether it should be backported now into your code?
Also I'm having doubts: when reading the initial code, it looks like when a cdnURL option is provided, the baseURL value is being discarded. In your PR, it looks though that the two values are assumed to be concatenated. Am I misreading something here?

Edit: Also there was a check for the src key in the sriHashes hash map on whether isAbsolute was true or not. Apparently it was trying to remove a potential leading slash in the src key before concatenating with cdnURL - basically enforcing the slash to always be on cdnURL and never on baseURL. With the new code doing src.replace, are we having a potential issue when there is no leading slash in the src key?

Would be good to have a series of tests that cover having a combination of cdnURL/baseURL values, either not defined or with/without slashes

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.

3 participants