282547 - [XHR] Login dialog with wrong XMLHttpRequest authentication

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Eugene Prokopiev]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ru-RU; rv:1.7.2) Gecko/20040808
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ru-RU; rv:1.7.2) Gecko/20040808

I get standart login dialog instead of error in JavaScript if I use not existing
user or password in XMLHttpRequest

Reproducible: Always

Steps to Reproduce:
var endPoint = "http://localhost:8080/wsapp/services/SecureHelloWorldService";   
var httpRequest = new XMLHttpRequest();   
httpRequest.open('GET', endPoint, false, 'webdeveloper', 'webpassword');
httpRequest.send(null);
Actual Results:  
Standart login dialog

Expected Results:  
Exception in JavaScript

Comment 1

[Darin Fisher]

over to the XML component

Comment 2

[Darin Fisher]

switch to default owner

Comment 3

[Wladimir Palant]

Confirmed. Here we have behavior that is different from IE. IE only asks users
about login data if the script doesn't specify it. If the script specifies
username and password and 401 comes back, the send() call just returns without
success (no JavaScript exception). I think we should do it the same way.

Comment 4

[Wladimir Palant]

Bug 221943 might be related.

Comment 5

[Boris Zbarsky [:bzbarsky]]

darin, how would we be able to do this?  Do we have to do our own auth prompt
impl and set it on the channel before we open it if we have a username/password set?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

bz, it seems XMLHttpRequest does that already... maybe it's enough to fail the
request for nsIAuthPrompt in its getInterface implementation; but the loadgroup
may have it, so maybe it should return an authprompt that will just return
failure when asked for a prompt.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, that last seems to be the simplest, if http deals well with it.

Doron, got time to do that?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

or maybe nsIAuthPromptProvider is better (fewer functions)

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

there's also bug 277322 which should probably not be regressed...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Returning failure when asked for a prompt is no good.  HTTP will try the
loadgroup if it can't get a prompt from the channel (even if the channel has an
nsIAuthPromptProvider).

I suppose we could impl a shim nsIAuthPrompt that would just return errors for
everything...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Darin, will the suggestion in comment 10 work?

Comment 12

[Alexander]

would be any progress here?

Comment 13

[Wladimir Palant]

См. http://developer.mozilla.org/en/docs/XMLHttpRequest#mozBackgroundRequest

Comment 14

[Wladimir Palant]

