cookies for worker

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 years ago
workers don't have access to document.cookie, but providers need cookie access.  This patch notifies the worker of cookie updates for its domain, and provides a port api for it to fetch its cookies.  This aids the worker in dealing with changes caused in browser tab content (e.g. login/logout)
(Assignee)

Comment 1

5 years ago
Created attachment 642290 [details] [diff] [review]
cookie API patch
Attachment #642290 - Flags: review?(gavin.sharp)
Comment on attachment 642290 [details] [diff] [review]
cookie API patch

I'd much rather just expose document.cookie, rather than re-implement it using a message (way too easy to get some security aspect wrong and end up leaking information we shouldn't to the provider).

But that alone wouldn't be sufficient, we also need "social.cookie-changed". That is at least a simpler addition. But it still makes me nervous and would require careful security review.

Longer term, I wonder whether a "cookie observer" web-exposed API might be the best solution.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Comment on attachment 642290 [details] [diff] [review]
> cookie API patch
> 
> I'd much rather just expose document.cookie, rather than re-implement it
> using a message (way too easy to get some security aspect wrong and end up
> leaking information we shouldn't to the provider).

Up until now we have managed to only expose functionality found in a real DOM worker to the FrameWorker - everything else is exposed via a message API.  So while we probably could use document.cookie as the implementation, it doesn't make sense (to me) to invent a new attribute/object in the Worker's global for this functionality as we might struggle to mirror that in a real worker.

The only "problem" with using document.worker as the implementation is that it means we need to let the "worker window" (or document) escape from FrameWorker.jsm, and the implementation using cookieMananger.getCookiesFromHost() seems fairly simple and hard to get wrong.  The "cookie changed" code is much more hairy and I agree it would be nice to do that better - but we probably need something like this very soon, so maybe the best step forward is just to ask for an explicit sec-review on this?  Another alternative if cookie-get survives is that the provider starts a timer and polls for cookie changes, but that sounds a little like sucking noises
(Assignee)

Comment 4

5 years ago
(In reply to Mark Hammond (:markh) from comment #3)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #2)
> > Comment on attachment 642290 [details] [diff] [review]
> > cookie API patch
> > 
> > I'd much rather just expose document.cookie, rather than re-implement it
> > using a message (way too easy to get some security aspect wrong and end up
> > leaking information we shouldn't to the provider).
> 
> Up until now we have managed to only expose functionality found in a real
> DOM worker to the FrameWorker - everything else is exposed via a message
> API.  

There are a few APIs that we expose that are not part of the W3C worker api; BlobBuilder, FileReader, Blob, and I'm not sure about localstorage.
(Assignee)

Comment 5

5 years ago
FWIW, the initial implementation did just expose document.cookie, and it was usable with polling, but I do think a notification is better than polling.
localstorage is already in Gecko's worker.  I asked about the others on #content and was told that as they intend exposing WebSocket, the others you mention will also exist.
(Assignee)

Comment 7

5 years ago
Created attachment 642634 [details] [diff] [review]
updated patch, no code changes
Attachment #642290 - Attachment is obsolete: true
Attachment #642290 - Flags: review?(gavin.sharp)
Attachment #642634 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
Created attachment 643455 [details] [diff] [review]
updated patch, no code changes
Attachment #642634 - Attachment is obsolete: true
Attachment #642634 - Flags: review?(gavin.sharp)
Attachment #643455 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

5 years ago
Created attachment 645123 [details] [diff] [review]
updated patch
Attachment #643455 - Attachment is obsolete: true
Attachment #643455 - Flags: review?(gavin.sharp)
Attachment #645123 - Flags: review?(gavin.sharp)
Comment on attachment 645123 [details] [diff] [review]
updated patch

We discussed some changes to this in person:
- use frameworker window to retrieve cookies, rather than getting them directly via nsICookieManager2
- remove cookie-changed notification for the moment, file a followup to implement with Gecko backend support
Attachment #645123 - Flags: review?(gavin.sharp)
(Assignee)

Comment 11

5 years ago
Created attachment 645361 [details] [diff] [review]
using document.cookie and fake message

I'm waiting for a build to finish before I run the test, but thought I'd post this for any feedback.
Attachment #645123 - Attachment is obsolete: true
Attachment #645361 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 12

5 years ago
Created attachment 645398 [details] [diff] [review]
using document.cookie and fake message
Attachment #645361 - Attachment is obsolete: true
Attachment #645361 - Flags: feedback?(gavin.sharp)
Attachment #645398 - Flags: review?(gavin.sharp)
(Assignee)

Comment 13

5 years ago
Created attachment 645874 [details] [diff] [review]
move message handling back to worker code, use document from worker
Attachment #645398 - Attachment is obsolete: true
Attachment #645398 - Flags: review?(gavin.sharp)
Attachment #645874 - Flags: review?(gavin.sharp)
Comment on attachment 645874 [details] [diff] [review]
move message handling back to worker code, use document from worker

Thanks, this looks a lot cleaner!

>diff --git a/toolkit/components/social/WorkerAPI.jsm b/toolkit/components/social/WorkerAPI.jsm

>+    "social.cookies-get": function(data) {

>+      cookies.forEach(function(aCookie) {
>+        let [name, value] = aCookie.split("=");
>+        results.push({name: unescape(name.trim()),
>+                      value: unescape(value.trim())});

Is unescape() really what we want here? Compared to, say, decodeURIComponent?

>diff --git a/toolkit/components/social/test/browser/browser_workerAPI.js b/toolkit/components/social

>+XPCOMUtils.defineLazyServiceGetter(Services, "cookies",
>+                                   "@mozilla.org/cookieService;1",
>+                                   "nsICookieService");
>+XPCOMUtils.defineLazyServiceGetter(Services, "cookiemgr",
>+                                   "@mozilla.org/cookiemanager;1",
>+                                   "nsICookieManager2");

Services.cookies already exists, and seems to be equivalent to the "cookiemgr" you define here. No need to import Services.jsm either, you can just assume it's been imported already for browser chrome tests.

>+XPCOMUtils.defineLazyGetter(this, "Social", function() {

This, and the runSocialTestWithProvider function that depends on it, is a browser-specific, so you can't rely on it from a toolkit test. Is it necessary to make these changes to the test?

>+  testCookies: function(next) {
>+    let provider = Social.provider;
>+    //let port = provider.workerAPI._port;

delete this

>+    Services.cookiemgr.remove('https://example.com', '/', 'cheez', false);

Why remove before you've added?

>+    Services.cookies.setCookieStringFromHttp(uri, null, null, "cheez=burger; max-age=1000", null, null);

You should be able to use  Services.cookies.add() for this?
Attachment #645874 - Attachment is patch: true
Attachment #645874 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 645906 [details] [diff] [review]
updated patch
Attachment #645874 - Attachment is obsolete: true
Attachment #645906 - Flags: review?(gavin.sharp)
Comment on attachment 645906 [details] [diff] [review]
updated patch

looks great!
Attachment #645906 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/677cdab1da6a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.