[webkit-reviews] review granted: [Bug 191517] [WebAuthN] A new request should always suppress the pending request if any : [Attachment 369216] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 191517: [WebAuthN] A new request should always suppress the pending request
if any
https://bugs.webkit.org/show_bug.cgi?id=191517

Attachment 369216: Patch

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




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 369216
  --> https://bugs.webkit.org/attachment.cgi?id=369216
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.