Bug 183530 – [WebAuthn] Roll back newly created credentials if an error occurs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Current error handling model for WebAuthN is just returning an exception.

A more graceful model should also cleanup any messed states as well.

[Radar WebKit Bug Importer]

<rdar://problem/43357305>

[Jiewen Tan]

Also, we need to carefully pick sentences to describe error such that we can protect user privacy: https://www.w3.org/TR/webauthn/#sctn-privacy-considerations.

For example, we might want to make NotAllowedError indistinguishable between each other.

[Jiewen Tan]

Created attachment 398712 [details]
Patch

[Brent Fulgham]

Comment on attachment 398712 [details]
Patch

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

I think this looks good, but I think you made a typo "NSDate instead of NSData".

> Source/WebKit/ChangeLog:3
> +        [WebAuthn] Roll back the newly created credential if an error happens afterwards

Maybe "Roll back newly created credentials if an error occurs"?

> Source/WebKit/ChangeLog:9
> +        Otherwise, the newly created credential will be a dangling credential.

Maybe "We should clean up any newly created credentials if an error occurs before the relying party registers the identity. Otherwise we are left with a dangling credential."

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:78
> +    RetainPtr<NSDate> m_newlyCreatedCredentialId;

This should be an NSData, I think (not an NSDate).

Maybe we should call this "m_provisionalCredentialId"?

Also: Should this be nulled at some point when the credential moves from 'provisional' to 'committed' (or 'complete') ?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:340
> +        m_newlyCreatedCredentialId = toNSData(credentialId);

m_provisionalCredentialId

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:605
> +            LOG_ERROR(makeString("Couldn't delete older credential: "_s, status).utf8().data());

I think this should say something like:

"Couldn't delete provisional credential while handling error: "

[Jiewen Tan]

Comment on attachment 398712 [details]
Patch

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

Thanks Brent for reviewing this patch.

>> Source/WebKit/ChangeLog:3
>> +        [WebAuthn] Roll back the newly created credential if an error happens afterwards
> 
> Maybe "Roll back newly created credentials if an error occurs"?

Fixed.

>> Source/WebKit/ChangeLog:9
>> +        Otherwise, the newly created credential will be a dangling credential.
> 
> Maybe "We should clean up any newly created credentials if an error occurs before the relying party registers the identity. Otherwise we are left with a dangling credential."

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:78
>> +    RetainPtr<NSDate> m_newlyCreatedCredentialId;
> 
> This should be an NSData, I think (not an NSDate).
> 
> Maybe we should call this "m_provisionalCredentialId"?
> 
> Also: Should this be nulled at some point when the credential moves from 'provisional' to 'committed' (or 'complete') ?

Oops, it is indeed a type. But strangely, it compiles and works. There is no needs to commit it. Once the credential is returned to the RP, the LocalAuthenticator object will be cleared.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:340
>> +        m_newlyCreatedCredentialId = toNSData(credentialId);
> 
> m_provisionalCredentialId

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:605
>> +            LOG_ERROR(makeString("Couldn't delete older credential: "_s, status).utf8().data());
> 
> I think this should say something like:
> 
> "Couldn't delete provisional credential while handling error: "

Fixed.

[Jiewen Tan]

Created attachment 398821 [details]
Patch

[Jiewen Tan]

Created attachment 398823 [details]
Patch

[Brent Fulgham]

Comment on attachment 398823 [details]
Patch

r=me

[Jiewen Tan]

(In reply to Brent Fulgham from comment #8)
> Comment on attachment 398823 [details]
> Patch
> 
> r=me

Thanks for r+ the patch.

[Jiewen Tan]

Committed r261366: <https://trac.webkit.org/changeset/261366>