diff --git a/.env.example b/.env.example index a73985a..76448d6 100644 --- a/.env.example +++ b/.env.example @@ -122,3 +122,15 @@ # https://. Set this to force it on for an https navi deploy behind an http # auth server (leave empty otherwise). # NAVI_AUTH_COOKIE_SECURE=true + +# ── 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}") +# and the value lives here / in the process environment. Canonical store is +# gnexus-creds — rotate there, then update this file. A blank value counts as +# missing and the server refuses to connect (see docs/mcp.md#secrets). +NAVI_MCP_GNEXUS_CREDS_TOKEN= +NAVI_MCP_GNTODO_TOKEN= +NAVI_MCP_HARD_PANEL_TOKEN= +NAVI_MCP_SYNAPSE_TOKEN= +NAVI_MCP_TGCLIENT_TOKEN= diff --git a/deploy/UPDATE.md b/deploy/UPDATE.md index 2f4082d..88bde41 100644 --- a/deploy/UPDATE.md +++ b/deploy/UPDATE.md @@ -140,14 +140,70 @@ (WebView `cacheMode = LOAD_NO_CACHE`). Reopen the app after the deploy. - **`.env` trap**: on `deploy` it is tracked; `git checkout master` deletes it. Only ever move the branch toward `deploy`, and back it up first. -- **Secrets in the tree**: `mcp_servers.d/*.json` is tracked and carries the - shared `gcr_…` credential in plaintext. A pull can overwrite the prod copy — - diff before accepting. +- **Secrets in the tree**: `mcp_servers.d/*.json` is tracked, but it no longer + carries a live credential — it names the variable (`Bearer ${NAVI_MCP_…}`) and + the value lives in `.env` (chmod 600, untracked). A prod copy still holding a + literal `gcr_…` predates that change: run the migration below **before** + pulling. - **Config slot must exist on prod too**: the MCP personal-key input appears only for servers whose prod-side config declares `user_key`. If the prod copy of `mcp_servers.d/gnexus-creds.json` is not updated, the panel shows the row read-only. +## Migration: MCP credentials moved out of the tree (one-time) + +Before this change the five credential-bearing configs +(`gnexus-creds`, `gntodo`, `hard-panel`, `synapse`, `tgclient`) carried their +bearer tokens in plaintext in `mcp_servers.d/*.json` — in git, and in every +clone. They now name a variable instead, and the values live in `.env`. + +Do this in order. Values are readable from the working tree until the pull, and +from `git show :mcp_servers.d/.json` afterwards, so a fast-forward +leaves nothing unrecoverable. + +```bash +cd ~/navi # the prod clone +cp .env ~/navi-env-$(date +%F-%H%M).bak # copy FROM .env, never onto it +SHA=$(git rev-parse --short HEAD) # the pre-change commit + +# 1. Harvest the five tokens into .env (chmod 600). Names come from +# deploy/env.template; values are the `Bearer …` payload without the prefix. +python3 - <<'PY' +import json, pathlib +servers = {"gnexus-creds": "NAVI_MCP_GNEXUS_CREDS_TOKEN", + "gntodo": "NAVI_MCP_GNTODO_TOKEN", + "hard-panel": "NAVI_MCP_HARD_PANEL_TOKEN", + "synapse": "NAVI_MCP_SYNAPSE_TOKEN", + "tgclient": "NAVI_MCP_TGCLIENT_TOKEN"} +out = [] +for srv, var in servers.items(): + raw = json.loads(pathlib.Path(f"mcp_servers.d/{srv}.json").read_text()) + out.append(f"{var}={raw['headers']['Authorization'][len('Bearer '):]}") +with open(".env", "a") as f: + f.write("\n# MCP server credentials (docs/mcp.md#secrets)\n" + "\n".join(out) + "\n") +PY +chmod 600 .env + +# 2. Only now update the code. +git fetch origin && git pull --ff-only +systemctl restart navi +curl -fsS http://127.0.0.1:8099/health +``` + +Verify: `/mcp/status` reports all servers connected and the startup log has **no** +`references ${NAVI_MCP_…}, which is not set` — that error means step 1 was skipped +or a value is blank, and the affected server will be down (by design: an +unauthenticated connect is refused rather than made silently). + +**Then rotate all five tokens at their sources** (gntodo, hard-panel, synapse, +tgclient, and the gnexus-creds MCP token itself) and update `.env` and +gnexus-creds. This is not optional: the old values stay in git history forever, +so only rotation makes them worthless. Rotation needs no commit — `.env` is +untracked, and it is re-read on every reconnect. + +Optional, if the repo is shared and you want the old values gone from history +too: `git filter-repo` / BFG on the GitBucket remote, then everyone re-clones. + ## This release, specifically (e68c33e → 10385c6) - Android WebView push through the `NaviAndroid` JS bridge, with a foreground diff --git a/deploy/env.template b/deploy/env.template index 61c6981..efb786c 100644 --- a/deploy/env.template +++ b/deploy/env.template @@ -65,3 +65,15 @@ NAVI_VAPID_PRIVATE_KEY= NAVI_VAPID_SUBJECT=mailto:admin@navi.local NAVI_PUSH_COOLDOWN_SEC=30 + +# ─── 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}") +# and the value lives here. Canonical store is gnexus-creds — rotate there, +# then update this file and restart. A blank value counts as missing and the +# server refuses to connect. See docs/mcp.md#secrets. +NAVI_MCP_GNEXUS_CREDS_TOKEN= +NAVI_MCP_GNTODO_TOKEN= +NAVI_MCP_HARD_PANEL_TOKEN= +NAVI_MCP_SYNAPSE_TOKEN= +NAVI_MCP_TGCLIENT_TOKEN= diff --git a/docs/mcp.md b/docs/mcp.md index ec7bba9..05a82db 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -11,7 +11,7 @@ { "transport": "streamable_http", "url": "https://creds.example.com/mcp", - "headers": { "Authorization": "Bearer gcr_system_default" }, + "headers": { "Authorization": "Bearer ${NAVI_MCP_GNEXUS_CREDS_TOKEN}" }, "groups": { "creds": ["search_secrets", "get_secret", "reveal_secret"] }, @@ -37,6 +37,39 @@ the whole model — write back the config you read, or a `user_key` slot added by hand is silently erased. +## Secrets + +`mcp_servers.d/*.json` is tracked in git, so a credential written into one is in +history for good — and `GET /admin/mcp/config` hands the same value to the admin +client. A config therefore never carries a live credential: it names the +variable that holds it. + +```json +"headers": { "Authorization": "Bearer ${NAVI_MCP_GNTODO_TOKEN}" } +``` + +- **Only `headers` and `env` values are scanned**, and only the braced `${NAME}` + form — a bare `$NAME` is left alone, so header values that contain `$` survive. +- **The value comes from the process environment over the project's `.env`** + (chmod 600, untracked; environment variable beats the file, as with + `Settings`). A blank `NAME=` in `.env` counts as missing. `.env` is re-read on + every (re)connect, so a rotated token is picked up without a restart — the + canonical store for the value itself is **gnexus-creds** (see the platform's + secrets rules; `deploy/env.template` lists the variable names). +- **A missing variable fails the connection** with `ValueError` naming the server + and the variable, rather than dropping the header — a server may answer an + unauthenticated request as an anonymous user instead of returning 401, so + silently dropping would fail open. `McpManager.load_all` isolates a failing + server, so this shows up as one broken entry in `/mcp/status`. +- **Resolution happens at transport open** (`navi/mcp/client.py`), next to the + project-relative path resolution. This is what keeps every stored and + serialised config placeholder-only: `save_mcp_servers` — reached by + `create_mcp_server` and by `PUT /admin/mcp/config` — cannot write a secret + back into a tracked file, and a GET→PUT round-trip is lossless. + +A user key (`user_key`) is injected first and overwrites the header, so a +per-user credential simply replaces the placeholder — no substitution happens. + ## Runtime - `McpManager` (`navi/mcp/manager.py`) holds one `McpClient` per server, @@ -109,5 +142,5 @@ The webclient lists all of them: rows for servers without a `user_key` slot are read-only context, and only slotted servers get a key input. Servers that declare an `Authorization`-style header (today `gnexus-creds`) carry a shared -default in the config, which is what a user without a personal key falls back -to. \ No newline at end of file +default in the config — as a `${...}` placeholder, not a literal (see +[Secrets](#secrets)) — which is what a user without a personal key falls back to. \ No newline at end of file diff --git a/manuals/create_mcp_server.md b/manuals/create_mcp_server.md index 6f873d3..623c43d 100644 --- a/manuals/create_mcp_server.md +++ b/manuals/create_mcp_server.md @@ -178,6 +178,14 @@ **Write `command` and `cwd` relative to the project root, as `./mcp-servers//...`.** This file is tracked in git: an absolute path would name the machine you happened to write it on, and the server would then fail to start anywhere else with `[Errno 2] No such file or directory`. Relative paths are resolved against the project root — the directory holding `mcp_servers.d/` — when the server is connected. Absolute paths still work, so an existing config is not broken; new ones should be relative. In `args` and `env`, a value is resolved the same way only when it starts with `./` or `../` (those fields also carry flags and URLs, which must be left alone). +**Never put a live credential in this file.** It is tracked in git, so a token written here is in history for good, and `GET /admin/mcp/config` hands the same value to the admin client. Put a placeholder in the config and the value in `.env` (chmod 600, untracked — the canonical store is gnexus-creds): + +```json +"headers": { "Authorization": "Bearer ${NAVI_MCP_MY_SERVER_TOKEN}" } +``` + +`${VAR}` is substituted in `headers` and `env` values when the server is connected; the environment wins over `.env`, a blank value counts as missing, and a missing one refuses the connection instead of dropping the header. See `docs/mcp.md#secrets`, and add the variable name to `deploy/env.template` and `.env.example`. + After editing `mcp_servers.d/.json`, call `reload_tools` to connect the server and register its tools. ## 6. Testing diff --git a/mcp_servers.d/gnexus-creds.json b/mcp_servers.d/gnexus-creds.json index 5ed950d..d7adfeb 100644 --- a/mcp_servers.d/gnexus-creds.json +++ b/mcp_servers.d/gnexus-creds.json @@ -3,7 +3,7 @@ "url": "https://creds.gnexus.space/mcp-protocol/", "user_key": { "header": "Authorization", "prefix": "Bearer " }, "headers": { - "Authorization": "Bearer gcr_68df12db3e7639da_cm2qvpXfRcut11NnB0VSBxjzXXaqyza5aN_42iSP3tk" + "Authorization": "Bearer ${NAVI_MCP_GNEXUS_CREDS_TOKEN}" }, "groups": { "read": [ diff --git a/mcp_servers.d/gntodo.json b/mcp_servers.d/gntodo.json index 998e11b..82186a2 100644 --- a/mcp_servers.d/gntodo.json +++ b/mcp_servers.d/gntodo.json @@ -3,7 +3,7 @@ "url": "https://tasks.gnexus.space/mcp/", "user_key": { "header": "Authorization", "prefix": "Bearer " }, "headers": { - "Authorization": "Bearer gnt_0zEyloBvV-OAd-q1T4oEgnXc4MGYxI9hw-DlEMGExww" + "Authorization": "Bearer ${NAVI_MCP_GNTODO_TOKEN}" }, "groups": { "read": [ diff --git a/mcp_servers.d/hard-panel.json b/mcp_servers.d/hard-panel.json index e31e793..8149f91 100644 --- a/mcp_servers.d/hard-panel.json +++ b/mcp_servers.d/hard-panel.json @@ -3,7 +3,7 @@ "url": "http://192.168.1.176:8000/mcp", "user_key": { "header": "Authorization", "prefix": "Bearer " }, "headers": { - "Authorization": "Bearer mcp_NFgXQg4IF9V7rQhSxLBrupt3lAsFXJQy", + "Authorization": "Bearer ${NAVI_MCP_HARD_PANEL_TOKEN}", "Host": "localhost:8000" }, "groups": { diff --git a/mcp_servers.d/synapse.json b/mcp_servers.d/synapse.json index bda81a3..e24be19 100644 --- a/mcp_servers.d/synapse.json +++ b/mcp_servers.d/synapse.json @@ -3,7 +3,7 @@ "url": "http://192.168.1.172:8013/mcp", "user_key": { "header": "Authorization", "prefix": "Bearer " }, "headers": { - "Authorization": "Bearer mcp_a09d3138c392dfb18ed07d0e9d13b85992a66b1f38b4947f8b42e952608e6da3", + "Authorization": "Bearer ${NAVI_MCP_SYNAPSE_TOKEN}", "Host": "localhost:8013" }, "groups": { diff --git a/mcp_servers.d/tgclient.json b/mcp_servers.d/tgclient.json index 229d0ff..30d95a5 100644 --- a/mcp_servers.d/tgclient.json +++ b/mcp_servers.d/tgclient.json @@ -3,7 +3,7 @@ "url": "https://tgclientmcp.gnexus.space/mcp", "user_key": { "header": "Authorization", "prefix": "Bearer " }, "headers": { - "Authorization": "Bearer mcp_HgTIltQ5U4hZPqvWVEm7WWr0NvFzVdNS" + "Authorization": "Bearer ${NAVI_MCP_TGCLIENT_TOKEN}" }, "groups": { "read": [ diff --git a/navi/core/subagent_runner.py b/navi/core/subagent_runner.py index 10b2f30..b2b6593 100644 --- a/navi/core/subagent_runner.py +++ b/navi/core/subagent_runner.py @@ -36,6 +36,22 @@ _SUBAGENT_THINKING_STALL_SECONDS = 60.0 _SUBAGENT_THINKING_STALL_CHARS = 12_000 +# Always injected, whatever profile or custom prompt the caller supplied. A +# sub-agent is handed exactly one step and sees a tool set drawn from its +# profile's MCP servers — including, before this anchor, a task board, a secret +# store and a chat client. Given those, a sub-agent asked to echo one word went +# off to pick a project off the board and research it for forty iterations +# instead. The rule belongs to the harness rather than to any one persona: the +# two profiles without a `subagent_system_prompt.txt` fall back to their main +# prompt, so a per-file edit would reach only some sub-agents and drift. +_SCOPE_ANCHOR = ( + "[Scope]\n" + "Your task is exactly the user message below — nothing else. Do not go " + "looking for additional work in task boards, memory, chat history or any " + "other source, and do not widen the task. If the task looks trivial, just " + "do it and report back." +) + @dataclass(frozen=True) class SubAgentOutcome: @@ -170,6 +186,7 @@ sys_parts.append(profile.subagent_system_prompt) if custom_system_prompt: sys_parts.append(custom_system_prompt) + sys_parts.append(_SCOPE_ANCHOR) if briefing: sys_parts.append(f"## Task context\n\n{briefing}") if parent_session_id: diff --git a/navi/mcp/client.py b/navi/mcp/client.py index ee237f2..a39b435 100644 --- a/navi/mcp/client.py +++ b/navi/mcp/client.py @@ -16,6 +16,7 @@ from mcp.types import Tool from .config import McpServerConfig, resolve_paths +from .secrets import resolve_secrets logger = logging.getLogger(__name__) @@ -184,9 +185,12 @@ async def open_transport() -> None: nonlocal session, instructions - # Project-relative paths from the config file become absolute here, - # so a config committed from another machine still runs on this one. - cfg = resolve_paths(self.config) + # Config values are made concrete here and nowhere else: project- + # relative paths become absolute, and `${VAR}` credentials are read + # from the environment. Keeping both at transport-open means the + # stored config — and anything that serialises it back to disk — + # never holds this machine's paths or a live secret. + cfg = resolve_paths(resolve_secrets(self.config, server=self.name)) try: if cfg.is_stdio: if not cfg.command: diff --git a/navi/mcp/secrets.py b/navi/mcp/secrets.py new file mode 100644 index 0000000..204177d --- /dev/null +++ b/navi/mcp/secrets.py @@ -0,0 +1,111 @@ +"""Runtime resolution of ``${VAR}`` placeholders in MCP credentials. + +A tracked config file must never carry a live credential: ``mcp_servers.d/*.json`` +lives in git, so a bearer token written there is in history for good. Instead the +file *names* the variable holding the value: + + "headers": {"Authorization": "Bearer ${NAVI_MCP_GNTODO_TOKEN}"} + +and the value lives in the service's ``.env`` (chmod 600, untracked) or in the +process environment — the platform's rule for runtime secrets. + +Resolution happens at transport-open time, next to the ``resolve_paths`` call +that already turns project-relative paths absolute. Keeping it there is what +makes the scheme safe: every stored or serialised config stays placeholder-only, +so ``save_mcp_servers`` — reached by ``create_mcp_server`` and +``PUT /admin/mcp/config`` — cannot write a secret back into a tracked file, and +``GET /admin/mcp/config`` returns the placeholder instead of the token. +""" + +from __future__ import annotations + +import os +import re +from collections.abc import Mapping +from pathlib import Path + +from dotenv import dotenv_values + +from navi.config import Settings + +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_]*)\}") + + +def env_values() -> dict[str, str]: + """Credential values: the process environment over the project's ``.env``. + + Precedence matches ``pydantic-settings`` (an environment variable beats the + file). Re-read on every call on purpose — a rotated token is picked up on the + next (re)connect without a restart. + """ + values: dict[str, str] = {} + env_file = Settings.model_config.get("env_file", ".env") + if env_file: + # A blank (`FOO=` in .env) is a missing credential, not an empty one. + for key, value in dotenv_values(Path(env_file)).items(): + if value: + values[key] = value + values.update(os.environ) + return values + + +def resolve_secrets( + cfg: McpServerConfig, + values: Mapping[str, str] | None = None, + server: str | None = None, +) -> McpServerConfig: + """Return a deep copy of *cfg* with ``${VAR}`` resolved in header/env values. + + Only ``headers`` and ``env`` are scanned: those are the credential-bearing + fields (``env`` is where a stdio server's key would live, as ``user_key``'s + env branch implies). + + Raises: + ValueError: a placeholder names a variable that is not set. We refuse to + connect rather than drop the key — a server may answer an + unauthenticated request as an anonymous user instead of returning + 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. + """ + if not cfg.headers and not cfg.env: + return cfg + + pool = dict(values) if values is not None else env_values() + resolved = cfg.model_copy(deep=True) + where = f"MCP server '{server}'" if server else "MCP config" + + for field_name in ("headers", "env"): + block = getattr(resolved, field_name) + if not block: + continue + setattr( + resolved, + field_name, + { + key: _substitute(value, pool, f"{where} {field_name}.{key}") + for key, value in block.items() + }, + ) + return resolved + + +def _substitute(value: object, pool: Mapping[str, str], where: str) -> object: + if not isinstance(value, str): + return value + + 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] + + return _PLACEHOLDER.sub(replace, value) diff --git a/navi/profiles/developer/config.json b/navi/profiles/developer/config.json index 6a9836b..1b41d66 100644 --- a/navi/profiles/developer/config.json +++ b/navi/profiles/developer/config.json @@ -105,17 +105,9 @@ "browse", "request" ], - "gntodo": [ - "read", - "write" - ], "gnexus-book": [ "read", "write" - ], - "gnexus-creds": [ - "read", - "write" ] } } diff --git a/navi/profiles/discuss/config.json b/navi/profiles/discuss/config.json index 111b88c..2aad51a 100644 --- a/navi/profiles/discuss/config.json +++ b/navi/profiles/discuss/config.json @@ -70,17 +70,9 @@ "subagent": { "native": [], "mcp": { - "gntodo": [ - "read", - "write" - ], "gnexus-book": [ "read", "write" - ], - "gnexus-creds": [ - "read", - "write" ] } } diff --git a/navi/profiles/modeler_3d/config.json b/navi/profiles/modeler_3d/config.json index 52052d0..c472233 100644 --- a/navi/profiles/modeler_3d/config.json +++ b/navi/profiles/modeler_3d/config.json @@ -102,21 +102,9 @@ "browse", "request" ], - "gntodo": [ - "read", - "write" - ], "gnexus-book": [ "read", "write" - ], - "gnexus-creds": [ - "read", - "write" - ], - "tgclient": [ - "read", - "write" ] } } diff --git a/navi/profiles/navi_code/config.json b/navi/profiles/navi_code/config.json index b8bdd5d..d6d65db 100644 --- a/navi/profiles/navi_code/config.json +++ b/navi/profiles/navi_code/config.json @@ -103,21 +103,9 @@ "browse", "request" ], - "gntodo": [ - "read", - "write" - ], "gnexus-book": [ "read", "write" - ], - "gnexus-creds": [ - "read", - "write" - ], - "tgclient": [ - "read", - "write" ] } } diff --git a/navi/profiles/secretary/config.json b/navi/profiles/secretary/config.json index 571a041..7a6697e 100644 --- a/navi/profiles/secretary/config.json +++ b/navi/profiles/secretary/config.json @@ -109,24 +109,9 @@ "navi_ui": [ "ui" ], - "gntodo": [ - "read", - "write" - ], "gnexus-book": [ "read", "write" - ], - "gnexus-creds": [ - "read", - "write" - ], - "tgclient": [ - "read", - "write" - ], - "synapse": [ - "read" ] } } diff --git a/navi/profiles/server_admin/config.json b/navi/profiles/server_admin/config.json index 8d7b79d..8ebf54e 100644 --- a/navi/profiles/server_admin/config.json +++ b/navi/profiles/server_admin/config.json @@ -111,30 +111,13 @@ "read", "write" ], - "gnexus-creds": [ - "read", - "write" - ], "navi-web": [ "search", "browse", "request" ], - "gntodo": [ - "read", - "write" - ], "hard-panel": [ "read" - ], - "tgclient": [ - "read", - "write" - ], - "synapse": [ - "read", - "write", - "admin" ] } } diff --git a/tests/_mcp_test_server.py b/tests/_mcp_test_server.py index ef877f7..07c9c92 100644 --- a/tests/_mcp_test_server.py +++ b/tests/_mcp_test_server.py @@ -1,5 +1,7 @@ """Minimal MCP server used only by integration tests.""" +import os + from mcp.server.fastmcp import FastMCP mcp = FastMCP("test-server") @@ -17,5 +19,11 @@ return a + b +@mcp.tool() +def echo_env(name: str) -> str: + """Return the value of an environment variable this server was launched with.""" + return os.environ.get(name, "") + + if __name__ == "__main__": mcp.run(transport="stdio") diff --git a/tests/integration/test_mcp_integration.py b/tests/integration/test_mcp_integration.py index ac4398c..3c0217b 100644 --- a/tests/integration/test_mcp_integration.py +++ b/tests/integration/test_mcp_integration.py @@ -70,3 +70,45 @@ # raise the cross-task cancel-scope RuntimeError. await asyncio.ensure_future(_disconnect()) assert not client.connected + + +def _env_server(env: dict[str, str]) -> McpServerConfig: + return McpServerConfig( + transport="stdio", + command=sys.executable, + args=["-m", "tests._mcp_test_server"], + env=env, + ) + + +@pytest.mark.anyio +async def test_credentials_are_resolved_at_connect(monkeypatch): + """`${VAR}` in a config reaches the server as the environment's value. + + Resolution happens at transport open (`navi/mcp/client.py`), which is what + lets the config object itself keep the placeholder — and therefore what + stops `save_mcp_servers` from writing a credential into a tracked file. + """ + monkeypatch.setenv("NAVI_TEST_INTEGRATION_TOKEN", "resolved-value") + cfg = _env_server({"TOKEN": "${NAVI_TEST_INTEGRATION_TOKEN}"}) + client = McpClient("test", cfg) + await client.connect() + try: + result = await client.call_tool("echo_env", {"name": "TOKEN"}) + assert "resolved-value" in result + assert client.config.env["TOKEN"] == "${NAVI_TEST_INTEGRATION_TOKEN}" + finally: + await client.disconnect() + + +@pytest.mark.anyio +async def test_a_missing_credential_fails_the_connection(monkeypatch): + """The connection is refused, not made without the header: a server may + answer an unauthenticated request as an anonymous user instead of 401.""" + monkeypatch.delenv("NAVI_TEST_DEFINITELY_UNSET", raising=False) + client = McpClient("test", _env_server({"TOKEN": "${NAVI_TEST_DEFINITELY_UNSET}"})) + + with pytest.raises(ValueError) as excinfo: + await client.connect() + assert "NAVI_TEST_DEFINITELY_UNSET" in str(excinfo.value) + assert not client.connected diff --git a/tests/unit/mcp/test_no_literal_secrets.py b/tests/unit/mcp/test_no_literal_secrets.py new file mode 100644 index 0000000..bd1bafa --- /dev/null +++ b/tests/unit/mcp/test_no_literal_secrets.py @@ -0,0 +1,74 @@ +"""The tracked MCP configs must never carry a live credential. + +``mcp_servers.d/*.json`` is committed, so a token written there is in git +history for good — and ``GET /admin/mcp/config`` hands the same value to the +admin client. A config therefore *names* the variable (``Bearer ${NAVI_MCP_X_TOKEN}``) +and the value lives in the service ``.env``; see ``docs/mcp.md``. + +This is the guard on that invariant: it is what fails when someone pastes a +token back into a config file, by hand or through ``PUT /admin/mcp/config``. +""" + +import json +import re +from pathlib import Path + +import navi.mcp.config as mcp_config +from navi.tools._internal.redact import is_sensitive_key + +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_]*)\}") + +# The credential-bearing servers, and the variable each one names. Keep in step +# with `deploy/env.template`. +EXPECTED_VARIABLES = { + "gnexus-creds": "NAVI_MCP_GNEXUS_CREDS_TOKEN", + "gntodo": "NAVI_MCP_GNTODO_TOKEN", + "hard-panel": "NAVI_MCP_HARD_PANEL_TOKEN", + "synapse": "NAVI_MCP_SYNAPSE_TOKEN", + "tgclient": "NAVI_MCP_TGCLIENT_TOKEN", +} + + +def _configs() -> dict[str, dict]: + return { + path.stem: json.loads(path.read_text(encoding="utf-8")) + for path in sorted(CONFIG_DIR.glob("*.json")) + } + + +def test_every_sensitive_header_or_env_value_is_a_placeholder(): + offenders = [ + f"{server}: {field}.{key}" + for server, cfg in _configs().items() + for field in ("headers", "env") + for key, value in (cfg.get(field) or {}).items() + if is_sensitive_key(key) and "${" not in str(value) + ] + assert not offenders, ( + "literal credential in a tracked config — replace it with " + f"`${{NAVI_MCP__TOKEN}}` and put the value in .env: {offenders}" + ) + + +def test_placeholder_names_are_env_variables(): + 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)) + } + 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)}" + ) + + +def test_the_credential_servers_name_their_variable(): + configs = _configs() + for server, variable in EXPECTED_VARIABLES.items(): + assert server in configs, f"{server}.json is gone — update this test with it" + assert configs[server]["headers"]["Authorization"] == f"Bearer ${{{variable}}}" diff --git a/tests/unit/mcp/test_secrets.py b/tests/unit/mcp/test_secrets.py new file mode 100644 index 0000000..cb49ebd --- /dev/null +++ b/tests/unit/mcp/test_secrets.py @@ -0,0 +1,110 @@ +"""Unit tests for `${VAR}` credential resolution in MCP server configs.""" + +import pytest + +from navi.mcp.config import McpServerConfig +from navi.mcp.secrets import env_values, resolve_secrets + + +def _http(authorization: str) -> McpServerConfig: + return McpServerConfig( + transport="streamable_http", + url="https://creds.example.com/mcp", + headers={"Authorization": authorization}, + ) + + +class TestSubstitution: + def test_header_placeholder_is_resolved(self): + resolved = resolve_secrets( + _http("Bearer ${NAVI_TEST_TOKEN}"), {"NAVI_TEST_TOKEN": "s3cret"}, server="demo" + ) + assert resolved.headers["Authorization"] == "Bearer s3cret" + + def test_stdio_env_placeholder_is_resolved(self): + cfg = McpServerConfig( + transport="stdio", command="python", env={"API_KEY": "${NAVI_TEST_KEY}"} + ) + assert resolve_secrets(cfg, {"NAVI_TEST_KEY": "abc"}).env["API_KEY"] == "abc" + + def test_several_placeholders_in_one_value(self): + cfg = McpServerConfig(transport="stdio", command="python", env={"PAIR": "${A}:${B}"}) + assert resolve_secrets(cfg, {"A": "1", "B": "2"}).env["PAIR"] == "1:2" + + def test_bare_dollar_names_are_left_alone(self): + """Header values legitimately contain `$` (nginx-style) — only the + braced form opts in.""" + resolved = resolve_secrets(_http("Bearer $NAVI_TEST_TOKEN"), {"NAVI_TEST_TOKEN": "x"}) + assert resolved.headers["Authorization"] == "Bearer $NAVI_TEST_TOKEN" + + def test_only_headers_and_env_are_scanned(self): + cfg = McpServerConfig( + transport="streamable_http", url="https://${HOST}/mcp", command="${CMD}" + ) + resolved = resolve_secrets(cfg, {"HOST": "creds.example.com", "CMD": "python"}) + assert resolved.url == "https://${HOST}/mcp" + assert resolved.command == "${CMD}" + + def test_values_without_placeholders_are_untouched(self): + cfg = McpServerConfig( + transport="stdio", + command="python", + env={"FLAG": "a/b", "SESSION_FILES_DIR": "./session_files"}, + ) + assert resolve_secrets(cfg, {}).env == { + "FLAG": "a/b", + "SESSION_FILES_DIR": "./session_files", + } + + def test_does_not_mutate_the_original(self): + """`save_mcp_servers` writes configs back to the file: a resolved copy + leaking into it would bake the credential into a tracked file.""" + cfg = _http("Bearer ${NAVI_TEST_TOKEN}") + resolve_secrets(cfg, {"NAVI_TEST_TOKEN": "s3cret"}) + assert cfg.headers["Authorization"] == "Bearer ${NAVI_TEST_TOKEN}" + + def test_resolution_defaults_to_the_environment(self, monkeypatch): + monkeypatch.setenv("NAVI_TEST_FROM_PROCESS", "from-process") + resolved = resolve_secrets(_http("Bearer ${NAVI_TEST_FROM_PROCESS}")) + assert resolved.headers["Authorization"] == "Bearer from-process" + + +class TestMissingVariable: + """A missing value fails loudly instead of dropping the header: a server may + answer an unauthenticated request as an anonymous user rather than 401.""" + + def test_raises_naming_the_server_and_the_variable(self): + with pytest.raises(ValueError) as excinfo: + resolve_secrets( + _http("Bearer ${NAVI_MCP_GNTODO_TOKEN}"), {}, server="gntodo" + ) + message = str(excinfo.value) + assert "gntodo" in message + assert "NAVI_MCP_GNTODO_TOKEN" in message + + def test_names_the_field_that_references_it(self): + with pytest.raises(ValueError) as excinfo: + resolve_secrets(_http("${NAVI_TEST_MISSING}"), {}, server="demo") + assert "headers.Authorization" in str(excinfo.value) + + +class TestEnvValues: + def test_process_environment_beats_the_env_file(self, monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + (tmp_path / ".env").write_text("NAVI_TEST_PREC=from-file\n") + monkeypatch.setenv("NAVI_TEST_PREC", "from-env") + assert env_values()["NAVI_TEST_PREC"] == "from-env" + + def test_blank_value_in_the_env_file_is_treated_as_missing(self, monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + (tmp_path / ".env").write_text("NAVI_TEST_BLANK=\nNAVI_TEST_FILLED=ok\n") + monkeypatch.delenv("NAVI_TEST_BLANK", raising=False) + + values = env_values() + assert "NAVI_TEST_BLANK" not in values + assert values["NAVI_TEST_FILLED"] == "ok" + + def test_missing_env_file_falls_back_to_the_process_environment(self, monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) # no .env here + monkeypatch.setenv("NAVI_TEST_ONLY_ENV", "v") + assert env_values()["NAVI_TEST_ONLY_ENV"] == "v"