-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: fonts csp #12103
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
feat: fonts csp #12103
Conversation
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Hey Florian! Just a couple of quick initial questions here to start!
<p> | ||
|
||
**Type:** `string[]`<br /> | ||
**Default:** `[]`<br /> |
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.
Didn't we run into a similar tricky situation documenting something not too long ago where adding any value overrides (and removes) the default that needs to be added back in? 😅 What was that and how did we handle that?
I think what feels off here is that if you do nothing, by default you get the same result as if you added this property with 'self'
, right? So it's kind of like the default value is 'self'
and not nothing?
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.
Indeed! FYI I just copied the other properties so they would also need to be updated
**Default:** `[]`<br /> | |
**Default:** `["'self'"]`<br /> |
<meta | ||
http-equiv="content-security-policy" | ||
content=" | ||
font-src https://fonts.cdn.example.com; |
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.
Is this the same situation where doing this overrides and removes 'self'
? If so, feels like that should also be mentioned, since it will remove the default?
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.
In this case, it's merged with the config so we could show 'self'
if we wanted. Maybe @ematipico has an opinion on 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.
OK, so if this merges that's probably easier to document! We would just make sure that we describe it as "additive"
@sarah11918 this is ready for review! |
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.
OK, here is all my confusion for you, Florian! 😅
|
||
**Type:** `string[]`<br /> | ||
**Default:** `["'self'"]`<br /> | ||
<Since v="5.12.7" /> |
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.
Just checking whether this is releasing with the minor, or if before, then update this right before the patch release when known!
<Since v="5.12.7" /> | |
<Since v="5.14.0" /> |
</head> | ||
``` | ||
|
||
When using [the experimental Fonts API](/en/reference/experimental-flags/fonts/), font resources will be injected by Astro (based on [`build.assetsPrefix`](/en/reference/configuration-reference/#buildassetsprefix)) as well as style [hashes](#hashes). |
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.
Just checking that this doesn't require experimental fonts to also be configured? When you say "when using the experimental fonts API" that means that all of the above is just generally true, even if not using. But now here, we are additionally specifying something that is true only when using experimental fonts, right?
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.
Maybe this is not the right place for this info? But yeah basically there are 2 things:
- The addition to the CSP API, that can be used on its own
- Experimental fonts supporting CSP, which uses the new CSP API + injects styles as needed
</head> | ||
``` | ||
|
||
When using [the experimental Fonts API](/en/reference/experimental-flags/fonts/), font resources will be injected by Astro (based on [`build.assetsPrefix`](/en/reference/configuration-reference/#buildassetsprefix)) as well as style [hashes](#hashes). |
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.
re: "font resources will be injected by Astro based on..." Would it be correct to say something like below, that I think is a bit more explicit?
When using [the experimental Fonts API](/en/reference/experimental-flags/fonts/), font resources will be injected by Astro (based on [`build.assetsPrefix`](/en/reference/configuration-reference/#buildassetsprefix)) as well as style [hashes](#hashes). | |
When using [the experimental Fonts API](/en/reference/experimental-flags/fonts/), Astro will apply both your [`build.assetsPrefix`](/en/reference/configuration-reference/#buildassetsprefix) configuration as well as style [hashes](#hashes) to your font resources in the `<meta>` element. |
And then this prompts the question... what if I'm not using experimental fonts?? Do I have to do anything special? Will this "just work" as expected, or do I need to set something manually because I'm not using experimental fonts? Do we need an "Otherwise, you will need to...." follow up statement if someone has assets.buildPrefix
set, or needs hashes to apply?
Also just checking where it says in hashes
:
If you have external scripts or styles that aren’t generated by Astro, or inline scripts, this configuration option allows you to provide additional hashes to be rendered.
After the build, the element will include your additional hashes in the script-src and style-src directives:
Do these now need to be extended to include fonts? Is there an analogous font line for this example? Are fonts simply an extension of styles and use their hashes, or are there separate font hashes?
(And noting that if this is only true when using experimental fonts, then, is there anything anyone needs to know in this section if they are NOT using experimental fonts?)
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.
And then this prompts the question... what if I'm not using experimental fonts?? Do I have to do anything special?
To recap:
- You're using experimental fonts: Astro will adds things to both
style.hashes
andfontDirectiveResources
(takingassets.buildPrefix
) - You're not using experimental fonts: you can use
fontDirectiveResources
without any issues
So I'd say no.
Do these now need to be extended to include fonts?
I don't think so? Fonts only have directives, not hashes
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.
OK, but if Astro works differently when I am or am not using experimental fonts, and I probably want one particular outcome, then... how do I get that same desired result in both cases if Astro is working differently in each case? I'm not sure how they can work differently and yet "it's all good" in either case?
Like, is taking those things into consideration "good" or "bad"? (Is it more like "I probably want it consistent with how I have other things set up, and yay, using experimental fonts it's done for me so I don't have to do X, Y and Z manually when I'm not using experimental fonts!" or is this "Be careful! If you use experimental fonts, this will happen, unlike when you're not. You might have to undo it manually!")
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.
Ah I think I see what you mean now. Basically, experimental fonts are dealing with font directives and style hashes, because the way it works is it injects an inline script.
Now if you're not using experimental fonts, you may be just setting fontDirectiveResources
and then doing things in a css file, in which case the style hashes section already covers that.
So I guess the main difference is this:
- When using experimental fonts,
fontDirectiveResources
may get things injected fromassets.buildPrefix
- When using
fontDirectiveResources
without experimental fonts, you have to duplicate things fromassets.buildPrefix
if applicable
Does that help?
<p> | ||
|
||
**Type:** `(resource: string) => void`<br /> | ||
<Since v="5.12.7" /> |
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.
same note re: version, whatever it happens to be!
@@ -411,3 +456,30 @@ After the build, the `<meta>` element for this individual page will add your has | |||
" | |||
> | |||
``` | |||
|
|||
### `insertFontResource` |
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.
Does this require experimental Fonts API, or work differently with or without it?
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.
It works without experimental fonts, and works the same with or without
Co-authored-by: Sarah Rainsberger <[email protected]>
Description (required)
Document new APIs that allow fonts to work with csp
Related issues & labels (optional)
For Astro version:
???
. See astro PR #14154.(Doesn't require a minor, but won't be ready for 5.13.0 so will release sometime after)