Skip to content

Add scoped FileHelper foundation for filesystem policy - #4908

Open
mnriem wants to merge 5 commits into
github:mainfrom
mnriem:mnriem-filehelper-foundation
Open

mnriem wants to merge 5 commits into
github:mainfrom
mnriem:mnriem-filehelper-foundation

Conversation

@mnriem

@mnriem mnriem commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Introduce a small filesystem-owning FileHelper foundation so future CLI migrations can enforce an explicit, scoped filesystem policy consistently instead of duplicating path checks around individual pathlib/shutil calls. This PR adds three files: src/specify_cli/file_helper.py, tests/test_file_helper.py, and design/file-helper.md. It also modifies src/specify_cli/integration_status.py to import the shared Windows extended-length prefix normalizer instead of duplicating it. Existing command policies, initialization, registration, and shared-infrastructure behavior are unchanged; no command subsystem is migrated here.

FileHelper(root, allow_symlinks=False) owns directory creation, byte/text reads, exclusive file creation, atomic file writes/upserts, deletion, and explicit symlink creation. Its policy is immutable and independent of Typer or console output.

  • Deny mode rejects accessed parent and leaf symlinks, including dangling links, without rejecting unrelated project links.
  • Allow mode follows only contained targets for reads/updates. Atomic updates replace the resolved file target while preserving the symlink chain.
  • Allowed leaf-link deletion unlinks the link itself, including dangling/external/cyclic links, never its target. Deletion through a linked parent addresses the contained real entry.
  • Recursive deletion preflights the selected tree before mutation and never descends through descendant symlinks.
  • Windows rooted-relative and drive-relative operation paths and link targets are rejected before walking. Directory junctions and other non-symlink reparse entries below the established root are explicitly unsupported in both modes, including deletion.

Extended-length Windows drive/UNC spellings are compared with their ordinary equivalents without resolving away the accessed hierarchy. UNC namespace markers are matched case-insensitively, preserving the remaining path spelling and rejecting external targets. The existing integration-status import name is retained and covered by its compatibility tests.

The caller-established root alias is trusted, including OS/worktree aliases and .. in that trusted prefix; operation-relative .. remains rejected. Portable checks are not a race-free sandbox, do not constrain subprocesses/third-party code, and do not guarantee rollback after I/O failures or crash durability.

Deferred to later reviewed changes: only specify init will expose/persist the project setting in .specify/init-options.json; other project commands will read current configuration on each invocation. There is no global flag or per-command override. Strict persisted-policy loading, configuration-read bootstrap semantics, and command migrations are not implemented here.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

Local validation was executed on macOS with this worktree's own virtual environment. The full suite uses the repository's documented worktree interpreter rather than the template's literal uv sync && uv run pytest wrapper, so that wrapper-specific checkbox remains unchecked.

Exact command Result
uv sync --extra test Passed for initial setup; restored the initially missing worktree environment without tracked dependency changes.
UV_NO_SYNC=1 UV_PROJECT_ENVIRONMENT="$PWD/.venv" uv run specify --help Passed after the exclusive-create correction using the worktree environment.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k 'windows_anchored_relative_forms_rejected_portably or directory_reparse_entries_rejected_before_traversal_portably' All eight selected Windows confinement regressions failed before their fix (DID NOT RAISE) and passed afterward.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k mixed_case_unc Before the UNC correction: 6 failed, 6 passed. After: 12 passed, 174 deselected at that checkpoint; path spelling and external-target rejection preserved.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k 'rejects_existing_leaf_before_open or rejects_dangling_leaf_with_windows_backend' Before the exclusive-create correction: 8 failed. After: 8 passed, 190 deselected; links/targets preserved, no backend opening of existing leaves or unintended target creation.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/specify_cli/integrations/test_command_status.py tests/test_shared_infra_lock.py tests/test_shared_infra_gitignore.py tests/test_shared_infra_integrity.py tests/test_registrar_path_traversal.py -q --cov=specify_cli.file_helper --cov-report=term-missing 299 passed, 53 skipped; 100% helper statement coverage after the exclusive-create correction.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q 9,833 passed, 72 skipped, 62 warnings in 553.40 seconds; 9,905 cases collected.
uvx --offline ruff@0.15.0 check src tests All checks passed with the CI-pinned Ruff version, including after full-suite completion.
.venv/bin/python -m compileall -q src/specify_cli/file_helper.py tests/test_file_helper.py Passed.
git diff --check Passed.

Initial baseline collection excluding the new module was 9,707; the current local full run collects 9,905, including 198 helper cases. No existing coverage was removed. The original collision test now labels ordinary-file, live-link, and dangling-link cases individually in both policy modes rather than hiding the failing leaf in a loop.

Coverage includes plain CRUD in both modes; leaf/intermediate internal and external links; atomic target and link preservation; exclusive-create collisions; dangling links; cycles; recursive preflight without partial deletion; special objects; trusted root aliases and operation-suffix escapes; unrelated links; permission/I/O failures; temporary cleanup; changed-path rechecks; and portable Windows path/reparse classification.

Windows CI evidence and limitation: .github/workflows/test.yml runs uv sync --extra test and uv run pytest on all three OSes with Python 3.13/3.14. Completed logs for preceding head ae29ace7a591465ebd9fadd854337a6211fd4422 in run 37999037692 show all 53 Windows-only helper cases passed on both Windows versions, with zero native-case skips. However, the generic allow-mode exclusive-create collision failed: the Windows 3.13 job failed overall, and the Windows 3.14 job was canceled by fail-fast after recording the same failure. Neither is claimed as a successful full Windows validation.

The exclusive-create correction explicitly checks existing leaves with no-follow lstat before opening, preserving FileExistsError/EEXIST, deny-mode rejection, contained parent traversal, and available O_EXCL/O_NOFOLLOW flags. Portable regressions reproduce a Windows-style dangling-target open fallback; this is not native execution or a race-free guarantee. Fresh-head Windows validation of this correction remains pending.

On non-Windows hosts, those 53 native cases are intentionally skipped: the completed Ubuntu Python 3.13 job at the preceding head has 133 helper passes and exactly those 53 skips; the current local macOS run also skips them. The fixtures additionally skip when actual symlink/junction creation is unavailable. Native cases include real extended-path symlink read/update/create and real contained/external mklink /J junctions; portable UNC parsing does not claim native network-share coverage. Manual sample-project testing was not run.

The coordinator independently reviewed the local correction and ran LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k exclusive_create: 14 passed, 184 deselected; git diff --check passed. All such review/testing is automated evidence, not human review.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot using GPT-6.1 Sol (model ID gpt-6.1-sol) acted in autonomous/autopilot mode on behalf of @mnriem. The reasoning-effort setting is not independently available in this session. Copilot authored the helper code, tests, contract documentation, checkpoint commits, review corrections, and this PR description; it executed the local checks and inspected completed CI logs. A separate coordinating Copilot session supplied automated review/testing. Native Windows evidence comes from the cited GitHub Actions jobs, not this macOS session; no human review or local Windows execution is claimed. Each commit also carries its own Assisted-by, Co-authored-by, and Copilot-Session disclosures.

Add filesystem-owning primitives with explicit symlink policy, scoped containment, atomic target updates, recursive deletion preflight, and Windows path/reparse rejection.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e
Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Windows extended-length symlink targets can be incorrectly rejected as escaping the configured root.

1 open finding
What changed in this PR

Introduces a root-scoped filesystem policy foundation for future CLI migrations.

Changes:

  • Adds immutable FileHelper operations and policy errors.
  • Documents filesystem and symlink behavior.
  • Adds extensive positive, negative, and platform-specific tests.
