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

Test TT is not enforced when taking an element out of a TT realm to a… #46432

Closed
wants to merge 1 commit into from
Closed

Test TT is not enforced when taking an element out of a TT realm to a… #46432

wants to merge 1 commit into from

Conversation

ziransun
Copy link
Member

… non-TT realm.

See discussions at w3c/trusted-types#425 (comment).

@wpt-pr-bot wpt-pr-bot requested a review from mikewest May 22, 2024 10:29
lukewarlow
lukewarlow requested changes May 22, 2024
View reviewed changes
Copy link
Member

@lukewarlow lukewarlow left a comment

Choose a reason for hiding this comment

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

This won't work as required. You need to have the element inside the iframe (and have TT enforced in the iframe not the parent document) and then move it to the parent. CSP inherits down into the iframe as currently written so this will fail

@ziransun ziransun force-pushed the non-TT branch 2 times, most recently from 37409ee to 27e7b24 Compare May 24, 2024 14:06
@ziransun
Copy link
Member Author

This won't work as required. You need to have the element inside the iframe (and have TT enforced in the iframe not the parent document) and then move it to the parent. CSP inherits down into the iframe as currently written so this will fail

Updated. PTAL. Thanks!

mbrodesser-Igalia
mbrodesser-Igalia requested changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@mbrodesser-Igalia mbrodesser-Igalia left a comment

Choose a reason for hiding this comment

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

Not finished reviewing, but changes are required.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
sourceFrame.addEventListener(
"load",
t.step_func_done(() => {
testCases.forEach(c => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Running all test cases in one step_func_done results in a test suite which is hard to debug. Consider changing this file to run one async_test or promise_test for every element of testCases.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
"load",
t.step_func_done(() => {
testCases.forEach(c => {
const elementId = c[0].concat('.').concat(c[1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-picking: c.join(".") would be more concise.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
<meta charset="utf-8">
<meta
http-equiv="Content-Security-Policy"
content="require-trusted-types-for 'script';"
Copy link
Contributor

Choose a reason for hiding this comment

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

The semicolon at the end is superfluous.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
];

const sourceFrame = document.createElement("iframe");
sourceFrame.srcdoc = iframePolicy.createHTML(
Copy link
Contributor

Choose a reason for hiding this comment

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

sourceFrame, at the time of assigning has no CSP, hence wrapping iframe_srcdoc in a TrustedHTML object is superfluous. Please remove that.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
testCases.forEach(c => {
const elementId = c[0].concat('.').concat(c[1]);
const sourceElement = sourceFrame.contentWindow.document.getElementById(elementId);
const testAttr = sourceElement.attributes[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

The order of .attribute's values my differ among browsers, see https://developer.mozilla.org/en-US/docs/Web/API/Element/attributes.

trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
testAttr.name
);
sourceElement.removeAttributeNode(sourceAttr);
// Now `sourceElement`'s node document's global belongs to a non TT-realm.
Copy link
Contributor

@mbrodesser-Igalia mbrodesser-Igalia Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The comment is false at that line. It should be true after appending to document.body, be moved there and rephrased to something like:

"Now sourceElement's node document's global should belong to a non TT-realm. Hence setAttributeNode and setAttributeNS with non-trusted input should pass".

mbrodesser-Igalia
mbrodesser-Igalia reviewed Dec 11, 2024
View reviewed changes
trusted-types/Element-setAttribute-not-enforced-in-non-TT-documents-globals-CSP.html
<head>
<meta charset="utf-8" />
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the relevant spec PR (whatwg/dom#1268) is merged, the relevant spec should be linked here via <link rel="help" href="<linkToSpec>">.

mbrodesser-Igalia
mbrodesser-Igalia requested changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@mbrodesser-Igalia mbrodesser-Igalia left a comment

Choose a reason for hiding this comment

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

@ziransun: took over the patch to https://phabricator.services.mozilla.com/D231921. Please close this review.

moz-wptsync-bot pushed a commit that referenced this pull request Dec 13, 2024
@mbrodesser-Igalia @moz-wptsync-bot
part 5) Add WPT <Element-setAttribute-respects-Elements-node-document…
fc0f781 
…s-globals-CSP-after-adoption-from-TT-realm.html>.

Replaces <#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1907849
gecko-commit: 5a48fdcefee755d543af0a415b6e1e216c853845
gecko-reviewers: smaug
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 13, 2024
@mbrodesser-Igalia
Bug 1907849: part 5) Add WPT <Element-setAttribute-respects-Elements-…
3bf3ba6 
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug

Replaces <web-platform-tests/wpt#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921
moz-wptsync-bot pushed a commit that referenced this pull request Dec 14, 2024
@mbrodesser-Igalia @moz-wptsync-bot
part 5) Add WPT <Element-setAttribute-respects-Elements-node-document…
9952c32 
…s-globals-CSP-after-adoption-from-TT-realm.html>.

Replaces <#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1907849
gecko-commit: 5a48fdcefee755d543af0a415b6e1e216c853845
gecko-reviewers: smaug
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Dec 14, 2024
@mbrodesser-Igalia
Bug 1907849: part 5) Add WPT <Element-setAttribute-respects-Elements-…
59f5467 
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug

Replaces <web-platform-tests/wpt#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Dec 16, 2024
@marco-c
Bug 1907849: part 5) Add WPT <Element-setAttribute-respects-Elements-…
852e09f 
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug

Replaces <web-platform-tests/wpt#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921

UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Dec 16, 2024
@marco-c
Bug 1907849: part 5) Add WPT <Element-setAttribute-respects-Elements-…
aa64c3f 
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug

Replaces <web-platform-tests/wpt#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921

UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Dec 16, 2024
@marco-c
Bug 1907849: part 5) Add WPT <Element-setAttribute-respects-Elements-…
83fa675 
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug

Replaces <web-platform-tests/wpt#46432>.

Differential Revision: https://phabricator.services.mozilla.com/D231921

UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
@ziransun ziransun closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrodesser-Igalia mbrodesser-Igalia mbrodesser-Igalia requested changes

@lukewarlow lukewarlow lukewarlow requested changes

@mikewest mikewest Awaiting requested review from mikewest

@annevk annevk Awaiting requested review from annevk

Assignees

@mikewest mikewest

Labels
trusted-types wg-s_webappsec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ziransun @lukewarlow @mbrodesser-Igalia @mikewest @wpt-pr-bot