Repository: cdent/paste

Issue:
gzip middleware adds a HTTP body when it shouldn't (e.g., HEAD requests)
The [HTTP specification says](https://tools.ietf.org/html/rfc7231#section-4.3.2):

> the server MUST NOT send a message body in the response (i.e., the response terminates at the end of the header section)

Even if the WSGI app is careful about this, the gzip middleware can violate this if the client is willing to accept gzip responses. This happens because simply opening the `GzipFile`, even if no bytes are written to it, will result in writing the gzip header. 

This is a problem because the spec doesn't say what happens if a server violates this MUST NOT clause. We've observed an upstream HAProxy 2 producing unexpected 502 errors to the originating client due to this (most likely in the presence of HTTP pipelining or keep-alive connections).

This can be produced easily. Given this python script, installed in a Python 3.x environment with Paste 3.2.5:
```python
# paste_test.py
from wsgiref.simple_server import make_server
from paste.gzipper import middleware

def app(environ, start_response):
    start_response('200 OK',
                   [('Content-Type', 'text/plain')])
    return []

gzipped = middleware(app)

with make_server('', 8080, gzipped) as httpd:
    httpd.serve_forever()
```

If we start it and make a request that *doesn't* allow a gzip response, we get back an empty body, and the server logs it as such:
```console
$ python /tmp/paste_test.py &
[1] 79887
$ echo -e 'HEAD / HTTP/1.0\r\nAccept-Encoding: identity\r\n\r\n' | socat TCP4:localhost:8080 -
HTTP/1.0 200 OK
Date: Fri, 10 Jan 2020 13:08:50 GMT
Server: WSGIServer/0.2 CPython/3.8.1
Content-Type: text/plain
Content-Length: 0

127.0.0.1 - - [10/Jan/2020 07:08:50] "HEAD / HTTP/1.0" 200 0
```

Now, if we allow a gzip response, we can see that we get 20 bytes of gibberish data:
```console
$ echo -e 'HEAD / HTTP/1.0\r\nAccept-Encoding: gzip\r\n\r\n' | socat TCP4:localhost:8080 -
HTTP/1.0 200 OK
Date: Fri, 10 Jan 2020 13:08:58 GMT
Server: WSGIServer/0.2 CPython/3.8.1
Content-Type: text/plain
content-encoding: gzip
Content-Length: 20

jw^�127.0.0.1 - - [10/Jan/2020 07:08:58] "HEAD / HTTP/1.0" 200 20
```

Which is exactly what you get simply opening and closing a `GzipFile`:
```pycon
>>> import io, gzip
>>> buf = io.BytesIO()
>>> f = gzip.GzipFile(mode='wb', fileobj=buf)
>>> buf.getvalue() # Header has already been written
b'\x1f\x8b\x08\x00%{\x18^\x02\xff'
>>> len(buf.getvalue())
10
>>> f.close()
None
>>> len(buf.getvalue()) # trailer has been added
20
```

I think a fix could be pretty simple, checking against the `REQUEST_METHOD`:
```diff
--- gzipper.py.orig	2020-01-10 07:10:16.406705538 -0600
+++ gzipper.py	2020-01-10 07:11:38.635141468 -0600
@@ -25,7 +25,8 @@
         self.compress_level = int(compress_level)

     def __call__(self, environ, start_response):
-        if 'gzip' not in environ.get('HTTP_ACCEPT_ENCODING', ''):
+        if 'gzip' not in environ.get('HTTP_ACCEPT_ENCODING', '') \
+           or environ['REQUEST_METHOD'] == 'HEAD':
             # nothing for us to do, so this middleware will
             # be a no-op:
             return self.application(environ, start_response)
```

I'm happy to work up a PR if desired.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=h1) Report
> Merging [#44](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=desc) into [master](https://codecov.io/gh/cdent/paste/commit/1cc7e73f9980618f620d8e176858fe8f59ed03fc?src=pr&el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/cdent/paste/pull/44/graphs/tree.svg?width=650&token=mPaT8M4yhm&height=150&src=pr)](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   54.01%   54.08%   +0.06%     
==========================================
  Files         125      125              
  Lines       14816    14822       +6     
  Branches     2500     2500              
==========================================
+ Hits         8003     8016      +13     
+ Misses       6267     6262       -5     
+ Partials      546      544       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #py27 | `52.98% <100%> (+0.06%)` | :arrow_up: |
| #py35 | `52.33% <100%> (+0.06%)` | :arrow_up: |
| #py36 | `52.33% <100%> (+0.06%)` | :arrow_up: |
| #py37 | `52.32% <100%> (+0.06%)` | :arrow_up: |
| #pypy | `52.25% <100%> (+0.06%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [paste/gzipper.py](https://codecov.io/gh/cdent/paste/pull/44/diff?src=pr&el=tree#diff-cGFzdGUvZ3ppcHBlci5weQ==) | `75.71% <100%> (+2.85%)` | :arrow_up: |
| [tests/test\_gzipper.py](https://codecov.io/gh/cdent/paste/pull/44/diff?src=pr&el=tree#diff-dGVzdHMvdGVzdF9nemlwcGVyLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [paste/fixture.py](https://codecov.io/gh/cdent/paste/pull/44/diff?src=pr&el=tree#diff-cGFzdGUvZml4dHVyZS5weQ==) | `47.03% <100%> (+0.22%)` | :arrow_up: |
| [paste/progress.py](https://codecov.io/gh/cdent/paste/pull/44/diff?src=pr&el=tree#diff-cGFzdGUvcHJvZ3Jlc3MucHk=) | `0% <0%> (ø)` | :arrow_up: |
| [paste/response.py](https://codecov.io/gh/cdent/paste/pull/44/diff?src=pr&el=tree#diff-cGFzdGUvcmVzcG9uc2UucHk=) | `69% <0%> (+4%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=footer). Last update [1cc7e73...49b36e4](https://codecov.io/gh/cdent/paste/pull/44?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

cdent: > It seems news.txt is maintained separately

Yeah, that's correct, I'll update that accordingly.

Thanks for including the nice tests.

Patch:
diff --git a/paste/fixture.py b/paste/fixture.py
index 4b88718..9f175fb 100644
--- a/paste/fixture.py
+++ b/paste/fixture.py
@@ -299,6 +299,18 @@ class TestApp(object):
                                  extra_environ=extra_environ,status=status,
                                  upload_files=None, expect_errors=expect_errors)
 
+    def head(self, url, headers=None, extra_environ=None,
+             status=None, expect_errors=False):
+        """
+        Do a HEAD request.  Very like the ``.get()`` method.
+
+        Returns a `response object
+        <class-paste.fixture.TestResponse.html>`_
+        """
+        return self._gen_request('HEAD', url, headers=headers,
+                                 extra_environ=extra_environ,status=status,
+                                 upload_files=None, expect_errors=expect_errors)
+
 
 
 
diff --git a/paste/gzipper.py b/paste/gzipper.py
index eca8775..0882c67 100644
--- a/paste/gzipper.py
+++ b/paste/gzipper.py
@@ -25,9 +25,12 @@ class middleware(object):
         self.compress_level = int(compress_level)
 
     def __call__(self, environ, start_response):
-        if 'gzip' not in environ.get('HTTP_ACCEPT_ENCODING', ''):
+        if 'gzip' not in environ.get('HTTP_ACCEPT_ENCODING', '') \
+           or environ['REQUEST_METHOD'] == 'HEAD':
             # nothing for us to do, so this middleware will
-            # be a no-op:
+            # be a no-op (there's no body expected in the HEAD case,
+            # and if we open a GzipFile we would produce an erroneous
+            # 20-byte header and trailer):
             return self.application(environ, start_response)
         response = GzipResponse(start_response, self.compress_level)
         app_iter = self.application(environ,


Test patch:
diff --git a/tests/test_gzipper.py b/tests/test_gzipper.py
index 54b7901..d56c406 100644
--- a/tests/test_gzipper.py
+++ b/tests/test_gzipper.py
@@ -4,8 +4,10 @@ import gzip
 import six
 
 def simple_app(environ, start_response):
-    start_response('200 OK', [('content-type', 'text/plain')])
-    return [b'this is a test']
+    start_response('200 OK',
+                   [('content-type', 'text/plain'),
+                    ('content-length', '0')])
+    return [b'this is a test'] if environ['REQUEST_METHOD'] != 'HEAD' else []
 
 wsgi_app = middleware(simple_app)
 app = TestApp(wsgi_app)
@@ -17,3 +19,9 @@ def test_gzip():
     assert res.body != b'this is a test'
     actual = gzip.GzipFile(fileobj=six.BytesIO(res.body)).read()
     assert actual == b'this is a test'
+
+def test_gzip_head():
+    res = app.head(
+        '/', extra_environ=dict(HTTP_ACCEPT_ENCODING='gzip'))
+    assert int(res.header('content-length')) == 0
+    assert res.body == b''