[webkit-reviews] review granted: [Bug 222018] PCM: Write more blinded secret tests : [Attachment 425536] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 222018: PCM: Write more blinded secret tests
https://bugs.webkit.org/show_bug.cgi?id=222018

Attachment 425536: Patch

https://bugs.webkit.org/attachment.cgi?id=425536&action=review




--- Comment #8 from John Wilander <wilander at apple.com> ---
Comment on attachment 425536
  --> https://bugs.webkit.org/attachment.cgi?id=425536
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.