[webkit-reviews] review denied: [Bug 238966] [WebAuthn] Implement getTransports() and getAuthenticatorData() on AuthenticatorAttestationResponse : [Attachment 457282] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied j_pascoe at apple.com
<j_pascoe at apple.com>'s request for review:
Bug 238966: [WebAuthn] Implement getTransports() and getAuthenticatorData() on
AuthenticatorAttestationResponse
https://bugs.webkit.org/show_bug.cgi?id=238966

Attachment 457282: Patch

https://bugs.webkit.org/attachment.cgi?id=457282&action=review




--- Comment #7 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 457282
  --> https://bugs.webkit.org/attachment.cgi?id=457282
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review

I think this looks good, but please correct the ArrayBuffer handling. At
present, I think you will end up returning deallocated memory as the
ArrayBuffer will be +0 reference count as currently written.

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:63
> +ArrayBuffer* AuthenticatorAttestationResponse::getAuthenticatorData() const

This should be RefPtr<ArrayBuffer> ...

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:68
> +	   return nullptr;

You could do something fancy for JavaScript:

if (!decodedResponse || !decodedResponse->isMap())
    return Exception { SyntaxError, "CBOR parsing failed"_s };

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:74
> +	   return nullptr;

Ditto:
if (it == attObjMap.end() || !it->second.isByteString())
    return Exception { SyntaxError, "Attestation missing authData <<OR
SOMETHING>>"_s {;

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:75
> +    }

Is there any benefit to caching the converted bytes so we don't have to do this
work repeatedly? Or does the m_attestationObject change too much for that to be
worth doing?

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77
> +    return ArrayBuffer::create(authData.data(), authData.size()).ptr();

Why don't we return the Ref<> instead of this bare pointer? Seems likely to be
the source of memory issues.

Instead, I think you should do:

return ArrayBuffer::tryCreate(authData.data(), authData.size());

Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too.

> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:44
> +    ArrayBuffer* getAuthenticatorData() const;

I think this should be something like:
    ExceptionOr<RefPtr<JSC::ArrayBuffer>> getAuthenticatorData();

Or just:
RefPtr<JSC::ArrayBuffer> getAuthenticationData();