Disallow empty strings

[selfissued]

Fixes #2068

Cc @zacknewman

---

Preview | Diff

[MasterKale]

I approved as-is, but I keep coming back to the idea that maybe we keep the existing preamble, but change the "SHOULD set to an empty string" guidance to something like "MUST omit the value" instead 🤔

[selfissued]

We could do that.

[emlun]

maybe we keep the existing preamble, but change the "SHOULD set to an empty string" guidance to something like "MUST omit the value" instead 🤔

That won't work with the dictionary IDL as currently defined, as displayName is a required, non-nullable member.

But maybe we could make it optional? I think the only blocker could be compatibility with CTAP2. But it actually looks like CTAP2 would already be compatible with that change. I noticed this in particular in §6.8.6. Updating user information:

Replace the matching credential’s PublicKeyCredentialUserEntity's name, displayName with the passed-in user details. If a field is not present in the passed user details, or it is present and empty, remove it from the matching credential’s PublicKeyCredentialUserEntity.

But one problem is that in CTAP2, PublicKeyCredentialUserEntity appears in both input (contravariant) and output (covariant) position. Making displayName optional is fine in input position, but could break CTAP2 clients if they read out a PublicKeyCredentialUserEntity structure for a discoverable credential and expect displayName to always be present. But on the other hand there's also this in the definition of the authenticatorGetAssertion response structure:

FIDO Devices - discoverable credentials: For discoverable credentials on FIDO devices, at least user "id" is mandatory.

So maybe clients are expected to expect that not all member will always be present.

[emlun]

Should we spell out the same requirement for name too?

[selfissued]

@emlun, would you be willing to create a PR making displayName and name optional in the IDL? I'll then update this PR to indicate that these fields MUST be omitted if they have no values. Thanks.

[agl]

If the value can't be empty, what should sites who don't have a value set it to?

Also, we can't make it optional without leaving a trap for sites which will then break with all clients that haven't been updated with that.

It's not clear that RFC8266 is worth the reference here. It's concerned with contexts where a nickname is presented to other users of the system. But, in WebAuthn, it's only presented to that same person.

[zacknewman]

If the value can't be empty, what should sites who don't have a value set it to?

Also, we can't make it optional without leaving a trap for sites which will then break with all clients that haven't been updated with that.

It's not clear that RFC8266 is worth the reference here. It's concerned with contexts where a nickname is presented to other users of the system. But, in WebAuthn, it's only presented to that same person.

To be clear, I never advocated for disallowing empty strings for displayName. I advocated for consistency in the spec recommendations where adherence to the stated recommendations doesn't cause issues. Merely amending the spec to explicitly state that Nickname enforcement SHOULD be done by both RPs and clients for non-empty strings is fine with me.

Something like below:

• Relying Parties SHOULD perform enforcement, as prescribed in Section 2.3 of [RFC8266] for the Nickname Profile of the PRECIS FreeformClass [RFC8264], when setting displayName's value to a non-empty string, or displaying the non-empty value to the user.

• This string MAY contain language and direction metadata. Relying Parties SHOULD consider providing this information. See § 6.4.2 Language and Direction Encoding about how this metadata is encoded.

• Clients SHOULD perform enforcement, as prescribed in Section 2.3 of [RFC8266] for the Nickname Profile of the PRECIS FreeformClass [RFC8264], on displayName's value prior to displaying the value to the user or including the value as a parameter of the authenticatorMakeCredential operation when the value is non-empty.

Completely removing any mention of RFC 8266 and Nickname Profile enforcement is also a good alternative (and is easier on the RP).

[emlun]

@emlun, would you be willing to create a PR making displayName and name optional in the IDL?

We can't trivially declare name as optional in the WebIDL because it's inherited from PublicKeyCredentialEntity and represents both rp.name and user.name. So we can't declare user.name as optional without doing the same with rp.name. Which perhaps we could, but we haven't decided to do that as of now.

Also, I think it's a bad idea to make both name and displayName optional. This was discussed at length in issue #1915.

Also, we can't make it optional without leaving a trap for sites which will then break with all clients that haven't been updated with that.

Oh right, this is also a very good point. In fact we had already come to this same conclusion in #2024.

Merely amending the spec to explicitly state that Nickname enforcement SHOULD be done by both RPs and clients for non-empty strings is fine with me.

This sounds like a reasonable compromise to me. The background for why the PRECIS enforcement was added is in #593 (comment) and I think most of that probably still applies, so I don't think we should drop it completely.

On the other hand there is #2068 (comment):

OpenID Connect also forbids empty strings as claim values in an attempt to preempt some interop problems. There, https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse says:

If a Claim is not returned, that Claim Name SHOULD be omitted from the JSON object representing the Claims; it SHOULD NOT be present with a null or empty string value.

We should do likewise.

But I think this probably overrules still:

If the value can't be empty, what should sites who don't have a value set it to?

If it can't be optional or nullable, then I guess empty string is the second best alternative?

[Firstyear]

If it can't be optional or nullable, then I guess empty string is the second best alternative?

We specifically enforce it can't be an empty string in webauthn-rs because it's potentially confusing vs a null/none type. And given how many providers offer OIDC, it seems far more prudent to say "must be a string excluding empty string".

Then you have to also consider how many resident key UI's would mishandle the empty string too in a device listing. So I think there are a fair number of potential ways for this to go sideways.

[sbweeden]

I don't like the idea that we define this parameter with "MUST NOT be the empty string". If we do that, its implied that the client should raise an error if the RP sets this value to the empty string, which would be a breaking change to RPs that might be doing that now. I think it would be better just to define what the client behaviour should be when the empty string is provided (which should be done if we think its an error anyway).

[MasterKale]

I'm leaning towards tweaking guidance on how to process displayName so that it can be empty, but when it's not empty it should be validated according to RFC 8266.

[nsatragno]

During the call, we decided: Let's state that the RFC only applies for non-empty values. This prevents us from breaking backwards compatibility, removes the contradiction, and keeps the value from specifying how the display name should be chosen.

The issue discussed during the call of compatibility issues due to empty display names is orthogonal, and likely an implementation issue. If we tried to fix it by making it optional, we'd risk the much worse compatibility risk of clients that haven't updated breaking. We'll figure out if there are any spec bugs leading to that offline with @MasterKale.

(this discussion was long, and I might have missed something worth summarizing, feel free to chime in!)

@selfissued said he'd be happy to update this PR to match.