Bug 233173 – PCM: Add capability for click destination to fire triggering event without cross-site requests to the click source

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

[John Wilander]

We should offer click destination sites a non-JavaScript way to fire triggering events without a requirement to make cross-site requests to source sites. This is referred to as a "same-site pixel API" and has been discussed in W3C Privacy CG: https://github.com/privacycg/private-click-measurement/issues/71. The reason why some merchants want such an "API" is reluctance to deploy new JavaScript on their sites. In some industries it's even a compliance issue. Legacy "pixels" are however accepted and so a same-site "pixel" can work for them.

[John Wilander]

<rdar://79426605>

[John Wilander]

Created attachment 444349 [details]
Patch

[John Wilander]

Created attachment 444422 [details]
Patch

[Alex Christensen]

Comment on attachment 444422 [details]
Patch

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

> Source/WTF/wtf/URL.cpp:1195
> +    return URLParser::parseURLEncodedForm(url.query());

This decodes percent encoded characters and replaces spaces with '+'.  It would be worth adding some percent-encoded non-ASCII characters in your tests to verify what happens there.

> Source/WTF/wtf/URL.h:253
> +WTF_EXPORT_PRIVATE Vector<KeyValuePair<String, String>> queryParameters(const URL&);

This should probably be Vector<KeyValuePair<String, String>> URL::queryParameters() const

> Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> +std::optional<String> PrivateClickMeasurement::parseAttributionRequestQuery(const URL& redirectURL, AttributionTriggerData& attributionTriggerData)

I would make this return an Expected<AttributionTriggerData, String> so you don't need a default constructed out param.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:109
> +    if (!redirectURL.hasQuery())

I would check if !parameters.size() to make it more straightforward below that parameters.first() will never go out of bounds.

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:251
> +    SourceSite sourceSite;

It isn't necessary to add a default constructor to SourceSite.  Could you just change the RegistrableDomain in the lines below, then still make a SourceSite with a RegistrableDomain?

[John Wilander]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 444422 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444422&action=review
> 
> > Source/WTF/wtf/URL.cpp:1195
> > +    return URLParser::parseURLEncodedForm(url.query());
> 
> This decodes percent encoded characters and replaces spaces with '+'.  It
> would be worth adding some percent-encoded non-ASCII characters in your
> tests to verify what happens there.

I'm looking for exact key matches. Are you suggesting negative tests or for the value which then gets turned into a URL?

> > Source/WTF/wtf/URL.h:253
> > +WTF_EXPORT_PRIVATE Vector<KeyValuePair<String, String>> queryParameters(const URL&);
> 
> This should probably be Vector<KeyValuePair<String, String>>
> URL::queryParameters() const

It's a free function. I believe Darin wanted us to make these as such. See removeQueryParameters().

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> > +std::optional<String> PrivateClickMeasurement::parseAttributionRequestQuery(const URL& redirectURL, AttributionTriggerData& attributionTriggerData)
> 
> I would make this return an Expected<AttributionTriggerData, String> so you
> don't need a default constructed out param.

It used to be called further down in PrivateClickMeasurement::parseAttributionRequest() at which point I had the AttributionTriggerData object. But I would have to return an AttributionTriggerData object from PrivateClickMeasurement::parseAttributionRequestQuery even if there was no query string, as long as there was no error.

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:109
> > +    if (!redirectURL.hasQuery())
> 
> I would check if !parameters.size() to make it more straightforward below
> that parameters.first() will never go out of bounds.

I'll add that.

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:251
> > +    SourceSite sourceSite;
> 
> It isn't necessary to add a default constructor to SourceSite.  Could you
> just change the RegistrableDomain in the lines below, then still make a
> SourceSite with a RegistrableDomain?

I'll have a look.

Thanks!

[John Wilander]

Created attachment 444460 [details]
Patch

[John Wilander]

Nothing done about the URLParser::parseURLEncodedForm(url.query()) testing bit. Are you suggesting URL API tests there? It feels like a more narrow function name could be good too, to signal to users what the decoding looks like. I could also skip adding this function and use URLParser::parseURLEncodedForm(url.query()) directly in the patch's code.

[John Wilander]

Comment on attachment 444460 [details]
Patch

The WK1 test failure is unrelated.

[John Wilander]

Comment on attachment 444460 [details]
Patch

Thank you, Alex!

[EWS]

Committed r285967 (244367@main): <https://commits.webkit.org/244367@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444460 [details].