[Django] #28104: Django conditional view processing decorator adds stale ETag
Django
-----------------------------------------+------------------------
Reporter: safialishah | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------+------------------------
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:-
{{{
#!div style="font-size: 80%"
{{{#!python
# 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 [http://stackoverflow.com/questions/43495112
/django-conditional-view-processing-decorator-adds-stale-etag here], to
comform to the RFC better, we should avoid sending ETag and Last-Modified
headers in response to unsafe methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/28104>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------------+------------------------------------
Reporter: Syed Safi Ali Shah | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
* component: Uncategorized => HTTP handling
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:1>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:2>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
* cc: k@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:3>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
* has_patch: 0 => 1
Comment:
I looked into this and implemented Simon's 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
[https://tools.ietf.org/html/rfc7231#section-7.2 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).
[https://github.com/django/django/pull/8468 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:4>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (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 [https://tools.ietf.org/html/rfc7231#section-4.3.4
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...")
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:5>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Changes (by Josh Schneier):
* cc: josh.schneier@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:6>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (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?)
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:7>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:8>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (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 [https://docs.djangoproject.com/en/dev/internals/release-
process/#supported-versions supported versions policy].
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:9>
Django
------------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
Comment (by Josh Schneier):
Thanks Tim, didn't realize the policy didn't cover bugs in general.
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:10>
Django
------------------------------------+------------------------------------
Type: Bug | Status: closed
Component: HTTP handling | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"37c9b81ebc9af3b61345a070a143ee8330a119b4" 37c9b81e]:
{{{
#!CommitTicketReference repository=""
revision="37c9b81ebc9af3b61345a070a143ee8330a119b4"
Fixed #28104 -- Prevented condition decorator from setting ETag/Last-
Modified headers for non-safe requests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:11>