Add `setCSPTrustedTypesPolicy()` for CSP trusted types. #81

lgarron · 2022-10-27T22:01:31Z

CSP trusted types is an API that allows a website to reduce the possibility of XSS by controlling what kind of content can be placed in a "sink" like .innerHTML. Since <include-fragment-element> is essentially a fancy .innerHTML convenience, it needs changes to be compatible with CSP trusted types.

This PR introduces a flexible callback that allows the calling code to provide its own sanitization or rejection of a server response for an <include-fragment-element>. For example, the site may want to allow the server to send a header to assert that certain HTML is sanitized and safe to use as-is, or the site may want to run the response through a sanitizer.

See the README.md changes for snippets making use of this change: https://github.com/github/include-fragment-element/pull/81/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

primer-css · 2022-10-27T22:06:46Z

👋 Hello and thanks for pinging us! This issue or PR has been added to our inbox and a Design Infrastructure first responder will review it soon.

🎨 If this is a PR that includes a visual change, please make sure to add screenshots in the description or deploy this code to a lab machine with instructions for how to test.
⏩ If this is a PR that includes changes to an interaction, please include a video recording in the description.
⚠️ If this is urgent, please visit us in #primer on Slack and tag the first responders listed in the channel topic.

src/index.ts

lgarron · 2022-10-27T22:20:11Z

I believe this PR still needs:

Tests
Documentation

I could also use thoughts on how to let the callback reject a response. Is it alright to let it throw an error? Should we add anything to the existing error handler for that?

Please let me know if 1) it also needs anything else, and 2) any thoughts I might want to keep in mind before writing those?

src/index.ts

keithamus

Does it make more sense here to pass the whole policy to the element (perhaps IncludeFragmentElement.trustedPolicy could be a writable field) and then have the code call createHTML on that? We could also pass the response object as a second arg for additional verification perhaps?

lgarron · 2022-10-28T00:30:58Z

Does it make more sense here to pass the whole policy to the element (perhaps IncludeFragmentElement.trustedPolicy could be a writable field) and then have the code call createHTML on that? We could also pass the response object as a second arg for additional verification perhaps?

I think this is a reasonable approach, given that there is an accommodation for args in the API: https://developer.mozilla.org/en-US/docs/Web/API/TrustedTypePolicy/createHTML

It might mean that the site ends up with more policies than otherwise, as the policy for include-fragment-element would have to wrap any other(s) that could have been used with (light) pre-processing. I haven't read much on the composition of policies, but I don't see why it shouldn't work.

Let me see how that would look.

lgarron · 2022-10-28T00:38:10Z

It might mean that the site ends up with more policies than otherwise, as the policy for include-fragment-element would have to wrap any other(s) that could have been used with (light) pre-processing. I haven't read much on the composition of policies, but I don't see why it shouldn't work.

Let me see how that would look.

There seems to be at least one fundamental tradeoff, which is that createHTML is required to be synchronous. However, I think we can allow the policy itself to be passed in as a Promise, to reduce the chance of race conditions.

[CSP trusted types](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/trusted-types) is an API that allows a website to reduce the possibility of XSS by controlling what kind of content can be placed in a "sink" like `.innerHTML`. This commit introduces a flexible callback that allows the calling code to provide its own validation or rejection of an server response for an `<include-fragment-element>`. For example, the site may want to allow the server to send a header to assert that certain HTML is sanitized and safe to use as-is, or the site may want to run the response through a sanitizer. Here is a snippet that looks for such a header and falls back to the `dompurify` library for extremely basic sanitization. ```ts import { setCSPTrustedTypesCallback } from "include-fragment-element"; import { default as DOMPurify } from "dompurify"; const policy = trustedTypes.createPolicy("server-sanitized", { createHTML: (s) => s }); setCSPTrustedTypesCallback(async (r: Response) => { if (r.headers.get("X-Response-Trusted-Types")?.split(",").includes("server-sanitized=true")) { return policy.createHTML(r.text()); } return DOMPurify.sanitize(await r.text(), { RETURN_TRUSTED_TYPE: true }); }); ```

keithamus

This LGTM 👍

…ll `load()`.

lgarron · 2022-11-16T20:40:02Z

src/index.ts

+}
+
+// TODO: find another way to make this available for testing.
+;(globalThis as any).setCSPTrustedTypesPolicy = setCSPTrustedTypesPolicy


The test doesn't actually have a way to use exported functions. Do you have a recommendation for how to get access to setCSPTrustedTypesPolicy from test.js without this hack?

