Opened 8 years ago

Closed 8 years ago

#3206 closed defect (fixed)

USE_ETAGS returns status 304 even if response.status_code=400

Reported by: mihai.preda@… Owned by: adrian
Component: Core (Other) Version: master
Severity: normal Keywords: rev 4235
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When USE_ETAGS=True,
a page which wants to return a 400 status code (by setting response.status_code=400),
may return a 304 status code on reloads (because the content of the page didn't change).

I don't know if this is the correct behavior or not, but I find it surprising that a 400-status is transformed into a 304-status.
I would expect USE_ETAGS to only affect pages with 200 status code (or something like it).

Attachments (2)

etag_304.diff (625 bytes) - added by Vinay Sajip <vinay_sajip@…> 8 years ago.
Patch for HTTP 304 being returned inappropriately
common.py.patch (682 bytes) - added by Manuel Saelices <msaelices@…> 8 years ago.
Patch that fixes previous patch

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by mihai_preda@…

sorry, I've mispelled the email in the initial ticket.

comment:2 Changed 8 years ago by Michael Radziej <mir@…>

Can you please refer me to any rfc or similar standard that backs your opinion?

comment:3 Changed 8 years ago by mihai_preda@…

In RFC 2616, section 10.3.5, 304 Not Modified:
"If the client has performed a conditional GET request and access is allowed, but the document has not been modified, the server SHOULD respond with this status code (304)."

I see two conditions for 304: GET, and "access is allowed". I interpret "access is allowed" as meaning: 2xx status code.

Also it is obvious (IMO) that not any status code (e.g. 4xx, 5xx) may be replaced with 304, as this would effectively erase this significant information (the status code).

I would suggest to play it conservativelly and only rewrite 200 to 304, unless some more authoritative understanding of RFC 2616 is expressed.

comment:4 Changed 8 years ago by Michael Radziej <mir@…>

  • Triage Stage changed from Unreviewed to Accepted

Makes sense to me. Thank you, Mihai Preda, for looking this up!

comment:5 Changed 8 years ago by cactus

I also get this behavior.

The client, in my case firefox, upon recieving an etag on a 404 response, will subsequently send an If-None-Match header for the next request of the same 404 page.

The rfc for If-None-Match states:

If the request would, without the If-None-Match header field, result in anything other than a 2xx or 304 status, then the If-None-Match header MUST be ignored. (See section 13.3.4 for a discussion of server behavior when both If-Modified-Since and If-None-Match appear in the same request.)

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26 

Django is returning a 304 not modified header, instead of a 404.

In duplicating the headers sent to an apache server serving a 404 page, I get a 404 on subsequent requests, and the if-none-match is responded to with a 404 as expected.

Changed 8 years ago by Vinay Sajip <vinay_sajip@…>

Patch for HTTP 304 being returned inappropriately

comment:6 Changed 8 years ago by Vinay Sajip <vinay_sajip@…>

  • Has patch set

Patch added to only return a 304 when the original status was 200 and the ETag matches. Possibly any 2xx return should be treated this way, but I've gone with just 200 for now.

comment:7 Changed 8 years ago by Vinay Sajip <vinay_sajip@…>

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 8 years ago by mtredinnick

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

(In [5407]) Fixed #3206 -- Changed ETag comparison to only return 304 when the normal
status code would be in the range 200 - 299. This matches RFC 2616
requirements. Based on a patch from Vinay Sajip.

comment:9 Changed 8 years ago by Manuel Saelices <msaelices@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

Patch is buggy.

Changed 8 years ago by Manuel Saelices <msaelices@…>

Patch that fixes previous patch

comment:10 Changed 8 years ago by mtredinnick

Wow.. that's a little humiliating (for me). Thanks for the extra pair of eyes.

comment:11 Changed 8 years ago by mtredinnick

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

(In [5417]) Fixed #3206 -- Fixed typo in [5407]. This time with bonus testing. Thanks,
Manuel Saelices.

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