[bug-fix] Harden persisted workflow run artifact access against symlinks by github-actions[bot] · Pull Request #4930 · github/spec-kit

github-actions · 2026-10-12T14:28:13Z

Bug fix — persisted-run-symlinks

Proposed fix for issue #4914, applying the remediation from the bug assessment.

Verdict: valid · Severity: medium

Summary

Introduces a shared persisted-run storage boundary that rejects symlinked run directories and artifact files before workflow state, input, snapshot, or log access. Resume, status, run listing, and snapshot creation now fail closed consistently, with the policy documented for workflow users and maintainers.

Changes

File	Change	Notes
`src/specify_cli/workflows/engine.py`	modified	Added shared run-directory/artifact validation and no-follow artifact opens; routed state load/save, input fallback, log writes, workflow snapshot creation, resume snapshot reads, and run listing through it.
`src/specify_cli/workflows/command_status.py`	modified	Converts unsafe storage failures from run listing into clean CLI errors.
`tests/specify_cli/workflows/test_run_state_storage.py`	added test	Covers symlinked run directories and state, input, workflow snapshot, and log artifacts for reads and writes.
`workflows/README.md`	modified	Documents the persisted-run symlink policy.
`workflows/ARCHITECTURE.md`	modified	Documents the storage boundary and no-follow behavior.

Tests Added or Updated

tests/specify_cli/workflows/test_run_state_storage.py::test_load_rejects_symlinked_run_directory — rejects redirected per-run directories.
tests/specify_cli/workflows/test_run_state_storage.py::test_run_state_rejects_symlinked_persisted_artifacts — rejects redirected state and input artifacts.
tests/specify_cli/workflows/test_run_state_storage.py::test_run_state_rejects_symlinked_artifact_writes — rejects redirected state, input, and log writes.
tests/specify_cli/workflows/test_run_state_storage.py::test_workflow_snapshot_artifact_is_checked — rejects redirected workflow snapshots.

Local Verification

Commands run: python3 -m compileall -q src/specify_cli/workflows/engine.py src/specify_cli/workflows/command_status.py tests/specify_cli/workflows/test_run_state_storage.py → passed.
Commands run: git diff --check → passed.
Targeted pytest command → not run because pytest is unavailable in the environment; no dependencies were installed.

Deviations from Assessment

The current branch has no workflow definition command, so no command-specific change was possible there; persisted snapshot access is covered in the shared engine layer.
The implementation uses O_NOFOLLOW where available plus immediate symlink checks; no concurrent mutation test was added because the targeted test runner is unavailable.

Risks & Review Notes

Existing workflows that intentionally relocate run storage through symlinks will now be rejected as an intentional compatibility trade-off.
Filesystem behavior around concurrent replacement remains platform-dependent; no-follow leaf opens reduce this risk where supported, while the explicit policy rejects static symlinked paths.

Refs #4914 · cc @markuswondrak

Fixes [Bug]: Harden persisted workflow run artifact access against symlinks #4914

Generated by 🛠️ Fix Bug from Labeled Issue for #4914 · copilot · gpt52codex · 6.54 AIC · ⌖ 0.836 AIC · ⊞ 24.4K · ◷

Apply the remediation from the bug assessment on issue #4914. Refs #4914 Assisted-by: GitHub Copilot (model: gpt-5.2-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Snapshot reads retain a symlink race, Ruff fails on an unused variable, and key consumer regressions remain untested.

4 open findings

Remove unused run directory assignment · New
Prevent symlink race when reopening workflow snapshots · New
Test safe list-runs behavior with symlinked storage · New
Add resume and execute symlink regression tests · New

What changed in this PR

Adds a shared security boundary for persisted workflow-run artifacts to reject symlink redirection.

Changes:

Validates run directories and artifacts before access.
Adds no-follow file opening and CLI error handling.
Documents and tests the symlink policy.

File	Description
`src/specify_cli/workflows/engine.py`	Implements storage validation and protected artifact access.
`src/specify_cli/workflows/command_status.py`	Handles unsafe listing failures.
`tests/specify_cli/workflows/test_run_state_storage.py`	Adds symlink rejection tests.
`workflows/README.md`	Documents user-facing policy.
`workflows/ARCHITECTURE.md`	Documents the storage boundary.

🧠 Review effort: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot · 2026-10-12T19:37:36Z

+        err = cli._error_console(json_output)
+        try:
+            runs = engine.list_runs()
+        except (OSError, ValueError) as exc:
+            err.print(f"[red]Error:[/red] {cli._escape_markup(str(exc))}")


Added workflow status --json regressions for symlinked run directories and state.json, asserting clean failure and stderr-only output. Fixed in 79f1da7.

The workflow status --json listing regressions for symlinked run directories and state.json are present and pass on the current branch (02159bf0).

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

Copilot

🟡 Changes recommended

A parent-directory replacement race remains, and an existing unreadable-state regression test is broken.

3 open findings

3 resolved since last review

🧠 Review effort: Balanced

Copilot · 2026-10-12T21:05:33Z

            if state_path.exists():
                try:
-                    with open(state_path, encoding="utf-8") as f:
+                    with self._open_artifact(state_path, "r", encoding="utf-8") as f:


The unreadable-state regression now patches RunState._open_artifact and still asserts that the bad row is skipped; it passes on the current branch (02159bf0).

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

Copilot

🟡 Changes recommended

