diff --git a/backend/settings.py b/backend/settings.py index 5ab0cd9..687eeed 100644 --- a/backend/settings.py +++ b/backend/settings.py @@ -66,6 +66,15 @@ config.TASK_WEB_SUMMARIZE, ) +# The three providers the page can actually display. Used to bound the +# per-provider catalog-state dict: without it, that dict is keyed by a +# raw client-supplied string and grows without limit. +_KNOWN_API_PROVIDERS = ( + config.API_PROVIDER_OPENAI, + config.API_PROVIDER_ANTHROPIC, + config.API_PROVIDER_GEMINI, +) + # Every persistence-touching mutation runs in a worker thread # (asyncio.to_thread) so SettingsManager._save_state's json-dump + fsync + # atomic-replace never stalls the event loop (and with it, every session's @@ -82,7 +91,7 @@ def _apply(mutation: Callable[..., None], *args: Any) -> None: mutation(*args) -def _redact(text: str, secret: str) -> str: +def _redact(text: str, secret: Any) -> str: # Post-review fix: some HTTP client libraries embed request parameters # (including a rejected API key) directly in exception text - the # legacy bridge this page replaces redacted for exactly this reason @@ -90,7 +99,15 @@ def _redact(text: str, secret: str) -> str: # Applied before any exception text reaches a WS-broadcast message, so # the write-only-key guarantee holds on the failure path too, not just # the happy path. - if not secret: + # + # Second-pass fix: the isinstance check is load-bearing, not defensive + # noise. Intent arguments arrive as raw JSON off the wire with no + # coercion anywhere in the dispatch path, and both call sites are + # inside `except` blocks - where a TypeError from str.replace would + # NOT be caught by its own `try` and would escape the handler, leaving + # the catalog pinned at "loading" (which disables the Load button) for + # the life of the process. + if not isinstance(secret, str) or not secret: return text return text.replace(secret, "***") @@ -232,12 +249,60 @@ async def set_viewing_api_provider(provider: str): await bus.publish("app-settings") async def load_api_models(provider: str, api_key: str, base_url: str = ""): + # Second-pass fix: coerce every wire argument BEFORE it is used for + # anything - `provider` in particular is used as a dict key below, + # and an unhashable JSON value (array/object) would raise TypeError + # straight out of the handler. Intent args reach here as raw JSON + # with no validation anywhere in the dispatch path. + provider = str(provider) + base_url = str(base_url).strip() + # Gemini has no live catalog endpoint wired (legacy never showed the # Load button for it either) - a stale/misbehaving client asking # anyway gets a clean error rather than a confusing initialize_api # call with the wrong semantics. if provider not in (config.API_PROVIDER_OPENAI, config.API_PROVIDER_ANTHROPIC): - api_catalog_state[provider] = {"status": "error", "message": f"{provider} does not support catalog refresh."} + # Only a provider the UI can actually VIEW gets a stored status + # slot: build_payload surfaces the slot for the viewed provider + # only, so a slot for an unknown string could never be read - + # it would just grow this dict without bound on a key the client + # controls. + if provider in _KNOWN_API_PROVIDERS: + api_catalog_state[provider] = { + "status": "error", + "message": f"{provider} does not support catalog refresh.", + } + await bus.publish("app-settings") + return + + if provider == config.API_PROVIDER_OPENAI and not base_url: + # Restores a guard both legacy predecessors had. Without it an + # empty Base URL silently falls through to api_provider's own + # "https://api.openai.com/v1" default, sending a key meant for a + # self-hosted proxy to OpenAI's public API instead. + api_catalog_state[provider] = { + "status": "error", + "message": "Please enter the Base URL for the OpenAI-compatible provider.", + } + await bus.publish("app-settings") + return + + # This page deliberately never sends the saved key to the client + # (see the module docstring), so a user who already has a key saved + # opens the dialog with a blank field. Legacy pre-filled the field + # and therefore always had a key to test with; the write-only port + # has to make up for that here instead, or Load is simply unusable + # for every already-configured user. Falling back server-side keeps + # the plaintext key off the wire entirely. + key = str(api_key).strip() + if not key: + key = ( + manager.get_openai_key() + if provider == config.API_PROVIDER_OPENAI + else manager.get_anthropic_key() + ) + if not key: + api_catalog_state[provider] = {"status": "error", "message": "Please enter the API Key."} await bus.publish("app-settings") return @@ -254,8 +319,8 @@ async def load_api_models(provider: str, api_key: str, base_url: str = ""): await asyncio.to_thread( api_provider.initialize_api, provider, - str(api_key), - str(base_url) if provider == config.API_PROVIDER_OPENAI else None, + key, + base_url if provider == config.API_PROVIDER_OPENAI else None, ) descriptors = await asyncio.to_thread(api_provider.get_available_model_descriptors) # Post-review fix: api_provider's provider globals are process- @@ -291,7 +356,11 @@ async def load_api_models(provider: str, api_key: str, base_url: str = ""): for descriptor in descriptors ] except Exception as exc: # noqa: BLE001 - redacted, then surfaced, matching legacy's error label - message = _redact(str(exc), api_key) + # Redact the EFFECTIVE key (which may be the stored one picked + # up by the fallback above), not the submitted argument - the + # effective key is what actually reached the provider SDK and + # so is what can come back embedded in its exception text. + message = _redact(str(exc), key) api_catalog_state[provider] = { "status": "error", "message": f"Catalog refresh failed: {message}\nSaved/custom model IDs remain usable if the endpoint supports them.", @@ -319,6 +388,18 @@ async def save_api_configuration(provider: str, base_url: str, api_key: str, mod if not isinstance(models_by_task, dict): models_by_task = {} + if provider == config.API_PROVIDER_OPENAI and not base_url: + # Same guard as load_api_models, and the more important of the + # two: an empty base_url persisted here is read straight back + # by SettingsManager.get_api_base_url (whose own default only + # applies to a MISSING key, not a stored ""), so every later + # provider bootstrap silently re-points the saved key at + # api.openai.com. + if notifications is not None: + notifications.show("Please enter the Base URL for the OpenAI-compatible provider.", "warning") + await bus.publish("notification") + return + if not api_key: if notifications is not None: notifications.show("Please enter your API Key.", "warning") @@ -384,7 +465,28 @@ def _persist() -> None: await bus.publish("app-settings") async def reset_api_settings(): - await asyncio.to_thread(_apply, manager.reset_api_settings) + def _reset() -> None: + manager.reset_api_settings() + # SettingsManager.reset_api_settings intentionally leaves + # api_model_catalog_by_provider alone (it is shared with the + # legacy Qt bridge), but "All API settings have been cleared" + # has to mean it - otherwise the page keeps offering the old + # provider's model catalog after the reset. Clearing through + # the existing public setter rather than editing the shared + # legacy module. + for known in _KNOWN_API_PROVIDERS: + manager.set_api_model_catalog([], known) + + await asyncio.to_thread(_apply, _reset) + # Transient per-provider status/message is UI state, not manager + # state, so it has to be cleared here too - otherwise the previous + # "Catalog refreshed - N model(s)" success banner survives a reset + # that just deleted that very catalog. + api_catalog_state.clear() + # Legacy's reset_settings snapped the provider dropdown back to + # OpenAI-Compatible; reset_api_settings restores exactly that as + # the persisted provider, so the viewed provider follows it. + viewing_api_provider["value"] = manager.get_api_provider() if notifications is not None: notifications.show("All API settings have been cleared.", "success") await bus.publish("notification") diff --git a/backend/tests/test_settings.py b/backend/tests/test_settings.py index 09c4066..2fa3831 100644 --- a/backend/tests/test_settings.py +++ b/backend/tests/test_settings.py @@ -7,6 +7,7 @@ import api_provider import graphlink_task_config as config +import backend.settings as settings_module from backend.events import SessionBus from backend.notifications import NotificationState from backend.settings import register_settings, settings_payload @@ -454,6 +455,149 @@ def _boom(*a, **k): assert "***" in payload["apiCatalogMessage"] +def test_load_api_models_falls_back_to_the_saved_key_when_the_field_is_blank(manager, monkeypatch): + # This page never sends the saved key to the client, so an already- + # configured user opens the dialog with a blank key field. Without a + # server-side fallback, Load is simply unusable for them - it fails + # with "API key not configured" while the same payload reports + # apiKeyConfigured=true. Legacy avoided this by pre-filling the field. + seen = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda provider, key, base_url=None: seen.append(key)) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_OPENAI) + monkeypatch.setattr(api_provider, "get_available_model_descriptors", lambda: []) + manager.set_api_settings(config.API_PROVIDER_OPENAI, "https://x/v1", "sk-saved-openai", "", "") + bus = SessionBus("settings-load-key-fallback-test") + register_settings(bus, manager) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "", "https://x/v1"])) + + assert seen == ["sk-saved-openai"] + + +def test_load_api_models_reports_a_clean_error_when_no_key_is_available_anywhere(manager, monkeypatch): + called = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: called.append(a)) + bus = SessionBus("settings-load-no-key-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "", "https://x/v1"])) + + assert called == [] # never contacts a provider with an empty key + payload = recorder.messages[-1]["payload"] + assert payload["apiCatalogStatus"] == "error" + assert payload["apiCatalogMessage"] == "Please enter the API Key." + + +def test_load_api_models_requires_a_base_url_for_the_openai_compatible_provider(manager, monkeypatch): + # Without this an empty Base URL falls through to api_provider's own + # api.openai.com default, sending a self-hosted-proxy key to OpenAI. + called = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: called.append(a)) + bus = SessionBus("settings-load-no-base-url-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "sk-test", " "])) + + assert called == [] + assert "Base URL" in recorder.messages[-1]["payload"]["apiCatalogMessage"] + + +def test_save_api_configuration_requires_a_base_url_for_the_openai_compatible_provider(manager): + # Worse on the save path than the load path: an empty base_url is + # PERSISTED, and SettingsManager.get_api_base_url's default only covers + # a missing key, not a stored "", so every later bootstrap silently + # re-points the saved key at api.openai.com. + notifications = NotificationState() + bus = SessionBus("settings-save-no-base-url-test") + bus.register_topic("notification", notifications.payload) + register_settings(bus, manager, notifications) + + asyncio.run( + bus.dispatch_intent( + "app-settings", "saveApiConfiguration", [config.API_PROVIDER_OPENAI, " ", "sk-test", _six_task_models()] + ) + ) + + assert manager.get_openai_key() == "" + assert notifications.msg_type == "warning" + assert "Base URL" in notifications.message + + +def test_load_api_models_survives_non_string_wire_arguments(manager, monkeypatch): + # Intent args arrive as raw JSON with no validation anywhere in the + # dispatch path. Two of these used to raise straight out of the + # handler: a non-str key hit str.replace inside the except block (where + # its own try cannot catch it), and an unhashable provider was used + # directly as a dict key. + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("nope"))) + bus = SessionBus("settings-load-wire-types-test") + register_settings(bus, manager) + + for bad_args in ( + [config.API_PROVIDER_OPENAI, 12345, "https://x/v1"], + [config.API_PROVIDER_OPENAI, ["a"], "https://x/v1"], + [config.API_PROVIDER_OPENAI, {"k": "v"}, "https://x/v1"], + [["unhashable"], "sk-test", "https://x/v1"], + [{"also": "unhashable"}, "sk-test", "https://x/v1"], + [config.API_PROVIDER_OPENAI, "sk-test", 99], + ): + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", bad_args)) # must not raise + + +def test_load_api_models_does_not_grow_catalog_state_for_unknown_providers(manager): + # api_catalog_state is keyed by a client-supplied string; only the + # three providers the UI can actually display may occupy a slot, or a + # misbehaving client grows this dict without bound for the life of the + # session. + bus = SessionBus("settings-load-unbounded-growth-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + for i in range(50): + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [f"junk-provider-{i}", "sk", "u"])) + + # Nothing was published at all for an unknown provider, and the viewed + # provider's own state is untouched. + asyncio.run(bus.publish("app-settings")) + assert recorder.messages[-1]["payload"]["apiCatalogStatus"] == "idle" + + +def test_reset_api_settings_also_clears_the_catalog_and_snaps_the_viewed_provider_back(manager, monkeypatch): + from graphlink_model_catalog import ModelDescriptor + + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_ANTHROPIC) + monkeypatch.setattr( + api_provider, + "get_available_model_descriptors", + lambda: [ModelDescriptor(model_id="claude-sonnet", provider=config.API_PROVIDER_ANTHROPIC)], + ) + bus = SessionBus("settings-reset-clears-catalog-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "setViewingApiProvider", [config.API_PROVIDER_ANTHROPIC])) + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_ANTHROPIC, "sk-ant", ""])) + assert recorder.messages[-1]["payload"]["apiCatalogStatus"] == "success" + + asyncio.run(bus.dispatch_intent("app-settings", "resetApiSettings", [])) + + payload = recorder.messages[-1]["payload"] + # "All API settings have been cleared" has to actually mean it: no + # stale success banner, no stale catalog, and the viewed provider + # follows the reset back to the default (as legacy's reset did). + assert payload["apiCatalogStatus"] == "idle" + assert payload["apiModelCatalog"] == [] + assert payload["viewingApiProvider"] == config.API_PROVIDER_OPENAI + assert manager.get_api_model_catalog(config.API_PROVIDER_ANTHROPIC) == [] + + def test_save_api_configuration_rejects_a_non_dict_models_argument_without_crashing(manager): # Regression test: a non-dict 4th argument (e.g. a WS client sending # null) used to raise an uncaught AttributeError from models_by_task.get @@ -502,20 +646,36 @@ def test_save_api_configuration_preserves_other_providers_keys(manager, monkeypa def test_save_api_configuration_reads_other_providers_keys_atomically_with_its_own_write(manager, monkeypatch): # Regression test for a post-review R7.4a race: the original code read # manager.get_anthropic_key()/get_openai_key()/get_gemini_key() for the - # "keep as-is" providers on the event loop, BEFORE awaiting - # initialize_api and persisting - so a second, concurrent - # saveApiConfiguration for a different provider (e.g. two browser tabs - # on the same session) landing in between got its own freshly-committed - # key silently reverted by this call's stale pre-read. The fix moved - # that read inside _persist(), which only ever runs under _manager_lock - # - this test simulates the interleaving by having initialize_api's own - # mock perform the "concurrent" write, proving the later read (inside - # _persist) picks up the fresh value rather than a stale one captured - # earlier. - def _simulate_concurrent_anthropic_save_during_our_own_initialize_api(*_args, **_kwargs): - manager.set_api_settings(config.API_PROVIDER_ANTHROPIC, "", "", "sk-concurrent-ant", "") - - monkeypatch.setattr(api_provider, "initialize_api", _simulate_concurrent_anthropic_save_during_our_own_initialize_api) + # "keep as-is" providers on the event loop, OUTSIDE _manager_lock, then + # wrote them back much later inside the locked persist - so a second, + # concurrent saveApiConfiguration for a different provider (two browser + # tabs share one session) committing in that window got its own + # freshly-saved key silently reverted by this call's stale pre-read. + # + # The injection point matters and is the whole point of this test. An + # earlier version of it injected the "concurrent" write inside the + # mocked initialize_api, which sits BEFORE the racy read - so it also + # passed against the still-racy ordering it was meant to catch (a + # second-pass audit caught that). The write has to land AFTER the read + # the buggy version performed and BEFORE the persist, which is exactly + # the window _apply() opens: patching _apply to run the concurrent + # commit first reproduces "another connection committed while this + # save was between its read and its write". + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + + real_apply = settings_module._apply + injected = {"done": False} + + def _apply_with_a_concurrent_commit_in_the_window(mutation, *args): + if not injected["done"]: + injected["done"] = True + # Another connection's save lands and commits, in full, right + # here - after our read (if the code reads too early) and + # before our own write. + manager.set_api_settings(config.API_PROVIDER_ANTHROPIC, "", "", "sk-concurrent-ant", "") + return real_apply(mutation, *args) + + monkeypatch.setattr(settings_module, "_apply", _apply_with_a_concurrent_commit_in_the_window) bus = SessionBus("settings-save-race-regression-test") register_settings(bus, manager) @@ -525,13 +685,52 @@ def _simulate_concurrent_anthropic_save_during_our_own_initialize_api(*_args, ** ) ) + assert injected["done"], "the concurrent commit never ran - the test no longer exercises the window" assert manager.get_openai_key() == "sk-openai" # Must NOT be reverted to "" - our own save's "keep Anthropic as-is" - # logic must see the concurrently-committed value, not a stale read - # taken before the concurrent write happened. + # read has to happen inside the same locked section as its write, so it + # sees the concurrently-committed value rather than a stale earlier one. assert manager.get_anthropic_key() == "sk-concurrent-ant" +def test_load_api_models_aborts_when_another_request_repointed_the_provider_globals(manager, monkeypatch): + # Regression test for the F2 stale-provider guard, which shipped with + # no coverage of its own (a second-pass audit found that deleting the + # guard left the whole suite green). api_provider's provider state is + # process-global and initialize_api/get_available_model_descriptors are + # two separate to_thread hops - a concurrent call for a different + # provider landing between them makes the descriptors describe someone + # else's provider. The guard must refuse to persist that under this + # call's provider name. + from graphlink_model_catalog import ModelDescriptor + + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_OPENAI) + + def _descriptors_after_someone_else_switched_provider(): + # Simulates another connection's initialize_api completing in the + # await gap and repointing the process-global provider state. + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_ANTHROPIC) + return [ModelDescriptor(model_id="claude-sonnet", provider=config.API_PROVIDER_ANTHROPIC)] + + monkeypatch.setattr(api_provider, "get_available_model_descriptors", _descriptors_after_someone_else_switched_provider) + bus = SessionBus("settings-stale-provider-guard-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run( + bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "sk-test", "https://x/v1"]) + ) + + payload = recorder.messages[-1]["payload"] + assert payload["apiCatalogStatus"] == "error" + assert "another request" in payload["apiCatalogMessage"] + # The decisive assertion: Anthropic's models must NOT have been filed + # under OpenAI-Compatible. + assert manager.get_api_model_catalog(config.API_PROVIDER_OPENAI) == [] + + def test_catalog_state_is_isolated_per_provider(manager, monkeypatch): # Regression test: apiCatalogStatus/apiCatalogMessage used to be one # flat pair of session-local cells shared by every provider, so a load diff --git a/web_ui/src/app/chrome/SettingsDialog.test.tsx b/web_ui/src/app/chrome/SettingsDialog.test.tsx index 63bcedb..6c8d33f 100644 --- a/web_ui/src/app/chrome/SettingsDialog.test.tsx +++ b/web_ui/src/app/chrome/SettingsDialog.test.tsx @@ -212,16 +212,43 @@ describe("SettingsDialog", () => { expect(screen.getByPlaceholderText("Enter your API key...")).toHaveValue(""); }); - it("Reset API Settings fires resetApiSettings", async () => { + it("Reset API Settings does nothing on the first click - it only arms the confirmation", async () => { + // Reset irreversibly wipes every provider's saved key, so it must not + // be a single-click action. Legacy gated it behind a "This cannot be + // undone" Yes/No prompt. const { user, push, intents } = setup(); await goToApiEndpoint(user, push); await user.click(screen.getByText("Reset API Settings")); + expect(intents.some(([, intent]) => intent === "resetApiSettings")).toBe(false); + expect(screen.getByText(/cannot be undone/)).toBeInTheDocument(); + expect(screen.getByText("Confirm Reset")).toBeInTheDocument(); + }); + + it("Cancel backs out of an armed Reset without firing anything", async () => { + const { user, push, intents } = setup(); + await goToApiEndpoint(user, push); + + await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Cancel")); + + expect(intents.some(([, intent]) => intent === "resetApiSettings")).toBe(false); + expect(screen.queryByText("Confirm Reset")).toBeNull(); + expect(screen.getByText("Reset API Settings")).toBeInTheDocument(); + }); + + it("Confirm Reset fires resetApiSettings", async () => { + const { user, push, intents } = setup(); + await goToApiEndpoint(user, push); + + await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Confirm Reset")); + expect(intents).toContainEqual(["app-settings", "resetApiSettings", []]); }); - it("Reset API Settings clears the local Base URL and model drafts too, not just the key", async () => { + it("Confirm Reset clears the local Base URL and model drafts too, not just the key", async () => { // Regression test: Reset originally only cleared draftApiKey, leaving // draftBaseUrl/draftModels showing their pre-reset values (the resync // effect only fires on a genuine provider switch, which Reset doesn't @@ -235,6 +262,7 @@ describe("SettingsDialog", () => { await user.type(screen.getByLabelText("Chat, Explain, Takeaways (main model)"), "gpt-4o"); await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Confirm Reset")); expect(screen.getByPlaceholderText("https://api.openai.com/v1")).toHaveValue("https://api.openai.com/v1"); expect(screen.getByLabelText("Chat, Explain, Takeaways (main model)")).toHaveValue(""); diff --git a/web_ui/src/app/chrome/SettingsDialog.tsx b/web_ui/src/app/chrome/SettingsDialog.tsx index 44a7445..88d99c2 100644 --- a/web_ui/src/app/chrome/SettingsDialog.tsx +++ b/web_ui/src/app/chrome/SettingsDialog.tsx @@ -234,6 +234,7 @@ function ApiProviderPage({ const [draftApiKey, setDraftApiKey] = useState(""); const [draftBaseUrl, setDraftBaseUrl] = useState(state.apiBaseUrl); const [draftModels, setDraftModels] = useState>(state.apiModels); + const [confirmingReset, setConfirmingReset] = useState(false); const [lastViewingProvider, setLastViewingProvider] = useState(viewingProvider); if (viewingProvider !== lastViewingProvider) { @@ -241,6 +242,8 @@ function ApiProviderPage({ setDraftApiKey(""); setDraftBaseUrl(state.apiBaseUrl); setDraftModels(state.apiModels); + // Don't leave a Reset armed across a provider switch. + setConfirmingReset(false); } const catalogModelIds = state.apiModelCatalog.map((entry) => entry.modelId); @@ -349,24 +352,46 @@ function ApiProviderPage({ ))} + {confirmingReset && ( +

+ This clears all saved API keys and model configurations and cannot be undone. +

+ )} +
- + {/* Reset wipes every provider's key irreversibly. Legacy gated it + behind a "This cannot be undone" Yes/No prompt; this is the same + two-step confirm the legacy settings island and ChatLibraryDialog + already use, rather than a one-click destructive button. */} + {confirmingReset ? ( + <> + + + + ) : ( + + )}