Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

website with csp header require-trusted-types-for 'script' don't work after release of chrome 83 #13228

Closed
tipra34 opened this issue May 22, 2020 · 8 comments
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@tipra34
Copy link

tipra34 commented May 22, 2020

Bug report

Describe the bug

script loading does not work for site with csp header require-trusted-types-for 'script' don't work after release of chrome 83.

this disables assigning of script.src to unsafe url (non trusted types)
[bug location] (https://github.com/zeit/next.js/blob/7eaf9b8bab4e93b891be89704399644eafe70e21/packages/next/client/page-loader.js#L265)

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. use chrome 83.
  2. goto here
  3. Click on the only link available on screen with text: clicking here will also not work
  4. See error on console

Expected behavior

it should work normally

Screenshots

screenshot

System information

  • OS: [ macOS, Windows]
  • Browser chrome
  • Version of Next.js: 9.X

Additional context

related blog

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels May 25, 2020
@TyMick
Copy link
Contributor

TyMick commented May 26, 2020

Looks like Trusted Types would be an important feature to support going forward. Considering the offending line in loadScript()...

https://github.com/zeit/next.js/blob/d94e8db531cd6af33aa650e3478b7fb45dcfcecc/packages/next/client/page-loader.js#L265

...the easiest way to turn script.src into a Trusted Type may be by using DOMPurify to sanitize url à la

import DOMPurify from "dompurify"
...
script.src = DOMPurify.sanitize(url)

That does add a new dependency, though... Would it be better to create a custom Trusted Types policy? According to that web.dev article, that would be implemented something like

if (window.trustedTypes && trustedTypes.createPolicy) { // Feature testing
  const escapeHTMLPolicy = trustedTypes.createPolicy('myEscapePolicy', {
    createHTML: string => string.replace(/\</g, '&lt;') // Escapes `<` characters to prevent the creation of new HTML elements
  })
}
...
script.src = escapeHTMLPolicy.createHTML(url)

So, DOMPurify? Custom policy? Something else?

@TyMick
Copy link
Contributor

TyMick commented May 27, 2020

@tipra34, could I take a look at your source files for https://arcane-refuge-12762.herokuapp.com so I can do a little testing on my end?

@tipra34
Copy link
Author

tipra34 commented May 27, 2020

@tywmick here is the code https://github.com/tipra34/next-trusted-types-issue
I have just added the header in metatag.

@tipra34
Copy link
Author

tipra34 commented May 27, 2020

So, DOMPurify? Custom policy? Something else?

I think custom policy would be good, since we dont need any additional features that dompurify provides.
Its just a few lines of code as opposed to a library which will increase bundle size.

@TyMick
Copy link
Contributor

TyMick commented May 27, 2020

Well, adding these lines to the beginning of loadScript() and testing your example with my locally compiled Next.js doesn't fix the problem:

// https://web.dev/trusted-types/#create-a-trusted-type-policy
if (window.trustedTypes && window.trustedTypes.createPolicy) {
  const escapeHTMLPolicy = window.trustedTypes.createPolicy(
    'myEscapePolicy',
    {
      createHTML: (string) => string.replace(/</g, '&lt;'), // Escapes `<` characters to prevent the creation of new HTML elements
    }
  )
  url = escapeHTMLPolicy.createHTML(url)
}

And actually, putting a few console.logs around loadScript() makes it look like the function isn't even being called.

The search continues.

@tipra34
Copy link
Author

tipra34 commented May 28, 2020

I added the following code at top of node_modules/next/dist/client/page-loader.js

const myPolicy = trustedTypes.createPolicy('my-policy', { // This code block needs security review, as it's capable of causing DOM XSS. createHTML(dirty) { return dirty.replace(/</g, '&lt;') }, createScriptURL(dirty) { return new URL(dirty, document.baseURI); }, });

and changed
script.src=url
to
script.src=myPolicy.createScriptURL(url)

build is working good. Dev server is not working because of webpack, its setting innerHTML and script src at multiple places.

@TyMick
Copy link
Contributor

TyMick commented May 28, 2020

Ah good catch! Got it working on my end with production builds, so I'll create a PR for that fix, at least.

As far as fixing the dev server, do you think that's something that needs to be fixed in webpack, or should we be able to overcome that on our end if we find all the places it's setting innerHTML and script src's?

@tipra34
Copy link
Author

tipra34 commented May 28, 2020

According to me there is no need for solving the development server issue. Since as far as the security headers are concerned, I would prefer them to be on nginx/apache server and not in meta tag.
If someone still chooses to add them in meta tag, they can always add check for environment and then only set the meta tag.

@Timer Timer removed the help wanted label Jun 3, 2020
@vercel vercel locked and limited conversation to collaborators Dec 7, 2021
@balazsorban44 balazsorban44 converted this issue into discussion #32241 Dec 7, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants