Repository: tornadoweb/tornado

Issue:
Incorrectly removes ‘Allow’ header from HTTP 204 response
In this application, Tornado (6.0.3) filters the `Allow` header out of the 204 response to an `OPTIONS` request, while the misspelled `Alloww` header is passed through. [RFC 7231 §7.4.1](https://tools.ietf.org/html/rfc7231#section-7.4.1) is clear that an `Allow` header MAY be sent on any response, and [§4.3.7](https://tools.ietf.org/html/rfc7231#section-4.3.7) says that it SHOULD be sent if applicable in response to an `OPTIONS` request, so Tornado should not filter it out.

```python
import tornado.ioloop
import tornado.web

class MainHandler(tornado.web.RequestHandler):
    def get(self):
        self.write("Hello, world")

    def options(self):
        self.set_header("Allow", "GET, OPTIONS")
        self.set_header("Alloww", "GET, OPTIONS")
        self.set_status(204)
        self.finish()

if __name__ == "__main__":
    app = tornado.web.Application([(r"/", MainHandler)])
    app.listen(8888)
    tornado.ioloop.IOLoop.current().start()
```

```console
$ curl -i -X OPTIONS http://localhost:8888
HTTP/1.1 204 No Content
Server: TornadoServer/6.0.3
Date: Mon, 12 Aug 2019 01:49:10 GMT
Alloww: GET, OPTIONS
```


Discussion:
bdarnell: We should definitely permit the `Allow` header for 204 responses, but the name `_clear_headers_for_304` is no longer literally accurate so I don't know if this is the change we want to make.

The primary purpose of this method is to clear the `Content-` headers from responses with no body, especially the `Content-Type` header that is added by `set_default_headers`. It's called for 1xx, 204, and 304 responses. I think we still want to do at least this much.

The RFCs have changed significantly since this function was written. It cites the definition of [entity headers](http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.1) from RFC 2616 §7.1, which appears to have been replaced by a much shorter list of [representation metadata headers](https://tools.ietf.org/html/rfc7231#section-3.1) in RFC 7231 §3.1 (`Allow` is no longer on the list). [RFC 2616 §10.3.5](https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.5) said that 304 responses MUST NOT contain other entity headers, while [RFC 7232 §4.1](https://tools.ietf.org/html/rfc7232#section-4.1) makes it a SHOULD NOT (and `Last-Modified` is given as an example of a header that's sometimes useful). 

So this is what I'd do: rename `_clear_headers_for_304` to `_clear_representation_headers`, change it to only clear `Content-Type`, `Content-Language`, and `Content-Encoding`, and keep calling it for all 1xx responses, 204, and 304. Make sense?
andersk: Sure. Updated as such.
bdarnell: Looks good, now the test `test_static_304_if_modified_since` just needs to be fixed up.

Patch:
diff --git a/tornado/web.py b/tornado/web.py
index adbf591e..9412ab44 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -1138,7 +1138,7 @@ class RequestHandler(object):
                 assert not self._write_buffer, (
                     "Cannot send body with %s" % self._status_code
                 )
-                self._clear_headers_for_304()
+                self._clear_representation_headers()
             elif "Content-Length" not in self._headers:
                 content_length = sum(len(part) for part in self._write_buffer)
                 self.set_header("Content-Length", content_length)
@@ -1803,21 +1803,13 @@ class RequestHandler(object):
     def _ui_method(self, method: Callable[..., str]) -> Callable[..., str]:
         return lambda *args, **kwargs: method(self, *args, **kwargs)
 
-    def _clear_headers_for_304(self) -> None:
-        # 304 responses should not contain entity headers (defined in
-        # http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.1)
+    def _clear_representation_headers(self) -> None:
+        # 304 responses should not contain representation metadata
+        # headers (defined in
+        # https://tools.ietf.org/html/rfc7231#section-3.1)
         # not explicitly allowed by
-        # http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.5
-        headers = [
-            "Allow",
-            "Content-Encoding",
-            "Content-Language",
-            "Content-Length",
-            "Content-MD5",
-            "Content-Range",
-            "Content-Type",
-            "Last-Modified",
-        ]
+        # https://tools.ietf.org/html/rfc7232#section-4.1
+        headers = ["Content-Encoding", "Content-Language", "Content-Type"]
         for h in headers:
             self.clear_header(h)
 


Test patch:
diff --git a/tornado/test/httpclient_test.py b/tornado/test/httpclient_test.py
index d1cc1467..11a4ee02 100644
--- a/tornado/test/httpclient_test.py
+++ b/tornado/test/httpclient_test.py
@@ -109,7 +109,7 @@ class ContentLength304Handler(RequestHandler):
         self.set_status(304)
         self.set_header("Content-Length", 42)
 
-    def _clear_headers_for_304(self):
+    def _clear_representation_headers(self):
         # Tornado strips content-length from 304 responses, but here we
         # want to simulate servers that include the headers anyway.
         pass
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index 5908710a..4a06c12d 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -1175,7 +1175,6 @@ class StaticFileTest(WebTestCase):
         )
         self.assertEqual(response2.code, 304)
         self.assertTrue("Content-Length" not in response2.headers)
-        self.assertTrue("Last-Modified" not in response2.headers)
 
     def test_static_304_if_none_match(self):
         response1 = self.get_and_head("/static/robots.txt")