[webkit-reviews] review denied: [Bug 233673] PCM: Unlinkable tokens for triggering event to prevent fraud : [Attachment 445521] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 233673: PCM: Unlinkable tokens for triggering event to prevent fraud
https://bugs.webkit.org/show_bug.cgi?id=233673

Attachment 445521: Patch

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 445521
  --> https://bugs.webkit.org/attachment.cgi?id=445521
Patch

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

> Source/WebCore/loader/PrivateClickMeasurement.cpp:95
> +PrivateClickMeasurement::UnlinkableToken
PrivateClickMeasurement::UnlinkableToken::isolatedCopy() const

nit: If you use trailing return type syntax, it reduces the number of
"PrivateClickMeasurement::" needed.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> +PrivateClickMeasurement::SourceUnlinkableToken
PrivateClickMeasurement::SourceUnlinkableToken::isolatedCopy() const

This is a lot of duplicate code.  Can we just have UnlinkableToken have an
isolatedCopy() without having 3 copies of the exact same code?	Same with the
secret tokens.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:164
> +    for (auto& parameter : parameters) {
> +	   if (parameter.key == "attributionSource") {

This seems to allow two attributionDestinationNonces or two attributionSources.
 I think the intent of the two parameters is that there is one
attributionSource and one attributionDestinationNonce.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:183
> +    AttributionTriggerData attributionTriggerData;

I prefer the pattern of calling the constructor with all the members when
returning it over calling the default constructor then filling in the members. 
It makes it easier to remember all the members.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:343
> +bool PrivateClickMeasurement::EphemeralDestinationNonce::isValid() const

Can we make EphemeralSourceNonce and EphemeralDestinationNonce share a parent
class to have less duplicate code like we do with the tokens?

> Source/WebCore/loader/PrivateClickMeasurement.cpp:365
> +const std::optional<const URL>
PrivateClickMeasurement::tokenPublicKeyURL(const RegistrableDomain&
registrableDomain)

A URL already has a null state.  std::optional<URL> is almost always overkill. 
Just check if the URL is non-null and valid.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:413
> +	   return reportDetails;

Do we want an empty object here instead of nullptr?

> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:91
> +Expected<PrivateClickMeasurement::DestinationSecretToken, String>
PrivateClickMeasurement::calculateAndUpdateDestinationSecretToken(const String&
serverResponseBase64URL, DestinationUnlinkableToken& unlinkableToken)

Why is this signature so different from calculateAndUpdateSourceSecretToken?

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDat
abase.cpp:719
>  bool Database::needsUpdatedSchema()

I'm not sure if this needs updating.  Schema migration needs a test like
MigrateFromPCM1

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDat
abase.h:104
> +    std::optional<WebCore::PrivateClickMeasurement::DestinationSecretToken>
m_destinationSecretToken;

Why do we need to keep a destination secret token here but not a source secret
token here?

>
Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementMan
ager.cpp:283
> +void
PrivateClickMeasurementManager::getSignedUnlinkableTokenForDestination(SourceSi
te&& sourceSite, AttributionDestinationSite&& destinationSite,
AttributionTriggerData&& attributionTriggerData, const
ApplicationBundleIdentifier& applicationBundleIdentifier)

This shares a lot of code with
PrivateClickMeasurementManager::getSignedUnlinkableToken.  Can they be better
abstracted?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:349
> +TEST(PrivateClickMeasurement, DestinationClickFraudPrevention)

This has a lot of duplicate code with TEST(PrivateClickMeasurement,
FraudPrevention).  Can they share anything?