File Description
src/​specify_cli/​file_helper.py Implements scoped filesystem operations.
tests/​test_file_helper.py Tests CRUD, confinement, symlinks, and Windows behavior.
design/​file-helper.md Defines policy, API, and safety limits.

🧠 Review effort: Balanced


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

Comment thread src/specify_cli/file_helper.py
Reuse the existing drive/UNC prefix normalization for root comparisons while retaining accessed hierarchy evidence and rejecting escaping or unsupported targets. Add portable regression and native Windows operation cases.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e
Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:32
@mnriem

mnriem commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the Windows extended-length target finding in commit 369fa47c0410054e0aa46703b59b834adbc6f4b3.

FileHelper now compares ordinary and extended-length drive/UNC spellings on both the target and root sides. The normalization utility was moved from integration_status.py to the CLI-independent helper module and imported back under its existing name, avoiding duplicated logic. Original paths and .. components are retained for the scoped walk; external targets, drive/root-relative forms, and unsupported device namespaces remain rejected. POSIX filenames are not reinterpreted as Windows paths.

Regression evidence: LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k windows_extended reproduced five failures before the fix and passed afterward: 16 passed, four native-Windows skips.

Final validation: LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/specify_cli/integrations/test_command_status.py tests/test_shared_infra_lock.py tests/test_shared_infra_gitignore.py tests/test_shared_infra_integrity.py tests/test_registrar_path_traversal.py -q --cov=specify_cli.file_helper --cov-report=term-missing — 265 passed, 53 native-Windows skips; 100% helper statement coverage. Test collection increased from 9,850 to 9,871, with no removals. Native Windows filesystem execution and the full suite were not run.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol) in autonomous/autopilot mode. The reasoning-effort setting is not independently available in this session. Copilot authored this correction, its tests/documentation, and this review response, and executed the reported checks. No human or native-Windows validation is claimed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Absolute operations derived from a trusted root containing .. are incorrectly rejected.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Rejects valid paths when the configured root contains ..

src/​specify_cli/​file_helper.py:145

Path.absolute() preserves .. components in the supplied root, so a valid helper created from a root such as ../project stores that spelling in self.root. Passing files.root / "child" (an absolute path beneath the trusted supplied root) is then rejected here before _parts() can remove the root prefix. Derive the root-relative parts first and reject .. only in that operation-relative result; please also cover a root spelling containing ...

🧠 Review effort: Balanced

Derive operation-relative parts before rejecting parent traversal so paths built from trusted roots containing parent components remain usable. Preserve lexical root evidence and reject traversal in operation suffixes; add positive and negative regression coverage.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e
@mnriem

mnriem commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the Previously missed (1) finding in review 5475578335, despite the overview reporting zero open findings. Correction: a4c857807e88120845722c12e4c440d106da7631.

_path() now derives the operation-relative components before checking for ... This allows absolute paths built from a caller-established root spelling such as anchor/../project, while still rejecting parent traversal in the operation suffix. The original root spelling is retained rather than lexically normalized, preserving trusted-alias semantics and accessed link evidence.

Added actual CRUD success cases for both policy modes and absolute/relative root spellings, root-deletion protection, rejected suffix traversal without mutation across read/create/write/mkdir/delete, and POSIX root-alias/parent-traversal selection checks.

Validation:

  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k 'trusted_parent_traversal_root or trusted_root_parent_traversal': before the fix, six failed and four passed; afterward, all ten passed.
  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/specify_cli/integrations/test_command_status.py tests/test_shared_infra_lock.py tests/test_shared_infra_gitignore.py tests/test_shared_infra_integrity.py tests/test_registrar_path_traversal.py -q --cov=specify_cli.file_helper --cov-report=term-missing: 275 passed, 53 native-Windows skips; 100% helper statement coverage.
  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest --collect-only -q: 9,881 collected, up ten from 9,871 without removals.
  • .venv/bin/python -m compileall -q src/specify_cli/file_helper.py tests/test_file_helper.py and git diff --check: passed.

