fix(mcp): feed dashboard-supplied env into catalog install without re-prompt
The dashboard route pre-wrote only secrets to .env and then called
install_entry, which prompted again for every auth.env var on a non-TTY
server stdin — the user's supplied non-secret value was read as EOF,
discarded, and config.yaml ended up with an unresolved ${VAR} ref
(asana: silent OAuth break; n8n: hard 400 on the required URL).
install_entry now accepts preloaded_env (dashboard form values) and
_prompt_env_vars skips the prompt for any spec already supplied, so the
route passes body.env straight through and the non-secret inline path
works end-to-end. The boundary test now exercises the real install_entry
(dashboard route, no mock) and asserts the non-secret lands in the server
config; asana manifest comments updated to match the inline behavior.
This commit is contained in:
@@ -455,15 +455,21 @@ def _expand_install_dir(value: str, install_dir: Optional[Path]) -> str:
|
||||
return value.replace(_INSTALL_DIR_VAR, str(install_dir))
|
||||
|
||||
|
||||
def _prompt_env_vars(specs: List[EnvVarSpec]) -> Dict[str, str]:
|
||||
def _prompt_env_vars(specs: List[EnvVarSpec], preloaded: Optional[Dict[str, str]] = None) -> Dict[str, str]:
|
||||
"""Prompt for each env spec.
|
||||
|
||||
Secrets persist to ~/.hermes/.env. Non-secrets are only collected and
|
||||
returned — the caller inlines them into the server config (config.yaml),
|
||||
since .env is secrets-only.
|
||||
since .env is secrets-only. Values already supplied by the caller
|
||||
(``preloaded``, e.g. from a dashboard form) skip the prompt.
|
||||
"""
|
||||
preloaded = preloaded or {}
|
||||
collected: Dict[str, str] = {}
|
||||
for spec in specs:
|
||||
pre = preloaded.get(spec.name)
|
||||
if pre is not None:
|
||||
collected[spec.name] = pre
|
||||
continue
|
||||
existing = get_env_value(spec.name) if spec.secret else None
|
||||
if existing:
|
||||
_say(f" ✓ {spec.name} already set in .env")
|
||||
@@ -703,12 +709,15 @@ def card_install_config(entry: CatalogEntry) -> dict:
|
||||
return cfg
|
||||
|
||||
|
||||
def install_entry(entry: CatalogEntry, *, enable: bool = True) -> None:
|
||||
def install_entry(entry: CatalogEntry, *, enable: bool = True, preloaded_env: Optional[Dict[str, str]] = None) -> None:
|
||||
"""Install a catalog entry end-to-end.
|
||||
|
||||
Order: git clone + bootstrap (if any); credential prompts (``auth.env``) to .env; write
|
||||
``mcp_servers.<name>`` (with the ``auth: oauth`` marker and any pre-registered ``oauth`` block); probe + tool checklist (falling back per
|
||||
:func:`_apply_tool_selection`); print post_install notes.
|
||||
|
||||
``preloaded_env`` carries env values already supplied by the caller (e.g.
|
||||
the dashboard form); they skip the interactive prompt.
|
||||
"""
|
||||
print()
|
||||
_say(f" Installing MCP '{entry.name}'", Colors.CYAN + Colors.BOLD)
|
||||
@@ -724,7 +733,7 @@ def install_entry(entry: CatalogEntry, *, enable: bool = True) -> None:
|
||||
if entry.auth.env:
|
||||
print()
|
||||
_say(" Configure credentials:", Colors.CYAN)
|
||||
env_values = _prompt_env_vars(entry.auth.env)
|
||||
env_values = _prompt_env_vars(entry.auth.env, preloaded_env or {})
|
||||
if entry.auth.type == "oauth" and entry.auth.provider:
|
||||
# Provider-mediated OAuth relies on the existing `hermes auth <provider>` flow; surface
|
||||
# guidance rather than auto-running it to keep install decoupled from provider-auth lifecycle.
|
||||
|
||||
@@ -470,11 +470,12 @@ async def install_mcp_catalog_entry(body: MCPCatalogInstall, profile: Optional[s
|
||||
raise HTTPException(status_code=400, detail=str(exc)) from exc
|
||||
|
||||
effective_profile = body.profile or profile
|
||||
# Secrets are persisted to .env here (before install_entry runs); non-secret
|
||||
# values ride preloaded_env into install_entry → config.yaml, so .env stays
|
||||
# secrets-only and nothing re-prompts on a non-TTY server.
|
||||
if body.env:
|
||||
def _write_env():
|
||||
with _profile_scope(effective_profile):
|
||||
# Only declared secrets go to .env; non-secret values ride in the
|
||||
# server config (inlined by install_entry) so .env stays secrets-only.
|
||||
for spec in entry.auth.env or []:
|
||||
value = (body.env or {}).get(spec.name)
|
||||
if spec.secret and value:
|
||||
@@ -497,7 +498,10 @@ async def install_mcp_catalog_entry(body: MCPCatalogInstall, profile: Optional[s
|
||||
# No git step — install synchronously; install_entry goes through the
|
||||
# call-time config/env resolvers so the profile scope covers it.
|
||||
try:
|
||||
await scoped_to_thread(effective_profile, lambda: mcp_catalog.install_entry(entry, enable=body.enable))
|
||||
await scoped_to_thread(
|
||||
effective_profile,
|
||||
lambda: mcp_catalog.install_entry(entry, enable=body.enable, preloaded_env=body.env or None),
|
||||
)
|
||||
except HTTPException:
|
||||
raise
|
||||
except Exception as exc:
|
||||
|
||||
@@ -20,8 +20,9 @@ transport:
|
||||
|
||||
auth:
|
||||
type: oauth
|
||||
# Prompted at install (CLI, dashboard, desktop) and stored in the profile's
|
||||
# .env — config.yaml only ever carries the ${VAR} references below.
|
||||
# Prompted at install (CLI, dashboard, desktop). Secrets are stored in the
|
||||
# profile's .env; non-secret values (client id below) are inlined into
|
||||
# config.yaml — config.yaml only ever carries ${VAR} references for secrets.
|
||||
env:
|
||||
- name: ASANA_CLIENT_ID
|
||||
prompt: "Asana MCP app Client ID (developer console → your MCP app → OAuth)"
|
||||
@@ -50,7 +51,8 @@ post_install: |
|
||||
- OAuth → Redirect URL: http://localhost:27890/callback (exactly).
|
||||
- Manage distribution → allow the workspace(s) you will use.
|
||||
Its Client ID / Client secret are the ASANA_CLIENT_ID / ASANA_CLIENT_SECRET
|
||||
values prompted above (stored in the profile's .env, never in config.yaml).
|
||||
values prompted above. The secret is stored in the profile's .env; the
|
||||
non-secret client id is inlined into config.yaml.
|
||||
|
||||
Then run `hermes mcp login asana` (or click Authorize in the dashboard /
|
||||
Desktop: the callback still arrives at http://localhost:27890/callback, so
|
||||
|
||||
@@ -71,7 +71,7 @@ def test_catalog_rejects_undeclared_key_before_any_write_or_install(
|
||||
monkeypatch.setattr(
|
||||
mcp_catalog,
|
||||
"install_entry",
|
||||
lambda entry, enable=True: installs.append(entry.name),
|
||||
lambda entry, enable=True, preloaded_env=None: installs.append(entry.name),
|
||||
)
|
||||
|
||||
response = client.post(
|
||||
@@ -119,7 +119,7 @@ def test_catalog_cannot_declare_reserved_control_key(
|
||||
monkeypatch.setattr(
|
||||
mcp_catalog,
|
||||
"install_entry",
|
||||
lambda entry, enable=True: installs.append(entry.name),
|
||||
lambda entry, enable=True, preloaded_env=None: installs.append(entry.name),
|
||||
)
|
||||
|
||||
response = client.post(
|
||||
@@ -147,6 +147,12 @@ def test_catalog_accepts_declared_credential(
|
||||
from tools.connectors.mcp import _CatalogBackend
|
||||
|
||||
probes: list[str] = []
|
||||
installs: list[str] = []
|
||||
monkeypatch.setattr(
|
||||
mcp_catalog,
|
||||
"install_entry",
|
||||
lambda entry, enable=True, preloaded_env=None: installs.append(entry.name),
|
||||
)
|
||||
|
||||
def probe(name, cfg, **_kwargs):
|
||||
# The credential is in scope for the probe, and nothing is saved before it answers.
|
||||
@@ -187,13 +193,21 @@ def test_catalog_non_secret_env_never_lands_in_env_file(
|
||||
"secret": False,
|
||||
}
|
||||
)
|
||||
# The transport references the non-secret var; install_entry inlines it.
|
||||
# (HTTP transport so the var lands in the server url.)
|
||||
manifest["transport"] = {"type": "http", "url": "${DEMO_BASE_URL}"}
|
||||
manifest["auth"]["type"] = "api_key"
|
||||
manifest["auth"]["env"] = [
|
||||
{"name": "MCP_DEMO_API_KEY", "prompt": "Demo API key", "secret": True},
|
||||
{"name": "DEMO_BASE_URL", "prompt": "Demo base URL", "secret": False},
|
||||
]
|
||||
manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8")
|
||||
|
||||
installs: list[str] = []
|
||||
# The real install_entry probes the server after writing config; avoid
|
||||
# launching a nonexistent binary in tests.
|
||||
monkeypatch.setattr(
|
||||
mcp_catalog,
|
||||
"install_entry",
|
||||
lambda entry, enable=True: installs.append(entry.name),
|
||||
"_probe_tools",
|
||||
lambda name: None,
|
||||
)
|
||||
|
||||
response = client.post(
|
||||
@@ -202,18 +216,26 @@ def test_catalog_non_secret_env_never_lands_in_env_file(
|
||||
json={
|
||||
"name": "demo",
|
||||
"env": {
|
||||
"DEMO_API_KEY": "valid-demo-value",
|
||||
"MCP_DEMO_API_KEY": "valid-demo-value",
|
||||
"DEMO_BASE_URL": "https://demo.example.test",
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert installs == ["demo"]
|
||||
env_text = (catalog_env / ".env").read_text(encoding="utf-8")
|
||||
assert "DEMO_API_KEY=valid-demo-value" in env_text
|
||||
assert "MCP_DEMO_API_KEY=valid-demo-value" in env_text
|
||||
assert "DEMO_BASE_URL" not in env_text
|
||||
assert "https://demo.example.test" not in env_text
|
||||
# The non-secret is inlined into config.yaml (server config carries the
|
||||
# literal; the raw file never stores it and never keeps a ${VAR} ref).
|
||||
from hermes_cli.config import load_config
|
||||
|
||||
server = load_config()["mcp_servers"]["demo"]
|
||||
assert server["url"] == "https://demo.example.test"
|
||||
assert "${DEMO_BASE_URL}" not in (
|
||||
catalog_env / "config.yaml"
|
||||
).read_text(encoding="utf-8")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
|
||||
Reference in New Issue
Block a user