diff --git a/deploy/env.template b/deploy/env.template index fce367f..a95fb16 100644 --- a/deploy/env.template +++ b/deploy/env.template @@ -71,6 +71,13 @@ # file ships with the repo; without this line the server runs with no persona. NAVI_PERSONA_FILE=persona.txt +# ─── Web search (navi-web MCP server) ───────────────────────────────── +# Optional. The server's primary search backend is a self-hosted SearXNG; +# without it the chain starts at DuckDuckGo scraping (slower, first to be +# rate-limited). The config maps this through ${SEARXNG_URL:-}, so a missing +# value costs you the backend, not the server. See docs/mcp.md#navi-web-settings. +#SEARXNG_URL=http://127.0.0.1:8888 + # ─── MCP server credentials ────────────────────────────────────────── # mcp_servers.d/*.json is tracked in git and must never hold a live token: it # names the variable (e.g. "Authorization": "Bearer ${NAVI_MCP_GNTODO_TOKEN}") diff --git a/docs/config.md b/docs/config.md index 17c293d..9116de4 100644 --- a/docs/config.md +++ b/docs/config.md @@ -33,10 +33,12 @@ ## Web Search -| Variable | Type | Default | Description | -|---|---|---|---| -| `BRAVE_SEARCH_API_KEY` | str | `""` | Brave Search API key (free tier: 2000 req/month). Used when DuckDuckGo returns no results. | -| `SEARXNG_URL` | str | `""` | Self-hosted SearXNG meta-search URL, e.g. `http://localhost:8888` | +Web search is not a `Settings` field: it is the `navi-web` MCP server. Its settings +reach it through `mcp_servers.d/navi-web.json`, which maps `SEARXNG_URL` (and, +optionally, `BRAVE_SEARCH_API_KEY`) from `.env` into the server's own environment. +`SEARXNG_URL` is therefore a real `.env` setting — a self-hosted SearXNG, e.g. +`http://localhost:8888` — just not one Navi itself reads. Without it the server +falls back to DuckDuckGo scraping. See [`mcp.md`](mcp.md#navi-web-settings). ### Multi-server fallback (`OLLAMA_BACKENDS_FILE`) diff --git a/navi/config.py b/navi/config.py index eda9c31..6b26f56 100644 --- a/navi/config.py +++ b/navi/config.py @@ -33,11 +33,8 @@ openai_base_url: str | None = None anthropic_api_key: str = "" - # Web search fallbacks (used when DuckDuckGo returns no results) - # Brave Search API: free tier = 2000 req/month — https://brave.com/search/api/ - brave_search_api_key: str = "" - # SearXNG: self-hosted meta-search, e.g. "http://localhost:8888" - searxng_url: str = "" + # Web search is not configured here: it is the navi-web MCP server's own + # settings (NAVI_WEB_*), mapped from .env by mcp_servers.d/navi-web.json. # Filesystem tool: comma-separated allowed root paths fs_allowed_paths: str = "*" diff --git a/navi/mcp/secrets.py b/navi/mcp/secrets.py index 204177d..334c8f8 100644 --- a/navi/mcp/secrets.py +++ b/navi/mcp/secrets.py @@ -30,10 +30,11 @@ from .config import McpServerConfig -# Braced ``${NAME}`` only. A bare ``$NAME`` is deliberately left alone: header -# values legitimately contain ``$`` (nginx-style variables), and substituting -# there would corrupt them. -_PLACEHOLDER = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}") +# Braced ``${NAME}`` only, plus the optional ``${NAME:-fallback}`` form for a +# setting that merely tunes a server. A bare ``$NAME`` is deliberately left +# alone: header values legitimately contain ``$`` (nginx-style variables), and +# substituting there would corrupt them. +_PLACEHOLDER = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}") def env_values() -> dict[str, str]: @@ -72,6 +73,13 @@ 401, so dropping silently fails open. ``McpManager.load_all`` isolates a failing server, so this surfaces as one broken entry in ``/mcp/status`` rather than a dead startup. + + A placeholder may opt out explicitly with ``${NAME:-fallback}`` + (the fallback may be empty). That form is for a *setting* that tunes + a server — a SearXNG URL, say — where being unset degrades one + feature rather than handing out someone else's access. Writing the + fallback down is the point: it is a decision in the config, not an + accident of a variable nobody set. """ if not cfg.headers and not cfg.env: return cfg @@ -101,11 +109,14 @@ def replace(match: re.Match[str]) -> str: name = match.group(1) - if name not in pool: - raise ValueError( - f"{where} references ${{{name}}}, which is not set — " - "add it to the service .env (see docs/mcp.md)" - ) - return pool[name] + fallback = match.group(2) # None for the plain ``${NAME}`` form + if name in pool: + return pool[name] + if fallback is not None: + return fallback + raise ValueError( + f"{where} references ${{{name}}}, which is not set — " + "add it to the service .env (see docs/mcp.md)" + ) return _PLACEHOLDER.sub(replace, value) diff --git a/tests/unit/mcp/test_no_literal_secrets.py b/tests/unit/mcp/test_no_literal_secrets.py index bd1bafa..68685fd 100644 --- a/tests/unit/mcp/test_no_literal_secrets.py +++ b/tests/unit/mcp/test_no_literal_secrets.py @@ -19,7 +19,13 @@ REPO_ROOT = Path(mcp_config.__file__).resolve().parents[2] CONFIG_DIR = REPO_ROOT / "mcp_servers.d" -_PLACEHOLDER = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}") +_PLACEHOLDER = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}") + +# Non-credential settings a config may map out of `.env`. Not secrets: the +# placeholder is there so the value stays out of a tracked file (one `.env` +# serves every machine), and the fallback form keeps a host without one working +# — see `docs/mcp.md#secrets`. A setting belongs in `env`, never in a header. +SETTINGS_VARIABLES = {"SEARXNG_URL"} # The credential-bearing servers, and the variable each one names. Keep in step # with `deploy/env.template`. @@ -32,6 +38,11 @@ } +def _names(value: object) -> list[str]: + return [match.group(1) for match in _PLACEHOLDER.finditer(str(value))] + + + def _configs() -> dict[str, dict]: return { path.stem: json.loads(path.read_text(encoding="utf-8")) @@ -53,20 +64,38 @@ ) -def test_placeholder_names_are_env_variables(): +def test_placeholder_names_are_credentials_or_known_settings(): names = { name for cfg in _configs().values() for field in ("headers", "env") for value in (cfg.get(field) or {}).values() - for name in _PLACEHOLDER.findall(str(value)) + for name in _names(value) } assert names, "no placeholders in any config — did a token get pasted back in?" - assert not [name for name in names if not name.startswith("NAVI_MCP_")], ( - f"unexpected placeholder names: {sorted(names)}" + unexpected = [ + name + for name in names + if not name.startswith("NAVI_MCP_") and name not in SETTINGS_VARIABLES + ] + assert not unexpected, ( + "unexpected placeholder names — a credential is NAVI_MCP_*, a plain " + f"setting must be listed in SETTINGS_VARIABLES: {sorted(unexpected)}" ) +def test_settings_placeholders_live_in_env_blocks(): + """`headers` carry credentials; a SearXNG URL there would mean someone + mapped a setting through the wrong field.""" + offenders = [ + f"{server}: headers.{key}" + for server, cfg in _configs().items() + for key, value in (cfg.get("headers") or {}).items() + if any(name in SETTINGS_VARIABLES for name in _names(value)) + ] + assert not offenders, f"a plain setting belongs in `env`: {offenders}" + + def test_the_credential_servers_name_their_variable(): configs = _configs() for server, variable in EXPECTED_VARIABLES.items(): diff --git a/tests/unit/mcp/test_secrets.py b/tests/unit/mcp/test_secrets.py index cb49ebd..d99a457 100644 --- a/tests/unit/mcp/test_secrets.py +++ b/tests/unit/mcp/test_secrets.py @@ -88,6 +88,48 @@ assert "headers.Authorization" in str(excinfo.value) +class TestExplicitFallback: + """``${NAME:-fallback}`` opts a *setting* out of the fail-loud rule. + + A credential must never take this form — dropping it fails open — but a + SearchXNG URL that is merely unset should degrade one backend instead of + taking the whole server (all of its tools) down with it. + """ + + def _env(self, value: str) -> dict: + return McpServerConfig(transport="stdio", command="python", env={"SETTING": value}).env + + def test_empty_fallback_yields_an_empty_string(self): + assert resolve_secrets( + McpServerConfig( + transport="stdio", command="python", env={"SETTING": "${NAVI_TEST_MISSING:-}"} + ), + {}, + ).env["SETTING"] == "" + + def test_written_fallback_is_used(self): + cfg = McpServerConfig( + transport="stdio", command="python", env={"SETTING": "${NAVI_TEST_MISSING:-x:1}"} + ) + assert resolve_secrets(cfg, {}).env["SETTING"] == "x:1" + + def test_a_set_variable_beats_the_fallback(self): + cfg = McpServerConfig( + transport="stdio", command="python", env={"SETTING": "${NAVI_TEST_SET:-fallback}"} + ) + assert resolve_secrets(cfg, {"NAVI_TEST_SET": "real"}).env["SETTING"] == "real" + + def test_the_plain_form_still_raises(self): + """The security-relevant shape: no fallback means no silent drop.""" + with pytest.raises(ValueError): + resolve_secrets( + McpServerConfig( + transport="stdio", command="python", env={"KEY": "${NAVI_TEST_MISSING}"} + ), + {}, + ) + + class TestEnvValues: def test_process_environment_beats_the_env_file(self, monkeypatch, tmp_path): monkeypatch.chdir(tmp_path)