fix(channels): harden runtime credential management APIs (#3581)

* fix(channels): harden runtime credential management APIs

* fix(channels): address review feedback on credential hardening

Follow-up to the runtime credential-hardening pass, resolving five review
findings:

- WeChat auth persistence now writes through a 0o600 NamedTemporaryFile +
  Path.replace instead of write_text-then-chmod, so the iLink bot_token is
  never briefly readable at umask defaults (mirrors ChannelRuntimeConfigStore).
- The post-write chmod is split into its own try/except: a chmod failure on a
  filesystem without POSIX perms now logs at debug instead of masquerading as
  a "failed to persist" warning.
- Extracted the three near-identical _require_admin_user helpers (mcp,
  channel_connections, channels) into a single require_admin_user(request, *,
  detail) in app/gateway/deps.py; each router supplies its own detail string.
- Strengthened the runtime-config-store chmod coverage: a new test injects a
  temp-file chmod failure and asserts it is logged at debug while the
  destination is still owner-only (mutation-verified to fail if the chmod is
  dropped), plus a loose-pre-existing-file case.
- Removed the unused _FakeRepo from the blocking-io test: its isinstance gate
  routes through the repo-less 503 path, so neither stub was ever invoked.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>

backend/tests/blocking_io/test_channel_runtime_config_store.py

@@ -17,7 +17,10 @@ from __future__ import annotations
 
 import asyncio
 import importlib
+import logging
+from pathlib import Path
 from types import SimpleNamespace
+from unittest import mock
 from uuid import UUID
 
 import pytest
@@ -55,18 +58,16 @@ def _make_request(tmp_path) -> Request:
         }
     )
     app.state.channels_config = {}
-    app.state.channel_connection_repo = _FakeRepo()
+    # No channel_connection_repo is set: _get_repository's isinstance gate then
+    # falls through to get_session_factory() (None in tests) and the handlers
+    # take the repo-less 503 path. These tests only assert the store's file IO
+    # is offloaded off the event loop, so the DB repo is intentionally absent.
     store = ChannelRuntimeConfigStore(tmp_path / "channels" / "runtime-config.json")
     app.state.channel_runtime_config_store = store
     user = SimpleNamespace(id=UUID("11111111-2222-3333-4444-555555555555"), system_role="admin")
     return Request({"type": "http", "app": app, "headers": [], "state": {"user": user}})
 
 
-class _FakeRepo:
-    async def list_connections(self, owner_user_id):
-        return []
-
-
 async def test_configure_runtime_channel_does_not_block_event_loop(tmp_path) -> None:
     request = await asyncio.to_thread(_make_request, tmp_path)
 
@@ -104,3 +105,71 @@ async def test_disconnect_runtime_channel_does_not_block_event_loop(tmp_path) ->
         "enabled": False,
         "_runtime_disabled": True,
     }
+
+
+async def test_runtime_config_store_file_is_owner_only(tmp_path) -> None:
+    path = tmp_path / "channels" / "runtime-config.json"
+    store = await asyncio.to_thread(ChannelRuntimeConfigStore, path)
+
+    await asyncio.to_thread(
+        store.set_provider_config,
+        "slack",
+        {"enabled": True, "bot_token": "xoxb-ui", "app_token": "xapp-ui"},
+    )
+
+    mode = await asyncio.to_thread(lambda: path.stat().st_mode & 0o777)
+    assert mode == 0o600
+
+
+async def test_runtime_config_store_overwrites_loose_existing_file(tmp_path) -> None:
+    """A pre-existing world-readable file is tightened to 0o600 after a save.
+
+    ``NamedTemporaryFile`` would yield 0o600 on a fresh path regardless of the
+    code under test, so seed the destination at 0o644 first: only the store's
+    atomic 0o600-temp + replace path produces an owner-only file here.
+    """
+    path = tmp_path / "channels" / "runtime-config.json"
+    path.parent.mkdir(parents=True, exist_ok=True)
+    path.write_text("{}", encoding="utf-8")
+    path.chmod(0o644)
+
+    store = await asyncio.to_thread(ChannelRuntimeConfigStore, path)
+    await asyncio.to_thread(
+        store.set_provider_config,
+        "slack",
+        {"enabled": True, "bot_token": "xoxb-ui"},
+    )
+
+    mode = await asyncio.to_thread(lambda: path.stat().st_mode & 0o777)
+    assert mode == 0o600
+
+
+async def test_runtime_config_store_chmod_failure_is_logged_not_fatal(tmp_path, caplog) -> None:
+    """A chmod failure on the temp file is logged at debug and never aborts the save.
+
+    This is the line the previous owner-only assertion could not protect: with the
+    pre-rename chmod patched to raise, the save must still persist the secret and
+    the destination must still end up owner-only (via the temp file's mkstemp mode
+    that ``Path.replace`` preserves). If the chmod call were dropped, the expected
+    debug record would be absent and this test would fail.
+    """
+    path = tmp_path / "channels" / "runtime-config.json"
+    store = await asyncio.to_thread(ChannelRuntimeConfigStore, path)
+
+    real_chmod = Path.chmod
+
+    def chmod_spy(self: Path, mode: int, *args, **kwargs):
+        if self.suffix == ".tmp":
+            raise OSError("chmod unsupported on this filesystem")
+        return real_chmod(self, mode, *args, **kwargs)
+
+    def _save_with_failing_temp_chmod() -> None:
+        with caplog.at_level(logging.DEBUG, logger="app.channels.runtime_config_store"), mock.patch.object(Path, "chmod", chmod_spy):
+            store.set_provider_config("slack", {"enabled": True, "bot_token": "xoxb-ui"})
+
+    await asyncio.to_thread(_save_with_failing_temp_chmod)
+
+    assert any("Unable to chmod temporary channel runtime config store" in record.getMessage() for record in caplog.records)
+    mode = await asyncio.to_thread(lambda: path.stat().st_mode & 0o777)
+    assert mode == 0o600
+    assert await asyncio.to_thread(store.get_provider_config, "slack") == {"enabled": True, "bot_token": "xoxb-ui"}

