Baroshem / nuxt-security

🛡 Automatically configure your app to follow OWASP security patterns and principles by using HTTP Headers and Middleware
https://nuxt-security.vercel.app/
MIT License
737 stars 56 forks source link

fix(csp): inline script/style have whitespace character #478

Closed hlhc closed 2 days ago

hlhc commented 1 week ago

Types of changes

Description

This PR updates the regular expressions in the 30-cspSsgHashes.ts file. The previous regular expression was not correctly capturing the content of inline script and style tags in all scenarios.

The old regular expression for inline scripts:

const INLINE_SCRIPT_RE = /<script(?![^>]*?\bsrc="[\w:.\-\\/]+")[^>]*>(.*?)<\/script>/gi

The updated regular expression:

const INLINE_SCRIPT_RE = /<script(?![^>]*?\bsrc="[\w:.\-\\/]+")[^>]*>([\s\S]*?)<\/script>/gi;

The change from (.*?) to ([\s\S]*?) ensures that the regular expression matches any character, including newlines, between the <script> and </script> tags. This change improves the accuracy of inline script content capture, ensuring that our CSP security hashes are correctly generated for all inline scripts.

Same changes are made in inline styles.

Checklist:

vercel[bot] commented 1 week ago

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 Jun 17, 2024 11:06am
Baroshem commented 1 week ago

Hey @hlhc

Thanks for this PR. @vejja @GalacticHypernova what are your thoughts on this? :)

vejja commented 1 week ago

Looks great Should we do the same for all regexes, not just the script one ?

GalacticHypernova commented 1 week ago

Just checked, works as expected. The wildcard appears to have a limitation so that was a good spot (ideally any character should whitespace, as per the definition of any character). I don't see a point in adding this to all regexes besides inline scripts. Maybe inline styles but no one ever reported an issue on it so I'll first check it and whether it accepts it or not. Links for sure don't need it because it's a one liner. Though I am wondering whether or not it's even required, because the code is transformed and minified by Nuxt before the runtime plugins apply, as part of the build time. @hlhc could you please share a reproduction with such inline scripts not being given a csp hash so I could experiment with some things?

vejja commented 1 week ago

inline styles can have newlines too

GalacticHypernova commented 1 week ago

Oh I know they can have newlines, I'm asking whether the nuxt minimization process takes care of these newlines or not

hlhc commented 1 week ago

Could you please share a reproduction with such inline scripts not being given a csp hash so I could experiment with some things?

I'll provide the reproduction later.

I'm asking whether the nuxt minimization process takes care of these newlines or not

Based on my research in unjs repos, there shouldn't be any minimization process inside unhead.

Looks great, Should we do the same for all regexes, not just the script one?

Yes, I missed that in this PR since I only having problem in inline scripts. I think it should also added to inline styles in case any linebreaks or whitespace inside.

GalacticHypernova commented 1 week ago

Based on my research in unjs repos, there shouldn't be any minimization process inside unhead.

It could also be handled inside vite or directly in nuxt, or maybe nitro if it's during the ssr render. That's why I would like to experiment with it

I'll provide the reproduction later.

Sounds good!

hlhc commented 1 week ago

It could also be handled inside vite or directly in nuxt, or maybe nitro if it's during the ssr render. That's why I would like to experiment with it

The HTML should only be handled inside Nuxt with Nitro. Inline styles will be built with Vite (with feature.inlineStyles enabled). There are still potential situations where there will be line breaks, for example, preserving license comments (not sure if its enabled or not), which are usually multiline.

GalacticHypernova commented 1 week ago

I assume that would differ between projects and specific nuxt configurations, maybe with sourcemaps enabled/disabled, which is why the best way to check it would be the reproduction of the issue that you encountered

hlhc commented 1 week ago

Reproduction: https://github.com/hlhc/csp-reproduce

moshetanzer commented 3 days ago

any timeframe for this merge as currently this break ui pro with default csp

Baroshem commented 3 days ago

Hey, I am up for releasing it even tomorrow as a next RC version if you agree :)

GalacticHypernova commented 3 days ago

Looks good on my end as well (sorry, was quite busy lately)

Baroshem commented 2 days ago

Published in https://github.com/Baroshem/nuxt-security/releases/tag/v2.0.0-rc.7. Thanks guys for the amazing work!