Installation failure due to installs_allowed_from returns INVALID_MANIFEST

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: ianbicking, Assigned: MattN)

Tracking

14 Branch
mozilla19
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 5 obsolete attachments)

If you install an app from an origin that isn't allowed due to installs_allowed_from, the error is INVALID_MANIFEST, which doesn't seem right and is definitely confusing.  PERMISSION_DENIED might be better, or a new error just for this case.

The problem is in Webapps.js, WebappsRegistry.checkManifest (or how that function is being used).
This sounds like a mozapps API bug, not a bug in the front-end UI.
Component: Web Apps → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: webapps → general
Blocks: 746465
Patch attached.  We don't have tests in the tree, but I have a manual test here: http://app1.ianbicking.org/?manifest=manifest_bad_installs_allowed_from.webapp

This adds a new error, INSTALL_FROM_DENIED

Because we've encountered this specific error several times in testing, and each time it confuses people, I think it is best to have a distinct error for just this case.
Attachment #617583 - Flags: review?(fabrice)
Comment on attachment 617583 [details]
Checks installs_allowed_from separately and has a separate error for that case

>diff -r 85abf12e5c83 dom/base/Webapps.js
>--- a/dom/base/Webapps.js	Mon Apr 23 07:11:19 2012 -0700
>+++ b/dom/base/Webapps.js	Mon Apr 23 14:04:08 2012 -0500
>@@ -35,25 +35,28 @@
>    * only the name property is mandatory
>    */
>   checkManifest: function(aManifest, aInstallOrigin) {
>-    // TODO : check for install_allowed_from
>     if (aManifest.name == undefined)
>       return false;
>+    return true;
>+  },
> 
>-    if (aManifest.installs_allowed_from) {
>-      ok = false;
>-      aManifest.installs_allowed_from.forEach(function(aOrigin) {
>-        if (aOrigin == "*" || aOrigin == aInstallOrigin)
>-          ok = true;
>-      });
>-      return ok;
>+  checkInstallAllowed: function checkInstallAllowed(manifest, installOrigin) {

use aManifest and aInstallOrigin here.

>+    if (! manifest.installs_allowed_from) {
>+      return true;
>     }
>+    for (let i=0; i<manifest.installs_allowed_from.length; i++) {

nit: spaces around < . Also, why not use forEach ?

>+      let origin = manifest.installs_allowed_from[i];
>+      if (origin == "*" || origin == installOrigin) {
>     return true;
>+      }
>+    }
>+    return false;
>   },
> 

r=me with nits addressed
Attachment #617583 - Flags: review?(fabrice) → review+
New patch with nits addressed.  I didn't use forEach because that required a flag to handle the return, so using an index is less total code.
Attachment #617583 - Attachment is obsolete: true
Attachment #617600 - Attachment is patch: true
Comment on attachment 617600 [details] [diff] [review]
Checks installs_allowed_from separately and has a separate error for that case (v2)

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

::: dom/base/Webapps.js
@@ +42,3 @@
>  
> +  checkInstallAllowed: function checkInstallAllowed(aManifest, aInstallOrigin) {
> +    if (! aManifest.installs_allowed_from) {

Nit: !aManifest.

@@ +46,2 @@
>      }
> +    for (let i=0; i < aManifest.installs_allowed_from.length; i++) {

Nit: i = 0

And what about:
installs_allowed_from.forEach(aOrigin) {
  if (aOrigin == "*" || aOrigin == aInstallOrigin) {
    return true;
  }
}

return false;
whitespace fixed
Attachment #617600 - Attachment is obsolete: true
Comment on attachment 617607 [details] [diff] [review]
Checks installs_allowed_from separately and has a separate error for that case (v3)

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

::: dom/base/Webapps.js
@@ +50,1 @@
>      return true;

Nit: indentation
Attachment #617607 - Flags: review+
Fix indentation (an unfortunate side effect from diff -w)
Attachment #617607 - Attachment is obsolete: true
No longer blocks: 746465
Component: DOM: Mozilla Extensions → DOM: Apps
Posted patch Rebase ianb's patch (v5) (obsolete) — Splinter Review
This patch had r+ but never got checked-in :(

I ran into this issue and found this bug so decided to rebase.  There has been a decent amount of change in this code so I'm requesting re-review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3e107bbcf247
Assignee: nobody → mnoorenberghe+bmo
Attachment #617624 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #682855 - Flags: review?(fabrice)
Comment on attachment 682855 [details] [diff] [review]
Rebase ianb's patch (v5)

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

Thanks for reviving that Matthew! Can you add a test also before we land? They are in dom/tests/mochitests/webapps
Attachment #682855 - Flags: review?(fabrice) → feedback+
I added tests for the basic install case but didn't see where the other cases are tested and I don't know enough about packaged apps to create one from scratch.

Before attaching v5 I searched for references to INVALID_MANIFEST and didn't see any cases that needed to be updated so I don't expect things to break since this patch just changed the error name for this case.
Attachment #682855 - Attachment is obsolete: true
Attachment #682935 - Flags: review?(fabrice)
Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

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

r=me
Can you ask for uplift to aurora 18?
Attachment #682935 - Flags: review?(fabrice) → review+
Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fddb9923ef0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): More prevalent confusion caused by bug 799563
User impact if declined: Developers will think their manifest is invalid when it is not.
Testing completed (on m-c, etc.): Just pushed to inbound
Risk to taking this patch (and alternatives if risky): Low risk IMO because the only difference is what error is thrown.
String or UUID changes made by this patch: None
Attachment #682935 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/4fddb9923ef0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [qa-]
Comment on attachment 682935 [details] [diff] [review]
Added test for non-packaged installation (v6)

[Triage Comment]
Low risk fix in support of web apps (with no chance of regression outside of apps).

If this is landed on mozilla-aurora before ~11AM PT tomorrow, this will make the merge from Aurora 18 to Beta 18. If landed 11AM-5PM PT tomorrow on mozilla-beta, it will make the first FF18 Beta. If landed after that, it will end up in the second FF18 beta.
Attachment #682935 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.