PRF inputs should be BufferSource instead of ArrayBuffer about webauthn

[agl]

PRF inputs should be BufferSource instead of ArrayBuffer

Absolutely. Thank you. (I'll fix in Chromium too.)

[Firstyear]

@emlun Is this similar to the issues previously mentioned about challenge and other types where the JS will need to base64 encode/decode to actually get this into a buffer though? Should we just try to nip this one early and make it base64urlsafe instead and get the browser to decode it?

[emlun]

No, I don't think it's very similar. Both of these types are already byte array types, just slightly different flavours of it.

And no, I think that having some parameters be BufferSources and some be base64 Strings would be much worse than having them all be BufferSources. If we should change some parameters we should change them all. But as noted in #1362 (comment), I don't think we should. In #1683 we opted for the solution in #1703 instead, so there's very little benefit in changing the types now.

[Firstyear]

If we are going to aim for consistency here, then should we also add this to the to/from base64 for the related browser methods then?

[emlun]

Yes, it's already covered in §5.1.8. Deserialize Registration ceremony options - PublicKeyCredential’s parseCreationOptionsFromJSON() Method:

[...] This conversion MUST also apply to any client extension inputs processed by the client.

[emlun]

Thanks @agl, but 5ebc257 doesn't quite resolve the issue - the inputs should be BufferSource, but the outputs should still be ArrayBuffer. 🙂 At least if we're going to be consistent with the rest of the spec.

[agl]

I don't think it's worth splitting the IDL just for the output. That causes additional code size in browsers too because an extra object needs to be supported.

BufferSource isn't a real type, it's just means either ArrayBuffer or ArrayBufferView. For inputs that has real benefits as you note, but on the output it doesn't really make any difference because the underlying object will still be an ArrayBuffer. E.g.:

> temp1.getClientExtensionResults().prf.results.first instanceof ArrayBuffer
> true

[emlun]

Hm, I see:

typedef (ArrayBufferView or ArrayBuffer) BufferSource;
typedef (Int8Array or Int16Array or Int32Array or
         Uint8Array or Uint16Array or Uint32Array or Uint8ClampedArray or
         BigInt64Array or BigUint64Array or
         Float32Array or Float64Array or DataView) ArrayBufferView;

But...

the underlying object will still be an ArrayBuffer. E.g.:

> temp1.getClientExtensionResults().prf.results.first instanceof ArrayBuffer
> true

I don't get the same results in Chrome (110.0.5481.77) or Firefox (109.0.1):

> new Uint8Array().buffer instanceof ArrayBuffer
true

> new Uint8Array() instanceof ArrayBuffer
false

and indeed, I can't find ArrayBuffer in the prototype hierarchy of Uint8Array either.

But on the other hand... in order to work with an ArrayBuffer (i.e., to base64-encode it) you'll most likely convert it to Uint8Array first, and the Uint8Array constructor accepts both ArrayBuffer as well as any TypedArray. You can just new Uint8Array(prf.results.first) and that will always work. So in that sense BufferSource is "Liskov substitution compatible" with ArrayBuffer, even though it's not formally a subtype.

So ok, yeah, I guess we could go either way. I do agree with not splitting the IDL just for this. It seems a bit inappropriate to use a union type for output, but I agree that BufferSource output seems less likely to trip people up than ArrayBuffer inputs.

So ok, this will be fixed by 5ebc257. Thanks!

[emlun]

Will be fixed by in commit 5ebc257 in PR #1836.