Closed Bug 1240929 Opened 8 years ago Closed 8 years ago

Necko eats my HTTP headers by default during redirects

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

See the discussion in bug 1237455.

From bug 1237455 comment 54:

(In reply to :Ehsan Akhgari from comment #54)
> (In reply to Ben Kelly [:bkelly] from comment #37)
> > (In reply to :Ehsan Akhgari from comment #36)
> > > Can you also file a Necko follow-up bug to clarify what should happen in the
> > > case of other internal redirects, such as the ones used for secure upgrades?
> > > Thanks!
> > 
> > This should be automatically handled by the code making the network request
> > via the normal AsyncOnChannelRedirect() callback.  If a caller, like xhr or
> > fetch, is adding headers for normal redirects then they also add them for
> > internal redirects.  So its totally up to the behavior the creator of the
> > channel wants.  I don't think we have to do anything special for internal
> > redirects in necko.
> > 
> > Does that make sense?
> 
> Depends on what the intended API is.  I still find it very surprising that
> the default behavior in the case of redirects is to lose the request
> headers.  We definitely have quite a few call sites that do not setup
> AsyncOnChannelRedirect() callbacks to propagate the headers, so whatever the
> intention is, our code is not expecting this in many places.
> 
> <https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#550>
> <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.
> cpp#336>
> <https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.
> cpp#4487>
> 
> (Note that these are just a few example call sites I found in <1 minute.  I
> didn't do an exhastive search.)

I think that the current behavior is insane.  It basically means all Necko consumers are wrong unless if they do the right thing in OnAsyncChannelRedirect().

Jason/Patrick/Honza: what is the intended behavior here?  We should either fix Necko, or every single consumer in Gecko.
Flags: needinfo?(mcmanus)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(honzab.moz)
Summary: Necko eats my HTTP headers by default → Necko eats my HTTP headers by default during redirects
Over in bug 1237455 Ben mentioned that the above call sites are just buggy.

The specific problem (which caused me to file this) is that even a call site doesn't want to handle redirects in any way, it has to, given things such as secure upgrades.  So effectively without changing anything in Necko, *all* consumers of SetRequestHeader() need to setup an nsIChannelEventSink callback as well, which makes using Necko pretty painful.

That being said, the documentation <https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannelEventSink.idl#56> seems to suggest that this behavior is unexpected for internal redirects.  In fact, that paragraph makes it sound like AsyncOnChannelRedirect should not even be called for internal redirects...
This is one of those things that I inherited, so I don't really know the backstory on the way it was designed, or perhaps more importantly what might be sitting on top of that assumption. I tend to tread carefully with that stuff.

My assumption has always been that headers are not copied between redirected channels because of concerns that they contain origin specific information. Things analagous to cookies - tho obviously the necko code could fixup cookies in specific.

For internal redirects, I think doing the copying automatically is fine. When bkelly and I talked about it that was the patch I expected to see.

For external redirects.. I can easily see an attribute on the channel that says you want that behavior in the case of redirects. But I'm still not sure of what the default should be.

Maybe I misunderstand the root of the current approach? Its buried beyond my historical xray vision - the origin specific header is just what I've always assumed.
Flags: needinfo?(mcmanus)
Nota, I did not write the internal redirect patch because I needed the AsyncOnChannelRedirected() code to set the headers for normal redirects anyway.  Fixing that solved the problem for the internal redirects for fetch.  In the end I concluded it was all just a fetch bug with using the existing API.
(In reply to Patrick McManus [:mcmanus] from comment #2)
> This is one of those things that I inherited, so I don't really know the
> backstory on the way it was designed, or perhaps more importantly what might
> be sitting on top of that assumption. I tend to tread carefully with that
> stuff.
> 
> My assumption has always been that headers are not copied between redirected
> channels because of concerns that they contain origin specific information.
> Things analagous to cookies - tho obviously the necko code could fixup
> cookies in specific.
> 
> For internal redirects, I think doing the copying automatically is fine.
> When bkelly and I talked about it that was the patch I expected to see.
> 
> For external redirects.. I can easily see an attribute on the channel that
> says you want that behavior in the case of redirects. But I'm still not sure
> of what the default should be.

Not that I pretend to understand the reason for why things are the way they are, I think it makes perfect sense to not copy all headers for external redirects, it's the internal redirect case that I'm objecting to.

I haven't come across any call sites that are obviously broken in the case of external redirects...
Ehsan, what are you proposing to do for internal redirects?

1) Copy all headers, then call AsyncOnChannelRedirect() for listeners which may set headers again?
2) Call AsyncOnChannelRedirect() for listeners which may set headers, then copy all headers
3) Copy all headers, but do not call AsyncOnChannelRedirect()?
4) Copy all headers iff there is no AsyncOnChannelRedirect() to call?

I think its possible some consumers do things in AsyncOnChannelRedirect() that are not header related, so not sure (3) is an option.  Overall its kind of hard to tell from necko if the redirect callback is handling headers or not.  And if its not, it might be purposeful.

Of the examples you reference, how many of those would not need to handle their header modifications on external redirects anyway?  In other words, how many forbid external redirects but allow internal redirects?
Flags: needinfo?(ehsan)
(In reply to Ben Kelly [:bkelly] from comment #5)
> Ehsan, what are you proposing to do for internal redirects?
> 
> 1) Copy all headers, then call AsyncOnChannelRedirect() for listeners which
> may set headers again?
> 2) Call AsyncOnChannelRedirect() for listeners which may set headers, then
> copy all headers
> 3) Copy all headers, but do not call AsyncOnChannelRedirect()?
> 4) Copy all headers iff there is no AsyncOnChannelRedirect() to call?
> 
> I think its possible some consumers do things in AsyncOnChannelRedirect()
> that are not header related, so not sure (3) is an option.  Overall its kind
> of hard to tell from necko if the redirect callback is handling headers or
> not.  And if its not, it might be purposeful.

