[Connection-Allowlist] Add iframe embedded enforcement [chromium/src : main]
Giovanni Del Valle (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Adam Rice
- Arthur Sonzogni
- Brandon Maslen
- Chromium IPC Reviews
- Dominic Farolino
- Mike West
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: aj...@chromium.org, arthurs...@chromium.org, d...@chromium.org, mk...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): aj...@chromium.org, arthurs...@chromium.org, d...@chromium.org, mk...@chromium.org
Reviewer source(s):
aj...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Adam Rice
- Alex Gough
- Andrew Verge
- Arthur Sonzogni
- Brandon Maslen
- Dominic Farolino
- Mike West
- Philip Rogers
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 @Andrew Verge and @Mike West for review of changes for connection allowlist. @Arthur for render host changes. @Adam for loader changes. Adding @Dominic Farolino to CC.
Tried adding relevant reviewers, if you think someone is a closer fit for some changes, please let me know. Thanks!
Related details
- Adam Rice
- Alex Gough
- Andrew Verge
- Arthur Sonzogni
- Brandon Maslen
- Dan Clark
- Dominic Farolino
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
AllowCSPFromHeaderValue? allow_connection_allowlist_from;does this type need renaming in a refactor before this CL?
Related details
- Adam Rice
- Andrew Verge
- Arthur Sonzogni
- Brandon Maslen
- Dan Clark
- Dominic Farolino
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 10 comments![]( "Open in Gerrit")
Thank you Giovanni!
I let 9 comments below, but I would like a general agreement from @mk...@chromium.org about the web platform feature before doing a deeper code review.
namespace {
// A parsed Connection-Allowlist is usable for embedded enforcement only if it
// did not hit a fatal parse error. A malformed header yields a present but
// empty allowlist; treating that as a valid (maximally strict) assertion would
// let a garbage header satisfy the embedded-enforcement handshake, so we reject
// it.
bool IsConnectionAllowlistUsable(
const network::ConnectionAllowlist& allowlist) {
return !std::ranges::contains(
allowlist.issues,
network::mojom::ConnectionAllowlistIssue::kInvalidHeader) &&
!std::ranges::contains(
allowlist.issues,
network::mojom::ConnectionAllowlistIssue::kItemNotInnerList);
}
} // namespace
Move it to the preexisting anonymous namespace.
// TODO(https://crbug.com/11129645): MHTML iframe not supported yet.Can you please explain the difficulty? E.G. do we need to do something to support it?
Do you foresee a future where this would be supported?
const GURL& url = GetURL();nit: Can we not create a "url" variable? Just use "GetURL()" below?
network::ParseConnectionAllowlist(*frame_attribute, url);Note: we generally avoid C++ parsers called from the browser process. Should we parse it from the renderer process instead before sending it to the browser process?
I guess this one is fuzzers as a side effect of being part of ParsedHeaders. Unfortunately, the coverage seems to be zero at the moment:
https://analysis.chromium.org/coverage/p/chromium/file?host=chromium.googlesource.com&project=chromium/src&ref=refs/heads/main&revision=1ce3f7eb2f4f9cc392edb04fc56fa2f432201ec6&path=//services/network/public/cpp/connection_allowlist_parser.cc&platform=fuzz&test_suite_type=any&modifier_id=0
network::ParseConnectionAllowlist(*required_connection_allowlist_, url);rule of two.
// Store the required Connection-Allowlist (used when this frame embeds adocument
return ParseHeader(header_value, response_url);Consider rename this ParseConnectionAllowlist and not introducing a new middleman function with no added value.
string? required_connection_allowlist;If possible, we would prefer the parsed value so that we don't have to parse it in C++ in the browser process.
Related details
- Adam Rice
- Andrew Verge
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
Hi Giovanni! Thanks for this submission. I'll dig into some detail of the CL shortly, but first I'd suggest that it would be best for you, Andrew, and I to align on the implementation and behavior with regard to connection allowlists before you loop in all the reviewers for specific areas of the codebase. It's somewhat likely that some things will change, and it would be ideal to only ask for OWNERS stamps once if possible.
I'd also note that this is a pretty large change, and does more than a few things at once. It might be easier to review if you split it up. For example, you could split the parsing changes in //services/network from the behavioral changes, allowing reviewers to focus on those parts they're likely to know best and have the strongest opinions about.
I'd also also note that since the behavior we're aiming for isn't yet well defined and specified, it would be ideal to put in another runtime enabled feature for this functionality, separate from the existing `ConnectionAllowlist` flag as we might want to keep iterating on this piece even while shipping the rest.
I'll provide more detailed feedback shortly. Thanks!
Related details
- Adam Rice
- Andrew Verge
- Brandon Maslen
- Dan Clark
- Dominic Farolino
- Giovanni Del Valle