794597 - mozbrowser doesn't send auth request off a redirect

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Load http://preview.tinyurl.com/9tjkkjh

(This redirects to a site protected by basic auth.)  There's no auth prompt, just a 401 error page.

If you navigate directly to the URL, the auth prompt is shown.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, pasted the wrong link: that should be

 (1) Load http://tinyurl.com/9tjkkjh

Comment 2

[Andrew Overholt [:overholt]]

Patrick, since you fixed the HTTP authentication crash in bug 775464, can you take a look here?

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Looks like that when Http request is redirected, the LoadContext of original HttpChannelParent isn't copied to the new HttpChannelParent created for handling redirected request.

In normal request, the LoadContext is passed through IPC and is handled by HttpChannelParent::RecvAsyncOpen() when a channel is opened. However, redirecting of a request doesn't call RecvAsyncOpen() of new HttpChannelParent. I think we would need a way to copy the LoadContext to new HttpChannelParent.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Send HttpChannelParent LoadContext when the request is redirected

Hi Jason,

This patch makes HttpChannelChild send its LoadContext when HttpChannelChild::ConnectParent() is called, and HttpChannelParent can get the LoadContext after redirect. Would you help to give feedback on this patch?

Thanks

Comment 5

[Jason Duell]

Comment on attachment 666936 [details] [diff] [review]
Send HttpChannelParent LoadContext when the request is redirected

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

You've got the diagnosis of the problem right--we need to make sure the redirected channel has a Loadcontext.

I'd rather fix this by making the PHttpChannel constructor take a SerializedLoadContext.  That needs to happen for some security work anyway, it'll make sure that we don't ever miss it, and it'll also be a little cleaner, I think.

Comment 6

[Jason Duell]

Attachment: v2: fix by requiring loadContent in IPDL constructor

Like so...   I've verified that this gets us the auth prompt. 

(Sorry to steal bug Patrick--I already had some of this code written.)

jdm: if you have any doubts about reviewing this pass over to :mayhemer.

Comment 7

[Jason Duell]

Comment on attachment 668672 [details] [diff] [review]
v2: fix by requiring loadContent in IPDL constructor

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

::: netwerk/ipc/PNecko.ipdl
@@ -42,5 @@
>    HTMLDNSPrefetch(nsString hostname, uint16_t flags);
>    CancelHTMLDNSPrefetch(nsString hostname, uint16_t flags, nsresult reason);
>  
> -both:
> -  PHttpChannel(nullable PBrowser browser);

We now only receive this on the parent now, so we should have changed the IPDL file a while ago.  Grep -r and g++ appear to agree with me.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +807,5 @@
>    // until OnStopRequest, or we do a redirect, or we hit an IPDL error.
>    AddIPDLReference();
>  
> +  if (!gNeckoChild->SendPHttpChannelConstructor(
> +                      this, tabChild, IPC::SerializedLoadContext(this))) {

This is the only real semantic change in the patch--everything else is boilerplate.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 668672 [details] [diff] [review]
v2: fix by requiring loadContent in IPDL constructor

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

r- for the non-existent channel problem. I'd like to see the solution before giving this my blessing.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +56,5 @@
> +    } else {
> +      mLoadContext = new LoadContext(loadContext);
> +    }
> +  } else if (loadContext.IsPrivateBitValid()) {
> +    nsCOMPtr<nsIPrivateBrowsingChannel> pbChannel = do_QueryInterface(mChannel);

I don't think this will work - we don't create the channel until RecvAsyncOpen (and presumably this will also fail for the RecvConnectParent path as well).

Comment 9

[Jason Duell]

Attachment: v2-v3 diff

an interdiff that just shows the fix on top of v2.

Comment 10

[Jason Duell]

Attachment: v3

nice catch--thanks.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 669411 [details] [diff] [review]
v3

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +254,5 @@
>    rv = NS_LinkRedirectChannels(channelId, this, getter_AddRefs(mChannel));
>    LOG(("  found channel %p, rv=%08x", mChannel.get(), rv));
>  
> +  if (mPBOverride != kPBOverride_Unset) {
> +    // redirected-to channel may not supports PB

s/supports/support/

Comment 12

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30f9ca7f991c

typo fixed.

Will land this on aurora once inbound shows green.

Comment 13

[Jason Duell]

inbound is gree, code is e10s-only and blocking-basecamp, so no approval-mozilla-aurora? needed.

  https://hg.mozilla.org/releases/mozilla-aurora/rev/2217b510b28a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30f9ca7f991c

Comment 15

[Jason Duell]

please correct me if this is the wrong flag to set for "I landed this on aurora too"

Comment 16

[Justin Lebar (not reading bugmail)]

(In reply to Jason Duell (:jduell) from comment #15)
> please correct me if this is the wrong flag to set for "I landed this on
> aurora too"
>
> status-firefox18: --- → fixed

That's right, although I'm not sure if we also need to set status-firefox19 → fixed.  (The merge tool should do this for us now that the flag is there, maybe?)

Comment 17

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0


Works for me on Firefox 19 beta 5 (Build ID: 20130206083616) when using the link from comment 1.