[FedCM]: Add Identity Handler Service Worker interception [chromium/src : main]

[suresh potti (Gerrit)]

suresh potti abandoned this change.

View Change

Abandoned

suresh potti abandoned this change

Related details

Attention set is empty

Submit Requirements:

• Code-Coverage
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: abandon

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I3424c7d6a52dc7a104f30e4cdacc4f3268b22eb1

Gerrit-Change-Number: 7974037

Gerrit-PatchSet: 1

Gerrit-Owner: suresh potti <sures...@microsoft.com>

Gerrit-Reviewer: suresh potti <sures...@microsoft.com>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>

Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

[Monica Chintala (Gerrit)]

Attention needed from suresh potti

Monica Chintala added 12 comments

File content/browser/webid/flags.h

Line 67, Patchset 10 (Latest):bool IsIdentityHandlerEnabled();

Monica Chintala . unresolved

is there a reason  kFedCmIdentityHandler  doesn't get a  chrome://flags  entry like its FedCM siblings ( kFedCmDelegation ,  kFedCmIdPRegistration , …)? An entry in  about_flags.cc  +  flag_descriptions.{h,cc}  would make local testing easier without requiring  --enable-features=.

Line 1, Patchset 10 (Latest):// Copyright 2021 The Chromium Authors

Monica Chintala . unresolved

Should we split the CL?
  
   1) mojom + Blink IDL surface + event class (RuntimeEnabled-gated)
   2) SW global scope dispatch hooks + WaitUntilObserver glue
   3) content/browser dispatcher + handler SW manager + unit tests + flags
   4) idp_network_request_manager integration and wpts

File content/browser/webid/identity_handler_service_worker_manager.h

Line 8, Patchset 10 (Latest):#include <optional>

Monica Chintala . unresolved

Nit: `OnRegistrationsForUnregisterFound` takes `const std::vector<scoped_refptr<ServiceWorkerRegistration>>&` but this header doesn't `#include <vector>`. Gets it transitively today. Per chromium IWYU, please add the direct include.

File content/browser/webid/identity_request_event_dispatcher.h

Line 65, Patchset 10 (Latest): base::TimeDelta timeout = base::Seconds(10));

Monica Chintala . unresolved

nit: prefer a file-scope  constexpr base::TimeDelta kIdentityRespondWithTimeout = base::Seconds(10);  over a literal in a default arg. Easier to grep/tune, and a clean seam if we ever want a Finch knob.

File content/browser/webid/identity_request_event_dispatcher.cc

Line 110, Patchset 10 (Latest): receiver_ref->Close();

Monica Chintala . unresolved

When the 10s timeout fires we close the response pipe and the destructor
 reports kRespondWithFailed, but the SW dispatch is not finished:

   - The renderer's respondWith() promise is author-controlled; closing the
     Mojo pipe does not cause it to settle.
   - Therefore IdentityRequestRespondWithObserver::OnResponseFulfilled /
     OnResponseRejected does not fire.
   - Therefore the CreateSimpleEventCallback(event_finish_id) ACK passed into
     DispatchIdentityRequestEvent never gets invoked.

 Net effect: the external-request slot created by
 StartRequestForFunctionalEvent stays open until ServiceWorkerVersion's
 built-in request timeout (~5 min) forcibly finishes it. A handler that
 does event.respondWith(new Promise(()=>{})) keeps the worker warm for
 ~5 min after we already reported failure to the FedCM caller, blocking
 soft update for the IDP SW and racing with unregister(). A repeating
 attacker can keep the worker warm indefinitely (one stuck respondWith per
 10s).

 Suggested fix: capture  event_finish_id  and reuse the existing  scoped_refptr<ServiceWorkerVersion>  already bound into OnTimeout , and call
   version->FinishRequest(event_finish_id, /*was_handled=*/false);
 before receiver_ref->Close(). That releases the slot promptly and aligns
 the SW-side lifetime with the 10s budget the user sees.

 Please also add a unit test along the lines of: "respondWith never
 settles -> 10s later, the version's external-request count is back to
 zero (or HasWork() returns false)".

File content/browser/webid/idp_network_request_manager.cc

Line 2005, Patchset 10 (Latest): result.error = IdentityCredentialTokenError{/*code=*/"", /*url=*/GURL()};

Monica Chintala . unresolved

OnSWTokenResponse synthesizes an empty IdentityCredentialTokenError{"", GURL()} for the kRespondWithFailed branch. Since parse_status already signals failure, this can either be dropped or filled with a meaningful code like "identity_handler_respond_with_failed".

File third_party/blink/public/mojom/webid/fedcm_identity_request.mojom

Line 37, Patchset 10 (Latest): OnError(string error_message);

Monica Chintala . unresolved

 error_message  is dead end-to-end:

 The renderer always sends the same literal string in   service_worker_global_scope.cc::RespondToIdentityRequestEvent :

 result_callback->OnError("Identity request event handler failed");

 even though  IdentityRequestRespondWithObserver  already distinguishes three failure paths (rejected, invalid MIME,  body-load failed). Today, only two of the three (rejected and invalid-MIME) emit a distinct console message; the  body-load-failed path ( DidFetchDataLoadFailed ) is silent both renderer-side and — via the dead  error_message  —  browser-side, so the SW author has no signal at all in that case.

 The browser-side  IdentityRequestResponseCallbackImpl::OnError  in  identity_request_event_dispatcher.cc  ignores  the argument:

 void OnError(const std::string& error_message) override {
   ...
   RunCallbackWithFailure(callback_, kRespondWithFailed);  // arg unused
 }

 Two options, pick one before this lands:

 (a) Drop the parameter:  OnError(); . Simpler interface, less confusion for future readers.

 (b) Actually plumb the specific reason — pass it through to the dispatch callback, surface it in a browser-side  console message (in addition to the renderer-side message the SW author already sees, so the RP-side developer also  sees why), and/or use it as a label on the  Blink.FedCm.IdentityHandler.FellBackToNetwork  (or a sibling "Failed")  histogram so we can tell rejected / invalid-mime / body-failed apart in the field. This also closes the  body-load-failed gap above.

 Currently this is API surface area with no behavior behind it

File third_party/blink/renderer/modules/credentialmanagement/identity_request_event.cc

Line 9, Patchset 10 (Latest):#include "third_party/blink/renderer/core/execution_context/execution_context.h"

Monica Chintala . unresolved

execution_context.h is included but ExecutionContext is never referenced.

File third_party/blink/renderer/modules/credentialmanagement/identity_request_event_test.cc

Line 1, Patchset 10 (Latest):// Copyright 2026 The Chromium Authors

Monica Chintala . unresolved

WPTs needs more coverage:

 •  idl.https.html  (idlharness) for the  IdentityRequestEvent  IDL surface.
 •  register-rejects-on-invalid-well-known  (negative path for the registration flow when the well-known file is malformed/missing).
 •  respondWith()  second-call rejection (per spec, calling twice on the same event must throw  InvalidStateError )

File third_party/blink/renderer/modules/credentialmanagement/identity_request_respond_with_observer.h

Line 9, Patchset 10 (Latest):#include "third_party/blink/public/mojom/webid/fedcm_identity_request.mojom-blink.h"

Monica Chintala . unresolved

nit: this include appears unused in the header — the only mojom symbol referenced ( ServiceWorkerResponseError ) comes from the line above. Can it be dropped or moved to the .cc?

File third_party/blink/renderer/modules/credentialmanagement/identity_request_respond_with_observer.cc

Line 146, Patchset 10 (Latest): buffer->StartLoading(FetchDataLoader::CreateLoaderAsString(

Monica Chintala . unresolved

The Response body is read in full as a UTF-8 string with no upper bound. The existing FedCM network-fetch path caps payloads at  maxResponseSizeInKiB * 1024  = 1 MiB ( idp_network_request_manager.cc:175 ). A buggy or malicious IDP SW can  respondWith(new Response("x".repeat(2**30)))  and allocate a multi-GiB Blink string. Please thread the same cap through  IdentityResponseBodyLoader  — ideally by tracking running bytes through the BytesConsumer  so the cap is enforced during the read (not just at  DidFetchDataLoadedString , which fires after the allocation has already happened).

File third_party/blink/renderer/platform/runtime_enabled_features.json5

Line 2751, Patchset 10 (Latest): public: true,

Monica Chintala . unresolved

missing status: "test" on the runtime feature?

Open in Gerrit

Related details

Attention is currently required from:

• suresh potti

Submit Requirements:

• 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.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I12b1e8655360f38c79cc270994a46754c931ebe7

Gerrit-Change-Number: 7961746

Gerrit-PatchSet: 10

Gerrit-Owner: suresh potti <sures...@microsoft.com>

Gerrit-Reviewer: suresh potti <sures...@microsoft.com>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>

Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Monica Chintala <moni...@microsoft.com>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: suresh potti <sures...@microsoft.com>

Gerrit-Comment-Date: Tue, 30 Jun 2026 06:43:38 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[suresh potti (Gerrit)]

Attention needed from Monica Chintala

suresh potti added 1 comment

File content/browser/webid/flags.h

Line 1, Patchset 10 (Latest):// Copyright 2021 The Chromium Authors

Monica Chintala . resolved

Should we split the CL?
  
   1) mojom + Blink IDL surface + event class (RuntimeEnabled-gated)
   2) SW global scope dispatch hooks + WaitUntilObserver glue
   3) content/browser dispatcher + handler SW manager + unit tests + flags
   4) idp_network_request_manager integration and wpts

suresh potti

Have plans to split the CL. The reason for sending a larger CL first is to help reviewers understand the flow. Once they’ve gone through it and there are no issues, I’ll send smaller CLs for review and code merge.

Open in Gerrit

Related details

Attention is currently required from:

• Monica Chintala

Submit Requirements:

• 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.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I12b1e8655360f38c79cc270994a46754c931ebe7

Gerrit-Change-Number: 7961746

Gerrit-PatchSet: 10

Gerrit-Owner: suresh potti <sures...@microsoft.com>

Gerrit-Reviewer: suresh potti <sures...@microsoft.com>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>

Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Monica Chintala <moni...@microsoft.com>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Monica Chintala <moni...@microsoft.com>

Gerrit-Comment-Date: Wed, 01 Jul 2026 05:29:08 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Monica Chintala <moni...@microsoft.com>

[suresh potti (Gerrit)]

Attention needed from Monica Chintala

suresh potti added 1 comment

File content/browser/webid/flags.h

Line 67, Patchset 10 (Latest):bool IsIdentityHandlerEnabled();

Monica Chintala . unresolved

is there a reason  kFedCmIdentityHandler  doesn't get a  chrome://flags  entry like its FedCM siblings ( kFedCmDelegation ,  kFedCmIdPRegistration , …)? An entry in  about_flags.cc  +  flag_descriptions.{h,cc}  would make local testing easier without requiring  --enable-features=.

suresh potti

Thanks. Added the changes.

Gerrit-Comment-Date: Wed, 01 Jul 2026 11:11:31 +0000

[suresh potti (Gerrit)]

Attention needed from Monica Chintala

suresh potti added 1 comment

File content/browser/webid/identity_handler_service_worker_manager.h

Line 8, Patchset 10 (Latest):#include <optional>

Monica Chintala . unresolved

Nit: `OnRegistrationsForUnregisterFound` takes `const std::vector<scoped_refptr<ServiceWorkerRegistration>>&` but this header doesn't `#include <vector>`. Gets it transitively today. Per chromium IWYU, please add the direct include.

suresh potti

Added. Thanks.

Gerrit-Comment-Date: Wed, 01 Jul 2026 11:18:14 +0000

[suresh potti (Gerrit)]

Attention needed from Monica Chintala

suresh potti added 1 comment

File content/browser/webid/identity_request_event_dispatcher.h

Line 65, Patchset 10 (Latest): base::TimeDelta timeout = base::Seconds(10));

Monica Chintala . unresolved

nit: prefer a file-scope  constexpr base::TimeDelta kIdentityRespondWithTimeout = base::Seconds(10);  over a literal in a default arg. Easier to grep/tune, and a clean seam if we ever want a Finch knob.

suresh potti

Added. Thanks.

Gerrit-Comment-Date: Wed, 01 Jul 2026 11:23:11 +0000

[suresh potti (Gerrit)]

Attention needed from Monica Chintala

suresh potti added 1 comment

File third_party/blink/renderer/modules/credentialmanagement/identity_request_event.cc

Line 9, Patchset 10 (Latest):#include "third_party/blink/renderer/core/execution_context/execution_context.h"

Monica Chintala . unresolved

execution_context.h is included but ExecutionContext is never referenced.

suresh potti

Removed.

Gerrit-Comment-Date: Wed, 01 Jul 2026 11:28:04 +0000