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

Fix various Trusted Types violations without use of policy #34726

Merged
merged 1 commit into from May 5, 2022

Conversation

jgoping
Copy link
Contributor

@jgoping jgoping commented Feb 23, 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.
  • 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.

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.

@jgoping jgoping force-pushed the tt-violation-fixes-without-policy branch 2 times, most recently from aab495e to c164fca Compare February 25, 2022 15:37
@ijjk
Copy link
Member

ijjk commented Feb 25, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
buildDuration 15.2s 15.1s -87ms
buildDurationCached 6.1s 6.2s ⚠️ +6ms
nodeModulesSize 475 MB 475 MB ⚠️ +334 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
/ failed reqs 0 0
/ total time (seconds) 3.497 3.47 -0.03
/ avg req/sec 714.87 720.41 +5.54
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.154 1.166 ⚠️ +0.01
/error-in-render avg req/sec 2166.36 2144.45 ⚠️ -21.91
Client Bundles (main, webpack)
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
925.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 28.8 kB 28.8 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 72.5 kB 72.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 308 B 308 B
css-HASH.js gzip 327 B 327 B
dynamic-HASH.js gzip 3.08 kB 3.08 kB
head-HASH.js gzip 359 B 359 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.71 kB 5.71 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.64 kB 2.64 kB
routerDirect..HASH.js gzip 320 B 320 B
script-HASH.js gzip 391 B 391 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16.3 kB 16.3 kB
Client Build Manifests
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
index.html gzip 530 B 530 B
link.html gzip 544 B 544 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
buildDuration 17.2s 17.2s ⚠️ +10ms
buildDurationCached 6.1s 6.1s -2ms
nodeModulesSize 475 MB 475 MB ⚠️ +334 B
Page Load Tests Overall increase ✓
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
/ failed reqs 0 0
/ total time (seconds) 3.493 3.506 ⚠️ +0.01
/ avg req/sec 715.76 713.03 ⚠️ -2.73
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.159 1.151 -0.01
/error-in-render avg req/sec 2157.76 2172.73 +14.97
Client Bundles (main, webpack)
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
925.HASH.js gzip 178 B 178 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 29.2 kB 29.2 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 73.1 kB 73.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 179 B 179 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 324 B 324 B
dynamic-HASH.js gzip 3.08 kB 3.08 kB
head-HASH.js gzip 357 B 357 B
hooks-HASH.js gzip 921 B 921 B
image-HASH.js gzip 5.76 kB 5.76 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 2.76 kB 2.76 kB
routerDirect..HASH.js gzip 322 B 322 B
script-HASH.js gzip 392 B 392 B
withRouter-HASH.js gzip 317 B 317 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16.5 kB 16.5 kB
Client Build Manifests
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
_buildManifest.js gzip 458 B 458 B
Overall change 458 B 458 B
Rendered Page Sizes
vercel/next.js canary jgoping/next.js tt-violation-fixes-without-policy Change
index.html gzip 530 B 530 B
link.html gzip 543 B 543 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB
Commit: 0e480f9

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, can we resolve the merge conflicts?

@uraj
Copy link

uraj commented May 4, 2022

@ijjk thanks for taking a look. Justin has finished his internship and is probably on vacation now. Is it OK to wait for a couple of days? Otherwise I can take this over, but I will probably need to create a new PR.

@jgoping
Copy link
Contributor Author

jgoping commented May 4, 2022

Hi @ijjk, thanks for the review! I will resolve the merge conflicts tomorrow and re-request your review once that has been done.

@ijjk
Copy link
Member

ijjk commented May 4, 2022

Yeah definitely no hurry, thanks for the work you've done on this!

@jgoping jgoping force-pushed the tt-violation-fixes-without-policy branch from 56aa933 to b15f852 Compare May 4, 2022 23:40
@jgoping jgoping force-pushed the tt-violation-fixes-without-policy branch from b15f852 to 0e480f9 Compare May 4, 2022 23:42
@jgoping jgoping requested a review from ijjk May 4, 2022 23:44
@kodiakhq kodiakhq bot merged commit 0dd6211 into vercel:canary May 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants