Closed Bug 1221320 Opened 4 years ago Closed 4 years ago

[e10s] Login dialog with wrong XMLHttpRequest authentication

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: mccr8, Assigned: baku)

References

Details

Attachments

(2 files, 5 obsolete files)

e10s regresses bug 282547, which can be seen by locally running dom/base/test/test_bug282547.html. If an XHR supplies a user name and password, then the user should not be prompted for a user name and password.

Bug 282547 was fixed by adding some code to nsXMLHttpRequest::GetInterface(), but this code is not reached when e10s is running. I haven't looked at this in great depth, but the authentication request is spawned from nsHttpChannel, so maybe that's happening in the parent process, and it uses some generic nsIAuthPrompt for embedding, and so the XHR itself is never queried.
tracking-e10s: --- → ?
Bug 1217876 is sort of related, but it is a case where we don't get the prompt when we should.
See Also: → 1217876
Yes, the XMLHttpRequest object lives in a different process than nsHttpChannel in e10s mode.
Attached patch bar.patch (obsolete) — Splinter Review
jduell, I dont' know if you like this approach, but basically I moved all the nsXMLHttpRequest code from there to the nsHttpChannel.
The reason why e10s fails is because TabParent/Child is a nsIAuthPrompt and that is used instead nsXMLHttpRequest.
Btw, this code seems cleaner than what we had without the patch.
Attachment #8683772 - Flags: review?(jduell.mcbugs)
Assignee: nobody → amarchesini
Comment on attachment 8683772 [details] [diff] [review]
bar.patch

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

This looks good to me.  I just want to check with Patrick that he's OK with sticking in what's basically XHR-specific logic into nsHttpChannel.  (As I mention below, if there's a way to check if an HTTP channel is for XHR and only do the check if so, that would be an improvement.)

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1636,5 @@
>          }
>          break;
>      case 401:
> +    case 407: {
> +        bool skipAuthentication;

default to 'false'?

@@ +1643,5 @@
> +            return rv;
> +        }
> +
> +        if (skipAuthentication) {
> +            LOG(("ProcessAuthentication skept\n"));

skipped

@@ +7114,5 @@
> +    MOZ_ASSERT(aCanSkip);
> +    *aCanSkip = false;
> +
> +    // Verify that it's ok to prompt for credentials here, per spec
> +    // http://xhr.spec.whatwg.org/#the-send%28%29-method

It would be nice if we only did these skipAuth checks for XHR channels.  Do we have any easy way to determine if an HTTP channel is for an XHR request?  I don't know of one (are there headers that are always present, etc, that we could check?).

As is this patch changes the behavior of non-XHR channels--if they provide a username and/or password we'll now skip showing an auth prompt (maybe that's fine?)

@@ +7123,5 @@
> +
> +    // If authentication fails, XMLHttpRequest origin and
> +    // the request URL are same origin, ...
> +    /* Disabled - bug: 799540
> +    if (mState & XML_HTTP_REQUEST_USE_XSITE_AC) {

There is no mState in nsHttpChannel.  That's just carried over from cut-and-pasting from nsXMLHttpChannel.  So I think it's time to just remove this commented-out code? (If we ever need this logic or anything else based on looking at mState we'll need to copy it across IPDL from the child.  Hopefully we won't need that).

@@ +7128,5 @@
> +        *aCanSkip = true;
> +    }
> +    */
> +
> +    // ... Authorization is not in the list of author request headers, ...

I find this comment vague.  How about

  // Skip auth popup if "Authorization" is in the request headers.

@@ +7129,5 @@
> +    }
> +    */
> +
> +    // ... Authorization is not in the list of author request headers, ...
> +    if (!*aCanSkip) {

If we're removing (or will never enable) the commented-out code above, then this is always true... not sure we need the if.

@@ +7140,5 @@
> +            NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +    }
> +
> +    // ... request username is null, and request password is null,

Replace with

  // Also skip if a username and/or password is provided in the URI.
