-
-
Couldn't load subscription status.
- Fork 33.3k
gh-135056: Add a --header CLI argument to http.server #135057
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
base: main
Are you sure you want to change the base?
Changes from 28 commits
0d02fbe
1838da7
a3256fd
77b5fff
5f89c97
a3243fe
b1026d2
6f88c13
7a793f2
9450b86
5a30d91
d317cc2
5f1fb94
c376a71
89a89f0
9653710
f3ae904
44efbed
d47c5a7
8d1286a
db9de68
e149708
c16f4c9
777b5b6
eac5c6a
c9c8083
c2d6bb3
3377cf7
06a9977
fae21f9
be78515
53965ff
c280ed8
64122df
f0d1bac
e99780e
2e829bb
8baa875
7856d27
303ab5b
ed0b0b3
79c577b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -362,7 +362,8 @@ instantiation, of which this module provides three different variants: | |||||||||
| delays, it now always returns the IP address. | ||||||||||
|
|
||||||||||
|
|
||||||||||
| .. class:: SimpleHTTPRequestHandler(request, client_address, server, directory=None) | ||||||||||
| .. class:: SimpleHTTPRequestHandler(request, client_address, server, \ | ||||||||||
| *, directory=None, extra_response_headers=None) | ||||||||||
|
|
||||||||||
| This class serves files from the directory *directory* and below, | ||||||||||
| or the current directory if *directory* is not provided, directly | ||||||||||
|
|
@@ -374,6 +375,9 @@ instantiation, of which this module provides three different variants: | |||||||||
| .. versionchanged:: 3.9 | ||||||||||
| The *directory* parameter accepts a :term:`path-like object`. | ||||||||||
|
|
||||||||||
| .. versionchanged:: next | ||||||||||
| Added *response_headers*. | ||||||||||
|
|
||||||||||
| A lot of the work, such as parsing the request, is done by the base class | ||||||||||
| :class:`BaseHTTPRequestHandler`. This class implements the :func:`do_GET` | ||||||||||
| and :func:`do_HEAD` functions. | ||||||||||
|
|
@@ -396,6 +400,13 @@ instantiation, of which this module provides three different variants: | |||||||||
| This dictionary is no longer filled with the default system mappings, | ||||||||||
| but only contains overrides. | ||||||||||
|
|
||||||||||
| .. attribute:: extra_response_headers | ||||||||||
|
|
||||||||||
| A sequence of ``(name, value)`` pairs containing user specified extra | ||||||||||
| HTTP response headers to add to each successful HTTP status 200 response. | ||||||||||
|
||||||||||
| A sequence of ``(name, value)`` pairs containing user specified extra | |
| HTTP response headers to add to each successful HTTP status 200 response. | |
| A sequence of ``(name, value)`` pairs containing user-defined extra | |
| HTTP response headers to add to each successful HTTP status 200 response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 53965ff
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| All other status code responses will not include these headers. | |
| These headers are not included in other status code responses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c280ed8
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``(name, value)`` pairs containing user specified extra response headers. | |
| ``(name, value)`` pairs containing user-defined extra response headers | |
| sent for a HTTP 200 response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 53965ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is me being nitpicky but would it be preferrable to parse header=value or header value separate by a space? what about cURL/wget and other CLIs? (I think it's easier not to expect = as you did here because we could always say that -H something means -H something 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a short discussion in the thread on python-ideas with @hugovk about this, see here
In short, cURL and wget do specify headers on their CLI as --header "name: value". In the python-ideas thread I had opposed using a name:value syntax as a positional cli parameter, since it could possibly clash with a port syntax like 127.0.0.1:8000. However if we require an explicit --header or -H before each "name: value", I'm not opposed to the idea.
I think the argument in favor of the PR as it is with -H <name> <value> is simplicity of implementation - it fits right in with argparse and nargs=2. However, if similarity with cURL / wget is desired it isn't much effort to implement a "name: value" format. I have a draft implementation in a separate branch: see aisipos@d878bf6
I'll admit to a slight leaning towards changing to the name: value format, given the principle of least surprise and the popularity of cURL and wget. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the simple space-separated stuff. It will also ease terminal shortcuts where you only delete the previous word (ctrl+w) to change for instance the value of a header. It also simplifies maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@picnixz OK, I'm fine with keeping the space separated / nargs implementation as is in this PR. I think I've made all the other changes you requested. What else remains to do for this PR?
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,18 @@ difflib | |
| (Contributed by Jiahao Li in :gh:`134580`.) | ||
|
|
||
|
|
||
| http.server | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub tells me that there is a conflict in this file, so you'll need to fix it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| ----------- | ||
|
|
||
| * Added a new ``extra_response_headers`` keyword argument to | ||
| :class:`SimpleHTTPRequestHandler` to support custom headers in HTTP responses. | ||
| (Contributed by Anton I. Sipos in :gh:`135057`.) | ||
|
|
||
| * Added a ``-H`` or ``--header`` flag to the :program:`python -m http.server` | ||
| command-line interface to support custom headers in HTTP responses. | ||
| (Contributed by Anton I. Sipos in :gh:`135057`.) | ||
|
|
||
|
|
||
| math | ||
| ---- | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -692,10 +692,11 @@ class SimpleHTTPRequestHandler(BaseHTTPRequestHandler): | |||||||||||||
| '.xz': 'application/x-xz', | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, *args, directory=None, **kwargs): | ||||||||||||||
| def __init__(self, *args, directory=None, extra_response_headers=None, **kwargs): | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it's important for instances to know whether extra_response_headers was specified (and empty) or was left unspecified? I would say "no", but this may decide whether we want to allow extensibility or not (with None, it allows instances to decide whether what is given at construction time is the default or explicitly "no headers"). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose you are considering an alternative where we don't specify I included |
||||||||||||||
| if directory is None: | ||||||||||||||
| directory = os.getcwd() | ||||||||||||||
| self.directory = os.fspath(directory) | ||||||||||||||
| self.extra_response_headers = extra_response_headers | ||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||
|
|
||||||||||||||
| def do_GET(self): | ||||||||||||||
|
|
@@ -713,6 +714,12 @@ def do_HEAD(self): | |||||||||||||
| if f: | ||||||||||||||
| f.close() | ||||||||||||||
|
|
||||||||||||||
| def _send_extra_response_headers(self): | ||||||||||||||
| """Send the headers stored in self.extra_response_headers""" | ||||||||||||||
| if self.extra_response_headers is not None: | ||||||||||||||
| for header, value in self.extra_response_headers: | ||||||||||||||
| self.send_header(header, value) | ||||||||||||||
|
|
||||||||||||||
| def send_head(self): | ||||||||||||||
| """Common code for GET and HEAD commands. | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -795,6 +802,7 @@ def send_head(self): | |||||||||||||
| self.send_header("Content-Length", str(fs[6])) | ||||||||||||||
| self.send_header("Last-Modified", | ||||||||||||||
| self.date_time_string(fs.st_mtime)) | ||||||||||||||
| self._send_extra_response_headers() | ||||||||||||||
| self.end_headers() | ||||||||||||||
| return f | ||||||||||||||
| except: | ||||||||||||||
|
|
@@ -859,6 +867,7 @@ def list_directory(self, path): | |||||||||||||
| self.send_response(HTTPStatus.OK) | ||||||||||||||
| self.send_header("Content-type", "text/html; charset=%s" % enc) | ||||||||||||||
| self.send_header("Content-Length", str(len(encoded))) | ||||||||||||||
| self._send_extra_response_headers() | ||||||||||||||
| self.end_headers() | ||||||||||||||
| return f | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -967,25 +976,33 @@ def _get_best_family(*address): | |||||||||||||
| return family, sockaddr | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _make_server(HandlerClass=BaseHTTPRequestHandler, | ||||||||||||||
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| ServerClass=ThreadingHTTPServer, | ||||||||||||||
| protocol="HTTP/1.0", port=8000, bind=None, | ||||||||||||||
| tls_cert=None, tls_key=None, tls_password=None): | ||||||||||||||
| ServerClass.address_family, addr = _get_best_family(bind, port) | ||||||||||||||
| HandlerClass.protocol_version = protocol | ||||||||||||||
|
|
||||||||||||||
| if tls_cert: | ||||||||||||||
| return ServerClass(addr, HandlerClass, certfile=tls_cert, | ||||||||||||||
| keyfile=tls_key, password=tls_password) | ||||||||||||||
| else: | ||||||||||||||
| return ServerClass(addr, HandlerClass) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test(HandlerClass=BaseHTTPRequestHandler, | ||||||||||||||
| ServerClass=ThreadingHTTPServer, | ||||||||||||||
| protocol="HTTP/1.0", port=8000, bind=None, | ||||||||||||||
| tls_cert=None, tls_key=None, tls_password=None): | ||||||||||||||
| """Test the HTTP request handler class. | ||||||||||||||
|
|
||||||||||||||
| This runs an HTTP server on port 8000 (or the port argument). | ||||||||||||||
|
|
||||||||||||||
| """ | ||||||||||||||
| ServerClass.address_family, addr = _get_best_family(bind, port) | ||||||||||||||
| HandlerClass.protocol_version = protocol | ||||||||||||||
|
|
||||||||||||||
| if tls_cert: | ||||||||||||||
| server = ServerClass(addr, HandlerClass, certfile=tls_cert, | ||||||||||||||
| keyfile=tls_key, password=tls_password) | ||||||||||||||
| else: | ||||||||||||||
| server = ServerClass(addr, HandlerClass) | ||||||||||||||
|
|
||||||||||||||
| with server as httpd: | ||||||||||||||
| with _make_server( | ||||||||||||||
| HandlerClass=HandlerClass, ServerClass=ServerClass, | ||||||||||||||
| protocol=protocol, port=port, bind=bind, tls_cert=tls_cert, | ||||||||||||||
| tls_key=tls_key, tls_password=tls_password | ||||||||||||||
|
||||||||||||||
| HandlerClass=HandlerClass, ServerClass=ServerClass, | |
| protocol=protocol, port=port, bind=bind, tls_cert=tls_cert, | |
| tls_key=tls_key, tls_password=tls_password | |
| HandlerClass=HandlerClass, ServerClass=ServerClass, | |
| protocol=protocol, port=port, bind=bind, | |
| tls_cert=tls_cert, tls_key=tls_key, tls_password=tls_password, |
I would prefer this split just to group the tls_* arguments together. I think it fits on 80 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 64122df. It ends up being only 68 chars so it fits easily under 80. I didn't leave in a trailing comma, if that matters I can add it in.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The can be used multiple times is already implied with `action='append' I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in the code the multiple hint is obvious, but I added this for users running --help in the CLI, which will show with python -m http.server --help:
-H, --header HEADER VALUE
Add a custom response header (can be used multiple times)
argparse doesn't give us an indicator in the --help output automatically for action-'append', so I added an explicit hint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. I think this should be improved in argparse then.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| '(can be used multiple times)') | |
| '(can be specified multiple times)') |
The trace.py argparse uses "specified"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f0d1bac
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -465,8 +465,16 @@ def test_err(self): | |||||
| self.assertEndsWith(lines[1], '"ERROR / HTTP/1.1" 404 -') | ||||||
|
|
||||||
|
|
||||||
| class CustomHeaderSimpleHTTPRequestHandler(SimpleHTTPRequestHandler): | ||||||
| extra_response_headers = None | ||||||
|
|
||||||
| def __init__(self, *args, **kwargs): | ||||||
| kwargs.setdefault('extra_response_headers', self.extra_response_headers) | ||||||
| super().__init__(*args, **kwargs) | ||||||
|
|
||||||
|
|
||||||
| class SimpleHTTPServerTestCase(BaseTestCase): | ||||||
| class request_handler(NoLogRequestHandler, SimpleHTTPRequestHandler): | ||||||
| class request_handler(NoLogRequestHandler, CustomHeaderSimpleHTTPRequestHandler): | ||||||
| pass | ||||||
|
|
||||||
| def setUp(self): | ||||||
|
|
@@ -823,6 +831,28 @@ def test_path_without_leading_slash(self): | |||||
| self.assertEqual(response.getheader("Location"), | ||||||
| self.tempdir_name + "/?hi=1") | ||||||
|
|
||||||
| def test_extra_headers_list_dir(self): | ||||||
|
||||||
| with mock.patch.object(self.request_handler, 'extra_response_headers', new=[ | ||||||
| ('X-Test1', 'test1'), | ||||||
| ('X-Test2', 'test2'), | ||||||
| ]): | ||||||
| response = self.request(self.base_url + '/') | ||||||
| self.assertEqual(response.getheader("X-Test1"), 'test1') | ||||||
| self.assertEqual(response.getheader("X-Test2"), 'test2') | ||||||
|
|
||||||
| def test_extra_response_headers_get_file(self): | ||||||
| with mock.patch.object(self.request_handler, 'extra_response_headers', new=[ | ||||||
|
||||||
| with mock.patch.object(self.request_handler, 'extra_response_headers', new=[ | |
| with mock.patch.object(self.request_handler, 'extra_response_headers', [ |
no need for using the 'new' keyword (same for the previous test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e99780e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensure 2 blank lines around classes (there is one blank line missing after this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2e829bb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to check this as --header is only used by the handler and not the server test server itself. Ideally, we should also extract the logic of making the parser to check that the parsed arguments are of the expected form (namely a sequence of pairs).
You can however check bad usages of --header (e.g., --header h1 should raise, and --header h1 h2 h3 as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some new tests to verify exceptions with incorrect header CLI usage in c2d6bb3.
I agree that test_header_flag doesn't quite do what we want. I only put it there to ensure all the argument parsing and logic in _main() doesn't raise in the presence of the new arguments before calling test(). I agree that refactoring out the parser logic would allow us to test this properly. However, I've been trying to keep the changes in this PR relatively small. If you feel though that a refactor is the better choice, I'm happy to do that.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Add a ``-H`` or ``--header`` CLI option to :program:`python -m http.server`. Contributed by | ||
| Anton I. Sipos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 06a9977 and be78515