diff --git a/routes/auth_routes.py b/routes/auth_routes.py index e171f97..42ba0cb 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -8,6 +8,7 @@ import os from core.auth import AuthManager from src.rate_limiter import RateLimiter +from src.settings_scrub import scrub_settings from src.settings import ( load_settings as _load_settings, save_settings as _save_settings, @@ -371,29 +372,6 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: # ---- App settings (admin-managed) ---- - _SECRET_KEY_PATTERNS = ("_api_key", "_password", "_secret", "_token", "_key") - - def _is_secret_key(name: str) -> bool: - n = (name or "").lower() - if n in ("google_pse_cx",): # public identifier, not a secret - return False - return any(n.endswith(p) or n == p.lstrip("_") for p in _SECRET_KEY_PATTERNS) - - def _scrub_settings(settings: dict) -> dict: - """Return a copy of settings with secret-shaped values masked. - - Frontend reads /settings without auth for things like keybinds + TTS - prefs. Secrets (search-provider keys, IMAP/SMTP passwords) must NOT - be exposed to non-admin callers. - """ - scrubbed = {} - for k, v in (settings or {}).items(): - if _is_secret_key(k) and isinstance(v, str) and v: - scrubbed[k] = "" # presence preserved, value blanked - else: - scrubbed[k] = v - return scrubbed - @router.get("/settings") async def get_settings(request: Request): """Returns app settings. Admins get the full set; non-admins get @@ -403,7 +381,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: settings = _load_settings() if user and auth_manager.is_admin(user): return settings - return _scrub_settings(settings) + return scrub_settings(settings) @router.post("/settings") async def set_settings(request: Request): diff --git a/src/settings_scrub.py b/src/settings_scrub.py new file mode 100644 index 0000000..614dbf9 --- /dev/null +++ b/src/settings_scrub.py @@ -0,0 +1,50 @@ +"""Secret-scrubbing for settings exposed to non-admin / unauthenticated callers. + +Deliberately dependency-light (stdlib only) and separate from +``routes/auth_routes.py`` so it can be imported and unit-tested without dragging +in the FastAPI app / auth / database import chain. + +``/api/auth/settings`` is auth-exempt — the frontend (and the pre-login page) +read it for keybinds + TTS prefs, so non-admin and unauthenticated callers get a +*scrubbed* copy. Secrets (provider API keys, IMAP/SMTP passwords, OAuth tokens) +must NOT leak to them — load-bearing when the app is reachable over a Cloudflare +tunnel / reverse proxy. Scrubbing is deep (recurses nested dicts/lists) and keyed +on secret-shaped names. +""" + +_SECRET_KEY_PATTERNS = ( + "_api_key", "_apikey", "_password", "_passwd", "_pass", "_pwd", + "_secret", "_client_secret", "_token", "_access_token", "_refresh_token", + "_credential", "_credentials", "_key", +) +_SECRET_KEY_ALLOW = ("google_pse_cx",) # public identifiers, not secrets + + +def is_secret_key(name: str) -> bool: + n = (name or "").lower() + if n in _SECRET_KEY_ALLOW: + return False + return any(n.endswith(p) or n == p.lstrip("_") for p in _SECRET_KEY_PATTERNS) + + +def _scrub_value(key, value): + """Mask secret-shaped leaves, recursing into nested dicts/lists so a secret + stored under a non-secret parent key (e.g. + ``{"email_account": {"smtp_password": "..."}}``) is still blanked. Only + non-empty *string* values are blanked; presence is preserved.""" + if isinstance(value, dict): + return { + k: ("" if (is_secret_key(k) and isinstance(v, str) and v) + else _scrub_value(k, v)) + for k, v in value.items() + } + if isinstance(value, list): + return [_scrub_value(key, item) for item in value] + if is_secret_key(key) and isinstance(value, str) and value: + return "" + return value + + +def scrub_settings(settings: dict) -> dict: + """Return a copy of ``settings`` with secret-shaped values masked (deep).""" + return {k: _scrub_value(k, v) for k, v in (settings or {}).items()} diff --git a/tests/test_settings_scrub.py b/tests/test_settings_scrub.py new file mode 100644 index 0000000..2d489aa --- /dev/null +++ b/tests/test_settings_scrub.py @@ -0,0 +1,61 @@ +"""Security tests for the /api/auth/settings secret scrubbing. + +The /settings endpoint is auth-exempt (the frontend + the pre-login page read it +for keybinds / TTS prefs), so non-admin and unauthenticated callers receive a +*scrubbed* copy. Secrets must never leak to them — load-bearing when the app is +reachable over a Cloudflare tunnel / reverse proxy. These pin the scrub: deep +(nested), broad secret-key coverage, and no collateral damage to real prefs. + +Imports the stdlib-only `src.settings_scrub` directly, so the test does not pull +in the FastAPI / auth / database import chain. +""" +from src.settings_scrub import is_secret_key, scrub_settings + + +def test_top_level_secrets_blanked(): + out = scrub_settings({"search_api_key": "S", "openai_api_key": "K", "smtp_password": "P"}) + assert out["search_api_key"] == "" and out["openai_api_key"] == "" and out["smtp_password"] == "" + + +def test_broadened_patterns_blanked(): + s = {"smtp_pass": "a", "db_pwd": "b", "oauth_client_secret": "c", + "gh_access_token": "d", "refresh_token": "e", "x_credential": "f", "z_apikey": "g"} + out = scrub_settings(s) + assert all(out[k] == "" for k in s), out + + +def test_nested_secret_blanked(): + out = scrub_settings({"email_account": {"host": "imap", "smtp_password": "NESTED"}}) + assert out["email_account"]["host"] == "imap" # non-secret preserved + assert out["email_account"]["smtp_password"] == "" # nested secret blanked + + +def test_secret_in_list_of_dicts_blanked(): + out = scrub_settings({"providers": [{"name": "a", "api_key": "P1"}, + {"name": "b", "access_token": "T2"}]}) + assert out["providers"][0]["name"] == "a" + assert out["providers"][0]["api_key"] == "" + assert out["providers"][1]["access_token"] == "" + + +def test_non_secret_keys_preserved(): + s = {"keybinds": {"send": "Enter"}, "theme": "dark", "image_model": "x", + "default_endpoint_id": "ep1", "search_result_count": 5, "tts_enabled": True} + assert scrub_settings(s) == s # untouched + + +def test_google_pse_cx_is_public(): + assert is_secret_key("google_pse_cx") is False + assert scrub_settings({"google_pse_cx": "cx123"})["google_pse_cx"] == "cx123" + + +def test_empty_and_nonstring_secret_values_untouched(): + out = scrub_settings({"api_key": "", "feature_key": 7, "x_token": None}) + assert out["api_key"] == "" # already empty + assert out["feature_key"] == 7 # int not blanked (string-only) + assert out["x_token"] is None # None not blanked + + +def test_exact_name_matches(): + out = scrub_settings({"password": "p", "token": "t", "secret": "s", "apikey": "a", "key": "k"}) + assert all(v == "" for v in out.values()), out