Closed Bug 1300642 Opened 3 years ago Closed 3 years ago

User-Agent header can be edited but not deleted.

Categories

(Core :: Networking: HTTP, defect, P5)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: xavierholt, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged [necko-active])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

I was experimenting with the WebExtension API, and wrote a simple function to drop headers from HTTP requests using the onBeforeSendHeaders listener (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebRequest/onBeforeSendHeaders).


Actual results:

When I edited the User-Agent header (I pretended to be cURL), everything worked as expected.  I inspected the outgoing headers in three locations, and saw cURL listed as the User-Agent in all three:
 - An onSendHeaders listener,
 - The in-page network console,
 - And Wireshark.

But when I removed the User-Agent header, Firefox re-added it before sending the request.  My listeners gave inconsistent results:
 - The onSendHeaders listener did NOT see the header,
 - The page's network console DID see it,
 - And Wireshark DID see it.


Expected results:

It seems like this header should be provided by default, but deletable just as it's editable.  Its presence is only a SHOULD in the RFC, not a MUST:  https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.43

Out of curiosity, I tweaked my extension to remove ALL headers just to see which ones would be re-added.  I found (and there may be more I didn't encounter):
 - DNT
 - User-Agent
 - Connection
 - If-Modified-Since
 - If-None-Match
 - Cache-Control

There may be another bug here:  onBeforeSendHeaders and onSendHeaders don't get to see the actual list of headers sent.  Let me know if I should file that, too!
Just saw this in the docs (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebRequest/onBeforeSendHeaders) in reference to the cache/performance-related headers, which more or less makes sense:

> Not all headers actually sent are always included in requestHeaders. In particular, headers related to caching (for example, Cache-Control, If-Modified-Since, If-None-Match) are never sent. Also, behavior here may differ across browsers.

It still seems like a bug that User-Agent (and DNT?) get(s) special treatment, though.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Summary: User-Agent header can be edited but not deleted. → [WebExtensions] User-Agent header can be edited but not deleted.
Component: WebExtensions → WebExtensions: Request Handling
Priority: -- → P5
Whiteboard: triaged
This behavior is baked into the network stack, intentionally, so we probably shouldn't try to circumvent it.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Can you elaborate on this?  If it was done "intentionally" then I'd like to understand the problem it solves.
I'm not the person to ask. I'll move the bug to Networking and re-open. Someone there should have a better answer.
Status: RESOLVED → REOPENED
Component: WebExtensions: Request Handling → Networking: HTTP
Ever confirmed: true
Product: Toolkit → Core
Resolution: WONTFIX → ---
Summary: [WebExtensions] User-Agent header can be edited but not deleted. → User-Agent header can be edited but not deleted.
Thanks!  Interested to hear what they say.
What is the use case for deleting the UA header completely?
I don't know where from exactly (at which point during the channel lifetime) the onBeforeSendHeaders is called, so hard to decide when exactly we re-add the UA header.  You need to ask someone from the WebExtensions team, Kris?  If you don't know, please help finding someone who does.

Then we can find someone from Necko to put some time to find out where from we re-add the UA header and potentially consider doing some changes.

But I don't see a defect here to fix.  As well as I don't understand the use case, maybe except some privacy aspects.
Status: REOPENED → NEW
Flags: needinfo?(kmaglione+bmo)
Yep - the use case I had in mind was a privacy / anonymity one.  That header provides zero information the server needs to know about (we all speak standard HTML these days, right?), and is basically just a big fingerprint.

Editing the UA string to some generic identifier would also work, but that's another potential fingerprint.  And yeah, deleting the thing is ALSO a fingerprint, but it seems like the correct path for privacy-minded folks who think the end game should be deprecating the header completely.

Unless there's some legit server-side use I don't know about?
We call the extension header observers from an http-on-modify-request observer in the child process.

I haven't looked into this in much detail, but looking at it again now, it seems like the problem is that we clear the header in the child channel, but when the headers are copied to the parent, there's no empty User-Agent header to copy, so the initial default value from the parent channel remains in place.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #9)
> We call the extension header observers from an http-on-modify-request
> observer in the child process.
> 
Yes, onBeforeSendHeaders listeners are called from an http-on-modify-request observer, but are you sure it's in the child process? I believed we were using http-on-* observers in the *parent* process for almost anything, except non-HTTP URLs which are handled by a nsIContentPolicy, usually in the child process.

Is it even possible to register an http-on-modify-request in the child these days, after bug 806753 (see https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverService.cpp#208 )?
Looking again, you're right, that observer is in WebRequest.jsm, which is in the main process.
What I believe is actually happening, from https://dxr.mozilla.org/mozilla-central/rev/26f2e65163e9f3cfab071a9823293217b1e1de8d/netwerk/protocol/http/nsHttpChannel.cpp#5891 on:

1. webRequest.onBeforeSendHeaders listeners modify the User-Agent header in onCallOnModifyRequest();
2. Immediately after that nsHttpChannel::SetLoadGroupUserAgentOverride() reverts it back to its default value, or the override provided by "http-on-useragent-request" observers, because it special-cases an empty user agent in https://dxr.mozilla.org/mozilla-central/rev/26f2e65163e9f3cfab071a9823293217b1e1de8d/netwerk/protocol/http/nsHttpChannel.cpp#8160

In order to work-around this, we could either 
a) Store a special webRequest.userAgentOverride property in the channel if onBeforeSendHeaders sets its custom User-Agent header and force it if found in setLoadGroupUserAgentOverride(). 
b) ... or introduce non-Chrome-compatible webRequest.onUserAgentOverride listener to explicitly wrap an "http-on-useragent-override" observer and participate into this mechanism. 

It basically depends if we want to give WebExtensions priority over it or not, I guess.
Maybe swapping CallOnModifyRequestObservers(); and SetLoadGroupUserAgentOverride(); would do the trick.  

Can somebody test it?

It has been added in bug 1148544.  The reason why it's made after onmodifyrequest is not stated in the code (no comments...!) nor in the bug...

Dylan, as that patch author, what was the reason to put SetLoadGroupUserAgentOverride after CallOnModifyRequestObservers?
Flags: needinfo?(droeh)
I don't *think* that switching the order of CallOnModifyRequestObservers() and SetLoadGroupUserAgentOverride() will break anything from my patch. As for the order of the calls in my patch, I don't think the case of deliberately sending an empty UA occurred to me, so it did not seem relevant.
Flags: needinfo?(droeh)
Thanks Dylan.  I think we try to swap those two lines and see what happens.

I can write the patch.  But, who can test this?
Assignee: nobody → honzab.moz
Whiteboard: triaged → triaged [necko-active]
Attachment #8811342 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5702ad092400
Set default UA request header before on-opening-request notification to allow its override. r=mcmanus
Keywords: checkin-needed
We also call SetDocshellUserAgentOverride() in HttpChannelChild, after calling OnOpeningRequest()
Do we also need to change it there?
Flags: needinfo?(honzab.moz)
https://hg.mozilla.org/mozilla-central/rev/5702ad092400
Status: NEW → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Valentin Gosu [:valentin] from comment #18)
> We also call SetDocshellUserAgentOverride() in HttpChannelChild, after
> calling OnOpeningRequest()
> Do we also need to change it there?

We don't to fix this bug, but for consistency sake, we probably should.
Flags: needinfo?(honzab.moz)
Blocks: 1318722
You need to log in before you can comment on or make changes to this bug.