Fix intersphinx leaking basic auth credentials in error and info messages

[joshuaswanson]

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_url helper 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_url or the "inventory has moved" log message.

[jayaddison]

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 test_inspect_main_url test case from tests/test_ext_intersphinx/test_ext_intersphinx.py, into test cases for the HTTP-error case and HTTP-redirection case respectively.

[jayaddison]

@picnixz are you able to review this pull request too?

[jayaddison]

Looks great - thanks for this and for your responsiveness to codereview, @joshuaswanson

[jayaddison]

@joshuaswanson sorry, one more thing that I forgot: you should also update the CHANGES.rst (changelog) file to describe the fix (and to add yourself a credit for it!).

[jayaddison]

👍 thanks again

[jdillard]

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}'

[jdillard]

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?

[jdillard]

Thanks!