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

Update Trusted Types content attributes test #49473

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

lukewarlow
Copy link
Member

  • Remove WindowEventHandlers from IDL as they're not TT sinks.

  • Skip attributes that are in WPT IDL but are not implemented in browser IDL.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers so I've kept the tests for them.

encrypted-media tests might need a follow up test change, or addition (to actually check they're enforced if need be).

Copy link
Contributor

@fred-wang fred-wang left a comment

Choose a reason for hiding this comment

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

In the commit message:

  1. Perhaps link to Finalise spec mechanism for event handlers w3c/trusted-types#520 so we can continue the discussion there and update the test later.

  2. I'm not sure the spec has a normative definition of "injection sink". I understand that https://w3c.github.io/trusted-types/dist/spec/#integration-with-dom changes setAttribute/setAttributeNS to call https://w3c.github.io/trusted-types/dist/spec/#get-trusted-type-data-for-attribute and then anything that would return a non-null data would be an "injection sink". So as I read the spec, calling element.setAttribute("onclick", unsafe) on an element with an exotic namespace (not HTML or SVG or MathML) provides an injection sink, and the unsafe value needs to be sanitized by the default policy, even though there is no IDL definition for this element or no corresponding content attribute defined for that element in any web spec. For WindowEventHandlers, see my inline comment.

@lukewarlow lukewarlow force-pushed the tt-content-attributes branch from be965aa to df88a3e Compare December 2, 2024 18:42
- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
@lukewarlow lukewarlow force-pushed the tt-content-attributes branch from df88a3e to 7c3f738 Compare December 2, 2024 18:48
@lukewarlow
Copy link
Member Author

I'm not sure the spec has a normative definition of "injection sink". I understand that https://w3c.github.io/trusted-types/dist/spec/#integration-with-dom changes setAttribute/setAttributeNS to call https://w3c.github.io/trusted-types/dist/spec/#get-trusted-type-data-for-attribute and then anything that would return a non-null data would be an "injection sink". So as I read the spec, calling element.setAttribute("onclick", unsafe) on an element with an exotic namespace (not HTML or SVG or MathML) provides an injection sink, and the unsafe value needs to be sanitized by the default policy, even though there is no IDL definition for this element or no corresponding content attribute defined for that element in any web spec. For WindowEventHandlers, see my inline comment.

I think "sink" as a concept is well understood. But yes the spec for event handlers is very wishy washy right now. I think being overly strict is probably fine (like your example). But still needs some work.

@lukewarlow lukewarlow enabled auto-merge (squash) December 2, 2024 18:51
@lukewarlow lukewarlow merged commit 59d1f70 into master Dec 2, 2024
17 checks passed
@lukewarlow lukewarlow deleted the tt-content-attributes branch December 2, 2024 18:59
fred-wang added a commit that referenced this pull request Jan 10, 2025
…erfaceName

This is a small refactoring of #49473

Instead of skipping unsupported attributes in the for loop, we filter
them out when building the list.

Technically, interfaceName is no longer used in the for loop, but we
keep it in case that's needed for future setAttribute/setAttributeNS
tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants