Closed Bug 1233885 Opened 8 years ago Closed 8 years ago

fix up docInfo and permission manager to use default user context

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

(Depends on 1 open bug)

Details

(Whiteboard: [userContextId][OA])

Attachments

(1 file, 5 obsolete files)

This bug is to cover the fix up of the permissions related calls to createCodebasePrincipal.  My audit shows that there are calls in browser/base/content/pageinfo/permissions.js, extensions/cookie/nsPermission.cpp, and extensions/cookie/nsPermissionManager.cpp that are all related.
Status: NEW → ASSIGNED
Attached patch Bug_1233885.patch (obsolete) — Splinter Review
fixes up the load info and permission manager to always use the default user context.  this also affects when permissions are set.
Attachment #8700229 - Flags: review?(jonas)
Comment on attachment 8700229 [details] [diff] [review]
Bug_1233885.patch

+function onLoadPermission(uri, originAttributes)
 {
   var permTab = document.getElementById("permTab");
   if (SitePermissions.isSupportedURI(uri)) {
     gPermURI = uri;
+    gOriginAttributes = ChromeUtils.globalContextOriginAttributes(originAttributes);
     var hostText = document.getElementById("hostText");
     hostText.value = gPermURI.prePath;
 
     for (var i of gPermissions)
       initRow(i);
     var os = Components.classes["@mozilla.org/observer-service;1"]
                        .getService(Components.interfaces.nsIObserverService);
     os.addObserver(permissionObserver, "perm-changed", false);

Do you mean defaultContextOriginAttributes here?
Attached patch Bug_1233885.patch (obsolete) — Splinter Review
fixed a name I missed before.
Attachment #8700229 - Attachment is obsolete: true
Attachment #8700229 - Flags: review?(jonas)
Attachment #8700767 - Flags: review?(jonas)
Component: Security → DOM
Product: Firefox → Core
Whiteboard: [userContextId]
Attachment #8700767 - Flags: review?(jonas)
Attached patch Bug_1233885.patch (obsolete) — Splinter Review
this depends on Bug 1229222 patches.  this enforces that all origin permissions are isolated by all other origin attributes *except* userContextId by forcing the user context id to always be the default value.
Attachment #8700767 - Attachment is obsolete: true
Attachment #8713796 - Flags: review?(jonas)
Depends on: 1229222
Attachment #8713796 - Flags: review?(jonas)
Attached patch Bug_1233885.patch (obsolete) — Splinter Review
updated for the new ChromeUtils api.
Attachment #8713796 - Attachment is obsolete: true
Attachment #8720500 - Flags: review?(jonas)
Comment on attachment 8720500 [details] [diff] [review]
Bug_1233885.patch

     // Copy the attributes over
-    mozilla::PrincipalOriginAttributes attrs = mozilla::BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
-    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(newURI, attrs);
+    mozilla::PrincipalOriginAttributes attrs = 
+      mozilla::BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
+
+    // ensure that the user context isolation is disabled
+    if (attrs.mUserContextId !=
+        nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID) {
+      return nullptr;
should we MOZ_ASSERT here instead?
+    }
+
Comment on attachment 8720500 [details] [diff] [review]
Bug_1233885.patch

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

Getting close, but I think this is not quite right yet.

::: browser/base/content/content.js
@@ +1011,5 @@
>      docInfo.compatMode = document.compatMode;
>      docInfo.contentType = document.contentType;
>      docInfo.characterSet = document.characterSet;
>      docInfo.lastModified = document.lastModified;
> +    docInfo.originAttributes = document.nodePrincipal.originAttributes;

Couldn't we just grab the whole principal here?

::: browser/base/content/pageinfo/permissions.js
@@ +34,5 @@
>    var permTab = document.getElementById("permTab");
>    if (SitePermissions.isSupportedURI(uri)) {
>      gPermURI = uri;
> +    gOriginAttributes = ChromeUtils.createOriginAttributesFromDict(originAttributes);
> +    gOriginAttributes.userContextId = 0;

Do you need to do this given that the permissionmanager will ignore the userContextId anyway?

If we do need it, please add a comment describing why.

@@ +193,5 @@
>      Components.classes["@mozilla.org/dom/quota-manager-service;1"]
>                .getService(nsIQuotaManagerService);
>    let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>                              .getService(Components.interfaces.nsIScriptSecurityManager)
> +                            .createCodebasePrincipal(gPermURI, gOriginAttributes);

It's unfortunate that we're creating a principal here.

Could we add gPermPrincipal instead of adding gOriginAttributes? And grab the principal directly from the page rather than try to recreate it here.

Doing that hinges on that we don't need to do the .userContextId=0 thing above.

@@ +198,3 @@
>    gUsageRequest =
>      quotaManagerService.getUsageForPrincipal(principal,
>                                               onIndexedDBUsageCallback);

Actually, I think this requires that we *don't* set userContextId=0 above. Since the IndexedDB usage *is* segmented per context.

@@ +213,3 @@
>    let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>                              .getService(Components.interfaces.nsIScriptSecurityManager)
> +                            .createCodebasePrincipal(gPermURI, gOriginAttributes);

Same here, if we do the userContextId=0 above, that will mean that we'll clear the IndexedDB data for the default user context, and not for the current page.

::: extensions/cookie/nsPermissionManager.cpp
@@ +2177,5 @@
> +      return nullptr;
> +    }
> +
> +    nsCOMPtr<nsIPrincipal> principal = 
> +      mozilla::BasePrincipal::CreateCodebasePrincipal(newURI, attrs);

Why don't we want to simply create a principal which has userContextId=0 here? No matter what the user context of the passed in principal was.
Attachment #8720500 - Flags: review?(jonas) → review-
Attached patch Bug_1233885.patch (obsolete) — Splinter Review
revised to preserve the origin attributes in the pageInfo code but forces the default user context in the permission manager.
Attachment #8720500 - Attachment is obsolete: true
Attachment #8720616 - Flags: review?(jonas)
Comment on attachment 8720616 [details] [diff] [review]
Bug_1233885.patch

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +2174,5 @@
> +    // ensure that the user context isolation is disabled
> +    if (attrs.mUserContextId !=
> +        nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID) {
> +      return nullptr;
> +    }

Shouldn't you just set |attrs.mUserContextId = DEFAULT_...| here?
Comment on attachment 8720616 [details] [diff] [review]
Bug_1233885.patch

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

r- based on previous comment.
Attachment #8720616 - Flags: review?(jonas) → review-
I made the requested changes.
Attachment #8720616 - Attachment is obsolete: true
Attachment #8735170 - Flags: review?(jonas)
Whiteboard: [userContextId] → [userContextId][OA]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6cd8c193328d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1284986
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: