From b96b46495aaade71f7dea4d21f5885cf2de28fab Mon Sep 17 00:00:00 2001 From: Adam Smith <2105363+adamjs83@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:08:20 -0400 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01739Seac5eMAfNwFeiwV5r4 --- .../openai_compatible/config_flow.py | 9 ++- .../openai_compatible/strings.json | 9 ++- .../openai_compatible/url_util.py | 36 ++++++++- tests/test_config_flow.py | 79 +++++++++++++++++++ tests/test_url_util.py | 21 +++++ 5 files changed, 147 insertions(+), 7 deletions(-) diff --git a/custom_components/openai_compatible/config_flow.py b/custom_components/openai_compatible/config_flow.py index 0940a27..06575ab 100644 --- a/custom_components/openai_compatible/config_flow.py +++ b/custom_components/openai_compatible/config_flow.py @@ -104,7 +104,7 @@ from .const import ( UNSUPPORTED_PRIORITY_SERVICE_TIERS_MODELS, UNSUPPORTED_WEB_SEARCH_MODELS, ) -from .url_util import InvalidBaseURL, normalize_base_url +from .url_util import CredentialsInBaseURL, InvalidBaseURL, normalize_base_url _LOGGER = logging.getLogger(__name__) @@ -148,6 +148,8 @@ class OpenAIConfigFlow(ConfigFlow, domain=DOMAIN): self._async_abort_entries_match(user_input) try: await validate_input(self.hass, user_input) + except CredentialsInBaseURL: + errors["base"] = "credentials_in_url" except InvalidBaseURL: errors["base"] = "invalid_base_url" except openai.APIConnectionError: @@ -216,7 +218,10 @@ class OpenAIConfigFlow(ConfigFlow, domain=DOMAIN): """Dialog that informs the user that reauth is required.""" if not user_input: return self.async_show_form( - step_id="reauth_confirm", data_schema=STEP_USER_DATA_SCHEMA + step_id="reauth_confirm", + data_schema=self.add_suggested_values_to_schema( + STEP_USER_DATA_SCHEMA, self._get_reauth_entry().data + ), ) return await self.async_step_user(user_input) diff --git a/custom_components/openai_compatible/strings.json b/custom_components/openai_compatible/strings.json index f4bac57..cf245e6 100644 --- a/custom_components/openai_compatible/strings.json +++ b/custom_components/openai_compatible/strings.json @@ -6,6 +6,7 @@ }, "error": { "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", + "credentials_in_url": "The base URL must not contain a username or password. Put authentication in the API key field instead.", "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", "invalid_base_url": "That does not look like a valid http(s) URL.", "unknown": "[%key:common::config_flow::error::unknown%]" @@ -13,12 +14,14 @@ "step": { "reauth_confirm": { "data": { - "api_key": "[%key:common::config_flow::data::api_key%]" + "api_key": "[%key:common::config_flow::data::api_key%]", + "base_url": "[%key:component::openai_compatible::config::step::user::data::base_url%]" }, "data_description": { - "api_key": "[%key:component::openai_compatible::config::step::user::data_description::api_key%]" + "api_key": "[%key:component::openai_compatible::config::step::user::data_description::api_key%]", + "base_url": "[%key:component::openai_compatible::config::step::user::data_description::base_url%]" }, - "description": "Reauthentication required. Please enter your updated API key." + "description": "Reauthentication required. Please enter your updated API key and confirm the API base URL." }, "user": { "data": { diff --git a/custom_components/openai_compatible/url_util.py b/custom_components/openai_compatible/url_util.py index 7ddd7c9..3b9b799 100644 --- a/custom_components/openai_compatible/url_util.py +++ b/custom_components/openai_compatible/url_util.py @@ -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 diff --git a/tests/test_config_flow.py b/tests/test_config_flow.py index 31965ca..9585815 100644 --- a/tests/test_config_flow.py +++ b/tests/test_config_flow.py @@ -647,6 +647,24 @@ async def test_form_invalid_auth(hass: HomeAssistant, side_effect, error) -> Non assert result2["errors"] == {"base": error} +async def test_form_credentials_in_base_url(hass: HomeAssistant) -> None: + """A base_url with embedded credentials must be rejected, not accepted.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + { + CONF_BASE_URL: "https://user:secret@192.0.2.10:4000/v1", + "api_key": "bla", + }, + ) + + assert result2["type"] is FlowResultType.FORM + assert result2["errors"] == {"base": "credentials_in_url"} + + @pytest.mark.parametrize( ("current_options", "new_options", "expected_options"), [ @@ -1597,6 +1615,67 @@ async def test_reauth(hass: HomeAssistant) -> None: assert mock_config_entry.data[CONF_API_KEY] == "new_api_key" +async def test_reauth_preserves_base_url(hass: HomeAssistant) -> None: + """Reauth must not silently reset a configured base_url to the default. + + Regression test: async_step_reauth_confirm used to show + STEP_USER_DATA_SCHEMA without prefilling suggested values (unlike + async_step_user, which does), so the reauth form displayed + DEFAULT_BASE_URL instead of the entry's actual base_url -- and a user + re-authenticating after a key rotation would have their integration + silently repointed from their configured endpoint (e.g. LiteLLM) back to + api.openai.com. + """ + hass.config.components.add("openai_compatible") + configured_base_url = "http://192.0.2.10:4000/v1" + mock_config_entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_API_KEY: "old-key", CONF_BASE_URL: configured_base_url}, + state=config_entries.ConfigEntryState.LOADED, + version=2, + minor_version=7, + ) + mock_config_entry.add_to_hass(hass) + + result = await mock_config_entry.start_reauth_flow(hass) + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reauth_confirm" + + # The shown form must suggest the entry's own base_url, not the schema + # default -- this is the actual bug. Without the fix this assertion + # fails because no suggested_value is set at all. + schema_keys = result["data_schema"].schema + base_url_marker = next(k for k in schema_keys if k == CONF_BASE_URL) + assert base_url_marker.description == {"suggested_value": configured_base_url} + + with ( + patch( + "custom_components.openai_compatible.config_flow.openai.resources.models.AsyncModels.list", + new_callable=AsyncMock, + ), + patch( + "custom_components.openai_compatible.async_setup_entry", + return_value=True, + ), + ): + # A real browser resubmits whatever is displayed in the form, so an + # unmodified base_url field is submitted back as-is alongside the + # new key -- this is what "the user didn't touch the field" means. + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + { + CONF_API_KEY: "new_api_key", + CONF_BASE_URL: configured_base_url, + }, + ) + await hass.async_block_till_done() + + assert result["type"] is FlowResultType.ABORT + assert result["reason"] == "reauth_successful" + assert mock_config_entry.data[CONF_API_KEY] == "new_api_key" + assert mock_config_entry.data[CONF_BASE_URL] == configured_base_url + + @pytest.mark.parametrize( ("current_llm_apis", "suggested_llm_apis", "expected_options"), [ diff --git a/tests/test_url_util.py b/tests/test_url_util.py index caba024..5cc0cb0 100644 --- a/tests/test_url_util.py +++ b/tests/test_url_util.py @@ -3,6 +3,7 @@ import pytest from custom_components.openai_compatible.url_util import ( + CredentialsInBaseURL, InvalidBaseURL, normalize_base_url, sanitize_url_for_logging, @@ -28,6 +29,26 @@ def test_normalize_rejects(raw): normalize_base_url(raw) +@pytest.mark.parametrize( + "raw", + [ + "https://user:secret@host:4000/v1", + "https://user@host:4000/v1", + "https://:secret@host:4000/v1", + ], +) +def test_normalize_rejects_embedded_credentials(raw): + """Authentication belongs in the API key field, never in base_url.""" + with pytest.raises(CredentialsInBaseURL): + normalize_base_url(raw) + + +def test_credentials_in_base_url_is_an_invalid_base_url(): + """CredentialsInBaseURL must still be catchable as InvalidBaseURL.""" + with pytest.raises(InvalidBaseURL): + normalize_base_url("https://user:secret@host:4000/v1") + + def test_sanitize_strips_credentials(): assert ( sanitize_url_for_logging("https://user:secret@host:4000/v1")