Support configurably omitting server header #187
Conversation
|
@NotBobTheBuilder PRs must pass tests. Failing test runs can be found here: Also you must sign the CONTRIBUTORS.txt file. I will update this repo with adapted HACKING.txt and contributing.md files. Thank you! |
|
Python 3.3 support should be removed, so ignore that error from Travis. However I do require 100% test coverage. |
|
Thanks folks. Pushed further changes as requested! |
waitress/adjustments.py
Outdated
|
|
||
| def str_ifnotnone(s): | ||
| if s is None: | ||
| return None |
There was a problem hiding this comment.
This line is not yet covered by the test. The test is using a bunch of dummies, instead you'll want to add to https://github.com/Pylons/waitress/blob/master/waitress/tests/test_adjustments.py
waitress/task.py
Outdated
| if not server_header: | ||
| response_headers.append(('Server', ident)) | ||
| else: | ||
| response_headers.append(('Via', ident)) |
There was a problem hiding this comment.
This should not be removed if ident is not set. We always went to send a Via header even if there is no user provided ident to make sure that that if waitress is used as a proxy it can't be used to do proxy looping. If there is no user provided ident, we need to make sure to set it to something reasonable like "waitress".
There was a problem hiding this comment.
I'm okay with dropping the Server header if none is set by the WSGI app, but we should absolutely set a Via header if a Server header is set by the WSGI app (likely means that waitress is being used as some sort of proxy).
If you also want to drop any potentially proxied Server header, I would recommend using some WSGI middleware to drop it from the response before it gets to Waitress, and then Waitress won't add a Via header.
waitress/tests/test_init.py
Outdated
| self.assertEqual(server.ran, True) | ||
|
|
||
| def test_empty_server_header(self): | ||
| server = DummyServerFactory() |
There was a problem hiding this comment.
DummyServerFactory uses a DummyAdjustment.
|
Looking good! @NotBobTheBuilder one more thing, please. Insert a changelog entry: Unreleased (yyyy-mm-dd)
-----------------------
- my feature description. See https://github.com/Pylons/waitress/pull/187@bertjwregeer would any updates to the docs be needed for this change, other than the changelog entry? |
|
@stevepiercy no, there are already docs for @NotBobTheBuilder I think it might also be nice to drop the |
|
Thanks for all this feedback - updating PR accordingly! |
waitress/task.py
Outdated
| if ident: | ||
| response_headers.append(('Server', ident)) | ||
| else: | ||
| response_headers.append(('Via', ident)) |
There was a problem hiding this comment.
If ident is falsey here, we should set it to 'waitress'. We should not append an empty Via header.
waitress/adjustments.py
Outdated
| return s | ||
|
|
||
| def str_iftruthy(s): | ||
| return str(s) if s else s |
There was a problem hiding this comment.
Shouldn't this be return str(s) if s else None?
waitress/tests/test_adjustments.py
Outdated
| self.assertEqual(inst.ident, None) | ||
|
|
||
| inst = self._makeOne(ident='') | ||
| self.assertEqual(inst.ident, '') |
There was a problem hiding this comment.
In this case the ident should be None. So that if a user sets it to an empty string in a config file, there is no ident sent.
|
@NotBobTheBuilder Awesome, thanks very much. This looks good. Let me give this one more look over later tonight but it looks ready for merging! |
|
Anything further needed from me on this one? :) |
|
No, I'm sorry I just haven't gotten back around to it. |
|
No worries - just wanted to make sure I wasn't blocking anything! |
3 participants
Hello Waitress Project!
We use Waitress in a number of our projects and recently noticed that it's not possible to withhold the Server header from HTTP responses, with the closest option being to set it to an empty string.
In line with the suggestion of configurability in RFC2616[0] we've filed this PR to add support for this choice by specifying
waitress.serve(ident=None).Hope this is helpful.
Thanks!
[0] https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.38