Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate rp.name #2121

Closed
nsatragno opened this issue Aug 14, 2024 · 6 comments · Fixed by #2159
Closed

Deprecate rp.name #2121

nsatragno opened this issue Aug 14, 2024 · 6 comments · Fixed by #2159

Comments

@nsatragno
Copy link
Member

nsatragno commented Aug 14, 2024

Proposed Change

Remove, deprecate, or ignore PublicKeyCredentialRPEntity.name. It's not used by any credential provider that I know about.

This might be tricky because it's inherited from PublicKeyCredentialEntity.name.

@nsatragno nsatragno self-assigned this Aug 14, 2024
@emlun
Copy link
Member

emlun commented Aug 14, 2024

Summary of the discussion on the 2024-08-14 WG call:

  • Add Signal API #2093 (comment) currently doesn't have a field for updating rp.name, and it seems inconsistent to allow updating some otherUI fields but not others.
  • No known client implementation actually uses rp.name in any meaningful way.
  • rp.name is arguably the "most phishable part" of WebAuthn, as unlike rp.id it's not subject to any security checks.
  • For these reasons, we don't really want to include an update signal for rp.name.
  • Another way to resolve the inconsistency is to deprecate/remove rp.name instead.

@emlun
Copy link
Member

emlun commented Aug 14, 2024

As also mentioned in the discussion, we probably can't actually remove rp.name, or make it optional, since it's required in L1 and L2. Both of those changes would create a trap where RPs don't include rp.name, since it's not required, but when that code happens to run in an L1 or L2 client it would raise a TypeError since the attribute is required in that version.

Instead, there was consensus on the call in favour of simply changing the definition and description of rp.name to "unused and meaningless, but still required for backwards compatibility, so just set it to empty string".

@zacknewman
Copy link
Contributor

zacknewman commented Aug 14, 2024

Instead, there was consensus on the call in favour of simply changing the definition and description of rp.name to "unused and meaningless, but still required for backwards compatibility, so just set it to empty string".

Setting it to an empty string will not be backwards compatible for clients that enforce the Nickname profile as recommended by the L2 spec since empty strings are not valid Nicknames; however enforcing Nicknames is “only” a recommendation and is likely not enforced by many clients.

@nadalin nadalin added this to the L3-WD-02 milestone Sep 11, 2024
@emlun emlun self-assigned this Sep 11, 2024
@nicksteele
Copy link
Contributor

General sentiment from the 2024-9-11 call seemed to be in favor of deprecation

@emlun
Copy link
Member

emlun commented Sep 11, 2024

2024-09-11 WG call: Hearing consensus in favour of deprecating rp.name. @emlun to write a PR.

@zacknewman has a fair point that empty string may not be an entirely safe fallback value. We don't know of any clients that actually enforce the nickname profile, but still.

@timcappalli
Copy link
Member

2024-09-18 call: instead of deprecating or making optional, add some text stating many clients will not display the value and to pass either the RP ID again, or an empty string. Also update passkeys.dev guidance.

@emlun emlun changed the title Remove rp.name Deprecate rp.name Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants