Closed
Bug 1233908
Opened 8 years ago
Closed 6 years ago
assert that social services has userContextId 0
Categories
(Core :: DOM: Security, defect, P3)
Core
DOM: Security
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
People
(Reporter: huseby, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [userContextId][OA-testing][domsecurity-backlog1])
Attachments
(1 file, 2 obsolete files)
3.89 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
In the file toolkit/components/social/SocialService.jsm there are calls to createCodebasePrincipal that need to be made origin attribute aware. I'm not sure what the right fix is here, do we want user context separate or not? Here's the offending code:
> toolkit/components/social/SocialService.jsm
> line 158
> let principal = Services.scriptSecurityManager.createCodebasePrincipal(URI, {});
>
> line 518
> principal = Services.scriptSecurityManager.createCodebasePrincipal(URI, {});
>
> line 719
> this.principal = Services.scriptSecurityManager.createCodebasePrincipal(originUri, {});
Comment 1•8 years ago
|
||
What is the code in toolkit/components/social/SocialService.jsm responsible for? That will help answer whether we need to separate based on usercontext or not.
Comment 2•8 years ago
|
||
It is basically used to verify same origin (using principal.checkMayLoad) when opening chat windows and generally resolving urls (using principal.URI.resolve). check used for opening chat windows: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#915 url resolution: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#534 https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#939
Reporter | ||
Comment 3•8 years ago
|
||
Shane, At least the part about opening chat windows needs to be user context aware. The solution may be as simple as passing the origin attributes in along with the URI. For instance, one caller of isSameOrigin is in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/social/MozSocialAPI.jsm#93 > let targetDocURI = targetWindow.document.documentURIObject; > if (!provider.isSameOrigin(targetDocURI)) { > let msg = "MozSocialAPI: not attaching mozSocial API for " + provider.origin + > " to " + targetDocURI.spec + " since origins differ." > Services.console.logStringMessage(msg); > return; > } the solution would be to pass in targetWindow.document.nodePrincpal() to provider.isSameOrigin() since opening a chat window should make sure the chat window is in the correct user context. We're going to need to add checks in isSameOrigin() to check the origin attributes and fail the chat window attach if the user context id's don't match. does this sound reasonable?
Flags: needinfo?(mixedpuppy)
Comment 4•8 years ago
|
||
Assume you login to social services (like facebook) and chat. And then go to a browser tab and open facebook, will you be logged in?
Comment 5•8 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #4) > Assume you login to social services (like facebook) and chat. And then go to > a browser tab and open facebook, will you be logged in? all social panels are browsers and have whatever context exists for the given domain, exactly as if you opened a tab, logged into facebook, then opened a second tab and loaded facebook. socialapi is also disabled in private windows.
Comment 6•8 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #5) > (In reply to Tanvi Vyas [:tanvi] from comment #4) > > Assume you login to social services (like facebook) and chat. And then go to > > a browser tab and open facebook, will you be logged in? > > all social panels are browsers and have whatever context exists for the > given domain, exactly as if you opened a tab, logged into facebook, then > opened a second tab and loaded facebook. socialapi is also disabled in > private windows. This means that social panels should use the default user context of 0 for v1 of Contextual Identity. This can get a confusing (and we can solve for that in v2+), because a user might be signed in to Facebook in the "Personal" context, then open social API and have to resign in. Then they may open facebook in the default context, assuming they are not signed in because they only signed in in personal, but find themselves signed in. We can solve for this by adding some UI around social API that shows which context it operates in or allows the user to switch context. But again, in a future iteration of Contextual Identites. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1234672 to look into this. Dave, some testing and playing with the feature is still useful.
Reporter | ||
Updated•8 years ago
|
Component: SocialAPI → DOM
Product: Firefox → Core
Reporter | ||
Updated•8 years ago
|
Component: DOM → SocialAPI
Product: Core → Firefox
Reporter | ||
Updated•8 years ago
|
Whiteboard: [userContextId]
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(mixedpuppy)
Comment 7•8 years ago
|
||
btw; comment 3 sounds reasonable. fyi frameworker will break with this, but that's ok, it should be removed at this point.
Reporter | ||
Comment 8•8 years ago
|
||
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → huseby
Reporter | ||
Comment 9•8 years ago
|
||
updated for new API.
Attachment #8717685 -
Attachment is obsolete: true
Attachment #8722059 -
Flags: review?(jonas)
Comment on attachment 8722059 [details] [diff] [review] Bug_1233908.patch Review of attachment 8722059 [details] [diff] [review]: ----------------------------------------------------------------- I think this one can assert rather than set to 0.
Attachment #8722059 -
Flags: review?(jonas) → review-
Reporter | ||
Comment 11•8 years ago
|
||
updated the patch to be asserts. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=787262c942f2
Attachment #8722059 -
Attachment is obsolete: true
Attachment #8737549 -
Flags: review?(jonas)
Comment on attachment 8737549 [details] [diff] [review] Bug_1233908.patch Review of attachment 8737549 [details] [diff] [review]: ----------------------------------------------------------------- I guess I'm not 100% sure that this is all correct. However if it's not we should see once assertions start firing.
Attachment #8737549 -
Flags: review?(jonas) → review+
Reporter | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 13•8 years ago
|
||
Since bug 898706 landed, there are no obstacles to running under private windows. I'm not certain how that affects the userContextId stuff.
Reporter | ||
Updated•8 years ago
|
Whiteboard: [userContextId] → [userContextId][OA]
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/623abe61b52a
Keywords: checkin-needed
Comment 15•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8623073&repo=fx-team
Flags: needinfo?(huseby)
Comment 16•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/e97898890d47
Reporter | ||
Comment 18•8 years ago
|
||
After looking at the test failures and tracing the code out farther, I have concluded that we don't actually need to fix these callsites. I'm closing this as WON'T FIX and will re-open if something changes.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Comment 19•8 years ago
|
||
(In reply to Dave Huseby [:huseby] from comment #18) > After looking at the test failures and tracing the code out farther, I have > concluded that we don't actually need to fix these callsites. I'm closing > this as WON'T FIX and will re-open if something changes. Why don't we need to fix these callsites? The social services API should use usercontextId=0. What is triggering the assert failure?
Flags: needinfo?(tanvi)
Flags: needinfo?(huseby)
Comment 20•8 years ago
|
||
Talked to Dave about this. This bug was just adding an assert for something we already knew to be true. We are calling createCodebasePrincipal with {} which means usercontext id is 0. Dave ran into problems with the debug.js include when pushed to try, so decided to close won't fix as we have plenty of other work to do. But taking a second look at this, it looks like we are testing that the origin passed into getOriginActivationType() has a usercontextId of 0. This should also be the case since its chrome code, but wouldn't be bad to add in later. So reopening, unassigning, and moving to oa-testing to work on later if there is time.
Status: RESOLVED → REOPENED
Flags: needinfo?(tanvi)
Flags: needinfo?(huseby)
Resolution: WONTFIX → ---
Summary: social services use of createCodebasePrincipal needs to be origin attribute aware → assert that social services has userContextId 0
Whiteboard: [userContextId][OA] → [userContextId][OA-testing]
Updated•8 years ago
|
Component: SocialAPI → DOM: Security
Product: Firefox → Core
Updated•8 years ago
|
Assignee: huseby → nobody
Updated•8 years ago
|
Assignee: nobody → huseby
Whiteboard: [userContextId][OA-testing] → [userContextId][OA-testing][domsecurity-active]
Updated•8 years ago
|
Assignee: huseby → nobody
Comment 21•8 years ago
|
||
social api may be deprecated. And is not exposed to web content.
Updated•8 years ago
|
Whiteboard: [userContextId][OA-testing][domsecurity-active] → [userContextId][OA-testing][domsecurity-backlog]
Comment 22•8 years ago
|
||
Dave, the patch here got r+ but never landed; reading through the comments I am not sure whether you want to land the patch or close the bug as a wontfix. In case you are not going to land the patch, can you mark it as WONTFIX please?
Flags: needinfo?(huseby)
Reporter | ||
Comment 23•8 years ago
|
||
chris, i'll take another look at this today. i think tanvi and i decided to re-open this but deprioritize it for now. i talked with tantek in london and he still very much cares about the social api, so i don't want to just "won't fix" this bug just yet. thanks for bringing it to my attention. i'm grabbing this and assigning it back to myself.
Assignee: nobody → huseby
Flags: needinfo?(huseby)
Comment 24•8 years ago
|
||
Yes, I want to ensure that the social services API always gets cookies for usercontextid=0, and we can do that with an assertion.
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [userContextId][OA-testing][domsecurity-backlog] → [userContextId][OA-testing][domsecurity-backlog2]
Updated•8 years ago
|
Whiteboard: [userContextId][OA-testing][domsecurity-backlog2] → [userContextId][OA-testing][domsecurity-active]
Updated•8 years ago
|
Priority: P3 → P2
Reporter | ||
Comment 25•8 years ago
|
||
I was just looking at this patch again. The NS_ASSERT trips because the code is running with some other user context, obviously. I'm wondering if we can get away with slamming the userContextId to zero in this case. Based on the documentation only the Social Share Provider is still available: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Share Reading that page, it sounds like the browser loads a web page from a social provider and presents it as part of the browser UI. Since it is loaded as part of the browser UI, I think we can slam the userContextId to 0 and limit it's access similar to the awesomebar. Thoughts? --dave
Flags: needinfo?(tanvi)
Comment 26•8 years ago
|
||
How does that affect share in a private browsing window? Ideal situation is that a user logs in and all windows use that account except for private browsing windows which end up (presumably) with a different context or set of cookies. If user logs into a share account on the private window, it may or may not be the same account. If it works like that, I'm +1, if not I want to understand how it will work.
Comment 27•8 years ago
|
||
Dave, can you push this to try and see what the failures are? Why would we get a non-zero usercontext id?
Flags: needinfo?(tanvi) → needinfo?(huseby)
Reporter | ||
Comment 28•8 years ago
|
||
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=669d761fde54
Flags: needinfo?(huseby)
Reporter | ||
Comment 30•8 years ago
|
||
Arthur Edelstein says, "I don't think so." This remains to be confirmed and I will look into soon.
Flags: needinfo?(huseby)
Reporter | ||
Updated•8 years ago
|
Assignee: huseby → nobody
Updated•7 years ago
|
Assignee: nobody → huseby
Reporter | ||
Comment 31•7 years ago
|
||
So the only client of the social api is the "share this page" functionality. Since the "share this page" functionality assumes that the user wants to share the page in the currently selected tab, why couldn't we also pick up the userContextId for the tab as well? That means the share content is loaded in the same container as the currently selected tab. This will make it possible for the user to log into twitter in a personal container tab, then when they click the "share this page" their twitter activation will also be logged in. If we did this, then the asserts in the social api shouldn't enforce the default userContextId.
Reporter | ||
Comment 32•7 years ago
|
||
the "share this page" doesn't get the correct userContextId when it loads the share content from the service provider. I propose, for "share this page" that not only do we pull the URI but we also pull the origin attributes from the currently selected tab. that way, if the user is logged into twitter in a personal container tab and they click "share this page", the twitter share content will already be logged in. if they then switch to a work container tab and they haven't logged into twitter in the work container, then clicking the share this page button would bring up twitter share content but won't be logged in.
Updated•7 years ago
|
Assignee: huseby → nobody
Comment 33•7 years ago
|
||
But then over time the social sites you use for sharing end up logged in in every context you share things from. That seems worse for privacy than the original suggestion of just using a fixed container 0. Or even give sharing it's own special context ID.
Comment 34•7 years ago
|
||
Since no one is assigned to this bug I am putting it in the backlog for now.
Priority: P2 → P3
Whiteboard: [userContextId][OA-testing][domsecurity-active] → [userContextId][OA-testing][domsecurity-backlog1]
Updated•7 years ago
|
Blocks: ContextualIdentity
Comment 35•7 years ago
|
||
Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020
status-firefox57:
--- → fix-optional
Comment 37•6 years ago
|
||
Yeah, this is no longer needed.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 6 years ago
Flags: needinfo?(mixedpuppy)
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•