-
Notifications
You must be signed in to change notification settings - Fork 774
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(runtime): support for CSP nonces #3823
Changes from 10 commits
2c79743
81b099a
2af9abc
9b2eeea
545f359
ea0db1b
ee06584
70b0fab
c6f9a01
7891b60
93dfedd
0d9ffbd
56a673f
c3a8da8
7d6d684
810c30f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import { win } from '../../../client-window'; | ||
import { addGlobalLink } from '../load-link-styles'; | ||
|
||
describe('loadLinkStyles', () => { | ||
describe('addGlobalLink', () => { | ||
global.fetch = jest.fn().mockResolvedValue({ text: () => '--color: var(--app-color);' }); | ||
|
||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it('should create a style tag within the link element parent node', async () => { | ||
const linkElm = document.createElement('link'); | ||
linkElm.setAttribute('rel', 'stylesheet'); | ||
linkElm.setAttribute('href', ''); | ||
|
||
const parentElm = document.createElement('head'); | ||
parentElm.appendChild(linkElm); | ||
|
||
await addGlobalLink(document, [], linkElm); | ||
|
||
expect(parentElm.innerHTML).toEqual('<style data-styles>--color: var(--app-color);</style>'); | ||
}); | ||
|
||
it('should create a style tag with a nonce attribute within the link element parent node', async () => { | ||
(win as any).nonce = 'abc123'; | ||
const linkElm = document.createElement('link'); | ||
linkElm.setAttribute('rel', 'stylesheet'); | ||
linkElm.setAttribute('href', ''); | ||
|
||
const parentElm = document.createElement('head'); | ||
parentElm.appendChild(linkElm); | ||
|
||
await addGlobalLink(document, [], linkElm); | ||
|
||
expect(parentElm.innerHTML).toEqual('<style data-styles nonce="abc123">--color: var(--app-color);</style>'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,6 +298,15 @@ export declare function getAssetPath(path: string): string; | |
*/ | ||
export declare function setAssetPath(path: string): string; | ||
|
||
/** | ||
* Used to specify a nonce value that corresponds with an application's CSP. | ||
tanner-reits marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* When set, the nonce will be added to all dynamically created script and style tags at runtime. | ||
* Alternatively, the nonce value can be set on the window object (window.nonce) and | ||
* will result in the same behavior. | ||
* @param nonce The value to be used for the nonce attribute. | ||
*/ | ||
export declare function setNonce(nonce: string): void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a format that a nonce should be in? I don't see any guidance in the JSDoc as to what I should pass in if I'm a user and looking at the documentation provided by my IDE 🤔 I wonder if there's anything we should do from a type perspective to enforce a certain format 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can definitely link out to guidelines around nonce generation, but it didn't seem like Stencil's responsibility to enforce a "correct" nonce. I found these guidelines on one of the pages I was using for reference during development/testing:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, perhaps this lives in the documentation to point to best practices around using a nonce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a guide drafted up that will go on our docs. I'll add a section for this topic and link out to that resource I was using. I'll get a PR up for that tomorrow. |
||
|
||
/** | ||
* Retrieve a Stencil element for a given reference | ||
* @param ref the ref to get the Stencil element for | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,5 +186,6 @@ export { | |
renderVdom, | ||
setAssetPath, | ||
setMode, | ||
setNonce, | ||
setValue, | ||
} from '@runtime'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ export { | |
setAssetPath, | ||
setErrorHandler, | ||
setMode, | ||
setNonce, | ||
setPlatformHelpers, | ||
State, | ||
Watch, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { plt } from '@platform'; | ||
|
||
/** | ||
* Assigns the given value to the nonce property on the runtime platform object. | ||
* During runtime, this value is used to set the nonce attribute on all dynamically created script and style tags. | ||
* @param nonce The value to be assigned to the platform nonce property. | ||
* @returns void | ||
*/ | ||
export const setNonce = (nonce: string) => (plt.$nonce$ = nonce); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably going to be covered by the docs, but is the assumption that users will have to change the nonce server-side for every request that's served? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorta. I'm not sure the best way to phrase it, but the idea is that a nonce should be unique "per page view". So, for SPA like an Angular app, you can just generate a nonce at initial bootstrap and use that for the page's lifetime |
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 wonder if grabbing a field called
nonce
onwindow
is the right thing to do... 🤔On one hand, the chances of it getting overwritten by other scripts is probably higher with a name of just 'nonce'. On the other, it would probably seem weird to see a server configure a
stencil_nonce
field.I suppose this is probably fine. Leaving it here in case anyone else wants to comment on
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.
Passing thought - instead of pulling this value off
window
, could we instead pull it as an attribute off thehtml
root element?To me, it seems the usage pattern here is currently:
<script nonce="value">
- for a single scriptwindow.nonce
- for everything in the DOMWhereas it could be:
<html nonce="value">
- for everything in the DOM<script nonce="value">
- for a single scriptConsistent DX with less brittleness tied to the
window
object.Do we have access to the root element in the execution context here?
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.
The way that Rails handles this is by adding a meta tag in the header which subsequent scripts can read from before injecting their content:
<meta name="csp-nonce" content="nonce_inserted_here">
I think this is preferable to setting on the main html tag (though I agree the DX is better to do that than window).
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.
@sean-perkins @cscorley Thanks for the feedback and suggestions! I like the idea of using the
meta
tag. I'll check it out and see the feasibility to replace thewindow
approach!