Closed Bug 1233904 Opened 4 years ago Closed 4 years ago

firefox sync needs to use the default user context origin attributes

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][OA])

Attachments

(1 file, 6 obsolete files)

In services/sync/Weave.js there is a call to createCodebasePrincipal that needs to be aware of originAttributes.

> services/sync/Weave.js
>   line 189
>     let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                 .getService(Ci.nsIScriptSecurityManager);
>     let principal = ssm.createCodebasePrincipal(uri, {});

I'm not sure if we want to use the default user context or if it is more complicated than that.  If we're sync'ing cookies, we want isolation but if we're sync'ing bookmarks maybe we don't?
IIUC, that code is simply trying to make a new channel to gain access to files in the profile directory for about:sync-log - it's not used to actual syncing at all. I don't know enough about channels, but I suspect that code was cargo-culted and will have an easy fix - all we need is for about:sync-log to work.
(In reply to Mark Hammond [:markh] from comment #1)
> IIUC, that code is simply trying to make a new channel to gain access to
> files in the profile directory for about:sync-log

What are the files used for?  What does about:sync-log have on it (I don't see anything when I access it on my browser).

My guess is that a default origin attributes could be used here, but a little more info would help to confirm that.
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> What are the files used for?  What does about:sync-log have on it (I don't
> see anything when I access it on my browser).

It has a typical "index" view of a directory in your profile, containing log files written by Sync via Log.jsm. It should appear on any profile where Sync is configured.

> My guess is that a default origin attributes could be used here, but a
> little more info would help to confirm that.

I'd guess that too, although there always seems to be funkiness with file:// URIs.
Component: Sync → DOM
Product: Firefox → Core
Component: DOM → Sync
Product: Core → Firefox
Whiteboard: [userContextId]
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
this makes sure the origin attributes used to create the principal for the channel that accesses the sync log files is set to the default user context.  we don't want any user context isolation for this (ATM).
Assignee: nobody → huseby
Status: NEW → ASSIGNED
Attachment #8709215 - Flags: review?(dolske)
Comment on attachment 8709215 [details] [diff] [review]
Bug_1233904.patch

rs+=me if all you want is "gee, I dunno what this does but the people in bug 1218479 seem smart". But I'll bounce this to markh in case he wants to say more than he already has. :)
Attachment #8709215 - Flags: review?(dolske) → review?(markh)
Comment on attachment 8709215 [details] [diff] [review]
Bug_1233904.patch

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

Looks fine to me.
Attachment #8709215 - Flags: review?(markh) → review+
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
This is a rebased and updated patch to match the changes in Bug 1229222.  This cannot land until 1229222 does.
Attachment #8709215 - Attachment is obsolete: true
Attachment #8713826 - Flags: review+
BTW, Weave/Sync is going to need a much more thorough audit to make sure that in all cases, the principals it uses all have the default user context id origin attribute set.  We have decided that we are not going to isolate bookmarks, usernames/passwords, etc based on user context id in the first version of the feature.  That means sync needs to always be working in the default user context id.

I am especially concerned with the sync tabs part of this feature.  We need to make sure that the serialization of tabs includes the full origin attributes so that upon restoration, it can recreate tabs in contexts.
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
This patch cleans up the previous patch to use the new API defined in bug 1229222.  The goal here is to make the about:sync-log page to use the default user context id.
Attachment #8713826 - Attachment is obsolete: true
Attachment #8715012 - Flags: review?(jonas)
Depends on: bz-bugwrite
Depends on: 1229222
No longer depends on: bz-bugwrite
Comment on attachment 8715012 [details] [diff] [review]
Bug_1233904.patch

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

::: services/sync/Weave.js
@@ +191,3 @@
>      let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                  .getService(Ci.nsIScriptSecurityManager);
> +    let attrs = ChromeUtils.setDefaultUserContextId(aLoadInfo.originAttributes);

Why can't this just do:

let attrs = aLoadInfo.originAttributes;
attrs.userContextId = 0;

?
(In reply to Jonas Sicking (:sicking) from comment #10)
> Comment on attachment 8715012 [details] [diff] [review]
> Bug_1233904.patch
> 
> Review of attachment 8715012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/Weave.js
> @@ +191,3 @@
> >      let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
> >                  .getService(Ci.nsIScriptSecurityManager);
> > +    let attrs = ChromeUtils.setDefaultUserContextId(aLoadInfo.originAttributes);
> 
> Why can't this just do:
> 
> let attrs = aLoadInfo.originAttributes;
> attrs.userContextId = 0;
> 
> ?

Again, we don't want to have manual assignments.  I originally submitted some patches like that to Bobby and he rejected it for maintenance reasons.  I think he was right, plus there's the JS -> C++ -> JS transition that forces all members to be defined and initialized with default values.
Attachment #8715012 - Flags: review?(jonas)
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
updated for new API.
Attachment #8715012 - Attachment is obsolete: true
Attachment #8722051 - Flags: review?(jonas)
Comment on attachment 8722051 [details] [diff] [review]
Bug_1233904.patch

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

::: services/sync/Weave.js
@@ +191,4 @@
>      let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                  .getService(Ci.nsIScriptSecurityManager);
> +    let attrs = ChromeUtils.originAttributesFromDict(aLoadInfo.originAttributes);
> +    attrs.userContextId = 0;

Change this to assert that userContextId is already 0. If it isn't, something very fishy is going on.
Attachment #8722051 - Flags: review?(jonas) → review-
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
removed assignment per review feedback.
Attachment #8722051 - Attachment is obsolete: true
Attachment #8722088 - Flags: review?(jonas)
Comment on attachment 8722088 [details] [diff] [review]
Bug_1233904.patch

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

::: services/sync/Weave.js
@@ +191,4 @@
>      let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                  .getService(Ci.nsIScriptSecurityManager);
> +    let attrs = ChromeUtils.originAttributesFromDict(aLoadInfo.originAttributes);
> +    let principal = ssm.createCodebasePrincipal(uri, attrs);

No need to call originAttributesFromDict here. Both because aLoadInfo.oA will already return a real OA, and because createCodebasePrincipal would accept a dict.

So just call ssm.createCodebasePrincipal(uri, aLoadInfo.oA)
Attachment #8722088 - Flags: review?(jonas) → review+
Attached patch Bug_1233904.patch (obsolete) — Splinter Review
already r+ by :sicking.  I'm incorporating the last change he requested.
Attachment #8722088 - Attachment is obsolete: true
Attachment #8726355 - Flags: review+
lot's of reds in windows 7.  let's try this one more time with newer code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee1f410e061c
looks good, ready to land.
Keywords: checkin-needed
+r by :sicking, updating the commit message to be more verbose.
Attachment #8726355 - Attachment is obsolete: true
Attachment #8733136 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3587b25bae30
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Whiteboard: [userContextId] → [userContextId][OA]
You need to log in before you can comment on or make changes to this bug.