[blink] Add connectionallowlist iframe attribute + IPC transport [chromium/src : main]
Giovanni Del Valle (Gerrit)
Message from Giovanni Del Valle![]( "Open in Gerrit")
Set Ready For Review
Related details
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 9 comments![]( "Open in Gerrit")
Second CL for Connection-Allowlist embedded enforcement (after theWould you mind collecting the relevant CLs here? See e.g. the commit message for https://chromium-review.googlesource.com/c/chromium/src/+/6985001.
It's quite a bit easier to follow the dependencies that way.
"ConnectionAllowlistEmbeddedEnforcement");Nit: Probably best to set `ConnectionAllowlist` as well.
kAllowlist, child_url)));Nit: Whitespace here is a little odd.
string? required_connection_allowlist;As noted in the previous CL: ideally, this would be a `network.mojom.ConnectionAllowlist?`. Is `response-origin` the reason you're avoiding this? If so, perhaps we should consider shifting our implementation to something more like CSP's `allow_self` (e.g. https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/content_security_policy.mojom;drc=a1b6be40cda9d857c4d94a0734df2b82c9a5e64c;l=78).
}As noted in the previous CL: Since this code more or less entirely replicates parsing the `csp` attribute, consider pulling this out into a helper function for this kind of attribute.
attributes->required_connection_allowlist = required_connection_allowlist_;As noted in the previous CL (and above), it seems reasonable to parse this in the renderer rather than the browser so that we're passing structured data around consistently.
[RuntimeEnabled=ConnectionAllowlistEmbeddedEnforcement, CEReactions, Reflect] attribute DOMString connectionAllowlist;Please add specific WPT coverage for this change (via IDLHarness or otherwise) in addition to updating the relevant test expectations noted in the test failures.
status: "experimental",Nit:
```suggestion
status: "experimental",
depends_on: ["ConnectionAllowlist"],
```
Related details
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
+Noam, FYI.
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
I'd like to see an Intent to Prototype for this.
Andrew Verge (Gerrit)
Andrew Verge added 2 comments![]( "Open in Gerrit")
Largely deferring this review to Mike as he has the most familiarity in this area. Mechanically things mostly LGTM and will +1 once his comments are resolved.
attributes->required_connection_allowlist = required_connection_allowlist_;As noted in the previous CL (and above), it seems reasonable to parse this in the renderer rather than the browser so that we're passing structured data around consistently.
+1 that ideally we'd use a parsed Connection Allowlist here and elsewhere in the renderer for consistency
Related details
- Brandon Maslen
- Giovanni Del Valle
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 3 comments![]( "Open in Gerrit")
Nit: Probably best to set `ConnectionAllowlist` as well.
Done
Nit: Whitespace here is a little odd.
Done
Nit:
```suggestion
status: "experimental",
depends_on: ["ConnectionAllowlist"],
```
- Andrew Verge
- Brandon Maslen
- Mike West
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Second CL for Connection-Allowlist embedded enforcement (after theWould you mind collecting the relevant CLs here? See e.g. the commit message for https://chromium-review.googlesource.com/c/chromium/src/+/6985001.
It's quite a bit easier to follow the dependencies that way.
Done
Related details
- Andrew Verge
- Brandon Maslen
- Mike West
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
status: "experimental",Giovanni Del ValleNit:
```suggestion
status: "experimental",
depends_on: ["ConnectionAllowlist"],
```
Done
Marked as unresolved due to build break. Taking the time to investigate.
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 4 comments![]( "Open in Gerrit")
As noted in the previous CL: ideally, this would be a `network.mojom.ConnectionAllowlist?`. Is `response-origin` the reason you're avoiding this? If so, perhaps we should consider shifting our implementation to something more like CSP's `allow_self` (e.g. https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/mojom/content_security_policy.mojom;drc=a1b6be40cda9d857c4d94a0734df2b82c9a5e64c;l=78).
Done. This is now network.mojom.ConnectionAllowlist?, and yes, response-origin was the reason for the earlier string. ConnectionAllowlist now carries a bool match_response_origin (comparable to allow_self)
As noted in the previous CL: Since this code more or less entirely replicates parsing the `csp` attribute, consider pulling this out into a helper function for this kind of attribute.
Done
attributes->required_connection_allowlist = required_connection_allowlist_;Andrew VergeAs noted in the previous CL (and above), it seems reasonable to parse this in the renderer rather than the browser so that we're passing structured data around consistently.
+1 that ideally we'd use a parsed Connection Allowlist here and elsewhere in the renderer for consistency
Done
[RuntimeEnabled=ConnectionAllowlistEmbeddedEnforcement, CEReactions, Reflect] attribute DOMString connectionAllowlist;Please add specific WPT coverage for this change (via IDLHarness or otherwise) in addition to updating the relevant test expectations noted in the test failures.
Done
Related details
- Andrew Verge
- Brandon Maslen
- Mike West
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
RuntimeEnabledFeatures::ConnectionAllowlistEnabled(I don't think you need this extra check if you use "depends_on" or "implied_by" in runtime_enabled features.
static const size_t kMaxLengthConnectionAllowlistAttribute = 4096;Why this number? Is this spec'ed, or something to do with implemenation? Add a comment.
if (value && (value.contains('\n') || value.contains('\r'))) {Do you need the boolean check? Seems redundant as the AtomicString would return false for the contains checks
GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(Not sure about adding a console warning for all of these. The console is extremely crowded.
Related details
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
- Mike West
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 6 comments![]( "Open in Gerrit")
I don't think you need this extra check if you use "depends_on" or "implied_by" in runtime_enabled features.
Done
static const size_t kMaxLengthConnectionAllowlistAttribute = 4096;Why this number? Is this spec'ed, or something to do with implemenation? Add a comment.
Done
if (value && (value.contains('\n') || value.contains('\r'))) {Do you need the boolean check? Seems redundant as the AtomicString would return false for the contains checks
Done
GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(Not sure about adding a console warning for all of these. The console is extremely crowded.
Done
} else if (value &&Giovanni Del ValleDitto re bool check
Done
status: "experimental",Giovanni Del ValleNit:
```suggestion
status: "experimental",
depends_on: ["ConnectionAllowlist"],
```
Giovanni Del ValleDone
Marked as unresolved due to build break. Taking the time to investigate.
Implemented the solution and fixed the build break associated.
Related details
- Andrew Verge
- Brandon Maslen
- Mike West
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 3 comments![]( "Open in Gerrit")
This looks pretty reasonable, thanks for taking another pass. I do think we can probably do better than holding the exact string, though. Perhaps `ConnectionAllowlist` could grow a serializer in a subsequent CL so that we're no longer passing the header through from the attribute directly, but instead ensuring that whatever we send the server is well-formed?
[blink] Add connectionallowlist iframe attribute + IPC transportSame nit as elsewhere: naming this CL with `[Connection-Allowlist]` makes it easier to find via the tagging system in Gerrit.
}Perhaps we could simplify this by parsing the header here rather than below, as that would reject `\n` and `\r` as part of the Structure Field parsing rules, and we could add the length restriction in `ParseConnectionAllowlistAttribute` where it might make more sense.
Related details
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
- Noam Rosenthal
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 2 comments![]( "Open in Gerrit")
[blink] Add connectionallowlist iframe attribute + IPC transportSame nit as elsewhere: naming this CL with `[Connection-Allowlist]` makes it easier to find via the tagging system in Gerrit.
Done
Perhaps we could simplify this by parsing the header here rather than below, as that would reject `\n` and `\r` as part of the Structure Field parsing rules, and we could add the length restriction in `ParseConnectionAllowlistAttribute` where it might make more sense.
ParseAttribute now validates by parsing the structured-field value. A malformed value is treated as no requirement (fail-open), matching csp. I also added a browsertest, AttributeWithControlCharactersNotDelivered, that sets a connectionallowlist with an embedded newline and asserts nothing reaches the browser.
Related details
- Andrew Verge
- Brandon Maslen
- Mike West
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Adding Rakina to review frame tree node changes and the new browser test.
Related details
- Andrew Verge
- Brandon Maslen
- Mike West
- Noam Rosenthal
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Gentle nudge that this CL is awaiting further review. PTAL, thanks.
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
- Mike West
- Noam Rosenthal
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 5 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | -1 |
5 comments
#include "services/network/public/cpp/connection_allowlist.h"Nit: move this include to the right place
// Connection-Allowlist embedded enforcement (the `connectionallowlist` iframeThis is a very heavy duty browser test. Is there a reason this is a browser test in the first place and not a set of WPT? This means that whatever is tested here cannot be made interoperable without converting it to WPT later.
"+url/gurl.h",Is that actually used? It seems to be included but not used.
GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(This shouldn't trigger "should_call_did_change_attributes_"?
// NOTE: Because `ConnectionAllowlist` is an origin-trial feature,This means that if we ship the header based solution first, this property would be shipped together with it but do nothing. This is unacceptable in terms of feature detection.
Related details
- Andrew Verge
- Brandon Maslen
- Giovanni Del Valle
- Mike West
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 3 comments![]( "Open in Gerrit")
#include "services/network/public/cpp/connection_allowlist.h"Nit: move this include to the right place
connection_allowlist_browsertest.cc maps to connection_allowlist.h via the _browsertest suffix even though this include was added in this CL. So git cl format sorts it first in its own block and moves it back to the top if it's placed in the services/network group
// Connection-Allowlist embedded enforcement (the `connectionallowlist` iframeThis is a very heavy duty browser test. Is there a reason this is a browser test in the first place and not a set of WPT? This means that whatever is tested here cannot be made interoperable without converting it to WPT later.
I've shaved down the browser test, but what remain assert browser-process state that has no web-exposed surface.
A WPT can observe the resulting allowed/blocked outcome but has no API to read the browser's stored requirement struct or the specific net error, so these checks can only run as browser tests.
Is that actually used? It seems to be included but not used.
- Andrew Verge
- Brandon Maslen
- Mike West
- Noam Rosenthal
- Rakina Zata Amni
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(This shouldn't trigger "should_call_did_change_attributes_"?
Triggering should_call_did_change_attributes_ here would propagate the cleared requirement, so a malformed value would drop the browser's enforcement entirely, which we specifically don't want for a security primitive, so we instead keep the last valid (stricter) requirement, mirroring the csp attribute above.
although the trade-off is a state divergence: the DOM attribute reads invalid while the browser keeps enforcing the previous value