Skip to content

Element.innerHTML - add info trusted types #40423

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

Merged
merged 6 commits into from
Aug 4, 2025

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Jul 18, 2025

This updates the following properties to explain how they are used with TrustedTypes:

This is in progress:

  • Add boilerplate
  • Add TT information, including the new exception and tinyfill
  • Restructured - this isn't to current templates. Need to move the guid-ish stuff either down as examples or up into description

This does not mention the sanitizer methods as alternatives since the setHTML() is not implemented anywhere, and the sanitizer in setHTMLUnsafe() is not yet implemented. We're in separate discussion on those and when they are in a release we can revisit.

Part of #37518 (tracking issue)

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

github-actions bot commented Jul 18, 2025

Preview URLs

External URLs (2)

URL: /en-US/docs/Web/API/Element/innerHTML
Title: Element: innerHTML property

(comment last updated: 2025-08-04 01:11:40)

@hamishwillee hamishwillee force-pushed the element-innerhtml_tt branch from 24677d9 to cfaddcf Compare July 21, 2025 05:20
@hamishwillee hamishwillee marked this pull request as ready for review July 21, 2025 05:22
@hamishwillee hamishwillee requested a review from a team as a code owner July 21, 2025 05:22
@hamishwillee hamishwillee requested review from sideshowbarker and wbamberg and removed request for a team July 21, 2025 05:22
@hamishwillee
Copy link
Collaborator Author

FYI @lukewarlow @wbamberg - this is the trusted types update for Element.innerHTML. Luke, I've taken on board your comments about sanitizer not yet being implemented. I still mention the setHTMLUnsafe() because it allows set with shadow roots, but not as alternatives with sanitization. It should however be possible to graft that information on here quite easily.

All the examples use trustedHTML. This also shows the first live example use of the tinyfill in the docs. This is duplication with the main docs, but it feels very practical to me given the current state of trustedtypes.

@sideshowbarker sideshowbarker removed their request for review July 21, 2025 06:06
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Just comments on innerHTML for now.

@hamishwillee hamishwillee force-pushed the element-innerhtml_tt branch from 8b5583a to 482eccd Compare August 1, 2025 04:23
@hamishwillee hamishwillee requested a review from wbamberg August 1, 2025 04:23
@hamishwillee
Copy link
Collaborator Author

Thanks @wbamberg ! Feedback for the first document integrated. Much appreciated.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the updates Hamish, they look great to me. I just had a couple of comments on the other page.

Comment on lines 127 to 129
While not required for this example, below we follow the recommendation of defining a policy to create {{domxref("TrustedHTML")}} objects from the input (we should also enforce the policy `safe-content-policy` using CSP).
In this case we know the input is safe so this policy passes it through without modification.
The commented code shows how you might instead use the "DOMPurify" library to sanitize content that wasn't trusted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly to the other example, I wonder about whether we should give people guidance like "we know the input is safe so this policy passes it through without modification".

I suppose part of the issue here is that you want to make it a live sample, but you can't do that and use DOMPurify, so it's hard to make it a "real" example.

One option would be, keep the same code but change the guidance from "we don't need a policy that does anything here" to "in order for this example to work without a third-party dependency, we've implemented a no-op policy here, but you shouldn't do this in real life".

Another option would be to stop making this a live sample, and use DOMPurify, but that would be sad because we want to show the effect of position.

Yet a third option would be to implement our own basic sanitizer, but that might be tricky to get right.

I'd love to know what @aaronshim thinks our guidance here ought to be. Is it good practice to have a no-op policy for content that we "know" is safe?

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've done a modification of "in order for this example to work without a third-party dependency, we've implemented a no-op policy here, but you shouldn't do this in real life".
Its the safest option and the one that most closely matches what we want users to do.
I definitely do not want to remove the live example - it is very helpful for visualising the insert behaviour :-)

Copy link
Collaborator Author

@hamishwillee hamishwillee Aug 4, 2025

Choose a reason for hiding this comment

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

I'd love to know what @aaronshim thinks our guidance here ought to be. Is it good practice to have a no-op policy for content that we "know" is safe?

Me too.

I tend to think that the advice should generally be no - if you want to use these unsafe APIs then you're better always sanitizing. In most cases you'd instead replace using TT and use setHTML() (once supported).
Of course the API itself doesn't care, and if the website has a policy that allows noops then developers will be allowed to use said noop policy.

The more interesting noop question is what you do with setHTMLUnsafe() (once supported) - the case where you know you have unsafe strings, but you perhaps need to allow them anyway. If that is a valid case, my contention is that you'd have a noop trusted type to avoid double sanitization.

That's a problem for #40420 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy with your choice here and let's merge it. We can always follow up if Aaron recommends something else.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Aug 2, 2025
Copy link
Contributor

github-actions bot commented Aug 2, 2025

This pull request has merge conflicts that must be resolved before it can be merged.

@hamishwillee hamishwillee force-pushed the element-innerhtml_tt branch from 482eccd to 64f8ca9 Compare August 3, 2025 23:57
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Aug 3, 2025
@hamishwillee hamishwillee requested a review from wbamberg August 4, 2025 01:01
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you Hamish!

@wbamberg wbamberg merged commit ffe043e into mdn:main Aug 4, 2025
8 checks passed
@hamishwillee hamishwillee deleted the element-innerhtml_tt branch August 4, 2025 23:11
@hamishwillee
Copy link
Collaborator Author

No, thanks for the review and all the great advice @wbamberg . I feel better that we're finally getting these updated.

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.

2 participants