[PEPC] Not allow PEPC in cross-origin subframe without frame-ancestors [chromium/src : main]
0 views
Skip to first unread message
Thomas Nguyen (Gerrit)
Apr 16, 2024, 9:34:56 AMApr 16
to Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen
Message from Thomas Nguyen![]( "Open in Gerrit")
Set Ready For Review
Related details
Attention is currently required from:
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 16, 2024, 1:29:37 PMApr 16
to Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Thomas Nguyen . resolved
Hi Mason, could you please take a look at this?
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Apr 17, 2024, 9:07:10 PMApr 17
to Thomas Nguyen, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Mason Freed . resolved
Looks good to me! I'm not a CSP expert though - it'd be good if you got someone with more direct experience to also take another look.
Related details
Attention is currently required from:
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 18, 2024, 3:56:55 AMApr 18
to Antonio Sartori, Mason Freed, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Antonio Sartori
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Thomas Nguyen . resolved
Antonio, could you please take a look at the CL?
Basically, the PEPC requires the presence of a frame-ancestors directive. This directive is required if the document using PEPC is embedded in a cross origin to top-level frame.
We have considered several approaches:
- Always require frame-ancestors, even for the top-level frame. This would cause the PEPC to fail fast and block many use cases.
- Require frame-ancestors even in the same origin. This seemed unnecessary because the same-origin embedded case will be legitimate.
- Require frame-ancestors in cross-origin to any ancestor in the tree or any ancestor is cross-origin.
- Require frame-ancestors in cross-origin to the top-level frame only.
- We chose the last option because it aligns with the delegation model, where the top-level frame has more responsibility and privilege.
Related details
Attention is currently required from:
- Antonio Sartori
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Apr 18, 2024, 10:10:53 AMApr 18
to Thomas Nguyen, Mason Freed, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen
Antonio Sartori voted and added 4 comments![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
4 comments
Patchset-level comments
Antonio Sartori . unresolved
Thanks. The CL LGTM, I left a couple comments on the design doc though.
File third_party/blink/renderer/core/html/html_permission_element.cc
Line 251, Patchset 4 (Latest):
bool HasEnforceFrameAncestorsCSPPresent(ExecutionContext* context) {Antonio Sartori . unresolved
I generally wonder if it would make more sense to put this logic inside content_security_policy.cc. Ideally external components would not inspect the parsed CSP policies directly (even if they are exposed), but rely instead on common methods. This is a bit of a particular case though, since the logic is very ad-hoc (and there is not much logic at all).
Just for you to think about it, but it's fine to leave it here for me.
File third_party/blink/renderer/core/html/html_permission_element_test.cc
Line 856, Patchset 4 (Latest):
"frame-ancestors 'self' https://example.com"}};Antonio Sartori . unresolved
(nit) I think we generally prefer using reserved test hosts (like example.test and cross-example.test) in tests and avoid including in our source code domains that can actually be registered and exist for real
Line 890, Patchset 4 (Latest):
permission_service()->set_pepc_registered_callback(
base::BindOnce(&NotReachedForPEPCRegistered));Antonio Sartori . unresolved
(super nit, feel free to ignore): It would seem safer to do this by instantiating an object that resets the pepc_registered_callback when it is destroyed.
Related details
Attention is currently required from:
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 19, 2024, 6:57:03 AMApr 19
to AyeAye, Antonio Sartori, Mason Freed, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, dpr...@google.com, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Antonio Sartori and Mason Freed
Thomas Nguyen added 3 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/html_permission_element.cc
Line 251, Patchset 4:
bool HasEnforceFrameAncestorsCSPPresent(ExecutionContext* context) {Antonio Sartori . resolved
I generally wonder if it would make more sense to put this logic inside content_security_policy.cc. Ideally external components would not inspect the parsed CSP policies directly (even if they are exposed), but rely instead on common methods. This is a bit of a particular case though, since the logic is very ad-hoc (and there is not much logic at all).
Just for you to think about it, but it's fine to leave it here for me.
Thomas Nguyen
+1, I will make the move, thanks for the feedback
File third_party/blink/renderer/core/html/html_permission_element_test.cc
(nit) I think we generally prefer using reserved test hosts (like example.test and cross-example.test) in tests and avoid including in our source code domains that can actually be registered and exist for real
Thomas Nguyen
Done
Line 890, Patchset 4:
permission_service()->set_pepc_registered_callback(
base::BindOnce(&NotReachedForPEPCRegistered));Antonio Sartori . resolved
(super nit, feel free to ignore): It would seem safer to do this by instantiating an object that resets the pepc_registered_callback when it is destroyed.
Thomas Nguyen
That's an interesting point, I guess we might have to change the TestPermissionService as well. I will do that, when this pattern is expanded in the future.
Related details
Attention is currently required from:
- Antonio Sartori
- Mason Freed
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 19, 2024, 6:58:10 AMApr 19
to AyeAye, Antonio Sartori, Mason Freed, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, dpr...@google.com, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Antonio Sartori and Mason Freed
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Antonio Sartori . unresolved
Thanks. The CL LGTM, I left a couple comments on the design doc though.
Thomas Nguyen
I moved the function to CSP code, can you please take a look?
Related details
Attention is currently required from:
- Antonio Sartori
- Mason Freed
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Apr 19, 2024, 7:03:38 AMApr 19
to Thomas Nguyen, AyeAye, Mason Freed, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, dpr...@google.com, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Mason Freed and Thomas Nguyen
Antonio Sartori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
Attention is currently required from:
- Mason Freed
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Apr 19, 2024, 9:52:27 AMApr 19
to Thomas Nguyen, Antonio Sartori, AyeAye, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, dpr...@google.com, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen
Mason Freed voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Apr 19, 2024, 10:59:31 AMApr 19
to Thomas Nguyen, dpr...@google.com, Mason Freed, Antonio Sartori, AyeAye, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen
Dominique Fauteux-Chapleau removed dpr...@google.com from this change![]( "Open in Gerrit")
Deleted Reviewers:
Related details
Attention is currently required from:
- Thomas Nguyen
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 19, 2024, 4:47:29 PMApr 19
to Mason Freed, Antonio Sartori, AyeAye, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Thomas NguyenThanks. The CL LGTM, I left a couple comments on the design doc though.
I moved the function to CSP code, can you please take a look?
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Apr 19, 2024, 4:47:37 PMApr 19
to Mason Freed, Antonio Sartori, AyeAye, Chromium LUCI CQ, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Thomas Nguyen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Apr 19, 2024, 5:44:02 PMApr 19
to Thomas Nguyen, Mason Freed, Antonio Sartori, AyeAye, Andy Paicu, Kamila Hasanbega, chromium...@chromium.org, arthursonzog...@chromium.org, antoniosarto...@chromium.org, blink-revi...@chromium.org, mkwst+w...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
[PEPC] Not allow PEPC in cross-origin subframe without frame-ancestors
The frame-ancestors CSP directive must be explicitly included to allow a
document using PEPC to be embedded cross-origin (to the top level
frame). That's a security measure outlined in DD
https://docs.google.com/document/d/1a1gjlJ4VkAWoG8AeGKZDcQXm_c0q-cFTs_5MxmjWVYI/edit?tab=t.0
Explainer PR: https://github.com/WICG/PEPC/pull/15/commits
Fixed: 335834282
Bug: 1462930
Change-Id: Iacdf80a425498f0ee3e3a068f583446a333e4592
Reviewed-by: Antonio Sartori <antonio...@chromium.org>
Reviewed-by: Mason Freed <mas...@chromium.org>
Commit-Queue: Thomas Nguyen <tun...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290205}
Files:
- M third_party/blink/renderer/core/frame/csp/content_security_policy.cc
- M third_party/blink/renderer/core/frame/csp/content_security_policy.h
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/core/html/html_permission_element_test.cc
Change size: M
Delta: 5 files changed, 105 insertions(+), 24 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Mason Freed, +1 by Antonio Sartori
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages