Skip to content

Reject non-regular files before reading - #29428

Closed
haileymarshall wants to merge 1 commit into
NousResearch:mainfrom
haileymarshall:fix/read-file-special-file-guard
Closed

Reject non-regular files before reading#29428
haileymarshall wants to merge 1 commit into
NousResearch:mainfrom
haileymarshall:fix/read-file-special-file-guard

Conversation

@haileymarshall

Copy link
Copy Markdown
Contributor

Summary

  • reject resolved local read targets that are devices, FIFOs, directories, sockets, or other non-regular files
  • add a shell-backed regular-file preflight before running wc, head, sed, or cat for file reads
  • add regression coverage for symlinked devices and non-regular read targets

Validation

  • scripts/run_tests.sh tests/tools/test_file_operations.py tests/tools/test_file_read_guards.py
  • scripts/run_tests.sh tests/tools/test_file_tools_live.py

@alt-glitch alt-glitch added type/security Security vulnerability or hardening tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists labels May 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #10133 (reject read_file symlinks to blocking devices). This PR is broader — rejects all non-regular files (directories, FIFOs, sockets, devices) at both the shell-ops and tool layers. Consider whether #10133 should be superseded.

@haileymarshall
haileymarshall force-pushed the fix/read-file-special-file-guard branch from bb6592c to 6927379 Compare May 20, 2026 19:57

@teknium1 teknium1 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.

Thanks for extending the device-symlink protection to the broader non-regular-file class. The premise remains valid on current main: tools/file_operations.py:1101-1160 still runs wc, head, and sed without a regular-file preflight, and tools/file_operations.py:1239-1259 similarly reaches cat through read_file_raw.

Problems

  • The proposed tool-layer Path.exists() / Path.is_file() check cannot be carried forward unchanged. Current main resolves container paths as PurePosixPath at tools/file_tools.py:440-446, so host filesystem inspection is unavailable and would fail before the backend-side read guard runs.
  • The new general guard coverage is mocked only. The repository’s rubric calls for E2E coverage for file I/O and remote backends; the PR should exercise real directory/FIFO targets and the backend-aware resolution path.

Suggested changes

  • Put the authoritative regular-file check in ShellFileOperations.read_file and read_file_raw so it executes in the selected terminal backend, while retaining the existing early known-device/symlink guard at tools/file_tools.py:544-581.
  • Add real special-file regressions, including a container/backend-path case.

This is an automated hermes-sweeper review.

Comment thread tools/file_tools.py
@@ -461,6 +469,14 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 500, task_id: str =
})

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.

Please avoid carrying this host-side Path.exists() / Path.is_file() guard forward unchanged. Current main resolves container-backend paths as PurePosixPath (tools/file_tools.py:440-446), so the regular-file inspection must run through ShellFileOperations in the selected backend rather than on the host.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing with credit: you were the FIRST to identify this bug class (May 20) — read_file hanging on non-regular files that the name-based device blocklist cannot see. The class is now fixed on main by two complementary guards: a host-side stat-type check (#82792, 0e63ed1) and a shell-layer probe covering all backends including remote (#83008, e0b5005, salvaged from #82075). We went with the #82075 mechanism for the shell layer because it folds the [ -f ] test into the existing wc -c invocation — same coverage as your preflight approach but without the extra shell round-trip per read. Your regression tests' shape (symlinked devices, command-ordering assertions) informed the final test suite. Thank you for finding this three months before anyone else!

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

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/file File tools (read, write, patch, search) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants