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

Report COEP violations in nested frame loading #10

Merged
merged 5 commits into from
Mar 18, 2020

Conversation

yutakahirano
Copy link
Collaborator

Modify the "check a navigation response’s adherence to its
embedder’s policy" logic so as to report (possibly potential)
COEP violations.

@yutakahirano
Copy link
Collaborator Author

yutakahirano commented Feb 20, 2020

@mikewest @annevk PTAL.

Notes:

Child frame's "report only value" is not taken into account because we block the navigation only when the child's policy is "unsafe-none", and we cannot attach a reporting endpoint when the value is "unsafe-none"

We attach "child cross-origin-embedder-policy" and "child cross-origin-embedder-policy (report only)" only when the report is made for "report only". Let's think about the following example:

parent:

  • Cross-Origin-Embedder-Policy: <missing>
  • Cross-Origin-Embedder-Policy-Report-Only: require-corp; endpoint="e1"

child (response):

  • Cross-Origin-Embedder-Policy: <missing>
  • Cross-Origin-Embedder-Policy-Report-Only: require-corp; endpoint="e2"

If the site owner updates the policy for the parent and the child at the same time, or update the policy for the child first, that should not be a problem. On the other hand, when the site owner updates the policy for the parent first, we'll see a block, so it is correct to report the potential blocking.

The user agent doesn't know the site owner's plan, so we report the case with COEP values so that the site owner can decide whether to take an action on or ignore the report.

@yutakahirano
Copy link
Collaborator Author

ping

1 similar comment
@yutakahirano
Copy link
Collaborator Author

ping

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.

Generally looks pretty good! Small comments.

index.bs Outdated
4. Let |body| be a new object containing the following properties with keys:

* key: "`blocked`", value: |serialized blocked url|.
* key: "`child cross-origin-embedder-policy`", value: |response policy|'s
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to reveal this? I don't think there's much risk in saying that the resource asserted same-origin vs same-site, but it doesn't seem like we should be giving that information away cross-origin in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

@arturjanc to have an opinion about this (or delegate!) from the server-side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point. @annevk is concerned about leaking information through redirects. On the other hand, if we reveal COEP values only when the request's url is same origin with the origin of the parent context, that reveals one-bit information, same-originness. Is it too subtle?

I'm fine with revealing the COEP values only when the request's url is same origin with the origin of the parent context.

Copy link
Member

Choose a reason for hiding this comment

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

I'm asking whether we need to reveal the value at all. "It doesn't match, otherwise I wouldn't be getting this violation report." seems like enough?

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 stated why this information is needed at #10 (comment) .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mike, does my above comment answer your question?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't, unfortunately. But let's land this as-is and argue about this detail offline. :)

@annevk
Copy link
Contributor

annevk commented Mar 9, 2020

The user agent doesn't know the site owner's plan, so we report the case with COEP values so that the site owner can decide whether to take an action on or ignore the report.

I don't understand what this is saying.

@yutakahirano
Copy link
Collaborator Author

The user agent doesn't know the site owner's plan, so we report the case with COEP values so that the site owner can decide whether to take an action on or ignore the report.

I don't understand what this is saying.

Does the previous paragraph make sense to you?

If the site owner updates the policy for the parent and the child at the same time, or update the policy for the child first, that should not be a problem. On the other hand, when the site owner updates the policy for the parent first, we'll see a block, so it is correct to report the potential blocking.

@annevk
Copy link
Contributor

annevk commented Mar 9, 2020

I see. Yeah, we should report when there would be a block (and not make that depend on report only values or some such).

@annevk
Copy link
Contributor

annevk commented Mar 9, 2020

I guess I don't see how it relates to needing to report the actual COEP values though. The result being blocked or allowed seems sufficient?

@yutakahirano
Copy link
Collaborator Author

Hmm, then the site owner needs to filter out reports by looking at "url" and "blocked-url". And that sounds OK. I'll remove the policy part.

Modify the "check a navigation response’s adherence to its
embedder’s policy" logic so as to report (possibly potential)
COEP violations.
@yutakahirano yutakahirano force-pushed the yhirano/report-to-navigation branch from 7585d21 to d456297 Compare March 9, 2020 12:53
@yutakahirano
Copy link
Collaborator Author

Done. @annevk, can you take a look again?

@annevk
Copy link
Contributor

annevk commented Mar 9, 2020

I share Mike's concern about the logic duplication. It seems we should be able to do this in a single pass whereby the reporting and actual blocking are both conditional on what policy is in effect.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 10, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
@yutakahirano
Copy link
Collaborator Author

I share Mike's concern about the logic duplication. It seems we should be able to do this in a single pass whereby the reporting and actual blocking are both conditional on what policy is in effect.

Done. @annevk, can you take a look again?

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

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 11, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 11, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 11, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749129}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 11, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749129}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Mar 11, 2020
Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749129}
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Mar 13, 2020
… a=testonly

Automatic update from web-platform-tests
Implement COEP reporting for navigation

Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749129}

--

wpt-commits: 7276e150b9b597e1e688fd1c892454411952e37a
wpt-pr: 22159
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 13, 2020
… a=testonly

Automatic update from web-platform-tests
Implement COEP reporting for navigation

Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749129}

--

wpt-commits: 7276e150b9b597e1e688fd1c892454411952e37a
wpt-pr: 22159
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Mar 16, 2020
… a=testonly

Automatic update from web-platform-tests
Implement COEP reporting for navigation

Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#749129}

--

wpt-commits: 7276e150b9b597e1e688fd1c892454411952e37a
wpt-pr: 22159

UltraBlame original commit: e7f3bc915e2476ecececc2f15e5d5ee3baed90f9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Mar 16, 2020
… a=testonly

Automatic update from web-platform-tests
Implement COEP reporting for navigation

Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#749129}

--

wpt-commits: 7276e150b9b597e1e688fd1c892454411952e37a
wpt-pr: 22159

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

Automatic update from web-platform-tests
Implement COEP reporting for navigation

Implements WICG/cross-origin-embedder-policy#10

Bug: 1052764
Change-Id: I76464069d3a0a4ea1463a2e626f5e0b355f175ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091326
Commit-Queue: Yutaka Hirano <yhiranochromium.org>
Reviewed-by: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#749129}

--

wpt-commits: 7276e150b9b597e1e688fd1c892454411952e37a
wpt-pr: 22159

UltraBlame original commit: e7f3bc915e2476ecececc2f15e5d5ee3baed90f9
@yutakahirano
Copy link
Collaborator Author

Let me land this for now. If you have further comments please let me know, I'll be happy to address them.

@yutakahirano yutakahirano merged commit 535fc0e into WICG:master Mar 18, 2020
@yutakahirano yutakahirano deleted the yhirano/report-to-navigation branch March 18, 2020 04:59
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.

3 participants