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 Violation Fixes #2

Closed
wants to merge 7 commits into from
Closed

Trusted Types Violation Fixes #2

wants to merge 7 commits into from

Conversation

jgoping
Copy link
Owner

@jgoping jgoping commented Feb 3, 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

Documentation

There are three tsec 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 functions can be passed to setTimeout. There is only one call to the sleep function and it passes in numbers only, 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 requested a review from uraj February 3, 2022 18:42
'http://www.w3.org/2000/svg',
'foreignObject'
)
foreignObject.setAttribute('width', 30)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using setAttribute, you can use the standard SVGAnimatedLength API:

foreignObject.width.baseVal.value = 30;

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed

@jgoping jgoping requested review from uraj and bjarkler February 7, 2022 16:55
Copy link
Collaborator

@bjarkler bjarkler left a comment

Choose a reason for hiding this comment

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

Looks great, ship it!

var fragment = document.createElement('div')
fragment.innerHTML =
'<svg><foreignObject width="30" height="30">\n <input type="text"/>\n </foreignObject></svg>'
// Returns <foreignObject width="30" height="30">\n <input type="text"/>\n </foreignObject>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Clean this comment up and add more context. E.g.

// Construct <foreignObject width="30" height="30"><input type="text"/></foreignObject>
// in a way that does not raise a Trusted Types violation.

@jgoping jgoping changed the base branch from tsec-integration to canary February 13, 2022 18:11
@jgoping jgoping changed the base branch from canary to tsec-integration February 13, 2022 18:21
@jgoping jgoping changed the base branch from tsec-integration to canary February 18, 2022 17:42
@jgoping jgoping closed this Feb 18, 2022
@jgoping jgoping deleted the violation-fixes branch February 18, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants