Merge ~cjwatson/lazr.restful:304-response-headers into lazr.restful:main

[Colin Watson]

Don't send superfluous HTTP headers with 304 responses

RFC 2616 section 10.3.5 and RFC 7232 section 4.1 are both clear that 304 responses SHOULD NOT (or in some cases MUST NOT) include headers other than a short list relevant to caching. In particular, sending `Content-Length: 0` is actively harmful because it can end up instructing caches to replace that header in their cached 200 response, resulting in incorrect empty cached responses.

I wasn't able to get rid of the `X-Powered-By:` header added by `zope.publisher`, but that's a fixed string and doesn't really matter.

[Guruprasad (lgp171188)]

LGTM 👍

review: Approve

diff --git a/NEWS.rst b/NEWS.rst
index e2a3c4c..bb1e578 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -5,6 +5,8 @@ NEWS for lazr.restful
 2.0.2
 =====
 
+- Don't send superfluous HTTP headers with 304 responses (bug 2039251).
+
 2.0.1 (2022-06-28)
 ==================
 
diff --git a/src/lazr/restful/_resource.py b/src/lazr/restful/_resource.py
index d6b1919..6aceaa5 100644
--- a/src/lazr/restful/_resource.py
+++ b/src/lazr/restful/_resource.py
@@ -65,7 +65,7 @@ from zope.location.interfaces import ILocation
 from zope.pagetemplate.engine import TrustedAppPT
 from zope.pagetemplate.pagetemplatefile import PageTemplateFile
 from zope.proxy import isProxy
-from zope.publisher.http import init_status_codes, status_reasons
+from zope.publisher.http import DirectResult, init_status_codes, status_reasons
 from zope.publisher.interfaces import NotFound
 from zope.publisher.interfaces.http import IHTTPRequest
 from zope.schema import ValidationError, getFieldsInOrder
@@ -357,6 +357,24 @@ class HTTPResource:
             if existing_etag in incoming_etags:
                 # The client already has this representation.
                 # No need to send it again.
+                # RFC 7232 section 4.1 says that 304 responses SHOULD NOT
+                # generate headers other than the following, unless they
+                # exist for the purpose of guiding cache updates.
+                keep_headers = {
+                    name: self.request.response.getHeader(name)
+                    for name in (
+                        "Cache-Control",
+                        "Content-Location",
+                        "Date",
+                        "ETag",
+                        "Expires",
+                        "Vary",
+                    )
+                }
+                self.request.response.reset()
+                for name, value in sorted(keep_headers.items()):
+                    if value is not None:
+                        self.request.response.setHeader(name, value)
                 self.request.response.setStatus(304)  # Not Modified
                 media_type = None
         return media_type
@@ -1341,7 +1359,7 @@ class EntryFieldResource(FieldUnmarshallerMixin, EntryManipulatingResource):
         media_type = self.handleConditionalGET()
         if media_type is None:
             # The conditional GET succeeded. Serve nothing.
-            return b""
+            return DirectResult(())
         else:
             self.request.response.setHeader("Content-Type", media_type)
             return self._representation(media_type).encode()
@@ -1630,7 +1648,7 @@ class EntryResource(
             media_type = self.handleConditionalGET()
             if media_type is None:
                 # The conditional GET succeeded. Serve nothing.
-                return b""
+                return DirectResult(())
             else:
                 self.request.response.setHeader("Content-Type", media_type)
                 return self._representation(media_type).encode()
@@ -2011,7 +2029,7 @@ class ServiceRootResource(HTTPResource):
         self.setCachingHeaders()
         if media_type is None:
             # The conditional GET succeeded. Serve nothing.
-            return b""
+            return DirectResult(())
         elif media_type in [self.WADL_TYPE, self.DEPRECATED_WADL_TYPE]:
             result = self.toWADL()
         elif media_type == self.JSON_TYPE:
diff --git a/src/lazr/restful/basic-site.zcml b/src/lazr/restful/basic-site.zcml
index 3e7eb2e..88e8901 100644
--- a/src/lazr/restful/basic-site.zcml
+++ b/src/lazr/restful/basic-site.zcml
@@ -8,6 +8,7 @@
     <include package="zope.component" file="meta.zcml"/>
     <include package="zope.security" file="meta.zcml"/>
     <include package="zope.component"/>
+    <include package="zope.publisher"/>
     <include package="zope.traversing"/>
     <include package="grokcore.component" file="meta.zcml" />
     <include package="lazr.enum"/>
diff --git a/src/lazr/restful/example/base/filemanager.py b/src/lazr/restful/example/base/filemanager.py
index 98c70ce..6d35a63 100644
--- a/src/lazr/restful/example/base/filemanager.py
+++ b/src/lazr/restful/example/base/filemanager.py
@@ -9,6 +9,7 @@ import hashlib
 
 import grokcore.component
 from zope.interface import implementer
+from zope.publisher.http import DirectResult
 
 from lazr.restful import ReadOnlyResource
 from lazr.restful.example.base.interfaces import IFileManager
@@ -64,7 +65,7 @@ class ManagedFileResource(ReadOnlyResource):
         incoming_etag = request.getHeader("If-None-Match")
         if incoming_etag == self.etag:
             response.setStatus(304)
-            return
+            return DirectResult(())
 
         response.setHeader("Content-Type", self.mediaType)
         response.setHeader("Last-Modified", self.last_modified.isoformat())
diff --git a/src/lazr/restful/example/base/tests/entry.txt b/src/lazr/restful/example/base/tests/entry.txt
index 045bd14..e888ca7 100644
--- a/src/lazr/restful/example/base/tests/entry.txt
+++ b/src/lazr/restful/example/base/tests/entry.txt
@@ -561,7 +561,11 @@ changed.
     >>> print(webservice.get(greens_url,
     ...                      headers={'If-None-Match': greens_etag}))
     HTTP/1.1 304 Not Modified
-    ...
+    Status: 304 Not Modified
+    Etag: "..."
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
+    <BLANKLINE>
 
 Conditional GET works the same way whether the request goes through
 the web service's virtual host or through the website-level interface
@@ -574,7 +578,11 @@ designed for Ajax.
     >>> etag = response.getheader("Etag")
     >>> print(ajax.get(greens_url, headers={'If-None-Match' : etag}))
     HTTP/1.1 304 Not Modified
-    ...
+    Status: 304 Not Modified
+    Etag: "..."
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
+    <BLANKLINE>
 
 When you make a PUT or PATCH request, you can provide the ETag as the
 If-Match header. This lets you detect changes that other people made
@@ -741,7 +749,11 @@ the condition will fail if _any_ part of the ETag is different.
     ...     greens_url,
     ...     headers={'If-None-Match': new_etag}))
     HTTP/1.1 304 Not Modified
