Bug 203830

Summary: [WebAuthn] Guard towards unexpected -[_WKWebAuthenticationPanel cancel]
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, beidson, bfulgham, commit-queue, jiewen_tan, jonathan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
bfulgham: review+, bfulgham: commit-queue-
Patch for landing none

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