Commits

Robert Brewer committed afecadd

Solution for #602 (ETag autotags are incorrect on 304):

1. Added WARNING to docstring.
2. Only generate ETag if status == 200.
3. Now performs If-Match, If-None-Match tests even if no ETag provided.
4. Corrected "< 299" to "<= 299".

Comments (0)

Files changed (2)

cherrypy/lib/cptools.py

 """Functions for builtin CherryPy tools."""
 
+import md5
 import re
 
 import cherrypy
     
     If autotags is True, an ETag response-header value will be provided
     from an MD5 hash of the response body (unless some other code has
-    already provided an ETag header). If False, the ETag will not be
-    automatic, and if no other code has provided an ETag value, then no
-    checks will be performed against If-Match or If-None-Match headers.
+    already provided an ETag header). If False (the default), the ETag
+    will not be automatic.
+    
+    WARNING: the autotags feature is not designed for URL's which allow
+    methods other than GET. For example, if a POST to the same URL returns
+    no content, the automatic ETag will be incorrect, breaking a fundamental
+    use for entity tags in a possibly destructive fashion. Likewise, if you
+    raise 304 Not Modified, the response body will be empty, the ETag hash
+    will be incorrect, and your application will break.
+    See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.24
     """
     response = cherrypy.response
     
     if hasattr(response, "ETag"):
         return
     
+    status, reason, msg = _http.valid_status(response.status)
+    
     etag = response.headers.get('ETag')
     
+    # Automatic ETag generation. See warning in docstring.
     if (not etag) and autotags:
-        import md5
-        etag = '"%s"' % md5.new(response.collapse_body()).hexdigest()
-        response.headers['ETag'] = etag
+        if status == 200:
+            etag = response.collapse_body()
+            etag = '"%s"' % md5.new(etag).hexdigest()
+            response.headers['ETag'] = etag
     
-    if etag:
-        response.ETag = etag
-        
-        status, reason, msg = _http.valid_status(response.status)
-        
+    response.ETag = etag
+    
+    # "If the request would, without the If-Match header field, result in
+    # anything other than a 2xx or 412 status, then the If-Match header
+    # MUST be ignored."
+    if status >= 200 and status <= 299:
         request = cherrypy.request
         
         conditions = request.headers.elements('If-Match') or []
         conditions = [str(x) for x in conditions]
         if conditions and not (conditions == ["*"] or etag in conditions):
-            if status >= 200 and status < 299:
-                raise cherrypy.HTTPError(412)
+            raise cherrypy.HTTPError(412, "If-Match failed: ETag %r did "
+                                     "not match %r" % (etag, conditions))
         
         conditions = request.headers.elements('If-None-Match') or []
         conditions = [str(x) for x in conditions]
         if conditions == ["*"] or etag in conditions:
-            if status >= 200 and status < 299:
-                if request.method in ("GET", "HEAD"):
-                    raise cherrypy.HTTPRedirect([], 304)
-                else:
-                    raise cherrypy.HTTPError(412)
+            if request.method in ("GET", "HEAD"):
+                raise cherrypy.HTTPRedirect([], 304)
+            else:
+                raise cherrypy.HTTPError(412, "If-None-Match failed: ETag %r "
+                                         "matched %r" % (etag, conditions))
 
 def validate_since():
     """Validate the current Last-Modified against If-Modified-Since headers.
         
         since = request.headers.get('If-Unmodified-Since')
         if since and since != lastmod:
-            if (status >= 200 and status < 299) or status == 412:
+            if (status >= 200 and status <= 299) or status == 412:
                 raise cherrypy.HTTPError(412)
         
         since = request.headers.get('If-Modified-Since')
         if since and since == lastmod:
-            if (status >= 200 and status < 299) or status == 304:
+            if (status >= 200 and status <= 299) or status == 304:
                 if request.method in ("GET", "HEAD"):
                     raise cherrypy.HTTPRedirect([], 304)
                 else:

cherrypy/test/test_etags.py

             return "Oh wah ta goo Siam."
         resource.exposed = True
         
-        def fail(self):
-            raise cherrypy.HTTPError(412)
+        def fail(self, code):
+            code = int(code)
+            if 300 <= code <= 399:
+                raise cherrypy.HTTPRedirect([], code)
+            else:
+                raise cherrypy.HTTPError(code)
         fail.exposed = True
     
     conf = {'/': {'tools.etags.on': True,
         self.assertStatus('200 OK')
         self.assertHeader('Content-Type', 'text/html')
         self.assertBody('Oh wah ta goo Siam.')
-        self.assertHeader('ETag')
-        for k, v in self.headers:
-            if k.lower() == 'etag':
-                etag = v
-                break
+        etag = self.assertHeader('ETag')
         
         # Test If-Match (both valid and invalid)
         self.getPage("/resource", headers=[('If-Match', etag)])
         self.assertStatus("200 OK")
         self.getPage("/resource", headers=[('If-Match', "*")])
         self.assertStatus("200 OK")
+        self.getPage("/resource", headers=[('If-Match', "*")], method="POST")
+        self.assertStatus("200 OK")
         self.getPage("/resource", headers=[('If-Match', "a bogus tag")])
         self.assertStatus("412 Precondition Failed")
         
         self.getPage("/resource", headers=[('If-None-Match', "a bogus tag")])
         self.assertStatus("200 OK")
         
-        # Test raising 412 in page handler
-        self.getPage("/fail", headers=[('If-Match', etag)])
+        # Test raising errors in page handler
+        self.getPage("/fail/412", headers=[('If-Match', etag)])
         self.assertStatus(412)
+        self.getPage("/fail/304", headers=[('If-Match', etag)])
+        self.assertStatus(304)
+        self.getPage("/fail/412", headers=[('If-None-Match', "*")])
+        self.assertStatus(412)
+        self.getPage("/fail/304", headers=[('If-None-Match', "*")])
+        self.assertStatus(304)
 
 
 if __name__ == "__main__":
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.