-
Notifications
You must be signed in to change notification settings - Fork 22.6k
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
base: main
Are you sure you want to change the base?
Conversation
Preview URLs (6 pages)External URLs (5)URL:
(comment last updated: 2025-02-07 02:22:58) |
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. |
- 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")}}. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
anddocument.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this 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.
Co-authored-by: Hamish Willee <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Hamish Willee <[email protected]>
const trustedHTML = policy.createHTML(userInput); | ||
element.innerHTML = trustedHTML; | ||
``` | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Co-authored-by: Hamish Willee <[email protected]>
Co-authored-by: Hamish Willee <[email protected]>
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:
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