Skip to content

Commit c010d11

Browse files
fix: address review feedback on RunState.load() TOCTOU fix
- Update docstring to describe the direct open() read instead of the removed exists() probe, keeping the security rationale accurate. - Add test_load_not_found_custom_message: verifies the custom 'Run state not found:' error message is raised. - Add test_load_not_found_no_exists_probe: patches builtins.open to raise FileNotFoundError and asserts the custom message, proving the TOCTOU-eliminated code path works correctly.
1 parent 7db94eb commit c010d11

2 files changed

Lines changed: 25 additions & 5 deletions

File tree

src/specify_cli/workflows/engine.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -733,11 +733,10 @@ def load(cls, run_id: str, project_root: Path) -> RunState:
733733
the lookup path. Without this guard, a caller passing a value like
734734
``../escape`` (e.g. via ``specify workflow resume`` CLI argument)
735735
would interpolate path-traversal segments into
736-
``runs_dir`` below, letting ``state_path.exists()`` probe arbitrary
737-
paths and ``json.load`` read attacker-planted JSON from outside
738-
the project's ``runs/`` directory. ``__init__`` already runs this
739-
check on the stored ``state_data["run_id"]``, but that fires
740-
*after* the file lookup — too late to prevent the disclosure.
736+
``runs_dir`` below, letting ``open()`` read attacker-planted JSON
737+
from outside the project's ``runs/`` directory. ``__init__`` already
738+
runs this check on the stored ``state_data["run_id"]``, but that
739+
fires *after* the file lookup — too late to prevent the disclosure.
741740
Mirrors the precedent in ``agents._ensure_within_directory``.
742741
"""
743742
cls._validate_run_id(run_id)

tests/test_workflows.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7086,6 +7086,27 @@ def test_load_not_found(self, project_dir):
70867086
with pytest.raises(FileNotFoundError):
70877087
RunState.load("nonexistent", project_dir)
70887088

7089+
def test_load_not_found_custom_message(self, project_dir):
7090+
"""Regression: RunState.load() must raise FileNotFoundError with
7091+
the custom 'Run state not found:' message, not a raw OSError."""
7092+
from specify_cli.workflows.engine import RunState
7093+
7094+
with pytest.raises(FileNotFoundError, match=r"Run state not found:.*nonexistent"):
7095+
RunState.load("nonexistent", project_dir)
7096+
7097+
def test_load_not_found_no_exists_probe(self, project_dir):
7098+
"""Regression: RunState.load() must not call exists() before open(),
7099+
so it cannot be tricked by a TOCTOU race where the file disappears
7100+
between the check and the read."""
7101+
from unittest.mock import patch
7102+
from specify_cli.workflows.engine import RunState
7103+
7104+
# Verify that a nonexistent state.json raises FileNotFoundError
7105+
# with the custom message even when we can't check exists() first.
7106+
with patch("builtins.open", side_effect=FileNotFoundError):
7107+
with pytest.raises(FileNotFoundError, match="Run state not found:"):
7108+
RunState.load("nonexistent", project_dir)
7109+
70897110
def test_load_rejects_stored_run_id_mismatch(self, project_dir):
70907111
"""The state payload cannot redirect later writes to another run."""
70917112
from specify_cli.workflows.engine import RunState

0 commit comments

Comments
 (0)