Open Bug 767505 Opened 8 years ago Updated 2 years ago

ProcessAuthentication misses error check for non-SSL CONNECT


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





(Reporter: jduell.mcbugs, Unassigned)



(Whiteboard: [necko-backlog])


(1 file)

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)
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+
Push backed out for M5 orange:

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.
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);
         rv = mAuthChannel->GetWWWChallenges(challenges);
     if (NS_FAILED(rv)) return rv;

hope that helps.
Whiteboard: [necko-backlog]
Bulk change to priority:
Priority: -- → P1
Bulk change to priority:
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.