Bug 191517

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

<rdar://problem/46888222>

Created attachment 369216 [details] Patch

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 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.

Created attachment 369332 [details] Patch for landing

Comment on attachment 369332 [details] Patch for landing Clearing flags on attachment: 369332 Committed r245043: <https://trac.webkit.org/changeset/245043>

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

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