[CSP]: Do not block same-document navigations. [chromium/src : main]
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori.
Arthur Sonzogni would like Antonio Sartori to review this change.
[CSP]: Do not block same-document navigations.
A cross-origin initiated same-document navigation caused crash when
blocked by CSP.
Stop blocking it + WPT regression test.
This is #9 Mac crasher on M95 stable. So expect M96 (beta) cherry-pick.
That's probably not enough for cherry-pick M95 (stable).
Bug:1262203
Change-Id: Ie70f77bd9ec69ac0659321f2e8e626b2bd091126
---
A third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js
M content/browser/renderer_host/navigation_request.cc
2 files changed, 51 insertions(+), 0 deletions(-)
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori.
1 comment:
Patchset:
Hi Antonio,
Could you please take a look?
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Attention is currently required from: Antonio Sartori.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/31413.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Antonio Sartori (Gerrit)
Attention is currently required from: Arthur Sonzogni.
Patch set 1:Code-Review +1
2 comments:
Patchset:
Thanks! LGTM, with a suggestion for improving the test. Since this is a fix for a crash which we'll probably want to merge back, it's fine to merge it as is, but I think it would be nice to extend the test in a follow up as I am proposing since that would ensure that browsers behave the same here.
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #1, Line 29:
await new Promise(resolve => test.step_timeout(resolve, 1500));I think it would be actually nice to check that the iframe navigated and CSP did not block anything. I suggest:
1) checking from the iframe that the iframe navigated to the fragment
2) adding a spv listener that fails the test
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #1, Line 29:
await new Promise(resolve => test.step_timeout(resolve, 1500));
I think it would be actually nice to check that the iframe navigated and CSP did not block anything. […]
Good idea! Done.
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori.
Arthur Sonzogni removed a vote from this change.
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori.
1 comment:
Patchset:
Thanks! LGTM, with a suggestion for improving the test. […]
Ooops, I didn't realized you wanted it for a follow-up. I did it here. Could you please take another look?
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Antonio Sartori (Gerrit)
Attention is currently required from: Arthur Sonzogni.
Patch set 3:Code-Review +1
8 comments:
Patchset:
A few very small nits, but still LGTM.
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #1, Line 29:
await new Promise(resolve => test.step_timeout(resolve, 1500));
Good idea! Done.
Thanks. Why don't you remove this blind timeout and instead use test.step_wait on the condition that await child.execute_script(() => windoc.counter) returns 1?
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #3, Line 8:
doesn"tnit: doesn't
Patch Set #3, Line 9:
doesn"tnit: doesn't
Patch Set #3, Line 20:
navigationnit: navigations
Patch Set #3, Line 30:
document.getElementsByTagName("head")[0]doesn't just `document.head` work?
nit: Give
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/support/executor.html:
Patch Set #3, Line 2:
those helper filesuper super super nit: "these helper files"
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori.
Patch set 5:Commit-Queue +2
9 comments:
Patchset:
Thanks!
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #1, Line 29:
await new Promise(resolve => test.step_timeout(resolve, 1500));
Thanks. Why don't you remove this blind timeout and instead use test. […]
I went with a promise instead of a waiting loop.
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js:
Patch Set #3, Line 8:
doesn"tnit: doesn't
Done
Patch Set #3, Line 8:
doesn"tnit: doesn't
Done
Patch Set #3, Line 9:
doesn"tnit: doesn't
Done
Patch Set #3, Line 20:
navigationnit: navigations
Done
Patch Set #3, Line 30:
document.getElementsByTagName("head")[0]
doesn't just `document. […]
Done
nit: Give
Done
File third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/support/executor.html:
Patch Set #3, Line 2:
those helper filesuper super super nit: "these helper files"
Done
To view, visit change 3247135. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js
Insertions: 9, Deletions: 10.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/support/executor.html
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
[CSP]: Do not block same-document navigations.
A cross-origin initiated same-document navigation caused crash when
blocked by CSP.
Stop blocking it + WPT regression test.
This is #9 Mac crasher on M95 stable. So expect M96 (beta) cherry-pick.
That's probably not enough for cherry-pick M95 (stable).
Bug: 1262203
Change-Id: Ie70f77bd9ec69ac0659321f2e8e626b2bd091126
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3247135
Commit-Queue: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Antonio Sartori <antonio...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#935920}
---
A third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/support/executor.html
A third_party/blink/web_tests/external/wpt/content-security-policy/frame-src/frame-src-cross-origin-same-document-navigation.window.js
M content/browser/renderer_host/navigation_request.cc
3 files changed, 81 insertions(+), 0 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/31413