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.

Sign up for GitHub

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

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

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

wants to merge 3 commits into from
+63 −27

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)

@vercel
Copy link

vercel bot commented Apr 3, 2025 •
edited
Loading

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

Baroshem commented Apr 10, 2025

@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 •
edited
Loading

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 •
edited
Loading

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

@vejja vejja mentioned this pull request May 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maxmaxme @Baroshem @vejja