646628 - Firefox Basic Auth interacts badly with Sync

Status: NEW

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

If you hit your own Sync URL (e.g., /info/collections), and enter a wrong username, you'll get a response of "5" -- user doesn't match URI.

Firefox will remember the bad credentials. That doesn't seem right.

Sync will now be broken until you restart -- every request gets a 400, presumably because we're adding an Authorization header and so is Firefox.

I haven't dug into this, but those are the symptoms.

Trivial log:

1301519766478	Net.Resource	DEBUG	GET fail 400 https://phx-sync223.services.mozilla.com/1.0/holygoat/info/collections
1301519766640	Service.Main	CONFIG	Starting backoff, next sync at:Wed Mar 30 2011 16:10:11 GMT-0700 (PDT)
1301519766640	Service.Main	DEBUG	Exception: aborting sync, failed to get collections No traceback available

Comment 1

[Richard Newman [:rnewman]]

See also Bug 201620. Yay really old bugs.

Sync sends a 400 if your auth credentials don't match the URI, so Firefox caches them.

It then adds the bad ones *after* Sync adds the header:

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

Simply adding

    channel.loadFlags |= Ci.nsIRequest.LOAD_ANONYMOUS;   // So we don't do auth.

to _createRequest in resource.js will avoid that chunk of code, at the cost of skipping proxy auth. I don't know if that's necessary.

Broadly speaking, it would be nice if Firefox's auth handling (a) didn't suck, (b) could be skipped programmatically.

Comment 2

[Philipp von Weitershausen [:philikon]]

I remember the Web Developer extension allows you to clear Firefox's BasicAuth header cache for the selected tab. I'm sure we could lift that code and transplant it into our BasicAuth authenticator?

Comment 3

[Mike Connor [:mconnor]]

Except that's all or nothing.  Logging out all http auth sessions would be pretty poor form.

Comment 4

[Richard Newman [:rnewman]]

(In reply to comment #2)
> I remember the Web Developer extension allows you to clear Firefox's BasicAuth
> header cache for the selected tab. I'm sure we could lift that code and
> transplant it into our BasicAuth authenticator?

Probably. Seems like an awful hack, though; I'd rather have a load flag that says "get out of my face, crappy platform code assumptions" and skip all those calls in the HTTP channel.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> Except that's all or nothing.  Logging out all http auth sessions would be
> pretty poor form.

Oh I thought it allowed you ditch just that one session. Never midn then.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> Probably. Seems like an awful hack, though; I'd rather have a load flag that
> says "get out of my face, crappy platform code assumptions" and skip all those
> calls in the HTTP channel.

+1 Make it so.

Comment 7

[Honza Bambas (:mayhemer)]

Hasn't this been fixed with one of bug 654348 or bug 761479 or bug 776171?

Comment 8

[Richard Newman [:rnewman]]

Possibly. We don't use XHR, so if the change to nsHttpChannelAuthProvider applies to direct channel usage, then maybe…

Comment 9

[Honza Bambas (:mayhemer)]

:rnewman - where is the code that does the sync?  if you are not using xhr, then what?

I personally don't agree with bug 646686 at all.

Currently cached credentials are removed from cache on 401/407.  Returning 400 (I understand why you are not returning 401) doesn't delete it.

I have few questions:
- how the sync auth code works?  how are you adding creds to the request?
- more question for my self: how it happens that the creds get cached at all?

Comment 10

[Richard Newman [:rnewman]]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> :rnewman - where is the code that does the sync?

/services/sync/modules/
/services/common/

In particular, here're we're talking about RESTRequest and AsyncResource.

> if you are not using xhr, then what?

nsIHttpRequest/Channel/etc. under the covers.

> Currently cached credentials are removed from cache on 401/407.  Returning
> 400 (I understand why you are not returning 401) doesn't delete it.

The bug is not really that credentials are not being removed from a cache; it's that there's no way to stop Necko from adding its own credentials, even when the caller specifies their own.

Sync (and other automated clients built into Firefox, like FHR) wants an HTTP request framework that it can muck around with, not an HTTP request that Firefox is going to chop up with its own auth/cert/etc. manipulators without any way to control them.

One could narrow the fix down to "don't overwrite an Authorization header that's present in a request", but that doesn't work for applications that are using some other auth mechanism but want to avoid Necko "form-filling" a Basic Auth header on their behalf.


> I have few questions:
> - how the sync auth code works?  how are you adding creds to the request?

request.addHeader, including in redirect listeners.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Richard Newman [:rnewman] from bug 646686 comment #9)
> Maybe the solution is to allow nsIHttp* users to easily replace the
> nsHttpChannelAuthProvider for their channels? Is that possible?

Hmm..  According how complicated and sensitive nsHttpChannelAuthProvider is, I would rather not do that.  This class exists only because we needed to share this code by httpchannels and websockets.

However, to allow a w/o hook might be a good idea.

(In reply to Richard Newman [:rnewman] from comment #10)
> In particular, here're we're talking about RESTRequest and AsyncResource.

Thanks, useful.

> The bug is not really that credentials are not being removed from a cache;
> it's that there's no way to stop Necko from adding its own credentials, even
> when the caller specifies their own.

That's true, we don't check "custom" auth headers, we just put our own.

> One could narrow the fix down to "don't overwrite an Authorization header
> that's present in a request", but that doesn't work for applications that
> are using some other auth mechanism but want to avoid Necko "form-filling" a
> Basic Auth header on their behalf.

Aha.. good point.

> request.addHeader, including in redirect listeners.

You mean setRequestHeader? http://hg.mozilla.org/mozilla-central/annotate/eccf45749400/services/common/rest.js#l700 ---> http://hg.mozilla.org/mozilla-central/annotate/eccf45749400/services/sync/modules/resource.js#l183


You might persuade me that simplest solution here would be to just tell the channel not to auth via HTTP auth with the target server.

In case we provide a hook to the auth provider, what API would work for you best?

Comment 12

[Richard Newman [:rnewman]]

(In reply to Honza Bambas (:mayhemer) from comment #11)

> You mean setRequestHeader?

Yeah. Sorry, going from memory.

> You might persuade me that simplest solution here would be to just tell the
> channel not to auth via HTTP auth with the target server.

That's exactly the proposal in Bug 646686 — add load flags to individually (a) turn off built-in HTTP auth, (b) turn off cookies, without going into full-anonymous mode.

> In case we provide a hook to the auth provider, what API would work for you
> best?

Load flags on the channel are something we already use, so that's the simplest case for Sync. But we can do pretty much anything.