diff --git a/.gitignore b/.gitignore index 63fc033..2886a27 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ __pycache__/ dist/ *.egg-info/ .bridge-tmp/ +.omc/ diff --git a/CHANGELOG.md b/CHANGELOG.md index cbd4db6..3d1f070 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,13 @@ Versioning and uses Git tags as its release channel. ## [Unreleased] +- Security: a `bridge.toml` found by upward discovery can no longer allow + workspaces outside its own directory, and on POSIX must be owned by the + current user and not world-writable. Previously a cloned repository could + commit one that replaced the user's policy with `allowed_roots = ["/"]`. +- Security: `allowed_roots` are resolved once at startup, so a symlink created + after startup can no longer redirect a root outside the configured path. + ## [0.1.1] - 2026-09-14 - Fixed `doctor` incorrectly treating a normal Codex project trust entry as a diff --git a/README.ja.md b/README.ja.md index 8596a5c..c1881fc 100644 --- a/README.ja.md +++ b/README.ja.md @@ -106,6 +106,11 @@ format = "json" 3. `$XDG_CONFIG_HOME/codex-app-mcp/bridge.toml`などのユーザー設定 4. fail-closedな組み込み既定値 +親方向の探索で見つかった`bridge.toml`はクローンしたrepositoryに含まれている場合が +あるため、その`allowed_roots`はファイルを置いたdirectory配下に限られます。 +また、POSIXではファイルが自分の所有で、world-writableでない必要があります。 +それ以外のworkspaceを許可するには`--config PATH`を指定します。 + 個々の設定値は`CLI > CODEX_APP_MCP_*環境変数 > TOML > 組み込み既定値`の順で 解決します。 diff --git a/README.md b/README.md index b266a2c..2e584d7 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,11 @@ Configuration discovery is bottom-up: 3. the user config (`$XDG_CONFIG_HOME/codex-app-mcp/bridge.toml`, or the platform equivalent) 4. built-in fail-closed defaults +A `bridge.toml` found by the upward search may come from a cloned repository, so +its `allowed_roots` must stay inside the directory that holds it, and on POSIX +the file must be owned by you and not world-writable. Pass `--config PATH` to +allow workspaces elsewhere. + Individual values use `CLI > CODEX_APP_MCP_* environment > TOML > built-in` precedence. `defaults.cwd` remains supported for unusual launchers, but normally the client should launch the bridge in its workspace and leave it unset. See diff --git a/docs/configuration.md b/docs/configuration.md index babb86e..23d6b3c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -5,6 +5,19 @@ directory for the nearest `bridge.toml`, then checks the user configuration directory. Built-in defaults are fail-closed. Per-setting precedence is `CLI > CODEX_APP_MCP_* environment > TOML > built-in`. +A file found by the upward search is project-local and may be committed to an +untrusted checkout. Startup rejects it with `CONFIG_KEY_DENIED` unless it is +owned by the current user and not world-writable (POSIX), and unless every +`allowed_roots` entry resolves inside the directory holding the file. An +explicit `--config` file and the user configuration are not confined; use +`--config` for other workspaces, or on filesystems that cannot represent +ownership. Because discovery stops at the nearest file, the user configuration +is not read while a project-local file exists. + +`allowed_roots` entries are resolved once at startup, relative entries against +the launch directory. Workspace checks never re-resolve them, so a symlink +created later cannot widen a root. + `codex-app-mcp init` creates a project-local file, allows the resolved Git root (or current directory), and never overwrites an existing file. `init --global` and `init --config PATH` select other destinations. diff --git a/docs/security.md b/docs/security.md index d921bcf..0d34548 100644 --- a/docs/security.md +++ b/docs/security.md @@ -9,7 +9,11 @@ The default sandbox is `read-only`. `workspace-write` requires operator permission; `danger-full-access` is rejected. `allowed_roots` controls which cwd and resumed-thread workspaces are accepted, but it is not itself a file-read sandbox. New and resumed threads must confirm the requested sandbox and approval -settings before a turn starts. +settings before a turn starts. A project-local `bridge.toml` found by upward +discovery can only allow workspaces inside its own directory, so a cloned +repository cannot authorize working directories elsewhere; it can still allow +itself, including `workspace-write`. Roots are resolved once at startup and +never re-resolved per call. Existing Codex authentication is inherited; tools never accept API keys or login operations. Logs exclude prompts, answers, file contents, tokens, and raw diff --git a/docs/testing.md b/docs/testing.md index 4155e3f..7bd7725 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -19,7 +19,8 @@ aqua exec -- uv run --frozen pytest -m live -q ``` Release gates cover tool schemas, model/effort inheritance, approval denial, -workspace/sandbox enforcement, early-event delivery, cancellation during +workspace/sandbox enforcement, project-local configuration confinement and +startup-resolved roots, early-event delivery, cancellation during startup and execution, transport loss, concurrency, stdout purity, output bounds, recursion prevention, close/stop ownership, and signal shutdown. diff --git a/src/codex_app_mcp/cli.py b/src/codex_app_mcp/cli.py index 12079fb..303654d 100644 --- a/src/codex_app_mcp/cli.py +++ b/src/codex_app_mcp/cli.py @@ -80,8 +80,8 @@ async def _serve(args: argparse.Namespace) -> int: try: from .config import discover_bridge_config, require_operational_policy - config_path = discover_bridge_config(args.config) - if config_path is None: + discovered = discover_bridge_config(args.config) + if discovered is None: raise BridgeError( code="VALIDATION_ERROR", message=( @@ -90,7 +90,7 @@ async def _serve(args: argparse.Namespace) -> int: ), ) config = load_bridge_config( - config_path, + discovered.path, CliOverrides( model=args.model, effort=args.effort, @@ -98,6 +98,7 @@ async def _serve(args: argparse.Namespace) -> int: sandbox=args.sandbox, approval_policy=args.approval_policy, ), + project_local=discovered.project_local, ) require_operational_policy(config) except BridgeError as error: @@ -152,7 +153,10 @@ def _init(args: argparse.Namespace) -> int: if destination.exists(): try: - config = load_bridge_config(destination) + # The default destination is the file serve discovers as project-local. + config = load_bridge_config( + destination, project_local=not (args.global_config or args.config) + ) except BridgeError as error: sys.stderr.write( f"configuration error [{error.code}]: existing {destination} was not changed: " diff --git a/src/codex_app_mcp/config.py b/src/codex_app_mcp/config.py index c07363f..cfce17d 100644 --- a/src/codex_app_mcp/config.py +++ b/src/codex_app_mcp/config.py @@ -11,6 +11,7 @@ from __future__ import annotations import os +import stat import tomllib from collections.abc import Mapping from dataclasses import dataclass, field, replace @@ -276,16 +277,60 @@ class CliOverrides: approval_policy: str | None = None +def _check_project_config_owner(config_path: Path, info: os.stat_result) -> None: + """Reject a discovered file another local user could have written.""" + if not hasattr(os, "getuid"): + return + if info.st_uid != os.getuid() or info.st_mode & stat.S_IWOTH: + raise BridgeError( + code=CONFIG_KEY_DENIED, + message=( + f"project-local {config_path} must be owned by the current user and " + "not writable by others; fix it or pass --config PATH" + ), + ) + + +def _freeze_roots(policy: PolicyConfig, project_config: Path | None) -> PolicyConfig: + """Resolve ``allowed_roots`` once, against the launch directory. + + Workspace checks never re-resolve a root, so a symlink created after + startup cannot widen it. A project-local file found by upward discovery + may come from an untrusted checkout, so its roots must also stay under the + directory that holds it. + """ + scope = project_config.parent.resolve() if project_config is not None else None + roots: list[str] = [] + for root in policy.allowed_roots: + try: + resolved = Path(root).resolve() + except (OSError, ValueError) as exc: + raise _err(f"[policy] allowed_roots entry {root!r} is not a valid path") from exc + if scope is not None and not resolved.is_relative_to(scope): + raise BridgeError( + code=CONFIG_KEY_DENIED, + message=( + f"[policy] allowed_roots entry {root!r} in project-local {project_config} " + f"is outside {scope}; pass --config PATH to allow workspaces outside " + "the project" + ), + ) + roots.append(str(resolved)) + return replace(policy, allowed_roots=tuple(roots)) + + def load_bridge_config( path: str | Path | None, cli: CliOverrides | None = None, *, env: Mapping[str, str] | None = None, + project_local: bool = False, ) -> BridgeConfig: """Resolve the effective bridge configuration. ``env`` defaults to ``os.environ``. Unknown TOML top-level sections are - rejected rather than silently ignored. + rejected rather than silently ignored. ``project_local`` marks a file + found by upward discovery, whose policy is confined to its directory. """ env = os.environ if env is None else env data: dict[str, object] = {} @@ -296,6 +341,8 @@ def load_bridge_config( raise _err(f"bridge config file not found: {config_path}") try: with open(config_path, "rb") as fh: + if project_local: + _check_project_config_owner(config_path, os.fstat(fh.fileno())) data = tomllib.load(fh) except tomllib.TOMLDecodeError as exc: raise _err(f"invalid TOML in {config_path}: {exc}") from exc @@ -311,6 +358,7 @@ def load_bridge_config( runtime, defaults, policy, limits, logging_config, provenance = _parse_toml_section(data) if source == "defaults": provenance = {} + policy = _freeze_roots(policy, Path(path) if project_local and path is not None else None) # Environment overrides (bridge-specific variables only). def _env(name: str) -> str | None: @@ -401,23 +449,31 @@ def user_config_path(*, env: Mapping[str, str] | None = None) -> Path: return base / "codex-app-mcp" / "bridge.toml" +@dataclass(frozen=True, slots=True) +class DiscoveredConfig: + """A selected config file; ``project_local`` when found by upward search.""" + + path: Path + project_local: bool = False + + def discover_bridge_config( explicit: str | Path | None, *, start_dir: str | Path | None = None, env: Mapping[str, str] | None = None, -) -> Path | None: +) -> DiscoveredConfig | None: """Find one config: explicit, nearest ancestor, then the XDG user file.""" if explicit is not None: - return Path(explicit).expanduser().resolve() + return DiscoveredConfig(Path(explicit).expanduser().resolve()) current = Path.cwd() if start_dir is None else Path(start_dir) current = current.expanduser().resolve() for directory in (current, *current.parents): candidate = directory / "bridge.toml" if candidate.is_file(): - return candidate + return DiscoveredConfig(candidate, project_local=True) candidate = user_config_path(env=env) - return candidate if candidate.is_file() else None + return DiscoveredConfig(candidate) if candidate.is_file() else None def find_workspace_root(start_dir: str | Path | None = None) -> Path: diff --git a/src/codex_app_mcp/doctor.py b/src/codex_app_mcp/doctor.py index dcfd25a..6bf8f1d 100644 --- a/src/codex_app_mcp/doctor.py +++ b/src/codex_app_mcp/doctor.py @@ -96,9 +96,11 @@ async def run_doctor( "run 'codex-app-mcp init' or pass --config PATH" ), ) - config = load_bridge_config(selected) + config = load_bridge_config(selected.path, project_local=selected.project_local) require_operational_policy(config) - checks.append(_check(True, f"configuration loaded: {selected}")) + checks.append(_check(True, f"configuration loaded: {selected.path}")) + if selected.project_local: + checks.append(_check(True, f"project-local policy confined to {selected.path.parent}")) checks.append(_check(True, "policy.allowed_roots configured")) default_cwd = config.defaults.cwd if default_cwd: diff --git a/src/codex_app_mcp/policy.py b/src/codex_app_mcp/policy.py index 115f81c..72799fa 100644 --- a/src/codex_app_mcp/policy.py +++ b/src/codex_app_mcp/policy.py @@ -20,6 +20,9 @@ class PolicyStore: def __init__(self, config: BridgeConfig) -> None: self._config = config + # Resolved once: re-resolving per call would let a symlink created + # after startup redirect a root outside the operator's intent. + self._roots = tuple(Path(root).resolve() for root in config.policy.allowed_roots) @property def policy(self) -> PolicyConfig: @@ -51,8 +54,7 @@ def check_workspace(self, absolute_cwd: str) -> None: ), ) target = Path(absolute_cwd).resolve() - for root in self.policy.allowed_roots: - root_path = Path(root).resolve() + for root_path in self._roots: try: target.relative_to(root_path) return diff --git a/tests/contract/test_process.py b/tests/contract/test_process.py index ea7a644..6208e99 100644 --- a/tests/contract/test_process.py +++ b/tests/contract/test_process.py @@ -263,6 +263,28 @@ def test_cli_init_creates_minimal_project_config_without_overwrite(tmp_path: Pat assert config_path.read_text(encoding="utf-8") == config +def test_cli_init_applies_project_confinement_to_existing_config(tmp_path: Path): + repo = tmp_path / "repo" + repo.mkdir() + (repo / ".git").mkdir() + config_path = repo / "bridge.toml" + original = f'[policy]\nallowed_roots = ["{repo}", "/"]\n' + config_path.write_text(original, encoding="utf-8") + env = dict(os.environ) + env["HOME"] = str(tmp_path / "home") + proc = subprocess.run( + [sys.executable, "-m", "codex_app_mcp.cli", "init"], + cwd=repo, + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 2 + assert "CONFIG_KEY_DENIED" in proc.stderr + assert config_path.read_text(encoding="utf-8") == original + + def test_serve_without_config_fails_before_stdio(tmp_path: Path): env = dict(os.environ) env["HOME"] = str(tmp_path / "home") @@ -298,6 +320,31 @@ def test_serve_rejects_empty_allowed_roots_at_startup(tmp_path: Path): assert str(tmp_path / "bridge.toml") in proc.stderr +def test_serve_rejects_project_config_widening_allowed_roots(tmp_path: Path): + """A bridge.toml committed to a checkout cannot authorize workspaces outside it.""" + repo = tmp_path / "repo" + repo.mkdir() + (repo / "bridge.toml").write_text( + '[defaults]\nsandbox = "workspace-write"\ncwd = "../.."\n\n' + '[policy]\nallowed_roots = ["/"]\n' + ) + env = dict(os.environ) + env["HOME"] = str(tmp_path / "home") + env["XDG_CONFIG_HOME"] = str(tmp_path / "xdg") + proc = subprocess.run( + [sys.executable, "-m", "codex_app_mcp.cli", "serve"], + cwd=repo, + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 2 + assert proc.stdout == "" + assert "CONFIG_KEY_DENIED" in proc.stderr + assert f"is outside {repo.resolve()}" in proc.stderr + + def test_cli_init_refuses_home_as_allowed_root(tmp_path: Path): env = dict(os.environ) env["HOME"] = str(tmp_path) diff --git a/tests/unit/test_config_policy.py b/tests/unit/test_config_policy.py index f0bc8bb..92550c5 100644 --- a/tests/unit/test_config_policy.py +++ b/tests/unit/test_config_policy.py @@ -2,12 +2,15 @@ from __future__ import annotations +import os + import pytest from codex_app_mcp.backend.interface import ModelCatalogEntry from codex_app_mcp.config import ( CliOverrides, DefaultsConfig, + DiscoveredConfig, LimitsConfig, PolicyConfig, discover_bridge_config, @@ -15,7 +18,7 @@ load_bridge_config, user_config_path, ) -from codex_app_mcp.doctor import _codex_mcp_references_bridge, _is_supported_python +from codex_app_mcp.doctor import _codex_mcp_references_bridge, _is_supported_python, run_doctor from codex_app_mcp.errors import ( CONFIG_KEY_DENIED, MODEL_NOT_ALLOWED, @@ -170,12 +173,137 @@ def test_config_discovery_prefers_explicit_then_nearest_then_user(tmp_path): explicit = tmp_path / "explicit.toml" env = {"HOME": str(home), "XDG_CONFIG_HOME": str(xdg)} - assert discover_bridge_config(explicit, start_dir=nested, env=env) == explicit - assert discover_bridge_config(None, start_dir=nested, env=env) == nearest + assert discover_bridge_config(explicit, start_dir=nested, env=env) == DiscoveredConfig(explicit) + assert discover_bridge_config(None, start_dir=nested, env=env) == DiscoveredConfig( + nearest, project_local=True + ) nearest.unlink() - assert discover_bridge_config(None, start_dir=nested, env=env) == parent + assert discover_bridge_config(None, start_dir=nested, env=env) == DiscoveredConfig( + parent, project_local=True + ) parent.unlink() - assert discover_bridge_config(None, start_dir=nested, env=env) == user + assert discover_bridge_config(None, start_dir=nested, env=env) == DiscoveredConfig(user) + + +@pytest.mark.parametrize("root", ["/", "..", "{outside}"]) +def test_project_config_cannot_allow_roots_outside_its_directory(tmp_path, monkeypatch, root): + """A bridge.toml committed to a checkout must not widen the workspace allowlist.""" + repo = tmp_path / "repo" + repo.mkdir() + monkeypatch.chdir(repo) + toml = repo / "bridge.toml" + toml.write_text( + f"[defaults]\nsandbox = 'workspace-write'\n\n" + f"[policy]\nallowed_roots = [{str(root).format(outside=tmp_path / 'other')!r}]\n" + ) + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == CONFIG_KEY_DENIED + assert "outside" in excinfo.value.message + # The same file stays unrestricted when the operator selects it explicitly. + assert load_bridge_config(toml).policy.allowed_roots + + +def test_project_config_rejects_symlink_escaping_its_directory(tmp_path, monkeypatch): + repo = tmp_path / "repo" + repo.mkdir() + (repo / "home").symlink_to(tmp_path) + monkeypatch.chdir(repo) + toml = repo / "bridge.toml" + toml.write_text("[policy]\nallowed_roots = ['home']\n") + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == CONFIG_KEY_DENIED + + +@pytest.mark.parametrize("project_local", [True, False]) +def test_roots_are_resolved_once_so_a_later_symlink_cannot_widen_them( + tmp_path, monkeypatch, project_local +): + repo = (tmp_path / "repo").resolve() + (repo / "ws").mkdir(parents=True) + (repo / "link").symlink_to(repo / "ws") + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.chdir(repo) + toml = repo / "bridge.toml" + toml.write_text("[defaults]\ncwd = 'ws'\n\n[policy]\nallowed_roots = ['link']\n") + config = load_bridge_config(toml, project_local=project_local) + assert config.policy.allowed_roots == (str(repo / "ws"),) + store = PolicyStore(config) + store.check_workspace(store.resolve_cwd(None, bridge_cwd=str(repo))) + + # A turn replaces the resolved root itself with a symlink to outside. + (repo / "ws").rmdir() + (repo / "ws").symlink_to(outside) + for cwd in (store.resolve_cwd(None, bridge_cwd=str(repo)), str(outside)): + with pytest.raises(BridgeError) as excinfo: + store.check_workspace(cwd) + assert excinfo.value.code == WORKSPACE_DENIED + + +def test_project_config_scope_holds_when_launched_from_a_subdirectory(tmp_path, monkeypatch): + repo = (tmp_path / "repo").resolve() + (repo / "sub").mkdir(parents=True) + monkeypatch.chdir(repo / "sub") + toml = repo / "bridge.toml" + toml.write_text("[defaults]\ncwd = '../..'\n\n[policy]\nallowed_roots = ['..']\n") + config = load_bridge_config(toml, project_local=True) + assert config.policy.allowed_roots == (str(repo),) + # An escaping defaults.cwd still fails the per-call workspace check. + store = PolicyStore(config) + with pytest.raises(BridgeError) as excinfo: + store.check_workspace(store.resolve_cwd(None, bridge_cwd=str(repo / "sub"))) + assert excinfo.value.code == WORKSPACE_DENIED + + toml.write_text("[policy]\nallowed_roots = ['../..']\n") + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == CONFIG_KEY_DENIED + + +def test_invalid_allowed_root_is_a_config_error(tmp_path): + toml = tmp_path / "bridge.toml" + toml.write_text('[policy]\nallowed_roots = ["a\\u0000b"]\n') + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == VALIDATION_ERROR + + +@pytest.mark.skipif(not hasattr(os, "getuid"), reason="POSIX ownership only") +def test_project_config_must_be_owned_and_not_world_writable(tmp_path, monkeypatch): + toml = tmp_path / "bridge.toml" + toml.write_text(f"[policy]\nallowed_roots = [{str(tmp_path)!r}]\n") + toml.chmod(0o646) + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == CONFIG_KEY_DENIED + + # Group-writable stays accepted for user-private-group umask 002 setups. + toml.chmod(0o664) + load_bridge_config(toml, project_local=True) + uid = os.getuid() + monkeypatch.setattr(os, "getuid", lambda: uid + 1) + with pytest.raises(BridgeError) as excinfo: + load_bridge_config(toml, project_local=True) + assert excinfo.value.code == CONFIG_KEY_DENIED + + +async def test_doctor_applies_project_config_confinement(tmp_path, monkeypatch): + repo = (tmp_path / "repo").resolve() + repo.mkdir() + monkeypatch.chdir(repo) + monkeypatch.setenv("CODEX_HOME", str(tmp_path / "codex")) + toml = repo / "bridge.toml" + toml.write_text("[policy]\nallowed_roots = ['/']\n") + report = await run_doctor(skip_runtime=True) + assert report["ok"] is False + assert any("CONFIG_KEY_DENIED" in check["detail"] for check in report["checks"]) + + toml.write_text("[policy]\nallowed_roots = ['.']\n") + report = await run_doctor(skip_runtime=True) + details = [check["detail"] for check in report["checks"]] + assert f"project-local policy confined to {repo}" in details def test_user_config_falls_back_when_xdg_is_relative(tmp_path):