I have always been under the impression that AsyncOnChannelRedirect() is fired for two reasons:

* In order to give the consumer a chance to cancel a redirect.
* In order to let them manipulate the new channel before it's opened (for example in order to set request headers)

It seems to me that internal and upgrade redirects should not be subject to either of the above.  It makes no sense to let the consumer disallow an internal redirect from happening, and since the redirect is not a real one, there is no reason why a consumer should want to manipulate the new channel.

Now, I don't know how close to reality my understanding has been.  The comments in nsIChannelEventSink.idl and this check <https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#69> seem to sort of agree with me (at least on it making no sense to veto an internal redirect) but at any rate I think a Necko peer is better able to pick a solution.

Also note that no matter what solution we pick, it's possible that we'd need to change code on both sides (Necko and its consumers) since at least it's quite clear that the two don't currently agree on a consistent set of semantics.

> Of the examples you reference, how many of those would not need to handle
> their header modifications on external redirects anyway?  In other words,
> how many forbid external redirects but allow internal redirects?

Of the three call sites that I looked at in comment 0, neither seem to provide an AsyncOnChannelRedirect() callback at all, so none of them forbid external redirects, and all of them do the wrong thing for all redirects as a result.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #6)
> I have always been under the impression that AsyncOnChannelRedirect() is
> fired for two reasons:
> 
> * In order to give the consumer a chance to cancel a redirect.
> * In order to let them manipulate the new channel before it's opened (for
> example in order to set request headers)

There is a third one: to update any internal mChannel references to the new channel.  Yeah, crazy, I know.
Flags: needinfo?(honzab.moz)
However, to answer the ni? request: I think for internal redirects we should copy headers.  For external don't.  But call the callback all the time.  It's important.
That sounds like a fine solution to me.
I'd like to talk to biesi to see if he remembers the reason for this.

> for internal redirects we should copy headers.  

Sounds like we want to not copy Cookie header, I assume also Authorization, and anything else origin-related.  Honza, do you have a list of specific headers we want to not copy?
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Maybe just copy-all iff same-URL-internal and copy-origin-non-specific (= not cookies and not auth, maybe others, would have to think) on hsts update?
Flags: needinfo?(honzab.moz)
And then AsyncOnChannelRedirect() adds duplicate headers for internal redirect?

