1233908 - assert that social services has userContextId 0

Status: RESOLVED WONTFIX

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

Description

[Dave Huseby [:huseby]]

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

[Tanvi Vyas[:tanvi]]

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

[Shane Caraveo (:mixedpuppy)]

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

Comment 3

[Dave Huseby [:huseby]]

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?

Comment 4

[Tanvi Vyas[:tanvi]]

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

[Shane Caraveo (:mixedpuppy)]

(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

[Tanvi Vyas[:tanvi]]

(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.

Comment 7

[Shane Caraveo (:mixedpuppy)]

btw; comment 3 sounds reasonable.  fyi frameworker will break with this, but that's ok, it should be removed at this point.

Comment 8

[Dave Huseby [:huseby]]

Attachment: Bug_1233908.patch

Comment 9

[Dave Huseby [:huseby]]

Attachment: Bug_1233908.patch

updated for new API.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Dave Huseby [:huseby]]

Attachment: Bug_1233908.patch

updated the patch to be asserts.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=787262c942f2

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Since bug 898706 landed, there are no obstacles to running under private windows.  I'm not certain how that affects the userContextId stuff.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/623abe61b52a

Comment 15

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8623073&repo=fx-team

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/e97898890d47

Comment 17

[Dave Huseby [:huseby]]

I'll take a look and get this landed. :/

Comment 18

[Dave Huseby [: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.

Comment 19

[Tanvi Vyas[:tanvi]]

(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?

Comment 20

[Tanvi Vyas[:tanvi]]

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.

Comment 21

[Tanvi Vyas[:tanvi]]

social api may be deprecated.  And is not exposed to web content.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 23

[Dave Huseby [: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.

Comment 24

[Tanvi Vyas[:tanvi]]

Yes, I want to ensure that the social services API always gets cookies for usercontextid=0, and we can do that with an assertion.

Comment 25

[Dave Huseby [:huseby]]

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

Comment 26

[Shane Caraveo (:mixedpuppy)]

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

[Tanvi Vyas[:tanvi]]

Dave, can you push this to try and see what the failures are?  Why would we get a non-zero usercontext id?

Comment 28

[Dave Huseby [:huseby]]

try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=669d761fde54

Comment 29

[Tanvi Vyas[:tanvi]]

Does the tor browser use social services?

Comment 30

[Dave Huseby [:huseby]]

Arthur Edelstein says, "I don't think so."  This remains to be confirmed and I will look into soon.

Comment 31

[Dave Huseby [: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.

Comment 32

[Dave Huseby [:huseby]]

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.

Comment 33

[Daniel Veditz [:dveditz]]

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

[Christoph Kerschbaumer [:ckerschb]]

Since no one is assigned to this bug I am putting it in the backlog for now.

Comment 35

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 36

[:Gijs (he/him)]

I think this code is gone, right Shane?

Comment 37

[Shane Caraveo (:mixedpuppy)]

Yeah, this is no longer needed.