[Django] #19705: CommonMiddleware handles If-Modified-Since incorrectly
Django
-----------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Two middleware check ETags for unmodified responses: `CommonMiddleware`
and `ConditionalGetMiddleware` and they do it inconsistently.
If the response's ETag matches the request's If-Modified-Since:
- `ConditionalGetMiddleware` changes the response code to 304, preserving
all headers; the content gets removed later on
- `CommonMiddleware` creates a new `HttpResponseNotModified` without
content and simply restores the cookies.
As a consequence, `CommonMiddleware` returns a response without ETag,
which is wrong. I detected this with RedBot on a Django site I run. Any
site with `USE_ETAGS = True` has this problem.
In general, wiping headers sounds like a bad idea. A 304 is supposed to
have the same headers as the 200. (Well, the RFC is more complicated, but
I think it's the general idea. [http://tools.ietf.org/html/draft-ietf-
httpbis-p1-messaging-21#section-3.3.2 Future versions of HTTP] will likely
require the Content-Length not to be 0.)
----
I believe that `CommonMiddleware` should simply generate the ETag and not
handle conditional content removal; that's the job of
`ConditionalGetMiddleware`.
For example, if one is using GzipMiddleware, the correct response chain
is:
- `CommonMiddleware` computes the ETag,
- `GzipMiddleware` compresses the content and modifies the ETag,
- `ConditionalGetMiddleware` uses the modified ETag to decide if the
response was modified or not.
This is a good reason to keep "ETag generation" and "Etag checking"
concerns separate. The same argument applies to any middleware that sets
or modifies ETags.
Unfortunately, `CommonMiddleware` is documented to "take care of sending
Not Modified responses, if appropriate", so this would be a backwards
incompatible change.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Old description:
New description:
Two middleware check ETags for unmodified responses: `CommonMiddleware`
and `ConditionalGetMiddleware` and they do it inconsistently.
If the response's ETag matches the request's If-None-Match:
----
--
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:1>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The discussion below https://code.djangoproject.com/ticket/16035#comment:9
was closed as a duplicate of this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:2>
Django
-------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by hirokiky):
* status: new => assigned
* cc: hirokiky@… (added)
* has_patch: 0 => 1
* owner: nobody => hirokiky
Comment:
I send a pull-request to solve this problem
[https://github.com/django/django/pull/700].
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:3>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: hirokiky
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mrmachine):
This still leaves a dependency on the order of middleware for correct
operation. I don't think Etags should be set or checked in middleware at
all. We already have a `USE_ETAGS` setting. Why not just return the
`HttpResponseNotModified` after all middleware has executed, when
`USE_ETAGS` is `True`?
We don't need middleware to create Etags, either. We could hook into
content and header assignment for `HttpResponse`, so that Etags will
always be set or updated whenever content or headers change, when
`USE_ETAGS` is `True`.
Another benefit would be that we remove the repetitive requirement for ALL
middleware that alters content or headers having to check the `USE_ETAGS`
setting and then conditionally recalculate and update the Etag header.
It's currently very easy for a middleware author to alter content without
realising that they must do this, which means we could be serving
responses with stale Etag headers.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:4>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: hirokiky
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mrmachine):
* cc: real.human@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:5>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: hirokiky
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
Comment:
Setting as "patch needs improvement" given the previous comment and the
fact that the PR no longer merges cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:6>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner:
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by hirokiky):
* owner: hirokiky =>
* status: assigned => new
Comment:
I de-assign once time. because I won't try to fix this problem these days.
sorry.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:7>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner:
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by syphar):
Since I stumbled on it while working with Django, I want to take care (if
possible) of this issue.
What seems to be missing (apart from implementation) is this major design
decision (of course there could be ways in between):
== Way 1: Cleanup middlewares ==
- as aaugustin proposed
- change middleware behavior (backwards incompatible)
- {{{CommonMiddleware}}} only calculates the ETag, if setting is set
- {{{GZipMiddleware}}} changes it probably
- {{{ConditionalGetMiddleware}}} takes care about the response code
- perhaps move {{{http.conditional_content_removal()}}}, since it then
removes the content based on the response code
- document usage and needed order of the middlewares
== Way 2: remove ETag handling in middlewares ==
- as mrmachine proposed
- also backwards incompatible
- no ETag / Conditional Handling in middleware
- ETags are always calculated in the request (when the setting is set)
- conditional view processing is also moved, and always done (open on
this, what about last-modified handling?)
- perhaps easier to use for users, since it hooks into the response-
content
- open question would be, how the view decorators {{{@etag}}} and
{{{@condition}}} would work then
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:8>
Django
-------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by syphar):
* owner: => syphar
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:9>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by syphar):
after talking to apollo13 and mjtamlyn we lean towards way 1, for a first
draft.
I'll look into the old PR and improve / finish it up.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:10>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by syphar):
This will get funny.
There is also some duplication between:
- {{{CommonMiddleware}}}
- {{{ConditionalGetMiddleware}}}
- the {{{@condition}}} decorator
also bugs fixed in only one of the places.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:11>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by mrmachine):
Was there any comment from those who prefer option 1 about all middleware
that is ever written in the future having to check the value of the
`USE_ETAGS` setting if the middleware alters content/headers, and
recalculate the etag? This seems very likely to not happen in a lot of
cases. Not all middleware authors will know or care about etags, and if
they write a middleware class that alters content/headers it will become
buggy when deployed by other developers who have enabled this setting.
I think that option 1 is just patching the symptom of coupled middleware
and repetitive etag handling in Django's own middleware, but doesn't do
much for the wider middleware ecosystem or preventing developers from
making the same mistakes in their own code.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:12>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by syphar):
Just rediscovered this issue.
* We refactored the conditional-get handling in
7a40fef17ab7918cbb1ddc3ba080f42b420f7a48. There we merged all the
different implementations of conditional-get handling into one place.
* there, based on the original code, a new response is generated as not-
modified. Only cookies are kept if they are there.
* I just started working on a new ticket that will deprecate / remove
{{USE_ETAGS}} setting, and get the conditional-get handling out of
{{CommonMiddleware}}. Then the {{ConditionalGetMiddleware}} will generate
a {{ETag}} if there is none. By this middleware just being the last one,
many of the edge-cases will be fixed.
Is there any knowledge on if we should keep all the headers in a 304
response? Before the refactor we had places that handled it differently.
At the moment we're returning a new 304 response all the time.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:13>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by marfire):
* cc: k@… (added)
Comment:
At the moment there are several interrelated problems with the way Django
handles ETags. Briefly:
* As stated in the original report above, we're not returning the correct
headers with our `304s`.
* Gzipped responses are not getting the benefits of `304s` since their
ETags are not compared properly. See tickets #16035 and #26771.
* Our processing of conditional requests does not follow the
specification's rewritten
[[https://tools.ietf.org/html/rfc7232#section-6|precedence rules]] from
2014.
* The `condition()`, `vary*()`, `cache_control()`, and `gzip_page()`
decorators work properly in some undocumented orders but not in others.
Here are the changes I would suggest to address these problems:
* Move ETag handling out of `CommonMiddleware` and into
`ConditionalGetMiddleware`. This change has already been accepted (#26447)
and partially implemented ([[https://github.com/django/django/pull/6393|PR
6393]]). (Though I'm less confident about the deprecation of `USE_ETAGS`
in that ticket.)
* Change `GZipMiddleware` to remove the `;gzip` token from the incoming
ETags in `process_request()`. That will allow comparisons to work
properly. Document this.
* Change the `304` headers to match the specification.
* Change our conditional view logic to match the (simpler) specified
precedence rules.
* Change our use of `GzipFile` to specify a modification time of 0. That
will make our gzip output dependent only on the response body, which means
that we can usefully compare ETags on gzipped content, which means that we
don't have to care about the order of the `gzip_page()` decorator.
* Document that the `condition()` decorator should be below `vary()` and
`cache_control()` so that those headers can be set properly.
In more detail:
[[https://code.djangoproject.com/ticket/19705#comment:12|mrmachine's
comment]] raises a fair point. The fundamental issue here is that Django's
architecture strives to be layered and decoupled, but ETags are by
definition highly coupled to the response, so it's difficult to isolate
them to one layer (whether that be middleware or a decorator).
That said, [[https://tools.ietf.org/html/rfc7232#section-2.1|the
specification]] only requires that the ETag change when the response body
changes, and I don't think that's a very common middleware behavior. Only
`GZipMiddleware` meets that definition among the middleware in core, for
example. And if the body ''is'' changed, the ETag can be changed in a
manner similar to what I'm suggesting for `GZipMiddleware`. I do think
this needs to be documented, though.
Regarding which headers to send back with `304s`,
[[https://tools.ietf.org/html/rfc7232#section-4.1|the specification]] says
this:
> The server generating a 304 response MUST generate any of the following
header fields that would have been sent in a 200 (OK) response to the same
request: `Cache-Control`, `Content-Location`, `Date`, `ETag`, `Expires`,
and `Vary`. Since the goal of a 304 response is to minimize information
transfer when the recipient already has one or more cached
representations, a sender SHOULD NOT generate representation metadata
other than the above listed fields unless said metadata exists for the
purpose of guiding cache updates (e.g., `Last-Modified` might be useful if
the response does not have an `ETag` field).
So I think we should return those headers from the response, along with
the cookies (since that is what Django already does, and since it's
[[https://bz.apache.org/bugzilla/show_bug.cgi?id=18388|apparently very
common]] despite not being mandated by the standard).
I'm able to work on this, but the next step depends on the fate of
[[https://github.com/django/django/pull/6393|PR 6393]] , since that
accomplishes some of the reorganization mentioned above.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:14>
Django
-------------------------------+------------------------------------
Reporter: aaugustin | Owner: syphar
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by marfire):
Another solution to the gzip problem (that is, that we never match a
gzipped `ETag` because we modify it on the way out but not on the way in)
would be to have `GZipMiddleware` change the `ETag` to a weak `ETag`. That
would be simple, consistent with the specification, and well-precedented:
* nginx does this ([http://nginx.org/en/CHANGES as of version 1.7.3]) when
it gzips responses with an `ETag`
* [https://github.com/rails/rails/pull/17573 Rails] and
[https://github.com/rack/rack/issues/681 Rake] have recently switched to
using weak `ETags`
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:15>
Django
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/7369 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bd7237d7ec5bb66add624a0cf31ac85f9aceadce" bd7237d7]:
{{{
#!CommitTicketReference repository=""
revision="bd7237d7ec5bb66add624a0cf31ac85f9aceadce"
Fixed #19705 -- Set proper headers on conditional Not Modified responses.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
[https://github.com/django/django/pull/7376 PR] for having
`GZipMiddleware` make `ETags` weak to allow conditional responses.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
[https://github.com/django/django/pull/7378 PR] for having GZipMiddleware
set gzip modification times to 0.
I'm not aware of any downside, and the advantage is that
`ConditionalGetMiddleware` will work (that is, produce 304 Not Modified
responses) on gzipped content (e.g. if the order of the middlewares is
reversed, or if the `gzip_page()` view decorator is used).
This usage is allowed by the specification ("MTIME = 0 means no time stamp
is available", Section 2.3.1 of [https://tools.ietf.org/html/rfc1952 RFC
1952]) and is in common use (for example, Java's `GZipOutputStream` sets
the MTIME to 0).
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"ad332e5ca9503bba2e14cc8c3ca675eed56d72bc" ad332e5c]:
{{{
#!CommitTicketReference repository=""
revision="ad332e5ca9503bba2e14cc8c3ca675eed56d72bc"
Refs #19705 -- Made GZipMiddleware make ETags weak.
Django's conditional request processing can now produce 304 Not Modified
responses for content that is subject to compression.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kevin Christopher Henry):
[https://github.com/django/django/pull/7388 PR] documenting the correct
order of the `condition()` decorator.
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"9eb49af821546af1cae8f3a91aefea4b99a6478f" 9eb49af8]:
{{{
#!CommitTicketReference repository=""
revision="9eb49af821546af1cae8f3a91aefea4b99a6478f"
Refs #19705 -- Documented decorator ordering with @condition().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"404587078112295d961939ae76b49ab22da3c79c" 40458707]:
{{{
#!CommitTicketReference repository=""
revision="404587078112295d961939ae76b49ab22da3c79c"
[1.10.x] Refs #19705 -- Documented decorator ordering with @condition().
Backport of 9eb49af821546af1cae8f3a91aefea4b99a6478f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Aymeric Augustin | Owner: Denis
| Cornehl
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"9108696a7553123f57c5d42f9c4a90cad44532f4" 9108696]:
{{{
#!CommitTicketReference repository=""
revision="9108696a7553123f57c5d42f9c4a90cad44532f4"
Refs #19705 -- Changed gzip modification times to 0.
This makes gzip output deterministic, which allows
ConditionalGetMiddleware to reliably compare ETags on gzipped
content (views using the gzip_page() decorator in particular).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19705#comment:24>