Attachment #8683772 - Flags: review?(mcmanus)
Attachment #8683772 - Flags: review?(jduell.mcbugs)
Attachment #8683772 - Flags: review+
Attached patch bar.patch (obsolete) — Splinter Review
Attachment #8683772 - Attachment is obsolete: true
Attachment #8683772 - Flags: review?(mcmanus)
Attachment #8684132 - Flags: review?(mcmanus)
Attached patch bar.patch (obsolete) — Splinter Review
The previous patch has adding the check too early in the chain. Here a better approach.
Attachment #8684132 - Attachment is obsolete: true
Attachment #8684132 - Flags: review?(mcmanus)
Attachment #8684387 - Flags: review?(mcmanus)
Blocks: 1217876
See Also: 1217876
Comment on attachment 8684387 [details] [diff] [review]
bar.patch

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

A couple of nits...

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1462,5 @@
> +    nsresult rv = mAuthChannel->GetHasAuthorizationHeader(&authorizationHeader);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (authorizationHeader) {
> +       *aCanSkip = true;

Slightly cleaner to just return NS_OK after setting to true, and then you can remove the 'if (!*canSkip)" clause below.   At least that's what :bz is always recommending me to do :)

::: netwerk/protocol/http/nsIHttpAuthenticableChannel.idl
@@ +57,5 @@
>  
>      /**
> +     * True if the request contains an Authorization header.
> +     */
> +    readonly attribute bool hasAuthorizationHeader;

Adding a new method to the IDL seems heavyweight.  Why don't you just use mAuthChannel->GetRequestHeader(Authorization)?
Attachment #8684387 - Flags: review+
Comment on attachment 8684387 [details] [diff] [review]
bar.patch

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

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1448,5 @@
>      return rv;
>  }
>  
> +nsresult
> +nsHttpChannelAuthProvider::CanSkipAuthentication(bool* aCanSkip)

Patrick: this is the place where we're putting XHR-specific logic into general HTTP channel logic.  I'm wondering if we need to limit these check to XHR channels (and if so, how easiest to identify if an http channel belongs to an XHR request).
jduell, question: the current patch breaks a couple of mochitests because it checks if we have an Authentication header.
With the previous setup we checked only if that header as part of the modified headers in XHR. This is currently impossible to do in the place touched by my patch.

Wondering if we can ignore that step, or if you have a nice solution for fixing this.
Flags: needinfo?(jduell.mcbugs)
baku:

How do the mochitests break?   Would it fix things if we only did the check on XHR channels, or do we specifically need to check whether some XHR logic has modified the Auth header in a way that only happens to certain XHR channels?

Either way it sounds like we could add some sort of attribute ('isXHR' or 'XHRModifiedAuthHeader') to nsIHTTPChannel channels, have XHR set it, send it across IPDL when we're using e10s, and then change the CanSkipAuthentication() function to only do the skip auth check if that flag is set.  Does that makes sense?  Assuming we would only set that flag before AsyncOpen time (?) we could add the new parameter to the HttpChannelOpenArgs struct in NeckoChannelParams.ipdlh.

Unless there's some other way to identify when this case is happening (for instance, if we can know whether to do the skip-auth check based on some XHR-specific headers that are only sent for XHR requests, or something like that).  I don't know XHR well enough to know if there's a way to identify XHR requests just from HTTP headers, and/or whether that would even work if we need to check only a subset of XHR channels (those that have modified the Auth header?).
Flags: needinfo?(jduell.mcbugs) → needinfo?(amarchesini)
> How do the mochitests break?   Would it fix things if we only did the check
> on XHR channels, or do we specifically need to check whether some XHR logic
> has modified the Auth header in a way that only happens to certain XHR
> channels?


We break mochitests because we don't consider the modified headers. Or better, we don't distinguish between original headers and modified ones.
Flags: needinfo?(amarchesini) → needinfo?(jduell.mcbugs)
Comment on attachment 8684387 [details] [diff] [review]
bar.patch

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

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1448,5 @@
>      return rv;
>  }
>  
> +nsresult
> +nsHttpChannelAuthProvider::CanSkipAuthentication(bool* aCanSkip)

I don't really look at this as xhr specific - they are just credentials specified out of band. It should be ok by me. Worth asking mayhemer if he has an opinion too - he's spent more time in auth.

