Opened 8 years ago

Closed 7 years ago

#4119 closed (fixed)

Incorrect HTTP-Date format in expire field of HTTP Header

Reported by: Ciantic Owned by: mtredinnick
Component: Contrib apps Version: master
Severity: Keywords: http-date header
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently the Django uses datetime.strftime to format it's HTTP-Date, because strftime is localized function it should not be used for it.

Changing locales to something else than English will break Django's session system on some browsers at the current state.

Using datetime.ctime instead makes Django's HTTP-Date format automatically correct and it is not anymore locale dependent. ctime function also automatically handles the GMT conversion if needed, that is why the patch also changes the utcnow to now. It conforms the HTTP Specification clearly as in here HTTP Specs is stated (the third choice).

This fixes only expire field in django.contrib.session.middleware but the problem might be elsewhere too (mostly where HTTP-Date is used). Common rule is that strftime is not meant for fixed date formats.

Attachments (4)

middleware.py.diff (1.0 KB) - added by Ciantic 8 years ago.
Fix for django.contrib.sessions.middleware locale dependency
middleware.py.2.diff (935 bytes) - added by Ciantic 8 years ago.
Fix for django.contrib.sessions.middleware locale dependency (first one had incorrect paths)
middleware.py.3.diff (938 bytes) - added by Ciantic 8 years ago.
Same fix, but apparenlty assumption of GMT convert was wrong. This uses utcnow again.
ticket4119.diff (4.8 KB) - added by Chris Bennett <chrisrbennett@…> 8 years ago.
fixes for this ticket

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by Ciantic

Fix for django.contrib.sessions.middleware locale dependency

Changed 8 years ago by Ciantic

Fix for django.contrib.sessions.middleware locale dependency (first one had incorrect paths)

comment:1 Changed 8 years ago by Ciantic

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Sorry about the duplicate patch, but apparently the first patch had wrong path.

Changed 8 years ago by Ciantic

Same fix, but apparenlty assumption of GMT convert was wrong. This uses utcnow again.

comment:2 Changed 8 years ago by Ciantic

One addition, the orginal "%a, %d-%b-%Y %H:%M:%S GMT" even in plain English is incorrect. It does not conform any of those three in HTTP Specs. So if this patch is not added then patch with strftime fixed is needed.

comment:3 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:4 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

Patch needs improving on two fronts:

  1. It looks like utils/cache.py at least has the same problem (just grepping for "expires"). Other places needs searching, too.
  2. Because we send out HTTP/1.1 responses (at least sometimes), we MUST use the first format ine section 3.3.1 of the HTTP 1.1 RFC. The attached patch implements the third format. So the date format needs improvement.

comment:5 follow-up: Changed 8 years ago by mtredinnick

  • Patch needs improvement set

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by anonymous

Replying to mtredinnick:
What better patch do I need? Change it to strftime perhaps?

comment:7 in reply to: ↑ 6 Changed 8 years ago by mtredinnick

Replying to anonymous:

What better patch do I need? Change it to strftime perhaps?

Whichever way you do it, the produced header string should exactly match the first format shown in section 3.3.1 of the HTTP/1.1 spec. Read the paragraph just under the examples in that section and you'll see that is marked as a "MUST" requirement. So whether there is time or datetime method that does it directly (in a locale-independent fashion) or whether we have to construct it by hand, that's the format we need to generate.

The important point about that format is that it is always the same number of characters long. It uses two-digit, zero-padded dates and hour values and four-digit years, for example.

Changed 8 years ago by Chris Bennett <chrisrbennett@…>

fixes for this ticket

comment:8 Changed 8 years ago by Chris Bennett <chrisrbennett@…>

  • Patch needs improvement unset

Attaching a patch that addresses the locale-dependency of strftime to address the requirement of HTTP/1.1 RFC2616 3.3.1:

email.utils.formatdate() returns a correctly formatted fixed length date without consideration of current locale (which is the correct behavior) for consideration of the HTTP RFC standards. (I don't like the name of the module this method is coming from either.)

Important notes about this patch:

  • I fixed a bug relating to the interpretation of the boolean True value in django/utils/cache.py (line 46) - it now reflects the behavior as described in the docstring above it. (So passing in max_age=1 does not result in the "Cache-Control" header being populated with simply "max-age")
  • The discussion in this ticket is confusing. The summary refers to "expire field of HTTP header" where the previously submitted patches attempt to fix _cookies_. This patch resolves that issue, in addition to fixing incorrect _HTTP response headers_ that should contain an HTTP 3.3.1 formatted date.
  • The '+ "GMT"' tackyness is to insure support with Python 2.3, whose formatdate() method does not yet have the usegmt enhancement.

Hope this helps, Chris

comment:9 Changed 8 years ago by mtredinnick

  • Owner changed from adrian to mtredinnick

You can always spot the python 2.5 users: email.utils is new in Python 2.5. It's called email.Utils in 2.3 and 2.4 and 2.5 also understands that name (there's an alias in email/init.py). So you should import email.Utils everywhere.

Other than that, the patch looks great. Agree with all your comments (including the discussion being confusing). I'll give it another review and check it in when I get a chance.

comment:10 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [5712] (although I typed the wrong ticket number into the commit message).

comment:11 follow-up: Changed 7 years ago by gwilson

  • Resolution fixed deleted
  • Status changed from closed to reopened

It appears that [6333] reverted this fix.

comment:12 in reply to: ↑ 11 Changed 7 years ago by gwilson

Replying to gwilson:

It appears that [6333] reverted this fix.

In django/trunk/django/contrib/sessions/middleware.py.

comment:13 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

#5816 is an attempt to fix the revert, so let's leave that as the active ticket.

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