improve: add path validation for filesystem-writing tools#91
improve: add path validation for filesystem-writing tools#91cagataycali wants to merge 3 commits intostrands-labs:mainfrom
Conversation
Adds a shared _path_validation module and applies it to all tools that accept user-supplied paths for file I/O: New module: strands_robots/tools/_path_validation.py - validate_save_path() rejects null bytes, '..' traversal components, and paths that resolve into protected system directories (/etc, /usr, /proc, /sys, /var/spool/cron, etc.). Updated tools: - lerobot_camera: validates save_path in capture, batch capture, video recording, and config save operations. - lerobot_calibrate: validates output_dir (backup) and backup_dir (restore) before any filesystem writes. - pose_tool: validates storage_dir in PoseManager initialization.
mrgh-test
left a comment
mrgh-test
left a comment
There was a problem hiding this comment.
Might want to add tests for the path validation as well.
| "/var/spool/cron", | ||
| "/var/spool/at", |
There was a problem hiding this comment.
These two probably also need trailing slashes?
There was a problem hiding this comment.
Fixed in 6ad33de — added trailing slashes to both /var/spool/cron/ and /var/spool/at/.
Also improved the matching logic: the resolved path now gets a trailing / appended before startswith comparison, so the exact directory path itself (e.g. /var/spool/cron) is also caught. Moved BLOCKED_PREFIXES to a module-level constant and added an invariant test (test_all_blocked_prefixes_end_with_slash) to prevent this class of bug from recurring.
Added 37 tests covering path validation — see tests/test_path_validation.py.
🤖 AI agent response. Strands Agents. Feedback welcome!
Address review feedback from PR strands-labs#91: 1. Fix trailing slashes on /var/spool/cron/ and /var/spool/at/ blocked prefixes — without the slash, paths like /var/spool/crondata would incorrectly match while /var/spool/cron itself could slip through. 2. Move BLOCKED_PREFIXES to module-level constant (importable, not re-created on every call). 3. Add check_path normalization — append '/' to resolved path before prefix comparison so exact directory matches (e.g. /var/spool/cron) are also caught. 4. Add comprehensive test suite (37 tests): - Happy paths (tmp, home, tilde expansion) - Null byte rejection - Directory traversal (.., backslash variants) - All blocked prefixes parametrized - Trailing-slash precision (sibling dir false-positive prevention) - Custom label propagation - Symlink resolution (blocked and safe targets) - Invariant: all prefixes must end with '/'
|
👋 Friendly nudge — the review feedback (trailing slashes on blocked prefixes) was addressed in commit Summary of changes since review:
CI is green. Ready for re-review when convenient. 🙏 🤖 AI agent response. Strands Agents. Feedback welcome! |
2 participants
Summary
Adds a shared path validation module and applies it to all tools that accept user-supplied paths for file I/O operations (camera captures, calibration backups/restores, pose storage).
Changes
New module:
strands_robots/tools/_path_validation.pyvalidate_save_path(path, label)performs three checks:\x00..components (e.g.../../etc/cron.d/evil)os.path.realpath()resolution, blocks writes into/etc/,/usr/,/bin/,/sbin/,/boot/,/dev/,/proc/,/sys/,/var/spool/cron,/var/spool/atReturns the resolved absolute path on success, raises
ValueErroron failure.Updated tools
lerobot_camerasave_pathin capture, batch capture, video recording, config savelerobot_calibrateoutput_dir(backup),backup_dir(restore)pose_toolstorage_dirinPoseManager.__init__Motivation
These tools are called by AI agents which construct file paths from conversation context. Without validation, a crafted path like
../../etc/cron.d/evilcould write to arbitrary locations. The validation module provides consistent defense-in-depth across all file-writing tools.Design decisions
_path_validation.py) rather than inline checks — single source of truth, easy to extend the blocked-prefix list.os.path.realpath— resolves symlinks to catch symlink-based bypasses.