[Direct Sockets] Implement dynamic enablement in Isolated Contexts [chromium/src : main]
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
Hey folks, PTAL at the respective parts of the stack.
Camille -- please check `navigation_request.cc` and `//content` changes -- is this the right place to intercept the navigation? Ideally I'd use a `WebContentsObserver`, but I'm not sure where to store it on the `//content` layer.
Daniel -- please check the flag propagation logic and the overall implementation.
Yoshisato -- please take a look at the flag override propagation to the dedicated worker & whether it's an acceptable solution :)
Related details
- Camille Lamy
- Daniel Cheng
- Yoshisato Yanagisawa
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")
depends_on: ["DirectSockets"],In the new scheme the flag is enabled once the isolation status of a process has been determined (which happens based on COOP/COEP headers), i.e. after header parsing (so the timing doesn't match).
I'm also wondering whether it's acceptable to keep the `IsolatedContext` visibility modifier -- it's not really needed in any other embedders, so it's more of a consistency question. @dch...@chromium.org
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
bool direct_sockets_force_enabled_in_parent = false;nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider naming this 'is_direct_sockets_force_enabled_in_parent'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Camille Lamy (Gerrit)
Camille Lamy added 2 comments![]( "Open in Gerrit")
Thanks! The plumbing in the worker code looks reasonable. I'll let Daniel comment on the rest of the Blink implementation. Do we have tests that check that DirectSockets are available or not when they're supposed to be?
#include "content/public/browser/navigation_controller.h"Why is this include removed?
Related details
- Andrew Rayskiy
- Daniel Cheng
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 3 comments![]( "Open in Gerrit")
I am not convinced the code style storing a flag inside GetRuntimeFeatureStateOverrideContext(). Is it natural way to do?
Also, is there an explainer etc for this behavior? All I can do for the area is verifying the implementation is aligned with the design approved by the security expert. Therefore, I hope to see such design.
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceEnabled();Will this runtime feature state only affects to this Execution Context? Or, can this be more global parameter?
Also, once the flag is enabled, the flag won't be disabled. Is it intended?
// then set worker global scope's cross-origin isolated capability to false."Upon https://html.spec.whatwg.org/#initialize-worker-policy-container, what happens if this is the about scheme?
I assume this step is Step 1 (Maybe updated since it was originally written?)
Or, is the behavior not specced yet?
Related details
- Andrew Rayskiy
- Daniel Cheng
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
Note that I cannot access the linked crbug.
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 3 comments![]( "Open in Gerrit")
I am not convinced the code style storing a flag inside GetRuntimeFeatureStateOverrideContext(). Is it natural way to do?
Also, is there an explainer etc for this behavior? All I can do for the area is verifying the implementation is aligned with the design approved by the security expert. Therefore, I hope to see such design.
As I wrote in the very first comment, I'll be happy to see any suggestions for a clearer/less invasive way of propagating the flag to workers.
The idea is outlined in [go/isolated-context-is-fun](http://go/isolated-context-is-fun); IsolatedContext is spec-ed [here](https://wicg.github.io/isolated-web-apps/isolated-contexts.html), Direct Sockets are spec-ed [here](https://wicg.github.io/direct-sockets/); my hope is to have @cl...@chromium.org and @dch...@chromium.org as security experts here :)
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceEnabled();Will this runtime feature state only affects to this Execution Context? Or, can this be more global parameter?
Also, once the flag is enabled, the flag won't be disabled. Is it intended?
Yes, this only affects the current execution context; example check for a different feature looks like [this](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/platform/runtime_enabled_features.cc;drc=7e4f7573fb027b6babb7fd8d086b295958b6f731;l=7722) (i.e. it first check the context-scoped override, and then defaults to the global per-process state). So it's intentional that it doesn't get reset.
// then set worker global scope's cross-origin isolated capability to false."Upon https://html.spec.whatwg.org/#initialize-worker-policy-container, what happens if this is the about scheme?
I assume this step is Step 1 (Maybe updated since it was originally written?)Or, is the behavior not specced yet?
I believe we never spec-ed this. Is this an issue? I was largely hoping to keep the same behavior that `IsolatedContext` currently provides.
Related details
- Daniel Cheng
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
3 comments
Andrew RayskiyI am not convinced the code style storing a flag inside GetRuntimeFeatureStateOverrideContext(). Is it natural way to do?
Also, is there an explainer etc for this behavior? All I can do for the area is verifying the implementation is aligned with the design approved by the security expert. Therefore, I hope to see such design.
As I wrote in the very first comment, I'll be happy to see any suggestions for a clearer/less invasive way of propagating the flag to workers.
The idea is outlined in [go/isolated-context-is-fun](http://go/isolated-context-is-fun); IsolatedContext is spec-ed [here](https://wicg.github.io/isolated-web-apps/isolated-contexts.html), Direct Sockets are spec-ed [here](https://wicg.github.io/direct-sockets/); my hope is to have @cl...@chromium.org and @dch...@chromium.org as security experts here :)
I feel the implementation reasonable.
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceEnabled();Andrew RayskiyWill this runtime feature state only affects to this Execution Context? Or, can this be more global parameter?
Also, once the flag is enabled, the flag won't be disabled. Is it intended?
Yes, this only affects the current execution context; example check for a different feature looks like [this](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/platform/runtime_enabled_features.cc;drc=7e4f7573fb027b6babb7fd8d086b295958b6f731;l=7722) (i.e. it first check the context-scoped override, and then defaults to the global per-process state). So it's intentional that it doesn't get reset.
Acknowledged
// then set worker global scope's cross-origin isolated capability to false."Andrew RayskiyUpon https://html.spec.whatwg.org/#initialize-worker-policy-container, what happens if this is the about scheme?
I assume this step is Step 1 (Maybe updated since it was originally written?)Or, is the behavior not specced yet?
I believe we never spec-ed this. Is this an issue? I was largely hoping to keep the same behavior that `IsolatedContext` currently provides.
- Andrew Rayskiy
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
3 comments
lgtm w/nits
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceDisabled();I guess I'm a little worried that this will be easy to overlook in the future. Is this specced/documented anywhere?
bool direct_sockets_enabled_in_parent = false,is there a reason this doesn't include "forced" here? If so, what's the distinction?
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 6 comments![]( "Open in Gerrit")
Hey folks, PTAL at the respective parts of the stack.
Camille -- please check `navigation_request.cc` and `//content` changes -- is this the right place to intercept the navigation? Ideally I'd use a `WebContentsObserver`, but I'm not sure where to store it on the `//content` layer.
Daniel -- please check the flag propagation logic and the overall implementation.
Yoshisato -- please take a look at the flag override propagation to the dedicated worker & whether it's an acceptable solution :)
Acknowledged
Andrew RayskiyI am not convinced the code style storing a flag inside GetRuntimeFeatureStateOverrideContext(). Is it natural way to do?
Also, is there an explainer etc for this behavior? All I can do for the area is verifying the implementation is aligned with the design approved by the security expert. Therefore, I hope to see such design.
Yoshisato YanagisawaAs I wrote in the very first comment, I'll be happy to see any suggestions for a clearer/less invasive way of propagating the flag to workers.
The idea is outlined in [go/isolated-context-is-fun](http://go/isolated-context-is-fun); IsolatedContext is spec-ed [here](https://wicg.github.io/isolated-web-apps/isolated-contexts.html), Direct Sockets are spec-ed [here](https://wicg.github.io/direct-sockets/); my hope is to have @cl...@chromium.org and @dch...@chromium.org as security experts here :)
I feel the implementation reasonable.
Acknowledged
#include "content/public/browser/navigation_controller.h"Why is this include removed?
Done
bool direct_sockets_force_enabled_in_parent = false;nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider naming this 'is_direct_sockets_force_enabled_in_parent'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceDisabled();I guess I'm a little worried that this will be easy to overlook in the future. Is this specced/documented anywhere?
I believe the idea was that `IsolatedContext` is an continuation of `CrossOriginIsolated`, hence it mirrors the conditions that reset `cross_origin_isolated_capability_` (and so do direct sockets). It's not specced IIUC.
is there a reason this doesn't include "forced" here? If so, what's the distinction?
- Camille Lamy
- Daniel Cheng
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 2 comments![]( "Open in Gerrit")
depends_on: ["DirectSockets"],In the new scheme the flag is enabled once the isolation status of a process has been determined (which happens based on COOP/COEP headers), i.e. after header parsing (so the timing doesn't match).
I'm also wondering whether it's acceptable to keep the `IsolatedContext` visibility modifier -- it's not really needed in any other embedders, so it's more of a consistency question. @dch...@chromium.org
(decided to keep this as is)
GetRuntimeFeatureStateOverrideContext()->SetDirectSocketsForceDisabled();Andrew RayskiyI guess I'm a little worried that this will be easy to overlook in the future. Is this specced/documented anywhere?
I believe the idea was that `IsolatedContext` is an continuation of `CrossOriginIsolated`, hence it mirrors the conditions that reset `cross_origin_isolated_capability_` (and so do direct sockets). It's not specced IIUC.
(discussed offline -- let's keep the status quo)
Related details
- Camille Lamy
- Daniel Cheng
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 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
1 comment
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Direct Sockets] Implement dynamic enablement in Isolated Contexts
This CL transitions Direct Sockets enablement to a dynamic model as
described in go/isolated-context-is-fun.
The goal is to allow Direct Sockets to remain available in
embedder-defined contexts (such as Chrome Apps, Extensions, and the
terminal on CrOS) while permitting a future tightening of the
"Isolated Context" definition that would otherwise hide these APIs.
Enablement is now explicitly calculated in the browser and propagated
to the renderer and its dedicated workers.
(See crrev.com/c/7642623 for a follow-up).
New Approach:
- Browser-side calculation in NavigationRequest::ReadyToCommitNavigation
- Propagation via a new 'direct_sockets_enabled_in_parent' field in
GlobalScopeCreationParams.
- Dedicated workers inherit this state from their parent context and
force-enable the feature in the worker's RuntimeFeatureStateContext
override.
Shortcomings & Scope:
- Shared and Service Workers are currently out of scope and default to
disabled. Since Direct Sockets were never launched for these types,
handling them separately is deferred.
- Permissions-Policy visibility is limited in embedder-defined
contexts (e.g., Chrome Apps, Extensions, or chrome-untrusted://terminal).
Capabilities like 'direct-sockets' will be enabled by default, but
cannot currently be disabled via headers in these contexts. This is
acceptable as these environments already gate access via manifest
permissions or internal allowlists.
- The implementation for dedicated workers is somewhat hacky as it
manually overrides the runtime feature state during initialization
to stay in sync with the parent's dynamic enablement.
NO_IFTTT=No changes to permissions policy exposure
- M content/browser/renderer_host/navigation_request.cc
- M services/network/public/cpp/permissions_policy/permissions_policy_features.json5
- M third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
- M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
- M third_party/blink/renderer/core/workers/dedicated_worker.cc
- M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.cc
- M third_party/blink/renderer/core/workers/dedicated_worker_global_scope.h
- M third_party/blink/renderer/core/workers/global_scope_creation_params.cc
- M third_party/blink/renderer/core/workers/global_scope_creation_params.h
- M third_party/blink/renderer/modules/direct_sockets/multicast_controller.idl
- M third_party/blink/renderer/modules/direct_sockets/tcp_server_socket.idl
- M third_party/blink/renderer/modules/direct_sockets/tcp_socket.idl
- M third_party/blink/renderer/modules/direct_sockets/udp_socket.idl
- M third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
Code-Review: +1 by Daniel Cheng, +1 by Camille Lamy, +1 by Yoshisato Yanagisawa
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |