[Connection-Allowlist] Enforce for FedCM API [chromium/src : main]
Xiaochen Zhou (Gerrit)
Xiaochen Zhou added 1 comment![]( "Open in Gerrit")
+averge@ for a first pass, thank you
Related details
- Andrew Verge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge voted Code-Review+1![]( "Open in Gerrit")
- Xiaochen Zhou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaochen Zhou (Gerrit)
Xiaochen Zhou added 1 comment![]( "Open in Gerrit")
+cbiesinger@ for
* code review as FedCM API owner for
* chrome/test/chromedriver/
* content/browser/webid/
* owner approval of
* content/browser/webid/
* VirtualTestSuites
Thank you.
Related details
- Christian Biesinger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaochen Zhou (Gerrit)
Xiaochen Zhou added 1 comment![]( "Open in Gerrit")
+yigu@ for code review as FedCM API owner for
- chrome/test/chromedriver/
- content/browser/webid/
and also owner approval of
- content/browser/webid/
Thank you.
Related details
- Yi Gu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yi Gu (Gerrit)
Yi Gu added 4 comments![]( "Open in Gerrit")
FrameTreeNode* node = FrameTreeNode::GloballyFindByID(frame_tree_node_id_);Q: how does the connection allowlist work for cross-origin/site iframes?
node->current_frame_host()->policy_container_host()->policies(),This could be null during teardown etc. Should we check for nullptr first?
return;This early return makes devtools be unaware of the failure. See line 191 below.
FrameTreeNode* node = FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
if (node && node->current_frame_host() &&
!ConnectionAllowlistAllowsUrlAndReportIfNeeded(
node->current_frame_host()->policy_container_host()->policies(),
resource_request->url)) {
if (callback) {
std::move(callback).Run(/*response_body=*/std::nullopt,
net::ERR_NETWORK_ACCESS_REVOKED, /*mime_type=*/"",
/*cors_error=*/false);
}
return;
}Could you leave some comments briefly explaining about why we are checking this?
Related details
- Xiaochen Zhou
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 22 comments![]( "Open in Gerrit")
// Dialog due to token fetch failure), the dialog should not be marked closed.Most fetch failures are asynchronous, so IMO it would be better to be more explicit here, something like `due to a synchronous token fetch failure such as a connection-allowlist block`
base::test::ScopedFeatureList fedcm_feature_list_{features::kFedCm};this isn't really necessary, fedcm has been enabled by default for years
// FedCM API's fetch to "https://<configURL-host>/.well-known/web-identity"host -> site (or eTLD+1)
// `URLLoaderInterceptor` is required. `RegisterResponse` cannot be used. ThisYou could set this flag to avoid this behavior if that simplifies things:
https://source.chromium.org/chromium/chromium/src/+/main:content/common/features.cc;l=240;drc=3ca5cb59557399c66447326d1f9682d135db816d;bpv=1;bpt=1
// Since the complete FedCM flow is not mocked, the FedCM API failed with theRather, FedCM avoids exposing specific errors the website, so all failures lead to "Error retrieving a token"
R"((response-origin "*://b.test:*/.well-known/web-identity"))"}}));So, if I am interpreting this correctly, then only the well known file itself is allowlisted. This means that the /fedcm.json file is not allowlisted, and so this test should still fail with net::ERR_NETWORK_ACCESS_REVOKED?
// is allowed by the connection allowlist. The response directs FedCM to fetchThat's not how fedcm works. Instead, if the well known file contains a different URL for the config file, we fail with a mismatch error; we never attempt to fetch that different URL
// FedCM API's fetch to "https://<configURL-host>/.well-known/web-identity"The fetch is made to the root domain of configURL's site (b.test) and not to the hostname (sub.b.test), which matches what your test expects but not what the comment says.
// the config fedcm.json using another URL, which is also allowed.again that is not how FedCM works and your test does request the file using `sub.b.test` as the initial configURL
manager->FetchClientMetadata(GURL("https://b.test/client_metadata"),Add a comment like `We fetch the client metadata endpoint directly using the network request manager because client metadata errors are not fatal during the FedCM flow`
"client_id", 1, 1, base::DoNothing());why not use a similar TestFuture like in ClientMetadataBlocked?
manager->SendAccountsRequest(url::Origin::Create(GURL("https://b.test")),Why are you calling this directly instead of using the regular FedCM flow like in the well known tests? (Or conversely, why are you using the regular fedcm flow in those tests?)
manager->SendAccountsRequest(url::Origin::Create(GURL("https://b.test")),If you want to call this directly, why not use TestFuture to check that it actually works completely? Alternatively, why not use the regular fedcm flow? (Are you trying to avoid dealing with the dialog?)
manager->SendTokenRequest(same question re: fedcm flow
manager->SendTokenRequest(same question re: TestFuture/fedcm flow
manager->DownloadAndDecodeImage(GURL("https://b.test/image.png"),I'm OK with this one because doing something else would require having valid image data in the test which seems not worth it.
IN_PROC_BROWSER_TEST_F(ConnectionAllowlistFedCmTest, RedirectToBlocked) {this does not actually test redirect_to. it just tests the token request. was this written by AI?
IN_PROC_BROWSER_TEST_F(ConnectionAllowlistFedCmTest, RedirectToAllowed) {same here
manager->SendDisconnectRequest(GURL("https://b.test/disconnect"),you probably could switch the disconnect tests to using JS `IdentityProvider.disconnect`, although you'd have to set up an account connection first, so maybe not worth it. but add a comment about why you're not doing that?
manager->SendLogout(GURL("https://b.test/logout"), future.GetCallback());I didn't realize this function still exists... we should remove it, it has no callers.
FrameTreeNode* node = FrameTreeNode::GloballyFindByID(frame_tree_node_id_);I think I would prefer remembering the render_frame_host in the constructor
is there a reason you're not testing the accounts endpoint with a WPT test as well?
(btw, why test the same thing in browser tests if you already have WPT tests?)
Related details
- Xiaochen Zhou
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. |