Write more blinded secret tests.
<rdar://problem/74674160>
Created attachment 425493 [details] Patch
Created attachment 425495 [details] Patch
The change seems to have broken the layout test.
Created attachment 425534 [details] Patch
(In reply to John Wilander from comment #4) > The change seems to have broken the layout test. Should be fixed.
Created attachment 425536 [details] Patch
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.
Created attachment 425581 [details] Patch for landing
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].
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.