Bug 221934 – PCM: Add high entropy attributionSourceNonce attribute to anchor tags

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

[John Wilander]

The source site needs to provide an optional high entropy nonce with which PCM can asynchronously validate its subsequent, asynchronous request for a signature of its unlinkable token.

[John Wilander]

<rdar://73581230>

[John Wilander]

Created attachment 420388 [details]
Patch

[Jiewen Tan]

Comment on attachment 420388 [details]
Patch

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

Overall, LGTM. Please address my comments.

> Source/WebCore/html/HTMLAnchorElement.cpp:458
> +        if (ephemeralNonce.isValid())

Can we just use if but for early return and remove the else statement?
if (!ephemeralNonce.isValid()) {
    document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, "attributionsourcenonce was not valid."_s);
    return WTF::nullopt;
}

> Source/WebCore/loader/PrivateClickMeasurement.h:204
> +    struct EphemeralSourceNonce {

Seem unnecessary to have ephemeral? Every nonce is supposed to be used once?

> Source/WebCore/loader/PrivateClickMeasurement.h:213
> +        bool isValid() const

Instead of dong a separate isValid check from the caller. Maybe we can have a create method that only returns a valid object if the input is valid. Otherwise a null opt?

> Source/WebCore/loader/PrivateClickMeasurement.h:215
> +            Vector<uint8_t> digest;

So we don't store the nonce? Then I guess we can just check the string's length? Not necessary need to decode it?

[John Wilander]

(In reply to Jiewen Tan from comment #3)
> Comment on attachment 420388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420388&action=review
> 
> Overall, LGTM. Please address my comments.
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:458
> > +        if (ephemeralNonce.isValid())
> 
> Can we just use if but for early return and remove the else statement?
> if (!ephemeralNonce.isValid()) {
>     document().addConsoleMessage(MessageSource::Other,
> MessageLevel::Warning, "attributionsourcenonce was not valid."_s);
>     return WTF::nullopt;
> }

Yes! I often miss these these things when I add the fail path last.

> > Source/WebCore/loader/PrivateClickMeasurement.h:204
> > +    struct EphemeralSourceNonce {
> 
> Seem unnecessary to have ephemeral? Every nonce is supposed to be used once?

I chose this name to make it reasonably clear that this nonce should not be stored in the database. We will transfer it to the network process where PCM data is then persisted. This value should never be persisted since it's tied to the user. That's why I call it ephemeral.

> > Source/WebCore/loader/PrivateClickMeasurement.h:213
> > +        bool isValid() const
> 
> Instead of dong a separate isValid check from the caller. Maybe we can have
> a create method that only returns a valid object if the input is valid.
> Otherwise a null opt?

Normally I'd only allow objects to be created if the input is valid but I believe it's the IPC encoding/decoding that requires empty objects to be allowed. I can certainly make the getter only return valid nonces but since this is a struct, I think any caller can just grab it.

> > Source/WebCore/loader/PrivateClickMeasurement.h:215
> > +            Vector<uint8_t> digest;
> 
> So we don't store the nonce? Then I guess we can just check the string's
> length? Not necessary need to decode it?

We don't *persist* it but we will transfer it to the network process and use it in a request.

We'd have to check that its characters are in the valid set for Base64URL and that it is exactly 128 bits. I don't know if there's an exact length match between encoded values and 128 bits. Maybe the allowed 64 characters makes it align that way?

Thanks for your comments!

[Jiewen Tan]

Comment on attachment 420388 [details]
Patch

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

>>> Source/WebCore/loader/PrivateClickMeasurement.h:213
>>> +        bool isValid() const
>> 
>> Instead of dong a separate isValid check from the caller. Maybe we can have a create method that only returns a valid object if the input is valid. Otherwise a null opt?
> 
> Normally I'd only allow objects to be created if the input is valid but I believe it's the IPC encoding/decoding that requires empty objects to be allowed. I can certainly make the getter only return valid nonces but since this is a struct, I think any caller can just grab it.

Got you.

>>> Source/WebCore/loader/PrivateClickMeasurement.h:215
>>> +            Vector<uint8_t> digest;
>> 
>> So we don't store the nonce? Then I guess we can just check the string's length? Not necessary need to decode it?
> 
> We don't *persist* it but we will transfer it to the network process and use it in a request.
> 
> We'd have to check that its characters are in the valid set for Base64URL and that it is exactly 128 bits. I don't know if there's an exact length match between encoded values and 128 bits. Maybe the allowed 64 characters makes it align that way?
> 
> Thanks for your comments!

It will be good to investigate, then we can save the decoding steps.

[John Wilander]

(In reply to Jiewen Tan from comment #5)
> Comment on attachment 420388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420388&action=review
> 
> >>> Source/WebCore/loader/PrivateClickMeasurement.h:213
> >>> +        bool isValid() const
> >> 
> >> Instead of dong a separate isValid check from the caller. Maybe we can have a create method that only returns a valid object if the input is valid. Otherwise a null opt?
> > 
> > Normally I'd only allow objects to be created if the input is valid but I believe it's the IPC encoding/decoding that requires empty objects to be allowed. I can certainly make the getter only return valid nonces but since this is a struct, I think any caller can just grab it.
> 
> Got you.
> 
> >>> Source/WebCore/loader/PrivateClickMeasurement.h:215
> >>> +            Vector<uint8_t> digest;
> >> 
> >> So we don't store the nonce? Then I guess we can just check the string's length? Not necessary need to decode it?
> > 
> > We don't *persist* it but we will transfer it to the network process and use it in a request.
> > 
> > We'd have to check that its characters are in the valid set for Base64URL and that it is exactly 128 bits. I don't know if there's an exact length match between encoded values and 128 bits. Maybe the allowed 64 characters makes it align that way?
> > 
> > Thanks for your comments!
> 
> It will be good to investigate, then we can save the decoding steps.

Would it be OK with you to land it with decoding and investigate later? I've made the other changes and can get a new patch up.

[Jiewen Tan]

(In reply to John Wilander from comment #6)
> (In reply to Jiewen Tan from comment #5)
> > Comment on attachment 420388 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420388&action=review
> > 
> > >>> Source/WebCore/loader/PrivateClickMeasurement.h:213
> > >>> +        bool isValid() const
> > >> 
> > >> Instead of dong a separate isValid check from the caller. Maybe we can have a create method that only returns a valid object if the input is valid. Otherwise a null opt?
> > > 
> > > Normally I'd only allow objects to be created if the input is valid but I believe it's the IPC encoding/decoding that requires empty objects to be allowed. I can certainly make the getter only return valid nonces but since this is a struct, I think any caller can just grab it.
> > 
> > Got you.
> > 
> > >>> Source/WebCore/loader/PrivateClickMeasurement.h:215
> > >>> +            Vector<uint8_t> digest;
> > >> 
> > >> So we don't store the nonce? Then I guess we can just check the string's length? Not necessary need to decode it?
> > > 
> > > We don't *persist* it but we will transfer it to the network process and use it in a request.
> > > 
> > > We'd have to check that its characters are in the valid set for Base64URL and that it is exactly 128 bits. I don't know if there's an exact length match between encoded values and 128 bits. Maybe the allowed 64 characters makes it align that way?
> > > 
> > > Thanks for your comments!
> > 
> > It will be good to investigate, then we can save the decoding steps.
> 
> Would it be OK with you to land it with decoding and investigate later? I've
> made the other changes and can get a new patch up.

Sure, let's file a follow up.

[John Wilander]

Created attachment 420411 [details]
Patch

[John Wilander]

(In reply to Jiewen Tan from comment #7)
> (In reply to John Wilander from comment #6)
> > (In reply to Jiewen Tan from comment #5)
> > > Comment on attachment 420388 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=420388&action=review
> > > 
> > > >>> Source/WebCore/loader/PrivateClickMeasurement.h:213
> > > >>> +        bool isValid() const
> > > >> 
> > > >> Instead of dong a separate isValid check from the caller. Maybe we can have a create method that only returns a valid object if the input is valid. Otherwise a null opt?
> > > > 
> > > > Normally I'd only allow objects to be created if the input is valid but I believe it's the IPC encoding/decoding that requires empty objects to be allowed. I can certainly make the getter only return valid nonces but since this is a struct, I think any caller can just grab it.
> > > 
> > > Got you.
> > > 
> > > >>> Source/WebCore/loader/PrivateClickMeasurement.h:215
> > > >>> +            Vector<uint8_t> digest;
> > > >> 
> > > >> So we don't store the nonce? Then I guess we can just check the string's length? Not necessary need to decode it?
> > > > 
> > > > We don't *persist* it but we will transfer it to the network process and use it in a request.
> > > > 
> > > > We'd have to check that its characters are in the valid set for Base64URL and that it is exactly 128 bits. I don't know if there's an exact length match between encoded values and 128 bits. Maybe the allowed 64 characters makes it align that way?
> > > > 
> > > > Thanks for your comments!
> > > 
> > > It will be good to investigate, then we can save the decoding steps.
> > 
> > Would it be OK with you to land it with decoding and investigate later? I've
> > made the other changes and can get a new patch up.
> 
> Sure, let's file a follow up.

I'll file a follow-up and add a fixme. Thanks!

[John Wilander]

Created attachment 420412 [details]
Patch

[Jiewen Tan]

Comment on attachment 420412 [details]
Patch

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

r=me. Please address the FIXME before landing.

> Source/WebCore/loader/PrivateClickMeasurement.h:214
> +        // https://bugs.webkit.org/show_bug.cgi?id=221945

I think it should be FIXME(221945)? Instead of having the whole bug report URL.

[John Wilander]

(In reply to Jiewen Tan from comment #11)
> Comment on attachment 420412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420412&action=review
> 
> r=me. Please address the FIXME before landing.
> 
> > Source/WebCore/loader/PrivateClickMeasurement.h:214
> > +        // https://bugs.webkit.org/show_bug.cgi?id=221945
> 
> I think it should be FIXME(221945)? Instead of having the whole bug report
> URL.

Huh. I've always heard people want a clickable URL in there and I mostly hear people want it on its own line unless the comment is super short. Looking through the code, I can see all kinds of formats.

[John Wilander]

Comment on attachment 420412 [details]
Patch

Thanks, Jiewen!

[EWS]

Committed r272894: <https://commits.webkit.org/r272894>

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