backend/tests/test_channels_router.py

@@ -0,0 +1,86 @@
+"""Router tests for legacy IM channel management endpoints."""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+from unittest.mock import AsyncMock
+from uuid import UUID
+
+from _router_auth_helpers import make_authed_test_app
+from fastapi.testclient import TestClient
+
+from app.gateway.auth.models import User
+from app.gateway.routers import channels
+
+
+def _admin_user() -> User:
+    return User(
+        id=UUID("11111111-2222-3333-4444-555555555555"),
+        email="admin@example.com",
+        password_hash="x",
+        system_role="admin",
+    )
+
+
+def _non_admin_user() -> User:
+    return User(
+        id=UUID("99999999-8888-7777-6666-555555555555"),
+        email="user@example.com",
+        password_hash="x",
+        system_role="user",
+    )
+
+
+def test_restart_channel_requires_admin(monkeypatch):
+    service = SimpleNamespace(restart_channel=AsyncMock(return_value=True))
+    monkeypatch.setattr("app.channels.service.get_channel_service", lambda: service)
+    app = make_authed_test_app(user_factory=_non_admin_user)
+    app.include_router(channels.router)
+
+    with TestClient(app) as client:
+        response = client.post("/api/channels/slack/restart")
+
+    assert response.status_code == 403
+    assert "Admin privileges" in response.json()["detail"]
+    service.restart_channel.assert_not_awaited()
+
+
+def test_restart_channel_allows_admin(monkeypatch):
+    service = SimpleNamespace(restart_channel=AsyncMock(return_value=True))
+    monkeypatch.setattr("app.channels.service.get_channel_service", lambda: service)
+    app = make_authed_test_app(user_factory=_admin_user)
+    app.include_router(channels.router)
+
+    with TestClient(app) as client:
+        response = client.post("/api/channels/slack/restart")
+
+    assert response.status_code == 200
+    assert response.json() == {
+        "success": True,
+        "message": "Channel slack restarted successfully",
+    }
+    service.restart_channel.assert_awaited_once_with("slack")
+
+
+def test_get_channels_status_remains_read_only(monkeypatch):
+    service = SimpleNamespace(
+        get_status=lambda: {
+            "service_running": True,
+            "channels": {
+                "slack": {
+                    "enabled": True,
+                    "running": True,
+                }
+            },
+        }
+    )
+    monkeypatch.setattr("app.channels.service.get_channel_service", lambda: service)
+    app = make_authed_test_app(user_factory=_non_admin_user)
+    app.include_router(channels.router)
+
+    with TestClient(app) as client:
+        response = client.get("/api/channels/")
+
+    assert response.status_code == 200
+    assert response.json()["service_running"] is True
+    assert response.json()["channels"]["slack"]["running"] is True

backend/tests/test_mcp_config_secrets.py

@@ -12,15 +12,16 @@ from types import SimpleNamespace
 import pytest
 from fastapi import HTTPException
 
