767505 - ProcessAuthentication misses error check for non-SSL CONNECT

Status: NEW

(Core :: Networking: Proxy, defect, P3)

Description

[Jason Duell]

Attachment: v1: fix logic for non-SSL CONNECT in processauth

Could result in target site forcing browser to ask for auth (but it still wouldn't see the reply with the auth info AFAICT: so not aurora/beta worthy?)

Comment 1

[Jason Duell]

Whoops--since this depends on bug 713026 and that isn't making it to aurora/beta, it's moot to consider backporting this as well.

Comment 2

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70cc47076115

Comment 3

[Ed Morley [:emorley]]

Push backed out for M5 orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d45ca69b67a3

Couldn't see you on #developers or see any try run URLs listed in-bug in the four bugs in the push, or I would have tried to find out if just part of it could have been backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/51cff2123f45

Comment 4

[Jason Duell]

Comment on attachment 635844 [details] [diff] [review]
v1: fix logic for non-SSL CONNECT in processauth

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

Patrick:  while you've got all the proxy code in your head, can you take a look at why this patch is failing?

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ -112,5 @@
>          if (!UsingHttpProxy()) {
>              LOG(("rejecting 407 when proxy server not configured!\n"));
>              return NS_ERROR_UNEXPECTED;
>          }
> -        if (UsingSSL() && !SSLConnectFailed) {

So this code rejects 407s from a non-proxy source (i.e. if CONNECT failed).  I assumed we'd want this in the non-SSL CONNECT case, too, but adding it caused all the bustage in comment 3.

Comment 5

[Patrick McManus [:mcmanus]]

ah passwordmgr 627616 - I break that one often :)

the old code was rejecting "any time we are making an SSL tunnel and got a 407 but there wasn't an error on the CONNECT".. (because origins should not be returning 407, just the proxy)

the new code is rejecting "any time get got a 407 and there wasn't a an error on CONNECT"..

the 627616 test case doesn't involve SSL or CONNECT or tunnels at all -its just a plain proxy returning a 407 but it gets caught up in that new logic.

what we want is "any time we are making a tunnel and got a 407 but there wasn't an error on the CONNECT"

Something like this fragment makes the 627616 test pass for me on my desktop:

@@ -116,20 +116,23 @@ nsHttpChannelAuthProvider::ProcessAuthen
         // credentials to an origin server.  We could attempt to proceed as
         // if we had received a 401 from the server, but why risk flirting
         // with trouble?  IE similarly rejects 407s when a proxy server is
         // not configured, so there's no reason not to do the same.
         if (!UsingHttpProxy()) {
             LOG(("rejecting 407 when proxy server not configured!\n"));
             return NS_ERROR_UNEXPECTED;
         }
-        if (UsingSSL() && !SSLConnectFailed) {
+        bool isConnect;
+        if (!ProxyConnectFailed &&
+            NS_SUCCEEDED(mAuthChannel->GetProxyMethodIsConnect(&isConnect)) &&
+            isConnect) {
             // we need to verify that this challenge came from the proxy
             // server itself, and not some server on the other side of the
-            // SSL tunnel.
+            // tunnel.
             LOG(("rejecting 407 from origin server!\n"));
             return NS_ERROR_UNEXPECTED;
         }
         rv = mAuthChannel->GetProxyChallenges(challenges);
     }
     else
         rv = mAuthChannel->GetWWWChallenges(challenges);
     if (NS_FAILED(rv)) return rv;


hope that helps.

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258