Bug 191517

Summary: [WebAuthN] A new request should always cancel any pending request
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, bfulgham, cdumez, commit-queue, james, kieun.shin, maxwellswadling
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2018-11-11 17:12:37 PST
A pending request should be cancelled whenever its initiator page is closed or refreshed.
Comment 1 Jiewen Tan 2018-12-20 17:05:36 PST
*** Bug 192846 has been marked as a duplicate of this bug. ***
Comment 2 Jiewen Tan 2018-12-20 17:05:56 PST
<rdar://problem/46888222>
Comment 3 Jiewen Tan 2019-05-06 18:58:09 PDT
Created attachment 369216 [details]
Patch
Comment 4 Brent Fulgham 2019-05-07 09:01:02 PDT
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.
Comment 5 Jiewen Tan 2019-05-07 14:28:00 PDT
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.
Comment 6 Jiewen Tan 2019-05-07 16:04:18 PDT
Created attachment 369332 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-05-07 16:43:25 PDT
Comment on attachment 369332 [details]
Patch for landing

Clearing flags on attachment: 369332

Committed r245043: <https://trac.webkit.org/changeset/245043>
Comment 8 Jiewen Tan 2019-05-08 01:06:05 PDT
A build fix is committed:
Committed r245052: <https://trac.webkit.org/changeset/245052>
Comment 9 Jiewen Tan 2019-06-17 11:05:47 PDT
*** Bug 192446 has been marked as a duplicate of this bug. ***