CommonMiddleware handles If-None-Match incorrectly
|Reported by:||aaugustin||Owned by:||syphar|
|Cc:||hirokiky@…, real.human@…||Triage Stage:||Accepted|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||yes|
Description (last modified by aaugustin)
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:
- 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. 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.
Change History (12)
comment:1 Changed 3 years ago by aaugustin
- Description modified (diff)
- Summary changed from CommonMiddleware handles If-Modified-Since incorrectly to CommonMiddleware handles If-None-Match incorrectly
- Triage Stage changed from Unreviewed to Accepted
comment:3 Changed 3 years ago by hirokiky
- Cc hirokiky@… added
- Has patch set
- Owner changed from nobody to hirokiky
- Status changed from new to assigned
comment:7 Changed 2 years ago by hirokiky
- Owner hirokiky deleted
- Status changed from assigned to new