Bug 222018

Summary:

PCM: Write more blinded secret tests

Product:

WebKit

Reporter:

Jiewen Tan <jiewen_tan>

Component:

WebKit Misc.

Assignee:

Jiewen Tan <jiewen_tan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, jiewen_tan, webkit-bug-importer, wilander

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

221291

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	wilander: review+
Patch for landing	none

Jiewen Tan

Reported 2021-02-16 19:11:53 PST

Write more blinded secret tests.

Attachments
Patch (17.30 KB, patch) 2021-04-08 03:54 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (17.45 KB, patch) 2021-04-08 04:04 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (19.08 KB, patch) 2021-04-08 12:53 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (19.81 KB, patch) 2021-04-08 12:57 PDT, Jiewen Tan	wilander: review+	Details Formatted Diff Diff
Patch for landing (20.48 KB, patch) 2021-04-08 20:40 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2021-02-23 19:12:14 PST

<rdar://problem/74674160>

Jiewen Tan

Comment 2 2021-04-08 03:54:15 PDT

Created attachment 425493 [details] Patch

Jiewen Tan

Comment 3 2021-04-08 04:04:02 PDT

Created attachment 425495 [details] Patch

John Wilander

Comment 4 2021-04-08 06:35:49 PDT

The change seems to have broken the layout test.

Jiewen Tan

Comment 5 2021-04-08 12:53:42 PDT

Created attachment 425534 [details] Patch

Jiewen Tan

Comment 6 2021-04-08 12:53:56 PDT

(In reply to John Wilander from comment #4) > The change seems to have broken the layout test. Should be fixed.

Jiewen Tan

Comment 7 2021-04-08 12:57:35 PDT

Created attachment 425536 [details] Patch

John Wilander

Comment 8 2021-04-08 15:03:11 PDT

Comment on attachment 425536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425536&action=review Looks good! Please address the comments below and also file a follow-up bug to add negative tests, i.e. tests for badly formatted keys and signatures. > Source/WebKit/ChangeLog:13 > + The KeyID is not truncated. Does this mean to say "The KeyID is no longer truncated"? > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:171 > + auto response = makeString("HTTP/1.1 200 OK\r\n" I think we should have a commented above this piece of code with an example response body. It's very hard to read what this test is expecting and developers might look to see what the server is supposed to do. > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:194 > + auto response = makeString("HTTP/1.1 200 OK\r\n" Ditto regarding an example comment above. > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:202 > + auto response = makeString("HTTP/1.1 200 OK\r\n" Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:212 > + // FIXME(224321): Determine if it is possible to verify the token and signature. So this would be the test case validating the source_secret_token? > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:214 > + EXPECT_TRUE(strnstr(request4.data(), "source_secret_token_signature", request4.size())); We should add an EXPECT_TRUE(unlinkableToken != secretToken) which is a basic condition here.

Jiewen Tan

Comment 9 2021-04-08 20:40:43 PDT

Created attachment 425581 [details] Patch for landing

EWS

Comment 10 2021-04-08 21:06:47 PDT

Committed r275748 (236326@main): <https://commits.webkit.org/236326@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425581 [details].

Jiewen Tan

Comment 11 2021-04-09 00:14:21 PDT

Comment on attachment 425536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425536&action=review Thanks John for r+ this patch. >> Source/WebKit/ChangeLog:13 >> + The KeyID is not truncated. > > Does this mean to say "The KeyID is no longer truncated"? Yes. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:171 >> + auto response = makeString("HTTP/1.1 200 OK\r\n" > > I think we should have a commented above this piece of code with an example response body. It's very hard to read what this test is expecting and developers might look to see what the server is supposed to do. Fixed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:194 >> + auto response = makeString("HTTP/1.1 200 OK\r\n" > > Ditto regarding an example comment above. Fixed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:202 >> + auto response = makeString("HTTP/1.1 200 OK\r\n" > > Ditto. Fixed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:212 >> + // FIXME(224321): Determine if it is possible to verify the token and signature. > > So this would be the test case validating the source_secret_token? We need to ask the Crypto team for this. That's what the follow up is about. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:214 >> + EXPECT_TRUE(strnstr(request4.data(), "source_secret_token_signature", request4.size())); > > We should add an EXPECT_TRUE(unlinkableToken != secretToken) which is a basic condition here. Fixed.

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