-
-
Couldn't load subscription status.
- Fork 33.2k
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 1 commit
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,7 @@ 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, 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 +374,10 @@ instantiation, of which this module provides three different variants: | |||||
| .. versionchanged:: 3.9 | ||||||
| The *directory* parameter accepts a :term:`path-like object`. | ||||||
|
|
||||||
| .. versionchanged:: 3.15 | ||||||
hugovk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| The *response_headers* parameter accepts an optional dictionary of | ||||||
|
||||||
| The *response_headers* parameter accepts an optional dictionary of | |
| Added *response_headers*, which accepts an optional dictionary of |
Also, did you consider accepting a list or iterable of (name, value) pairs instead, like returned by http.client.HTTPResponse.getheaders? That would be better for sending multiple Set-Cookie fields.
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.
Ah yes, sending multiple headers of the same name would indeed be necessary. I updated to use an iterable of name value pairs instead in 7a793f2
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.
Might be worth clarifying how these fields interact with other fields such as Server specified under BaseHTTPRequestHandler.send_response, and Last-Modified under do_GET.
Also clarify which responses the fields are included in, assuming it wasn’t your intention to include them for 404 Not Found, 304 Not Modified, lower-level errors, etc.
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.
In the latest commits, I've noted that the custom headers are only sent in success cases. What do you mean by interaction though? The custom headers currently get sent after Last-Modified, should I mention the placement of the custom headers and that they appear after Last-Modified?
aisipos marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
Isn’t this redundant with the entry already under the constructor heading?
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.
Perhaps - it seems the constructor documentation is used to make a brief mention of each argument and when it was added, with more detail being filled in later. My latest commits make several changes requested elsewhere for other reasons, but if the current version is still too redundant in multiple places I can make some more edits.
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 should add this information. There is no notion of an "instance argument": it should rather be an instance attribute, and this should be documented through a .. attribute::, below .. attribute:: extensions_map
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.
As Hugo said, since we're anyway exposing response-headers, I think we should also expose it from the CLI. It could be useful for users in general (e.g., --add-header NAME VALUE with the -H alias).
aisipos marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -117,13 +117,22 @@ class HTTPServer(socketserver.TCPServer): | |||||||||||||||||||||
| allow_reuse_address = True # Seems to make sense in testing environment | ||||||||||||||||||||||
| allow_reuse_port = True | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def __init__(self, *args, response_headers=None, **kwargs): | ||||||||||||||||||||||
| self.response_headers = response_headers if response_headers is not None else {} | ||||||||||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def server_bind(self): | ||||||||||||||||||||||
| """Override server_bind to store the server name.""" | ||||||||||||||||||||||
| socketserver.TCPServer.server_bind(self) | ||||||||||||||||||||||
| host, port = self.server_address[:2] | ||||||||||||||||||||||
| self.server_name = socket.getfqdn(host) | ||||||||||||||||||||||
| self.server_port = port | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def finish_request(self, request, client_address): | ||||||||||||||||||||||
| """Finish one request by instantiating RequestHandlerClass.""" | ||||||||||||||||||||||
| args = (request, client_address, self) | ||||||||||||||||||||||
| kwargs = dict(response_headers=self.response_headers) if self.response_headers else dict() | ||||||||||||||||||||||
| self.RequestHandlerClass(*args, **kwargs) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class ThreadingHTTPServer(socketserver.ThreadingMixIn, HTTPServer): | ||||||||||||||||||||||
| daemon_threads = True | ||||||||||||||||||||||
|
|
@@ -132,7 +141,7 @@ class ThreadingHTTPServer(socketserver.ThreadingMixIn, HTTPServer): | |||||||||||||||||||||
| class HTTPSServer(HTTPServer): | ||||||||||||||||||||||
| def __init__(self, server_address, RequestHandlerClass, | ||||||||||||||||||||||
| bind_and_activate=True, *, certfile, keyfile=None, | ||||||||||||||||||||||
| password=None, alpn_protocols=None): | ||||||||||||||||||||||
| password=None, alpn_protocols=None, response_headers=None): | ||||||||||||||||||||||
|
||||||||||||||||||||||
| password=None, alpn_protocols=None, response_headers=None): | |
| password=None, alpn_protocols=None, **http_server_kwargs): |
And pass http_server_kwargs to super()
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.
| def __init__(self, *args, directory=None, response_headers=None, **kwargs): | |
| if directory is None: | |
| directory = os.getcwd() | |
| self.directory = os.fspath(directory) | |
| self.response_headers = response_headers or {} | |
| def __init__(self, *args, directory=None, response_headers=None, **kwargs): | |
| if directory is None: | |
| directory = os.getcwd() | |
| self.directory = os.fspath(directory) | |
| self.response_headers = response_headers |
You're already checking for is not None later
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.
let's make it a private method, say self._add_custom_response_headers or something like that
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.
Or is moving this to an extended send_response override an option? That way you would include the fields for all responses.
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.
| tls_cert=None, tls_key=None, tls_password=None, response_headers=None): | |
| tls_cert=None, tls_key=None, tls_password=None, | |
| response_headers=None): |
Let's group the parameters per purpose
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.
| handler_args = (request, client_address, self) | |
| handler_kwargs = dict(directory=args.directory) | |
| if self.response_headers: | |
| handler_kwargs['response_headers'] = self.response_headers | |
| self.RequestHandlerClass(*handler_args, **handler_kwargs) | |
| self.RequestHandlerClass(request, client_address, self, | |
| directory=args.directory, | |
| response_headers=self.response_headers) |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,11 +81,12 @@ def test_https_server_raises_runtime_error(self): | |||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class TestServerThread(threading.Thread): | ||||||||||||||
| def __init__(self, test_object, request_handler, tls=None): | ||||||||||||||
| def __init__(self, test_object, request_handler, tls=None, server_kwargs=None): | ||||||||||||||
| threading.Thread.__init__(self) | ||||||||||||||
| self.request_handler = request_handler | ||||||||||||||
| self.test_object = test_object | ||||||||||||||
| self.tls = tls | ||||||||||||||
| self.server_kwargs = server_kwargs or {} | ||||||||||||||
|
|
||||||||||||||
| def run(self): | ||||||||||||||
| if self.tls: | ||||||||||||||
|
|
@@ -95,7 +96,8 @@ def run(self): | |||||||||||||
| request_handler=self.request_handler, | ||||||||||||||
| ) | ||||||||||||||
| else: | ||||||||||||||
| self.server = HTTPServer(('localhost', 0), self.request_handler) | ||||||||||||||
| self.server = HTTPServer(('localhost', 0), self.request_handler, | ||||||||||||||
|
||||||||||||||
| **self.server_kwargs) | ||||||||||||||
|
||||||||||||||
| self.test_object.HOST, self.test_object.PORT = self.server.socket.getsockname() | ||||||||||||||
| self.test_object.server_started.set() | ||||||||||||||
| self.test_object = None | ||||||||||||||
|
|
@@ -113,12 +115,14 @@ class BaseTestCase(unittest.TestCase): | |||||||||||||
|
|
||||||||||||||
| # Optional tuple (certfile, keyfile, password) to use for HTTPS servers. | ||||||||||||||
| tls = None | ||||||||||||||
| server_kwargs = None | ||||||||||||||
|
|
||||||||||||||
| def setUp(self): | ||||||||||||||
| self._threads = threading_helper.threading_setup() | ||||||||||||||
| os.environ = os_helper.EnvironmentVarGuard() | ||||||||||||||
| self.server_started = threading.Event() | ||||||||||||||
| self.thread = TestServerThread(self, self.request_handler, self.tls) | ||||||||||||||
| self.thread = TestServerThread(self, self.request_handler, self.tls, | ||||||||||||||
| self.server_kwargs) | ||||||||||||||
| self.thread.start() | ||||||||||||||
| self.server_started.wait() | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -824,6 +828,16 @@ def test_path_without_leading_slash(self): | |||||||||||||
| self.tempdir_name + "/?hi=1") | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class CorsHTTPServerTestCase(SimpleHTTPServerTestCase): | ||||||||||||||
| server_kwargs = dict( | ||||||||||||||
| response_headers = {'Access-Control-Allow-Origin': '*'} | ||||||||||||||
| ) | ||||||||||||||
|
||||||||||||||
| server_kwargs = dict( | |
| response_headers = {'Access-Control-Allow-Origin': '*'} | |
| ) | |
| server_kwargs = { | |
| 'response_headers': {'Access-Control-Allow-Origin': '*'} | |
| } |
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.
| def test_cors(self): | |
| def test_cors(self): |
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 ``--cors`` cli option to ``python -m http.server``. Contributed by | ||
hugovk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Anton I. Sipos | ||
hugovk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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.
Document that
directoryandresponse_headersare keyword arguments actually.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.
Done in c376a71