Opened 8 years ago
Closed 7 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 |
Description
For conditional view processing, Django provides the @condition decorator, which is defined here https://github.com/django/django/blob/master/django/views/decorators/http.py
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.
or
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 by , 8 years ago
| Component: | Uncategorized → HTTP handling |
|---|---|
| Easy pickings: | unset |
| Summary: | Django conditional view processing decorator adds stale ETag → conditional view processing decorator adds stale ETag |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 8 years ago
comment:3 by , 8 years ago
| Cc: | added |
|---|
comment:4 by , 8 years ago
| 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 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).
comment:5 by , 8 years ago
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 by , 8 years ago
| Cc: | 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 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
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.
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.PUTconflict).