From 94d952c3bb5643eaf779ccef723fa1747f5f436d Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 28 Jun 2026 05:57:56 +0000 Subject: [PATCH] fix: harden None providers config across onboarding/providers/routes (salvage of #3967) A config.yaml with an explicit empty/null `providers:` key parses to None in PyYAML. `cfg.get("providers", {})` only returns the {} default when the key is *absent*, so an explicit null yields None instead of an empty mapping. master already guards each read with `isinstance(providers_cfg, dict)`, but the value fed to that guard is still a footgun: the natural chained form `cfg.get("providers", {}).get(...)` (already fixed once in api/config.py) crashes with AttributeError on None. Harden every providers-key read in the three remaining files to `cfg.get("providers") or {}` so an explicit null degrades to an empty dict at the source, matching the existing api/config.py convention. Sites hardened: api/onboarding.py:578 (_provider_api_key_present) api/providers.py:1051 (_provider_has_shadowed_codex_oauth_value) api/providers.py:1219 (_provider_has_key) api/providers.py:1265 (_get_provider_api_key) api/providers.py:2348 (get_providers catalog) api/providers.py:2781 (_clean_provider_key_from_config) api/routes.py:5045 (_context_length_config_api_key_for_provider) api/routes.py:5110 (_context_length_lookup_inputs_for_model) api/routes.py:15916 (_handle_live_models) Adds tests/test_none_providers_config_guard.py: per-file source-form pins (fail on master / on any single-file revert) plus behavioural checks driving the real functions with `providers: None` to prove no crash and parity with the empty-mapping config. Updates the existing test_issue3717 routes-string pin to match the hardened form. This is a focused salvage of grab-bag PR #3967 -- only the None-providers hardening is taken; the PR's unrelated changes (account-usage worker-pool refactor, function removals) are intentionally dropped. Co-authored-by: lidi1011 --- api/onboarding.py | 5 +- api/providers.py | 10 +- api/routes.py | 6 +- ...e3717_context_length_provider_overrides.py | 5 +- tests/test_none_providers_config_guard.py | 189 ++++++++++++++++++ 5 files changed, 205 insertions(+), 10 deletions(-) create mode 100644 tests/test_none_providers_config_guard.py diff --git a/api/onboarding.py b/api/onboarding.py index 7cf1bbe1..ecaf1fbc 100644 --- a/api/onboarding.py +++ b/api/onboarding.py @@ -572,7 +572,10 @@ def _provider_api_key_present( if isinstance(model_cfg, dict) and str(model_cfg.get("api_key") or "").strip(): return True - providers_cfg = cfg.get("providers", {}) + # ``cfg.get("providers", {})`` only returns the default when the key is + # absent; an explicit ``providers:`` (null) in config.yaml yields ``None``. + # ``... or {}`` degrades that null to an empty mapping (salvage of #3967). + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): provider_cfg = providers_cfg.get(provider, {}) if ( diff --git a/api/providers.py b/api/providers.py index ec4a1b49..489aedc8 100644 --- a/api/providers.py +++ b/api/providers.py @@ -1048,7 +1048,7 @@ def _provider_has_shadowed_codex_oauth_value(provider_id: str) -> bool: active_provider = str(model_cfg.get("provider") or "").strip().lower() if active_provider == provider_id: values.append(model_cfg.get("api_key")) - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): provider_cfg = providers_cfg.get(provider_id, {}) if isinstance(provider_cfg, dict): @@ -1216,7 +1216,7 @@ def _provider_has_key(provider_id: str) -> bool: if _provider_value_counts_as_api_key(provider_id, model_cfg.get("api_key")): return True # Check providers..api_key - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): provider_cfg = providers_cfg.get(provider_id, {}) if isinstance(provider_cfg, dict) and str(provider_cfg.get("api_key") or "").strip(): @@ -1262,7 +1262,7 @@ def _get_provider_api_key(provider_id: str) -> str | None: if model_key and active_provider == provider_id and _provider_value_counts_as_api_key(provider_id, model_key): return model_key - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): provider_cfg = providers_cfg.get(provider_id, {}) if isinstance(provider_cfg, dict): @@ -2345,7 +2345,7 @@ def get_providers() -> dict[str, Any]: # Also detect providers from config.yaml providers section cfg = get_config() - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): known_ids.update(providers_cfg.keys()) @@ -2778,7 +2778,7 @@ def _clean_provider_key_from_config(provider_id: str) -> None: return # 1. Clean providers..api_key - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): provider_cfg = providers_cfg.get(provider_id, {}) if isinstance(provider_cfg, dict) and provider_cfg.get("api_key"): diff --git a/api/routes.py b/api/routes.py index b4daa03b..279b3ca0 100644 --- a/api/routes.py +++ b/api/routes.py @@ -5042,7 +5042,7 @@ def _context_length_config_api_key_for_provider( return resolved return "" - providers_cfg = cfg.get("providers", {}) + providers_cfg = cfg.get("providers") or {} if isinstance(providers_cfg, dict): for provider_key, provider_cfg in providers_cfg.items(): if not isinstance(provider_cfg, dict): @@ -5107,7 +5107,7 @@ def _context_length_lookup_inputs_for_model( custom_providers = None provider_context_length = None - providers_cfg = cfg.get("providers", {}) if isinstance(cfg, dict) else {} + providers_cfg = (cfg.get("providers") or {}) if isinstance(cfg, dict) else {} if isinstance(providers_cfg, dict): for provider_key, provider_cfg in providers_cfg.items(): if not isinstance(provider_cfg, dict): @@ -16243,7 +16243,7 @@ def _handle_live_models(handler, parsed): if _ep: try: import urllib.request - _providers_cfg = cfg.get("providers", {}) + _providers_cfg = cfg.get("providers") or {} _prov = _providers_cfg.get(provider, {}) if isinstance(_providers_cfg, dict) else {} # Only use a provider-scoped key. A top-level model.api_key # is safe here only when it belongs to the requested provider; diff --git a/tests/test_issue3717_context_length_provider_overrides.py b/tests/test_issue3717_context_length_provider_overrides.py index 39e7ab18..f1fe0c20 100644 --- a/tests/test_issue3717_context_length_provider_overrides.py +++ b/tests/test_issue3717_context_length_provider_overrides.py @@ -193,6 +193,9 @@ def test_streaming_fallbacks_use_shared_provider_context_helper(): def test_route_helper_keeps_all_context_length_sources_aligned(): assert "def _context_length_lookup_inputs_for_model" in ROUTES_PY - assert 'cfg.get("providers", {})' in ROUTES_PY + # Hardened against an explicit null ``providers:`` key (salvage of #3967): + # ``cfg.get("providers") or {}`` instead of ``cfg.get("providers", {})`` so + # a config.yaml with ``providers:`` (None) degrades to an empty mapping. + assert 'cfg.get("providers") or {}' in ROUTES_PY assert 'cfg.get("custom_providers")' in ROUTES_PY assert "_model_matches_configured_default" in ROUTES_PY diff --git a/tests/test_none_providers_config_guard.py b/tests/test_none_providers_config_guard.py new file mode 100644 index 00000000..8a5c8ce7 --- /dev/null +++ b/tests/test_none_providers_config_guard.py @@ -0,0 +1,189 @@ +"""Regression guard: a config with an explicit null ``providers:`` key must not +break the provider-config read paths in onboarding / providers / routes, and +must behave exactly as if ``providers`` were an empty mapping. + +Background (salvage of grab-bag PR #3967, thanks @lidi1011) +---------------------------------------------------------- +``cfg.get("providers", {})`` only returns the ``{}`` default when the key is +*absent*. When ``config.yaml`` contains an explicit empty/null key:: + + providers: + +PyYAML parses that as ``None``, so ``cfg.get("providers", {})`` yields ``None`` +(not ``{}``). master already wraps each of these reads in an +``isinstance(providers_cfg, dict)`` guard, so the None value silently disables +the provider lookup -- but the value handed to that guard is still a footgun: +any future maintainer who chains ``cfg.get("providers", {}).get(...)`` (the +natural-looking pattern, already used in api/config.py before its own #3967-style +fix) would crash with ``AttributeError: 'NoneType' object has no attribute +'get'``. + +The hardening replaces every such read with ``cfg.get("providers") or {}`` so an +explicit null degrades to an empty dict at the *source*, matching the existing +api/config.py convention. + +This module pins the fix two ways: + +1. **Source-form pins** (``test__hardens_providers_key_read``): assert that + onboarding / providers / routes no longer contain the unguarded + ``cfg.get("providers", {})`` form and *do* contain the hardened + ``cfg.get("providers") or {}`` form. These FAIL on master (where the + unguarded form is present) and would FAIL again if anyone reverts the guard + in any one of the three files -- so the test is non-vacuous per file. + +2. **Behavioural checks**: drive the real functions with ``providers: None`` and + assert they neither raise nor diverge from the ``providers: {}`` / absent-key + behaviour. This guards the runtime contract end to end. +""" + +import ast +import pathlib + +import pytest + +import api.config as config +import api.onboarding as onboarding +import api.providers as providers +import api.routes as routes + +_REPO = pathlib.Path(__file__).resolve().parents[1] + +_UNGUARDED = 'cfg.get("providers", {})' +_HARDENED = 'cfg.get("providers") or {}' + + +def _src(name: str) -> str: + return (_REPO / "api" / name).read_text(encoding="utf-8") + + +# --- source-form pins (fail on master / on any per-file revert) ----------- + + +@pytest.mark.parametrize("filename", ["onboarding.py", "providers.py", "routes.py"]) +def test_file_hardens_providers_key_read_against_none(filename): + """Each target file must read the providers key as ``... or {}``. + + Non-vacuous: on master the unguarded ``cfg.get("providers", {})`` form is + present in every one of these files, so this assertion fails there. Reverting + the guard in any single file re-fails it -- the guard is pinned per file. + """ + source = _src(filename) + # The hardened form must appear at least once. + assert _HARDENED in source, ( + f"api/{filename} should read the providers key as `{_HARDENED}` so an " + f"explicit null `providers:` degrades to an empty mapping (salvage #3967)." + ) + # The unguarded form must not appear as live code. It may legitimately appear + # inside an explanatory comment, so strip comment-only occurrences before + # asserting absence. + code_lines = [] + for line in source.splitlines(): + stripped = line.lstrip() + if stripped.startswith("#"): + continue + # Drop trailing inline comments crudely (good enough: the token we look + # for contains no '#'). + code_lines.append(line.split("#", 1)[0]) + code = "\n".join(code_lines) + assert _UNGUARDED not in code, ( + f"api/{filename} still reads the providers key as the unguarded " + f"`{_UNGUARDED}`; an explicit `providers:` (None) in config.yaml would " + f"yield None instead of an empty mapping (salvage #3967)." + ) + + +@pytest.mark.parametrize("filename", ["onboarding.py", "providers.py", "routes.py"]) +def test_target_files_parse(filename): + """Sanity: the hardened files are valid Python (guards real edits).""" + ast.parse(_src(filename), filename=filename) + + +# --- behavioural: onboarding ---------------------------------------------- + + +def _onboarding_cfg(providers_value): + return { + "model": {"provider": "custom", "api_key": "", "base_url": "http://localhost:1"}, + "providers": providers_value, + "custom_providers": [], + } + + +def test_onboarding_provider_api_key_present_handles_none_providers(): + """``_provider_api_key_present`` must treat ``providers: None`` like ``{}``.""" + none_cfg = _onboarding_cfg(None) + empty_cfg = _onboarding_cfg({}) + + for provider in ("custom", "openai", "anthropic"): + got_none = onboarding._provider_api_key_present(provider, none_cfg, {}) + got_empty = onboarding._provider_api_key_present(provider, empty_cfg, {}) + assert got_none == got_empty + assert got_none is False # no key configured anywhere + + +def test_onboarding_none_providers_does_not_mask_configured_key(): + """A provider key set under model.api_key is still found with providers: None.""" + cfg = _onboarding_cfg(None) + cfg["model"] = {"provider": "custom", "api_key": "sk-xyz", "base_url": "http://x"} + assert onboarding._provider_api_key_present("custom", cfg, {}) is True + + +# --- behavioural: providers (catalog + key lookup) ------------------------ + + +def _patch_get_config(monkeypatch, cfg): + """Force api.providers.get_config()/api.config.get_config() to return ``cfg``.""" + monkeypatch.setattr(providers, "get_config", lambda: cfg) + # Some helpers import get_config from api.config at call time. + monkeypatch.setattr(config, "get_config", lambda: cfg, raising=False) + + +def _providers_cfg(providers_value): + return { + "model": {"provider": "custom", "api_key": "", "base_url": "http://localhost:1"}, + "providers": providers_value, + "custom_providers": [], + } + + +def test_providers_has_key_handles_none_providers(monkeypatch): + """``_provider_has_key`` must not crash and must match the empty-dict result.""" + monkeypatch.setattr(providers, "_provider_env_var_for", lambda pid: None) + monkeypatch.setattr( + providers, "_load_env_file", lambda path: {}, raising=False + ) + + for providers_value, label in ((None, "none"), ({}, "empty")): + _patch_get_config(monkeypatch, _providers_cfg(providers_value)) + # Should return False (no key) without raising on the None config. + assert providers._provider_has_key("custom") is False, label + assert providers._get_provider_api_key("custom") is None, label + + +# --- behavioural: routes (context-length helper + key resolution) ---------- + + +def test_routes_context_length_helper_handles_none_providers(): + """``_context_length_lookup_inputs_for_model`` must accept providers: None.""" + cfg_none = { + "model": {"provider": "custom", "base_url": "http://localhost:1"}, + "providers": None, + "custom_providers": [], + } + cfg_empty = dict(cfg_none, providers={}) + + # Pass cfg explicitly so the helper does not hit real config/disk. + out_none = routes._context_length_lookup_inputs_for_model( + "@custom:my-model", "custom", cfg=cfg_none + ) + out_empty = routes._context_length_lookup_inputs_for_model( + "@custom:my-model", "custom", cfg=cfg_empty + ) + # No crash, and the None config produces the same lookup inputs as the + # empty-mapping config (no provider override available in either case). + # ``_ContextLengthLookupInputs`` is a __slots__ class without __eq__, so + # compare the materialised fields rather than object identity. + fields = ("config_context_length", "custom_providers", "base_url", "provider", "api_key") + snapshot_none = {f: getattr(out_none, f) for f in fields} + snapshot_empty = {f: getattr(out_empty, f) for f in fields} + assert snapshot_none == snapshot_empty