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

Add Trusted Types support #32209

Closed
timneutkens opened this issue Dec 7, 2021 · 3 comments
Closed

Add Trusted Types support #32209

timneutkens opened this issue Dec 7, 2021 · 3 comments
Assignees
Labels
Navigation Related to Next.js linking (e.g., <Link>) and navigation. type: chrome

Comments

@timneutkens
Copy link
Member

timneutkens commented Dec 7, 2021

Describe the feature you'd like to request

Backstory and initial implementation can be found here: #13509

Describe the solution you'd like

As mentioned in #13509 Trusted Types helps prevent XSS attack, we're planning to add support for it in Next.js natively so that there is a default configuration.

Someone from the Chrome team is going to start working on adding Trusted Types to Next.js early January.

@jgoping
Copy link
Contributor

jgoping commented Feb 2, 2022

The pull request above is to add a tool called tsec to the linting process to check for Trusted Types violations. Going forward, I plan to create more pull requests to fix all of the violations detected by tsec. Also, tsec is not guaranteed to catch all violations, so I plan to find and fix violations that occur when running Next.js applications locally or from running unit/integration tests. After all of these violations are fixed, then application developers can choose to enforce Trusted Types without being blocked by the framework.

@kara kara assigned jgoping and unassigned kara Feb 14, 2022
kodiakhq bot pushed a commit that referenced this issue May 3, 2022
Linked to issue #32209.

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation
There is one tsec violation that is fixed in this PR:
### 1. ban-script-src-assignment: route-loader.ts
XSS can occur with the line script.src = src in appendScript(src, script) if src can be controlled by a malicious user. From tracing through the code, it was determined that src comes from the function `getFilesForRoute(route)`. The behaviour of this function differs depending on the environment (development vs. production), but in both cases the function will construct strings that lead to valid file paths. These strings depend on two variables: `assetPrefix` and `route`, but due to the nature of the constructed strings it was determined that the scripts here are safe to use. Thus, the solution was to promote these strings to `TrustedScriptURL`s. This is the Trusted Types way of declaring that the script URL passed to the DOM sink is safe from DOM XSS attacks.

To create a `TrustedScriptURL`, a policy needs to be created. This policy was put in its own file: `client/trusted-types.ts`. This policy has the name `nextjs`. If this name should be changed to something else, feel free to change it now. However, once it is released to the public and application developers begin using it, it may be harder to change the value since any application developers with a custom policy name allowlist would now need to update their `next.config.js` headers to allow this new name.

The code was tested in a sample application to ensure it behaved as expected.
kodiakhq bot pushed a commit that referenced this issue May 5, 2022
Linked to issue #32209.

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation
There are three Trusted Types violations that are fixed in this PR:
### 1. ban-element-innerhtml-assignments: maintain--tab-focus.ts
The innerHTML assignment here is unsafe as a string is being used that could contain an XSS attack. The solution chosen was to replace the string containing HTML with programmatically-created DOM elements. This removes the Trusted Types violation as there is no longer a string passed in that can contain an XSS attack.

Notes on solution:
-  The `<svg>` tag is omitted completely since the original snippet returns fragment.firstChild.firstChild. The first firstChild omits the `<div>`, and the second firstChild omits the `<svg>`, so to remove unnecessary code the created elements start at the foreignObject level.
-  The reason createElementNS is used instead of createElement is because the ‘foreignObject’ element is a separate namespace from the default HTML elements. The documentation for this command is found [here](https://developer.mozilla.org/en-US/docs/Web/API/Document/createElementNS).

The code was tested to be equivalent by rendering both the original code and the re-written code in a browser to see if they evaluate to the same thing in the DOM. The DOM elements styles were then compared to ensure that they were identical.

### 2. ban-window-stringfunctiondef: packages/next/lib/recursive-delete.ts
The setTimeout function caused a Trusted Types violation because if a string is passed in as the callback, XSS can occur. The solution to this problem is to ensure that only function callbacks can be passed to setTimeout. There is only one call to the sleep function and it does not involve a string callback, so this can be enforced without breaking the application logic. In the process of doing this, promisify has been removed and the promise has been created explicitly.

The code was tested in a sample application to ensure it behaved as expected.

### 3. ban-window-stringfunctiondef: packages/next/client/dev/fouc.ts
This file also uses setTimeout, so the call was wrapped in a `safeSetTimeout` call that specifies that the callback argument is not a string.
kodiakhq bot pushed a commit that referenced this issue May 22, 2022
Linked to issue #32209.

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation
The purpose of this PR is to enable Trusted Types compatibility in Webpack. When the app is run in development mode, Webpack is currently set to use an [eval-source-map](https://github.com/vercel/next.js/blob/5a16b1a26f6c213776deed9ac465e2bc81cdf5f3/packages/next/build/webpack/config/blocks/base.ts#L33). This source map involves passing raw strings to `eval()` calls, which raise Trusted Types violations. The solution to this problem is to set `webpack5Config.output.trustedTypes` in the Webpack config. As shown in the documentation [here](https://webpack.js.org/configuration/output/#outputtrustedtypes), setting this value to a string will create a Trusted Types policy with the specified name. By creating a policy within Webpack, the raw strings passed to the `eval()` calls will be promoted to be of type `TrustedScript`. The issue where this was addressed in Webpack can be found [here](webpack/webpack#14075).

### Note:
The policy name that is set in the Webpack config is currently `nextjs#bundler`. Once it is released to the public and application developers begin using it, it may be harder to change the value since any application developers with a custom policy name allowlist would now need to update their next.config.js headers to allow this new name. Thus, a good name should ideally be determined before this pull request is merged. The reason that `nextjs#bundler` is preferred over `nextjs#webpack` is in case Next.js moves to a different bundler in the future. Having a generic name would allow for application developers to keep their next.config.js file the same after the bundler switch has occurred. If a different name is preferred, feel free to comment what that would be.

The code was tested in a sample application to ensure it behaved as expected.

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
@manuel-mauky

This comment has been minimized.

@samcx
Copy link
Member

samcx commented Feb 28, 2024

Moving this to :nextjs: discussions since this is a feature request.

@vercel vercel locked and limited conversation to collaborators Feb 28, 2024
@samcx samcx converted this issue into discussion #62618 Feb 28, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Navigation Related to Next.js linking (e.g., <Link>) and navigation. type: chrome
Projects
None yet
Development

No branches or pull requests

5 participants