The test suite should be able to import functions. I'd like us to avoid attaching globals (that aren't the elements themselves).

Could you point me to the mechanism/syntax for importing functions from the test? I only see the test config somehow importing the file separately.

Apologies I thought we had updated to web-test-runner. I've now done so in #86 so if you rebase it should be evident how to import the function now, as there is an import statement at the top of the file.

src/index.ts

…onal caveats.

test/test.js

lgarron · 2022-11-19T01:01:48Z

Just realized this was still marked as approved/reviewed, despite substantial revisions and pending questions. I hope I've marked it correctly now to request another review (certainly no rush, though, given this is late on Friday. :-P).

fletchto99

I'll let web systems provide specific code feedback but we were able to test this for our use case in trusted types and it worked as expected 🎉

JoshBowdenConcepts

Looks good on behalf of @github/primer-reviewers

lgarron · 2022-11-21T21:07:34Z

Looks good on behalf of @github/primer-reviewers

Thanks! I still have a few open questions in the comments that I'd love to resolve before merging; could I ask for any thoughts on those?

lgarron · 2022-11-30T04:44:00Z

src/index.ts

@@ -45,8 +62,9 @@ export default class IncludeFragmentElement extends HTMLElement {
    this.setAttribute('accept', val)
  }

+  // TODO: Should this return a TrustedHTML if available, or always a string?


I could use advice on this. On the one hand, it would be less surprising (and backwards-compatible) to always return a string.

However, that string can't be assigned to another .innerHTML without going through a CSP trusted types policy again.

(Copied from an older comment.)

We can always mark this as a breaking change and return TrustedHTML

We discussed this in team pairing and replaced it with a comment, because we're uncertain whom this might break and we don't need it right now. Does that sound alright to you?

keithamus

I'm largely in favour of this change. I have some small nitpicks, and I think it would be worth us making a breaking change here, to allow for the change in types.

src/index.ts

keithamus · 2022-11-30T09:20:15Z

src/index.ts

@@ -45,8 +62,9 @@ export default class IncludeFragmentElement extends HTMLElement {
    this.setAttribute('accept', val)
  }

+  // TODO: Should this return a TrustedHTML if available, or always a string?


We can always mark this as a breaking change and return TrustedHTML

src/index.ts

test/test.js

Co-authored-by: Matt Langlois <fletchto99@github.com> Co-authored-by: Lucas Garron <lgarron@github.com>

Made two changes based on feedback.

rzhade3 · 2022-11-30T19:04:04Z

@keithamus we've adjusted the PR to address your feedback above. Mind taking one last pass before we merge in?

README.md

lgarron · 2022-12-01T02:23:18Z

Since the last commit after approval was the requested documentation changes, I'm gonna go ahead and merge and then prep a version bump PR!

lgarron requested a review from a team as a code owner October 27, 2022 22:01

lgarron commented Oct 27, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

lgarron commented Oct 27, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

lgarron commented Oct 27, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

lgarron commented Oct 27, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

keithamus reviewed Oct 27, 2022

View reviewed changes

lgarron mentioned this pull request Oct 28, 2022

Switch Promise chaining to async-await syntax. #82

Merged

lgarron added 3 commits October 31, 2022 15:27

Apply code review suggestion from @keithamus.

2f35ecd

Format.

b07009d

keithamus previously approved these changes Nov 1, 2022

View reviewed changes

lgarron added 2 commits November 14, 2022 10:22

Inline the CSP types.

52116a2

Fix function name typo.

c4e4dac

lgarron changed the title ~~Add setCSPTrustedTypesCallback for CSP trusted types.~~ Add setCSPTrustedTypesPolicy() for CSP trusted types. Nov 14, 2022

lgarron added 7 commits November 14, 2022 20:38

Merge branch 'main' into lgarron/csp-trusted-types

4da1393

Fix a function call.

201a903

Rename cspTrustedTypesPolicy to cspTrustedTypesPolicyPromise.

fae809b

Remove a test assumptions about microtasking order when awaiting a fu…

0b02f05

…ll `load()`.

Allow clearing the policy using null.

4ff5d3d

Update docs.

19f57a3

Add CSP trusted types tests.

f470332

lgarron mentioned this pull request Nov 16, 2022

Fix up types for cached and returned data. #84

Merged

lgarron commented Nov 16, 2022

View reviewed changes

lgarron added 3 commits November 16, 2022 13:00

Update comments on the CSP trusted types TypeScript types with additi…

