If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Load flag to disable built-in Basic Auth

NEW
Unassigned

Status

()

Core
Networking: HTTP
P5
normal
7 years ago
11 days ago

People

(Reporter: rnewman, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Bug 646628 describes a situation in which user activity can put Sync into a non-working state, because the Authorization header set by Sync is apparently overwritten by this code:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#221

Setting the LOAD_ANONYMOUS load flag avoids the problem, but is a sledgehammer for this issue. It would be much nicer if there was a smaller hammer.

Attached momentarily will be a trivial, untested patch which demonstrates the idea.

Thoughts, necko people?
How about changing the code not to set an Authorization header if one is present already, unless we tried it and it failed?
(Reporter)

Comment 2

7 years ago
Created attachment 523171 [details] [diff] [review]
Sketch implementation.
Attachment #523171 - Flags: feedback?
(Reporter)

Comment 3

7 years ago
(In reply to comment #1)
> How about changing the code not to set an Authorization header if one is
> present already, unless we tried it and it failed?

That's a reasonable alternative (though I don't think there's any advantage in, e.g., attempting to use the built-in credentials if the ones we supplied didn't work -- we'd rather just get the 401).

The main thing is that we have some way to not have our headers overwritten! :)
Hasn't this been fixed with one of bug 654348 or bug 761479 or bug 776171?
(Reporter)

Comment 5

5 years ago
Without thoroughly studying those bugs, I don't think so.

* This is not calling through XHR.
* This bug is about the smallest possible hammer: LOAD_ANONYMOUS/mozAnon are very big hammers. Several people think that offering LOAD_NOCOOKIES, LOAD_NOAUTH, etc. would provide more flexibility.

Discussion about this is occurring on dev-platform:

https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/pxBCKzxBMDA
Comment on attachment 523171 [details] [diff] [review]
Sketch implementation.

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

::: netwerk/base/public/nsIRequest.idl
@@ +223,5 @@
> +    /**
> +     * When set, this flag indicates that built-in HTTP Basic Auth should not
> +     * be attempted for this request.
> +     */
> +    const unsigned long LOAD_BYPASS_BASIC_AUTH = 1 << 15;

<<15 already taken by LOAD_FRESH_CONNECTION.

As discussed on IRC with bz, we only currently have one bit free (1<< 24). So it's time to suck it up and turn the definition of nsLoadFlags (in nsIRequest.idl) into 'unsigned long long'. That'll probably mean a lot of tedious changing of code in C++ that uses the API just to change the type.
I've filed bug 846629 specifically for switching the loadFlags to 64 bit. Then we can add the LOAD_BYPASS_BASIC_AUTH here

(Are you sure you only want to disable basic auth, and not all kinds of auth? NTLM, etc?)
Attachment #523171 - Flags: feedback? → feedback-
Depends on: 846629
Comment on attachment 523171 [details] [diff] [review]
Sketch implementation.

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

Honza (or maybe jdm) is the right person to ask about whether the short-circuit in nsHttpChannelAuthProvider is the right thing to do.
Attachment #523171 - Flags: feedback- → feedback?(honzab.moz)
With what I know at the moment, I'm personally pretty against this bug.  Please see bug 646628 comment 9 first.
(Reporter)

Comment 9

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #8)
> With what I know at the moment, I'm personally pretty against this bug. 
> Please see bug 646628 comment 9 first.

See my response there; in general, my assertion is that tightly coupling Firefox's auth system with its HTTP system is opposed to the idea of service client libraries; there needs to be a way for Necko users to prevent other layers of the stack from mucking around with requests.

Maybe the solution is to allow nsIHttp* users to easily replace the nsHttpChannelAuthProvider for their channels? Is that possible?
Comment on attachment 523171 [details] [diff] [review]
Sketch implementation.

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

::: netwerk/base/public/nsIRequest.idl
@@ +223,5 @@
> +    /**
> +     * When set, this flag indicates that built-in HTTP Basic Auth should not
> +     * be attempted for this request.
> +     */
> +    const unsigned long LOAD_BYPASS_BASIC_AUTH = 1 << 15;

First, call this BYPASS_*BASIC*_AUTH is bad.  There are several other schemes for http auth.

This has to be named BYPASS_WWW_AUTHENTICATION.  We still want to auth with proxies, but the ANON flag already handles this.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +215,5 @@
>                                 mProxyIdent);
>  
> +    if (loadFlags & nsIRequest::LOAD_BYPASS_BASIC_AUTH) {
> +        return NS_OK;
> +    }

When the server returns 401 for your provided credentials, we will try another round with cached credentials.

Probably not what you and we want.

My proposal is to:
- make LOAD_ANONYMOUS flag consist of the finer grained flags, where disable LOAD_WWW_AUTHENTICATION is one of them
- replace LOAD_ANONYMOUS in this file with LOAD_WWW_AUTHENTICATION flag checks


BTW, what cs is this patch based on?  On this place in the current code has to be check for the LOAD_ANONYMOUS flag..  The parent changes seems to be from Wed Mar 30 13:53:35 2011 !
Attachment #523171 - Flags: feedback?(honzab.moz) → feedback-
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.