Closed
Bug 676358
Opened 13 years ago
Closed 10 years ago
HTTP auth realm parameter parsed as quoted-string doesn't get unescaped
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: julian.reschke, Assigned: bagder)
References
()
Details
Attachments
(1 file, 2 obsolete files)
3.34 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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');
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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?
Attachment #8360968 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 4•10 years ago
|
||
same as Daniel's patch, but in hg format; I also tested the patch and it appears to resolve the problem
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → daniel
Assignee | ||
Updated•10 years ago
|
Attachment #8360415 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8360968 -
Attachment is obsolete: true
Attachment #8360968 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8361661 [details] [diff] [review] 676358 This is the hg version of the v2 patch - I obsoleted the other patches.
Attachment #8361661 -
Flags: review?(jduell.mcbugs)
Comment 6•10 years ago
|
||
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.
Attachment #8361661 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d651d6239641
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•