Bug 203830 - [WebAuthn] Guard towards unexpected -[_WKWebAuthenticationPanel cancel]
Summary: [WebAuthn] Guard towards unexpected -[_WKWebAuthenticationPanel cancel]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
: 204034 (view as bug list)
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2019-11-04 16:03 PST by Jiewen Tan
Modified: 2019-11-08 17:25 PST (History)
7 users (show)

See Also:


Attachments
Patch (13.67 KB, patch)
2019-11-04 16:18 PST, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (13.66 KB, patch)
2019-11-04 19:20 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-11-04 16:03:36 PST
-[_WKWebAuthenticationPanel cancel] can be called by clients in many different scenarios other than to express a real user cancel. We should expect that as well.
Comment 1 Jiewen Tan 2019-11-04 16:03:51 PST
<rdar://problem/56797134>
Comment 2 Jiewen Tan 2019-11-04 16:18:08 PST
Created attachment 382785 [details]
Patch
Comment 3 Brent Fulgham 2019-11-04 18:53:07 PST
Comment on attachment 382785 [details]
Patch

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

R=me

> Source/WebKit/ChangeLog:3
> +        [WebAuthn] Guard towards unexpected -[_WKWebAuthenticationPanel cancel]

Guard against

> Source/WebKit/ChangeLog:9
> +        -[_WKWebAuthenticationPanel cancel] was only expected to be called on behave of an

On behalf of

> Source/WebKit/ChangeLog:11
> +        unexpected scenarios as well. We should guard ourselves towards that.

We should guard against that.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:182
> +    if (m_pendingRequestData.frameID) {

If (auto* pendingFrameID = m_pendingRequestData.frameID)

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:62
> +    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the states

State should be singular in this use.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:63
> +    // of current run loop in unexpected ways.

Of the current

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:94
> +    // of current run loop in unexpected ways.

Ditto for both prior comments.
Comment 4 Jiewen Tan 2019-11-04 19:05:22 PST
Comment on attachment 382785 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/ChangeLog:3
>> +        [WebAuthn] Guard towards unexpected -[_WKWebAuthenticationPanel cancel]
> 
> Guard against

Fixed.

>> Source/WebKit/ChangeLog:9
>> +        -[_WKWebAuthenticationPanel cancel] was only expected to be called on behave of an
> 
> On behalf of

Fixed.

>> Source/WebKit/ChangeLog:11
>> +        unexpected scenarios as well. We should guard ourselves towards that.
> 
> We should guard against that.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:182
>> +    if (m_pendingRequestData.frameID) {
> 
> If (auto* pendingFrameID = m_pendingRequestData.frameID)

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:62
>> +    // Call delegates in the next run loop to prevent clients' reentrance that would potentially modify the states
> 
> State should be singular in this use.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:63
>> +    // of current run loop in unexpected ways.
> 
> Of the current

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:94
>> +    // of current run loop in unexpected ways.
> 
> Ditto for both prior comments.

Fixed.
Comment 5 Jiewen Tan 2019-11-04 19:10:29 PST
Comment on attachment 382785 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:182
>>> +    if (m_pendingRequestData.frameID) {
>> 
>> If (auto* pendingFrameID = m_pendingRequestData.frameID)
> 
> Fixed.

m_pendingRequestData.frameID is of type Optional<>. So I modified it to auto pendingFrameID = m_pendingRequestData.frameID.
Comment 6 Jiewen Tan 2019-11-04 19:20:24 PST
Created attachment 382797 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-11-04 20:05:15 PST
Comment on attachment 382797 [details]
Patch for landing

Clearing flags on attachment: 382797

Committed r252035: <https://trac.webkit.org/changeset/252035>
Comment 8 Jiewen Tan 2019-11-08 17:25:47 PST
*** Bug 204034 has been marked as a duplicate of this bug. ***