-    ...
+    Status: 304 Not Modified
+    Etag: "..."
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
+    <BLANKLINE>
 
     >>> print(webservice.get(
     ...     greens_url,
diff --git a/src/lazr/restful/example/base/tests/field.txt b/src/lazr/restful/example/base/tests/field.txt
index ca6ec54..2bd775c 100644
--- a/src/lazr/restful/example/base/tests/field.txt
+++ b/src/lazr/restful/example/base/tests/field.txt
@@ -219,7 +219,11 @@ respond to conditional GET.
 
     >>> print(webservice.get(field_url, headers={'If-None-Match': etag}))
     HTTP/1.1 304 Not Modified
-    ...
+    Status: 304 Not Modified
+    Etag: "..."
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
+    <BLANKLINE>
 
     >>> ignored = set_description("new description")
     >>> print(webservice.get(field_url,
diff --git a/src/lazr/restful/example/base/tests/hostedfile.txt b/src/lazr/restful/example/base/tests/hostedfile.txt
index 6cafe8d..3d60f69 100644
--- a/src/lazr/restful/example/base/tests/hostedfile.txt
+++ b/src/lazr/restful/example/base/tests/hostedfile.txt
@@ -82,7 +82,10 @@ Make a second request using the ETag, and you'll get the response code 304
     >>> print(webservice.get(filemanager_url,
     ...                      headers={'If-None-Match': etag}))
     HTTP/1.1 304 Not Modified
-    ...
+    Status: 304 Not Modified
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
+    <BLANKLINE>
 
 PUT is also used to modify a hosted file. Here's one that provides a
 filename as part of Content-Disposition.
diff --git a/src/lazr/restful/example/base/tests/root.txt b/src/lazr/restful/example/base/tests/root.txt
index da8fede..f8a697a 100644
--- a/src/lazr/restful/example/base/tests/root.txt
+++ b/src/lazr/restful/example/base/tests/root.txt
@@ -78,9 +78,14 @@ current ETag, the representation won't be served again.
 
     >>> conditional_response = webservice.get(
     ...     '/', headers={'If-None-Match' : etag})
-    >>> conditional_response.status
-    304
-    >>> print(conditional_response.body.decode('UTF-8'))
+    >>> print(conditional_response)
+    HTTP/1.1 304 Not Modified
+    Status: 304 Not Modified
+    Cache-Control: max-age=2
+    Date: ...
+    Etag: "95e194e33248fa743d7066994043aa17b90c0989"
+    X-Powered-By: Zope (www.zope.org), Python (www.python.org)
+    <BLANKLINE>
     <BLANKLINE>
 
     >>> conditional_response = webservice.get(
diff --git a/src/lazr/restful/publisher.py b/src/lazr/restful/publisher.py
index 2680999..e153ec0 100644
--- a/src/lazr/restful/publisher.py
+++ b/src/lazr/restful/publisher.py
@@ -219,7 +219,11 @@ class WebServicePublicationMixin:
     def callObject(self, request, object):
         """Help web browsers handle redirects correctly."""
         value = super().callObject(request, object)
-        self._processNotifications(request)
+        if request.response.getStatus() != 304:
+            # RFC 7232 section 4.1 says that 304 responses SHOULD NOT
+            # generate headers other than a short list of headers relevant
+            # to caching.
+            self._processNotifications(request)
         if request.response.getStatus() // 100 == 3:
             if IWebBrowserInitiatedRequest.providedBy(request):
                 # This request was (probably) sent by a web
diff --git a/src/lazr/restful/testing/webservice.py b/src/lazr/restful/testing/webservice.py
index e3f8b7e..b80220c 100644
--- a/src/lazr/restful/testing/webservice.py
+++ b/src/lazr/restful/testing/webservice.py
@@ -37,7 +37,7 @@ import wsgi_intercept
 from zope.component import adapter, getGlobalSiteManager, getUtility
 from zope.configuration import xmlconfig
 from zope.interface import alsoProvides, implementer, Interface
-from zope.publisher.interfaces.http import IHTTPApplicationRequest
+from zope.publisher.interfaces.http import IHTTPApplicationRequest, IResult
 from zope.publisher.paste import Application
 from zope.proxy import ProxyBase
 from zope.schema import TextLine
@@ -137,8 +137,10 @@ class FakeResponse:
         if result is None:
             result = ""
 
-        if not isinstance(result, str):
-            raise ValueError("only strings and None results are handled")
+        if not isinstance(result, str) and not IResult.providedBy(result):
+            raise ValueError(
+                "The result should be None, a string, or an IResult."
+            )
 
         self.result = result