From 12d985b76b4418cf8d3d11a314043d788a66c01a Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 09:51:49 +0200 Subject: [PATCH] Fix disabled-workflow bypass via symlinked .specify project root workflow run's ownership check derived registry_root/registered_id from the lexical path, then checked the id directory and workflow.yml leaf for symlinks -- but never checked .specify or .specify/workflows themselves for that derived root. _reject_unsafe_workflow_storage only guards the cwd's project_root, which can differ from the path-derived registry_root (a direct path into an unrelated project, or that project's own .specify being a symlink to an attacker-controlled tree). WorkflowRegistry's own symlinked-parent handling silently substitutes an empty registry instead of raising, so a query against it (is_installed/get returning "not found") is not a safety signal a caller can rely on: with a symlinked .specify, the disabled check saw no registry entry and let a disabled workflow run anyway. Fix: reject an unsafe .specify/.specify-workflows for the actual derived registry_root before ever consulting the registry, reusing the existing _reject_unsafe_dir helper already used by _reject_unsafe_workflow_storage. Red-first end-to-end repro: victim project's .specify symlinked to an attacker-controlled tree containing a disabled workflow entry, run invoked with a direct path from an unrelated cwd -- confirmed the disabled workflow executed (exit 0) before the fix, now refused cleanly. Tests: tests/test_workflows.py 475 passed. Full suite: 3977 passed, 110 skipped. ruff check: all checks passed. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 14 +++++++ tests/test_workflows.py | 54 ++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 267aea757..d35689525 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -414,6 +414,20 @@ def workflow_run( if parts[i] == ".specify" and parts[i + 1] == "workflows": registry_root = Path(*parts[:i]) if i else Path(lexical.anchor or ".") registered_id = parts[i + 2] + # The path-derived registry_root here may differ from the + # cwd's project_root already checked by + # _reject_unsafe_workflow_storage above (e.g. this path + # points into another project entirely, or this project's + # own .specify is itself a symlink to an attacker-controlled + # tree) -- check it explicitly rather than trusting that + # cwd-scoped guard, and don't rely on WorkflowRegistry's own + # symlinked-parent handling below (it silently substitutes + # an empty registry instead of raising, so a query against + # it can't be trusted as a safety signal here). + _reject_unsafe_dir(registry_root / ".specify", ".specify") + _reject_unsafe_dir( + registry_root / ".specify" / "workflows", ".specify/workflows" + ) # A legitimately installed workflow's own directory tree # never contains a symlink (workflow add/remove both refuse # one at install time); one appearing here means the file diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 0fce0d968..e0cd927f2 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8851,6 +8851,60 @@ steps: assert result.exception is None or isinstance(result.exception, SystemExit) assert "disabled" in result.output or "symlink" in result.output.lower() + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_run_refuses_symlinked_specify_dir_hiding_disabled_workflow( + self, temp_dir, monkeypatch + ): + """A victim project's own .specify directory being a symlink to an + attacker-controlled tree must not bypass the disabled-workflow guard. + _reject_unsafe_workflow_storage only checks the *cwd's* project root + (unrelated here); the id/leaf symlink-component loop only checks + components from the id directory onward, missing .specify/ + .specify/workflows themselves. The ownership check must reject an + unsafe .specify/.specify-workflows for the actual path-derived + registry root before ever consulting the registry -- it must not + rely on WorkflowRegistry's own symlinked-parent handling, which + silently returns an empty registry instead of raising and so is not + a safety signal a caller can depend on.""" + from typer.testing import CliRunner + from specify_cli import app + + victim = temp_dir / "victim" + victim.mkdir() + attacker_real = temp_dir / "attacker-real" + (attacker_real / "workflows" / "evil").mkdir(parents=True) + (attacker_real / "workflows" / "evil" / "workflow.yml").write_text( + self.WORKFLOW_YAML.format(version="1.0.0"), encoding="utf-8" + ) + (attacker_real / "workflows" / "workflow-registry.json").write_text( + json.dumps( + { + "schema_version": "1.0", + "workflows": { + "evil": { + "name": "Evil", + "version": "1.0.0", + "source": "dev", + "enabled": False, + } + }, + } + ), + encoding="utf-8", + ) + (victim / ".specify").symlink_to(attacker_real) + + unrelated_cwd = temp_dir / "unrelated-cwd" + unrelated_cwd.mkdir() + monkeypatch.chdir(unrelated_cwd) + + runner = CliRunner() + target = victim / ".specify" / "workflows" / "evil" / "workflow.yml" + result = runner.invoke(app, ["workflow", "run", str(target)]) + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "symlink" in result.output.lower() + def test_disable_shows_marker_in_list(self, project_dir, monkeypatch): from typer.testing import CliRunner from specify_cli import app