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)
Core
DOM: Core & HTML
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)
8.93 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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?
Assignee | ||
Comment 3•8 years ago
|
||
fixed a name I missed before.
Attachment #8700229 -
Attachment is obsolete: true
Attachment #8700229 -
Flags: review?(jonas)
Attachment #8700767 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Component: Security → DOM
Product: Firefox → Core
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId]
Assignee | ||
Updated•8 years ago
|
Attachment #8700767 -
Flags: review?(jonas)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8713796 -
Flags: review?(jonas)
Assignee | ||
Comment 5•8 years ago
|
||
updated for the new ChromeUtils api.
Attachment #8713796 -
Attachment is obsolete: true
Attachment #8720500 -
Flags: review?(jonas)
Comment 6•8 years ago
|
||
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-
Assignee | ||
Comment 8•8 years ago
|
||
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-
Assignee | ||
Comment 11•8 years ago
|
||
I made the requested changes.
Attachment #8720616 -
Attachment is obsolete: true
Attachment #8735170 -
Flags: review?(jonas)
Assignee | ||
Comment 12•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=779d6ed5aaa7
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId] → [userContextId][OA]
Attachment #8735170 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebad046a7a9b
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd8c193328d
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6cd8c193328d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 16•8 years ago
|
||
This caused regression https://bugzilla.mozilla.org/show_bug.cgi?id=1284986
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•