Bug 213404

Summary:

[WebAuthn] Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid

Product:

WebKit

Reporter:

Jiewen Tan <jiewen_tan>

Component:

WebKit Misc.

Assignee:

Jiewen Tan <jiewen_tan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, darin, jiewen_tan, webkit-bug-importer

Priority:

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

Jiewen Tan

Reported 2020-06-19 13:54:06 PDT

Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid.

Attachments
Patch (18.82 KB, patch) 2020-06-19 14:03 PDT, Jiewen Tan	bfulgham: review+ bfulgham: commit-queue-	Details Formatted Diff Diff
Patch for landing (18.37 KB, patch) 2020-06-19 16:18 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-06-19 13:54:37 PDT

<rdar://problem/64543894>

Jiewen Tan

Comment 2 2020-06-19 14:03:30 PDT

Created attachment 402322 [details] Patch

Brent Fulgham

Comment 3 2020-06-19 15:12:09 PDT

Comment on attachment 402322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402322&action=review > Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:277 > + if (!pin) { I know this works, but it's confusing. I'd prefer to see: if (pin.isNull()) { Also: What if the pin is an empty string? Shouldn't we hit this code too? operator ! only checks null, which means we would pass the empty string to the late parts of this method. Maybe that's okay because it fails the 'validateAndConvertToUTF8' step? if (pin.isNull() || pin.isEmpty()) { > Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:975 > + webAuthenticationPanelPin = "1234"; Should we have a test case where the WebKit client passed "" as the string?

Darin Adler

Comment 4 2020-06-19 15:22:52 PDT

(In reply to Brent Fulgham from comment #3) > if (pin.isNull() || pin.isEmpty()) { You never need to write this. Instead: if (pin.isEmpty()) {

Jiewen Tan

Comment 5 2020-06-19 15:51:06 PDT

Comment on attachment 402322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402322&action=review Thanks, Brent for r+ this patch. >> Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:277 >> + if (!pin) { > > I know this works, but it's confusing. I'd prefer to see: > > if (pin.isNull()) { > > Also: What if the pin is an empty string? Shouldn't we hit this code too? > > operator ! only checks null, which means we would pass the empty string to the late parts of this method. Maybe that's okay because it fails the 'validateAndConvertToUTF8' step? > > if (pin.isNull() || pin.isEmpty()) { Yes, it fails validateAndConvertToUTF8 step. Therefore, it's not necessary to check anything here. Actually, I think it is better because in the current implementation of the UI, the user can submit an empty PIN code, and I don't think we will to treat it as a cancel.

Jiewen Tan

Comment 6 2020-06-19 15:52:30 PDT

Comment on attachment 402322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402322&action=review >> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:975 >> + webAuthenticationPanelPin = "1234"; > > Should we have a test case where the WebKit client passed "" as the string? It has the same effect as the "123" above. So I think it's okay to not have it.

Jiewen Tan

Comment 7 2020-06-19 16:18:52 PDT

Created attachment 402342 [details] Patch for landing

Jiewen Tan

Comment 8 2020-06-19 16:19:01 PDT

(In reply to Darin Adler from comment #4) > (In reply to Brent Fulgham from comment #3) > > if (pin.isNull() || pin.isEmpty()) { > > You never need to write this. Instead: > > if (pin.isEmpty()) { Thanks, Darin!

EWS

Comment 9 2020-06-19 16:40:08 PDT

Committed r263296: <https://trac.webkit.org/changeset/263296> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402342 [details].

Note You need to log in before you can comment on or make changes to this bug.