The "webapps-manage" permission is checked in the child when the navigator.mozApps.mgmt is used, but we always return an object. Then we throw if you try to set oninstall/onuninstall handlers. If you then call "applyDownload", "getAll", or "getNotInstalled" we for some reason *don't* throw and let the call succeed but then parent will kill the process. Also, we send the "hasPrivileges" value from the child to the parent for the "getAll" function, and that value is used in the parent after we've already asserted the permission in the parent.

Ben, what would be the appropriate behavior ? Throw when accessing mozApps.mgmt without privileges, or returning null? I'll keep the assertion in the parent for all the calls that can come from mgmt.*

I think our unofficial convention is to return null when you don't have permission. Sicking?

Yes. The property should still exist as indication that we support the API. But it should return null as indication that the caller doesn't have permission to use it.

basecamp- as this is nice to have any not actually a security threat.

Comment on attachment 704085 [details] [diff] [review] patch Review of attachment 704085 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.js @@ +256,5 @@ > + .getService(Ci.nsIScriptSecurityManager); > + let perm = principal == secMan.getSystemPrincipal() > + ? Ci.nsIPermissionManager.ALLOW_ACTION > + : Services.perms > + .testExactPermissionFromPrincipal(principal, "webapps-manage"); You should be able to call Services.perms.textExact..(principal, ...) even when principal is the system principal. It should always return ALLOW_ACTION. @@ +260,5 @@ > + .testExactPermissionFromPrincipal(principal, "webapps-manage"); > + > + //only pages with the webapps-manage permission set can get access to > + // the mgmt object. > + this.hasPrivileges = perm == Ci.nsIPermissionManager.ALLOW_ACTION; Maybe name this this.hasMgmtPrivileges. I think elsewhere we use "hasPrivileges" to indicate if the feature itself is permitted.

Comment on attachment 704085 [details] [diff] [review] patch [Approval Request Comment] User impact if declined: Makes it harder to cleanly check access to mozApps.mgmt Testing completed: passes tests and didn't regress my local b2g18 build Risk to taking this patch (and alternatives if risky): none String or UUID changes made by this patch: none

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/39bc4c77571f for Linux browser-chrome failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=18945788&tree=Mozilla-Inbound

green run on try with a test fix: https://tbpl.mozilla.org/?tree=Try&rev=8f67e699181a https://hg.mozilla.org/integration/mozilla-inbound/rev/6d14b59b7ce5