644734 - The sync client is sending the cookie value for .mozilla.org/com on all sync requests

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Chris Lyon [:clyon]]

Looking at the sync logs from a few different sync clients, it seems that we shouldn't be sending the cookie value in all requests to the sync servers. 

I did talk with mconnor about this and suggested this: 

https://developer.mozilla.org/en/Creating_Sandboxed_HTTP_Connections

Comment 1

[Allison Naaktgeboren :ally]

Connor, is this still an issue and how should we be prioritizing it?

Comment 2

[Mike Connor [:mconnor]]

Almost certainly still an issue.  It's not a killer thing, but we really should fix it sooner rather than later.

I suspect our network wrapper makes this relatively easy to fix.  cc-ing gps for opinion.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Who set the cookies in question? It seems like the sync server shouldn't set them in the first place if you don't want to send them?

Comment 4

[Mike Connor [:mconnor]]

They're domain cookies from visiting mozilla.com or other sites that set domain cookies.

Comment 5

[Allison Naaktgeboren :ally]

Attachment: v 0 + test

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 659905 [details]
v 0 + test

+  var server = httpd_setup({"/original": original});

original should probably get a different name now, maybe just "handler"

+  var uri = CommonUtils.makeURI("http://original.com/");
+  cookieSer.setCookieString(uri, null, "test=test; path=/; domain=original.com;", null);

This needs to use localhost, not original.com. I'm not sure how domain cookies work for localhost, but it should be safe to make this a host cookie and just remove the domain=original.com; part.

@@ -143,6 +143,8 @@ AsyncResource.prototype = {

You don't have a test for AsyncResource. Looks like you should add one to http://mxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_resource_async.js ?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(I'm unclear on what RESTRequest vs AsyncResource is used for, but I trust gps will understand that part of the patch)

Comment 8

[Allison Naaktgeboren :ally]

Comment on attachment 659905 [details]
v 0 + test

no point in pestering gps for review until feedback is addressed

Comment 9

[Allison Naaktgeboren :ally]

Attachment: v1 + tests

biesi's feedback applied, resource test added.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 660242 [details] [diff] [review]
v1 + tests

+++ b/js/src/jsinterp.cpp
+    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();

I guess you don't want this change in here :)

looks good otherwise! f+

Comment 11

[Allison Naaktgeboren :ally]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #10)
> Comment on attachment 660242 [details] [diff] [review]
> v1 + tests
> 
> +++ b/js/src/jsinterp.cpp
> +    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();
> 
> I guess you don't want this change in here :)

...nope. Its a hack to get m-c with clang to compile properly. :)

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 660242 [details] [diff] [review]
v1 + tests

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

Nit: use let everywhere. Always. The only place var is allowed is inside content. And, even then it is preferred to have the compartment use a modern version of JS that supports let.

f+ because I want to see a full/as-is patch before r+.

::: js/src/jsinterp.cpp
@@ +3650,4 @@
>  BEGIN_CASE(JSOP_LEAVEFORLETIN)
>  BEGIN_CASE(JSOP_LEAVEBLOCKEXPR)
>  {
> +    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();

We need to get you using the right compiler...

::: services/common/rest.js
@@ +117,4 @@
>    response: null,
>  
>    /**
> +   * nsIRequest load flags. Don't do any caching by default. Don't  send user

Nit: whitespace

@@ +122,2 @@
>     */
> +  loadFlags: Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING | Ci.nsIRequest.LOAD_ANONYMOUS,

RESTRequest is shared by multiple modules. Adjusting the default load flags could have undesired consequences.

It is arguably silly that we set these flags by default. But, given that RESTRequest is intended for and mostly used by automated services (which typically don't use cookies and are authenticated therefore don't need a cache), I think I'll let this slide.

::: services/common/tests/unit/test_restrequest.js
@@ +777,5 @@
> +  }
> +  var server = httpd_setup({"/test": handler});
> +
> +  var cookieSer = Components.classes["@mozilla.org/cookieService;1"]
> +    .getService(Components.interfaces.nsICookieService);

Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService)

If you need to wrap:

Cc["@mozilla.org/..."]
  .getService("XXX"); // . is aligned with [

Comment 13

[Allison Naaktgeboren :ally]

Attachment: v2

While I followed directions, I'm not sure if the --no-prefix really formatted the patch in a manner that is acceptable for hg. I think the internet might be misleading me there.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 660584 [details] [diff] [review]
v2

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

Please land in inbound.

::: services/common/tests/unit/test_restrequest.js
@@ +778,5 @@
> +  let server = httpd_setup({"/test": handler});
> +
> +  let cookieSer = Components.classes["@mozilla.org/cookieService;1"]
> +                                    .getService(Components.interfaces
> +                                    .nsICookieService);

If you used Cc and Ci instead of Components.classes and Components.interfaces, respectively, you wouldn't need this crazy formatting.

Comment 15

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/541a789f3912

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/541a789f3912

Comment 17

[Richard Newman [:rnewman]]

Ally: bsmith mentioned that there might be some interaction (read: it doesn't work) between nsIRequest.LOAD_ANONYMOUS and authenticating proxies. Worth investigating?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

btw why is this a mozilla corporation bug as opposed to a security bug?

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Richard Newman [:rnewman] from comment #17)
> Ally: bsmith mentioned that there might be some interaction (read: it
> doesn't work) between nsIRequest.LOAD_ANONYMOUS and authenticating proxies.
> Worth investigating?

Bug 701019. Looks like it was fixed.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

What's the plan for opening this bug to the public? Any reason not to do it now?

Comment 21

[Richard Newman [:rnewman]]

I have no problem with opening this bug to the public.