757226 - Implement mozApps app.replaceReceipt()

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P2)

Description

[Ian Bicking (:ianbicking)]

We should implement a replaceReceipt() method as described here:

https://groups.google.com/d/msg/mozilla.dev.webapps/_9OjB23au6Q/RDJE0K25-Y8J

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

we need this API so that we can support the expiring receipts described in Appendix D of this specification:

https://wiki.mozilla.org/Apps/WebApplicationReceipt/GenerationService

This blocks k9o because it is critical to how we cope with compromised signing keys at our marketplace

Comment 2

[Jason Smith [:jsmith]]

FYI to Myk - Bill wants this to block the desktop web runtime release. Thoughts?

Comment 3

[Jason Smith [:jsmith]]

I'm questioning now if we should block the desktop release on this, so I'm removing the priority and blocking flag. We've got way too much on our plate for a 1st release of the web runtime as is. Is this really needed for a first release?

This needs to be re-evaluated in our triage meeting.

Comment 4

[Ian Bicking (:ianbicking)]

Reinstallation also serves as a fallback for replaceReceipt(); replaceReceipt should be more graceful, but without it we won't be entirely incapable of managing receipt updates and changes.  As such I don't think it's the highest priority.

Comment 5

[Andy McKay]

I added notes on how this could work with the reissue URL:

https://wiki.mozilla.org/Apps/WebApplicationReceiptRefresh

Comment 6

[Wil Clouser [:clouserw]]

The certs which sign receipts currently expire every 2 weeks.  Without being able to replace receipts using the new certs all receipts become invalid every 2 weeks.

Comment 7

[Jason Smith [:jsmith]]

Bill - We need input on what to do here.

Comment 8

[Anant Narayanan [:anant]]

Ian, do you have some details on how this bug relates to bug 745319? If we allow the user-agent to automatically refresh receipts when they are expired (which is what 745319 is about), do we also need a separate replaceReceipt method to let the marketplace reissue them?

Comment 9

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Andreas Gal :gal from comment #9)
> Renom if you think we can't ship a v1 without this.

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 11

[Jason Smith [:jsmith]]

Bill's indicated that this is a nice to have, not necessary to ship if bug 745319 is completed. Removing the nom as a result.

Comment 12

[Andy McKay]

Attachment: naive start

My naive start attached. I was trying to figure out how to write tests for this next, but someone will be able to do it way faster than me I'm sure.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Andy McKay [:andym] from comment #12)
> Created attachment 753501 [details] [diff] [review]
> naive start
> 
> My naive start attached. I was trying to figure out how to write tests for
> this next, but someone will be able to do it way faster than me I'm sure.

You should be able to use the existing mochitest chrome tests in dom/apps/tests/mochitest to test this. Or you could take advantage of some of the newer work in dom/apps/tests that disable the install prompt when installing an app.

Comment 14

[Travis Choma]

