Closed
Bug 757226
Opened 13 years ago
Closed 11 years ago
Implement mozApps app.replaceReceipt()
Categories
(Core Graveyard :: DOM: Apps, defect, P2)
Core Graveyard
DOM: Apps
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)
(deleted),
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
We should implement a replaceReceipt() method as described here:
https://groups.google.com/d/msg/mozilla.dev.webapps/_9OjB23au6Q/RDJE0K25-Y8J
Comment 1•13 years ago
|
||
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: --- → ?
Updated•13 years ago
|
Whiteboard: [blocking-webrtdesktop1]
Updated•13 years ago
|
Whiteboard: [blocking-webrtdesktop1] → [blocking-webrtdesktop1+]
Comment 2•13 years ago
|
||
FYI to Myk - Bill wants this to block the desktop web runtime release. Thoughts?
Priority: -- → P1
Comment 3•13 years ago
|
||
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+]
Reporter | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
I added notes on how this could work with the reissue URL:
https://wiki.mozilla.org/Apps/WebApplicationReceiptRefresh
Updated•13 years ago
|
blocking-kilimanjaro: ? → +
Comment 6•12 years ago
|
||
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: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Comment 8•12 years ago
|
||
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?
Updated•12 years ago
|
Priority: -- → P2
Comment 9•12 years ago
|
||
Renom if you think we can't ship a v1 without this.
blocking-basecamp: + → ---
Comment 10•12 years ago
|
||
(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: --- → ?
Updated•12 years ago
|
Whiteboard: [blocked-on-input] → [blocked-on-input Bill Walker]
Comment 11•12 years ago
|
||
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: ? → ---
Updated•12 years ago
|
Whiteboard: [blocked-on-input Bill Walker]
Updated•12 years ago
|
Assignee: nobody → amckay
Updated•12 years ago
|
Keywords: dev-doc-needed
Updated•12 years ago
|
Assignee: amckay → nobody
Comment 12•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #753501 -
Attachment is patch: true
Attachment #753501 -
Attachment mime type: text/x-patch → text/plain
Comment 13•12 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 15•11 years ago
|
||
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
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #781230 -
Flags: feedback?(mcaceres)
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
(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.
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #781230 -
Attachment is obsolete: true
Attachment #786550 -
Flags: review?(fabrice)
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
(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•11 years ago
|
||
Bump. If we could keep this moving it would be appreciated, the number of things it blocks grows regularly.
Comment 27•11 years ago
|
||
+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•11 years ago
|
||
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)
Assignee | ||
Comment 29•11 years ago
|
||
(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)
Assignee | ||
Comment 30•11 years ago
|
||
Bugzilla is live again, here's the new patch.
Attachment #786550 -
Attachment is obsolete: true
Attachment #8358866 -
Flags: review?(fabrice)
Comment 31•11 years ago
|
||
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+
Assignee | ||
Comment 32•11 years ago
|
||
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)
Assignee | ||
Comment 33•11 years ago
|
||
About the limit, I don't know, what would be a reasonable limit?
Comment 34•11 years ago
|
||
(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)
Comment 35•11 years ago
|
||
A few hundred receipts *per app* should be enough to cover in-app purchases.
Flags: needinfo?(amckay)
Comment 36•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 39•11 years ago
|
||
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 40•11 years ago
|
||
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 41•11 years ago
|
||
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+
Assignee | ||
Comment 42•11 years ago
|
||
(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 43•11 years ago
|
||
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+
Assignee | ||
Comment 44•11 years ago
|
||
(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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 45•11 years ago
|
||
Keywords: checkin-needed
Comment 46•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 47•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [qa-]
Comment 48•10 years ago
|
||
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)
Updated•7 years ago
|
Product: Core → Core Graveyard
Comment 49•6 years ago
|
||
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.
Description
•