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

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

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.

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?

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

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 ?

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

Comment on attachment 659905 [details] v 0 + test no point in pestering gps for review until feedback is addressed

biesi's feedback applied, resource test added.

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+

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

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

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

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

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

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

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