Closed
Bug 856978
Opened 11 years ago
Closed 11 years ago
Removing authentication headers via extension is broken
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: gk, Unassigned)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
2.79 KB,
application/x-xpinstall
|
Details | |
7.38 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20100101 Firefox/17.0 Steps to reproduce: We have an extensions that defends against tracking with (third party) authentication requests. See: http://jeremiahgrossman.blogspot.fr/2007/04/tracking-users-without-cookies.html for the details. It basically removes the Authorization header on http-on-modify-request. Actual results: This worked until bug 761479 landed. After that the defense is broken if the user has JavaScript allowed. Expected results: The defense should still work regardless JavaScript on/off.
Reporter | ||
Comment 1•11 years ago
|
||
I just realized that there is more to it as the fix in bug 696662 is broken as well (i have not bisected that yet but the change occurs somewhere after Firefox 17 got released), ugh. Maybe that is the root cause here although I still don't understand why only users with JS seem to be affected.
Blocks: 696662
Keywords: regression
Comment 2•11 years ago
|
||
1. I'm not sure what you exactly mean by "users with JS" ; today every browser is by default JS-enabled 2. Do I understand well that your bisection might be wrong? If so, I'll wait for a better regression window. Otherwise I may take a look if you arm me with ref to the extension.
Comment 3•11 years ago
|
||
I also suspect the culprit here could be a change in how the on-modify-request event is fired. ni? on jason who remembers this better then me.
Flags: needinfo?(jduell.mcbugs)
Reporter | ||
Comment 4•11 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2) > 1. I'm not sure what you exactly mean by "users with JS" ; today every > browser is by default JS-enabled Sure. But as our users have often/by default JsvaScript disabled due to tracking risks we have developed a test that tries to take that into account. You find it on http://ip-check.info. > 2. Do I understand well that your bisection might be wrong? Well, there are two issues here which maybe deserve two different bug entries (I am not sure about that yet) but both show that removing the authentication headers via extensions is "broken". One is that, given JS is enabled, the defense developed in the extension is not working anymore since bug 761479 landed. You might test that with the minimal test extension I attached to this bug. Just go to http://ip-check.org and watch the Authentication row in the test results. It is green without JS enabled and red with JS enabled in current Firefox versions but always green with the latest ESR. The other thing is that it seems that the authentication headers are again added _after_ http-on-modify-request is fired a thing bug 696662 "fixed" (I just looked at the logs and have not bisected that yet). Thus, we can't reliably stop the tracking anymore I mentioned in my initial comment. I have added that to this bug as IMO there is some hope that issue one might not be an issue any longer if the second one get fixed (which we need anyway).
Reporter | ||
Comment 5•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Attachment #732773 -
Attachment mime type: application/octet-stream → application/x-xpinstall
Reporter | ||
Comment 6•11 years ago
|
||
This patch fixes the regression. I ran the xpcshell tests locally and this patch did not cause a test failure. Given that the auth headers are currently not removable at all and this patch fixes this even with bug 761479, the latter should not be blocked anymore by this bug.
Attachment #748763 -
Flags: review?(jduell.mcbugs)
Comment 7•11 years ago
|
||
Comment on attachment 748763 [details] [diff] [review] This patch makes the auth headers removable again Review of attachment 748763 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to take a look at this patch as well.
Attachment #748763 -
Flags: review?(honzab.moz)
Comment 8•11 years ago
|
||
Comment on attachment 748763 [details] [diff] [review] This patch makes the auth headers removable again Review of attachment 748763 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +4419,4 @@ > // If mTimingEnabled flag is not set after OnModifyRequest() then > // clear the already recorded AsyncOpen value for consistency. > if (!mTimingEnabled) > mAsyncOpenTime = TimeStamp(); Also move this bit after CallOnModifyRequestObservers(); Otherwise looks good to me, but needs a good testing...
Attachment #748763 -
Flags: review?(honzab.moz) → review+
Comment 9•11 years ago
|
||
Comment on attachment 748763 [details] [diff] [review] This patch makes the auth headers removable again Review of attachment 748763 [details] [diff] [review]: ----------------------------------------------------------------- honza already reviewed this and knows auth code better than me. Sounds like we just need a test here.
Attachment #748763 -
Flags: review?(jduell.mcbugs)
Flags: needinfo?(jduell.mcbugs)
Reporter | ||
Comment 10•11 years ago
|
||
This improved patch takes the review comment into account add contains the needed unit test.
Attachment #748763 -
Attachment is obsolete: true
Attachment #758698 -
Flags: review?(honzab.moz)
Comment 11•11 years ago
|
||
Comment on attachment 758698 [details] [diff] [review] This patch makes the auth headers removable again -- second shot Review of attachment 758698 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab please check how the test behaves (if actually fails) when the change in http channel is not in the tree. As well please run the whole xpcshell test set in /netwerk/test (mach xpcshell-test netwerk/test). ::: netwerk/test/unit/test_bug856978.js @@ +94,5 @@ > + } else { > + do_test_pending(); > + httpServer.stop(do_test_finished); > + } > + do_test_finished(); The second do_test_finished(); call is redundant?
Attachment #758698 -
Flags: review?(honzab.moz) → review+
Reporter | ||
Comment 12•11 years ago
|
||
> please check how the test behaves (if actually fails) when the change in > http channel is not in the tree. As well please run the whole xpcshell test > set in /netwerk/test (mach xpcshell-test netwerk/test). I had done the first request already and the test is failing as there are is no auth header available on http-on-modify-request without the patch. The whole test set is passing with one exception but that is due to a test failing locally and tracked in bug 833723. > ::: netwerk/test/unit/test_bug856978.js > @@ +94,5 @@ > > + } else { > > + do_test_pending(); > > + httpServer.stop(do_test_finished); > > + } > > + do_test_finished(); > > The second do_test_finished(); call is redundant? Actually, no. The test is not passing without it. See: https://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_authentication.js#248ff. for a similar code path.
Reporter | ||
Comment 13•11 years ago
|
||
I made the patch ready for check-in. Could somebody with the necessary privs do that for me? Thanks!
Attachment #758698 -
Attachment is obsolete: true
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9c7600208a
Flags: in-testsuite+
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1e9c7600208a
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•