Skip to content

Issue-618: SSG build fails if cdnUrl is relative path #622

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 1 commit into
base: main
Choose a base branch
from

Conversation

TomHeinemeyer
Copy link

If the cdnUrl in the Nuxt config is not a valid url, e.g. a relative path like ./ a call to the URL constructor fails because it expects a valid url. In this case just the cdnUrl is used.

Copy link

vercel bot commented Apr 30, 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 Apr 30, 2025 9:20pm

Copy link
Owner

@Baroshem Baroshem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey,

Great work!

Would you be able to fix the linting? The only necessary changes are in lines 332 while others are not needed.

Apart from that, it is ready to be merged :)

@vejja
Copy link
Collaborator

vejja commented May 5, 2025

Are we sure a relative cdnURL is valid?

@TomHeinemeyer
Copy link
Author

TomHeinemeyer commented May 5, 2025

Are we sure a relative cdnURL is valid?

In our deployment it works and Nuxt also doesn't complain. It is just prepended to the path of the assets, so an actual cdn URL will work as well as local path. In the end the browser will just try to load the assets from whatever we give it and it is our duty to provide a valid path.

@TomHeinemeyer
Copy link
Author

TomHeinemeyer commented May 5, 2025

Hey,

Great work!

Would you be able to fix the linting? The only necessary changes are in lines 332 while others are not needed.

Apart from that, it is ready to be merged :)

Seems like linting wasn't enabled in my IDE, thanks for spotting this. I will push a fix soon 💪🏼

@vejja
Copy link
Collaborator

vejja commented May 5, 2025

@TomHeinemeyer The official Nuxt docs require an absolute URL https://nuxt.com/docs/api/nuxt-config#cdnurl

I'm not against supporting it if it works for you, however we would need to think carefully about not breaking things further down the line because the SRI hash map is defined relative to buildAssetsDir which itself could be based on cdnURL (but not always: see https://nuxt.com/docs/api/nuxt-config#buildassetsdir).
Say you define cdnURL as '../foo', assuming your deployment environment deploys on https://example.com/bar, would we still resolve the sriHashes correctly ?

Would be really wonderful if you can include some test cases around this and also have a look at #615 to make sure both PRs do not conflict?

@TomHeinemeyer
Copy link
Author

TomHeinemeyer commented May 5, 2025

@TomHeinemeyer The official Nuxt docs require an absolute URL https://nuxt.com/docs/api/nuxt-config#cdnurl

I'm not against supporting it if it works for you, however we would need to think carefully about not breaking things further down the line because the SRI hash map is defined relative to buildAssetsDir which itself could be based on cdnURL (but not always: see https://nuxt.com/docs/api/nuxt-config#buildassetsdir).
Say you define cdnURL as '../foo', assuming your deployment environment deploys on https://example.com/bar, would we still resolve the sriHashes correctly ?

Would be really wonderful if you can include some test cases around this and also have a look at #615 to make sure both PRs do not conflict?

I see, wasn't aware of that. I was just thinking of the deployed application. I will try to make up some test cases for that and will also have a look into the other PR 👍🏼

@Baroshem
Copy link
Owner

Baroshem commented Jul 9, 2025

Hey @TomHeinemeyer

Have you maybe looked at it? :)

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