The unconditional Unix-specific filesystem requirements break persisted workflow operations on supported Windows systems.

3 open findings

1 resolved since last review

Prevent run-directory symlink races during artifact access

🧠 Review effort: Balanced

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

Close artifact handles in the symlink regression and explain cleanup failure handling. Forward directory descriptors through checkpoint test mocks to preserve failure and interrupt coverage. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a8a13d-1542-470f-87b8-5a3a1dc122f5

Exercise native and forced portable storage access in run-directory replacement regressions. Expect safe rejection without directory descriptors and assert that external artifacts remain untouched. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a8a13d-1542-470f-87b8-5a3a1dc122f5

Copilot

🟡 Changes recommended

Descriptor opening can still follow concurrently replaced ancestor symlinks, and dangling run-root symlinks can produce successful empty listings.

3 open findings

1 resolved since last review

Persisted workflow operations fail on Windows

🧠 Review effort: Balanced

Walk storage ancestors using descriptor-relative no-follow opens and use directory descriptors for creation and listing. Reject dangling run-root symlinks and add ancestor replacement regressions while preserving the portable fallback. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a8a13d-1542-470f-87b8-5a3a1dc122f5

Copilot

🟡 Changes recommended

The portable write fallback weakens state and input file permissions, potentially exposing persisted workflow data.

3 open findings

1 resolved since last review

Walk path components safely to prevent symlink escape

Previously missed (1)

In code that hasn't changed since last review

Narrow symlink-free tree claim to access-time checks

workflows/README.md:527

This promises that every symlink anywhere below runs is rejected, but the implementation checks only storage ancestors and one of the four known artifacts when that path is accessed; it does not scan unrelated entries (or an unused inputs.json). Narrow this statement to the access-time policy so users are not told that the whole tree is proactively symlink-free.

🧠 Review effort: Balanced

+                with tmp_path.open(
+                    "x", encoding="utf-8", errors="backslashreplace"
+                ) as f:


Create portable atomic-write temporary files with explicit 0600 permissions. Cover temporary and final state/input modes under a permissive POSIX umask, including replacement of existing files. Clarify that symlink rejection is enforced on accessed storage paths, not a proactive tree scan. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a8a13d-1542-470f-87b8-5a3a1dc122f5

Fix persisted-run-symlinks: reject symlinked workflow artifacts

1650af6

Apply the remediation from the bug assessment on issue #4914. Refs #4914 Assisted-by: GitHub Copilot (model: gpt-5.2-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions Bot added automated bug-fix Trigger the bug-fix agentic workflow labels Oct 12, 2026

github-actions Bot mentioned this pull request Oct 12, 2026

[Bug]: Harden persisted workflow run artifact access against symlinks #4914

Open

3 tasks

github-code-quality Bot found potential problems Oct 12, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py Fixed

mnriem requested a balanced review from Copilot October 12, 2026 14:44

Copilot started reviewing on behalf of mnriem October 12, 2026 14:45 View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

mnriem marked this pull request as ready for review October 12, 2026 19:32

mnriem self-requested a review as a code owner October 12, 2026 19:32

Copilot started work on behalf of mnriem October 12, 2026 19:32 View session

Fix persisted workflow snapshot symlinks

79f1da7

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

Copilot finished work on behalf of mnriem October 12, 2026 19:38

mnriem requested a balanced review from Copilot October 12, 2026 20:05

Copilot started reviewing on behalf of mnriem October 12, 2026 20:06 View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Copilot started work on behalf of mnriem October 12, 2026 20:14 View session

Secure workflow artifacts against run-directory swaps

bd3507f

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

Copilot finished work on behalf of mnriem October 12, 2026 20:23

mnriem requested a balanced review from Copilot October 12, 2026 20:48

Copilot started reviewing on behalf of mnriem October 12, 2026 20:49 View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py Outdated

Copilot started work on behalf of mnriem October 12, 2026 20:57 View session

Support workflow persistence without dir-fd APIs

02159bf

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>

github-code-quality Bot found potential problems Oct 12, 2026

View reviewed changes

Comment thread tests/specify_cli/workflows/test_run_state_storage.py Fixed

Comment thread src/specify_cli/workflows/engine.py Fixed

Copilot finished work on behalf of mnriem October 12, 2026 21:06

mnriem added the bug-test label Oct 12, 2026

mnriem requested a balanced review from Copilot October 13, 2026 16:46

Copilot started reviewing on behalf of mnriem October 13, 2026 16:47 View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py Outdated

mnriem requested a balanced review from Copilot October 13, 2026 18:35

Copilot started reviewing on behalf of mnriem October 13, 2026 18:36 View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py Outdated

Comment on lines +960 to +962

with tmp_path.open(

"x", encoding="utf-8", errors="backslashreplace"

) as f:

Navigation Menu

Repository navigation

[bug-fix] Harden persisted workflow run artifact access against symlinks - #4930

[bug-fix] Harden persisted workflow run artifact access against symlinks#4930
github-actions[bot] wants to merge 8 commits into
mainfrom
fix/4914-persisted-run-symlinks-2a3452971b7ee167

github-actions Bot commented Oct 12, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 12, 2026

Uh oh!

Copilot AI Oct 12, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Oct 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

github-actions Bot commented Oct 12, 2026

Bug fix — persisted-run-symlinks

Summary

Changes

Tests Added or Updated

Local Verification

Deviations from Assessment

Risks & Review Notes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants