Make //content have no baseline permissions policy for isolated apps [chromium/src : main]
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 2 comments![]( "Open in Gerrit")
Allowing it to be optional (and falling back to how regular pagesSooo do I see it right that after this change an IWA that does not specify the `permissions_policy` field within the manifest at all Chrome will just rely 100% on the headers? Effectively creating a bypass for a whole permissions-in-the-manifest mechanism? If so, that would be a strong -1 from me.
return std::vector<blink::mojom::IsolatedAppPermissionPolicyEntryPtr>();There's another aspect to this though. There is an explicit difference between the following situations:
- We don't have permissions policies cached for this IWA at all.
- We have the permissions policy but it's just empty.
Usually the former is a sign of a grave error somewhere (e.g. manifest parsing failure)—while the latter is perfectly okay.
Also, this is the main real use case of this being `optional`. If this is changed, the method should just return a vector (and only make it empty sometimes).
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Code-Review | +1 |
2 comments
Allowing it to be optional (and falling back to how regular pagesSooo do I see it right that after this change an IWA that does not specify the `permissions_policy` field within the manifest at all Chrome will just rely 100% on the headers? Effectively creating a bypass for a whole permissions-in-the-manifest mechanism? If so, that would be a strong -1 from me.
Okay, I reevaluated it, the behavior here makes sense.
return std::vector<blink::mojom::IsolatedAppPermissionPolicyEntryPtr>();There's another aspect to this though. There is an explicit difference between the following situations:
- We don't have permissions policies cached for this IWA at all.
- We have the permissions policy but it's just empty.
Usually the former is a sign of a grave error somewhere (e.g. manifest parsing failure)—while the latter is perfectly okay.
Also, this is the main real use case of this being `optional`. If this is changed, the method should just return a vector (and only make it empty sometimes).
Nevermind, after discussing offline and re-reading things here it does make sense.
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
Hey Camille, PTAL at `//content` files. Ideally the change should be a no-op for `//chrome` IWAs :)
Related details
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 5 comments![]( "Open in Gerrit")
Thanks! A few comments below.
// This function intentionally returns an empty vector (which is equal to aOk so that I understand correctly: in regular operations this will always return a non-optional vector, but we're keeping the optional return value because we don't want a value in content/browsertests. Is that correct?
If so, it feels a bit annoying that we're having a less optimal return type just for tests. I wonder if it would make sense to split this into two functions, one which checks whether we need to query for a baseline or not, and one which checks what the baseline actually is when we know we need to look for a baseline. Or maybe we can have a test only function that just overrides prevent the ask for a baseline in tests, with a ForTesting suffix that ensures it's only used in testing code.
class UrlBuilder {Since this is exposed in the general content::testing namespace, could we maybe name it something a bit more specific?
bool ShellContentBrowserClient::ShouldUrlUseApplicationIsolationLevel(Camille LamyWeb platform tests run on a known origin with an unknown port; this origin is passed to `GetIsolatedContextOriginSetFromFlag()` via cmdline.
`IsIsolatedContextAllowedForUrl()` (on the left) uses `ProcessLock().GetProcessLockURL()` which strips the port, whereas `ShouldUrlUseApplicationIsolationLevel()` includes it -> hence the tweak.
Ah yes this an issue for a bunch of tests that hard code isolated contexts.
auto url_without_port = GURL(You should use GURL::Replacements to clear the port. Example of usage: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=1052?q=url::replacement%20siteinfo&ss=chromium%2Fchromium%2Fsrc
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
// This function intentionally returns an empty vector (which is equal to aOk so that I understand correctly: in regular operations this will always return a non-optional vector, but we're keeping the optional return value because we don't want a value in content/browsertests. Is that correct?
If so, it feels a bit annoying that we're having a less optimal return type just for tests. I wonder if it would make sense to split this into two functions, one which checks whether we need to query for a baseline or not, and one which checks what the baseline actually is when we know we need to look for a baseline. Or maybe we can have a test only function that just overrides prevent the ask for a baseline in tests, with a ForTesting suffix that ensures it's only used in testing code.
Yes, that's correct! I guess I'll split it, this sounds more than reasonable.
One minor correction: web platform tests run on top of `content_shell`, but the `content_shell` itself isn't a test-only target.
Related details
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Auto-Submit | +1 |
3 comments
// This function intentionally returns an empty vector (which is equal to aAndrew RayskiyOk so that I understand correctly: in regular operations this will always return a non-optional vector, but we're keeping the optional return value because we don't want a value in content/browsertests. Is that correct?
If so, it feels a bit annoying that we're having a less optimal return type just for tests. I wonder if it would make sense to split this into two functions, one which checks whether we need to query for a baseline or not, and one which checks what the baseline actually is when we know we need to look for a baseline. Or maybe we can have a test only function that just overrides prevent the ask for a baseline in tests, with a ForTesting suffix that ensures it's only used in testing code.
Yes, that's correct! I guess I'll split it, this sounds more than reasonable.
One minor correction: web platform tests run on top of `content_shell`, but the `content_shell` itself isn't a test-only target.
Done
Since this is exposed in the general content::testing namespace, could we maybe name it something a bit more specific?
Done
You should use GURL::Replacements to clear the port. Example of usage: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=1052?q=url::replacement%20siteinfo&ss=chromium%2Fchromium%2Fsrc
Done
Related details
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 2 comments![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
2 comments
auto* cbc = GetContentClient()->browser();Let's avoid calling this cbc. I think it is fine to just call both methods directly on GetContentClient()->browser(), and more in style with the rest of the file.
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Auto-Submit | +1 |
| Commit-Queue | +2 |
1 comment
Let's avoid calling this cbc. I think it is fine to just call both methods directly on GetContentClient()->browser(), and more in style with the rest of the file.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
23 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/renderer_host/render_frame_host_impl.cc
Insertions: 17, Deletions: 10.
@@ -12532,16 +12532,19 @@
DCHECK_EQ(this, navigation_request->GetRenderFrameHost());
AssertBrowserContextShutdownHasntStarted();
- auto* cbc = GetContentClient()->browser();
if (IsOutermostMainFrame() &&
GetSiteInstance()
->GetWebExposedIsolationInfo()
.is_isolated_application() &&
- cbc->SupportsBaselinePermissionsPolicyForIsolatedApp()) {
+ GetContentClient()
+ ->browser()
+ ->SupportsBaselinePermissionsPolicyForIsolatedApp()) {
commit_params->isolated_app_policy =
- cbc->GetBaselinePermissionsPolicyForIsolatedApp(
- GetBrowserContext(),
- url::Origin::Create(navigation_request->GetURL()));
+ GetContentClient()
+ ->browser()
+ ->GetBaselinePermissionsPolicyForIsolatedApp(
+ GetBrowserContext(),
+ url::Origin::Create(navigation_request->GetURL()));
}
bool is_same_document =
@@ -14296,14 +14299,18 @@
// Interpretation of the permission policies and merging is done in the
// renderer. However, as it is untrusted, we perform a sanity check whether
// it did not add any new policies which would mean it's been compromised.
- auto* cbc = GetContentClient()->browser();
if (!header_policy.empty() &&
- cbc->SupportsBaselinePermissionsPolicyForIsolatedApp() &&
+ GetContentClient()
+ ->browser()
+ ->SupportsBaselinePermissionsPolicyForIsolatedApp() &&
!VerifyHeaderPermissionsPolicyAgainstBaseline(
header_policy,
- cbc->GetBaselinePermissionsPolicyForIsolatedApp(
- GetBrowserContext(),
- GetSiteInstance()->GetWebExposedIsolationInfo().origin()))) {
+ GetContentClient()
+ ->browser()
+ ->GetBaselinePermissionsPolicyForIsolatedApp(
+ GetBrowserContext(), GetSiteInstance()
+ ->GetWebExposedIsolationInfo()
+ .origin()))) {
bad_message::ReceivedBadMessage(
GetProcess(), bad_message::BadMessageReason::
RFH_NEW_ISOLATED_WEB_APP_PERMISSION_POLICIES);
```
Change information
Make //content have no baseline permissions policy for isolated apps
Baseline permissions policy (aka high-watermark permissions policy) is
strictly an Isolated Web Apps concept for the //chrome layer -- it's not
exactly spec-ed and isn't a hard requirement for other embedders.
Allowing it to be optional outside of //chrome (and falling back to how
regular pages behave) eliminates the need to define approximate baseline
policies in WPTs and //content browser tests.
In addition this CL opts WPTs relying on isolated-context-origins into
proper process-level isolation instead of pretending that they're
running in embedder-defined IsolatedContext.
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/chrome_content_browser_client.h
- M content/browser/direct_sockets/direct_sockets_open_browsertest.cc
- M content/browser/direct_sockets/direct_sockets_tcp_browsertest.cc
- M content/browser/direct_sockets/direct_sockets_test_utils.cc
- M content/browser/direct_sockets/direct_sockets_test_utils.h
- M content/browser/direct_sockets/direct_sockets_udp_browsertest.cc
- M content/browser/renderer_host/isolated_web_app_permissions_policy_unittest.cc
- M content/browser/renderer_host/isolated_web_app_throttle_browsertest.cc
- M content/browser/renderer_host/isolated_web_app_throttle_unittest.cc
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_frame_host_impl.h
- M content/browser/renderer_host/render_frame_host_impl_browsertest.cc
- M content/browser/smart_card/smart_card_browsertest.cc
- M content/public/browser/content_browser_client.cc
- M content/public/browser/content_browser_client.h
- M content/shell/browser/shell_content_browser_client.cc
- M content/shell/browser/shell_content_browser_client.h
- M content/test/content_test_bundle_data.filelist
- D content/test/data/direct_sockets/open.html.mock-http-headers
- D content/test/data/direct_sockets/tcp.html.mock-http-headers
- D content/test/data/direct_sockets/udp.html.mock-http-headers
- M third_party/blink/web_tests/external/wpt/direct-sockets/tcp_socket.https.html.headers
- M third_party/blink/web_tests/external/wpt/direct-sockets/udp_socket.https.html.headers
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/socket-default.php
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/tcp-socket-success.php
- M third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/udp-socket-success.php
Code-Review: +1 by Zgroza (Luke) Klimek, +1 by Camille Lamy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/58303
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |