[webkit-reviews] review granted: [Bug 222141] PCM: Request server public keys to generate secret token : [Attachment 420885] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 18 17:36:32 PST 2021

John Wilander <wilander at apple.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 222141: PCM: Request server public keys to generate secret token

Attachment 420885: Patch


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

View in context: https://bugs.webkit.org/attachment.cgi?id=420885&action=review

r=me with comments.

> Source/WebCore/ChangeLog:3
> +	   PCM: Request server public keys to generate secret token

Nit: key, not keys

> Source/WebKit/ChangeLog:28
> +	   Teaches the PCMM to send the token public key request.

Nit: PCM, not PCMM.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:41
> +static const char privateClickMeasurementTokenPublicKeyPath[] =

We should formulate this as an action, so /get-unlinkable-token-public-key/.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:95
> +    request.setHTTPMethod(httpMethod);


> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:119
> +static NetworkResourceLoadParameters
generateNetworkResourceLoadParametersForPost(URL&& url, Ref<JSON::Object>&&
jsonPayload, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)

I would add HTTP, so generateNetworkResourceLoadParametersForHTTPPost.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:124
> +static NetworkResourceLoadParameters
generateNetworkResourceLoadParametersForGet(URL&& url,
PrivateClickMeasurement::PcmDataCarried dataTypeCarried)

I would add HTTP, so generateNetworkResourceLoadParametersForHTTPGet.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:129
> +void

I wonder if other WebKit developers will bark at this use of "get" as the
leading verb? Maybe we should use the word "retrieve" instead just to avoid the
potential complaint? In my personal view it *is* good use of the word "get"
since we are getting things from the server. Naming it "fetch" instead doesn't
help since it would lead people to think that this is tied to Fetch.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:135
> +    auto pcmDataCarried =

I would add a comment here saying "This is guaranteed to be close in time to
the navigational click which makes it likely to be personally identifiable."

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:159
> +	   getSignedUnlinkableToken(attribution);


> Tools/ChangeLog:3
> +	   PCM: Request server public keys to generate secret token

Nit: key

> LayoutTests/ChangeLog:14
> +	   *

Please explain briefly what changed in this test case.

> LayoutTests/http/tests/privateClickMeasurement/resources/signToken.php:4
> +$tokenSigningFile = fopen($tokenSigningFilePath . ".tmp", 'a');

So this will open the file twice and only after the second append rename it so
that the polling finds it? Trying to understand the logic. We should add a
comment that explains this. Another way of doing it is having a dedicated PHP
file for fetching the singing key.

More information about the webkit-reviews mailing list