Sorry about the spam, wrong tab :-(

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does the spec say anything here?

Comment 17

[fcp]

(In reply to comment #15)
> Does the spec say anything here?

Just in case, according to the current W3C working draft, a user agent must simply return the status 401 in this case without prompting the end user for a username and a password (as mentioned in bug 520148 comment #0).

http://www.w3.org/TR/2009/WD-XMLHttpRequest-20090820/#the-send-method
> [...]  If authentication fails, and request username and request password are
> provided, user agents must not prompt the end user for credentials.  [...]
> Note: End users are not prompted if credentials are provided through the
> open() API so that authors can implement their own user interface.

Comment 19

[Mark]

I was wondering what the status of this bug is. It is marked as new but appears not so new.

- The same problem occurs in Firefox 4.0b9.

- The relevant text, noted in comment #17, has not changed in the current W3C candidate XMLHttpRequest document and is included in the W3C XMLHttpRequest2 working draft.

Note: Chrome 8.0.552.237 appears to have the same behaviour as Firefox.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Mark, "new" just means it's confirmed but no one is working on it yet.

Comment 21

[Mark]

Thanks. I understand. It is a minor issue.
It is just an annoying issue because a server side script is required to hide any authorisation errors from XHR.

Comment 22

[Bram]

Is there any news on thisone? I understand it's a minor issue but it's been like this for over 6 years now (the bug was opened in 2005). I do not have access to the serverside of the API I am using but would like to hide the authentication window when a user enters wrong credentials on my addon.

Comment 23

[Adam Hooper]

I had the same problem. I happened to be using JSONP. I worked around it by using a web worker and assuming a timeout means there's a hanging password prompt somewhere. Here's my code, paraphrased in CoffeeScript for brevity:

Worker:

this.onmessage = (event) =>
  request_id = event.data.request_id
  callback_name = "callback_#{request_id}"
  # "this" here is the global scope, because we used "=>" above
  this[callback_name] = (data) =>
    delete this[callback_name]
    this.postMessage({ request_id: request_id, data: data })

  username = encodeURIComponent(event.data.username)
  password = encodeURIComponent(event.data.password)
  importScripts("https://#{username}:#{password}@example.org/path/to/api/endpoint.json?callback=#{encodeURIComponent(callback_name)}")
  # For XMLHttpRequest, send "username" and "password" as normal

And in the main script, where you'd normally do a JSONP request:

worker_request_id = 0
worker_callbacks = {}
worker = new Worker("login-worker.js")

worker.onmessage = (event) ->
  request_id = event.data.request_id
  if worker_callbacks[request_id]?
    callback = worker_messages[request_id]
    delete worker_messages[request_id]
    callback(event.data.data)

prompt_for_credentials = (callback) ->
  ... some DOM code, with, for instance, a <form> submit calling callback.apply({})

until_credentials_valid = (callback) ->
  recurse = () -> prompt_for_credentials -> until_credentials_valid(callback)

  username = # get username
  password = # get password

  request_id = "#{worker_request_id += 1}"
  worker_callbacks[request_id] = callback
  worker.postMessage({ request_id: request_id, username: username, password: password })
  window.setTimeout(->
    if worker_callbacks[request_id]?
      # Request failed; prompt for password again
      delete worker_callbacks[request_id]
      recurse()
  , 20000) # 20s timeout

(Please excuse typos.)

Comment 24

[Jim Porter (:squib)]

I think this is now necessary for the Gaia email app, since we do form-based HTTP Auth, and B2G/Gaia now supports the HTTP auth dialog, meaning we get an unnecessary popup when typing in the wrong password for our email.

I believe the corresponding bug that added the auth dialog to B2G is: https://github.com/mozilla-b2g/gaia/issues/4732

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Josh: You have been all over XHR issues lately. Could you take this one?

Comment 26

[Jim Porter (:squib)]

Sorry to be changing flags back and forth, but I found a series of magical incantations that satisfies my needs for the Gaia email app. Specifically, if I manually add the Authorization header *and* set mozAnon: true in the XHR constructor, things work as expected. This would still be nice-to-have, but I think it would only directly benefit me if bug 776171 were also fixed.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, Andrea might be able to have a look at this since it seems similar to bug 761479.

Andrea: Sounds like this isn't a high priority bug right now, but if you have extra cycles, feel free to have a look.

Comment 28

[Jim Porter (:squib)]

Right. I don't *need* this for basecamp, but if we want the Gaia email app to be a proper "open web app" instead of an "open web app that requires some Firefox-specific extensions", we'll need to get this fixed eventually.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 667456 [details] [diff] [review]
patch 1

What is the goal this patch is aiming to achieve in terms of desired behavior?

It would be good to put that into the checkin comment, in fact.

Comment 31

[Andrea Marchesini [:baku]]

With this patch, XMLHttpRequest doesn't show any prompt asking for username/password.
I don't know if this is what we want or we prefer: if (URL has Credentials) { no_prompt }

Comment 32

[Boris Zbarsky [:bzbarsky]]

Well, what does the spec say?

Comment 33

[Andrea Marchesini [:baku]]

If authentication fails, XMLHttpRequest origin and the request URL are same origin, Authorization is not in the list of author request headers, request username is null, and request password is null, user agents should prompt the end user for their username and password.

Otherwise, if authentication fails, user agents must not prompt the end user for their username and password.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: patch 2

This new patch uses a dummy auth prompt object only if the URI contains username+password.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 668028 [details] [diff] [review]
patch 2

Please remove the else-after-return and outdent that else clause.

And shouldn't we be checking at least for the Authorization header?  And possibly for the same-origin thing....

Comment 36

[Andrea Marchesini [:baku]]

Attachment: patch 2b

I think the check for the Authorization header and some-origin should be done by the nsHttpChannel. If we have this issue, we should file a new bug.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I don't think it makes any sense for nsHttpChannel to do any sort of origin checks.

I'm also not convinced it should be checking for Authorization, honestly, but I could maybe be argued into that one.  But any sort of same-origin checking needs to be in the XHR code.

Comment 38

[Andrea Marchesini [:baku]]

Attachment: patch 3

3 checks are implemented by this patch:

1. CheckMayLoad
2. authorization
3. username + password in the URI

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 668458 [details] [diff] [review]
patch 3

I'm not sure what the "on cascade" comment means.  Perhaps:

  // Verify that it's ok to prompt for credentials here, per spec

and maybe even with a spec pointer (link or section number).

Can we just use the XML_HTTP_REQUEST_USE_XSITE_AC for the same-origin bit?  Seems like we should be able to.  If we can't, please add a comment explaining why not.

s/LowerCaseEqualsASCII/LowerCaseEqualsLiteral/

We want to set showPrompt to false if either username or password is null.  So we want "||" there, not "&&".

r=me with those nits.  Thank you for sticking with this!

Comment 40

[Boris Zbarsky [:bzbarsky]]

In particular, I think for system XHR CheckMayLoad() would prevent prompts when we might want them here...

Comment 41

[Andrea Marchesini [:baku]]

Attachment: patch 3b

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1cd655dfe5

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1c1cd655dfe5

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 668820 [details] [diff] [review]
patch 3b

[Approval Request Comment]
This patch must be landed with 799540.