fix(security): prevent shell command injection in sandbox file ops (#313)#321
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens sandbox file operations against shell command injection by validating/normalizing paths and shell-escaping paths used in /bin/sh -c flows, with accompanying unit test and documentation updates.
Changes:
- Added path normalization/validation helpers for submission file paths and
/tmpasset targets, and appliedshlex.quote()to paths used in shell redirection. - Switched some container operations (
mkdir,chmod) to list-formexec_runcalls to avoid shell parsing where possible. - Updated unit tests and documentation to reflect the hardened injection/extraction behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
sandbox_manager/sandbox_container.py |
Adds path validation/normalization and shell escaping for prepare_workdir, inject_assets, and extract_file. |
tests/unit/test_extract_file.py |
Adds regression tests for rejecting non-absolute paths and ensuring shell-escaped extraction commands. |
tests/unit/test_asset_injection.py |
Updates assertions to match list-form exec_run and the new shell-escaped Base64 write flow. |
tests/unit/test_sandbox_manager.py |
Minor test comment cleanup and whitespace change. |
docs/pipeline/04-pre-flight.md |
Updates pre-flight docs to describe the shell-escaped Base64 asset injection flow. |
docs/features/setup_config_feature.md |
Updates setup-config docs to describe the shell-escaped Base64 injection method. |
docs/architecture/sandbox_manager.md |
Updates architecture docs to describe the shell-escaped Base64 injection approach. |
Comment on lines
85
to
87
| for submission_file in submission_files.values(): | ||
| file_path = submission_file.filename | ||
| file_path = self._normalize_relative_path(submission_file.filename) | ||
| file_content = submission_file.content |
Reduce required complexity across all issue templates and keep only essential fields for faster triage and issue authoring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Issue #313 identified shell command injection risks in sandbox file operations where user-controlled filenames/paths were interpolated into shell commands.
Solution
sandbox_manager/sandbox_container.pypath handling:/tmppath normalization/validation for injected assetsprepare_workdir,inject_assets, andextract_fileviashlex.quotetests/unit/test_extract_file.pytests/unit/test_asset_injection.pytests/unit/test_sandbox_manager.pydocs/features/setup_config_feature.mddocs/pipeline/04-pre-flight.mddocs/architecture/sandbox_manager.mdFurther clarifications
run_commandsremains unchanged in this patch because issue [Security] Shell Command Injection via Unvalidated Filenames #313 scope was specifically filename/path injection in write/extract flows.Related issues
Closes #313
Checklist