diff --git a/CHANGELOG.md b/CHANGELOG.md index 482b799..23ef611 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and versions are tracked in the repo-root `VERSION` file. ### Changed +- Bound convenience-profile discovery and validate implicit project configuration trust; cap YAML input size (#385). - Skip contended retention passes instead of blocking CLI invocations on housekeeping locks (#386). - Reuse secure log lock descriptors and cache source paths per invocation; logging I/O failures stay inside logging (#381). - Align the Typer support floor with the tested matrix and cover representative diff --git a/docs/api-reference.md b/docs/api-reference.md index 320b048..e1390ec 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -116,7 +116,7 @@ base_cli.TyperAdapter(...) ### `BatteriesIncludedConfigLoader` **Kind:** class -**Signature:** `BatteriesIncludedConfigLoader(cli_name: 'str | None' = None, *, user_config_dir: 'Path | None' = None, user_config_name: 'str' = 'config.yaml', project_config_name: 'str' = '.base-cli.yaml', environment_dir_name: 'str' = 'environments') -> 'None'` +**Signature:** `BatteriesIncludedConfigLoader(cli_name: 'str | None' = None, *, user_config_dir: 'Path | None' = None, user_config_name: 'str' = 'config.yaml', project_config_name: 'str' = '.base-cli.yaml', environment_dir_name: 'str' = 'environments', verify_project_config: 'bool' = False) -> 'None'` **Behavior:** Load conventional user, project, environment, and explicit layers. diff --git a/docs/local-config.md b/docs/local-config.md index 3f51f61..bcc2abb 100644 --- a/docs/local-config.md +++ b/docs/local-config.md @@ -29,3 +29,27 @@ directory below the platform's default config root (for example, `~/.config/tool` on Linux). Consumers with an existing configuration-root policy should pass `user_config_dir` explicitly; the identity is then metadata only. + +## Trust of discovered project configuration + +`CliProfile.batteries_included()` validates implicit project configuration before +loading it. On POSIX, the file and directories between the working directory and +the discovered project must be owned by the invoking user or root and must not +be writable by group/other. Symlinks and Windows reparse points are refused, +including project environment files. Refusals raise `ConfigurationError` naming +the path. Windows ACL ownership/write permissions are not evaluated; applications +using shared Windows workspaces must provide their own discovery/trust policy. + +Discovery checks the current directory and at most 32 ancestors, stops at `.git` +(including worktree marker files), and never crosses a filesystem boundary. +`max_project_ancestor_depth=0` restricts discovery to the current directory; +`project_boundary_marker` changes the marker or accepts `None` to disable markers. +`verify_discovered_config=False` explicitly opts out of permission/reparse checks +for knowingly shared workspaces. It does not disable depth/filesystem limits. +Custom discovery callbacks own discovery boundaries; their project files still +receive the loader's trust checks. `CliProfile.generic()` remains unchanged. + +All YAML files are limited to 1 MiB of UTF-8 input, 64 container levels, and +100,000 visited values (including alias expansion); recursive aliases are refused. +Explicit `--config` is an intentional file choice and bypasses discovery trust, +but still uses these parsing bounds. diff --git a/docs/security-threat-model.md b/docs/security-threat-model.md index 78e479d..b7c46e6 100644 --- a/docs/security-threat-model.md +++ b/docs/security-threat-model.md @@ -128,3 +128,13 @@ Before shipping a CLI built on the framework, the consumer should: behavior; and 6. run the release/security checklist in [`security-review.md`](security-review.md) for every release and whenever a trust boundary changes. + +## Repository-controlled configuration + +The convenience profile's implicit configuration can affect environment, logging, +and retained diagnostics. POSIX owner/mode checks, reparse/symlink refusal, project +boundaries, and bounded YAML parsing protect against accidental adoption of +ancestor configuration. See [local configuration](local-config.md) for the explicit +shared-workspace opt-out and Windows ACL limitation. These checks are not a +sandbox for a hostile process running as the same account; custom consumer +configuration and discovery policies remain the consumer's responsibility. diff --git a/lib/python/base_cli/config.py b/lib/python/base_cli/config.py index d1caef4..1808540 100644 --- a/lib/python/base_cli/config.py +++ b/lib/python/base_cli/config.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import re import stat from collections.abc import Mapping @@ -26,6 +27,7 @@ _LOG_LEVELS = frozenset({"debug", "info", "warning", "error", "critical"}) _SAFE_NAME = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]*\Z") _SAFE_FILENAME = re.compile(r"(?:[A-Za-z0-9][A-Za-z0-9_.-]*|\.[A-Za-z0-9][A-Za-z0-9_.-]*)\Z") +_CONFIG_MAX_BYTES = 1_048_576 _CONFIG_MAX_DEPTH = 64 _CONFIG_MAX_NODES = 100_000 @@ -194,6 +196,7 @@ def __init__( user_config_name: str = "config.yaml", project_config_name: str = ".base-cli.yaml", environment_dir_name: str = "environments", + verify_project_config: bool = False, ) -> None: if _SAFE_FILENAME.fullmatch(user_config_name) is None: raise ValueError("user_config_name must be a simple filename") @@ -216,6 +219,7 @@ def __init__( self.user_config_name = user_config_name self.project_config_name = project_config_name self.environment_dir_name = environment_dir_name + self.verify_project_config = verify_project_config @property def user_config_path(self) -> Path: @@ -246,6 +250,8 @@ def load( ) -> ConfigSnapshot: user_values = load_yaml_file(self.user_config_path) project_path = self.project_config_path(project_root) + if self.verify_project_config and project_path is not None and project_path.exists(): + validate_discovered_config_path(project_path, project_root) project_values = load_yaml_file(project_path) if project_path is not None else {} explicit_values = load_yaml_file(explicit_path, required=True) if explicit_path is not None else {} @@ -262,6 +268,8 @@ def load( selected_environment, ) user_environment = load_yaml_file(user_environment_path) + if self.verify_project_config and project_environment_path is not None and project_environment_path.exists(): + validate_discovered_config_path(project_environment_path, project_root) project_environment = load_yaml_file(project_environment_path) if project_environment_path is not None else {} merged: dict[str, Any] = {} @@ -286,6 +294,35 @@ def load( ) +def validate_discovered_config_path(path: Path, root: Path | None = None) -> None: + """Refuse implicit configuration controlled through unsafe path components.""" + paths = [path] + if root is not None: + parent = path.parent + while True: + paths.append(parent) + if parent == root: + break + if parent == parent.parent: + raise ConfigurationError(f"Discovered config '{path}' is outside project root '{root}'.") + parent = parent.parent + for candidate in paths: + try: + current = candidate.lstat() + except OSError as exc: + raise ConfigurationError(f"Cannot validate discovered configuration path '{candidate}': {exc}") from exc + if stat.S_ISLNK(current.st_mode) or getattr(current, "st_file_attributes", 0) & 0x400: + raise ConfigurationError( + f"Refusing discovered configuration through symlink or reparse point '{candidate}'." + ) + if os.name != "nt" and (current.st_mode & 0o002 or current.st_uid not in {0, os.getuid()}): + raise ConfigurationError( + f"Untrusted discovered configuration path '{candidate}': require user/root ownership " + "and no other-write permission. Fix permissions or see the " + "local configuration trust policy for an explicit shared-workspace opt-out." + ) + + def load_yaml_file(path: Path, *, required: bool = False) -> dict[str, Any]: """Load a YAML mapping, optionally requiring a regular file to exist. @@ -311,7 +348,11 @@ def load_yaml_file(path: Path, *, required: bool = False) -> dict[str, Any]: raise ConfigurationError(str(exc)) from exc try: - contents = path.read_text(encoding="utf-8") + with path.open("rb") as stream: + raw = stream.read(_CONFIG_MAX_BYTES + 1) + if len(raw) > _CONFIG_MAX_BYTES: + raise ConfigurationError(f"Config file '{path}' exceeds the maximum size of {_CONFIG_MAX_BYTES} bytes.") + contents = raw.decode("utf-8") except FileNotFoundError as exc: if required: raise ConfigurationError(f"Config file '{path}' does not exist.") from exc @@ -321,7 +362,40 @@ def load_yaml_file(path: Path, *, required: bool = False) -> dict[str, Any]: except UnicodeDecodeError as exc: raise ConfigurationError(f"Unable to read config file '{path}': {exc}") from exc try: - data = yaml.safe_load(contents) + loader = yaml.SafeLoader(contents) + try: + node = loader.get_single_node() + # Inspect the composed graph before constructors expand YAML merge + # aliases. Count repeated edges, not just distinct node identities. + stack = [(False, node, 0, "")] if node is not None else [] + active: set[int] = set() + nodes = 0 + while stack: + exiting, current, depth, location = stack.pop() + if exiting: + active.remove(id(current)) + continue + nodes += 1 + if id(current) in active: + raise ConfigurationError(f"Config file '{path}' contains a recursive value at '{location}'.") + if nodes > _CONFIG_MAX_NODES: + raise ConfigurationError(f"Config file '{path}' exceeds YAML expansion limits.") + if depth > _CONFIG_MAX_DEPTH: + raise ConfigurationError( + f"Config file '{path}' exceeds the maximum nesting depth of {_CONFIG_MAX_DEPTH}." + ) + if isinstance(current, (yaml.MappingNode, yaml.SequenceNode)): + active.add(id(current)) + stack.append((True, current, depth, location)) + if isinstance(current, yaml.MappingNode): + for key, child in current.value: + child_path = f"{location}.{key.value}" if location else str(key.value) + stack.append((False, child, depth + 1, child_path)) + else: + stack.extend((False, child, depth + 1, location) for child in current.value) + data = loader.construct_document(node) if node is not None else None + finally: + loader.dispose() except RecursionError as exc: raise ConfigurationError( f"Config file '{path}' exceeds the maximum nesting depth of {_CONFIG_MAX_DEPTH}." diff --git a/lib/python/base_cli/profile.py b/lib/python/base_cli/profile.py index bb1a467..5211929 100644 --- a/lib/python/base_cli/profile.py +++ b/lib/python/base_cli/profile.py @@ -10,6 +10,7 @@ BatteriesIncludedConfigLoader, ConfigSnapshot, load_yaml_file, + validate_discovered_config_path, ) from .context import Context from .history import display_command as _generic_history_display_command @@ -203,6 +204,9 @@ def batteries_included( environment_dir_name: str = "environments", discover_project: ProjectDiscovery | None = None, resolve_runtime: RuntimeResolver | None = None, + verify_discovered_config: bool = True, + max_project_ancestor_depth: int = 32, + project_boundary_marker: str | None = ".git", ) -> CliProfile: """Create an opt-in profile with conventional layered YAML config. @@ -211,6 +215,18 @@ def batteries_included( The generic profile remains convention-free; this method is the explicit adoption point for applications that want these conventions. """ + if ( + not isinstance(max_project_ancestor_depth, int) + or isinstance(max_project_ancestor_depth, bool) + or max_project_ancestor_depth < 0 + ): + raise ValueError("max_project_ancestor_depth must be a nonnegative integer") + if project_boundary_marker is not None and ( + not project_boundary_marker + or Path(project_boundary_marker).name != project_boundary_marker + or project_boundary_marker in {".", ".."} + ): + raise ValueError("project_boundary_marker must be a simple filename") normalized_name = normalize_cli_name(cli_name) if not normalized_name: raise ValueError("cli_name must contain a non-empty command name") @@ -221,8 +237,14 @@ def batteries_included( user_config_name=user_config_name, project_config_name=project_config_name, environment_dir_name=environment_dir_name, + verify_project_config=verify_discovered_config, + ) + project_discovery = discover_project or _conventional_project_discovery( + project_config_name, + trust=verify_discovered_config, + max_depth=max_project_ancestor_depth, + boundary_marker=project_boundary_marker, ) - project_discovery = discover_project or _conventional_project_discovery(project_config_name) def load_user_config() -> object | None: values = load_yaml_file(loader.user_config_path) @@ -261,17 +283,26 @@ def _discover_no_project(_cwd: Path) -> ProjectInfo | None: return None -def _conventional_project_discovery(config_name: str) -> ProjectDiscovery: +def _conventional_project_discovery( + config_name: str, *, trust: bool = True, max_depth: int = 32, boundary_marker: str | None = ".git" +) -> ProjectDiscovery: def discover(cwd: Path) -> ProjectInfo | None: current = cwd.expanduser().resolve() - for directory in (current, *current.parents): + device = current.stat().st_dev + visited: list[Path] = [] + for depth, directory in enumerate((current, *current.parents)): + if depth > max_depth or directory.stat().st_dev != device: + break + visited.append(directory) candidate = directory / config_name if candidate.is_file(): - return ProjectInfo( - root=directory, - manifest=candidate, - name=directory.name, - ) + if trust: + for component in visited: + validate_discovered_config_path(component) + validate_discovered_config_path(candidate) + return ProjectInfo(root=directory, manifest=candidate, name=directory.name) + if boundary_marker is not None and (directory / boundary_marker).exists(): + break return None return discover diff --git a/tests/test_config_discovery_trust.py b/tests/test_config_discovery_trust.py new file mode 100644 index 0000000..0b9a8f0 --- /dev/null +++ b/tests/test_config_discovery_trust.py @@ -0,0 +1,93 @@ +from __future__ import annotations + +import os +from pathlib import Path + +import pytest +from base_cli import CliProfile +from base_cli.config import ConfigurationError, load_yaml_file + + +def test_unsafe_ancestor_is_refused_and_optout_is_explicit(tmp_path: Path) -> None: + if os.name == "nt": + pytest.skip("POSIX ownership/mode contract") + project = tmp_path / "project" + child = project / "sub" + child.mkdir(parents=True) + config = project / ".base-cli.yaml" + config.write_text("keep_temp: true\n") + project.chmod(0o777) + try: + with pytest.raises(ConfigurationError, match="Untrusted.*project"): + CliProfile.batteries_included("trust").discover_project(child) + assert CliProfile.batteries_included("trust", verify_discovered_config=False).discover_project(child) + finally: + project.chmod(0o700) + config.chmod(0o666) + with pytest.raises(ConfigurationError, match="Untrusted.*base-cli"): + CliProfile.batteries_included("trust").discover_project(child) + + +def test_group_writable_owned_project_paths_are_accepted(tmp_path: Path) -> None: + if os.name == "nt": + pytest.skip("POSIX ownership/mode contract") + project = tmp_path / "project" + child = project / "sub" + child.mkdir(parents=True) + config = project / ".base-cli.yaml" + config.write_text("keep_temp: true\n") + project.chmod(0o775) + config.chmod(0o664) + try: + assert CliProfile.batteries_included("group-writable").discover_project(child) + finally: + config.chmod(0o600) + project.chmod(0o700) + + +def test_marker_and_depth_bound_discovery(tmp_path: Path) -> None: + (tmp_path / ".base-cli.yaml").write_text("environment: test\n") + child = tmp_path / "project" + child.mkdir() + (child / ".git").write_text("gitdir: elsewhere\n") + assert CliProfile.batteries_included("trust").discover_project(child) is None + assert CliProfile.batteries_included("trust", project_boundary_marker=None).discover_project(child) + assert ( + CliProfile.batteries_included( + "trust", project_boundary_marker=None, max_project_ancestor_depth=0 + ).discover_project(child) + is None + ) + assert CliProfile.generic().discover_project(tmp_path) is None + + +def test_yaml_input_size_is_bounded(tmp_path: Path) -> None: + path = tmp_path / "large.yaml" + path.write_bytes(b"value: " + b"x" * 1_048_576) + with pytest.raises(ConfigurationError, match="maximum size"): + load_yaml_file(path) + + +def test_project_environment_file_has_same_trust_gate(tmp_path: Path) -> None: + if os.name == "nt": + pytest.skip("POSIX permission contract") + (tmp_path / ".base-cli.yaml").write_text("environment: test\n") + environment = tmp_path / "environments" / "test.yaml" + environment.parent.mkdir() + environment.write_text("keep_temp: true\n") + environment.chmod(0o666) + profile = CliProfile.batteries_included("trust", user_config_dir=tmp_path / "user") + project = profile.discover_project(tmp_path) + with pytest.raises(ConfigurationError, match="Untrusted.*test.yaml"): + profile.load_config(project, None) + + +def test_merge_alias_expansion_is_bounded_before_construction(tmp_path: Path) -> None: + path = tmp_path / "aliases.yaml" + lines = ["level0: &level0 {key: value}"] + for depth in range(1, 8): + aliases = ", ".join([f"*level{depth - 1}"] * 10) + lines.append(f"level{depth}: &level{depth} {{<<: [{aliases}]}}") + path.write_text("\n".join(lines)) + with pytest.raises(ConfigurationError, match="expansion"): + load_yaml_file(path) diff --git a/tests/test_optional_yaml_dependency.py b/tests/test_optional_yaml_dependency.py index 34fc404..6a37242 100644 --- a/tests/test_optional_yaml_dependency.py +++ b/tests/test_optional_yaml_dependency.py @@ -72,7 +72,7 @@ def test_yaml_config_converts_parser_recursion_error(self) -> None: path = Path(tmpdir) / "config.yaml" path.write_text("answer: 42\n", encoding="utf-8") yaml = mock.Mock() - yaml.safe_load.side_effect = RecursionError("parser recursion") + yaml.SafeLoader.return_value.get_single_node.side_effect = RecursionError("parser recursion") yaml.YAMLError = type("YAMLError", (Exception,), {}) with mock.patch("base_cli.config.require_yaml", return_value=yaml): with self.assertRaisesRegex(ConfigurationError, r"maximum nesting depth of 64"):