Fix intersphinx leaking basic auth credentials in error and info messages#14366
Fix intersphinx leaking basic auth credentials in error and info messages#14366joshuaswanson wants to merge 9 commits intosphinx-doc:masterfrom
Conversation
|
Thanks for working on a fix for this @joshuaswanson - please could you add some test coverage to accompany it? I would recommend copying and adapting the |
|
|
||
| get_request.side_effect = ConnectionError('connection refused') | ||
| with pytest.raises(ConnectionError, match='connection refused'): | ||
| _fetch_inventory_url( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| cache_path=None, | ||
| ) | ||
| status_output = app.status.getvalue() | ||
| assert 'secret' not in status_output |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ial leak in error string
| @mock.patch('sphinx.ext.intersphinx._load.InventoryFile') | ||
| @mock.patch('sphinx.ext.intersphinx._load.requests.get') | ||
| @pytest.mark.sphinx('html', testroot='root') | ||
| def test_fetch_inventory_redirect_hides_credentials(get_request, InventoryFile, app): | ||
| """Credentials should not appear in redirect log messages.""" | ||
| mocked_get = get_request.return_value.__enter__.return_value | ||
| intersphinx_setup(app) | ||
| mocked_get.content = b'# Sphinx inventory version 2' |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@picnixz are you able to review this pull request too? |
| _, stderr = capsys.readouterr() | ||
| assert 'secret' not in stderr |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| status_output = app.status.getvalue() | ||
| assert 'secret' not in status_output | ||
| assert 'user@localhost' in status_output |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| stdout, stderr = capsys.readouterr() | ||
| full_output = stdout + stderr + '\n'.join(caplog.messages) | ||
| assert 'secret' not in full_output | ||
| assert 'user@localhost' in full_output |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| with http_server(RedirectHandler) as server: | ||
| port = server.server_port | ||
| url = f'http://user:secret@localhost:{port}/{INVENTORY_FILENAME}' | ||
| inspect_main([url]) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| assert actual == expected | ||
|
|
||
|
|
||
| def test_fetch_inventory_url_error_hides_credentials(capsys): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jayaddison
left a comment
jayaddison
left a comment
There was a problem hiding this comment.
Looks great - thanks for this and for your responsiveness to codereview, @joshuaswanson
2 participants
Fixes #14342.
When intersphinx fails to fetch an inventory or detects a redirect, the error and info messages include the raw URL with basic auth credentials. The
_get_safe_urlhelper already exists in this file (and is used for the initial "loading inventory" info message) but wasn't applied to the error path in_fetch_inventory_urlor the "inventory has moved" log message.