Closed Bug 757226 Opened 8 years ago Closed 6 years ago

Implement mozApps app.replaceReceipt()

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: ianbicking, Assigned: marco)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [qa-])

Attachments

(1 file, 8 obsolete files)

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

https://groups.google.com/d/msg/mozilla.dev.webapps/_9OjB23au6Q/RDJE0K25-Y8J
Depends on: 757225
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
blocking-kilimanjaro: --- → ?
Whiteboard: [blocking-webrtdesktop1]
Whiteboard: [blocking-webrtdesktop1] → [blocking-webrtdesktop1+]
FYI to Myk - Bill wants this to block the desktop web runtime release. Thoughts?
Priority: -- → P1
Blocks: 746465
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.
Priority: P1 → --
Whiteboard: [blocking-webrtdesktop1+]
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.
I added notes on how this could work with the reissue URL:

https://wiki.mozilla.org/Apps/WebApplicationReceiptRefresh
blocking-kilimanjaro: ? → +
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.
blocking-basecamp: --- → ?
Blocks: 769793
blocking-basecamp: ? → +
No longer blocks: 746465
Component: DOM: Mozilla Extensions → DOM: Apps
Bill - We need input on what to do here.
Whiteboard: [blocked-on-input]
Blocks: 745319
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?
Priority: -- → P2
Renom if you think we can't ship a v1 without this.
blocking-basecamp: + → ---
(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
blocking-basecamp: --- → ?
Whiteboard: [blocked-on-input] → [blocked-on-input Bill Walker]
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.
blocking-basecamp: ? → ---
Whiteboard: [blocked-on-input Bill Walker]
Blocks: 855912
Assignee: nobody → amckay
Keywords: dev-doc-needed
Depends on: 874646
Assignee: amckay → nobody
No longer depends on: 874646
Attached patch naive start (obsolete) — Splinter Review
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.
Attachment #753501 - Attachment is patch: true
Attachment #753501 - Attachment mime type: text/x-patch → text/plain
(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.
Blocks: 881788
Blocks: 867662
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.
Attached patch Patch (obsolete) — Splinter Review
This is for the proposed replaceReceipt with two arguments. It would be simple to modify the patch to have three different functions though.
Assignee: nobody → mcastelluccio
Attachment #753501 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 781200 [details] [diff] [review]
Patch

This was based on the earlier patch, but turns out the earlier patch wasn't complete.
Attachment #781200 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #781230 - Flags: feedback?(mcaceres)
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.
Attachment #781230 - Flags: feedback?(mcaceres) → feedback?(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.
Attachment #781230 - Flags: feedback?(mounir) → feedback?(jonas)
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.
Attachment #781230 - Flags: feedback?(jonas) → feedback+
(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).
That sounds like we need to fix. Separate bug though.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #781230 - Attachment is obsolete: true
Attachment #786550 - Flags: review?(fabrice)
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.
Attachment #786550 - Flags: review?(fabrice)
(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.
Blocks: 929745
Blocks: 910270
Blocks: 944480
Blocks: 944523
Bump. If we could keep this moving it would be appreciated, the number of things it blocks grows regularly.
+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().
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!
Flags: needinfo?(mar.castelluccio)
(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.
Flags: needinfo?(mar.castelluccio)
Attached patch Patch (obsolete) — Splinter Review
Bugzilla is live again, here's the new patch.
Attachment #786550 - Attachment is obsolete: true
Attachment #8358866 - Flags: review?(fabrice)
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
Attachment #8358866 - Flags: review?(fabrice) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8358866 - Attachment is obsolete: true
Attachment #8359491 - Flags: review?(fabrice)
About the limit, I don't know, what would be a reasonable limit?
(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?)
Flags: needinfo?(amckay)
A few hundred receipts *per app* should be enough to cover in-app purchases.
Flags: needinfo?(amckay)
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 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 ;)
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
Another version of the patch that should address kumar's and andym's comments.
Attachment #8359491 - Attachment is obsolete: true
Attachment #8359491 - Flags: review?(fabrice)
Attachment #8360125 - Flags: feedback?(kumar.mcmillan)
Attachment #8360125 - Flags: feedback?(amckay)
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.
Attachment #8360125 - Flags: review+
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?
Attachment #8360125 - Flags: feedback?(kumar.mcmillan) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
(In reply to Kumar McMillan [:kumar] from comment #41)
> is this if statement still needed?

It was a leftover, I forgot to remove it.
Attachment #8360125 - Attachment is obsolete: true
Attachment #8360125 - Flags: feedback?(amckay)
Attachment #8360715 - Flags: review?(fabrice)
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
Attachment #8360715 - Flags: review?(fabrice) → review+
Attached patch PatchSplinter Review
(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+.
Attachment #8360715 - Attachment is obsolete: true
Attachment #8361378 - Flags: review+
Keywords: checkin-needed
Blocks: 960837
https://hg.mozilla.org/mozilla-central/rev/f9f2a9547858
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
Whiteboard: [qa-]
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
Flags: needinfo?(cmills)
Product: Core → Core Graveyard
Removing need'info, as this is irrelevant at this point. 4 years too late ;-)
blocking-kilimanjaro: + → ---
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.