[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths [chromium/src : main]
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland, Robert Sesek.
Ari Chivukula would like Ian Clelland and Robert Sesek to review this change.
[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths
Let's stop using our custom system and just defer to the CSP logic where
possible. This should be a no-op functionality wise.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: I81b3be6f406241e6d7f278c7070c2b60fbdfb3d2
---
M chrome/browser/web_applications/web_app_install_utils_unittest.cc
M content/browser/site_per_process_browsertest.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/renderer/core/html/html_iframe_element_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
9 files changed, 122 insertions(+), 135 deletions(-)
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland, Robert Sesek.
Robert Sesek (Gerrit)
Attention is currently required from: Ari Chivukula, Ian Clelland.
Patch set 5:Code-Review +1
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
2 comments:
File third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc:
Patch Set #9, Line 76:
url::Origin::Create(GURL("file:///test")), true, true,What are the implications of this change? Is this a case where we may have disallowing something before that would pass now?
File third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h:
Patch Set #9, Line 8:
#include "services/network/public/mojom/content_security_policy.mojom.h"Should this include `services/network/public/mojom/content_security_policy.mojom-blink.h` instead? That's where the type is defined; this file just includes some utilities that act on it.
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
2 comments:
File third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc:
Patch Set #9, Line 76:
url::Origin::Create(GURL("file:///test")), true, true,
What are the implications of this change? Is this a case where we may have disallowing something bef […]
I don't think so as there was no prior test for this case, I don't believe that delegating to files is meaningful here. I added this test when OriginWithWildcard was defined and simply codified existing behavior.
File third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h:
Patch Set #9, Line 8:
#include "services/network/public/mojom/content_security_policy.mojom.h"
Should this include `services/network/public/mojom/content_security_policy.mojom-blink. […]
I thought the -blink.h files were for inclusion in files outside common (that can't be used by the browser) only. Like, that file is referencing SecurityOrigins https://source.chromium.org/chromium/chromium/src/+/main:out/Debug/gen/services/network/public/mojom/content_security_policy.mojom-blink.h;l=310
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 9:Code-Review +1
1 comment:
File third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h:
Patch Set #9, Line 8:
#include "services/network/public/mojom/content_security_policy.mojom.h"
I thought the -blink. […]
You're right -- I somehow mistook this for services/network/public/cpp/content_security_policy/csp_source.h, which is actually included in the .cc file instead.
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng.
Ari Chivukula would like Daniel Cheng to review this change.
[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths
Let's stop using our custom system and just defer to the CSP logic where
possible. This should be a no-op functionality wise.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: I81b3be6f406241e6d7f278c7070c2b60fbdfb3d2
---
M chrome/browser/web_applications/web_app_install_utils_unittest.cc
M content/browser/site_per_process_browsertest.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/renderer/core/html/html_iframe_element_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
9 files changed, 130 insertions(+), 129 deletions(-)
Daniel Cheng (Gerrit)
Attention is currently required from: Ari Chivukula.
2 comments:
File third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h:
Patch Set #9, Line 22:
class BLINK_COMMON_EXPORT StructTraits<network::mojom::CSPSourceDataView,I think this is pre-existing, but I can't find a typemap configuration for CSPSource to OriginWithPossibleWildcards.
As a result I have two questions:
1. Does it *always* make sense to map CSPSource to OriginWithPossibleWildcards? It doesn't seem like it to me—it seems like OriginWithPossibleWildcards is a strict subset of CSPSource.
2. As such, should we define our own Mojo struct with just the fields we care about (e.g. dropping path and is_port_wildcard from the equation)? The Blink type can just wrap the underlying C++ type still, but that way, we only send the bits we actually allow over IPC instead of having comments about it.
Patch Set #9, Line 37:
static const std::string path(const blink::OriginWithPossibleWildcards&Nit: return a base::StringPiece here and return "" below instead of a temporary.
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 10:Auto-Submit +1Commit-Queue +1
2 comments:
File third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h:
Patch Set #9, Line 22:
class BLINK_COMMON_EXPORT StructTraits<network::mojom::CSPSourceDataView,
I think this is pre-existing, but I can't find a typemap configuration for CSPSource to OriginWithPo […]
The goal of this is to unblock the ability to use the deeper matching functionality of the CSP parser. Right now we want to keep only existing matching abilities, but there's a proposal to deepen this.
Further, there was a request by the TAG to re-use CSP logic rather than build a custom matcher (as I had) to reduce complexity and increase the chance of adoption by other browsers.
There's also a goal to move parsing into the network process (where CSP parsing occurs) and at that point it would make sense to generalize OriginWithPossibleWildcards to CSPSource (as a class).
Patch Set #9, Line 37:
static const std::string path(const blink::OriginWithPossibleWildcards&Nit: return a base::StringPiece here and return "" below instead of a temporary.
Done
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 10:Code-Review +1Commit-Queue +2
1 comment:
Patchset:
We talked offline. ari@ will try to add back the bespoke Mojo type for IPCs (since I originally approved that, not realizing that some of the fields should never be specified), but that will be in a followup. However, the underlying C++ types will still be unified to allow sharing of the CSP matching code.
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Avi Drissman, Daniel Cheng.
Ari Chivukula would like Avi Drissman to review this change.
[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths
Let's stop using our custom system and just defer to the CSP logic where
possible. This should be a no-op functionality wise.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: I81b3be6f406241e6d7f278c7070c2b60fbdfb3d2
---
M chrome/browser/web_applications/web_app_install_utils_unittest.cc
M content/browser/site_per_process_browsertest.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/renderer/core/html/html_iframe_element_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
9 files changed, 133 insertions(+), 132 deletions(-)
Ari Chivukula (Gerrit)
Attention is currently required from: Avi Drissman.
Ari Chivukula uploaded patch set #11 to this change.
[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths
Let's stop using our custom system and just defer to the CSP logic where
possible. This should be a no-op functionality wise.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
(4) Fork CSP Mojom to avoid sending unused data
Bug: 1418009
Change-Id: I81b3be6f406241e6d7f278c7070c2b60fbdfb3d2
---
M chrome/browser/web_applications/web_app_install_utils_unittest.cc
M content/browser/site_per_process_browsertest.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/renderer/core/html/html_iframe_element_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
9 files changed, 133 insertions(+), 132 deletions(-)
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Avi Drissman (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 12:Code-Review +1Commit-Queue +2
To view, visit change 4345816. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Permissions Policy + CSP] (3) Use CSP parsing and comparison paths
Let's stop using our custom system and just defer to the CSP logic where
possible. This should be a no-op functionality wise.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
(4) Fork CSP Mojom to avoid sending unused data
Bug: 1418009
Change-Id: I81b3be6f406241e6d7f278c7070c2b60fbdfb3d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4345816
Auto-Submit: Ari Chivukula <ari...@chromium.org>
Commit-Queue: Avi Drissman <a...@chromium.org>
Reviewed-by: Ian Clelland <icle...@chromium.org>
Reviewed-by: Robert Sesek <rse...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Avi Drissman <a...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123611}
---
M chrome/browser/web_applications/web_app_install_utils_unittest.cc
M content/browser/site_per_process_browsertest.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/renderer/core/html/html_iframe_element_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
9 files changed, 133 insertions(+), 132 deletions(-)