Opened 3 years ago

Last modified 3 years ago

#5677 defect new

WWW-Authenticate schema is not title cased, causes the request library to fail, Chrome to reprompted for auth

Reported by: myers Owned by: itamar
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/http-auth-5677
(diff, github, buildbot, log)
Author: myers Launchpad Bug:

Description

http://www.faqs.org/rfcs/rfc2617.html shows the auth scheme as being capitalized. Twisted 12.0 doesn't do this right. This causes the 'requests' python library to fail, and causes Chrome to prompt for username/password every time you reload a page.

Attachments (2)

5677-fix-http-auth-schema-capitalization.patch (1.3 KB) - added by myers 3 years ago.
patch against trunk @ 34408
5677-fix-http-auth-schema-capitalization-take-2.patch (2.3 KB) - added by myers 3 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 3 years ago by myers

patch against trunk @ 34408

comment:2 Changed 3 years ago by itamar

  • Keywords review added
  • Owner set to itamar

Since this is a patch that is ready for application, you want to add the 'review' keyword in the future so it ends up in the review queue.

comment:3 Changed 3 years ago by exarkun

I don't object to changing this, but note that Twisted behaves correctly in this case. According to RFC 2616, header field names are case insensitive. The bug really belongs to requests and Chrome.

comment:4 Changed 3 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to myers

Thanks for the bug report, this is indeed a problem. To clarify to JP, this isn't about the header, it's about the token identifying which schema is being used, e.g. "Basic" where we send "basic". The RFC has hard-coded strings, basically tokens, which we should be using. Therefore:

  1. Rather than using title(), I would suggest changing the underlying schema names, e.g. from "basic" to "Basic" in twisted.web._auth.basic, and similarly for digest. That is, we should hard-code the correct schema name, rather than hard-coding a wrong one and then transforming it.
  2. Digest will need an equivalent test to the basic making sure we send the correct value.

Please update the patch to address these two issues and re-add the "review" keyword; if you don't have the time to do so let me know and I can do the change, but then someone else will need to review it.

comment:5 Changed 3 years ago by myers

I didn't make a test for digest like you asked. Ran out of time.

comment:6 Changed 3 years ago by myers

  • Keywords review added

comment:7 Changed 3 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/http-auth-5677

(In [34413]) Branching to 'http-auth-5677'

comment:8 Changed 3 years ago by itamar

  • Keywords review removed
  • Owner changed from myers to itamar

Thanks for the update! Still missing a couple of tests, to make sure Digest is sent, and perhaps a tweak of the HTTP layer's leniency and corresponding test.

comment:9 Changed 3 years ago by itamar

  • Author changed from itamarst to myers
  • Keywords review added
  • Owner itamar deleted

Which I've added myself. Ready for hopefully final review, and I started test run:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/http-auth-5677

comment:10 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to itamar

Thanks!

  1. The new tests which refer to the RFC could do with a link and a section reference. It's probably also worth pointing out that the string must exactly match, not just case-insensitively match.
  2. The docstring (and, arguably, name) of _authorizedBasicLogin is wrong now. It will use whatever scheme the caller supplies. I suggest fixing the docstring, renaming, and making the scheme argument required instead of optional.
  3. What's the difference between "correct" and "sufficiently correct"? :)
  4. test_renderAuthorizedWrongCapitalizationScheme has a typo in its docstring, capitaliztion.
  5. The use of "correct" throughout is a little bit sad. If you can replace it with descriptions of what "correct" actually means, at least in the changed tests, that'd be really great.

comment:11 Changed 3 years ago by myers

Re: Chrome, I think I was incorrect about why that keeps prompting. I think it's due to requests coming 15 farther that 15 minutes apart.

comment:12 Changed 3 years ago by itamar

Further investigation of RFC suggests that it *is* supposed to be case insensitive. Probably being the wrong case will break some things, though (e.g. requests) so we should do this anyway. Do you want to file a separate bug for the Chrome issue?

Note: See TracTickets for help on using tickets.