Opened 10 years ago

Closed 10 years ago

#24137 closed Cleanup/optimization (fixed)

Reason phrases use all upper case by default; standard suggests title case

Reported by: Jon Dufresne Owned by: nobody
Component: HTTP handling Version: dev
Severity: Normal Keywords:
Cc: jon.dufresne@…, cmawebsite@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Reason phrases were added in ticket #12747. The phrases were added as all upper case.

When looking at the HTTP standard as reported by different sources. The default phrases are never all upper case. Instead they typically use a title case where the first letter of each word is capitalized and the remaining words are lower case. I found it odd and unusual that Django would follow a different default here.

Pull request to follow.

See documentation at:

Change History (12)

comment:1 by Claude Paroz, 10 years ago

Triage Stage: UnreviewedAccepted

Upper case for this phrases were introduced long ago, see 6e034a831aea204b39f4863eddfbc223a638fb76 (from 2005). The only concern is backwards compatibility, of course. Maybe a string subclass which could compare case-insensitively?

comment:2 by Jon Dufresne, 10 years ago

Cc: jon.dufresne@… added
Has patch: set

https://github.com/django/django/pull/3902

Maybe a string subclass which could compare case-insensitively?

I can certainly look into adding this.

comment:3 by Aymeric Augustin, 10 years ago

What problems does uppercase cause in practice?

I'm not in favor of adding the complexity of a str subclass if we don't have a compelling reason.

comment:4 by Aymeric Augustin, 10 years ago

The patch looks good. Reusing the stdlib's list makes sense. I prefer documenting the change than adding complexity.

comment:5 by Claude Paroz, 10 years ago

Aymeric, my only concern is that all tests comparing reason phrases will suddenly fail after upgrading. But if you think this is an acceptable backwards incompatibility if documented, then fine for me.

comment:6 by Claude Paroz, 10 years ago

Patch needs improvement: set

I've made some comments on the PR.

comment:7 by Aymeric Augustin, 10 years ago

Browsers ignore the phrase and only look at the code. Our testing tools encourage testing the status code.

That's why I don't think many people test the phrases and I'm not too worried about making a backwards-incompatible change in this area.

in reply to:  3 comment:8 by Jon Dufresne, 10 years ago

Replying to aaugustin:

What problems does uppercase cause in practice?

I can share my motivation.

I was analyzing request/responses in Firefox and noticed that Django's requests always looked different than Apache static file requests and PHP requests. The main difference, of course, was that Django returned all upper case for reason phrases. I investigated if one was more correct than the other. The HTTP standard recommends using title case, but it is true that it is merely a recommendation. As stated in the standard, the reason phrase is for humans only. My thinking:

  • Title case is easier to read for most English reading humans (probably why it is recommend)
  • Django has no good reason to use a non-recommended reason phrase (there is no benefit)
  • Use Python stdlib code to reduce duplication and maintenance
  • Why go out of the way to make Django responses appear different?

Having thought about this some more, I think there is a very slight (I'd like to emphasize very slight) security implication here. If Django always looks different, a malicious user could use this difference to know when a request is served by Django instead of some other application server. If there is a known exploit -- perhaps recently made public by a release -- this information could allow the malicious user to find Django servers and use that known exploit.

Now, I'm not saying there aren't other Django identifying factors, I'm sure there are. But this is one step in that direction.

I'm not in favor of adding the complexity of a str subclass if we don't have a compelling reason.

Personally, I agree. I don't see the benefit of adding a str subclass. Generally, machines should be using the status code and not the reason phrase (which can be altered). The status code has very specific meaning while the reason phrase is intended for humans only.

At this point I have not investigated adding a str subclass in the PR. Unless there is a strong suggestion to do it in order to finish the PR, I will not.

comment:9 by Aymeric Augustin, 10 years ago

While there are still some details to figure out, I think the PR is on the right track.

comment:10 by Jon Dufresne, 10 years ago

Patch needs improvement: unset

I have updated the PR by incorporating feedback from reviews.

comment:11 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added
Triage Stage: AcceptedReady for checkin

comment:12 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 24b2bc635e23e4235f51095fbe099beec27a65c8:

Fixed #24137 -- Switched to HTTP reason phrases from Python stdlib.

Note: See TracTickets for help on using tickets.
Back to Top