Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions sphinx/ext/intersphinx/_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -399,17 +399,18 @@ def _fetch_inventory_url(
raw_data = r.content
new_inv_location = r.url
except Exception as err:
safe_url = _get_safe_url(inv_location)
err.args = (
'intersphinx inventory %r not fetchable due to %s: %s',
inv_location,
safe_url,
err.__class__,
str(err),
str(err).replace(inv_location, safe_url),

Copy link
Copy Markdown
Member

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, so inv_location contains the password s%7Eecret, but Requests constructs the error using the equivalent normalized form s~ecret. The replacement therefore does not match, leaving the password in the error output.

For example, an HTTP 500 currently produces output equivalent to:

intersphinx inventory 'http://user@example.com/objects.inv' not fetchable:
500 Server Error for url: http://user:s~ecret@example.com/objects.inv

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:

intersphinx inventory 'http://user@example.com/objects.inv' not fetchable:
500 Server Error for url: http://user@example.com/objects.inv

Could we redact the URL from err.request.url or err.response.url when available, while retaining the original URL as a fallback?

)
raise

if inv_location != new_inv_location:
msg = __('intersphinx inventory has moved: %s -> %s')
LOGGER.info(msg, inv_location, new_inv_location)
LOGGER.info(msg, _get_safe_url(inv_location), _get_safe_url(new_inv_location))

if target_uri in {
inv_location,
Expand Down
50 changes: 50 additions & 0 deletions tests/test_ext_intersphinx/test_ext_intersphinx.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two issues with this line:

  1. The production redirect log message contains two independently redacted values: the original inventory URL (inv_location) and the final URL after following the redirect (new_inv_location). The current test gives only the original URL a password:

    original:    http://user:secret@localhost/objects.inv
    destination: http://localhost/new/objects.inv
    

    That verifies redaction of inv_location, but it cannot verify redaction of new_inv_location. For example, if someone accidentally changed the implementation to the following, the current test would still pass:

    LOGGER.info(msg, _get_safe_url(inv_location), new_inv_location)

    The second URL is now logged without redaction, but it has no password to leak, so the assertion looking for secret detects nothing.

    Giving the destination its own credentials closes that coverage gap:

    destination: http://redirect-user:redirect-secret@localhost/new/objects.inv
    

    The correct implementation logs redirect-user@localhost; an implementation that omits _get_safe_url(new_inv_location) logs redirect-secret and fails the existing assertion. This makes the test protect both halves of the redirect message from future regressions.

    - new_url = f'http://localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}'
    + new_url = f'http://redirect-user:redirect-secret@localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}'
  2. The ty check fails here because BaseHTTPRequestHandler.server is typed as BaseServer, which does not expose a server_port attribute. An explicit type narrowing should preserve the current behavior while allowing ty to recognize the concrete server type:

    + assert isinstance(self.server, http.server.HTTPServer)
      new_url = f'http://localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}'

This suggestion addresses both:

Suggested change
new_url = f'http://localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}'
assert isinstance(self.server, http.server.HTTPServer)
new_url = f'http://redirect-user:redirect-secret@localhost:{self.server.server_port}/new/{INVENTORY_FILENAME}'

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.


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
Expand Down
Loading