fix: reauth must preserve base_url; reject credentials embedded in base_url
Critical (review finding 1): async_step_reauth_confirm showed STEP_USER_DATA_SCHEMA directly instead of prefilling it via add_suggested_values_to_schema (unlike async_step_user, which does), so the reauth form displayed DEFAULT_BASE_URL rather than the entry's configured endpoint. A user rotating their API key would have their integration silently repointed from e.g. LiteLLM back to api.openai.com on save. Fixed by prefilling from self._get_reauth_entry().data, same as the user step. strings.json's reauth_confirm block now documents the base_url field too. Regression test: test_reauth_preserves_base_url (asserts both the shown form's suggested_value and the final stored base_url). Important (review finding 2): base_url is user-suppliable and could carry embedded user:pass@ credentials that later surface through vendored exception logging (e.g. entity.py's "Error talking to OpenAI: %s") this fork does not and should not edit. Rather than touch vendored logging, reject credentials at the boundary: normalize_base_url now raises the new CredentialsInBaseURL (a subclass of InvalidBaseURL) when the URL has a username or password, mapped to a distinct "credentials_in_url" form error telling the user to use the API key field instead. Tests added in test_url_util.py and test_config_flow.py. Important (review finding 3): documented on sanitize_url_for_logging that it is not a general-purpose credential scrubber -- it only masks what urlparse recognizes as a netloc's user:pass@ authority component, so a scheme-less/relative string with embedded credentials passes through unmasked. Safe today because every caller normalizes first (which now also rejects embedded credentials outright), but it is exported for a later task's use and must not be assumed bulletproof on an arbitrary unvalidated string. Not fixed (reviewer's Minor 4, by direction): the default-config setup call reaching api.openai.com with Authorization: Bearer not-required when both base_url and api_key are left at defaults -- cosmetic, and correctly surfaces as invalid_auth. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01739Seac5eMAfNwFeiwV5r4
This commit is contained in:
1 parent
15889c905d
commit
b96b46495a
5 files changed
+147
-7
No files matched your search
@@ -15,8 +15,24 @@ class InvalidBaseURL(ValueError):
|
||||
"""Raised when a base URL is unusable."""
|
||||
|
||||
|
||||
class CredentialsInBaseURL(InvalidBaseURL):
|
||||
"""Raised when a base URL embeds a username or password.
|
||||
|
||||
A distinct subclass (rather than reusing the generic InvalidBaseURL
|
||||
message) so the config flow can show a specific, actionable error
|
||||
instead of the generic "not a valid URL" one.
|
||||
"""
|
||||
|
||||
|
||||
def normalize_base_url(url: str) -> str:
|
||||
"""Strip whitespace and trailing slashes, and validate the scheme."""
|
||||
"""Strip whitespace and trailing slashes, and validate the scheme.
|
||||
|
||||
Rejects a URL carrying a username or password component: authentication
|
||||
belongs in the API key field, which is handled (and never logged) safely
|
||||
on its own — a base_url is user-suppliable and read back into log/error
|
||||
text by vendored code this fork does not touch, so credentials must never
|
||||
be accepted here in the first place.
|
||||
"""
|
||||
cleaned = (url or "").strip()
|
||||
if not cleaned:
|
||||
raise InvalidBaseURL("base URL is empty")
|
||||
@@ -26,12 +42,28 @@ def normalize_base_url(url: str) -> str:
|
||||
raise InvalidBaseURL(f"unsupported scheme: {parsed.scheme or '(none)'}")
|
||||
if not parsed.netloc:
|
||||
raise InvalidBaseURL("base URL has no host")
|
||||
if parsed.username or parsed.password:
|
||||
raise CredentialsInBaseURL(
|
||||
"base URL must not contain a username or password; "
|
||||
"use the API key field for authentication"
|
||||
)
|
||||
|
||||
return urlunparse(parsed._replace(path=parsed.path.rstrip("/")))
|
||||
|
||||
|
||||
def sanitize_url_for_logging(url: str) -> str:
|
||||
"""Return the URL with any embedded credentials replaced by ***."""
|
||||
"""Return the URL with any embedded credentials replaced by ***.
|
||||
|
||||
This only recognizes credentials urlparse places in an authority
|
||||
(``user:pass@host``) component of an absolute http(s)/-scheme URL. It is
|
||||
not a general-purpose credential scrubber: a scheme-less or relative
|
||||
string with an embedded ``user:pass@`` substring passes through
|
||||
unmasked, since urlparse won't parse it as a netloc. Safe today because
|
||||
every caller normalizes with normalize_base_url() first (which both
|
||||
requires an absolute http(s) URL and rejects embedded credentials
|
||||
outright) before this is ever called on it; do not rely on this function
|
||||
alone to sanitize an arbitrary, unvalidated string.
|
||||
"""
|
||||
parsed = urlparse(url)
|
||||
if not parsed.username and not parsed.password:
|
||||
return url
|
||||
|
||||
Reference in new issue
Block a user