mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
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>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user