Native Windows execution and the full suite were not run. Two new POSIX symlink/parent-traversal cases explicitly skip on Windows rather than claiming identical OS path semantics.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol) in autonomous/autopilot mode. The reasoning-effort setting is not independently available in this session. Copilot authored the fix, tests, documentation, checkpoint commit, and this response, and executed the reported checks. No human or native-Windows validation is claimed.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 21:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Mixed-case extended UNC prefixes can incorrectly reject contained Windows paths, and the PR description omits a changed file.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Medium severity Match UNC namespace prefixes case-insensitively

src/​specify_cli/​file_helper.py:18

The UNC marker is matched case-sensitively, but Windows paths (including this namespace component) are case-insensitive. A contained target such as \\?\unc\server\share\project\file therefore raises PathEscapeError, while the equivalent uppercase spelling succeeds; PureWindowsPath also treats those spellings as equal. Match the \\?\UNC\ prefix case-insensitively while slicing the original string, and add a mixed-case portable regression.

Low severity Update PR description to document integration_status.py changes

src/​specify_cli/​integration_status.py:11

The PR description says this change adds only three files and leaves existing files unchanged, but this import changes a fourth file by moving its normalizer into file_helper. Update the description/change inventory to include integration_status.py and explain the shared normalization extraction introduced by the follow-up fix.

🧠 Review effort: Balanced

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e
Copilot AI balanced review requested due to automatic review settings October 9, 2026 22:24
@mnriem

mnriem commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed both Previously missed items in review 5475685859.

Commit ae29ace7a591465ebd9fadd854337a6211fd4422 makes Windows extended UNC namespace matching case-insensitive while preserving the remaining path spelling. Eight contained root/target combinations and four external-target rejection cases cover the correction: before the fix, six failed and six passed; afterward all 12 pass.

The PR description now identifies all four changed files, including integration_status.py's import of the extracted shared normalizer, and explains the compatibility coverage. Its upstream template headings, comments, and checklists are preserved; initialization and command-policy migrations remain deferred.

Exact command Result
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k mixed_case_unc 12 passed, 174 deselected.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/specify_cli/integrations/test_command_status.py -q 175 passed, 53 skipped.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q 9,821 passed, 72 skipped, 62 warnings; 9,893 collected in 507.39 seconds.
uvx --offline ruff@0.15.0 check src tests All checks passed with the CI-pinned version.
git diff --check Passed before commit.

The coordinator independently ran the mixed-case selector (12 passed, 174 deselected) and whitespace check. This is automated evidence, not human review. Native Windows syscalls remain unverified locally: all 53 Windows-only helper cases skip on macOS, and portable parsing/reparse tests do not substitute for Windows execution. Manual sample-project testing was not run.

The preceding failed matrix run 37995540382 failed installing dependencies when PyPI's hatchling index connection reset after retries; other matrix jobs were canceled by fail-fast. Ruff in that run passed. No workflow/configuration changes were made for that infrastructure failure, and the local results above do not claim that fresh remote CI has passed.

AI disclosure: posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol) in autonomous/autopilot mode. The reasoning-effort setting is not independently available. Copilot authored this correction, tests, documentation, commit, PR-body update, and summary; it executed the stated checks and investigated CI. The separate coordinating Copilot session supplied automated review and regression execution. No human or native-Windows validation is claimed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive cross-platform filesystem behavior lacks a completed native Windows CI validation.

0 open findings

🧠 Review effort: Balanced

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e
Copilot AI balanced review requested due to automatic review settings October 10, 2026 10:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive cross-platform filesystem behavior still awaits fresh native Windows validation.

0 open findings

🧠 Review effort: Balanced

@mnriem

mnriem commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 5475915989 and the Windows exclusive-create failure in commit 2ad33b7663752bd8c531174b1ddcfca6e0b67efc.

The existing CI matrix does execute the native Windows tests: on the preceding head, all 53 Windows-only helper cases passed on both Python versions. However, the generic allow-mode collision test failed; the Windows 3.13 full job failed and 3.14 was canceled by fail-fast. That was an actual defect, not missing Windows coverage or a dependency-download issue.

Exclusive creation now checks an existing leaf with no-follow lstat before calling the backend and raises FileExistsError/EEXIST without opening its target. This prevents unintended creation through a dangling leaf link on Windows. Deny-mode link rejection, contained linked-parent traversal, and O_EXCL/available O_NOFOLLOW remain unchanged; portable preflight is not claimed to eliminate concurrent path-change races.

Fresh-head run 38046306348 completed successfully at 2ad33b7663752bd8c531174b1ddcfca6e0b67efc. All six OS/Python pytest jobs and Ruff passed. Both Windows jobs installed dependencies with uv sync --extra test and ran the full suite with uv run pytest:

Completed Windows job Full suite Individual helper outcomes
Python 3.13, job 114196440328 11,293 passed, 581 skipped, 33 warnings in 1,141.47 seconds 192 passed, 6 POSIX-only skips; all 53 native cases and all six named file/alias/dangling × deny/allow collision cases passed.
Python 3.14, job 114196440327 11,293 passed, 581 skipped, 33 warnings in 1,726.95 seconds 192 passed, 6 POSIX-only skips; all 53 native cases and all six named collision cases passed.

The downloaded completed logs were checked individually: no native or collision case was skipped or failed on either Windows version. The additional eight backend regressions also executed as part of each full helper module.

The native subset comprises 40 anchored-relative operation cases, four extended-path symlink read/update/create cases, four invalid link-target cases, one absolute/relative scoping case, and four actual mklink /J junction cases. All are deliberately skipped on other operating systems. Capability-based fixture skips are possible when symlink/junction creation is unavailable, so individual completed test outcomes—not just a successful matrix label—are the verification source.

Exact local command Result
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k 'rejects_existing_leaf_before_open or rejects_dangling_leaf_with_windows_backend' Before: 8 failed. After: 8 passed, 190 deselected; link spelling/state and target preservation verified, backend not opened.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/specify_cli/integrations/test_command_status.py tests/test_shared_infra_lock.py tests/test_shared_infra_gitignore.py tests/test_shared_infra_integrity.py tests/test_registrar_path_traversal.py -q --cov=specify_cli.file_helper --cov-report=term-missing 299 passed, 53 skipped; 100% helper statement coverage.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q 9,833 passed, 72 skipped, 62 warnings in 553.40 seconds; 9,905 collected.
uvx --offline ruff@0.15.0 check src tests All checks passed.
git diff --check Passed before commit.

The coordinator independently reviewed the correction and ran LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py -q -k exclusive_create: 14 passed, 184 deselected; whitespace validation also passed. This is automated evidence, not human review. The original collision test is now parametrized into six named file/alias/dangling × deny/allow cases, retaining all original assertions while identifying the leaf in future CI logs.

The PR's validation evidence was refreshed without altering its description, template comments, or checklist states. No tests were removed, no workflow/configuration changes or CI retries were made, and no threads were resolved. Local macOS skips do not imply missing Windows execution; the native evidence comes from GitHub Actions. Native network-share coverage and race-free enforcement are not claimed. Manual sample-project testing remains unperformed; policy wiring and command migrations remain paused.

AI disclosure: posted on behalf of @mnriem by GitHub Copilot using GPT-6.1 Sol (gpt-6.1-sol) in autonomous/autopilot mode. The reasoning-effort setting is not independently available. Copilot authored the correction, tests, contract documentation, checkpoint, PR validation refresh, and this summary; it executed local checks and inspected completed CI logs. A separate coordinating Copilot session supplied automated review/testing. Windows execution was performed by the cited GitHub Actions jobs, not this macOS session; no human review is claimed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive, cross-platform filesystem policy warrants final human review despite broad tests and successful current CI.

0 open findings

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants