Allow passing a nonce to two third-party libraries #61820
Conversation
Vinnl
requested review from
a team,
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
Vinnl
requested review from
jh3y and
delbaoliveira
and removed request for
a team
Specifically, <GoogleAnalytics> and <GoogleTagManager>. I did not know how to add it to the YouTube and Google Maps embeds, since they do not use the <Script> tag.
Vinnl
force-pushed
the
61714-csp-third-parties
branch
from
310ebe6
to
8568d4f
Compare
| @@ -43,11 +50,13 @@ export function GoogleTagManager(props: GTMParams) { | |||
| ${dataLayer ? `w[l].push(${JSON.stringify(dataLayer)})` : ''} | |||
| })(window,'${dataLayerName}');`, | |||
There was a problem hiding this comment.
Google docs have a modified version of scripts when using a CSP, that seems to pass the nonce as an attribute. This might need to change
There was a problem hiding this comment.
@Vinnl Do you have the bandwidth to update your PR?
There was a problem hiding this comment.
Thanks for asking, but not really - I don't use GTM myself, so I'm not sure what should be needed here, and it doesn't look like this PR is going to get looked at anyway...
There was a problem hiding this comment.
Thanks for the reply. Basically need a small fix to the script according to @Stuj1's suggestion. Do you want to give it a try or I can raise another PR to fix that?
There was a problem hiding this comment.
Happy to merge a PR into mine, or for you to create a separate PR - whatever works for you!
There was a problem hiding this comment.
would be great if this can be used soon!
There was a problem hiding this comment.
Hi missed this comment originally. I am not able to add anything to this PR but I made the necessary fixes here:
There was a problem hiding this comment.
@zhawtof Maybe create a new PR with those changes? Then you can add own the changes and review feedback, and add a proper description describing the added changes and why they are needed?
balazsorban44
added
Documentation
| @@ -11,11 +13,13 @@ export type GTMParams = { | |||
| dataLayerName?: string | |||
| auth?: string | |||
| preview?: string | |||
| nonce?: ScriptProps['nonce'] | |||
There was a problem hiding this comment.
We can use simple type to avoid relying on next/script only for types
| nonce?: ScriptProps['nonce'] | |
| nonce?: string |
There was a problem hiding this comment.
Happy to change, but I figured this made more sense in case <Script> accepts different values for nonce in the future - is there a downside to importing from next/script?
As for your top-level comment:
can we add a test for it?
I mentioned above, I'd like to, but:
Unfortunately, my laptop froze when trying to run non-unit tests or build, so I haven't been able to do that. I also didn't find existing unit tests I could expand on. However, I did copy the code mozilla/blurts-server#4160, and it worked well there.
But if you have some pointers on what it would look like, I could see if I could give it a shot.
|
This PR seems stalled. I came here to create this exact PR. Let me know if I can help in any way. |
Specifically,
<GoogleAnalytics>and<GoogleTagManager>. I did not know how to add it to the YouTube and Google Maps embeds, since they do not use the<Script>tag directly and I didn't quite know how those are run, but this at least partially fixes #61714.Unfortunately, my laptop froze when trying to run non-unit tests or build, so I haven't been able to do that. I also didn't find existing unit tests I could expand on. However, I did copy the code into ours, and it worked well there.