Opened 10 years ago

Closed 5 years ago

Last modified 5 years ago

#717 closed defect (fixed)

If-Modified-Since checked for exact match

Reported by: Maniac <Maniac@…> Owned by: adrian
Component: HTTP handling Version:
Severity: normal Keywords: http middleware conditional get if-modified-since
Cc: aaugustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In ConditionalGetMiddleWare Last-Modified date is checked for exact match with If-Modified-Since and only then 'Not Modified' response returned. There should be check if Last-Modified >= If-Modiifed-Since.

Attachments (4)

717.diff (1.1 KB) - added by Maniac <Maniac@…> 10 years ago.
Patch
ifmodifiedsince.diff (6.2 KB) - added by julianb 5 years ago.
717.2.diff (17.8 KB) - added by aaugustin 5 years ago.
717.3.diff (17.6 KB) - added by aaugustin 5 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by adrian

Nice catch. Looks like the solution will require parsing the Last-Modified and HTTP_IF_MODIFIED_SINCE headers into datetime objects for comparison.

comment:2 Changed 10 years ago by adrian

From http://fishbowl.pastiche.org/2002/10/21/http_conditional_get_for_rss_hackers :

"""
Technically, what you should do with an If-Modified-Since header is convert it to a date, and compare it with your stored date. However, 90% of the time you can get away with just doing a straight match, so it's probably not worth the effort.
"""

Thoughts?

Changed 10 years ago by Maniac <Maniac@…>

Patch

comment:3 Changed 10 years ago by Maniac <Maniac@…>

I've made a simple patch with such parsing. It's very nice that HTTP requires everything in one timezone :-)

However I just thought that in practice exact checking would work anyway since in most cases Last-Modified changes only to bigger values and browsers ask If-Modified-Since only from the last Last-Modified given :-).

comment:4 Changed 10 years ago by Maniac <Maniac@…>

Ah... I just missed your second comment and made it in my own :-)

In fact I don't know... Comparing dates seem somewhat cleaner. But this is more error-prone. If someone gives 'Last-Modified' in other two allowed formats in HTTP there'll be an exception (but again nobody uses those formats :-) ).

Indeed may be we shouldn't fix it if it's not broken...

comment:5 Changed 10 years ago by adrian

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

Yeah, let's keep it the way it is for now. Thanks for taking the time to submit a patch; it'll be here if we ever decide to change our thinking.

comment:6 Changed 10 years ago by hugo

Just for the records: there is a use-case for really doing date comparisons, that's when people don't store the actual last-modified header but store the content in the cache and use the cache-storage-date for later if-modified-since checking.

comment:7 Changed 6 years ago by julianb

  • Component changed from django.contrib.admin to HTTP handling
  • Has patch set
  • Keywords http middleware conditional get if-modified-since added
  • Needs tests set
  • Resolution wontfix deleted
  • Status changed from closed to reopened

After 5 years, I propose we tackle this. Currently it's not in line with the RFC (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html).

I have observed Google to often check with a newer Last-Modified date, and Django replying with "Oh, sure it got modified, but long time before what you ask, here's the resource anyways: 200 OK"

comment:8 Changed 6 years ago by lrekucki

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

Django's policy is that you shouldn't reopen tickets resolved as "won't fix" without prior discussion on django-developers mailing list. Please post there describing why this ticket should be accepted. It's also a good idea to describe problem with current behavior in more detail.

comment:9 Changed 5 years ago by lukeplant

  • Patch needs improvement set
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

Accepted after discussion on mailing list, and a valid use case based on headers that Google sends.

The patch needs tests, including what happens with valid dates before and after, and what happens with invalid dates. Please can we also have it tested with someone who knows exactly what Google is sending.

Thanks!

comment:10 Changed 5 years ago by julianb

I improved the patch for the middleware by using parsedate from email.utils which might parse more dates.

The HTTP decorater also has the same issue and I managed to add some tests for it. Tests for the ConditionalGetMiddleware are still needed, as I am not really into tests.

Changed 5 years ago by julianb

comment:11 Changed 5 years ago by aaugustin

New patch:

  • builds upon julianb's last patch, but does much less changes to django/views/decorators/http.py,
  • adds a parse_http_date function in django.utils.http that implements a RFC-compliant parsing of HTTP dates, and uses it wherever ad-hoc parsing was implemented; that function may be simplified with email.Utils.parsedate if RFC compliance is not required;
  • adds tests for ConditionalGetMiddleware (fairly similar to the tests in conditional_processing, but those test the decorators and not the middleware),
  • fixes a related test that used UTC instead of GMT.

Changed 5 years ago by aaugustin

comment:12 Changed 5 years ago by aaugustin

  • Needs tests unset
  • Patch needs improvement unset

comment:13 Changed 5 years ago by aaugustin

  • Cc aaugustin added

comment:14 Changed 5 years ago by lukeplant

  • Patch needs improvement set

This looks like a very good patch in general, but with some problems:

  • Why have tests for ConditionalGetMiddleware been added to tests/regressiontests/middleware/tests.py, instead of to tests/regressiontests/conditional_processing/models.py with the others? And why is there such overlap between the new tests (e.g. in 'ETag' processing) and the existing ones? This duplication is confusing and unhelpful.
  • The HttpDateProcessing class should inherit from unittest.TestCase (specifically, django.utils.unittest.TestCase), and not django.test.TestCase.

comment:15 Changed 5 years ago by aaugustin

  • Patch needs improvement unset

Existing tests in tests/regressiontests/conditional_processing/models.py are for the view decorators condition, last_modified and etag. The tests I added are for ConditionnalGetMiddleware. These are two different mechanisms to achieve the same goal, the former per-view, the latter globally. Obviously tests are similar, but they test different modules.

Currently there are no tests for ConditionnalGetMiddleware. It seems to me that tests/regressiontests/middleware/tests.py is the proper place to test middleware, so I put the tests there.

I have updated the patch to cope with the refactoring of django.views.static and django.contrib.staticfiles.
I'm now using django.utils.unittest.TestCase when django.test.TestCase is not needed.

New patch attached. Thanks for your review, lukeplant!

Changed 5 years ago by aaugustin

comment:16 Changed 5 years ago by lukeplant

Ah, you're right - I think I was wrongly assuming that the decorators 'condition' and others were created using decorator_from_middleware, in which case they would be implicitly testing ConditionalGetMiddleware. In fact, only 'conditional_page' is created that way, which isn't tested in tests/regressiontests/conditional_processing/models.py. Thanks for the explanation and updates.

comment:17 Changed 5 years ago by lukeplant

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

In [15696]:

Fixed #717 - If-Modified-Since handling should compare dates according to RFC 2616

Thanks to Maniac for the report, julienb for the initial patch, and
especially to aaugustin for the final patch and tests.

comment:18 Changed 5 years ago by lukeplant

In [15697]:

[1.2.X] Fixed #717 - If-Modified-Since handling should compare dates according to RFC 2616

Thanks to Maniac for the report, julienb for the initial patch, and
especially to aaugustin for the final patch and tests.

Backport of [15696] from trunk.

comment:19 Changed 5 years ago by lukeplant

In [15699]:

Added file missing from [15696], sorry for breakage.

Thanks to Ramiro for letting me know. Refs #717.

comment:20 Changed 5 years ago by lukeplant

In [15700]:

[1.2.X] Added file missing from [15696], sorry for breakage.

Thanks to Ramiro for letting me know. Refs #717.

Backport of [15699] from trunk.

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