[Django] #28104: Django conditional view processing decorator adds stale ETag

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Django] #28104: Django conditional view processing decorator adds stale ETag

Django
#28104: Django conditional view processing decorator adds stale ETag
-----------------------------------------+------------------------
               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.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/054.e7c12a187c4a3cf222fc60a0305136f5%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag (was: Django conditional view processing decorator adds stale ETag)

Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.a711de260465210acc9940891f2667bc%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.54565c405f97636c191e7ef8e5de1d73%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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
Easy pickings:  0                   |                    UI/UX:  0
------------------------------------+------------------------------------
Changes (by Kevin Christopher Henry):

 * cc: k@… (added)


--
Ticket URL: <https://code.djangoproject.com/ticket/28104#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.d313e886781d5d5a44464821c5681849%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  0
------------------------------------+------------------------------------
Changes (by Josh Schneier):

 * 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.0fcf9e264041c636dcd6c7892093d1b1%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.470e805eed5db9a59b0f2bb3b9c659a6%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.66fa5237f9ad3b11eb3951865b4e2245%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.5484b8881c3dc9bd339535b6ea6b0f85%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.1a094ba32597aba6c69ae150b1178d1d%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.5e0b841ead8acb3787d8f35433500876%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     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:  1                   |      Needs documentation:  0
  Needs tests:  0                   |  Patch needs improvement:  0
Easy pickings:  0                   |                    UI/UX:  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.728e54c5c7d44e479d53d974bfd7a584%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Django] #28104: conditional view processing decorator adds stale ETag

Django
In reply to this post by Django
#28104: conditional view processing decorator adds stale ETag
------------------------------------+------------------------------------
     Reporter:  Syed Safi Ali Shah  |                    Owner:  nobody
         Type:  Bug                 |                   Status:  closed
    Component:  HTTP handling       |                  Version:  1.11
     Severity:  Normal              |               Resolution:  fixed
     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:  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>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/069.836b3b5a05bac202a5e294bf0eccde28%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Loading...