-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
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 5 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
526e499
46c1c91
c1fee3b
3a4fed6
9f0ed01
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:: next | ||||||
| The *response_headers* parameter accepts an optional dictionary of | ||||||
|
Member
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. In previous versions, this was not a valid parameter at all.
Suggested change
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.
Author
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. 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 |
||||||
| additional HTTP headers to add to each response. | ||||||
|
Member
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. 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.
Author
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. 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 |
||||||
|
|
||||||
| 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. | ||||||
|
|
@@ -428,6 +432,9 @@ instantiation, of which this module provides three different variants: | |||||
| followed by a ``'Content-Length:'`` header with the file's size and a | ||||||
| ``'Last-Modified:'`` header with the file's modification time. | ||||||
|
|
||||||
| The headers specified in the dictionary instance argument | ||||||
| ``response_headers`` are each individually sent in the response. | ||||||
|
|
||||||
| Then follows a blank line signifying the end of the headers, and then the | ||||||
| contents of the file are output. | ||||||
|
|
||||||
|
|
@@ -437,6 +444,9 @@ instantiation, of which this module provides three different variants: | |||||
| .. versionchanged:: 3.7 | ||||||
| Support of the ``'If-Modified-Since'`` header. | ||||||
|
|
||||||
| .. versionchanged:: next | ||||||
| Support ``response_headers`` as an instance argument. | ||||||
|
Member
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. Isn’t this redundant with the entry already under the constructor heading?
Author
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. 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.
Member
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 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 |
||||||
|
|
||||||
| The :class:`SimpleHTTPRequestHandler` class can be used in the following | ||||||
| manner in order to create a very basic webserver serving files relative to | ||||||
| the current directory:: | ||||||
|
|
@@ -543,6 +553,14 @@ The following options are accepted: | |||||
|
|
||||||
| .. versionadded:: 3.14 | ||||||
|
|
||||||
| .. option:: --cors | ||||||
|
Member
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. As Hugo said, since we're anyway exposing |
||||||
|
|
||||||
| Adds an additional CORS (Cross-Origin Resource sharing) header to each response:: | ||||||
|
|
||||||
| Access-Control-Allow-Origin: * | ||||||
|
|
||||||
| .. versionadded:: next | ||||||
|
|
||||||
|
|
||||||
|
picnixz marked this conversation as resolved.
|
||||||
| .. _http.server-security: | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -117,13 +117,24 @@ 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 | ||||||
| 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 = {} | ||||||
| if hasattr(self, 'response_headers'): | ||||||
| kwargs['response_headers'] = self.response_headers | ||||||
| self.RequestHandlerClass(request, client_address, self, **kwargs) | ||||||
|
|
||||||
| class ThreadingHTTPServer(socketserver.ThreadingMixIn, HTTPServer): | ||||||
| daemon_threads = True | ||||||
|
|
@@ -132,7 +143,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, **http_server_kwargs): | ||||||
| try: | ||||||
| import ssl | ||||||
| except ImportError: | ||||||
|
|
@@ -150,7 +161,8 @@ def __init__(self, server_address, RequestHandlerClass, | |||||
|
|
||||||
| super().__init__(server_address, | ||||||
| RequestHandlerClass, | ||||||
| bind_and_activate) | ||||||
| bind_and_activate, | ||||||
| **http_server_kwargs) | ||||||
|
|
||||||
| def server_activate(self): | ||||||
| """Wrap the socket in SSLSocket.""" | ||||||
|
|
@@ -692,10 +704,11 @@ class SimpleHTTPRequestHandler(BaseHTTPRequestHandler): | |||||
| '.xz': 'application/x-xz', | ||||||
| } | ||||||
|
|
||||||
| def __init__(self, *args, directory=None, **kwargs): | ||||||
| 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 | ||||||
|
Member
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. Clarify as an internal private attribute:
Suggested change
Or document SimpleHTTPRequestHandler.response_headers as a public attribute.
Author
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. So far I have intended |
||||||
| super().__init__(*args, **kwargs) | ||||||
|
|
||||||
| def do_GET(self): | ||||||
|
|
@@ -736,6 +749,10 @@ def send_head(self): | |||||
| new_url = urllib.parse.urlunsplit(new_parts) | ||||||
| self.send_header("Location", new_url) | ||||||
| self.send_header("Content-Length", "0") | ||||||
| # User specified response_headers | ||||||
| if self.response_headers is not None: | ||||||
| for header, value in self.response_headers.items(): | ||||||
| self.send_header(header, value) | ||||||
|
Member
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. let's make it a private method, say
Member
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. Or is moving this to an extended send_response override an option? That way you would include the fields for all responses. |
||||||
| self.end_headers() | ||||||
| return None | ||||||
| for index in self.index_pages: | ||||||
|
|
@@ -795,6 +812,9 @@ def send_head(self): | |||||
| self.send_header("Content-Length", str(fs[6])) | ||||||
| self.send_header("Last-Modified", | ||||||
| self.date_time_string(fs.st_mtime)) | ||||||
| if self.response_headers is not None: | ||||||
| for header, value in self.response_headers.items(): | ||||||
| self.send_header(header, value) | ||||||
| self.end_headers() | ||||||
| return f | ||||||
| except: | ||||||
|
|
@@ -970,7 +990,8 @@ def _get_best_family(*address): | |||||
| def test(HandlerClass=BaseHTTPRequestHandler, | ||||||
| ServerClass=ThreadingHTTPServer, | ||||||
| protocol="HTTP/1.0", port=8000, bind=None, | ||||||
| tls_cert=None, tls_key=None, tls_password=None): | ||||||
| tls_cert=None, tls_key=None, tls_password=None, | ||||||
| response_headers=None): | ||||||
| """Test the HTTP request handler class. | ||||||
|
|
||||||
| This runs an HTTP server on port 8000 (or the port argument). | ||||||
|
|
@@ -981,9 +1002,10 @@ def test(HandlerClass=BaseHTTPRequestHandler, | |||||
|
|
||||||
| if tls_cert: | ||||||
| server = ServerClass(addr, HandlerClass, certfile=tls_cert, | ||||||
| keyfile=tls_key, password=tls_password) | ||||||
| keyfile=tls_key, password=tls_password, | ||||||
| response_headers=response_headers) | ||||||
| else: | ||||||
| server = ServerClass(addr, HandlerClass) | ||||||
| server = ServerClass(addr, HandlerClass, response_headers=response_headers) | ||||||
|
|
||||||
| with server as httpd: | ||||||
| host, port = httpd.socket.getsockname()[:2] | ||||||
|
|
@@ -1024,6 +1046,13 @@ def _main(args=None): | |||||
| parser.add_argument('port', default=8000, type=int, nargs='?', | ||||||
| help='bind to this port ' | ||||||
| '(default: %(default)s)') | ||||||
| parser.add_argument('--cors', action='store_true', | ||||||
| help='Enable Access-Control-Allow-Origin: * header') | ||||||
| parser.add_argument('-H', '--header', nargs=2, action='append', | ||||||
| # metavar='HEADER VALUE', | ||||||
|
Member
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. Remove commented code:
Suggested change
Author
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. Fixed in 9450b86 |
||||||
| metavar=('HEADER', 'VALUE'), | ||||||
| help='Add a custom response header ' | ||||||
| '(can be used multiple times)') | ||||||
|
Member
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. The
Author
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 agree in the code the multiple hint is obvious, but I added this for users running
Member
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. Oh right. I think this should be improved in argparse then.
Member
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.
Suggested change
The trace.py argparse uses "specified"
Author
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. Fixed in f0d1bac |
||||||
| args = parser.parse_args(args) | ||||||
|
|
||||||
| if not args.tls_cert and args.tls_key: | ||||||
|
|
@@ -1052,14 +1081,21 @@ def server_bind(self): | |||||
|
|
||||||
| def finish_request(self, request, client_address): | ||||||
| self.RequestHandlerClass(request, client_address, self, | ||||||
| directory=args.directory) | ||||||
| directory=args.directory, | ||||||
| response_headers=self.response_headers) | ||||||
|
Member
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. Why not do this the same way the --directory or --protocol options are implemented? Either way should avoid adding internal parameters to unrelated HTTPServer and BaseRequestHandler classes. You could build the response_headers dictionary before the DualStackServerMixin class, and then pass it by referencing the outer scope like is already done with args.directory:
Suggested change
Or set the response_headers attribute on the SimpleHTTPRequestHandler class rather than in its constructor, like is done for protocol_verison in the test function.
Author
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 followed your advice, see b1026d2. I made |
||||||
|
|
||||||
| class HTTPDualStackServer(DualStackServerMixin, ThreadingHTTPServer): | ||||||
| pass | ||||||
| class HTTPSDualStackServer(DualStackServerMixin, ThreadingHTTPSServer): | ||||||
| pass | ||||||
|
|
||||||
| ServerClass = HTTPSDualStackServer if args.tls_cert else HTTPDualStackServer | ||||||
| response_headers = {} | ||||||
| if args.cors: | ||||||
| response_headers['Access-Control-Allow-Origin'] = '*' | ||||||
| for header, value in args.header or []: | ||||||
| response_headers[header] = value | ||||||
|
|
||||||
|
|
||||||
| test( | ||||||
| HandlerClass=SimpleHTTPRequestHandler, | ||||||
|
|
@@ -1070,6 +1106,7 @@ class HTTPSDualStackServer(DualStackServerMixin, ThreadingHTTPSServer): | |||||
| tls_cert=args.tls_cert, | ||||||
| tls_key=args.tls_key, | ||||||
| tls_password=tls_key_password, | ||||||
| response_headers=response_headers or None | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,21 +81,24 @@ 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: | ||||||||
| certfile, keyfile, password = self.tls | ||||||||
| self.server = create_https_server( | ||||||||
| certfile, keyfile, password, | ||||||||
| request_handler=self.request_handler, | ||||||||
| **self.server_kwargs | ||||||||
| ) | ||||||||
| else: | ||||||||
| self.server = HTTPServer(('localhost', 0), self.request_handler) | ||||||||
| self.server = HTTPServer(('localhost', 0), self.request_handler, | ||||||||
|
Member
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. You must also modify |
||||||||
| **self.server_kwargs) | ||||||||
|
Member
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. This appears to only be testing the undocumented or internal HTTPServer parameter. It would be good to test the new documented SimpleHTTPRequestHandler parameter instead or as well.
Author
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 have removed |
||||||||
| self.test_object.HOST, self.test_object.PORT = self.server.socket.getsockname() | ||||||||
| self.test_object.server_started.set() | ||||||||
| self.test_object = None | ||||||||
|
|
@@ -113,12 +116,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 +829,17 @@ def test_path_without_leading_slash(self): | |||||||
| self.tempdir_name + "/?hi=1") | ||||||||
|
|
||||||||
|
|
||||||||
| class CorsHTTPServerTestCase(SimpleHTTPServerTestCase): | ||||||||
| server_kwargs = { | ||||||||
| 'response_headers': {'Access-Control-Allow-Origin': '*'} | ||||||||
| } | ||||||||
|
|
||||||||
| def test_cors(self): | ||||||||
|
Member
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.
Suggested change
|
||||||||
| response = self.request(self.base_url + '/test') | ||||||||
| self.check_status_and_reason(response, HTTPStatus.OK) | ||||||||
| self.assertEqual(response.getheader('Access-Control-Allow-Origin'), '*') | ||||||||
|
|
||||||||
|
|
||||||||
| class SocketlessRequestHandler(SimpleHTTPRequestHandler): | ||||||||
| def __init__(self, directory=None): | ||||||||
| request = mock.Mock() | ||||||||
|
|
@@ -1306,6 +1322,7 @@ class CommandLineTestCase(unittest.TestCase): | |||||||
| 'tls_cert': None, | ||||||||
| 'tls_key': None, | ||||||||
| 'tls_password': None, | ||||||||
| 'response_headers': None, | ||||||||
| } | ||||||||
|
|
||||||||
| def setUp(self): | ||||||||
|
|
@@ -1371,6 +1388,29 @@ def test_protocol_flag(self, mock_func): | |||||||
| mock_func.assert_called_once_with(**call_args) | ||||||||
| mock_func.reset_mock() | ||||||||
|
|
||||||||
| @mock.patch('http.server.test') | ||||||||
| def test_cors_flag(self, mock_func): | ||||||||
| self.invoke_httpd('--cors') | ||||||||
| call_args = self.args | dict( | ||||||||
| response_headers={ | ||||||||
| 'Access-Control-Allow-Origin': '*' | ||||||||
| } | ||||||||
| ) | ||||||||
| mock_func.assert_called_once_with(**call_args) | ||||||||
| mock_func.reset_mock() | ||||||||
|
|
||||||||
| @mock.patch('http.server.test') | ||||||||
| def test_header_flag(self, mock_func): | ||||||||
| self.invoke_httpd('--header', 'h1', 'v1', '-H', 'h2', 'v2') | ||||||||
| call_args = self.args | dict( | ||||||||
| response_headers={ | ||||||||
| 'h1': 'v1', | ||||||||
| 'h2': 'v2' | ||||||||
| } | ||||||||
| ) | ||||||||
| mock_func.assert_called_once_with(**call_args) | ||||||||
|
Member
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 don't think we need to check this as You can however check bad usages of
Author
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 added some new tests to verify exceptions with incorrect header CLI usage in c2d6bb3. I agree that |
||||||||
| mock_func.reset_mock() | ||||||||
|
|
||||||||
| @unittest.skipIf(ssl is None, "requires ssl") | ||||||||
| @mock.patch('http.server.test') | ||||||||
| def test_tls_cert_and_key_flags(self, mock_func): | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Add a ``--cors`` cli option to :program:`python -m http.server`. Contributed by | ||
|
Member
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. Let's also update What's New/3.15.rst
Author
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 used blurb to make this entry in NEWS.d, not knowing when it's appropriate to edit the main 3.15.rst file. I think once we know if we're doing --cors / --header , or both, I can make the appropriate update to What's New/3.15.rst |
||
| 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.
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