1435155 - Redact the full shipping address from the shippingaddresschange event

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Matthew N. [:MattN]]

Currently Edge and Chrome leak the user's full shipping address (including phone number and name) to the merchant before the user even decides to "Pay". This will be a surprise to users as there wasn't clear consent given yet… perhaps they just want to see what the total price will be on an item and don't plan on purchasing.

https://github.com/w3c/payment-request/issues/648 plans to specify redaction of the shipping address to the minimum required to calculate shipping costs. Filing this already to get it tracked for implementation as it'll be a blocker for shipping and probably for enabling on Nightly and testing with partners in milestone 3 of the front end implementation.

Comment 1

[Henri Sivonen (:hsivonen)]

Marcos, can you show me a toy app that receives this event?

AFAICT, when the user has not agreed to an analog of the geolocation prompt, it's not OK for the browser to expose the zip code and country.

I'd like to understand better what the user-perceived flow is like for it to make sense to be in a state where it's already OK to expose the zip code and country but not already OK to expose the full address.

(In any case, the spec really needs to cover this in detail. It's not OK to leave stuff like method of redaction up to the browsers if the level of redaction is sensitive to what costs the sites can calculate.)

Comment 2

[Matthew N. [:MattN]]

(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Marcos, can you show me a toy app that receives this event?

https://googlechrome.github.io/samples/paymentrequest/shipping-options/ but note that it doesn't work in Fx yet because it uses legacy syntax: https://github.com/GoogleChrome/samples/pull/534

Comment 3

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Marcos, can you show me a toy app that receives this event?

Matt's example will probably do. You can view it in Chrome for now, as our payment sheet is not quite ready. 
 
> AFAICT, when the user has not agreed to an analog of the geolocation prompt,
> it's not OK for the browser to expose the zip code and country.

Agree... but there are some presuppositions made here: 

 1. the user wants to buy a physical thing - there was some action that brought them to the merchant's website.
 2. the merchant needs to send the user that physical thing - but it costs money depending on locale, and some things can't be shipped to some places. 
 3. the user has clicked "I want to buy this thing" on the site - showing the payment sheet is gated on user action.   
 
Thus, the dialog between the user and the merchant requires a negotiation of location information. We require the user to click, or otherwise indicate that they consent, to sharing some location information with the merchant in order to calculate shipping costs.

(Yes, I know the above can be remodeled as an attack... but let's assume the common case for now)

> I'd like to understand better what the user-perceived flow is like for it to
> make sense to be in a state where it's already OK to expose the zip code and
> country but not already OK to expose the full address.


Depends on how we view the world: 

##REDACTED
We believe that a merchant can determine the cost of shipping with only a minimal amount of information (excludeList). And, we believe the only use case for firing a "shippingaddresschange" event is for calculating shipping.

Pros: privacy preserving for user. 

Cons: information might be insufficient to calculate shipping. The final address might turn out to be somewhere the merchant cannot ship to (e.g., PO Box).

User flow: browser shows payment sheet. User selects or enters an address. Merchant updates shipping options, signals error, or aborts. If user agrees with shipping costs, they hit Pay. Merchant gets final address. Merchant completes response with "error" if they can't ship, or completes successfully. 

## COMPLETE
We believe that the best price determination can only come from giving a merchant a PaymentAddress that is as populated as possible. The merchant can then use that information to adjust prices and shipping options as they see fit. 

Pros: merchant can make highly accurate pricing suggestions with relation to shipping. Merchant can more quickly determine if they can ship to the address or not. 

Cons: merchant gets more information than they might need to adequately determine the cost of shipping. They could misuse this information, by storing it before the use has actually committed to buying anything.  

User flow: browser shows payment sheet, user selects or enters an address. Merchant updates shipping options, signals error, or aborts. If user agrees with shipping costs, they hit Pay. Merchant is confident they can ship, so payment completes successfully. 

## USER CONTROLLED
We believe the user is in the best position to determine what to share with the site. The site should negotiate with the user as to the information they need.

Pros: user in control of what they share during the shipping cost negotiation phase. 

Cons: may require a lot of back and forth between merchant and user. Not really supported by the API right now, because the API assumed everyone would have a "COMPLETE" world view. 

User flow: browser shows payment sheet, user selects what parts of the address they want to share. Merchant updates shipping options, signals error, or aborts (there is no way to do fine grained error correction right now). If user agrees with shipping costs, they hit Pay. Merchant is confident they can ship, so payment completes successfully. 

> (In any case, the spec really needs to cover this in detail. It's not OK to
> leave stuff like method of redaction up to the browsers if the level of
> redaction is sensitive to what costs the sites can calculate.)

The situation is as follows, when the user changes the shipping address:

 * Safari: only exposes enough information to calculate shipping (REDACTED world view). 
 * Chrome, Edge - returns everything (COMPLETE world view).

The spec tries to capture the above situation. Where we (Firefox) sit is up to us, but we were aligning with the REDACTED view.

I'm of course open to clarifying that further in the spec in the privacy and security section.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #3)
>  3. the user has clicked "I want to buy this thing" on the site - showing
> the payment sheet is gated on user action.   

To be clear: When we expose the zip code and country, has the user taken some non-"Cancel" action on the payment sheet or just on the site?

> (Yes, I know the above can be remodeled as an attack... but let's assume the
> common case for now)

Surely we should assume an attack when evaluating the design.

> > I'd like to understand better what the user-perceived flow is like for it to
> > make sense to be in a state where it's already OK to expose the zip code and
> > country but not already OK to expose the full address.
> 
> 
> Depends on how we view the world: 
> 
> ##REDACTED
> We believe that a merchant can determine the cost of shipping with only a
> minimal amount of information (excludeList). And, we believe the only use
> case for firing a "shippingaddresschange" event is for calculating shipping.
> 
> Pros: privacy preserving for user. 
> 
> Cons: information might be insufficient to calculate shipping. The final
> address might turn out to be somewhere the merchant cannot ship to (e.g., PO
> Box).
> 
> User flow: browser shows payment sheet. User selects or enters an address.

Is this the point at which Firefox exposes the zip code and country? I.e. in response to user interaction on the payment sheet?

> Merchant updates shipping options, signals error, or aborts. If user agrees
> with shipping costs, they hit Pay. Merchant gets final address. Merchant
> completes response with "error" if they can't ship, or completes
> successfully. 
...
> > (In any case, the spec really needs to cover this in detail. It's not OK to
> > leave stuff like method of redaction up to the browsers if the level of
> > redaction is sensitive to what costs the sites can calculate.)
> 
> The situation is as follows, when the user changes the shipping address:
> 
>  * Safari: only exposes enough information to calculate shipping (REDACTED
> world view). 
>  * Chrome, Edge - returns everything (COMPLETE world view).
> 
> The spec tries to capture the above situation. Where we (Firefox) sit is up
> to us, but we were aligning with the REDACTED view.

The spec is https://w3c.github.io/payment-request/ , right? I don't find the case-insensitive string "redact" in the spec and I don't see a discussion of redaction by another name in proximity to the shippingaddresschange event. The Privacy Considerations section is silent on this.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #2)
> (In reply to Henri Sivonen (:hsivonen) from comment #1)
> > Marcos, can you show me a toy app that receives this event?
> 
> https://googlechrome.github.io/samples/paymentrequest/shipping-options/ but
> note that it doesn't work in Fx yet because it uses legacy syntax:
> https://github.com/GoogleChrome/samples/pull/534

Thanks. Looks like I need to host a patched copy of this myself then.

Comment 5

[Matthew N. [:MattN]]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> (In reply to Marcos Caceres [:marcosc] from comment #3)
> > > (In any case, the spec really needs to cover this in detail. It's not OK to
> > > leave stuff like method of redaction up to the browsers if the level of
> > > redaction is sensitive to what costs the sites can calculate.)
> > 
> > The situation is as follows, when the user changes the shipping address:
> > 
> >  * Safari: only exposes enough information to calculate shipping (REDACTED
> > world view). 
> >  * Chrome, Edge - returns everything (COMPLETE world view).
> > 
> > The spec tries to capture the above situation. Where we (Firefox) sit is up
> > to us, but we were aligning with the REDACTED view.
> 
> The spec is https://w3c.github.io/payment-request/ , right? I don't find the
> case-insensitive string "redact" in the spec and I don't see a discussion of
> redaction by another name in proximity to the shippingaddresschange event.
> The Privacy Considerations section is silent on this.

It's still in a pull request at the moment: https://github.com/w3c/payment-request/pull/654

Comment 6

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Surely we should assume an attack when evaluating the design.

Yes, absolutely.

> > User flow: browser shows payment sheet. User selects or enters an address.
> 
> Is this the point at which Firefox exposes the zip code and country? I.e. in
> response to user interaction on the payment sheet?

Yes. An event is fired at this point. 

> > The spec tries to capture the above situation. Where we (Firefox) sit is up
> > to us, but we were aligning with the REDACTED view.
> 
> The spec is https://w3c.github.io/payment-request/ , right? I don't find the
> case-insensitive string "redact" in the spec and I don't see a discussion of
> redaction by another name in proximity to the shippingaddresschange event.

A Matt mentioned, it's still a PR, but you can preview it here:
https://pr-preview.s3.amazonaws.com/w3c/payment-request/654/f5096c6...465a9f7.html

I've renamed `excludeList` to `redactList` based on what we discussed. 

> The Privacy Considerations section is silent on this.

It will go into: 
https://github.com/w3c/payment-request/pull/683

We are rewriting the Priv/Sec section.

Comment 7

[Marcos Caceres [:marcosc]]

This is probably a better preview:
https://pr-preview.s3.amazonaws.com/w3c/payment-request/pull/654.html#physical-addresses

There are too many changes for the diff view.

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #6)
> (In reply to Henri Sivonen (:hsivonen) from comment #4)
> > Surely we should assume an attack when evaluating the design.
> 
> Yes, absolutely.
> 
> > > User flow: browser shows payment sheet. User selects or enters an address.
> > 
> > Is this the point at which Firefox exposes the zip code and country? I.e. in
> > response to user interaction on the payment sheet?
> 
> Yes. An event is fired at this point. 

It appears that a shippingaddresschange event is currently fired immediately when the payment sheet opens before any user interaction on the sheet.

I suggest that when the site requests a payment, we pop up the sheet without any address selected, without dispatching an event and disable the "Pay" button. Then we could fire the event and enable the Pay button only once the user chooses an address.

Would that make sense?

This would have the inconvenience of the user having to make a choice from a menu of shipping addresses (potentially containing only one address) even on legitimate sites. However, as it stands, even with just added redaction as requested in this bug, we'd still expose the zip and country without any user interaction on the payment sheet, which would be bad and inconsistent with our geolocation permission UI.

(In reply to Marcos Caceres [:marcosc] from comment #7)
> This is probably a better preview:
> https://pr-preview.s3.amazonaws.com/w3c/payment-request/pull/654.
> html#physical-addresses
> 
> There are too many changes for the diff view.

Looks like this still leaves zip code redaction as an exercise for the implementor, which is bad both for privacy and for interop.

Comment 9

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #8)
> It appears that a shippingaddresschange event is currently fired immediately
> when the payment sheet opens before any user interaction on the sheet.
> 
> I suggest that when the site requests a payment, we pop up the sheet without
> any address selected, without dispatching an event and disable the "Pay"
> button. Then we could fire the event and enable the Pay button only once the
> user chooses an address.
> 
> Would that make sense?

Yes, that makes sense. 

> This would have the inconvenience of the user having to make a choice from a
> menu of shipping addresses (potentially containing only one address) even on
> legitimate sites. However, as it stands, even with just added redaction as
> requested in this bug, we'd still expose the zip and country without any
> user interaction on the payment sheet, which would be bad and inconsistent
> with our geolocation permission UI.

From a technical standpoint, I agree. But this might be up to the UX folks? For example, in the UI, a user could check a box to "always use this address by default". One can make a case on privacy grounds that that might not be a good idea - we should definitely discuss this further with the folks doing the design and user studies for the UX/UI.


> (In reply to Marcos Caceres [:marcosc] from comment #7)
> > This is probably a better preview:
> > https://pr-preview.s3.amazonaws.com/w3c/payment-request/pull/654.
> > html#physical-addresses
> > 
> > There are too many changes for the diff view.
> 
> Looks like this still leaves zip code redaction as an exercise for the
> implementor, which is bad both for privacy and for interop.

Unfortunately, we can't force other browsers to change their behavior irrespective of what is in the spec. As I explained above, other browsers take radically different world views about this - and that's reflected in how Safari implements the this VS how Chrome implements this VS how we implement this.  

From a spec perspective, best I can do during the CR Phase is document the current situation - and then seek to align the platform once we gain implementation and real-world experience. We talk to the folks from other browsers a lot, as well as some merchants, so they will let us know if there is a concern, and we can continue to voice interop and privacy concerns.  

Nevertheless, we should implement what we believe to be best for our users and hope others will follow.

Does that work? Or should we try to take a harder position here?

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #9)
> > This would have the inconvenience of the user having to make a choice from a
> > menu of shipping addresses (potentially containing only one address) even on
> > legitimate sites. However, as it stands, even with just added redaction as
> > requested in this bug, we'd still expose the zip and country without any
> > user interaction on the payment sheet, which would be bad and inconsistent
> > with our geolocation permission UI.
> 
> From a technical standpoint, I agree. But this might be up to the UX folks?

Not only the UX folks but the privacy folks, too.

> For example, in the UI, a user could check a box to "always use this address
> by default". One can make a case on privacy grounds that that might not be a
> good idea - we should definitely discuss this further with the folks doing
> the design and user studies for the UX/UI.

Yes, and people who oversee privacy matters should be included.

> > (In reply to Marcos Caceres [:marcosc] from comment #7)
> > > This is probably a better preview:
> > > https://pr-preview.s3.amazonaws.com/w3c/payment-request/pull/654.
> > > html#physical-addresses
> > > 
> > > There are too many changes for the diff view.
> > 
> > Looks like this still leaves zip code redaction as an exercise for the
> > implementor, which is bad both for privacy and for interop.
> 
> Unfortunately, we can't force other browsers to change their behavior
> irrespective of what is in the spec. As I explained above, other browsers
> take radically different world views about this - and that's reflected in
> how Safari implements the this VS how Chrome implements this VS how we
> implement this.  
> 
> From a spec perspective, best I can do during the CR Phase is document the
> current situation - and then seek to align the platform once we gain
> implementation and real-world experience. We talk to the folks from other
> browsers a lot, as well as some merchants, so they will let us know if there
> is a concern, and we can continue to voice interop and privacy concerns.  
> 
> Nevertheless, we should implement what we believe to be best for our users
> and hope others will follow.
> 
> Does that work? Or should we try to take a harder position here?

How would an implementor figure out how to do partial zip code redaction when needed on a world-wide basis? (The answer to this should probably go in the spec.) Does CLDR have data for this?

Comment 11

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #10)
> Yes, and people who oversee privacy matters should be included.

Sure. Can you help us get this on front of the right people? I don't know exactly who that would be inside Moz.

> > Does that work? Or should we try to take a harder position here?
> 
> How would an implementor figure out how to do partial zip code redaction
> when needed on a world-wide basis? (The answer to this should probably go in
> the spec.) Does CLDR have data for this?

Oh wait, sorry... the *partial* zip redaction is not part of the spec. There is no standard for that that I know of... that's very Apple specific.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #11)
> 
> (In reply to Henri Sivonen (:hsivonen) from comment #10)
> > Yes, and people who oversee privacy matters should be included.
> 
> Sure. Can you help us get this on front of the right people? I don't know
> exactly who that would be inside Moz.

I'll try to find out.

> Oh wait, sorry... the *partial* zip redaction is not part of the spec. There
> is no standard for that that I know of... that's very Apple specific.

That's ... not ideal. :-(

Here's a more useful modification of the Chrome demo:
https://hsivonen.com/test/chromesamples/paymentrequest.html

While implementing redaction, I noticed that by spec, when redaction is not in use, when the request promise resolves, both the request and the response have the full address, but when redaction is in use, when the request promise resolves, only the response has the full address and the redacted address remains in the request. This seems like a footgun: Web developers who test with a browser that doesn't redact might, at the time of the request promise resolving, mistakenly pull the shipping address from the request object instead of the response object and it would work in browsers that don't redact and break in browsers that do redact.

I'm inclined to file a spec issue requesting that the spec be changed to swap the full address into the request object without firing a shippingaddresschange event right before resolving the promise. Is there a reason not to file such spec issue?

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1435155 - Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully.

Review commit: https://reviewboard.mozilla.org/r/225692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225692/

Comment 14

[Henri Sivonen (:hsivonen)]

Filed bug 1443735

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956732 [details]
Bug 1435155 - Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225692/diff/1-2/

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8956732 [details]
Bug 1435155 - Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully.

https://reviewboard.mozilla.org/r/225692/#review231606

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a29a154e88d
Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully. r=baku

Comment 18

[Andreea Pavel [:apavel]]

Backed out for browser chrome leaks

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8a29a154e88dc58d0ffde543a2159d70638ce53c

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8a29a154e88dc58d0ffde543a2159d70638ce53c

Backout: https://hg.mozilla.org/integration/autoland/rev/e1ae4f79fd6c2acf2c92233edb62fbfbf97ca30e

Comment 19

[Henri Sivonen (:hsivonen)]

I don't understand how the changes here could have added a leak. The new owning reference for the full address is written in exactly the same way (RefPtr) as the existing owning address reference.

Considering that the consistent leak was in the "stylo disabled" configuration and didn't happen in the "stylo enabled" configuration, wouldn't it be more likely that something in the old style system leaked instead?

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This started with the push of those patches and got fixed by the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&group_state=expanded&tochange=e1ae4f79fd6c2acf2c92233edb62fbfbf97ca30e&fromchange=bdffa40b603a94537b544b4ead70f9c7e59dc9c7&filter-searchStr=linux%20stylo%20debug%20browser-chrome

The failing chunks contain the payments test.

Emilio, any idea why this only leaks in non-stylo builds?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Off-hand there's nothing in the patch that could look style-system dependent. Looks like a whole document is getting leaked. Maybe it's a bug with all the XBL stuff in the old style system? Though their handling isn't that much different to ours, and it's unlikely that this bug could've triggered.

Looking a bit closer, looks like the patch doesn't include the new PaymentRequest member in the cc traversal, like mShippingAddress is:

  https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/payments/PaymentRequest.cpp#33

That can probably cause the window reference that PaymentAddress holds to leak.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Now why that happened only with stylo-disabled, I wish I knew :).

Probably they tickle the CC in a different way.

Comment 23

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> > Oh wait, sorry... the *partial* zip redaction is not part of the spec. There
> > is no standard for that that I know of... that's very Apple specific.
> 
> That's ... not ideal. :-(
> 
> Here's a more useful modification of the Chrome demo:
> https://hsivonen.com/test/chromesamples/paymentrequest.html
> 
> While implementing redaction, I noticed that by spec, when redaction is not
> in use, when the request promise resolves, both the request and the response
> have the full address, but when redaction is in use, when the request
> promise resolves, only the response has the full address and the redacted
> address remains in the request. This seems like a footgun: Web developers
> who test with a browser that doesn't redact might, at the time of the
> request promise resolving, mistakenly pull the shipping address from the
> request object instead of the response object and it would work in browsers
> that don't redact and break in browsers that do redact.

Good point. 

> I'm inclined to file a spec issue requesting that the spec be changed to
> swap the full address into the request object without firing a
> shippingaddresschange event right before resolving the promise. Is there a
> reason not to file such spec issue?

We should fix this as part of https://github.com/w3c/payment-request/pull/654 - I'll add some prose and get you to review it.

Comment 24

[Henri Sivonen (:hsivonen)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> Looking a bit closer, looks like the patch doesn't include the new
> PaymentRequest member in the cc traversal, like mShippingAddress is:

Good catch. Thanks!

Now that this got backed away anyway, I'm going to bake in the implementation of https://github.com/w3c/payment-request/issues/692 in the revised patch.

Comment 25

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956732 [details]
Bug 1435155 - Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225692/diff/2-3/

Comment 26

[Henri Sivonen (:hsivonen)]

baku, does the r+ hold for the revised patch? (I don't know how to re-request review on ReviewBoard.)

Comment 27

[Andrea Marchesini [:baku]]

Yes.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbc6dec319be
Redact the shipping address as it is visible before the promise from PaymentRequest.show() resolves successfully. r=baku

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/fbc6dec319be