::: netwerk/protocol/http/nsIHttpAuthenticableChannel.idl
@@ +57,5 @@
>  
>      /**
> +     * True if the request contains an Authorization header.
> +     */
> +    readonly attribute bool hasAuthorizationHeader;

i don't think we need this

::: netwerk/protocol/http/nsIHttpChannelAuthProvider.idl
@@ +24,5 @@
>   * request, checkForSuperfluousAuth MAY be called, and disconnect MUST be
>   * called.
>   */
>  
> +[scriptable, uuid(c909ef31-1f71-4745-873f-5c0f5af6021f)]

seems un-necessary
Attachment #8684387 - Flags: review?(mcmanus) → feedback+
To be more precise, we don't distinguish between modified headers (set by XHR) and 'original' headers.
I can change how we store the headers, but I need a feedback for this before changing the code.
Andrea,

So I'm still not exactly sure of the issue here.  So what's the difference between requests that have an Authorization header but shoudn't skip auth popup (the broken mochitests IIUC) and the XHR case?

If it's a matter of whether there's been a SetHeader(Auth) call, in e10s at least we do keep a list of the headers that were modified by client code (the 'requestHeaderTuples' arguments in  struct HttpChannelOpenArgs, and also in the Redirect2Verify() IPDL method.  Is that what you need to check?
Flags: needinfo?(jduell.mcbugs) → needinfo?(amarchesini)
This is where we receive and set any 'custom' headers from the child on the parent:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#447
> If it's a matter of whether there's been a SetHeader(Auth) call, in e10s at
> least we do keep a list of the headers that were modified by client code
> (the 'requestHeaderTuples' arguments in  struct HttpChannelOpenArgs, and
> also in the Redirect2Verify() IPDL method.  Is that what you need to check?

Yes. This is the point. But we don't have a similar division code in non-e10s, do we?
But the real question I guess is: Do we want to show the prompt when there is an Autorization header?
With the patch here we break all the proxy http channel tests.
Flags: needinfo?(amarchesini) → needinfo?(jduell.mcbugs)
Attached patch xhr.patch (obsolete) — Splinter Review
Attachment #8684387 - Attachment is obsolete: true
Attached file packages.pcapng
Here the network packages of this test: netwerk/test/unit/test_auth_proxy.js
as you can see, at some point we have an Authentication header and that makes my patch to break that test.
Without my patch it works because the Authentication header check is done only on the modified header of XHR (not involved in this test).
OK, from looking at the test case and the wireshark pcap, it looks like the test expects to pass in an incorrect password and that should result in a popup, but we skip if with your patch.

I'd love to get feedback from honza when he gets back in a day or two, but I suspect this would probably fix things:

1) keep the "should we skip auth?P logic in XHR (i.e don't move it to HTTP), but instead of providing a do-nothing XMLHttpRequestAuthPrompt when we want to skip, add some new method (HttpBaseChannel::SkipAuthPopups()) and call it on the channel, which will set a new mSkipAuth variable.

2) Arrange for the mSkip bool to be send across IPDL when channels are created

3) Check the mSkipAuth flag in nsHttpChannel, in the place where your patch is currently adding the check for the Auth header.

I.e. basically comment 10, but I'm not sure we need to add the SkipAuthPopups() method to nsIHttpChannelInternal.  It could just be a function in HttpBaseChannel (unless it's easier for the XHR code to deal with an IDL interface).

Does that make sense?
Flags: needinfo?(jduell.mcbugs) → needinfo?(amarchesini)
Attached patch xhr2.patch (obsolete) — Splinter Review
I totally like this approach. What about this patch?
Attachment #8689172 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8690545 - Flags: review?(jduell.mcbugs)
See Also: → 1237834
Comment on attachment 8690545 [details] [diff] [review]
xhr2.patch

Honza, can you review this? I wanted to make sure you're ok with the approach anyway (see comment 19 especially)
Attachment #8690545 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Comment on attachment 8690545 [details] [diff] [review]
xhr2.patch

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

I like this approach much more than what has been added in bug 282547 (that I would personally r- ;))

Few things to fix, tho.

