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

Expand trusted types guidance #37917

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Feb 2, 2025

I had thought to defer updates to the trusted types docs until it ships cross-browser, but given conversations like w3c-cg/swag#9 (comment) and our general desire to talk about this API, I thought it was worth expanding on things a little now.

We should think of this PR as a stop-gap though: I still think we should revisit these docs, write a proper guide, and refresh all the API pages, when the API ships in Firefox or Safari.

This PR expands the Trusted Types API overview page in two ways:

  • explain better about enforcement using a CSP
  • explain about polyfills, since people can get the benefit of this API cross-browser now

I also updated the rest of the section to be clearer about what the API actually does and how it connects with XSS - I thought things like "lock down the insecure parts of the DOM API" weren't very meaningful.

I hope I understood the bit about polyfills!

@aaronshim, @lukewarlow

@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Feb 2, 2025
Copy link
Contributor

github-actions bot commented Feb 2, 2025

Preview URLs (6 pages)
External URLs (5)

URL: /en-US/docs/Web/API/Trusted_Types_API
Title: Trusted Types API

(comment last updated: 2025-02-07 02:22:58)

@wbamberg wbamberg marked this pull request as ready for review February 2, 2025 17:36
@wbamberg wbamberg requested a review from a team as a code owner February 2, 2025 17:36
@wbamberg wbamberg requested review from sideshowbarker and hamishwillee and removed request for a team and sideshowbarker February 2, 2025 17:36
@lukewarlow
Copy link
Contributor

I will try to review this change during this week.

The polyfill specifically might need some updates to match it with latest spec changes I'm not sure of its current state.

Comment on lines -25 to +19
- Functions that insert HTML into the document such as {{domxref("Element.innerHTML")}}, {{domxref("Element.outerHTML")}}, or {{domxref("Document.write()")}}.
- Functions that insert HTML into the document such as {{domxref("Element.innerHTML")}}.
Copy link
Collaborator

@hamishwillee hamishwillee Feb 3, 2025

Choose a reason for hiding this comment

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

This "Concepts and usage" section is now linked from all of the various trusted type interfaces as "injection syncs". For instance, in trusted HTML it says

The TrustedHTML interface of the Trusted Types API represents a string that a developer can insert into an injection sink that will render it as HTML.

But in TrustedHtml there is no listing of what interfaces this might be used with - and if you navigate here, just this single example of Element.innerHTML.

This is perhaps intentional - you don't want to have a list that can't be maintained as exhaustive? I think in this case it would be useful to either list the problematic interfaces in TrustedHtml and friends, or here have a larger subset of what they are used for. You can still say "such as" even for an incomplete list. FWIW I think page is good, and would have it in the interfaces as per https://github.com/mdn/content/pull/37917/files#r1938643809

FYI I'm looking at documenting Document.write() and Document.writeln() and when I was looking at this page, knowing this was an affected case of HTML insertion was useful.

Copy link
Collaborator Author

@wbamberg wbamberg Feb 3, 2025

Choose a reason for hiding this comment

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

I've made two changes here from what was there before:

  • removed the "injection sinks" heading
  • removed a couple of the examples, specifically outerHTML and document.write().

So pretty minor change. I haven't changed it from an exhaustive to a non-exhaustive list, it was never exhaustive. I don't want the "injection sinks" heading there, or a very long list, because I think it breaks the flow of the document.

But I do agree that it is a bit weird having the "injection sinks" link in pages like https://developer.mozilla.org/en-US/docs/Web/API/TrustedScript/toString go to "concepts and usage". To address that I might suggest having an "Injection sinks" H3 heading at the end of "concepts and usage", which looks quite a bit like the existing one at https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API#injection_sinks.

Separately, yes it would be good to have an exhaustive list! But I can't find one in the specification. Also I think that's out of the intended scope of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think even a non-exhaustive list would be useful. But appreciate you have a clear understanding of what you want to achieve.

No worse that what was here before so OK as an edit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think BTW that in pages like https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML we should talk about trusted types. It's unclear to me how someone can really use this API effectively without knowing exactly which APIs are governed by it. But again I don't want to do this now, because it turns into "rewrite everything".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I plan to do that as part of #37518 - discovered it had not been done yesterday.

Copy link
Collaborator

@hamishwillee hamishwillee Feb 3, 2025

Choose a reason for hiding this comment

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

Note, at this point, covering the APIs in that PR (write, writeln from Document, the setInterval/Time, Element.innerHTML, ShadowRoot.innerHTML.
There are others that may also be affected, such as outerHTML, but I'm constraining myself in order to meet a specific FF release now overdue (i.e. what I have tested works in FF).

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Yes, much better. Just a few comments.

wbamberg and others added 3 commits February 3, 2025 14:39
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mdn mdn deleted a comment from Kleres818 Feb 4, 2025
const trustedHTML = policy.createHTML(userInput);
element.innerHTML = trustedHTML;
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is one other thing it would be good to mention here, and that is default policy, which I would pull out of the createPolicy docs.

At least for the Document.write() method, the spec says that IFF CSP is enforced then any strings passed to injection sink (or at least write()) will be offered to the default policy, and only if that default policy doesn't exist is there a TypeError. The idea is that you log this default policy, and eventually end up with everything migrated.

IF you don't do it, then I'll have a go after this merges.

…d-types-patch

* origin/trusted-types-patch:
  Update files/en-us/web/api/trustedscript/tostring/index.md
@wbamberg wbamberg requested a review from hamishwillee February 6, 2025 22:26
Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Much better IMO. Added a suggestion. I'm approving rather than merging, because not clear to me if @lukewarlow has more review to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants