Bug 191517 – [WebAuthN] A new request should always cancel any pending request

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

[Jiewen Tan]

A pending request should be cancelled whenever its initiator page is closed or refreshed.

[Jiewen Tan]

*** Bug 192846 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

<rdar://problem/46888222>

[Jiewen Tan]

Created attachment 369216 [details]
Patch

[Brent Fulgham]

Comment on attachment 369216 [details]
Patch

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

Looks good! I made some suggestions for the text of the comments and changelog notes.

> Source/WebCore/ChangeLog:3
> +        [WebAuthN] A new request should always suppress the pending request if any

Suppressing the request makes it sound like the original request is still happening, but is silenced or delayed.

I think it would be better (based on the code) to describe this as "A new request should always cancel any pending request".

> Source/WebCore/ChangeLog:11
> +        hanedled/timeout]. Therefore, the policy is then made to always suppress the pending

... the policy will be to always cancel any pending requests whenever a new request is made. This will enforce the policy of handling only one request at a time."

> Source/WebKit/ChangeLog:12
> +        to call. Therefore, the above issue doesn't exist anymore.

'Previously we blocked new WebAuthN requests whenever a pending request was in progress to prevent background tabs from DoS foreground tabs. However, in r244938, the WebAuthN API was changed to restrict request handling to the focused document. Therefore, we no longer have a risk of DoS."

> Source/WebKit/ChangeLog:15
> +        WebAuhtN API in the period between [the previous initating page is closed, the pending

WebAuthN

> Source/WebKit/ChangeLog:18
> +        Also, it makes sense to have the current focused document to preempt the pending request.

"Also, it makes sense to have the current focused document preempt the pending request.

> Source/WebKit/ChangeLog:26
> +        this issue at this moment. It will be addressed in Bug 191523.

Note that the current implementation doesn't explicitly cancel pending requests in the Authenticators, which means that we could receive responses from the Authenticator that were meant for a previous (now cancelled) request. A follow-up patch (see Bug 191523) will implement an Authenticator feature to support immediate cancellation.

In the meantime, to protect the atomicity of the request/response pair, i.e., preventing an old response being used for a new request, there are two safeguards:

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:63
> +    // Only one request is allowed at one time. A new reqeuest will suppress the pending request if any.

A new request will cancel any pending request

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:132
> +        m_pendingCompletionHandler(ExceptionData { NotAllowedError, "This request is cancelled by a new request."_s });

"This request has been cancelled by a new request"?

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:151
> +        m_pendingCompletionHandler(ExceptionData { NotAllowedError, "This request is cancelled by a new request."_s });

Ditto

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:83
> +    // Request: We only allow one request per time. A new request will cancel the pending one if any.

... A new request will cancel any pending ones.

[Jiewen Tan]

Comment on attachment 369216 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebCore/ChangeLog:3
>> +        [WebAuthN] A new request should always suppress the pending request if any
> 
> Suppressing the request makes it sound like the original request is still happening, but is silenced or delayed.
> 
> I think it would be better (based on the code) to describe this as "A new request should always cancel any pending request".

Fixed.

>> Source/WebCore/ChangeLog:11
>> +        hanedled/timeout]. Therefore, the policy is then made to always suppress the pending
> 
> ... the policy will be to always cancel any pending requests whenever a new request is made. This will enforce the policy of handling only one request at a time."

Fixed.

>> Source/WebKit/ChangeLog:12
>> +        to call. Therefore, the above issue doesn't exist anymore.
> 
> 'Previously we blocked new WebAuthN requests whenever a pending request was in progress to prevent background tabs from DoS foreground tabs. However, in r244938, the WebAuthN API was changed to restrict request handling to the focused document. Therefore, we no longer have a risk of DoS."

Fixed.

>> Source/WebKit/ChangeLog:15
>> +        WebAuhtN API in the period between [the previous initating page is closed, the pending
> 
> WebAuthN

Fixed.

>> Source/WebKit/ChangeLog:18
>> +        Also, it makes sense to have the current focused document to preempt the pending request.
> 
> "Also, it makes sense to have the current focused document preempt the pending request.

Fixed.

>> Source/WebKit/ChangeLog:26
>> +        this issue at this moment. It will be addressed in Bug 191523.
> 
> Note that the current implementation doesn't explicitly cancel pending requests in the Authenticators, which means that we could receive responses from the Authenticator that were meant for a previous (now cancelled) request. A follow-up patch (see Bug 191523) will implement an Authenticator feature to support immediate cancellation.
> 
> In the meantime, to protect the atomicity of the request/response pair, i.e., preventing an old response being used for a new request, there are two safeguards:

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorCoordinatorClient.h:63
>> +    // Only one request is allowed at one time. A new reqeuest will suppress the pending request if any.
> 
> A new request will cancel any pending request

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:132
>> +        m_pendingCompletionHandler(ExceptionData { NotAllowedError, "This request is cancelled by a new request."_s });
> 
> "This request has been cancelled by a new request"?

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:151
>> +        m_pendingCompletionHandler(ExceptionData { NotAllowedError, "This request is cancelled by a new request."_s });
> 
> Ditto

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:83
>> +    // Request: We only allow one request per time. A new request will cancel the pending one if any.
> 
> ... A new request will cancel any pending ones.

Fixed.

[Jiewen Tan]

Created attachment 369332 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369332 [details]
Patch for landing

Clearing flags on attachment: 369332

Committed r245043: <https://trac.webkit.org/changeset/245043>

[Jiewen Tan]

A build fix is committed:
Committed r245052: <https://trac.webkit.org/changeset/245052>

[Jiewen Tan]

*** Bug 192446 has been marked as a duplicate of this bug. ***