223846 - AuthName with escaped " in it sets wrong realm

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Henrik Gemal]

if you go to:
http://gemal.dk/test/javajava.html
the realm is shown as "\""

when in IE it's shown as "\"bug 119924" which is correct

http log says:
3584[abd660]:   WWW-Authenticate: Basic realm="\"bug 119924"
0[2c3f98]:   challenge=Basic realm="\"bug 119924"
0[2c3f98]: nsHttpAuthCache::GetAuthEntryForDomain [host=gemal.dk:80 realm=\]

20031024

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, nsHttpChannel::ParseRealm
(http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#2195)
needs to handle \".

ccing dwitte in case he's bored and wants to fix.. ;)

Comment 2

[timeless]

Attachment: draft

Comment 3

[timeless]

gemal, what should happen if a server sends:

WWW-Authenticate: Basic realm="\"bug 119924"; annoying="yes"

Comment 4

[Darin Fisher]

this will likely conflict with cneberg's patch for bug 180049.

timeless: i think your patch breaks digest auth:

  HTTP/1.1 401 Unauthorized
  WWW-Authenticate: Digest
          realm="[e-mail]",
          qop="auth,auth-int",
          nonce="dcd98b7102dd2f0e8b11d0f600bfb0c093",
          opaque="5ccc069c403ebaf9f0171e9517f40e41"

a better patch would loop through the chars and skip any quote that is preceded
by a backslash.

Comment 5

[Darin Fisher]

i think we should use nsIMIMEHeaderParam to parse the realm string.  doing so
should also solve bug 41489 :-)

Comment 6

[Darin Fisher]

-> default owner

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2.

* Uses |GetParameter|

Only requesting a r for this one, will request a sr if a positive review is given.

Comment 8

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

Comment on attachment 244331 [details] [diff] [review]
Patch v2

this isn't enough

you need to update PromptForIdentity to convert from UTF-8 instead of from ASCII

looks like the other uses of the realm already handle UTF-8 fine

however, I wouldn't use realmA as the name of the variable... it makes me think that the var stores ASCII. I'd use realmUTF16 or something

+    mimeHdrParser->GetParameter(nsDependentCString(challenge), "realm", EmptyCString(), PR_FALSE,
+                                    nsnull, realm);

wrong indentation on the second line here

-}
+}
\ No newline at end of file

please don't remove the newline at eof :)

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.1

Patch v2.1

(In reply to comment #8)
> (From update of attachment 244331 [details] [diff] [review] [edit])
> this isn't enough
> 
> you need to update PromptForIdentity to convert from UTF-8 instead of from
> ASCII

Fixed.

> 
> looks like the other uses of the realm already handle UTF-8 fine
> 
> however, I wouldn't use realmA as the name of the variable... it makes me think
> that the var stores ASCII. I'd use realmUTF16 or something

Fixed, now using the variable name realmUTF16.

> 
> +    mimeHdrParser->GetParameter(nsDependentCString(challenge), "realm",
> EmptyCString(), PR_FALSE,
> +                                    nsnull, realm);
> 
> wrong indentation on the second line here
> 

Fixed, now correctly indented.

> -}
> +}
> \ No newline at end of file
> 
> please don't remove the newline at eof :)
> 

Fixed, didn't mean to do that :)

Comment 10

[Darin Fisher]

Comment on attachment 244432 [details] [diff] [review]
Patch v2.1

Please wrap long lines to 80 chars.

sr=darin

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin

For check-in, same patch but word-wrapped the largest line.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp 	1.297
mozilla/netwerk/protocol/http/src/nsHttpChannel.h 	1.84

Comment 13

[(not reading, please use [e-mail] instead)]

re-opening, as I backed this out because it caused two regressions:  bug #359824 and bug #361544

Comment 14

[Robert Sayre]

HTTP authentication doesn't interoperate outside of ASCII (or Latin-1 on a good day). We should find a way to do this without converting to UTF-8.

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3

Patch v3.

* Trying a search-and-skip method for finding the quoted-string without looking directly for a quote.

Comment 16

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Unit test

Patch to add a unit test for this (thanks to biesi for his help and patience).

Comment 17

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

Comment on attachment 247742 [details] [diff] [review]
Patch v3

this doesn't do the same thing as the old code if the realm has a starting quote but no terminating quote. I don't think this is a problem, so r=biesi.

Comment 18

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 247742 [details] [diff] [review]
Patch v3

Transferring Darin's sr request to Biesi per Darin's request.

Comment 19

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

Comment on attachment 247742 [details] [diff] [review]
Patch v3

ok then, r+sr=me :)

Comment 20

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For Checkin

For Checkin

* This patch is the merged patch for the unit test and "Patch v3" with a comment removed by Biesi's request via IRC.

Comment 21

[Nickolay_Ponomarev]

patch: **** malformed patch at line 46: Index: netwerk/test/unit/test_authentication.js

With that fixed, landed:
mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp  1.304
mozilla/netwerk/test/unit/test_authentication.js     1.8