Opened 6 years ago

Closed 6 years ago

#28104 closed Bug (fixed)

conditional view processing decorator adds stale ETag

Reported by: Syed Safi Ali Shah Owned by: nobody
Component: HTTP handling Version: 1.11
Severity: Normal Keywords:
Cc: k@…, josh.schneier@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


For conditional view processing, Django provides the @condition decorator, which is defined here

While it works nicely for GET requests, it has a bug when handling unsafe methods such as PUT or PATCH that modify a resource.
If the PUT or PATCH request contains a valid ETag in the If-Match header, the request is allowed to go through to the view for processing. Once processed, the resource would have been modified, which would most likely cause the ETag to be updated.

However, the @condition decorator would simply add the ETag that it had computed *before* the request was processed, into the PUT or PATCH response. By now this ETag is not valid anymore. Same would apply to the Last-Modified header.

Below is the snippet of the buggy code:-

          # Set relevant headers on the response if they don't already exist.
          if res_last_modified and not response.has_header('Last-Modified'):
              response['Last-Modified'] = http_date(res_last_modified)  # possibly stale value
          if res_etag and not response.has_header('ETag'):
              response['ETag'] = res_etag  # possible stale value

There are two solutions:
1) If the request was for an unsafe method, re-compute the ETag before sending the response back.
2) As pointed out by Kevin here, to comform to the RFC better, we should avoid sending ETag and Last-Modified headers in response to unsafe methods.

Change History (11)

comment:1 Changed 6 years ago by Tim Graham

Component: UncategorizedHTTP handling
Easy pickings: unset
Summary: Django conditional view processing decorator adds stale ETagconditional view processing decorator adds stale ETag
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Simon Charette

The way conditional decorators are designed will make it hard to implement 1. which can already be worked around by explicitly setting response['Etag'] in the view once the ressources have been altered.

I think 2. is the way to go here with documentation mentioning response['Etag'] should be set manually when dealing with unsafe methods (e.g. PUT conflict).

comment:3 Changed 6 years ago by Kevin Christopher Henry

Cc: k@… added

comment:4 Changed 6 years ago by Josh Schneier

Has patch: set

I looked into this and implemented option 2 but just want to note that the RFC link applies only to PUT in particular. POST and other unsafe methods can return validator headers as discussed in Section 7.2.

I do agree that computing it for POST would make the code a bit ugly due to needing to run the functions twice and can be thought of as a new feature (I haven't thought through the feasibility that much yet) instead of the bug which is this. I could also use some help framing this in documentation (does it warrant a ..versionchanged, technically it does change things so I wasn't sure if it should go in 1.11 or not).


Last edited 6 years ago by Josh Schneier (previous) (diff)

comment:5 Changed 6 years ago by Kevin Christopher Henry

Great, thanks for taking the initiative.

Your documentation is putting the case too strongly:

You must provide ETag or Last-Modified headers in response to non-safe methods.

It's always correct not to return those headers. The client can do a fresh GET to fetch the current representation and its conditional headers. Including the headers in the response, on the other hand, is a performance enhancement that is sometimes incorrect. ("An origin server MUST NOT send a validator header field (Section 7.2), such as an ETag or Last-Modified field, in a successful response to PUT unless the request's representation data was saved without any transformation applied to the body...")

comment:6 Changed 6 years ago by Josh Schneier

Cc: josh.schneier@… added

Yep I mostly put that language in because it is so obviously wrong. I just updated the docs & added some release notes. Still not sure about the framing.

comment:7 Changed 6 years ago by Kevin Christopher Henry

I added some comments to the PR.

On your versionchanged question, my opinion is that it's not necessary. The current behavior isn't documented, and is wrong enough that no one can be successfully relying on it. So I think a line in the release notes is enough. (But should it be 1.11.2 rather than 2.0?)

comment:8 Changed 6 years ago by Josh Schneier

Given it's a bugfix I agree that it should be backported to 1.11 (an LTS no less) but I'll leave that up to the maintainers. Thanks for working through documentation changes with me.

comment:9 Changed 6 years ago by Tim Graham

LTS releases don't receive special treatment with regards to bug fixes, so unless this is a regression, it probably doesn't qualify for a backport based on the supported versions policy.

comment:10 Changed 6 years ago by Josh Schneier

Thanks Tim, didn't realize the policy didn't cover bugs in general.

comment:11 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 37c9b81e:

Fixed #28104 -- Prevented condition decorator from setting ETag/Last-Modified headers for non-safe requests.

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