Add Trusted Types data for missing HTML sinks + refine existing#27114
Add Trusted Types data for missing HTML sinks + refine existing#27114
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
Just to clarify: Is this a draft? If not, should we move that list to an issue? |
|
That list comes from the linked issue but yes I'll remove it from the PR description here |
|
To clarify this PR is ready for review |
api/DOMParser.json
Outdated
| "accepts_TrustedHTML": { | ||
| "__compat": { | ||
| "description": "Can be set with a `TrustedHTML` instance", |
There was a problem hiding this comment.
@caugner There was a post by @lukewarlow in mdn/content#37518 (comment) which we are best of to discuss here.
In summary, "accepts" is not quite right because actually you can pass anything that resolves to a string to any method that takes a string. This is why we can use the tinyfill described here to fake TTs on systems that don't support them.
The thing that makes these methods special is that they enforce TTs being passed when enforcement is enabled.
While I don't think that most users will appreciate the difference, we would not be wrong to make it. This would would require changes to the other cases too.
Something like::
| "accepts_TrustedHTML": { | |
| "__compat": { | |
| "description": "Can be set with a `TrustedHTML` instance", | |
| "enforces_TrustedHTML": { | |
| "__compat": { | |
| "description": "Requires `TrustedHTML` instance when trusted types are enforced", |
From Luke on other post
I'm not sure where best to raise this as I can't file a raw issue on BCD I'll mention here.
I think the "accepts_TrustedHTML" style of subfeature in BCD may be the wrong way to frame what this data is for.
I think "enforces_TrustedTypes" might be a better key with something like "Must be set with a
TrustedHTMLinstance when trusted types are enforced." (or something more succint)Any API that accepts a string can be set to an TT object because they'll just get stringified by IDL type conversions.
That way stuff like Chromium's bug in
new Function()can be reflected by a partial implemention and a note that TT is enforced for the sink but passing a TrustedScript doesn't actually work (it gets treated as an unsafe string and put through the default policy)
There was a problem hiding this comment.
We discussed this shortly in today's BCD meeting:
- We should always strive to make the description as clear as possible.
- While we generally avoid renames, in this case we think it's fine, because this PR adds significantly more of these features than currently exists.
- Given that the spec mentions "enforcement", the proposed new key seems good (*).
@hamishwillee (or @lukewarlow) Just to double-check: Do the new key and description reflect what the old key and description was intended to capture? (Technically, as Luke wrote, these features already accepted TrustedHTML before, except that there was no Trusted Types enforcement.)
(*) To avoid confusion (there is no TrustedTypes interface), we might want to rename to enforces_trusted_types though?!
There was a problem hiding this comment.
Yes, the new key approaches do reflect the intent. I agree with @lukewarlow suggestion #27114 (comment) that it makes sense to use a common key everywhere and in the description specify the actual type (I like the common key simply because it is common, and captures the main point, and I like the description, because it is a useful hint to readers.
So updated suggestion reflecting this particular instance would be
| "accepts_TrustedHTML": { | |
| "__compat": { | |
| "description": "Can be set with a `TrustedHTML` instance", | |
| "enforces_trusted_types": { | |
| "__compat": { | |
| "description": "Requires `TrustedHTML` instance when trusted types are enforced", |
Thanks for offering to do the PR @lukewarlow - I assume since we seem to be agreement with each other and @caugner you might reasonably start that.
There was a problem hiding this comment.
Yes, let's rename the new ones and the existing ones together in this PR.
|
I'm fairly confident the proposed key and description (or a variant of them) covers what the original key is trying to convey. It's saying hey for this API surface you need to give an appropriate trusted type. We could either do enforces_trusted_types across the board and the description would say which type (TrustedHTML, TrustedScript or TrustedScriptURL) or we could do enforces_TrustedX as appropriate. I lean towards enforces_trusted_types because it's slightly more correct (with default policy and all that). I'll make a separate PR to make that change and then rebase this PR atop it once it's merged. |
Yes, let's go with this one. You can either rename the existing ones in a separate PR (probably slightly "cleaner"), or inside this one. |
Co-authored-by: Luke Warlow <luke@warlow.dev>
|
Thanks @lukewarlow! |
|
Apologies the updates to this fell off my radar! |
|
Thanks for this from me too! |
Summary
Add TT support data for some missing sinks.
This adds a subfeature data for whether sinks accept Trusted objects.
Test results and supporting details
Related issues
See #26242