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(refactor): use csp_nonce_html_transformer library #96

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

JakeChampion
Copy link
Contributor

@JakeChampion JakeChampion commented Nov 8, 2024

csp_nonce_html_transformer internally has a custom non-asyncify wasm build of lol-html which only includes the setAttribute method, as that is all that is needed to add nonce attributes to elements. This reduces the wasm size by ~70%. It is also using the latest v2.0.0 of lol-html.

csp_nonce_html_transformer is a standalone library for Deno which takes in a Response instance and will add CSP directives to the configured CSP header and will add nonce attributes to the response body.

Copy link

netlify bot commented Nov 8, 2024

Deploy Preview for csp-nonce ready!

Name Link
🔨 Latest commit f696b08
🔍 Latest deploy log https://app.netlify.com/sites/csp-nonce/deploys/67361a574c255400087aeeb4
😎 Deploy Preview https://deploy-preview-96--csp-nonce.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@ndhoule ndhoule left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, a few other questions:

  1. The main issue we're experiencing with the CSP extension right now is crashes at runtime; can you explain how this PR addresses that problem? Were you able to repro the problem and confirm that this changeset fixes it?
  2. The CSP extension/plugin are one of our most used extensions/plugins; given how large a set of changes this is, what's our testing/rollout strategy for these changes?
  3. Can you add the @netlify/composable-platform team as an admin to the https://github.com/netlify/csp_nonce_html_transformer repository?

manifest.yml Outdated Show resolved Hide resolved
src/__csp-nonce.ts Show resolved Hide resolved
src/__csp-nonce.ts Outdated Show resolved Hide resolved
Copy link

@ndhoule ndhoule left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, a few other questions:

  1. The main issue we're experiencing with the CSP extension right now is crashes at runtime; can you explain how this PR addresses that problem? Were you able to repro the problem and confirm that this changeset fixes it?
  2. The CSP extension/plugin are one of our most used extensions/plugins; given how large a set of changes this is, what's our testing/rollout strategy for these changes?
  3. Can you add the @netlify/composable-platform team as an admin to the https://github.com/netlify/csp_nonce_html_transformer repository?

@JakeChampion
Copy link
Contributor Author

@ndhoule unfortunately I don't have admin rights on https://github.com/netlify/csp_nonce_html_transformer/ - I think right now only Netlify GitHub Owners are admins... @eduardoboucas I see you are an owner, would you be able to add @netlify/composable-platform team as an admin to the https://github.com/netlify/csp_nonce_html_transformer repository 🙏

@JakeChampion
Copy link
Contributor Author

In addition to the inline comments, a few other questions:

1. The main issue we're experiencing with the CSP extension right now is crashes at runtime; can you explain how this PR addresses that problem? Were you able to repro the problem and confirm that this changeset fixes it?

Yes, the library uses a custom build of lol-html to wasm, specifically it uses a version which does not use asyncify, and asyncify was the cause of the crashes from what we could see in the logs - I've deployed this plugin to test sites which have pages of large (13MB+) html and pages of HTML which customers saw issues with, all those are behaving correctly for me, and I have given them a high load from a single POP and not seen any isolate crashes or errors thrown.

2. The CSP extension/plugin are one of our most used extensions/plugins; given how large a set of changes this is, what's our testing/rollout strategy for these changes?

We can release the plugin as a prerelease at first, and deploy it to some site that we own which get medium-to-large traffic, and then eventually promote it to a full release. Once at a full release, we would update the extension to use it.

@JakeChampion JakeChampion force-pushed the jake/library branch 3 times, most recently from b19630e to fb0a77a Compare November 14, 2024 10:25
csp_nonce_html_transformer internally has a custom non-asyncify wasm build of lol-html which only includes the setAttribute method, as that is all that is needed to add nonce attributes to elements. This reduces the wasm size by ~70%. It is also using the latest v2.0.0 of lol-html.

csp_nonce_html_transformer is a standalone library for Deno which takes in a Response instance and will add CSP directives to the configured CSP header and will add nonce attributes to the response body.
Copy link

@ndhoule ndhoule left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes!

@JakeChampion JakeChampion merged commit abefa61 into main Nov 15, 2024
6 checks passed
@JakeChampion JakeChampion deleted the jake/library branch November 15, 2024 09:48
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.

2 participants