676358 - HTTP auth realm parameter parsed as quoted-string doesn't get unescaped

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Julian Reschke]

See 

  http://greenbytes.de/tech/tc/httpauth/#simplebasicrealmsqc

FF correctly *parses* quoted-string realm parameters, but then fails to unescape.

This is a common UA problem, right now only Chrome gets it right.

Comment 1

[Julian Reschke]

It appears that test_authentication.js has a test for this:

  response.setHeader("WWW-Authenticate", 'Basic realm="\\"foo_bar"', false);

but checks or bs-escaping not taking place:

  do_check_eq(authInfo.realm, '\\"foo_bar');

Comment 2

[--]

Attachment: nsHttpChannelAuthProvider-ParseRealm.patch

Here's a first take at changing the realm parser to skip backslash-escaped characters. I didn't update the test case yet, this is not a final version.

Comment 3

[--]

Attachment: v2-nsHttpChannelAuthProvider-ParseRealm.patch

So here's a v2 of my HTTP auth realm parser patch. Now with the test case updated accordingly.

I traced back this wonky not-unescaping-backslashes habit in the realm parser and it seems to have existed in here for a very long time.

I think my updated version behaves more in line with the RFC and Julian's test case: http://greenbytes.de/tech/tc/httpauth/#simplebasicrealmsqc

Since this will alter how Firefox shows the realm to users and to scripts (if it contains backslashed characters), is there anything more I should do?

Comment 4

[Julian Reschke]

Attachment: 676358

same as Daniel's patch, but in hg format; I also tested the patch and it appears to resolve the problem

Comment 5

[--]

Comment on attachment 8361661 [details] [diff] [review]
676358

This is the hg version of the v2 patch - I obsoleted the other patches.

Comment 6

[Jason Duell]

Comment on attachment 8361661 [details] [diff] [review]
676358

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

Good stuff.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +871,5 @@
> +                    // escaped character, store that one instead if not zero
> +                    if (!*++end)
> +                        break;
> +                }
> +                else if (*end == '\"')

I find this

  }
  else 

style thing so horrible that I encourage people to turn it into a one-line in their patches, even if it's not the predominant style in the file. Feel free to do that or not (you're not required to join all my crusades :)

Also, put { } around the else clause.  We had a long fight at Mozilla over whether we should always use {} even for one-line bodies.  The {} people won, at least for new code.  For old code I allow it, but not if a comment makes it multi-line, which is the case here.

Comment 7

[--]

I pushed it to the try server:

  https://tbpl.mozilla.org/?tree=Try&rev=e6c3d86f46d7

But I'm having trouble interpreting those 4 emulator busts at the bottom. Did my patch really cause them and if so, why? Or perhaps what should I do to make them not happen?

(this is my first push there, please bear with me!)

Comment 8

[Jason Duell]

I'm not sure why the B2G emulators are failing, but it clearly has nothing to do with your patch, so don't worry about it.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d651d6239641

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d651d6239641