::: dom/base/nsXMLHttpRequest.h
@@ +803,5 @@
>    bool mIsMappedArrayBuffer;
>  
>    void ResetResponse();
>  
> +  bool CanSkipAuthPopups();

rename to ShoudlBlockAuthPrompt

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3139,5 @@
> +
> +NS_IMETHODIMP
> +HttpBaseChannel::SetCanSkipAuthPopups(bool aValue)
> +{
> +  mCanSkipAuthPopups = aValue;

if you want to enforce this being called before AsyncOpen, we have macros to fail it.  Like ENSURE_CALLED_BEFORE_CONNECT() and others.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +474,5 @@
>  
>    bool                              mForcePending;
>    nsCOMPtr<nsIURI>                  mTopWindowURI;
>  
> +  bool                              mCanSkipAuthPopups;

put it among the bitfields we use here (see a bit above)

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1461,5 @@
> +    if (!httpChannel) {
> +        return NS_OK;
> +    }
> +
> +    nsresult rv = httpChannel->GetCanSkipAuthPopups(aCanSkip);

having GetCanSkipAuthPopups infallible, put this all code directly to the BlockPrompt() method.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +389,5 @@
> +     * This is used by XHR to inform the http about skipping the authentication
> +     * based on username/password in the URI and/or in the Authenticate
> +     * modified header. It must be set before calling AsyncOpen*.
> +     */
> +    attribute boolean canSkipAuthPopups;

I'd rather see this on nsIHttpChannelInternal and as an [infallible]

rename to blockAuthPrompt
Attachment #8690545 - Flags: review?(honzab.moz) → feedback+
> I'd rather see this on nsIHttpChannelInternal and as an [infallible]

I'm applying all your comments but I cannot make this because nsIHttpChannelInternal is not a builtinclass. Maybe it should. But I would like to do it in a follow up.
Attached patch xhr.patchSplinter Review
Attachment #8690545 - Attachment is obsolete: true
Attachment #8710354 - Flags: review?(honzab.moz)
(In reply to Andrea Marchesini (:baku) from comment #23)
> > I'd rather see this on nsIHttpChannelInternal and as an [infallible]
> 
> I'm applying all your comments but I cannot make this because
> nsIHttpChannelInternal is not a builtinclass. Maybe it should. But I would
> like to do it in a follow up.

Hmm.. I thought it was.  I think it should, please file a bug for that.  Thanks.
(In reply to Honza Bambas (:mayhemer) from comment #25)
> (In reply to Andrea Marchesini (:baku) from comment #23)
> > > I'd rather see this on nsIHttpChannelInternal and as an [infallible]
> > 
> > I'm applying all your comments but I cannot make this because
> > nsIHttpChannelInternal is not a builtinclass. Maybe it should. But I would
> > like to do it in a follow up.
> 
> Hmm.. I thought it was.  I think it should, please file a bug for that. 
> Thanks.

definitely should be.. feel free to change it to be so
Comment on attachment 8710354 [details] [diff] [review]
xhr.patch

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

r+ with the comments fixed.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +819,5 @@
> +
> +    bool skipAuthentication = false;
> +    nsresult rv = chanInternal->GetBlockAuthPrompt(&skipAuthentication);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return false;

no no.  if it fails, just ignore the error.  it should look like this:

nsresult rv = chanInternal->GetBlockAuthPrompt(&skipAuthentication);
if (NS_SUCCEEDED(rv) && skipAuthentication) {
  return true;
}

// and here the original code continues...

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +266,5 @@
> +
> +    /**
> +     * This is used by XHR to inform the http about skipping the authentication
> +     * based on username/password in the URI and/or in the Authenticate
> +     * modified header. It must be set before calling AsyncOpen*.

please don't refer XHR.  just explain what this property does in general:

"When set to true, the channel will not pop any authentication prompts up to the user.  When provided or cached credentials lead to an authentication failure, that failure will be propagated to the channel listener.  Must be called before opening the channel, otherwise throws."
Attachment #8710354 - Flags: review?(honzab.moz) → review+
Blocks: 1241565
https://hg.mozilla.org/mozilla-central/rev/ac3fd8078f4a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.