+from app.gateway.deps import require_admin_user
 from app.gateway.routers import mcp as mcp_router
 from app.gateway.routers.mcp import (
+    _ADMIN_REQUIRED_DETAIL,
     _MCP_STDIO_COMMAND_ALLOWLIST_ENV,
     McpConfigUpdateRequest,
     McpOAuthConfigResponse,
     McpServerConfigResponse,
     _mask_server_config,
     _merge_preserving_secrets,
-    _require_admin_user,
     _validate_mcp_update_request,
     reset_mcp_tools_cache_endpoint,
     update_mcp_configuration,
@@ -334,10 +335,10 @@ def _request_with_role(system_role: str):
 @pytest.mark.asyncio
 async def test_mcp_config_requires_admin_user():
     """MCP config is system-level executable configuration, not a normal user setting."""
-    await _require_admin_user(_request_with_role("admin"))
+    await require_admin_user(_request_with_role("admin"), detail=_ADMIN_REQUIRED_DETAIL)
 
     with pytest.raises(HTTPException) as exc_info:
-        await _require_admin_user(_request_with_role("user"))
+        await require_admin_user(_request_with_role("user"), detail=_ADMIN_REQUIRED_DETAIL)
 
     assert exc_info.value.status_code == 403
 

backend/tests/test_wechat_channel.py

@@ -5,8 +5,10 @@ from __future__ import annotations
 import asyncio
 import base64
 import json
+import logging
 from pathlib import Path
 from typing import Any
+from unittest import mock
 from unittest.mock import AsyncMock
 
 from app.channels.message_bus import InboundMessageType, MessageBus, OutboundMessage
@@ -1310,5 +1312,67 @@ def test_qrcode_login_binds_and_persists_auth_state(monkeypatch, tmp_path: Path)
         assert auth_state["status"] == "confirmed"
         assert auth_state["bot_token"] == "bound-token"
         assert auth_state["ilink_bot_id"] == "bot-99"
+        assert ((state_dir / "wechat-auth.json").stat().st_mode & 0o777) == 0o600
 
     _run(go())
+
+
+def test_save_auth_state_tightens_preexisting_loose_file(tmp_path: Path):
+    """A world-readable auth file is replaced by an owner-only one, atomically.
+
+    The bot_token must never be observable at loose permissions: the atomic
+    0o600-temp + ``Path.replace`` path swaps in a fresh owner-only inode rather
+    than truncating the existing 0o644 file in place. Seeding the destination at
+    0o644 first means a regression back to ``write_text`` + late ``chmod`` would
+    leave a detectable window (and, here, the temp-file artifact behind).
+    """
+    from app.channels.wechat import WechatChannel
+
+    state_dir = tmp_path / "wechat-state"
+    state_dir.mkdir(parents=True, exist_ok=True)
+    auth_path = state_dir / "wechat-auth.json"
+    auth_path.write_text(json.dumps({"status": "pending"}), encoding="utf-8")
+    auth_path.chmod(0o644)
+
+    channel = WechatChannel(
+        bus=MessageBus(),
+        config={"state_dir": str(state_dir), "qrcode_login_enabled": True},
+    )
+    channel._save_auth_state(status="confirmed", bot_token="bound-token", ilink_bot_id="bot-1")
+
+    assert (auth_path.stat().st_mode & 0o777) == 0o600
+    assert json.loads(auth_path.read_text(encoding="utf-8"))["bot_token"] == "bound-token"
+    # Atomic write leaves no temp-file residue behind.
+    assert list(state_dir.glob("*.tmp")) == []
+
+
+def test_save_auth_state_chmod_failure_is_logged_not_warned(tmp_path: Path, caplog):
+    """A chmod failure on a perms-less filesystem must not look like a persist failure.
+
+    With the post-replace chmod split into its own try/except, a chmod ``OSError``
+    is logged at debug while the JSON is genuinely on disk — operators must not see
+    the misleading ``failed to persist`` warning that the shared try/except produced.
+    """
+    from app.channels.wechat import WechatChannel
+
+    state_dir = tmp_path / "wechat-state"
+    channel = WechatChannel(
+        bus=MessageBus(),
+        config={"state_dir": str(state_dir), "qrcode_login_enabled": True},
+    )
+
+    real_chmod = Path.chmod
+
+    def chmod_spy(self: Path, mode: int, *args, **kwargs):
+        if self.suffix == ".json":
+            raise OSError("chmod unsupported on this filesystem")
+        return real_chmod(self, mode, *args, **kwargs)
+
+    with caplog.at_level(logging.DEBUG, logger="app.channels.wechat"), mock.patch.object(Path, "chmod", chmod_spy):
+        channel._save_auth_state(status="confirmed", bot_token="bound-token")
+
+    auth_path = state_dir / "wechat-auth.json"
+    assert json.loads(auth_path.read_text(encoding="utf-8"))["bot_token"] == "bound-token"
+    messages = [record.getMessage() for record in caplog.records]
+    assert any("unable to chmod auth state" in message for message in messages)
+    assert not any("failed to persist auth state" in message for message in messages)