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.

Sign up for GitHub

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

Jump to bottom

Add test for importing a srcdoc attribute node from a non-TT realm to a TT iframe element throws #44323

Merged
merged 10 commits into from
Apr 15, 2024
Merged

Add test for importing a srcdoc attribute node from a non-TT realm to a TT iframe element throws #44323

merged 10 commits into from
Apr 15, 2024

Conversation

mbrodesser-Igalia
Copy link
Contributor

First step to fix w3c/trusted-types#425.

Will add separate commits for the other tests requested at above ticket.

@mbrodesser-Igalia
Copy link
Contributor Author

Stumbled on #44352, which either needs to be fixed or worked around.

@mbrodesser-Igalia mbrodesser-Igalia mentioned this pull request Mar 7, 2024
Open
annevk
annevk reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

It would help to have a pointer where the throwing is defined. I didn't see it in the linked issue either.

trusted-types/block-string-assignment-to-Element-setAttributeNode.html Outdated
<body>
<div id="nonSVGTestElements">
<iframe srcdoc="v"></iframe>
<embed src="v">
Copy link
Member

Choose a reason for hiding this comment

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

For embed and object the expectation needs to be different right, per recent events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably you refer to w3c/trusted-types#486? Yes. Presumably more tests will have to be adapted; it seems cleaner to do that collectively in w3c/trusted-types#486.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm planning to go through the test suite and probably just removing most references to embed and object. With potentially one left to make sure that they're not enforcing TT restrictions.

So feel free to just not add them to this test, or to add them and I can remove them them in my PR.

Copy link
Member

Choose a reason for hiding this comment

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

Just to loop back here I've started WebKit/WebKit#26554

So it would be best if no new usages of embed or object were added.

@mbrodesser-Igalia
Copy link
Contributor Author

mbrodesser-Igalia commented Mar 27, 2024 •
edited
Loading

It would help to have a pointer where the throwing is defined. I didn't see it in the linked issue either.

Indeed not obvious. The throwing stems from: whatwg/dom#1247 (diff: https://whatpr.org/dom/1247/6a9b5a2...10ce041.html) which invokes https://w3c.github.io/trusted-types/dist/spec/#validate-attribute-mutation.
The latter invokes https://w3c.github.io/trusted-types/dist/spec/#abstract-opdef-get-trusted-type-compliant-string which throws a TypeError in step 6.3.

@mbrodesser-Igalia mbrodesser-Igalia force-pushed the TrustedTypesSpec_425_import_srcdoc_attr_node_which_throws branch from 9af8771 to 9839909 Compare March 28, 2024 11:26
@mbrodesser-Igalia
Add test for importing a srcdoc attribute node from a non-TT realm …
3bd4d23 
…to a TT iframe element throws

First step to fix <w3c/trusted-types#425>.

Will add separate commits for the other tests requested at above ticket.
@mbrodesser-Igalia
Schedule the new test before a default policy is created and simplify…
d120fc8 
… the new test
@mbrodesser-Igalia
Move test to <block-string-assignment-to-Element-setAttributeNode.htm…
2c2ea19 
…l> and further simplify it
@mbrodesser-Igalia
Add TODO
aeed729
@mbrodesser-Igalia
Add remaining tests for the DOM integration of TT when attribute node…
f484397 
…s created in a non-TT enforcing realm are imported to a TT-enforcing realm

See
<https://w3c.github.io/trusted-types/dist/spec/#validate-attribute-mutation>.

This excludes tests for
<https://w3c.github.io/trusted-types/dist/spec/#enforcement-in-event-handler-content-attributes>
which will have to be added once that part of the spec is propagated to
the HTML spec.

The remaining tests mentioned at
<w3c/trusted-types#425 (comment)> will be added in
separate commits.
@mbrodesser-Igalia
Change test to check aTestElement's ownerDocument equals document
4a9e49c
@mbrodesser-Igalia
Refactor test to use the XLink namespace for SVG's script element
d615169
@mbrodesser-Igalia
Remove unnecessary assertions and explain in a comment that `aTestEle…
26c25f9 
…ment`'s and `sourceAttr`s realms differ
@mbrodesser-Igalia
Change test to check setAttributeNode and setAttribute respect th…
b617318 
…e element's node document's global's CSP

Requires web-platform-tests#45405 to be
fixed.
@mbrodesser-Igalia
Use sourceFrame.contentWindow's TyperError constructor
dfd95a9
@mbrodesser-Igalia mbrodesser-Igalia force-pushed the TrustedTypesSpec_425_import_srcdoc_attr_node_which_throws branch from 9839909 to dfd95a9 Compare April 8, 2024 09:46
annevk
annevk approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like the correct test.

I wonder if we should also test the inverse. That when you take an element out of a TT global, TT is no longer enforced for it.

@mbrodesser-Igalia
Copy link
Contributor Author

Thanks, this looks like the correct test.

I wonder if we should also test the inverse. That when you take an element out of a TT global, TT is no longer enforced for it.

I don't know how relevant that scenario is.

@koto: any experience with that?

@annevk
Copy link
Member

annevk commented Apr 11, 2024

It's relevant for ensuring the specification is implemented correctly.

@mbrodesser-Igalia
Copy link
Contributor Author

It's relevant for ensuring the specification is implemented correctly.

True; but is that scenario actually relevant in practice?

@annevk
Copy link
Member

annevk commented Apr 15, 2024

That does not matter for conformance tests. The whole point is that we can't know what websites might do and might rely on.

@mbrodesser-Igalia
Copy link
Contributor Author

That does not matter for conformance tests. The whole point is that we can't know what websites might do and might rely on.

Agreed. Added w3c/trusted-types#425 (comment) so that another test will be added.

@mbrodesser-Igalia
Copy link
Contributor Author

@annevk: can you please merge this test, I lack rights.

@annevk annevk merged commit 560c8d6 into web-platform-tests:master Apr 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukewarlow lukewarlow lukewarlow left review comments

@annevk annevk annevk approved these changes

@mikewest mikewest Awaiting requested review from mikewest

Assignees

@mikewest mikewest

Labels
assets attribution-reporting ci console credential-management CSS2 css-anchor-position css-animations css-backgrounds css-cascade css-color css-contain css-display css-flexbox css-grid css-overflow css-page css-position css-scroll-snap css-text css-text-decor css-transitions css-ui css-values css-view-transitions css-writing-modes cssom-view custom-elements custom-state-pseudo-class dom editing encoding event-timing fenced-frame fetch fledge generic-sensor html infra intersection-observer long-animation-frame mathml mediacapture-insertable-streams mediacapture-record notifications orientation-event pointerevents printing requirements_tests.txt scroll-animations selection selectors serial service-workers shadow-dom shared-storage shared-storage-selecturl-limit speculation-rules svg trusted-types uievents web-animations web-locks webaudio webdriver webdriver-bidi webnn webrtc wg-cg_wicg wg-css wg-das wg-math wg-s_audio wg-s_htmlwg wg-s_pointer-events wg-s_webapps wg-s_webappsec wg-s_webmachinelearning wg-s_webperf wg-s_webrtc wg-svg wg-sw wg-testing_browser wg-wai_pf wg-webplatform workers wptrunner The automated test runner, commonly called through ./wpt run wptserve zoom
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve test coverage for DOM integration in WPT
7 participants
@mbrodesser-Igalia @annevk @lukewarlow @mikewest @cookiecrook @wpt-pr-bot @moz-wptsync-bot