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.
<rdar://73581230>
Created attachment 420388 [details] Patch
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?
(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!
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.
(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.
(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.
Created attachment 420411 [details] Patch
(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!
Created attachment 420412 [details] Patch
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.
(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.
Comment on attachment 420412 [details] Patch Thanks, Jiewen!
Committed r272894: <https://commits.webkit.org/r272894> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420412 [details].