Closed Bug 1196665 Opened 4 years ago Closed 4 years ago

Add originAttributes into SpecialPowers

Categories

(Firefox OS Graveyard :: Runtime, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S10 (30Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 9 obsolete files)

Right now SpecialPowers will pass appId and isInBrowserElement.
We should merge it into OriginAttributes.
Status: NEW → ASSIGNED
Attached patch WIP Patch. (obsolete) — Splinter Review
WIP patch, still have some orange in try.
Attached patch WIP Patch. (obsolete) — Splinter Review
fixed more tests.
Attachment #8670595 - Attachment is obsolete: true
Target Milestone: --- → FxOS-S10 (30Oct)
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8675623 - Attachment is obsolete: true
Attachment #8675626 - Flags: review?(bobbyholley)
Comment on attachment 8675626 [details] [diff] [review]
Patch.

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

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +322,5 @@
>        case "SPPermissionManager": {
>          let msg = aMessage.json;
>  
>          let secMan = Services.scriptSecurityManager;
> +        let principal = secMan.createCodebasePrincipal(this._getURI(msg.url), msg.originAttributes);

The message manager knows how to serialize principals. So why don't we create the principal in the content process (in specialpowersAPI), and send bonafide principal with SPPermissionManager messages? I think that would reduce the complexity of the code in specialPowersAPI.
Attachment #8675626 - Flags: review?(bobbyholley)
Thanks, that's a great idea. I should have thought of this. :P

I'll also check if it's reasonable to change the SpecialPowers API to take principal as one of the arguments, for example 
* pushPermissions
* testPermission
* hasPermission
* removePermission
* addPermission
They have an 'arg', which could be a string, or a plain JS object, or an instance of document,
it looks we could replace arg with principal, so the callers could pass document.nodePrincipal, or app.principal, or construct the principal by secMan.createCodebasePrincipal, but if the change is too large, I'll file another bug to change those API entirely.
(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Thanks, that's a great idea. I should have thought of this. :P
> 
> I'll also check if it's reasonable to change the SpecialPowers API to take
> principal as one of the arguments, for example 
> * pushPermissions
> * testPermission
> * hasPermission
> * removePermission
> * addPermission
> They have an 'arg', which could be a string, or a plain JS object, or an
> instance of document,
> it looks we could replace arg with principal, so the callers could pass
> document.nodePrincipal, or app.principal, or construct the principal by
> secMan.createCodebasePrincipal, but if the change is too large, I'll file
> another bug to change those API entirely.

In general I think this would be great. However, it's worth noting that untrusted content cannot access nsIPrincipal objects without SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed instance of nsIPrincipal, but should also support passing in a document, as well as a manual { url : foo, originAttributes : { ...} } object.
Attached patch WIP Patch v2. (obsolete) — Splinter Review
This version passes principal to SpecialPowersObserverAPI.js.
I haven't done the passing wrapped version of nsIPrincipal yet.

However I am still trying to fix an error when running dom/permission/tests/test_alarms.html.

In [1], it will pass SpecialPowers.wrap(iframe).contentDocument
but the nodePrincipal of this object seems has some problems 

From the mochitest

[21685] WARNING: 'aRv.Failed()', file /home/allstars/ssd/gecko-dev/dom/ipc/StructuredCloneData.cpp, line 72
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 334: NS_ERROR_FAILURE: Failure arg 0 [nsIPermissionManager.testPermissionFromPrincipal]

will spend more time on checking this.

[1] : https://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/file_framework.js#96
Attachment #8675626 - Attachment is obsolete: true
You need to do unwrapIfWrapped on the wrapped nodePrincipal.
(In reply to Bobby Holley (:bholley) from comment #7)
> In general I think this would be great. However, it's worth noting that
> untrusted content cannot access nsIPrincipal objects without
> SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed
> instance of nsIPrincipal, but should also support passing in a document, as
> well as a manual { url : foo, originAttributes : { ...} } object.

Thanks for the comments,

If we still need to support passing a 'document' then I think it's easier than passing document.nodePrincipal.

For the app cases, I found we only support principal in mozIApplication but not DOMApplication, it looks providing manifestURL is also easier.

So I didn't add another support for passing nsIPrincipal in the end.

(In reply to Bobby Holley (:bholley) from comment #9)
> You need to do unwrapIfWrapped on the wrapped nodePrincipal.

Yeah, it's working, thanks again :D
Attachment #8677392 - Flags: review?(bobbyholley)
Attachment #8677393 - Flags: review?(bobbyholley)
Comment on attachment 8677392 [details] [diff] [review]
Part 1: Add originAttributes into SpecialPowers.

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

r=me with those comments addressed.

::: dom/browser-element/mochitest/browserElement_DisallowEmbedAppsInOOP.js
@@ +28,5 @@
>  
>    document.body.appendChild(iframe);
>  
> +  var context = {'url': 'http://example.org',
> +                 'originAttributes': {'inBrowser': true}};

You don't need to put quotation marks around the key name in a JS object literal. 

This occurs throughout the patch - please fix it everywhere.

::: dom/cache/test/mochitest/test_cache_orphaned_body.html
@@ +46,2 @@
>      }
> +    SpecialPowers.getStorageUsageForURI(document.documentURI, resolve, attrs);

It looks like all of this code is just passing the nodePrincipal of |document|. Can you just do that? Same for all the other test_cache* things, and various other places in this patch. Please fix them.

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.html
@@ +95,5 @@
>    var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL);
> +  is(appId, 0, "appId should become NO_APP_ID");
> +  // since gApp is uninstalled, calling SpecialPowers.hasPermission with the
> +  // app's properties (manifestURL, origin, principal, ... etc) will throw.
> +  // So we don't need to test hasPermission for the app.

Wait, why did this change with this patch? hasPermission was working before, right?

::: testing/specialpowers/components/SpecialPowersObserver.js
@@ +235,5 @@
>            // so we fake these properties.
>            msg.permission = {
> +            principal: {
> +              appId: permission.principal.appId,
> +              originAttributes: {appId: permission.principal.appId}

Shouldn't the appId item be removed, since it's contained within the originAttributes?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1852,3 @@
>      }
>  
> +    return [ principal, isSystem ];

Given that we're returning a real principal, I don't think it's necessary to return isSystem anymore, since the caller can just ask that question directly.

I filed bug 1218039 to add an ".isSystemPrincipal" to nsIPrincipal.
Attachment #8677392 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #14)
> :::
> testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.
> html
> @@ +95,5 @@
> >    var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL);
> > +  is(appId, 0, "appId should become NO_APP_ID");
> > +  // since gApp is uninstalled, calling SpecialPowers.hasPermission with the
> > +  // app's properties (manifestURL, origin, principal, ... etc) will throw.
> > +  // So we don't need to test hasPermission for the app.
> 
> Wait, why did this change with this patch? hasPermission was working before,
> right?
> 
There are two problems in the original test case.
1. gApp.origin is 'undefined', which is type of string.

2. The original test case has thrown in the parent side
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 181: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]

so inside hasPermisison in specialpowersAPI.js, the line
this._sendSyncMessage('SPPermissionManager', msg)[0] actually returns undefined, instead of false.

Back to the test case 
ok(!SpecialPowers.hasPermission('pAppPermission', context), 'pAppPermission should not have permission');
it will evaluate as true, and the test passes.

So I remove the check in my patch.
Comment on attachment 8677393 [details] [diff] [review]
Part 2: update QuotaManager API fors SpecialPowers.

will merge this into Part 1
Attachment #8677393 - Flags: review?(bobbyholley)
addressed bobby's comment and rebase on Bug 1218039

https://treeherder.mozilla.org/#/jobs?repo=try&revision=def1dc8485b4
Attachment #8677392 - Attachment is obsolete: true
Attachment #8677393 - Attachment is obsolete: true
Attachment #8678701 - Flags: review+
QA Whiteboard: [COM=NSec]
oh, Bug 1196654 landed this morning, but I didn't notice that.
will submit a Part 2 patch to rebase on Bug 1196654.
Attachment #8679293 - Flags: review?(timdream)
rebase
Attachment #8678701 - Attachment is obsolete: true
Attachment #8679293 - Attachment is obsolete: true
Attachment #8679395 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0bf567d77e0c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.