From 79aa4f7fc08799266ea3fdedab3c328e643cec76 Mon Sep 17 00:00:00 2001 From: latte Date: Wed, 14 Oct 2026 17:22:29 +0200 Subject: [PATCH] fix: load ./.env only when private and owned, add --env-file and AEGIS_ENV_FILE (SEC-04) --- README.md | 10 +- docs/configuration.md | 25 +++- docs/getting-started.md | 2 +- docs/security.md | 2 +- docs/troubleshooting.md | 4 +- src/aegis_gitea_mcp/cli.py | 22 +++- src/aegis_gitea_mcp/config.py | 107 ++++++++++++++++- src/aegis_gitea_mcp/setup_cmd.py | 7 +- tests/conftest.py | 4 +- tests/security/test_env_file.py | 193 +++++++++++++++++++++++++++++++ tests/test_config.py | 4 +- 11 files changed, 360 insertions(+), 20 deletions(-) create mode 100644 tests/security/test_env_file.py diff --git a/README.md b/README.md index dc95e80..2c4d909 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,9 @@ uv run aegis-gitea-mcp setup they are missing, and keeping values it does not know; - prints ready-made client configurations and a docker-compose snippet. -Secrets are only ever written to the env file, never printed. +Secrets are only ever written to the env file, never printed. `serve` reads `./.env` only +while it keeps the 0600 mode and your ownership; to keep the file elsewhere use +`--env-file PATH` (or `AEGIS_ENV_FILE`) on both commands. Run it: @@ -247,8 +249,10 @@ More in [docs/deployment.md](docs/deployment.md) and [docs/hardening.md](docs/ha ## Configuration -All settings are environment variables (or `.env` in the working directory), case-insensitive. -Core ones: +All settings are environment variables (or an env file), case-insensitive. The env file is +`--env-file PATH`, else `AEGIS_ENV_FILE`, else `./.env` only when it is a regular file owned by +you with mode 0600 (what `setup` writes); any other `./.env` is ignored with a warning (run +`chmod 600 .env` or pass `--env-file`). Real environment variables always win. Core ones: | Variable | Default | Meaning | |---|---|---| diff --git a/docs/configuration.md b/docs/configuration.md index 8b6469b..94cc6eb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1,9 +1,28 @@ # Configuration All settings are environment variables. They are read case-insensitively from the process -environment and from a `.env` file in the working directory (the environment wins). Unknown -variables are ignored. A blank value (`KEY=` in `.env`) counts as unset for the optional -secrets, URLs and paths. List values accept commas or whitespace as separators. +environment and from an env file (the environment always wins). Unknown variables are ignored. +A blank value (`KEY=` in the env file) counts as unset for the optional secrets, URLs and +paths. List values accept commas or whitespace as separators. + +## Which env file is read + +The env file is never trusted just because it sits in the working directory: MCP clients start +stdio servers inside the project they work on, which can be a cloned repository carrying a +planted `.env` that switches off secret masking or the audit log or points `GITEA_URL` at +another host. The file is chosen in this order: + +1. `--env-file PATH` of `serve` (and `setup`): used as given. It must exist and be a regular + file. +2. The `AEGIS_ENV_FILE` environment variable: same rules. +3. `./.env`, but only when it is a regular file (not a symlink), owned by the user running the + server, with no group or other permissions (mode 0600, which `setup` writes). Otherwise it is + ignored and one warning goes to stderr that says why and how to fix it: + `chmod 600 .env`, or pass `--env-file`. + +Real environment variables take precedence over every file. A missing `./.env` is silent. +Containers pass settings as environment variables (`env_file:` in compose), not through the +working directory. `.env.example` is the annotated template. `uv run aegis-gitea-mcp setup` fills one in for you (see [getting-started.md](getting-started.md)). Secret settings never appear in logs, errors or diff --git a/docs/getting-started.md b/docs/getting-started.md index c296022..9fd4fe3 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -29,7 +29,7 @@ Setup prompts for the Gitea URL and the token (hidden input). For unattended use | `--gitea-url` | Gitea base URL | | `--public-base-url` | Public https URL of this server. Enables registration of the OAuth application | | `--no-oauth` | Do not create the Gitea OAuth application | -| `--env-file` | Env file to create or update (default `./.env`) | +| `--env-file` | Env file to create or update (default `AEGIS_ENV_FILE`, else `./.env`) | | `--template` | Env template (default `./.env.example`, else the copy built into the package) | | `--yes` | Never prompt, and accept regenerating an OAuth client secret | diff --git a/docs/security.md b/docs/security.md index aa630b4..8869b54 100644 --- a/docs/security.md +++ b/docs/security.md @@ -72,7 +72,7 @@ Tampering | Query smuggling of credentials or Sudo | `token`, `access_token`, `sudo`, `_sudo` refused as query names (any case) | `gitea/encoding.py` | | Malformed arguments | JSON Schema validation (Draft 2020-12, `additionalProperties: false`) before anything runs; messages never echo values | `server.py` | | Store tampering or wrong key | Fernet (authenticated encryption) with an HKDF-SHA256 key; undecryptable store stops startup instead of being wiped; symlinks refused | `oauth/store.py` (rollback residual: SEC-13) | -| Settings injection from a planted `.env` | none yet | `config.py` (SEC-04) | +| Settings injection from a planted `.env` | `./.env` is loaded only when it is a regular file, not a symlink, owned by the user, mode 0600; otherwise ignored with a warning; `--env-file` or `AEGIS_ENV_FILE` name a file explicitly; the environment wins | `config.py` (`select_env_file`), `cli.py` | Repudiation diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f65d581..e5eddaf 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -8,7 +8,9 @@ Gitea's sanitized message, and the audit log has one event per call (see | Message or exit | Cause and fix | |---|---| -| Exit code 2, "GITEA_TOKEN" required (stdio) | stdio needs `GITEA_TOKEN`. Put it in the client's `env` block or in `.env` in the working directory the client starts the server in. | +| Exit code 2, "GITEA_TOKEN" required (stdio) | stdio needs `GITEA_TOKEN`. Put it in the client's `env` block, or in an env file passed with `--env-file` (or `AEGIS_ENV_FILE`). A `.env` in the working directory counts only when it is a regular file you own with mode 0600. | +| Warning "ignoring ./.env: ..." | The working directory's `.env` is a symlink, owned by someone else or accessible to group or others, so its settings are not loaded. Run `chmod 600 .env`, or pass `--env-file PATH`. See "Which env file is read" in [configuration.md](configuration.md). | +| Exit code 2, "env file ... does not exist" | The file named by `--env-file` or `AEGIS_ENV_FILE` is missing or not a regular file. | | Exit code 2, refuses a non-loopback bind | `MCP_HOST` is not loopback and there is no `MCP_BEARER_TOKEN` or complete OAuth setup. Set one of them, or bind `127.0.0.1` behind a reverse proxy. | | Exit code 2, lists OAuth settings | `OAUTH_ENABLED=true` needs `GITEA_OAUTH_CLIENT_ID`, `GITEA_OAUTH_CLIENT_SECRET`, `TOKEN_ENCRYPTION_KEY` and `PUBLIC_BASE_URL`. | | Validation error naming a setting | See the rules in [configuration.md](configuration.md): URLs must be http(s) without credentials, `PUBLIC_BASE_URL` must be https, secrets need 32 characters, `OAUTH_SCOPES` accepts only granular scopes. | diff --git a/src/aegis_gitea_mcp/cli.py b/src/aegis_gitea_mcp/cli.py index afd69c7..44e103b 100644 --- a/src/aegis_gitea_mcp/cli.py +++ b/src/aegis_gitea_mcp/cli.py @@ -13,7 +13,14 @@ from collections.abc import Sequence from pydantic import ValidationError from aegis_gitea_mcp import __version__, setup_cmd -from aegis_gitea_mcp.config import Settings, get_settings, reset_settings +from aegis_gitea_mcp.config import ( + ENV_FILE_VARIABLE, + EnvFileError, + Settings, + get_settings, + reset_settings, + set_env_file, +) from aegis_gitea_mcp.logging_utils import configure_logging @@ -51,6 +58,15 @@ def build_parser() -> argparse.ArgumentParser: metavar="PORT", help="HTTP port (default: MCP_PORT)", ) + serve.add_argument( + "--env-file", + default=None, + metavar="PATH", + help=( + f"env file to load (default: {ENV_FILE_VARIABLE}, else ./.env when it is a regular " + "file owned by you with mode 0600; real environment variables always win)" + ), + ) setup = subparsers.add_parser("setup", help="verify a Gitea instance and write the env file") setup_cmd.add_arguments(setup) @@ -69,9 +85,13 @@ def _load_settings(args: argparse.Namespace) -> Settings | None: for name, value in overrides.items(): if value is not None: os.environ[name] = value + set_env_file(getattr(args, "env_file", None)) reset_settings() try: return get_settings() + except EnvFileError as exc: + print(f"aegis-gitea-mcp: {exc}", file=sys.stderr) + return None except ValidationError as exc: # Print field names and messages only: str(exc) would echo the offending values. print("aegis-gitea-mcp: invalid configuration:", file=sys.stderr) diff --git a/src/aegis_gitea_mcp/config.py b/src/aegis_gitea_mcp/config.py index 2112ec2..40b3546 100644 --- a/src/aegis_gitea_mcp/config.py +++ b/src/aegis_gitea_mcp/config.py @@ -1,14 +1,18 @@ """Configuration management for AegisGitea MCP. -All settings come from environment variables (case-insensitive) or a ``.env`` -file in the working directory. Secrets are ``SecretStr`` so they never appear in -``repr()`` or model dumps. Requirements that depend on how the server is started +All settings come from environment variables (case-insensitive) or an env file. +The env file is never trusted implicitly: see :func:`select_env_file`. Secrets +are ``SecretStr`` so they never appear in ``repr()`` or model dumps. Requirements that depend on how the server is started (for example ``GITEA_TOKEN`` for stdio) are checked at serve time, not on import. """ from __future__ import annotations import ipaddress +import os +import stat +import sys +from collections.abc import Mapping from pathlib import Path from typing import Annotated, Any, Literal from urllib.parse import urlsplit @@ -107,7 +111,7 @@ class Settings(BaseSettings): """Application settings loaded from environment variables.""" model_config = SettingsConfigDict( - env_file=".env", + # No implicit env_file: get_settings() decides which file to load (SEC-04). env_file_encoding="utf-8", case_sensitive=False, extra="ignore", @@ -347,14 +351,105 @@ class Settings(BaseSettings): return self.oauth_enabled and not self.oauth_config_errors() +#: Environment variable naming an env file (same as ``--env-file``). +ENV_FILE_VARIABLE = "AEGIS_ENV_FILE" +DEFAULT_ENV_FILE = ".env" + + +class EnvFileError(ValueError): + """An explicitly requested env file cannot be used (the message is safe to print).""" + + +def env_file_problem(path: Path) -> str | None: + """Return why the implicit env file ``path`` is not trusted, or None when it may be loaded. + + Trusted means: a regular file (not a symlink) owned by the current user that + group and others cannot access (mode 0600, which ``setup`` writes). A missing + file is not a problem here. + """ + try: + info = os.lstat(path) + except FileNotFoundError: + return None + except OSError as exc: + return f"it cannot be inspected ({exc.strerror or type(exc).__name__})" + if stat.S_ISLNK(info.st_mode): + return "it is a symlink" + if not stat.S_ISREG(info.st_mode): + return "it is not a regular file" + if info.st_uid != getattr(os, "geteuid", lambda: info.st_uid)(): + return "it is owned by another user" + mode = stat.S_IMODE(info.st_mode) + if mode & 0o077: + return f"its permissions are {mode:04o} (group or others have access)" + return None + + +def select_env_file( + explicit: str | Path | None = None, + *, + environ: Mapping[str, str] | None = None, + directory: Path | None = None, +) -> tuple[Path | None, str | None]: + """Choose the env file to load; returns ``(path, warning)``. + + Order: ``explicit`` (``--env-file``), then ``AEGIS_ENV_FILE``, then ``./.env``. + A file named explicitly is the operator's choice and is used as is. The + implicit ``./.env`` is loaded only when :func:`env_file_problem` finds nothing + wrong; otherwise it is ignored and ``warning`` says why and how to fix it. A + working directory is not trustworthy by itself: clients start stdio servers + inside the project (possibly a cloned repository) they are working on, and a + planted ``.env`` could switch off redaction or audit, or redirect the token. + Real environment variables always take precedence over the file. + + Raises: + EnvFileError: An explicitly named file is missing or not a regular file. + """ + env = os.environ if environ is None else environ + named = explicit or env.get(ENV_FILE_VARIABLE, "").strip() or None + if named is not None: + path = Path(named).expanduser() + try: + regular = stat.S_ISREG(os.stat(path).st_mode) + except OSError: + regular = False + if not regular: + raise EnvFileError(f"env file {path} does not exist or is not a regular file") + return path, None + path = (directory or Path.cwd()) / DEFAULT_ENV_FILE + problem = env_file_problem(path) + if problem is not None: + return None, ( + f"ignoring ./{DEFAULT_ENV_FILE}: {problem}. Run 'chmod 600 {DEFAULT_ENV_FILE}' " + f"(and make sure you own it), or pass --env-file PATH (or set {ENV_FILE_VARIABLE}) " + "to load a file explicitly" + ) + return (path if path.exists() else None), None + + _settings: Settings | None = None +_env_file_choice: str | Path | None = None + + +def set_env_file(path: str | Path | None) -> None: + """Choose the env file for the next :func:`get_settings` call (``--env-file``).""" + global _env_file_choice + _env_file_choice = path + reset_settings() def get_settings() -> Settings: - """Get or create the global settings instance.""" + """Get or create the global settings instance. + + Raises: + EnvFileError: An explicitly named env file cannot be used. + """ global _settings if _settings is None: - _settings = Settings() # type: ignore[call-arg] + path, warning = select_env_file(_env_file_choice) + if warning: + print(f"aegis-gitea-mcp: warning: {warning}", file=sys.stderr) + _settings = Settings(_env_file=path) # type: ignore[call-arg] return _settings diff --git a/src/aegis_gitea_mcp/setup_cmd.py b/src/aegis_gitea_mcp/setup_cmd.py index 21e3301..7598ea6 100644 --- a/src/aegis_gitea_mcp/setup_cmd.py +++ b/src/aegis_gitea_mcp/setup_cmd.py @@ -744,7 +744,10 @@ def add_arguments(parser: argparse.ArgumentParser) -> None: help="public https URL of this MCP server (enables the OAuth app)", ) parser.add_argument( - "--env-file", default=".env", help="env file to create or update (default: ./.env)" + "--env-file", + default=None, + metavar="PATH", + help="env file to create or update (default: AEGIS_ENV_FILE, else ./.env)", ) parser.add_argument( "--template", @@ -798,7 +801,7 @@ def _generated(existing: dict[str, EnvEntry], key: str, values: dict[str, str]) def _run(args: argparse.Namespace) -> int: - env_file = Path(args.env_file) + env_file = Path(args.env_file or os.environ.get("AEGIS_ENV_FILE", "").strip() or ".env") check_env_target(env_file) template = _load_template(args.template) existing, old_mode = _read_existing(env_file) diff --git a/tests/conftest.py b/tests/conftest.py index 0e60f91..cc0a08c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,12 +13,13 @@ from pathlib import Path import pytest from aegis_gitea_mcp.audit import reset_audit_logger -from aegis_gitea_mcp.config import Settings, reset_settings +from aegis_gitea_mcp.config import Settings, reset_settings, set_env_file from aegis_gitea_mcp.request_context import clear_gitea_auth_context def _reset_singletons() -> None: """Reset every module-level singleton.""" + set_env_file(None) reset_settings() reset_audit_logger() clear_gitea_auth_context() @@ -29,6 +30,7 @@ def reset_globals(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Generator[ """Isolate each test: clean env, tmp cwd, tmp state paths, fresh singletons.""" for field_name in Settings.model_fields: monkeypatch.delenv(field_name.upper(), raising=False) + monkeypatch.delenv("AEGIS_ENV_FILE", raising=False) monkeypatch.chdir(tmp_path) data_dir = tmp_path / "data" diff --git a/tests/security/test_env_file.py b/tests/security/test_env_file.py new file mode 100644 index 0000000..5e40f4b --- /dev/null +++ b/tests/security/test_env_file.py @@ -0,0 +1,193 @@ +"""``./.env`` is not trusted implicitly (SEC-04). + +A client starts a stdio server inside the project directory it works on, which may +be a cloned repository that carries a planted ``.env``. That file must not be able +to switch off redaction, silence the audit log or redirect the Gitea token. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from aegis_gitea_mcp.cli import main +from aegis_gitea_mcp.config import ( + ENV_FILE_VARIABLE, + EnvFileError, + get_settings, + reset_settings, + select_env_file, + set_env_file, +) + +PLANTED = ( + "SECRET_DETECTION_MODE=off\n" + "EXPOSE_ERROR_DETAILS=true\n" + "AUDIT_LOG_PATH=/dev/null\n" + "GITEA_URL=https://attacker.example.com\n" + "READ_ONLY=true\n" +) + + +@pytest.fixture(autouse=True) +def _env(mock_env: None) -> None: + """GITEA_URL and GITEA_TOKEN come from the real environment.""" + + +def write_env(path: Path, text: str = PLANTED, mode: int = 0o600) -> Path: + path.write_text(text) + path.chmod(mode) + return path + + +@pytest.mark.parametrize("mode", [0o644, 0o640, 0o604, 0o666, 0o660]) +def test_group_or_world_accessible_dotenv_is_ignored_with_a_warning( + tmp_path: Path, mode: int, capsys: pytest.CaptureFixture[str] +) -> None: + write_env(tmp_path / ".env", mode=mode) + os.environ.pop("AUDIT_LOG_PATH") + + settings = get_settings() + + assert settings.secret_detection_mode == "mask" + assert settings.expose_error_details is False + assert settings.read_only is False + assert settings.gitea_url == "https://gitea.example.com" + assert settings.resolved_audit_log_path == settings.data_dir / "audit.log" + err = capsys.readouterr().err + assert "ignoring ./.env" in err + assert "chmod 600 .env" in err and "--env-file" in err + assert "attacker" not in err # the warning never prints file content + + +def test_symlinked_dotenv_is_ignored(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + target = write_env(tmp_path / "real.env") + (tmp_path / ".env").symlink_to(target) + + assert get_settings().secret_detection_mode == "mask" + assert "it is a symlink" in capsys.readouterr().err + + +def test_dotenv_that_is_not_a_regular_file_is_ignored( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + (tmp_path / ".env").mkdir() + + assert get_settings().secret_detection_mode == "mask" + assert "not a regular file" in capsys.readouterr().err + + +def test_dotenv_owned_by_another_user_is_ignored( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + write_env(tmp_path / ".env") + monkeypatch.setattr(os, "geteuid", lambda: os.stat(tmp_path / ".env").st_uid + 1) + + assert get_settings().secret_detection_mode == "mask" + assert "owned by another user" in capsys.readouterr().err + + +def test_private_dotenv_owned_by_the_user_is_honoured( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + write_env(tmp_path / ".env", "READ_ONLY=true\nMCP_PORT=9191\n") + + settings = get_settings() + + assert settings.read_only is True and settings.mcp_port == 9191 + assert capsys.readouterr().err == "" + + +def test_environment_variables_win_over_the_env_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + write_env(tmp_path / ".env", "MCP_PORT=9191\nGITEA_URL=https://file.example.com\n") + monkeypatch.setenv("MCP_PORT", "9000") + + settings = get_settings() + + assert settings.mcp_port == 9000 + assert settings.gitea_url == "https://gitea.example.com" + + +def test_missing_dotenv_is_silent(capsys: pytest.CaptureFixture[str]) -> None: + get_settings() + + assert capsys.readouterr().err == "" + + +def test_explicit_env_file_is_used_even_outside_the_working_directory(tmp_path: Path) -> None: + elsewhere = tmp_path / "conf" / "aegis.env" + elsewhere.parent.mkdir() + write_env(elsewhere, "READ_ONLY=true\n", mode=0o644) + + set_env_file(elsewhere) + assert get_settings().read_only is True + + +def test_env_file_variable_selects_the_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + chosen = write_env(tmp_path / "chosen.env", "READ_ONLY=true\n") + # The (untrusted) ./.env must not matter when a file is named. + write_env(tmp_path / ".env", mode=0o644) + monkeypatch.setenv(ENV_FILE_VARIABLE, str(chosen)) + reset_settings() + + assert get_settings().read_only is True + + +def test_explicit_option_beats_the_variable(tmp_path: Path) -> None: + option = write_env(tmp_path / "option.env", "MCP_PORT=9001\n") + variable = write_env(tmp_path / "variable.env", "MCP_PORT=9002\n") + + path, warning = select_env_file(option, environ={ENV_FILE_VARIABLE: str(variable)}) + + assert path == option and warning is None + + +@pytest.mark.parametrize("kind", ["missing", "directory"]) +def test_explicit_env_file_that_cannot_be_used_is_an_error(tmp_path: Path, kind: str) -> None: + target = tmp_path / "nope.env" + if kind == "directory": + target.mkdir() + + with pytest.raises(EnvFileError): + select_env_file(target) + + +def test_serve_with_a_missing_env_file_exits_2( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert main(["serve", "--env-file", str(tmp_path / "nope.env")]) == 2 + assert "does not exist" in capsys.readouterr().err + + +def test_serve_ignores_a_planted_dotenv_and_audits_to_the_real_location( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + from aegis_gitea_mcp import cli, transports + + monkeypatch.setattr(cli, "configure_logging", lambda level: None) + write_env(tmp_path / ".env", mode=0o644) + seen: list[object] = [] + monkeypatch.setattr(transports, "run_stdio", lambda settings: seen.append(settings) or 0) + + assert main(["serve"]) == 0 + + (settings,) = seen + assert settings.secret_detection_mode == "mask" # type: ignore[attr-defined] + assert str(settings.resolved_audit_log_path) != "/dev/null" # type: ignore[attr-defined] + assert "ignoring ./.env" in capsys.readouterr().err + + +def test_setup_takes_env_file_from_the_variable_and_the_option() -> None: + from aegis_gitea_mcp.cli import build_parser + + args = build_parser().parse_args(["setup", "--env-file", "/tmp/x.env"]) + assert args.env_file == "/tmp/x.env" + assert build_parser().parse_args(["setup"]).env_file is None diff --git a/tests/test_config.py b/tests/test_config.py index 5b5b7ae..545618d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -91,7 +91,9 @@ def test_env_names_are_case_insensitive(monkeypatch: pytest.MonkeyPatch) -> None def test_reads_dotenv_file(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: - (tmp_path / ".env").write_text("GITEA_URL=https://dotenv.example.com\nREAD_ONLY=true\n") + dotenv = tmp_path / ".env" + dotenv.write_text("GITEA_URL=https://dotenv.example.com\nREAD_ONLY=true\n") + dotenv.chmod(0o600) # an implicit ./.env is only trusted with mode 0600 (SEC-04) reset_settings() settings = get_settings()