Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configurably omitting server header #187

Merged
merged 6 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@NotBobTheBuilder
Copy link
Contributor

NotBobTheBuilder commented May 18, 2018

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

@stevepiercy

This comment has been minimized.

Copy link
Member

stevepiercy commented May 18, 2018

@NotBobTheBuilder PRs must pass tests. Failing test runs can be found here:
https://travis-ci.org/Pylons/waitress/builds/380907838

Also you must sign the CONTRIBUTORS.txt file.

I will update this repo with adapted HACKING.txt and contributing.md files.

Thank you!

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented May 18, 2018

Python 3.3 support should be removed, so ignore that error from Travis.

However I do require 100% test coverage.

@NotBobTheBuilder

This comment has been minimized.

Copy link
Contributor Author

NotBobTheBuilder commented May 19, 2018

Thanks folks.

Pushed further changes as requested!

@@ -66,6 +66,11 @@ def slash_fixed_str(s):
s = '/' + s.lstrip('/').rstrip('/')
return s

def str_ifnotnone(s):
if s is None:
return None

This comment has been minimized.

@bertjwregeer

bertjwregeer May 19, 2018

Member

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

if not server_header:
response_headers.append(('Server', ident))
else:
response_headers.append(('Via', ident))

This comment has been minimized.

@bertjwregeer

bertjwregeer May 19, 2018

Member

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".

This comment has been minimized.

@bertjwregeer

bertjwregeer May 19, 2018

Member

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.

@@ -14,6 +14,13 @@ def test_it(self):
self.assertEqual(result, None)
self.assertEqual(server.ran, True)

def test_empty_server_header(self):
server = DummyServerFactory()

This comment has been minimized.

@bertjwregeer

bertjwregeer May 19, 2018

Member

DummyServerFactory uses a DummyAdjustment.

@stevepiercy

This comment has been minimized.

Copy link
Member

stevepiercy commented May 19, 2018

Looking good! @NotBobTheBuilder one more thing, please. Insert a changelog entry:
https://github.com/Pylons/waitress/blob/master/CHANGES.txt#L1

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?

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented May 19, 2018

@stevepiercy no, there are already docs for ident. This just changes the behaviour slightly.

@NotBobTheBuilder I think it might also be nice to drop the ident if it set to an empty string from a config file, when not using it through the Python API directly.

@NotBobTheBuilder

This comment has been minimized.

Copy link
Contributor Author

NotBobTheBuilder commented May 21, 2018

Thanks for all this feedback - updating PR accordingly!

@@ -251,9 +251,11 @@ def close_on_finish():
# if the server is used as a proxy.
ident = self.channel.server.adj.ident
if not server_header:
response_headers.append(('Server', ident))
if ident:
response_headers.append(('Server', ident))
else:
response_headers.append(('Via', ident))

This comment has been minimized.

@bertjwregeer

bertjwregeer May 21, 2018

Member

If ident is falsey here, we should set it to 'waitress'. We should not append an empty Via header.

@@ -66,6 +66,9 @@ def slash_fixed_str(s):
s = '/' + s.lstrip('/').rstrip('/')
return s

def str_iftruthy(s):
return str(s) if s else s

This comment has been minimized.

@bertjwregeer

bertjwregeer May 21, 2018

Member

Shouldn't this be return str(s) if s else None?

self.assertEqual(inst.ident, None)

inst = self._makeOne(ident='')
self.assertEqual(inst.ident, '')

This comment has been minimized.

@bertjwregeer

bertjwregeer May 21, 2018

Member

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.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented May 22, 2018

@NotBobTheBuilder Awesome, thanks very much. This looks good. Let me give this one more look over later tonight but it looks ready for merging!

@NotBobTheBuilder

This comment has been minimized.

Copy link
Contributor Author

NotBobTheBuilder commented Jul 10, 2018

Anything further needed from me on this one? :)

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jul 10, 2018

No, I'm sorry I just haven't gotten back around to it.

@NotBobTheBuilder

This comment has been minimized.

Copy link
Contributor Author

NotBobTheBuilder commented Jul 10, 2018

No worries - just wanted to make sure I wasn't blocking anything!

@bertjwregeer bertjwregeer merged commit 702bc67 into Pylons:master Aug 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@NotBobTheBuilder NotBobTheBuilder deleted the NotBobTheBuilder:suppress-empty-server-header branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.