From 01e0455f254e2f605424db49a307fbfa5cd95856 Mon Sep 17 00:00:00 2001 From: Markus Date: Thu, 16 Jul 2026 14:47:38 +0200 Subject: [PATCH] fix(workflows): reject symlinked overlay directories in layer sources Address PR #3557 review comments r3594064534 and r3594064563: - ProjectOverlaySource.collect now rejects symlinked per-workflow overlay directories (.specify/workflows/overlays/) before iterating - InstalledOverlaySource.collect now rejects symlinked installed overlay directories (.specify/workflows//overlays) before iterating - workflow_overlay_list catches ValueError from resolver and exits with code 1 instead of crashing on unhandled exceptions - Added .specify/workflows/overlays to _reject_unsafe_workflow_storage chokepoint for defense-in-depth These guards prevent symlinked overlay directories from redirecting auto-loaded overlay YAML to attacker-controlled content outside the project, which could inject executable shell steps into trusted workflows. Refs: PR #3557 review comments r3594064534, r3594064563 Assisted-by: opencode-go/qwen3.7-max (autonomous) --- src/specify_cli/workflows/_commands.py | 7 +- .../workflows/overlays/_commands.py | 10 ++- .../workflows/overlays/layer_sources.py | 10 +++ tests/workflows/test_overlay_security.py | 84 +++++++++++++++++++ tests/workflows/test_resolver_integration.py | 79 +++++++++++++++++ 5 files changed, 186 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 384f08c5c..0a5a43fbc 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -109,6 +109,10 @@ def _reject_unsafe_workflow_storage(project_root: Path) -> None: project_root / ".specify" / "workflows" / "runs", ".specify/workflows/runs", ) + _reject_unsafe_dir( + project_root / ".specify" / "workflows" / "overlays", + ".specify/workflows/overlays", + ) _WORKFLOW_ID_PATTERN = re.compile(r"^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$") @@ -1818,7 +1822,8 @@ def workflow_overlay_list_cmd( from .overlays._commands import workflow_overlay_list project_root = _require_specify_project() - workflow_overlay_list(project_root, workflow_id) + if workflow_overlay_list(project_root, workflow_id) is None: + raise typer.Exit(1) @workflow_app.command("resolve") diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index bccb7872b..3686d02e3 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -284,14 +284,18 @@ def workflow_overlay_remove( return True -def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str, Any]]: +def workflow_overlay_list(project_root: Path, workflow_id: str) -> list[dict[str, Any]] | None: """List all overlays for a workflow and print a summary table. - Returns the raw list data for machine-readable callers. + Returns the raw list data for machine-readable callers, or None on error. """ _validate_workflow_id_or_exit(workflow_id) resolver = WorkflowResolver(project_root) - layers = resolver.collect_all_layers(workflow_id) + try: + layers = resolver.collect_all_layers(workflow_id) + except ValueError as exc: + err_console.print(f"[red]Error:[/red] {exc}") + return None overlays = [layer for layer in layers if layer.tier != "base"] if not overlays: diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index 8f4082c61..a5d76f14a 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -42,6 +42,11 @@ class ProjectOverlaySource: def collect(self, workflow_id: str) -> list[Layer]: """Collect all project-local overlays for the given workflow id.""" workflow_overlay_dir = self.overlays_dir / workflow_id + if workflow_overlay_dir.is_symlink(): + raise OverlayLoadError( + workflow_overlay_dir, + ["Symlinked overlay directories are not allowed"], + ) if not workflow_overlay_dir.is_dir(): return [] layers: list[Layer] = [] @@ -82,6 +87,11 @@ class InstalledOverlaySource: def collect(self, workflow_id: str) -> list[Layer]: """Collect all installed overlays shipped with the given workflow.""" installed_overlay_dir = self.workflows_dir / workflow_id / "overlays" + if installed_overlay_dir.is_symlink(): + raise OverlayLoadError( + installed_overlay_dir, + ["Symlinked overlay directories are not allowed"], + ) if not installed_overlay_dir.is_dir(): return [] layers: list[Layer] = [] diff --git a/tests/workflows/test_overlay_security.py b/tests/workflows/test_overlay_security.py index 3871d8921..e41b7ff8d 100644 --- a/tests/workflows/test_overlay_security.py +++ b/tests/workflows/test_overlay_security.py @@ -202,3 +202,87 @@ class TestOverlayPathTraversal: result = runner.invoke(app, ["workflow", "overlay", "enable", "wf", "../other"]) assert result.exit_code != 0, result.output assert "invalid" in result.output.lower() or "traversal" in result.output.lower() + + def test_overlay_rejects_symlinked_overlays_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay commands must reject a symlinked .specify/workflows/overlays directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a symlinked overlays directory pointing outside the project + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + overlays_dir = project_dir / ".specify" / "workflows" / "overlays" + overlays_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() + + def test_overlay_list_rejects_symlinked_per_workflow_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay list must reject a symlinked per-workflow overlay directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a real overlay directory outside the project. + outside_dir = tmp_path / "outside_wf" + outside_dir.mkdir() + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the per-workflow overlay directory to the outside location. + overlays_root = project_dir / ".specify" / "workflows" / "overlays" + overlays_root.mkdir(parents=True, exist_ok=True) + symlink_dir = overlays_root / "wf" + symlink_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() + + def test_overlay_list_rejects_symlinked_installed_overlays_dir(self, project_dir, monkeypatch, tmp_path): + """Overlay list must reject a symlinked installed overlays directory.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + + # Create a real overlays directory outside the project. + outside_dir = tmp_path / "outside_installed" + outside_dir.mkdir() + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "echo"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Create a workflow directory and symlink its overlays/ to the outside. + wf_dir = project_dir / ".specify" / "workflows" / "wf" + wf_dir.mkdir(parents=True, exist_ok=True) + symlink_dir = wf_dir / "overlays" + symlink_dir.symlink_to(outside_dir) + + result = runner.invoke(app, ["workflow", "overlay", "list", "wf"]) + assert result.exit_code != 0, result.output + assert "symlink" in result.output.lower() diff --git a/tests/workflows/test_resolver_integration.py b/tests/workflows/test_resolver_integration.py index cb14ec340..b7ee052ef 100644 --- a/tests/workflows/test_resolver_integration.py +++ b/tests/workflows/test_resolver_integration.py @@ -453,6 +453,85 @@ class TestWorkflowResolver: with pytest.raises(ValueError, match="Composed workflow is invalid"): resolver.resolve("wf") + def test_resolve_rejects_symlinked_project_overlay_dir(self, project_dir, tmp_path): + """ProjectOverlaySource must reject a symlinked per-workflow overlay directory.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + # Create a real overlay directory outside the project with a malicious overlay. + outside_dir = tmp_path / "outside_overlays" / "wf" + outside_dir.mkdir(parents=True, exist_ok=True) + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the per-workflow overlay directory to the outside location. + overlays_root = project_dir / ".specify" / "workflows" / "overlays" + overlays_root.mkdir(parents=True, exist_ok=True) + symlink_dir = overlays_root / "wf" + symlink_dir.symlink_to(outside_dir) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): + resolver.resolve("wf") + + def test_resolve_rejects_symlinked_installed_overlay_dir(self, project_dir, tmp_path): + """InstalledOverlaySource must reject a symlinked installed overlays directory.""" + data = { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "speckit.specify"}], + } + _write_workflow(project_dir, "wf", data) + + # Create a real overlays directory outside the project with a malicious overlay. + outside_dir = tmp_path / "outside_installed" + outside_dir.mkdir(parents=True, exist_ok=True) + outside_dir.joinpath("evil.yml").write_text( + yaml.safe_dump( + { + "id": "evil", + "extends": "wf", + "priority": 100, + "edits": [ + { + "operation": "insert_after", + "anchor": "a", + "step": {"id": "evil-step", "type": "command", "command": "rm -rf /"}, + } + ], + } + ), + encoding="utf-8", + ) + + # Symlink the installed overlays directory to the outside location. + wf_dir = project_dir / ".specify" / "workflows" / "wf" + symlink_dir = wf_dir / "overlays" + symlink_dir.symlink_to(outside_dir) + + resolver = WorkflowResolver(project_dir) + with pytest.raises(ValueError, match="Symlinked overlay directories are not allowed"): + resolver.resolve("wf") + def test_engine_load_workflow_uses_resolver(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine