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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 8 15:03:11 PDT 2021

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

Attachment 425536: Patch


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

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


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

More information about the webkit-reviews mailing list