ProcessAuthentication misses error check for non-SSL CONNECT

NEW
Unassigned

Status

()

Core
Networking: HTTP
P3
normal
6 years ago
6 months ago

People

(Reporter: jduell, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment)

Created attachment 635844 [details] [diff] [review]
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?)
Attachment #635844 - Flags: review?(mcmanus)
(Reporter)

Comment 1

6 years ago
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.
Blocks: 767516
Depends on: 713026
Attachment #635844 - Flags: review?(mcmanus) → review+

Comment 3

6 years ago
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
(Reporter)

Comment 4

6 years ago
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.
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.
Whiteboard: [necko-backlog]
You need to log in before you can comment on or make changes to this bug.