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

Add a javascript: URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution #49403

Merged

Conversation

mbrodesser-Igalia
Copy link
Contributor

Preparation for fixing whatwg/html#4651.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Really clever work here at creating a deterministic test for the differences in approaches. I might have spotted a bug, or maybe I just managed to confuse myself...

@mbrodesser-Igalia mbrodesser-Igalia changed the title Add a javascript: URL navigation test which checks the parent's CSP was snapshotted Add a javascript: URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution Dec 3, 2024
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This looks like a great strategy! I will approve since I think the things I found do not affect test correctness. But let me know if you want another double-check after changing those.

const iframe = document.getElementById("iframe");
iframe.contentWindow.location.href = "javascript:parent.f()";
addCSP();
iframe.contentWindow.location.href = "javascript:g()";
Copy link
Member

Choose a reason for hiding this comment

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

Is the iframe necessary or helpful here anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it ensures the parent's CSP is checked.

A separate test for checking the target's CSP is needed. The desired behavior for that is unclear to me, given whatwg/html#4651 (comment). I'll test locally and open a separate PR.

@mbrodesser-Igalia mbrodesser-Igalia merged commit 4d21424 into master Dec 4, 2024
19 checks passed
@mbrodesser-Igalia mbrodesser-Igalia deleted the AddWptForJavascriptUrlNavigationCheckingSnapshotting branch December 4, 2024 09:45
@mbrodesser-Igalia
Copy link
Contributor Author

@domenic: the test is still wrong. It may pass erroneously, because:

https://github.com/web-platform-tests/wpt/pull/49403/files#diff-7827e8417fc230100eb3bf6171e46dc9330bafb73831ff7c1234d40709ab1474R17

doesn't necessarily immediately schedule a task for a "securitypolicyviolation". If the snapshotted CSP is checked during task execution, a task for "javascript-h" will be scheduled. In that case the behavior is:

     [...]
      // Actually after step 3:
      //   Queue 1: [javascript-f, javascript-g]
      //   After javascript-f:
      //     Queue 1: [javascript-g, javascript-h]
      //     After javascript-g:
      //       Queue 1: [javascript-h], Queue 2: [e1]
      //       After javascript-h:
      //         Queue 2: [e1, e2]

So the the order of e1, e2 would let the test pass.

Potentially a fix is to change https://github.com/web-platform-tests/wpt/pull/49403/files#diff-7827e8417fc230100eb3bf6171e46dc9330bafb73831ff7c1234d40709ab1474R17
to iframe.src = "http://example.com"; because that immediately queues a "securitypolicyviolation", relying on other functionality than the one here being tested.
However, the setter for iframe.src is pretty complex, so I don't know if that behavior is specified. Do you know?

@domenic
Copy link
Member

domenic commented Dec 6, 2024

However, the setter for iframe.src is pretty complex, so I don't know if that behavior is specified. Do you know?

I traced through the spec for iframe.src and it synchronously reaches https://html.spec.whatwg.org/#navigate .

We then go in-parallel. The request gets created in https://html.spec.whatwg.org/#create-navigation-params-by-fetching ... and we run into whatwg/html#10796 , ugh. So it's not even well-defined what CSP is consulted.

I'll comment over there, but, this is discouraging.

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.

4 participants