a7e3454

…onal caveats.

Sacrifice to the linter gods.

2f4f682

Restructed bullet notes.

0256c74

lgarron added 2 commits November 16, 2022 13:49

Add a compat note pointing to the tinyfill.

302221b

Merge branch 'main' into lgarron/csp-trusted-types

e2717c7

lgarron commented Nov 17, 2022

View reviewed changes

test/test.js Show resolved Hide resolved

lgarron requested a review from keithamus November 19, 2022 01:00

fletchto99 approved these changes Nov 21, 2022

View reviewed changes

JoshBowdenConcepts approved these changes Nov 21, 2022

View reviewed changes

lgarron added 2 commits November 29, 2022 20:36

Merge branch 'main' into lgarron/csp-trusted-types

0eab434

Update tests.

244e73c

lgarron commented Nov 30, 2022

View reviewed changes

keithamus reviewed Nov 30, 2022

View reviewed changes

Make setCSPTrustedTypesPolicy static function

672e836

Co-authored-by: Matt Langlois <fletchto99@github.com> Co-authored-by: Lucas Garron <lgarron@github.com>

rzhade3 requested a review from keithamus November 30, 2022 19:03

keithamus approved these changes Nov 30, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README

4aa0075

lgarron merged commit c56996d into github:main Dec 1, 2022

fletchto99 deleted the lgarron/csp-trusted-types branch December 1, 2022 22:47

rzhade3 mentioned this pull request Dec 8, 2022

Add Trusted Types github/jtml#26

Merged

keithamus mentioned this pull request Mar 28, 2023

add csp trusted types policy github/auto-complete-element#102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `setCSPTrustedTypesPolicy()` for CSP trusted types. #81

Add `setCSPTrustedTypesPolicy()` for CSP trusted types. #81

lgarron commented Oct 27, 2022 •

edited

Loading

primer-css commented Oct 27, 2022

lgarron commented Oct 27, 2022 •

edited

Loading

keithamus left a comment

lgarron commented Oct 28, 2022

lgarron commented Oct 28, 2022

keithamus left a comment

lgarron Nov 16, 2022

keithamus Nov 19, 2022

lgarron Nov 21, 2022

keithamus Nov 22, 2022

lgarron commented Nov 19, 2022

fletchto99 left a comment

JoshBowdenConcepts left a comment

lgarron commented Nov 21, 2022 •

edited

Loading

lgarron Nov 30, 2022

keithamus Nov 30, 2022

lgarron Nov 30, 2022 •

edited

Loading

keithamus left a comment

keithamus Nov 30, 2022

rzhade3 commented Nov 30, 2022

lgarron commented Dec 1, 2022

Add setCSPTrustedTypesPolicy() for CSP trusted types. #81

Add setCSPTrustedTypesPolicy() for CSP trusted types. #81

Conversation

lgarron commented Oct 27, 2022 • edited Loading

primer-css commented Oct 27, 2022

lgarron commented Oct 27, 2022 • edited Loading

keithamus left a comment

Choose a reason for hiding this comment

lgarron commented Oct 28, 2022

lgarron commented Oct 28, 2022

keithamus left a comment

Choose a reason for hiding this comment

lgarron Nov 16, 2022

Choose a reason for hiding this comment

keithamus Nov 19, 2022

Choose a reason for hiding this comment

lgarron Nov 21, 2022

Choose a reason for hiding this comment

keithamus Nov 22, 2022

Choose a reason for hiding this comment

lgarron commented Nov 19, 2022

fletchto99 left a comment

Choose a reason for hiding this comment

JoshBowdenConcepts left a comment

Choose a reason for hiding this comment

lgarron commented Nov 21, 2022 • edited Loading

lgarron Nov 30, 2022

Choose a reason for hiding this comment

keithamus Nov 30, 2022

Choose a reason for hiding this comment

lgarron Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

keithamus left a comment

Choose a reason for hiding this comment

keithamus Nov 30, 2022

Choose a reason for hiding this comment

rzhade3 commented Nov 30, 2022

lgarron commented Dec 1, 2022

Add `setCSPTrustedTypesPolicy()` for CSP trusted types. #81

Add `setCSPTrustedTypesPolicy()` for CSP trusted types. #81

lgarron commented Oct 27, 2022 •

edited

Loading

lgarron commented Oct 27, 2022 •

edited

Loading

lgarron commented Nov 21, 2022 •

edited

Loading

lgarron Nov 30, 2022 •

edited

Loading