Based on recent conversations, this seems like an important bug to fix, firstly from a security point of view. Speaking with Raymond and Guillaume I confirmed that the original intent of expiring receipts was to protect against the case where our signing key is compromised (also referenced earlier here in appendix C https://wiki.mozilla.org/Apps/WebApplicationReceipt/GenerationService). 

Right now, as I understand it, if apps were to respect receipt expiration and receive a new receipt from the MP/verifier they would have to trigger a reinstall of the app and this involves a user prompt. Adding a way to replace receipts programmatically would allow this situation to be handled behind the scenes without disrupting the user with an action they will likely not understand.

Beyond security this functionality can also lay the foundation for supporting new payment flows like subscriptions and trial offers.

I had a discussion with Fabrice, and he had some feedback on the api signatures, rather than a replaceReceipts call which relies on differing behaviors based on null parameters, it'd be cleaner to have an addReceipt, removeReceipt and replaceReceipt call.

Adding Jonas and Marcos for some additional feedback from a WebAPI point of view, and Raymond and Guillaume from a security point of view.

Let me know your thoughts. If there is consensus, I suppose the way forward is to revise this patch, add test coverage and have it reviewed.

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch

This is for the proposed replaceReceipt with two arguments. It would be simple to modify the patch to have three different functions though.

Comment 16

[Marco Castelluccio [:marco]]

Comment on attachment 781200 [details] [diff] [review]
Patch

This was based on the earlier patch, but turns out the earlier patch wasn't complete.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 781230 [details] [diff] [review]
Patch

Mounir mentions Marcos is on PTO for 4 weeks. Going to redirect the feedback request to Mounir, as we're looking for API feedback from a W3C spec owner perspective.

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 781230 [details] [diff] [review]
Patch

Review of attachment 781230 [details] [diff] [review]:
-----------------------------------------------------------------

Neither Marcos or me have been involved in App Payments and receipt. SysApps isn't involved with that either.

I guess Jonas might be a better person to get feedback from.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 781230 [details] [diff] [review]
Patch

Review of attachment 781230 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! I especially like that you have to pass in the old receipt. That ensures that callers that doesn't have access to old receipts can't install new ones. We should just make sure that the check lives in the parent process so that if a child process gets hacked that you can't access or replaces receipts.

Comment 21

[Marco Castelluccio [:marco]]

(In reply to Jonas Sicking (:sicking) from comment #20)
> This looks great! I especially like that you have to pass in the old
> receipt. That ensures that callers that doesn't have access to old receipts
> can't install new ones. We should just make sure that the check lives in the
> parent process so that if a child process gets hacked that you can't access
> or replaces receipts.

Actually at the moment the receipts are accessible by a child process (http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/nsIDOMApplicationRegistry.idl#11). So a child process could replace receipts even if the check was in the parent process (because it knows the old receipts).

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That sounds like we need to fix. Separate bug though.

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 786550 [details] [diff] [review]
Patch v2

Review of attachment 786550 [details] [diff] [review]:
-----------------------------------------------------------------

As I said I'd rather have explicit calls for addReceipt(), removeReceipt() and replaceReceipt() than this one-size-fits-all api. Let's get consensus on that first.

Comment 25

[Andy McKay]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> As I said I'd rather have explicit calls for addReceipt(), removeReceipt()
> and replaceReceipt() than this one-size-fits-all api. Let's get consensus on
> that first.

The explicit calls feels nicer and self documenting, but I have no huge opinion either way.

Thanks for getting this going marco.

Comment 26

[Andy McKay]

Bump. If we could keep this moving it would be appreciated, the number of things it blocks grows regularly.

Comment 27

[Kumar McMillan [:kumar]]

+1 for addReceipt(), removeReceipt() and replaceReceipt(). One thing we may want to do is to call addReceipt() for in-app purchases which would be separate from app receipts. In that case we wouldn't be doing replaceReceipt().

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Marco, are you actively working on this issue? It seems that it is blocking the in-app payments work. I can take it if you can't work on this. Thanks!

Comment 29

[Marco Castelluccio [:marco]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #28)
> Marco, are you actively working on this issue? It seems that it is blocking
> the in-app payments work. I can take it if you can't work on this. Thanks!

I was waiting for a decision about the API (a single replaceReceipt or separate addReceipt, removeReceipt and replaceReceipt). I prefer the second solution and, given most people here agree, I'm going to go with it.

Comment 30

[Marco Castelluccio [:marco]]

Attachment: Patch

Bugzilla is live again, here's the new patch.

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8358866 [details] [diff] [review]
Patch

Review of attachment 8358866 [details] [diff] [review]:
-----------------------------------------------------------------

mostly nits in the current patches, but I'm also worried about the possible abuse of this api: in its current form this lets apps store arbitrary data bypassing all control mechanisms. Could we add list verify that the data stored is actually a receipt, and maybe put a reasonable upper limit to the number of receipts that can be stored for an app?

::: dom/apps/src/Webapps.jsm
@@ +3576,5 @@
> +
> +    let id = this._appIdForManifestURL(aData.manifestURL);
> +    let app = this.webapps[id];
> +
> +    app.receipts.push(receipt);

what if app.receipts is null?

@@ +3581,5 @@
> +
> +    this._saveApps(function() {
> +      aData.receipts = app.receipts;
> +      aMm.sendAsyncMessage("Webapps:AddReceipt:Return:OK", aData);
> +    }.bind(this));

no need to bind(this)

@@ +3598,5 @@
> +
> +    let id = this._appIdForManifestURL(aData.manifestURL);
> +    let app = this.webapps[id];
> +
> +    let index = app.receipts.indexOf(receipt);

guard against a null app.receipts

@@ +3600,5 @@
> +    let app = this.webapps[id];
> +
> +    let index = app.receipts.indexOf(receipt);
> +    if (index == -1) {
> +      aData.error = "RECEIPT_NON_EXISTENT";

nit: I would prefer NO_SUCH_RECEIPT but no big deal

@@ +3610,5 @@
> +
> +    this._saveApps(function() {
> +      aData.receipts = app.receipts;
> +      aMm.sendAsyncMessage("Webapps:RemoveReceipt:Return:OK", aData);
> +    }.bind(this));

no need to bind

@@ +3628,5 @@
> +
> +    let id = this._appIdForManifestURL(aData.manifestURL);
> +    let app = this.webapps[id];
> +
> +    let oldIndex = app.receipts.indexOf(oldReceipt);

guard against a null app.receipts

@@ +3640,5 @@
> +
> +    this._saveApps(function() {
> +      aData.receipts = app.receipts;
> +      aMm.sendAsyncMessage("Webapps:ReplaceReceipt:Return:OK", aData);
> +    }.bind(this));

no need to bind

::: dom/apps/tests/test_receipt_operations.html
@@ +33,5 @@
> +}
> +
> +function continueTest() {
> +  try { gGenerator.next(); }
> +  catch (e) { dump("Got exception: " + e + "\n"); }

nit: indentation is weird

@@ +37,5 @@
> +  catch (e) { dump("Got exception: " + e + "\n"); }
> +}
> +
> +function finish() {
> +  SpecialPowers.removePermission("webapps-manage", document);

I think you don't need that when you use pushPermissions

Comment 32

[Marco Castelluccio [:marco]]

Attachment: Patch v2

I've added a new function (isReceipt) that does some basic checks on the receipt data.
We could do a more elaborate verification, but maybe that belongs to another bug.

Comment 33

[Marco Castelluccio [:marco]]

About the limit, I don't know, what would be a reasonable limit?

Comment 34

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #33)
> About the limit, I don't know, what would be a reasonable limit?

Not sure. If that includes in-app payments we need something reasonably large (a few hundred?)

Comment 35

[Kumar McMillan [:kumar]]

A few hundred receipts *per app* should be enough to cover in-app purchases.

Comment 36

[Kumar McMillan [:kumar]]

Comment on attachment 8359491 [details] [diff] [review]
Patch v2

Review of attachment 8359491 [details] [diff] [review]:
-----------------------------------------------------------------

[drive-by]

::: dom/apps/src/Webapps.jsm
@@ +3565,5 @@
>  
> +  /* Check if |data| is actually a receipt */
> +  isReceipt: function(data) {
> +    try {
> +      let segments = data.split('.');

I think you need to try splitting on tilde (~) first, right? https://wiki.mozilla.org/Apps/WebApplicationReceipt#Firefox_Marketplace_receipts

@@ +3617,5 @@
> +
> +    if (!app.receipts) {
> +      app.receipts = [];
> +    }
> +    app.receipts.push(receipt);

What if an app developer accidentally calls addReceipt(receipt) with a receipt that already exists? Will replaceReceipt() and removeReceipt() still work after that?

::: dom/apps/tests/test_receipt_operations.html
@@ +127,5 @@
> +     "reissue": "http://mochi.test:8888/reissue/5169314356"
> +    }
> +  */
> +  let receipt3 = 'eyJhbGciOiAiSFMyNTYiLCAidHlwIjogIkpXVCJ9.eyJwcm9kdWN0IjogeyJ1cmwiOiAiaHR0cHM6Ly93d3cubW96aWxsYS5vcmciLCAic3RvcmVkYXRhIjogIjUxNjkzMTQzNTgifSwgInJlaXNzdWUiOiAiaHR0cDovL21vY2hpLnRlc3Q6ODg4OC9yZWlzc3VlLzUxNjkzMTQzNTYiLCAidXNlciI6IHsidHlwZSI6ICJkaXJlY3RlZC1pZGVudGlmaWVyIiwgInZhbHVlIjogIjRmYjM1MTUxLTJiOWItNGJhMi04MjgzLWM0OWQzODE2NDBiZCJ9LCAidmVyaWZ5IjogImh0dHA6Ly9tb2NoaS50ZXN0Ojg4ODgvdmVyaWZ5LzUxNjkzMTQzNTYiLCAiaXNzIjogImh0dHA6Ly9tb2NoaS50ZXN0Ojg4ODgiLCAiaWF0IjogMTMxMzYwMTg4LCAidHlwIjogInB1cmNoYXNlLXJlY2VpcHQiLCAibmJmIjogMTMxMzYwMTg1LCAiZGV0YWlsIjogImh0dHA6Ly9tb2NoaS50ZXN0Ojg4ODgvcmVjZWlwdC81MTY5MzE0MzU2In0.7_BBVdPxEHK2Lk3StqvRIKtmZTmU_XM0nibrT3S0rfs';
> +

It would be good to have a test for Marketplace receipts which are separated by tilde. Here's an example of a valid one:

eyJhbGciOiAiUlMyNTYiLCAidHlwIjogIkpXVCIsICJqa3UiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5jZG4ubW96aWxsYS5uZXQvcHVibGljX2tleXMvbWFya2V0cGxhY2Utcm9vdC1wdWIta2V5Lmp3ayJ9.eyJpc3MiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5jZG4ubW96aWxsYS5uZXQvcHVibGljX2tleXMvbWFya2V0cGxhY2Utcm9vdC1wdWIta2V5Lmp3ayIsICJwcmljZV9saW1pdCI6IDEwMCwgImp3ayI6IFt7ImFsZyI6ICJSU0EiLCAibW9kIjogIkFMYkszek5VQ0lFTEJRZ1QycGUzTEkwdC1sR0w5OElFTnBWOUtuX0F4VGxjLXZzX0ZFMlVyNzU2Z012bHA3a3BWVmFEWVNCdnVCQjgtZEZpU3VJbHdCUFB2bWFIaTFhd0xJMjRRY2JOMVJrN3pZS01SclVfSzdkVEN6MEh6VHoza01YVXp1ci1ySTIxS3BKb0NSZFNxeUl4bHpnUWFna1dUUWxIYUI2VzkzUjBacUxlQk9lUzhjbzNOUlczdjFfY0h4VTE1d0k4T0JHY0tRSXB3VHpONUVfRFdNZ0F1MGFQMHlWY3EzT0FwXy1fa1pjYXBtQnpSTmVMOHBxMjZXN01jMUpJZVBnZVZ5SXExcFBLMU9ldGhmdF9KeTk5R19EWWxQNW15YjFEY1VpbHE3RVNKc1UyeUZPUjJhWmkyYU1lTkRZekwyUmdZSGt2RWxyNDRMM2NZM0UiLCAiZXhwIjogIkFRQUIiLCAia2lkIjogImFwcHN0b3JlLm1vemlsbGEuY29tLTIwMTMtMTEtMjcifV0sICJleHAiOiAxMzg2Nzg4NDAxLCAiaWF0IjogMTM4NTU3ODgwMSwgInR5cCI6ICJjZXJ0aWZpZWQta2V5IiwgIm5iZiI6IDEzODU1Nzg4MDF9.Ne5AffwNIjbQmwY_dSKVXR0R0wdB92sW_BWQWbN2WKa_Ep6V0Fwr2pfcv0KenZcYKdxhhSPBrs5R38EcIqTYYrgIeeJyM_gGzv-ESsUsqbFejAbVH2xfwATZ1lXNPh0VSt33Drf2RY5jeU5PD3usXgOPr8RYAGkMxz_0SUay5WCBVRLkrgtrCUNyIKBwuHlxKK1JkncVXsN0mr_gwbm0EpBgIOEZQj75TE0KcviMUvYn8uhVYEwYMLzMQmUbI5quxH2z5mcK2DDNQGgT6ABJljKWCY-PPuMo9tsgXe6L7MTafulBuSIjs1ztAl4ZnwZjKmxWmhdeiaT41tCFlr4K8Q~eyJqa3UiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5jZG4ubW96aWxsYS5uZXQvcHVibGljX2tleXMvbWFya2V0cGxhY2Utcm9vdC1wdWIta2V5Lmp3ayIsICJ0eXAiOiAiSldUIiwgImFsZyI6ICJSUzI1NiJ9.eyJwcm9kdWN0IjogeyJ1cmwiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5maXJlZm94LmNvbSIsICJzdG9yZWRhdGEiOiAiaWQ9NDM4OTc4In0sICJpc3MiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5maXJlZm94LmNvbSIsICJ2ZXJpZnkiOiAiaHR0cHM6Ly9yZWNlaXB0Y2hlY2subWFya2V0cGxhY2UuZmlyZWZveC5jb20vdmVyaWZ5LyIsICJkZXRhaWwiOiAiaHR0cHM6Ly9tYXJrZXRwbGFjZS5maXJlZm94LmNvbS9hcGkvdjEvcmVjZWlwdHMvcmVpc3N1ZS8iLCAicmVpc3N1ZSI6ICJodHRwczovL21hcmtldHBsYWNlLmZpcmVmb3guY29tL2FwaS92MS9yZWNlaXB0cy9yZWlzc3VlLyIsICJ1c2VyIjogeyJ0eXBlIjogImRpcmVjdGVkLWlkZW50aWZpZXIiLCAidmFsdWUiOiAiMTkzMzI2LTVjMTUzNmQ1LWUxMDQtNDAzYy04NDBlLTQ5YjMyMmQ5Yjg4NSJ9LCAiZXhwIjogMTQwMTgyNTEyOCwgImlhdCI6IDEzODYxMDAzMjgsICJ0eXAiOiAicHVyY2hhc2UtcmVjZWlwdCIsICJuYmYiOiAxMzg2MTAwMzI4fQ.r2DVUpouRDJYqZe61LJBcIwmeF2mI8FmbGMRlfNFcinKAIs8nMVVNX8xSWJ6jXXgZ62VfHJCLHapADX8rCg6NgxFV_FdP7j2H_2Ufo0E0TREifTN6V4v1dCnzDulNhZmO8G-nQJUVOAtNfNC95PY7tVa8WC7dYXnKZsD6NhIxxVEtBGuiiySpWArI-g3pcl41rXNHHpJbRfrOD4QgVNrsV83TWILYRr6PWr3aqOM2XT_x2SzEfhBNvdG8AJmR0MKQytvfcgz3Vt1hMak88nFrzTLiKkuuPAXpwB5q83LZIl4EYG3UAnte4-XWlLb-NJ78vgXa64myy-3fPr7EO6LaQ

Comment 37

[Kumar McMillan [:kumar]]

Comment on attachment 8359491 [details] [diff] [review]
Patch v2

Review of attachment 8359491 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/Webapps.jsm
@@ +3570,5 @@
> +      if (segments.length != 3) {
> +        return false;
> +      }
> +
> +      let decodedReceipt = JSON.parse(decodeURIComponent(escape(atob(segments[1]))));

In theory, JWTs can be URL-safe base64 encoded, per http://tools.ietf.org/rfc/rfc4648.txt

atob(urlSafeBase64Value) will fail here (I think?) so you need to account for it. You could do:

segments[1] = segments[1].replace("-", "+", "g").replace("_", "/", "g");
let payload = atob(segments[1]);

... or something along those lines.

This is how it was fixed in payments: http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#279

@@ +3580,5 @@
> +      if (!decodedReceipt.typ ||
> +          !decodedReceipt.product || !decodedReceipt.product.url ||
> +          !decodedReceipt.product.storedata ||
> +          !decodedReceipt.user.type || !decodedReceipt.user.value ||
> +          !decodedReceipt.iss || !decodedReceipt.nbf || !decodedReceipt.iat) {

I'm pretty sure product.url and product.storedata are not required but I could be wrong.

:andym would know

@@ +3581,5 @@
> +          !decodedReceipt.product || !decodedReceipt.product.url ||
> +          !decodedReceipt.product.storedata ||
> +          !decodedReceipt.user.type || !decodedReceipt.user.value ||
> +          !decodedReceipt.iss || !decodedReceipt.nbf || !decodedReceipt.iat) {
> +        return false;

because there are so many falsey conditions in this check, could we bubble up specific errors to developers like MISSING_RECEIPT_FIELD here or CANNOT_DECODE_RECEIPT up above, etc?

@@ +3585,5 @@
> +        return false;
> +      }
> +
> +      let allowedTypes = [ "purchase-receipt", "developer-receipt", "reviewer-receipt",
> +                           "test-receipt" ];

we'll be expanding these types when we add in-app purchase receipts. Again, it would be nice to show developers something like INVALID_RECEIPT_TYPE in case we forget to fix it in the platform ;)

Comment 38

[Andy McKay]

(In reply to Kumar McMillan [:kumar] from comment #37)
> Comment on attachment 8359491 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8359491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +3570,5 @@
> > +      if (segments.length != 3) {
> > +        return false;
> > +      }
> > +
> > +      let decodedReceipt = JSON.parse(decodeURIComponent(escape(atob(segments[1]))));
> 
> In theory, JWTs can be URL-safe base64 encoded, per
> http://tools.ietf.org/rfc/rfc4648.txt
> 
> atob(urlSafeBase64Value) will fail here (I think?) so you need to account
> for it. You could do:
> 
> segments[1] = segments[1].replace("-", "+", "g").replace("_", "/", "g");
> let payload = atob(segments[1]);
> 
> ... or something along those lines.
> 
> This is how it was fixed in payments:
> http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#279
> 
> @@ +3580,5 @@
> > +      if (!decodedReceipt.typ ||
> > +          !decodedReceipt.product || !decodedReceipt.product.url ||
> > +          !decodedReceipt.product.storedata ||
> > +          !decodedReceipt.user.type || !decodedReceipt.user.value ||
> > +          !decodedReceipt.iss || !decodedReceipt.nbf || !decodedReceipt.iat) {
> 
> I'm pretty sure product.url and product.storedata are not required but I
> could be wrong.

The spec isn't especially well written and has been a bit loosey goosey on these things. I think we should probably err on the side of not checking too much until we can nail this down. I think checking that we can find that typ, product, usr, iss, nbf and iat exist is enough.

We aren't do receipt verification here, just checking that the receipt that comes in looks kinda like a receipt and isn't dodgy. Eventually I would love to move the receipt verification down into the platform and then we can add in isReceiptVerified() and that would do more of these checks.

But I think to do that we need to make it clearer what the spec is, socialise it and get it accepted somewhere written as a formal spec. Oh and give our receipts a version number for the format.

> @@ +3581,5 @@
> > +          !decodedReceipt.product || !decodedReceipt.product.url ||
> > +          !decodedReceipt.product.storedata ||
> > +          !decodedReceipt.user.type || !decodedReceipt.user.value ||
> > +          !decodedReceipt.iss || !decodedReceipt.nbf || !decodedReceipt.iat) {
> > +        return false;
> 
> because there are so many falsey conditions in this check, could we bubble
> up specific errors to developers like MISSING_RECEIPT_FIELD here or
> CANNOT_DECODE_RECEIPT up above, etc?

+1 

> @@ +3585,5 @@
> > +        return false;
> > +      }
> > +
> > +      let allowedTypes = [ "purchase-receipt", "developer-receipt", "reviewer-receipt",
> > +                           "test-receipt" ];
> 
> we'll be expanding these types when we add in-app purchase receipts. Again,
> it would be nice to show developers something like INVALID_RECEIPT_TYPE in
> case we forget to fix it in the platform ;)

I don't think we want to do in-app purchase receipts as a type. I'm tempted to put that in product store-data. I think this will be fine.

One extra check on this should be to check the size of the incoming data, in case someone tries to send in a 10MB blob of something naughty.

Comment 39

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Another version of the patch that should address kumar's and andym's comments.

Comment 40

[Andy McKay]

Comment on attachment 8360125 [details] [diff] [review]
Patch v3

Review of attachment 8360125 [details] [diff] [review]:
-----------------------------------------------------------------

The checking looks good from me, I know there's others who want to review though.

Comment 41

[Kumar McMillan [:kumar]]

Comment on attachment 8360125 [details] [diff] [review]
Patch v3

Review of attachment 8360125 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc from me. Thanks!

::: dom/apps/src/Webapps.jsm
@@ +3660,5 @@
> +      aMm.sendAsyncMessage("Webapps:AddReceipt:Return:KO", aData);
> +      return;
> +    }
> +
> +    if (!app.receipts[receipt])

is this if statement still needed?

Comment 42

[Marco Castelluccio [:marco]]

Attachment: Patch v3

(In reply to Kumar McMillan [:kumar] from comment #41)
> is this if statement still needed?

It was a leftover, I forgot to remove it.

Comment 43

[[:fabrice] Fabrice Desré]

Comment on attachment 8360715 [details] [diff] [review]
Patch v3

Review of attachment 8360715 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed.

::: dom/apps/src/Webapps.jsm
@@ +3566,5 @@
> +  /* Check if |data| is actually a receipt */
> +  isReceipt: function(data) {
> +    try {
> +      // The receipt data shouldn't be too big (allow up to 1 MB of data)
> +      if (data.length > 1000000) {

make that a constant. Also, it's no really 1MB ;)

@@ +3567,5 @@
> +  isReceipt: function(data) {
> +    try {
> +      // The receipt data shouldn't be too big (allow up to 1 MB of data)
> +      if (data.length > 1000000) {
> +        return "TOO_BIG_RECEIPT";

s/TOO_BIG_RECEIPT/RECEIPT_TOO_BIG

Comment 44

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Fabrice Desré [:fabrice] from comment #43) 
> ::: dom/apps/src/Webapps.jsm
> @@ +3566,5 @@
> > +  /* Check if |data| is actually a receipt */
> > +  isReceipt: function(data) {
> > +    try {
> > +      // The receipt data shouldn't be too big (allow up to 1 MB of data)
> > +      if (data.length > 1000000) {
> 
> make that a constant. Also, it's no really 1MB ;)

The standards now state that the SI prefixes (kilo, mega, giga, etc.) shouldn't be used to indicate powers of 2, but only powers of 10. So 1 MB is actually 1000000 :P
Anyway I've changed the value to 1048576 (1 MiB) to avoid confusion.

Carrying r+.

Comment 45

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/f9f2a9547858

Comment 46

[Carsten Book [:Tomcat]]

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

Comment 47

[Kumar McMillan [:kumar]]

Sorry for the comment noise, I just wanted to say THANK YOU MARCO :) You unblocked 19 bugs https://bugzilla.mozilla.org/showdependencytree.cgi?id=757226&hide_resolved=1

Comment 48

[Kumar McMillan [:kumar]]

Hey Chris, are there any plans to get this documented soon? I don't see the new methods listed here yet: https://developer.mozilla.org/en-US/docs/Web/API/App

Comment 49

[Chris Mills [:cmills]]

Removing need'info, as this is irrelevant at this point. 4 years too late ;-)