-
Notifications
You must be signed in to change notification settings - Fork 33
Content Security Policy (CSP) #1168
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
base: main
Are you sure you want to change the base?
Conversation
bbed778 to
85545c3
Compare
|
@ematipico everybody is calling hashes, this can be nice, however wouldn't be nice to use a simple nonce generated by the user. and that the user can assign it to for instance Astro.cspNonce. In that case it would be very easy I guess ? But in the meantime will there be a possible workaround ? |
|
@NepCono in the RFC I explained why nonce wouldn't work for Astro. Did you read that part? |
|
@ematipico just read about the nonce, very understandable :-) I was just saying that the nonce solution would be more simplified than the hash. However still a question remaining would the out of the box implementation security > csp and strictdynamic > true that we dont have to insert a nonce on the script (not a server island:script) which will lead to using typescript instead of javascript ? btw; when will this be released and can I access it earlier ? |
proposals/0055-csp.md
Outdated
| Include any useful background detail that that explains why this RFC is important. | ||
| What are the problems that this RFC sets out to solve? Why now? Be brief! | ||
|
|
||
| It can be useful to illustrate your RFC as a user problem in this section. | ||
| (ex: "Users have reported that it is difficult to do X in Astro today.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from the default template, we can probably extract things from the original discussion
|
|
||
| - Provide *multiple* solutions to fix the same problem | ||
| - Support for third-party scripts dynamically imported by users e.g. `document.createElement("script")`, `import("https://path.to/script.js")`; | ||
| - Support for third-party styles dynamically imported by users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I undestand correctly, injectCspAsset would allow this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to rephrase it then. What I meant is that out of the box Astro won't create a hash for the dynamically imported script/style. Any suggestions?
proposals/0055-csp.md
Outdated
| Users can add an entire directive. Astro won't check if the directive is grammatically correct. | ||
|
|
||
| ```js | ||
| Astro.insertCspPolicy(`image-src '${Astro.site}'`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this API covers injecting hashes directly eg.
Astro.insertCspPolicy(`script-src 'self' '${myHash}'`)I like this, it's better then my proposal on Discord to have it on insertCspAsset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can't provide such API, in fact I think I will try to make it more strict. Astro must control the script-src and style-src directives, because that's where we append all the hashes. If a user provides their own directive, they override ours, and it's all for nought
|
|
||
| # Drawbacks | ||
|
|
||
| The hashes solution will negatively impact the rendering engine because Astro will have to calculate the hashes **at runtime**. The most expensive solution is the support of server islands because the contents of their scripts depend on props and slots, which are only known during the rendering phase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know how big the impact is? Genuinely curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but we can set up a codspeed benchmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a comparable solution, it would take around 500ms for dozens of pages in a Netlify Free tier build. This should be the base performance
Co-authored-by: Florian Lefebvre <[email protected]>
|
I changed the APIs for additional directives. I believe the new API is better because counts for better DX: |
|
We also have to keep in mind, that in order to work out of the box. If I am correct the CSP policy will be configured in astro.config.mjs and that the CSP is handled by Astro. |
Co-authored-by: Marocco2 <[email protected]>
|
As discussed in the API bash, view transitions break when using the CSP settings without setting |
proposals/0055-csp.md
Outdated
| ``` | ||
|
|
||
|
|
||
| #### `Astro.insertScriptHash(string)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to expose utilities to compute the hash, so it can be fed to this function eg.
import { generateHash } from 'astro/csp'
const hash = await generateHash(content, 'SHA-256')
Astro.insertScriptHash(`sha256-${hash}`)Naming is bad but you get the idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this API isn't useful, as it essentially creates some unusual usage. Let's say you have some script.js somewhere. What would happen is the following:
- The user must import
script.jssomewhere, and provide its content as a string to thegenerateHash - The generated hash must be saved or sent somewhere
- Then the user must provide this hash to Astro via configuration or runtime
- This very script must be added to the application
Let's wait and see.
Co-authored-by: Matt Kane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on the proposal! Really exited with the new Header and adapter support 🙌
It could be a benefit to document this:
Authors are strongly encouraged to place meta elements as early in the document as possible, because policies in meta elements are not applied to content which precedes them. In particular, note that resources fetched or prefetched using the Link HTTP response header field, and resources fetched or prefetched using link and script elements which precede a meta-delivered policy will not be blocked.
– https://www.w3.org/TR/CSP3/#meta-element
Would also be valuable to make sure the Astro puts it on the top of head?
Seems from my quick testing it is added at the bottom of <head />:

proposals/0055-csp.md
Outdated
|
|
||
| The mapping is defined as `Map<IntegrationResolvedRoute, string>`, and it's the list of **static routes**, where: | ||
| - `IntegrationResolvedRoute` is the route type provided to the integrations; | ||
| - `string` is the **content** of the entire CSP header e.g. `"script-src: 'self' sha256-yadayada; style-src: 'self' sha256-yadayada` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - `string` is the **content** of the entire CSP header e.g. `"script-src: 'self' sha256-yadayada; style-src: 'self' sha256-yadayada` | |
| - `string` is the **content** of the entire CSP header e.g. `"script-src 'self' sha256-yadayada; style-src 'self' sha256-yadayada` |
Directive should not have a : between key and value
Co-authored-by: Johannes Andersen <[email protected]>
|
Thank you @Johannes-Andersen, I applied your suggestions. It makes more sense to have the user write their format (apostrophe VS no apostrophe). Since this is an advanced API, we can reasonably assume that users are familiar with its usage. I will have to send a PR to change the APIs. |
Co-authored-by: Johannes Andersen <[email protected]>
|
@stefanmaric Thank you for your suggestion. Unfortunately, this API wouldn't play well in Astro, where we need more control over the |
|
Great implementation as far as I can tell. Two things I want to throw into the discussion: NamingCurrently the Runtime APIs do not really reflect in their naming that they have something to do with CSP. Maybe they need a kind of prefix, eg. Disable one of the twoCurrently it is only possible to activate both |
|
Or maybe |
|
If in your Astro config, you have |
|
@Eveeifyeve can you share a reproduction? |
|
This is expected. CSP doesn't work in development. It's explicitly stated in this RFC and the documentation |
Since it's an insertion, this API will add an item to the existing array. If that's incorrect, what do you expect from the API? |
This comment has been minimized.
This comment has been minimized.
|
I would expect it to merge, like what we did for fonts basically |
Why though? I'm not saying it's incorrect, but |
This comment has been minimized.
This comment has been minimized.
Sorry I was on phone so it wasn't very clear. My point is not about deduplication but rather avoiding loosing directives. Taking my example again:
Here are the 3 outcomes:
What do you think? |
This comment has been minimized.
This comment has been minimized.
|
Hello, I was pointed in this direction from the discord, in relation to an inquiry about use cases of, "style-src-attr". Repo, https://github.com/cleancommunities/csp-test (https://discordapp.com/channels/830184174198718474/830184175176122389/1411782698720170046) |
|
I would go for the third option, however I think we should change the APIs, the make the merging safer and less prone to error on our side e.g. Astro.csp.insertDirective("img-src", "whatever") |
|
Not necessarily, we already have the logic for it with |

Summary
Links