CredentialRequestOptions make otherwise valid values invalid in an undesirable way

[bzbarsky]

23-Jul-2018 NOTE: see STATUS SUMMARY here: #750 (comment)

---

https://w3c.github.io/webauthn/#credentialrequestoptions-extension has:

partial dictionary CredentialRequestOptions {
    PublicKeyCredentialRequestOptions      publicKey;
};

and PublicKeyCredentialRequestOptions has:

required BufferSource                challenge;

What this means in practice is that if this part of the spec is implemented and whatwg/webidl#76 is fixed then {} stops being a valid value for a CredentialRequestOptions.

Assuming that CredentialsContainer.prototype.get should be usable to request non-public-key credentials, and that whatwg/webidl#76 will be fixed at some point, this is not ideal.

[equalsJeffH]

we discussed this on 17-Jan-2018 webauthn call. This requires some thought and reading-up on whatwg/webidl#76.

During the discussion, we noted that we could nominally remove the "required" stipulation for PublicKeyCredentialRequestOptions.challenge, and note in spec prose that if one does not pass in a challenge when calling navigator.credentials.{create(),get()}, that the latter calls will not succeed (in some fashion...throw an error? return a null credential/assertion?).

Though we noted that such an approach is not ideal and we need to research and discuss this issue further.

Note also I believe @jcjones noted that the resolution to whatwg/webidl#76 is already implemented in Mozilla's Gecko.

[bzbarsky]

More precisely, "a resolution". Whether that's what ends up happening in the webidl spec is unclear at this point.

[equalsJeffH]

Ok, @bzbarsky, I've been RFTM'g here and have some questions...

Gecko implements this proposed resolution to whatwg/webidl#76:

1. A dictionary is "elidable" (better-naming-wanted) if it is either empty or only has members whose values are themselves elidable dictionaries.
2. When converting a dictionary to a JS value, skip over members whose values are elidable dictionaries, just like we skip over members that are not present.

For dictionaries that have default values that will still cause them to appear (with those default values)

..yes?

Assuming that what gecko implements (above?) is what is decided upon as the solution to whatwg/webidl#76, AND if I understand the latter issue (and the related moz bugs) correctly, then I note that PublicKeyCredentialRequestOptions et al has more required members than just challenge:

dictionary PublicKeyCredentialCreationOptions {
    required PublicKeyCredentialRpEntity         rp;
    required PublicKeyCredentialUserEntity       user;

    required BufferSource                             challenge;
    required sequence<PublicKeyCredentialParameters>  pubKeyCredParams;

    unsigned long                                timeout;
    sequence<PublicKeyCredentialDescriptor>      excludeCredentials = [];
    AuthenticatorSelectionCriteria               authenticatorSelection;
    AttestationConveyancePreference              attestation = "none";
    AuthenticationExtensionsClientInputs         extensions;
};

dictionary PublicKeyCredentialEntity {
    required DOMString    name;
    USVString             icon;
};

dictionary PublicKeyCredentialRpEntity : PublicKeyCredentialEntity {
    DOMString      id;
};

dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    required BufferSource   id;
    required DOMString      displayName;
};

dictionary PublicKeyCredentialParameters {
    required PublicKeyCredentialType      type;
    required COSEAlgorithmIdentifier      alg;
};

..then it sounds like there are two more-or-less plausible approaches for the webauthn spec (again, IIUC):

1. Remove the WebIDL "required" stipulation for all required fooOptions dictionary members (see above), and note in spec prose that if one does not pass all of them in with values when calling navigator.credentials.{create(),get()}, that the latter calls will not succeed (in some fashion: throw an error? return a null credential/assertion?).

2. Provide default values for all required fooOptions dictionary members, for example:

dictionary PublicKeyCredentialCreationOptions {
    required PublicKeyCredentialRpEntity         rp  = {};
    required PublicKeyCredentialUserEntity       user = {}; 
    required BufferSource                        challenge = null;
   [...]
};

dictionary PublicKeyCredentialEntity {
    required DOMString    name  =  "";
    USVString             icon;
};

dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    required BufferSource   id = null;
    required DOMString      displayName = "";
};

..(I'm not sure if the above syntactically correct). This would cause those dictionaries with default values to "appear" (with those default values), e.g. if someone does..

CredentialRequestOptions     bar = {};

..yes?

I'm not sure whether this covers all the bases. Nor, if it does, which is the "better" option.

thoughts?

[bzbarsky]

Gecko implements this proposed resolution to whatwg/webidl#76:

Gecko doesn't implement the elidable stuff yet.

and note in spec prose that if one does not pass all of them in with values when calling navigator.credentials.{create(),get()}, that the latter calls will not succeed

Right, this is option 1.

Provide default values for all required fooOptions dictionary members

Huh. IDL allows a default value for required members in the grammar... I filed whatwg/webidl#569 to fix that.

But even without that, the prose in https://heycam.github.io/webidl/#required-dictionary-member explicitly says:

A required dictionary member must not have a default value.

because there are no sane semantics for a required thing with a default value... That said, you could do non-required members with default values. That would be option 2.

Option 3 is to argue that whatwg/webidl#76 should be wontfixed and that dictionary members of dictionaries should not be treated the same way as dictionaries as arguments. Basically comes down to what a dictionary "is", conceptually...

[emlun]

Why should the default value for non-required members be the empty dictionary rather than undefined or null? It seems to me like if something is optional with no default value, then the default should be that it isn't there at all.

It seems to me like that behaviour would break all sorts of things. To take an example from the WebAuthn spec, CollectedClientData has this definition:

dictionary CollectedClientData {
    required DOMString           type;
    required DOMString           challenge;
    required DOMString           origin;
    TokenBinding                 tokenBinding;
};

dictionary TokenBinding {
    required TokenBindingStatus status;
    DOMString id;
};

Now, let's assume that whatwg/webidl#76 is implemented and say I create a CollectedClientData instance and do not set thetokenBinding member. Since CollectedClientData is a dictionary and the tokenBinding member is not required, this would mean that the tokenBinding member in the result will be set to the empty dictionary. But since that dictionary is of type TokenBinding which has a required member, the empty dictionary is an invalid value.

This is probably just another example of the very problem pointed out in the OP, but this example is much easier for me to think about. I imagine this kind of construction (an optional value with a required subcomponent if it's present) is very common, and it seems to me like whatwg/webidl#76 would invalidate all such constructions. So unless I'm mistaken somewhere in my analysis (please point it out if I am), I think whatwg/webidl#76 should be wontfixed.

[equalsJeffH]

@bzbarsky replied:

Gecko implements this proposed resolution to whatwg/webidl#76:

Gecko doesn't implement the elidable stuff yet.

Hm, I was interpreting your #750 (comment) in combination with whatwg/webidl#76 (comment) and @jcjones's comment on the webauthn call, to mean that Gecko does implement the "elidable stuff".

You implied gecko implements "a resolution" and are now indicating that the aforementioned resolution is different than the "elidable stuff" ?

and note in spec prose that if one does not pass all of them in
with values when calling navigator.credentials.{create(),get()},
that the latter calls will not succeed

Right, this is option 1.

ok.

... the prose in
https://heycam.github.io/webidl/#required-dictionary-member
explicitly says:

A required dictionary member must not have a default value.

doh.

you could do non-required members with default values. That would be option 2.

ah! so we can do this:

dictionary PublicKeyCredentialCreationOptions {
    PublicKeyCredentialRpEntity         rp  = {};
    PublicKeyCredentialUserEntity       user = {}; 
    BufferSource                        challenge = null;
    [...]
};

dictionary PublicKeyCredentialEntity {
    DOMString     name  =  "";
    USVString     icon;
};

dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    BufferSource   id = null;
    DOMString      displayName = "";
};

...? (I simply removed all the required keywords). Tho, perhaps we cannot assign {} to PublicKeyCredentialCreationOptions.rp and .user ?

Option 3 is to argue that whatwg/webidl#76
should be wontfixed and that dictionary members of dictionaries
should not be treated the same way as dictionaries as arguments.
Basically comes down to what a dictionary "is", conceptually...

yes, this seems to be an interesting question to discuss over there. @emlun followed up his above comment here with whatwg/webidl#76 (comment)

[bzbarsky]

I imagine this kind of construction (an optional value with a required subcomponent if it's present) is very common

So far we have two specs trying to do it: webauthn and some proposals for webrtc. So no, it's not very common. Maybe because required is somewhat new?

You implied gecko implements "a resolution" and are now indicating that the aforementioned resolution is different than the "elidable stuff" ?

Gecko implements a specific behavior which is one of the proposals under consideration in whatwg/webidl#76: optional dictionaries always default to null and no other changes from the current WebIDL spec. Adding the elidable thing is an additional proposal which would affect various dictionary-to-JS conversions, possibly including already-shipping ones. I haven't checked to see whether there are currently dictionaries returned to JS that include nested dictionaries that would be affected.

ah! so we can do this:

Sort of. You can't use {} as a default value; see grammar at https://heycam.github.io/webidl/#prod-DefaultValue. But you can use null, which will have the same effect.

[emlun]

So far we have two specs trying to do it: webauthn and some proposals for webrtc. So no, it's not very common. Maybe because required is somewhat new?

Ah, ok. Though I wasn't thinking only about specs, but also API design in a broader sense. For example, I've worked in a project where we used JSON schema to declare an API where these kinds of required members nested in optional members were quite frequent. It seems to me like a very natural thing to do with a language as expressive as JS literals are - and if people do it in APIs I think they'll expect it to work the same in specs. So if this is a new feature I imagine that this usage will only grow more common over time.

[equalsJeffH]

Gecko implements a specific behavior which is one of the proposals under consideration in whatwg/webidl#76: optional dictionaries always default to null ...

"optional dictionaries" in the dictionaries-as-optional-function-args case?

You can't use {} as a default value; see grammar at https://heycam.github.io/webidl/#prod-DefaultValue. But you can use null, which will have the same effect.

Ok, so are you saying that we can address this issue by updating our spec in this fashion:

dictionary PublicKeyCredentialCreationOptions {
    PublicKeyCredentialRpEntity         rp  = null;
    PublicKeyCredentialUserEntity       user = null; 
    BufferSource                        challenge = null;
    [...]
};

dictionary PublicKeyCredentialEntity {
    DOMString     name  =  "";
    USVString     icon;
};

dictionary PublicKeyCredentialUserEntity : PublicKeyCredentialEntity {
    BufferSource   id = null;
    DOMString      displayName = "";
};

...? i.e., remove required and supply default values as above?

thx.

[bzbarsky]

"optional dictionaries" in the dictionaries-as-optional-function-args case?

In all cases when the value is normally optional. That means optional function args and dictionary members.

Ok, so are you saying that we can address this issue by updating our spec in this fashion:

You can, yes.

I really think it's worth seeing exactly what happens in whatwg/webidl#76 before making changes here, though.

[equalsJeffH]

Thx for clarifications @bzbarsky.

I really think it's worth seeing exactly what happens in whatwg/webidl#76 before making changes here, though.

That sounds prudent. So we can punt on this issue for now ? We are trying to attain Proposed Rec soon and Rec before calendar-year-end.

[bzbarsky]

Does the spec actually describe what should happen when the .publicKey dictionary is missing? As far as I can tell https://w3c.github.io/webauthn/#createCredential asserts it's present in the CredentialCreationOptions, but it's not clear to me what guarantees that, exactly... It's also not clear to me when the "publicKey" member of CredentialRequestOptions ever gets used.

As long as the spec completely describes behavior when the dictionaries involved are missing, it should be fine as things stand.

[bzbarsky]

(Oh, and as long as the behavior is reasonable; there are some issues with webappsec-credential-management doing stuff in parallel sections that might involve buffer sources given the dictionaries this spec defines.)

[emlun]

As far as I understand, the presence of the publicKey member is what triggers the client to invoke the WebAuthn methods instead of other Credential Management backends.

[bzbarsky]

@emlun Where is that specified? That's the part I was trying to find and failing.

[emlun]

Looks like it's defined in CredMan, loosely here and in more detail here.

[bzbarsky]

Right, but where in the processing model is that determination of what to call actually made?

[emlun]

I think it happens here, where interfaces refers to here where options is the options argument object sent to the create() method and the [[type]] slot for the PublicKeyCredential interface contains "public-key".

...actually, does that mean that the publicKey member name in CredentialCreationRequestOptions should also be "public-key", or vice-versa? @equalsJeffH @selfissued Who's our expert on this?

[equalsJeffH]

I think it happens here, where interfaces refers to here where options is the options argument object sent to the create() method and the [[type]] slot for the [PublicKeyCredential][pkc] interface contains "public-key".

IIUC, the above is correct.

...actually, does that mean that the publicKey member name in CredentialCreationRequestOptions should also be "public-key", or vice-versa?

IIUC, I do not think that the difference between the [[type]] slot's value of public-key and the CredentialCreationOptions and CredentialRequestOptions dictionary extensions' member name of publicKey causes any issues.

cc: @mikewest

[bzbarsky]

@emlun Thank you! That does look like the right thing now, and it's doing the "is missing" check I was kinda looking for.

But I think you're right about the name mismatch being a problem. @equalsJeffH how could it not cause issues? The algorithm checks whether the dictionary has a "public-key" member, which it never does if the dictionary member is called "publicKey".

[equalsJeffH]

thanks @bzbarsky, as I said, IIUC, which also means I might not understand correctly, which seems to be the case here (sigh, sorry)...

Looking more closely at credman's magic relevant credential interface objects sub-algorithm, i'm now thinking you & @emlun are correct, due to steps 4.2 & 4.3 in said sub-alg:

4. For each object in interface objects:
    1. [ . . . ]
    2. Let key be object’s [[type]] slot’s value.   // "public-key"
    3. If options[key] exists, append object to 
        relevant interface objects.                 // options[public-key] throws (?)
                                                    // i.e., it ought to be 
                                                    // options[publicKey] given  
                                                    // how spec is currently written.

So, perhaps browser implementors can weigh-in on whether it matters if we change..

1. the CredentialCreationOptions and CredentialRequestOptions dictionary extensions' member name of publicKey to be public-key, or
2. the PublicKeyCredentialType's enumeration value to be "publicKey" (rather than the present "public-key").

..i.e., what are present implementations doing, would making this spec fix affect them at all?

cc @kpaulh @agl @akshayku @jcjones @ttaubert

[emlun]

For what it's worth, (1) would require changes to RP code as well in addition to clients, while (2) would not. On the other hand, (2) looks like more idiomatic JS since a bare public-key is not a valid object key literal in JS (the string literal "public-key" is a valid key, though).

[equalsJeffH]

hm. Plus there's the orange note in WebIDL S2.8. Enumerations that sez in part:

It is strongly suggested that enumeration values be all lowercase, and that multiple words be separated using dashes or not be separated at all, unless there is a specific reason to use another value naming scheme. For example, an enumeration value that indicates an object should be created could be named "createobject" or "create-object".

[bzbarsky]

Right, enumerations are meant to be values, not property names...

[equalsJeffH]

Ok, thx, so how do we fix this such that we can have our cake and eat it too, i.e., have credman's magic relevant credential interface objects sub-algorithm work for us? Or, is this a credman flaw?

Apparently, it's primarily a spec-level issue because there's running impls of this stuff...

[bzbarsky]

It's probably best to spin off the enumerations bit into a separate issue, since it's not really related to this issue.

And presumably impls are just doing something different from what the spec says and it's worth asking them what they do.

[equalsJeffH]

STATUS SUMMARY

The above interstitial thread beginning with this comment by @bzbarsky -- #750 (comment), and continuing to the above comment #750 (comment), inclusive -- is now forked over to issue #1004.

THIS ISSUE #750 has a proposed solution in this above comment: #750 (comment), to which @bzbarsky suggests (in #750 (comment)) that we wait until whatwg/webidl#76 is resolved before applying any solution to this original issue.

[bzbarsky]

I think you should consider whatwg/webidl#76 resolved (as "no change to the webidl spec") for purposes of this discussion. The only question left open there does not affect this spec.

[equalsJeffH]

In discussion on webauthn call today 3-Oct-2018, it seems that we ought to add to the beginning of #createCredential and #getAssertion algs a step that checks for "required" options dictionary members' presence and bail out if any are not present.

Also need to make determination whether we need to convert present "required" dict members to having default null values, or no default values (but with comments indicating "requiredness") (?)

[agl]

In Chromium, we have set the members of CredentialCreationOptions as nullable (source). Thus if a given member isn't present, it's null, and then everything works out.

That contrasts with what's in Webauthn. Perhaps Webauthn should allow these fields to be nullable so that the required members within each subtype's options aren't a problem?

[bzbarsky]

In Chromium, we have set the members of CredentialCreationOptions as nullable (source)

That's invalid IDL that only works due to a bug in Chrome's IDL parser. See https://bugs.chromium.org/p/chromium/issues/detail?id=890375.

I don't understand what's wrong with my proposal from #750 (comment) exactly. Webauthn's IDL is fine at this point. It just needs to clearly define what happens when those members are not present. It's not clear to me that this is clearly specified right now.

[equalsJeffH]

@bzbarsky -- just to clarify:

Webauthn's IDL is fine at this point. It just needs to clearly define what happens when those members are not present.

So are you saying that we can leave WebAuthn's IDL written as it presently is, with various dictionary members declared with the required keyword?

And also we ought to add to the beginning of each of #createCredential and #getAssertion algs a step that checks for "required" options dictionary members' presence and bail out (with some appropriate error) if any are not present ?

[bzbarsky]

So are you saying that we can leave WebAuthn's IDL written as it presently is, with various dictionary members declared with the required keyword?

Yes.

And also we ought to add to the beginning of each of #createCredential and #getAssertion algs a step that checks for "required" options dictionary members' presence

Not quite, if I understand correctly how this works. So to take a concrete example, let's look at CredentialCreationOptions. The spec needs to define what happens when the publicKey member is not present. If it is present, then Web IDL will guarantee that its required members (e.g. rp) are also present.

I think the idea is to not call the relevant #createCredential at all if the corresponding member is not present in CredentialCreationOptions, right? As long as that's clearly defined, you should be all good.

This will give a behavior where passing {} or { publicKey: undefined } for the CredentialCreationOptions will not invoke the relevant #createCredential, while passing { publicKey: null } will throw. I don't know whether that matches what Chrome does right now.

[emlun]

The spec needs to define what happens when the publicKey member is not present. [...] I think the idea is to not call the relevant #createCredential at all if the corresponding member is not present in CredentialCreationOptions

Yes, I think that behaviour is implied by the CredMan spec (see #750 (comment) and the subsequent few comments), so I don't think we really need to do much more here. As far as I can tell, the one thing we do need to figure out in order to integrate correctly with CredMan on this is to align the "public-key" type value and the publicKey member name.

[equalsJeffH]

@bzbarsky wrote:

I think the idea is to not call the relevant #createCredential at all if the corresponding member is not present in CredentialCreationOptions, right?

Correct, as @emlun notes above.

@emlun wrote:

I don't think we really need to do much more here.

Agreed, given @bzbarsky's comment above.

As far as I can tell, the one thing we do need to figure out in order to integrate correctly with CredMan on this is to align the "public-key" type value and the publicKey member name.

Agreed. As I noted above, that issue is now forked over to issue #1004.

So, are we good with closing this issue #750?

[bzbarsky]

I am, yes.

[equalsJeffH]

ok, closing per discussion on 14-nov-2018 call