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

Trusted Types tsec Integration #1

Closed
wants to merge 5 commits into from
Closed

Trusted Types tsec Integration #1

wants to merge 5 commits into from

Conversation

jgoping
Copy link
Owner

@jgoping jgoping commented Jan 26, 2022

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • 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

Motivation

This is a pull request to begin integrating Trusted Types into the Next.js codebase. Background information can be found here.

tsec is a wrapper around tsc that performs Trusted Types checking. Any violations it detects will be caught and returned as an error. In this pull request, tsec has been added to the linting process for the packages next and react-dev-overlay as these were identified to have Trusted Types violations. To avoid these errors from failing the lint checks, all of the violations have been added to exemption lists. In future pull requests, these violations will be fixed and removed from the exemption list.

Once Next.js is fully compatible with Trusted Types, application developers can choose to adapt to Trusted Types without being blocked by the framework.

Notes

  • Currently, tsec has been added to the lint-typescript process. However, it can also be checked during each build or moved into its own script.
  • tsec is added as a dependency to the root package.json, although it can alternatively be added to the next and react-web-overlay package.json files if this is seen as a better practice.

@uraj
Copy link
Collaborator

uraj commented Jan 27, 2022

Thanks for this! A few minor comments (can't comment in line; probably due to not being a reviewer)

  • In "Motivation," change the wording "vulnerability" to "violation." A TT violation isn't necessarily exploitable. Similarly, let's not say we are making next.js more secure. Instead, we're trying to make next.js compatible with TT such that application developers can choose to adapt to TT without being blocked by the framework.
  • Double check if tsc --noEmit --declaration emits any output. My guess is that --noEmit should override --declaration, but we want to be sure.
  • Rename exemption-list.json to something like tsec-exemption.json so that its purpose is more clear.
  • Squash the commits into a single one before we open the PR in the next.js repo.

Other than that it looks good!

@jgoping jgoping closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants