Set limit on size of providers
#711
Conversation
We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313}
We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313}
We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313}
| @@ -794,6 +794,7 @@ the exception thrown. | |||
| {{IdentityCredentialRequestOptionsMode/passive}} on |globalObject|'s [=Window/navigable=]'s | |||
| [=navigable/top-level traversable=] or on any of its descendants, reject the pending | |||
| request with a "{{NetworkError}}" {{DOMException}}. | |||
| 1. If |providerList|'s [=list/size=] is greater than 10, return (failure, true). | |||
There was a problem hiding this comment.
This seems arbitrarily small. I understand some of the risks if it is too large, but it seems to me it should not be hard-coded at the spec level. Relevant admin users should be able to adjust this as appropriate to a given deployment.
There was a problem hiding this comment.
We can add enterprise settings in order to make this larger for admins that control browser deployments if needed. But I think it is more interoperable to have a concrete specific (and yes, arbitrary) value here so that the behavior is consistent across browsers and developers do not accidentally request a number of IdPs which works on some browsers but not on others.
There was a problem hiding this comment.
What happens when there are ten large IdPs and some number of small? Or is there an unstated goal to limit the overall number of IdPs?
There was a problem hiding this comment.
Not sure what the size of the IdP has to do with anything? We want to limit the number of IdPs that a single RP can request in one go. Each IdP in the request can potentially introduce 5+ fetches so it would be too slow to include too many IdPs.
There was a problem hiding this comment.
Imagine, limiting the size of the IdP pool to 5 providers. Who might they be? Probably GoogleIdP, XIdP, AppleIdP, MicrosoftIdP, maybe GitHubIdP. What about JoeSmithIdP? What about DisneyIdP?
There was a problem hiding this comment.
Federation is not only done through FedCM, so the question about today is about the current state of the world.
I'll reiterate that what you are imagining is not feasible without IDP registration, because the RP would need to embed one SDK per IDP, and we'd need to perform multiple fetches per IDP. So a get() with hundreds of IDPs is not feasible.
This limit is intended to only be limiting to people attempting to abuse the API. We can always change it or increase it if there are real-world requests where they would need a larger limit. Today, the largest I've seen is 5, so the limit I am proposing is double that.
There was a problem hiding this comment.
But IDP registration is something we want to do, and that use case is entirely boxed out by this approach to limiting only to 10.
There was a problem hiding this comment.
Why is that? Usually you just need 1 IDP in your get call to request any registered IDP. We can also tweak the limit to completely exclude registration cases if needed, when we spec that.
There was a problem hiding this comment.
Ah, I see, this is the provided idp config list, not the actual set that shows up in the UI. I was a little turned around. Then I don't have strong opinions against this, but it really isn't needed in Firefox because of the IDP chooser.
There was a problem hiding this comment.
Ok. We don't need to add this to the spec, as I think the spec already provides enough leeway for the user agent to reject a request for whatever reason. But I figured it might be helpful to have a shared limit to reduce possible compat issues.
…ed, a=testonly Automatic update from web-platform-tests [FedCM] Limit the number of IDPs requested We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313} -- wpt-commits: a5f4f35aa3381bcf01ceac355ff777608e9a5570 wpt-pr: 51725
…ed, a=testonly Automatic update from web-platform-tests [FedCM] Limit the number of IDPs requested We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313} -- wpt-commits: a5f4f35aa3381bcf01ceac355ff777608e9a5570 wpt-pr: 51725
…ed, a=testonly Automatic update from web-platform-tests [FedCM] Limit the number of IDPs requested We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313} -- wpt-commits: a5f4f35aa3381bcf01ceac355ff777608e9a5570 wpt-pr: 51725
…ed, a=testonly Automatic update from web-platform-tests [FedCM] Limit the number of IDPs requested We reject in the renderer when more than 10 IDPs are requested. Then we ReportBadMessageAndDeleteThis() in the browser if the same happens, since an uncompromised renderer should not send this request. Matches proposed spec change at w3c-fedid/FedCM#711 Fixed: 399872188 Change-Id: I6ab154bed8034bbb15db34d20f2e30b0ea80d9aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6413326 Reviewed-by: Christian Biesinger <[email protected]> Commit-Queue: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1440313} -- wpt-commits: a5f4f35aa3381bcf01ceac355ff777608e9a5570 wpt-pr: 51725
|
I think this issue's title should be changed from |
I think your suggestion is unclear. Let me set providers in quote, does that help |
|
"Set "Set limit on number of |
|
"Set limit on size of |
No. "Set limit on size of list of providers" or "Set limit on size of set of providers" is exactly what this PR does. |
|
Discussed during the FedID WG/CG meeting 22 April 2025 https://github.com/w3c-fedid/meetings/blob/main/2025/2025-04-22-FedCM-agenda.md |
1 similar comment
|
Discussed during the FedID WG/CG meeting 22 April 2025 https://github.com/w3c-fedid/meetings/blob/main/2025/2025-04-22-FedCM-agenda.md |
Fixes w3c-fedid/multi-idp#6
Preview | Diff