Closed Bug 1233908 Opened 4 years ago Closed 2 years ago

assert that social services has userContextId 0

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox57 --- fix-optional

People

(Reporter: huseby, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [userContextId][OA-testing][domsecurity-backlog1])

Attachments

(1 file, 2 obsolete files)

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, {});
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.
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
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)
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?
(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.
Blocks: 1234672
(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.
Component: SocialAPI → DOM
Product: Firefox → Core
Component: DOM → SocialAPI
Product: Core → Firefox
Whiteboard: [userContextId]
Flags: needinfo?(mixedpuppy)
btw; comment 3 sounds reasonable.  fyi frameworker will break with this, but that's ok, it should be removed at this point.
Attached patch Bug_1233908.patch (obsolete) — Splinter Review
Assignee: nobody → huseby
Attached patch Bug_1233908.patch (obsolete) — Splinter Review
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-
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+
Status: NEW → ASSIGNED
Since bug 898706 landed, there are no obstacles to running under private windows.  I'm not certain how that affects the userContextId stuff.
Whiteboard: [userContextId] → [userContextId][OA]
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8623073&repo=fx-team
Flags: needinfo?(huseby)
I'll take a look and get this landed. :/
Flags: needinfo?(huseby)
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: 4 years ago
Resolution: --- → WONTFIX
(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)
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]
Component: SocialAPI → DOM: Security
Product: Firefox → Core
Assignee: huseby → nobody
Assignee: nobody → huseby
Whiteboard: [userContextId][OA-testing] → [userContextId][OA-testing][domsecurity-active]
Assignee: huseby → nobody
social api may be deprecated.  And is not exposed to web content.
Whiteboard: [userContextId][OA-testing][domsecurity-active] → [userContextId][OA-testing][domsecurity-backlog]
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)
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)
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
Priority: P2 → P3
Whiteboard: [userContextId][OA-testing][domsecurity-backlog] → [userContextId][OA-testing][domsecurity-backlog2]
Whiteboard: [userContextId][OA-testing][domsecurity-backlog2] → [userContextId][OA-testing][domsecurity-active]
Priority: P3 → P2
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)
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.
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)
Does the tor browser use social services?
Flags: needinfo?(huseby)
Arthur Edelstein says, "I don't think so."  This remains to be confirmed and I will look into soon.
Flags: needinfo?(huseby)
Assignee: huseby → nobody
Assignee: nobody → huseby
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.
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.
Assignee: huseby → nobody
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.
Depends on: 1347233
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]
I think this code is gone, right Shane?
Flags: needinfo?(mixedpuppy)
No longer depends on: 1347233
Yeah, this is no longer needed.
Status: REOPENED → RESOLVED
Closed: 4 years ago2 years ago
Flags: needinfo?(mixedpuppy)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.