How about letting consumers flag headers that should be copied-on-redirect when they call SetRequestHeader()?  Then necko can just respect that flag for all redirects consistently.
Blocks: 1243453
(In reply to Ben Kelly [:bkelly] from comment #12)
> And then AsyncOnChannelRedirect() adds duplicate headers for internal
> redirect?

Or fix them to not do that.

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Maybe just copy-all iff same-URL-internal and copy-origin-non-specific (=
> not cookies and not auth, maybe others, would have to think) on hsts update?

Err, why would we not copy the headers on HSTS upgrade?  Surely we don't want the user typing in "facebook" + Ctrl/Enter to show the login page but "https://www.facebook.com/" to show the Facebook home page?
(In reply to :Ehsan Akhgari from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #11)
> > Maybe just copy-all iff same-URL-internal and copy-origin-non-specific (=
> > not cookies and not auth, maybe others, would have to think) on hsts update?
> 
> Err, why would we not copy the headers on HSTS upgrade?  Surely we don't
> want the user typing in "facebook" + Ctrl/Enter to show the login page but
> "https://www.facebook.com/" to show the Facebook home page?

I actually may not be the best person to ask on this.  It's more about security concerns.

However, HSTS update is considered a (special case) internal redirect, so yes, we are probably OK to copy all.  There is no risk of loosing data since we go from HTTP to HTTPS and the request should be considered "the same".
One point of confusion for me is, in the HSTS case what should the Origin header contain?  The original http:// origin or the new https:// origin?  If its the new value, then its not a copy of the headers from the original channel.
Patrick, we're in serious need of addressing this in 47.  Is this something that the Necko team can fix (pretty please :)?
Flags: needinfo?(mcmanus)
We anyway need some white or black list.  We cannot copy e.g. If- cache headers.  As well as *-Authorization and probably Cookie.  Those will be put on the channel again using the default mechanisms.  Prefilled may cause behavioral changes of the target channel (expected).  This just comes from top of my head, but sooner or later we may find out we only have to copy any custom headers (non-standard) or any of the previously mentioned headers when set prior the "default mechanism to add them" were engaged.  E.g. you can set If- cache headers prior opening the channel.  However, it's a bit disputable if exactly those should be copied as well, since those probably deal with a different (http:) resource while we would carry them to work with https: resource (a different one).

One of possible cases... there might be more.  Maybe add an API setHeaderAndCopyOnRedirect?  Sounds odd to me...  But may prevent any white/black lists and other copy/don't copy logic hacks.
(In reply to Honza Bambas (:mayhemer) from comment #17)
> One of possible cases... there might be more.  Maybe add an API
> setHeaderAndCopyOnRedirect?  Sounds odd to me...  But may prevent any
> white/black lists and other copy/don't copy logic hacks.

I'd take setHeaderAndCopyOnRedirect over magical white-list/black-lists any day of the week. :-)
who is supposed to call setHeaderAndCopyOnRedirect? The channel caller in a million places?

I think we can just do the right thing silently here for internal redirects - and this is still about internal redirects, right?

So anything that is origin specific should be stripped and the others copied. References to cache entries, cookies, auth should be stripped.. honestly if we get it 95% right I think we're doing better than right now.

as for the origin header, that's not about the origin of this transaction its about the origin of the transaction that caused it to be created.. and therefore it shouldn't be changed when this transaction is modified.. right?

honza will you take this work on?
Flags: needinfo?(mcmanus) → needinfo?(honzab.moz)
(In reply to Patrick McManus [:mcmanus] from comment #19)
> who is supposed to call setHeaderAndCopyOnRedirect? The channel caller in a
> million places?

I am not exactly sure what headers wants Eshan be copied.  I somehow tend to give him this API to put his special headers on the channel with it.  Simple and clear.

> 
> I think we can just do the right thing silently here for internal redirects
> - and this is still about internal redirects, right?

HSTS is treated as internal redir, yes, regardless it has its own flag.

> 
> So anything that is origin specific should be stripped and the others
> copied. References to cache entries, cookies, auth should be stripped..
> honestly if we get it 95% right I think we're doing better than right now.

=black list.

> 
> as for the origin header, that's not about the origin of this transaction
> its about the origin of the transaction that caused it to be created.. and
> therefore it shouldn't be changed when this transaction is modified.. right?

yup.

> 
> honza will you take this work on?

what can I do ;)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-active]
Attached patch v1 (obsolete) — Splinter Review
Attachment #8738553 - Flags: review?(mcmanus)
Attached patch v2Splinter Review
Ups, forgot to do this only for internal/sts redirs!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aaf0751f4a3
Attachment #8738553 - Attachment is obsolete: true
Attachment #8738553 - Flags: review?(mcmanus)
Attachment #8738612 - Flags: review?(mcmanus)
Comment on attachment 8738612 [details] [diff] [review]
v2

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

thank you for doing this!
Attachment #8738612 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f81406821d56
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.