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

x/pkgsite: Google Tag Manager is loaded twice #40321

Open
andybons opened this issue Jul 21, 2020 · 8 comments
Open

x/pkgsite: Google Tag Manager is loaded twice #40321

andybons opened this issue Jul 21, 2020 · 8 comments
Assignees
Labels
Milestone

Comments

@andybons
Copy link
Member

@andybons andybons commented Jul 21, 2020

The following snippet:

f = document.createElement("iframe")
f.src = gtmURL;
f.height = "0";
f.width = "0";
f.style = "display:none;visibility:hidden"
s = document.createElement("noscript");
s.appendChild(f);
document.head.appendChild(s);

Is meant to replace the following:

<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=GTM-W8MVQXG" height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>

However, the <noscript> tag is explicitly a fallback for when JavaScript is not available, so replacing it with JavaScript doesn’t achieve its desired purpose (and ends up loading the GTM snippet again within the iframe).

As I understand it, this was done for security reasons.

/cc @jba @jamalc

@andybons andybons added this to the Unreleased milestone Jul 21, 2020
@jba jba self-assigned this Jul 21, 2020
@jba
Copy link
Contributor

@jba jba commented Jul 21, 2020

That change was made in golang/pkgsite@1940919. The author didn't appreciate the meaning of noscript.

We still have the problem that we can't use nonces, so I'm not sure how to generate the iframe securely.
@empijei
@FiloSottile

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2020

Change https://golang.org/cl/243858 mentions this issue: content/static/html: remove bad JS

gopherbot pushed a commit to golang/pkgsite that referenced this issue Jul 21, 2020
There was an attempt to create a noscript tag, which only runs when JS
is disabled, using JS.

Remove it.

For golang/go#40321.

Change-Id: I99c02810ed7c299fb606259823ef9b764c525bb6
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/243858
Reviewed-by: Julie Qiu <julie@golang.org>
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jul 21, 2020

We still have the problem that we can't use nonces, so I'm not sure how to generate the iframe securely.

I am not the expert, but I think you can use nonces, as long as you wire them through the template so they only get applied where you intend them to be applied. I can review a CL that introduces nonces if you'd like.

@jba
Copy link
Contributor

@jba jba commented Jul 27, 2020

My understanding (from @empijei) is that the nonce isn't necessary.

But there is another problem with restoring the noscript iframe: the GTM ID is now in the DOM (rather than part of the template data). Can it be accessed without JS?

@jba
Copy link
Contributor

@jba jba commented Jul 27, 2020

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 29, 2020

Change https://golang.org/cl/245557 mentions this issue: content/static/html: restore GTM iframe

@andybons
Copy link
Member Author

@andybons andybons commented Jul 29, 2020

But there is another problem with restoring the noscript iframe: the GTM ID is now in the DOM (rather than part of the template data). Can it be accessed without JS?

Can we include it in the template data?

@julieqiu julieqiu changed the title pkgsite: Google Tag Manager is loaded twice x/pkgsite: Google Tag Manager is loaded twice Jul 29, 2020
@jba
Copy link
Contributor

@jba jba commented Jul 29, 2020

There was a reason we removed it in the first place...not sure what it was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.