Bug 222018 – PCM: Write more blinded secret tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Write more blinded secret tests.

[Radar WebKit Bug Importer]

<rdar://problem/74674160>

[Jiewen Tan]

Created attachment 425493 [details]
Patch

[Jiewen Tan]

Created attachment 425495 [details]
Patch

[John Wilander]

The change seems to have broken the layout test.

[Jiewen Tan]

Created attachment 425534 [details]
Patch

[Jiewen Tan]

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

Should be fixed.

[Jiewen Tan]

Created attachment 425536 [details]
Patch

[John Wilander]

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]

Created attachment 425581 [details]
Patch for landing

[EWS]

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