Closed Bug 676358 Opened 9 years ago Closed 6 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: julian.reschke, Assigned: bagder)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
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');
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.
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)
Attached patch 676358Splinter Review
same as Daniel's patch, but in hg format; I also tested the patch and it appears to resolve the problem
Assignee: nobody → daniel
Attachment #8360415 - Attachment is obsolete: true
Attachment #8360968 - Attachment is obsolete: true
Attachment #8360968 - Flags: review?(jduell.mcbugs)
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 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+
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!)
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d651d6239641
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.