569648 - First async XHR without other network activity has null securityInfo for the channel when using auto-detect proxy

Status: VERIFIED FIXED

(Toolkit Graveyard :: Security, defect)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 471889.

Might also affect sync.

On Windows in Firefox with a new profile:
Set Options -> Advanced -> Network -> Settings button -> Auto-detect proxy setting for network.
note: bug 471889 comment 21 states this can also happen with a PAC file.
Set Options -> General -> When Firefox starts to "Show a blank page" so no pages are opened.
Restart Firefox.
Select Help -> Check for Updates.

I'll try to come up with a test that exhibits this.

Comment 1

[Robert Strong (they/them - no direct email)]

I wasn't able to come up with a test since we need to use mochitest, the http server requires the use of a proxy, and to see the bug requires changing the proxy setting. :(

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: xpcshell test - uses an extrenal server since xpcshell doesn't support ssl

The first request fails and the second request succeeds even though they are the exact same request.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, the first issue I see in that test is that the BadCertHandler fails out the attempt to replace the original load with the one for the correct proxy.  As a result, the whole load is aborted, and of course the aborted channel didn't have any security info yet...

None of that seems like a DOM problem to me, so far.  I'm just not sure where to put BadCertHandler bugs.

Does "Check for Updates" use BadCertHandler, though?

Comment 4

[Robert Strong (they/them - no direct email)]

It uses a slightly different implementation of BadCertHandler.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/shared/CertUtils.jsm

Sorry, I filed in DOM due to other XHR bugs getting filed in DOM. If this needs to be handled by the BadCertHandler then it should go under toolkit -> security or toolkit -> app update would be fine since it is one of the consumers.

Before moving though...
In this instance there is no proxy and the client is configured to Auto-detect a proxy. I haven't verified this but I have read reports where this works fine when there is a proxy. Seems like this should be handled outside of BadCertHandler.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Sorry, I filed in DOM due to other XHR bugs getting filed in DOM.

Not a problem; it's not like you knew the issue was BadCertHandler.

> In this instance there is no proxy and the client is configured to Auto-detect
> a proxy.

Right; the way necko handles that is by starting out with the assumption that there is a proxy and then doing a fake redirect to a no-proxy channel once it discovers there isn't one.  That's the redirect that BadCertHandler aborts.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Just as a note, that fake redirect is marked as such (has the "internal redirect" flag set).

Comment 7

[Robert Strong (they/them - no direct email)]

Thank you for the explanation! It is much appreciated

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Dan, there isn't a way to add a test that I can think of (see previous comments) for this regretfully.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 448938 [details] [diff] [review]
patch rev1

r=dveditz

Comment 10

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/54b1ef730167

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with builds on OS X and Windows like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100606 Minefield/3.7a5pre.

A Litmus test will be added on bug 471889.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 448938 [details] [diff] [review]
patch rev1

Drivers, this adds a check to the bad cert handler used by app update, blocklist, and extension manager so we don't try to validate the cert when the redirect is internal (e.g. when we try to detect if there is a proxy). There is no test due to our tests requiring a proxy and this bug happens when we try to detect a proxy when there isn't one... catch 22.

Comment 13

[LegNeato]

Comment on attachment 448938 [details] [diff] [review]
patch rev1

We will not be taking this for 3.6.6. Moving approval request forward.

If you disagree, send me an email.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 448938 [details] [diff] [review]
patch rev1

I'm going to reset the request so I get the approval granted email if it is approved

Comment 15

[LegNeato]

Comment on attachment 448938 [details] [diff] [review]
patch rev1

a=LegNeato for 1.9.2.9. This needs to land by tomorrow to make it into 3.6.9

Comment 16

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a14d33d80d4e