Skip to content
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

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

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

hlhc
Copy link
Contributor

@hlhc hlhc commented Jun 16, 2024

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 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:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Copy link

vercel bot commented Jun 16, 2024

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
Copy link
Owner

Hey @hlhc

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

@vejja
Copy link
Collaborator

vejja commented Jun 17, 2024

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

@GalacticHypernova
Copy link
Contributor

GalacticHypernova commented Jun 17, 2024

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
Copy link
Collaborator

vejja commented Jun 17, 2024

inline styles can have newlines too

@GalacticHypernova
Copy link
Contributor

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

@hlhc
Copy link
Contributor Author

hlhc commented Jun 17, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

hlhc commented Jun 17, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

hlhc commented Jun 17, 2024

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

@hlhc hlhc force-pushed the fix/ssg-hash-inline-regex branch from 7a10605 to 4aede1b Compare June 17, 2024 11:03
@hlhc hlhc changed the title fix(csp): inline script have whitespace character fix(csp): inline script/style have whitespace character Jun 17, 2024
@moshetanzer
Copy link
Contributor

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

@Baroshem
Copy link
Owner

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

@GalacticHypernova
Copy link
Contributor

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

@Baroshem Baroshem merged commit 1dd0496 into Baroshem:main Jun 24, 2024
3 checks passed
@Baroshem
Copy link
Owner

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

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.

None yet

5 participants