Skip to content
This repository has been archived by the owner on Oct 16, 2020. It is now read-only.

Run the CORP check while processing navigation response #11

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

yutakahirano
Copy link
Collaborator

In order to run the CORP check for main resources for nested frames,
run the CORP check in the "process navigation response" algorithm.
This is needed because we want to run the CORP check against the
parent frame's origin, not the origin of the source browsing context.

Fixes whatwg/html#5308.

@yutakahirano
Copy link
Collaborator Author

@annevk @mikewest PTAL.

This change conflicts with #9 and #10, but please don't care. I'll rebase changes whenever necessary.
As CORP is checked only for nested frames, we don't need to check the destination in CORP. On the other hand, we need to run CORP from fetch (and related callers) only for "no-cors" requests.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 25, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 26, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 26, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
@yutakahirano
Copy link
Collaborator Author

ping

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 2, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745857}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 2, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745857}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Mar 2, 2020
This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745857}
@annevk
Copy link
Contributor

annevk commented Mar 2, 2020

I think this check is run too late as it wouldn't be run for redirect responses. This might be a problem in how set things up in HTML to begin with though.

@domenic and I were looking at this the other day and there might be room for improvement here.

E.g., I notice that the current setup would also not enforce X-Frame-Options set upon a redirect response, which seems somewhat bad.

@yutakahirano
Copy link
Collaborator Author

Thank you! I moved the insertion point from "process navigate a response" to "process navigate a fetch".

@mikewest
Copy link
Member

mikewest commented Mar 3, 2020

E.g., I notice that the current setup would also not enforce X-Frame-Options set upon a redirect response, which seems somewhat bad.

Note that Chrome doesn't enforce XFO on redirects. I don't think Firefox or Safari do either, though I haven't checked at all recently. See the data I pasted into web-platform-tests/wpt#21730 (comment), and the underlying discussion in https://crbug.com/835465.

That said, I think it's quite reasonable to make a different decision here and enforce COEP/CORP on redirect responses, since this mechanism is new and we can do the right thing from the get go.

index.bs Outdated
For every navigation request for a nested browsing context whose parent's embedder
policy is "`require-corp`", CORP is checked.

Run the following step before the sixth step of [=process a navigate fetch=].
Copy link
Member

Choose a reason for hiding this comment

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

Note that this runs before frame-ancestors checks, which sounds fine. I don't think there's any good reason to prefer one ordering to the other, but it's something we should probably lock down in tests so that the right set of things get reported consistently across browsers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack.

index.bs Outdated
Comment on lines 379 to 380
For every navigation request for a nested browsing context whose parent's embedder
policy is "`require-corp`", CORP is checked.
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to add this to the list of changes in https://mikewest.github.io/corpp/#integration-html (around line 333). It might be reasonable to drop this whole section in favor of simply monkey-patching there, or it may make sense to label the following as an algorithm that's called-out-to in the same way we do for "process a navigate response". I'm happy either way. Depending on which route you take, I'd adjust this text accordingly, perhaps copy/pasting the existing "process a navigation response" text, a la:

Suggested change
For every navigation request for a nested browsing context whose parent's embedder
policy is "`require-corp`", CORP is checked.
If a document's [=document/embedder policy=] is "`require-corp`", then any document it embeds in a
[=nested browsing context=] must positively assert a "`require-corp`" [=/embedder policy=] (see
[[#cascade-vs-require]]). Here, we modify HTML's [=process a navigation fetch=] algorithm to perform
the CORP check upon each redirect in a navigation chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I inlined the section. Does it look good?


1. If |response| is not a `Response` object, or the result of performing a
1. If |response| is not a `Response` object, or _event_'s request's associated request's
[=request/mode=] is "`no-cors`" and the result of performing a
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see what this algorithm looks like after #9 and #10 land. Can we get those in, and rebase this accordingly? It's hard to piece together otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rebased this change on top of #9. It is shown as a combined change on the review. dcd0828 shows the diff.

I believe #10 and #11 is almost independent each other because I stopped working on "process navigate a response" on this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now #9 is merged so hopefully this change is clear enough.

@yutakahirano yutakahirano force-pushed the yhirano/corp-navigation branch 2 times, most recently from dcd0828 to 3a1b259 Compare March 3, 2020 13:01
@annevk
Copy link
Contributor

annevk commented Mar 3, 2020

That sounds great and happy to give old policies an exception (for now).

@yutakahirano yutakahirano force-pushed the yhirano/corp-navigation branch from f670bc8 to 148ee1f Compare March 4, 2020 10:54
In order to run the CORP check for main resources for nested frames,
run the CORP check in the "process navigation response" algorithm.
This is needed because we want to run the CORP check against the
parent frame's origin, not the origin of the source browsing context.

Fixes whatwg/html#5308.
Co-Authored-By: Mike West <mike@mikewest.org>
@yutakahirano yutakahirano force-pushed the yhirano/corp-navigation branch from 148ee1f to 8fe7a55 Compare March 4, 2020 10:59
@yutakahirano
Copy link
Collaborator Author

I think I addressed all the comments. PTAL again.

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for iterating on this.

@yutakahirano yutakahirano merged commit f393ddc into WICG:master Mar 5, 2020
@yutakahirano yutakahirano deleted the yhirano/corp-navigation branch March 5, 2020 06:56
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Mar 5, 2020
…testonly

Automatic update from web-platform-tests
Run CORP on nested frame navigations

This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745857}

--

wpt-commits: 52191577ce292eed7bf42651764b8010b112ec8c
wpt-pr: 21943
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 5, 2020
…testonly

Automatic update from web-platform-tests
Run CORP on nested frame navigations

This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745857}

--

wpt-commits: 52191577ce292eed7bf42651764b8010b112ec8c
wpt-pr: 21943
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 6, 2020
…testonly

Automatic update from web-platform-tests
Run CORP on nested frame navigations

This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Cr-Commit-Position: refs/heads/master{#745857}

--

wpt-commits: 52191577ce292eed7bf42651764b8010b112ec8c
wpt-pr: 21943

UltraBlame original commit: 4f49d0f971bf8d187409fcd5cda3dab341b46bbc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Mar 9, 2020
…testonly

Automatic update from web-platform-tests
Run CORP on nested frame navigations

This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Cr-Commit-Position: refs/heads/master{#745857}

--

wpt-commits: 52191577ce292eed7bf42651764b8010b112ec8c
wpt-pr: 21943

UltraBlame original commit: 4f49d0f971bf8d187409fcd5cda3dab341b46bbc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 10, 2020
…testonly

Automatic update from web-platform-tests
Run CORP on nested frame navigations

This implements WICG/cross-origin-embedder-policy#11.

Change-Id: I7bb07f0616cc947c8a84fa140af58984046c7587
Bug: 887967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066358
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Cr-Commit-Position: refs/heads/master{#745857}

--

wpt-commits: 52191577ce292eed7bf42651764b8010b112ec8c
wpt-pr: 21943

UltraBlame original commit: 4f49d0f971bf8d187409fcd5cda3dab341b46bbc
@yutakahirano yutakahirano mentioned this pull request May 29, 2020
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

COEP, CORP and nested frame navigations
3 participants