-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix intersphinx leaking basic auth credentials in error and info messages #14366
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: master
Are you sure you want to change the base?
Changes from 9 commits
f6c4851
7121494
f51a0b0
e467c92
3f96487
d154e84
c38ee69
5ab2973
1ed6154
b8bc837
c14aa00
d95fd56
a9f1ce3
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -666,6 +666,56 @@ def test_getsafeurl_unauthed() -> None: | |||||||
| assert actual == expected | ||||||||
|
|
||||||||
|
|
||||||||
| def test_fetch_inventory_url_error_hides_credentials(capsys, caplog): | ||||||||
| """Credentials should not appear in error messages on fetch failure.""" | ||||||||
|
|
||||||||
| class ErrorHandler(http.server.BaseHTTPRequestHandler): | ||||||||
| def do_GET(self): | ||||||||
| self.send_error(500, 'Internal Server Error') | ||||||||
|
|
||||||||
| def log_message(*args, **kwargs): | ||||||||
| pass | ||||||||
|
|
||||||||
| with http_server(ErrorHandler) as server: | ||||||||
| url = f'http://user:secret@localhost:{server.server_port}/{INVENTORY_FILENAME}' | ||||||||
| inspect_main([url]) | ||||||||
|
|
||||||||
| stdout, stderr = capsys.readouterr() | ||||||||
| assert 'secret' not in stdout | ||||||||
| assert 'secret' not in stderr | ||||||||
| assert not any('secret' in message for message in caplog.messages) | ||||||||
| assert 'user@localhost' in stderr | ||||||||
|
|
||||||||
|
|
||||||||
| def test_fetch_inventory_redirect_hides_credentials(capsys, caplog): | ||||||||
| """Credentials should not appear in redirect log messages.""" | ||||||||
|
|
||||||||
| class RedirectHandler(http.server.BaseHTTPRequestHandler): | ||||||||
| def do_GET(self): | ||||||||
| if '/new/' not in self.path: | ||||||||
| self.send_response(302) | ||||||||
| new_url = f'http://localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}' | ||||||||
|
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. There are two issues with this line:
This suggestion addresses both:
Suggested change
|
||||||||
| self.send_header('Location', new_url) | ||||||||
| self.end_headers() | ||||||||
| else: | ||||||||
| self.send_response(200, 'OK') | ||||||||
| self.end_headers() | ||||||||
| self.wfile.write(INVENTORY_V2) | ||||||||
|
|
||||||||
| def log_message(*args, **kwargs): | ||||||||
| pass | ||||||||
|
|
||||||||
| with http_server(RedirectHandler) as server: | ||||||||
| url = f'http://user:secret@localhost:{server.server_port}/{INVENTORY_FILENAME}' | ||||||||
| inspect_main([url]) | ||||||||
|
Comment on lines
+725
to
+727
This comment was marked as resolved.
Sorry, something went wrong. |
||||||||
|
|
||||||||
| stdout, stderr = capsys.readouterr() | ||||||||
| assert 'secret' not in stdout | ||||||||
| assert 'secret' not in stderr | ||||||||
| assert not any('secret' in message for message in caplog.messages) | ||||||||
| assert any('user@localhost' in message for message in caplog.messages) | ||||||||
|
|
||||||||
|
|
||||||||
| def test_inspect_main_noargs(capsys): | ||||||||
| """inspect_main interface, without arguments""" | ||||||||
| assert inspect_main([]) == 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.
Exact-string replacement does not reliably redact the URL after Requests has normalized it. For example, a valid URL may encode
~as%7E, soinv_locationcontains the passwords%7Eecret, but Requests constructs the error using the equivalent normalized forms~ecret. The replacement therefore does not match, leaving the password in the error output.For example, an HTTP 500 currently produces output equivalent to:
The first occurrence is redacted, but the normalized URL in the underlying Requests error still contains the password. After redacting the URL retained by the exception, both occurrences would be safe:
Could we redact the URL from
err.request.urlorerr.response.urlwhen available, while retaining the original URL as a fallback?