AuthName with escaped " in it sets wrong realm

RESOLVED FIXED

Status

()

RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: bugzilla, Assigned: sciguyryan)

Tracking

({helpwanted})

Trunk
helpwanted
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

15 years ago
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
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.. ;)
OS: Windows XP → All
Hardware: PC → All

Comment 3

15 years ago
gemal, what should happen if a server sends:

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

Comment 4

15 years ago
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="testrealm@host.com",
          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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6beta

Updated

15 years ago
Attachment #134255 - Flags: review-

Comment 5

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

Updated

15 years ago
Target Milestone: mozilla1.6beta → mozilla1.7alpha

Updated

15 years ago
Target Milestone: mozilla1.7alpha → mozilla1.7beta

Updated

15 years ago
Target Milestone: mozilla1.7beta → mozilla1.8alpha

Updated

14 years ago
Keywords: helpwanted
Whiteboard: [good first bug]
Target Milestone: mozilla1.8alpha1 → Future

Comment 6

12 years ago
-> default owner
Assignee: darin → nobody
Status: ASSIGNED → NEW
Component: Networking: HTTP → Networking
QA Contact: networking.http → networking
Target Milestone: Future → ---
(Assignee)

Comment 7

12 years ago
Created attachment 244331 [details] [diff] [review]
Patch v2

Patch v2.

* Uses |GetParameter|

Only requesting a r for this one, will request a sr if a positive review is given.
Attachment #244331 - Flags: review?(cbiesinger)
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 :)
Attachment #244331 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 9

12 years ago
Created attachment 244432 [details] [diff] [review]
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 :)
Assignee: nobody → sciguyryan+bugzilla
Attachment #134255 - Attachment is obsolete: true
Attachment #244331 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #244432 - Flags: review?(cbiesinger)
Attachment #244432 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

12 years ago
Attachment #244432 - Flags: superreview?(darin.moz)

Comment 10

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

Please wrap long lines to 80 chars.

sr=darin
Attachment #244432 - Flags: superreview?(darin.moz) → superreview+
(Assignee)

Comment 11

12 years ago
Created attachment 244632 [details] [diff] [review]
For checkin

For check-in, same patch but word-wrapped the largest line.
Attachment #244432 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Whiteboard: [good first bug] → [good first bug] [checkin needed]
mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp 	1.297
mozilla/netwerk/protocol/http/src/nsHttpChannel.h 	1.84
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug] [checkin needed]
re-opening, as I backed this out because it caused two regressions:  bug #359824 and bug #361544
Status: RESOLVED → REOPENED
Depends on: 361544
Resolution: FIXED → ---

Comment 14

12 years ago
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.
(Assignee)

Comment 15

12 years ago
Created attachment 247742 [details] [diff] [review]
Patch v3

Patch v3.

* Trying a search-and-skip method for finding the quoted-string without looking directly for a quote.
Attachment #244632 - Attachment is obsolete: true
Attachment #247742 - Flags: superreview?(darin.moz)
Attachment #247742 - Flags: review?(cbiesinger)
(Assignee)

Comment 16

12 years ago
Created attachment 253012 [details] [diff] [review]
Unit test

Patch to add a unit test for this (thanks to biesi for his help and patience).
Attachment #253012 - Flags: review?(cbiesinger)
Attachment #253012 - Flags: review?(cbiesinger) → review+
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.
Attachment #247742 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 18

12 years ago
Comment on attachment 247742 [details] [diff] [review]
Patch v3

Transferring Darin's sr request to Biesi per Darin's request.
Attachment #247742 - Flags: superreview?(darin.moz) → superreview?(cbiesinger)
Comment on attachment 247742 [details] [diff] [review]
Patch v3

ok then, r+sr=me :)
Attachment #247742 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 20

12 years ago
Created attachment 254307 [details] [diff] [review]
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.
Attachment #247742 - Attachment is obsolete: true
Attachment #253012 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]

Comment 21

12 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.