Python: harden Hyperlight output capture against symlinks - #6601
Conversation
Mirror the input-staging symlink hardening on the output-capture path of HyperlightExecuteCodeTool. Output discovery now walks via the symlink-safe _iter_real_entries instead of rglob, per-file collection validates that no path component is a symlink and the final entry is a regular file, and file reads use os.O_NOFOLLOW. Adds regression tests for the output path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR hardens Hyperlight’s Python HyperlightExecuteCodeTool output-capture path to avoid following symlinks under the sandbox-controlled /output directory, aligning it with the existing symlink-safe input-staging behavior.
Changes:
- Replaces
Path.rgloboutput enumeration with_iter_real_entriesto avoid descending into symlinked directories. - Adds
_is_safe_output_file(component-by-componentlstatvalidation) and_read_output_file_bytes(final-componentO_NOFOLLOWopen) to reduce symlink/TOCTOU exfiltration risk. - Adds regression tests covering symlinked outputs and the “happy path” for real output files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/hyperlight/agent_framework_hyperlight/_execute_code_tool.py | Adds symlink-safe validation + reading helpers and switches output enumeration to symlink-safe walking. |
| python/packages/hyperlight/tests/hyperlight/test_hyperlight_codeact.py | Adds regression tests for output capture behavior around symlinks and real output files. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 86%
✓ Correctness
This PR correctly hardens the output capture path against symlink attacks, mirroring protections already on the input path. _collect_output_relative_paths now uses the symlink-safe _iter_real_entries walker instead of Path.rglob. _is_safe_output_file walks each path component with lstat to reject symlinks at any level. _read_output_file_bytes uses O_NOFOLLOW for defense-in-depth against TOCTOU races. The except clause correctly broadens to OSError to catch ELOOP from O_NOFOLLOW. All three layers (enumeration, validation, read) are consistent and well-tested. No correctness issues found.
✓ Security Reliability
This PR correctly hardens the output-capture path against symlink-based sandbox escapes, mirroring protections already applied on the input side. The three-layer defense (symlink-safe enumeration via
_iter_real_entries, component-by-componentlstatvalidation in_is_safe_output_file, andO_NOFOLLOWon read) is well-designed. One concern:_read_output_file_bytessilently falls back to following symlinks on platforms that lackO_NOFOLLOW, which undermines the TOCTOU defense the docstring promises. Overall the changes are sound and well-tested.
✓ Test Coverage
The PR adds five well-structured tests covering the main symlink attack vectors on the output path: final-component file symlink, symlinked directory, intermediate directory symlink from backend listing, and a regression test for genuine files. The coverage of the two higher-level functions (
_collect_output_relative_pathsand_parse_output_files) is solid. The main gap is that_read_output_file_bytes—the TOCTOU defense layer explicitly highlighted in the PR description—has no direct test exercising itsO_NOFOLLOWrejection behavior; it is only tested on the happy path through integration tests.
✓ Failure Modes
The PR correctly hardens the output-capture path against symlink-based exfiltration.
_is_safe_output_filewalks each path component withlstat,_iter_real_entriesreplacesPath.rglob, and_read_output_file_bytesusesO_NOFOLLOWas TOCTOU defense-in-depth. Theexcept (PermissionError, OSError)broadening is justified by the newELOOPfromO_NOFOLLOW. The layered defenses are sound:_collect_output_relative_pathsfilters at enumeration,_is_safe_output_filevalidates before read, andO_NOFOLLOWcatches TOCTOU races. Tests cover all key scenarios. No blocking failure modes found.
✓ Design Approach
I found one design-level gap in the new output-read hardening. The approach closes the TOCTOU window on platforms that provide
os.O_NOFOLLOW, but it silently degrades back to a normal open on supported Windows runners, so the final-component symlink swap the PR is trying to prevent can still succeed there.
Automated review by eavanvalkenburg's agents
- _is_safe_output_file now rejects '.'/'..' components (lexical relative_to could otherwise escape root without a symlink) - _read_output_file_bytes adds a cross-platform TOCTOU guard (lstat/fstat st_dev+st_ino identity check) since O_NOFOLLOW is absent on Windows - fix intermediate-dir-symlink test to use a relative listing path so it exercises normalization + validation; add a parent-traversal unit test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation & Context
The
HyperlightExecuteCodeToolalready hardens its input-staging path so thatsymlinks in a user-supplied
workspace_root/file_mountstree are neverfollowed when staging files into the sandbox (
_copy_path,_iter_real_entries,_path_tree_signature). The mirror output-capture path did not have the sameprotection: it enumerated and read files from the sandbox-controlled
/outputdirectory using APIs that follow symlinks (
Path.rglob,Path.is_file,Path.read_bytes). This change brings the output path in line with the inputpath so both sides treat symlinked entries consistently.
Description & Review Guide
What are the major changes?
_collect_output_relative_pathsnow walks the output directory via theexisting symlink-safe
_iter_real_entrieshelper instead ofPath.rglob,so symlinked entries and symlinked directories are not surfaced.
_is_safe_output_filehelper validates each path component from theoutput root to the target with
lstat, rejecting any component that is asymlink and requiring the final entry to be a regular file. It replaces the
previous
Path.is_filegate in_parse_output_files._read_output_file_byteshelper that opens withos.O_NOFOLLOWinstead ofPath.read_bytes; the resultingOSErrorishandled alongside
PermissionErrorin_parse_output_files.symlink, symlinked directory, intermediate directory symlink in a
backend-provided listing, and the real-file happy path).
What is the impact of these changes?
entries in
/outputare no longer returned asContent. No public APIchanges.
What do you want reviewers to focus on?
_is_safe_output_filecomponent-by-component validation and theO_NOFOLLOWread path.Related Issue
N/A — no linked issue.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.