fix: sanitize object IDs to prevent path traversal in BaseApiManager#3299
fix: sanitize object IDs to prevent path traversal in BaseApiManager#3299
Conversation
…ves API POST /api/v2/objectives accepted unsanitized id values used as filenames, allowing directory traversal (e.g. id='../../evil') to write YAML files outside data/objectives/. Apply os.path.basename() sanitization and reject ids starting with '.'. The _sanitize_id() helper is applied in create_on_disk_object, _get_existing_object_file_path, and remove_object_from_disk_by_id, covering all create/update/replace/delete disk paths. Same vulnerability exists for all other object types sharing BaseApiManager: adversaries (adversary_id), fact sources (id), abilities (ability_id), and planners (planner_id) — all now protected by the same central fix. LOCAL BRANCH ONLY — security disclosure pending
…zation 14 tests covering _sanitize_id() in BaseApiManager: normal ids pass, traversal sequences are stripped via os.path.basename, and ids that resolve to empty or dot-prefixed names raise ValueError.
There was a problem hiding this comment.
Pull request overview
Fixes a path traversal vulnerability by sanitizing object IDs before they’re used to build on-disk paths in BaseApiManager, and adds regression tests to prevent reintroduction.
Changes:
- Add
BaseApiManager._sanitize_id()and apply it to ID-to-path flows (create, lookup, delete). - Add security tests covering traversal sequences, absolute paths, and invalid IDs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/api/v2/managers/base_api_manager.py | Introduces _sanitize_id() and applies it to disk path computation and deletion paths. |
| tests/security/test_path_traversal.py | Adds regression tests validating sanitization behavior for a variety of malicious/invalid ID inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review flagged that using os.path.basename() to sanitize IDs silently rewrites attacker-controlled values (e.g. '../../etc/passwd' -> 'passwd'), causing potential ID collisions. Change _sanitize_id() to reject any ID containing '/', '..', or os.sep rather than stripping. Update tests to assert ValueError on traversal and absolute-path inputs.
There was a problem hiding this comment.
Pull request overview
Fixes a path traversal risk by validating/sanitizing object IDs before they are used to build on-disk file paths in BaseApiManager, and adds regression tests covering common traversal patterns.
Changes:
- Added
BaseApiManager._sanitize_id()and applied it to on-disk create/delete and “get existing path” flows. - Added security regression tests asserting traversal/absolute/empty IDs are rejected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
app/api/v2/managers/base_api_manager.py |
Adds _sanitize_id() and uses it before computing file paths to block path traversal via crafted IDs. |
tests/security/test_path_traversal.py |
Adds regression tests for traversal-like IDs and a few “valid ID” cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Relax _sanitize_id() '..' check to reject only standalone '..' token, not embedded '..' like 'version..2' which is safe without a separator - Add defense-in-depth: call _sanitize_id() in _get_new_object_file_path() - Add tests: embedded '..' in ID is permitted, standalone '..' still rejected
There was a problem hiding this comment.
Pull request overview
Hardens API v2 on-disk object CRUD by validating object IDs before they are used to construct filesystem paths, aiming to prevent path traversal via crafted IDs.
Changes:
- Add
BaseApiManager._sanitize_id()and apply it to on-disk create/delete and file path resolution helpers. - Ensure IDs are sanitized before building
data/<ram_key>/<id>.ymlpaths. - Add security regression tests covering common traversal/invalid ID patterns.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/api/v2/managers/base_api_manager.py | Adds and applies _sanitize_id() to prevent traversal when IDs become filename components |
| tests/security/test_path_traversal.py | Adds unit-style regression tests for _sanitize_id() rejecting traversal-like inputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…or for proper HTTP 400, validate type, fix docstring
There was a problem hiding this comment.
Pull request overview
Hardens BaseApiManager on-disk object handling against path traversal by validating/sanitizing object IDs before using them in filesystem paths, and adds regression tests to ensure malicious IDs are rejected.
Changes:
- Added
BaseApiManager._sanitize_id()and applied it to on-disk create/remove and file-path resolution. - Introduced security regression tests covering traversal patterns, absolute paths, dotfiles, and invalid types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/api/v2/managers/base_api_manager.py | Adds ID validation and applies it before constructing/looking up object file paths. |
| tests/security/test_path_traversal.py | Adds regression tests asserting traversal-style IDs raise DataValidationError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
❌ The last analysis has failed. |
|
|
1 similar comment
|
Summary
Object IDs passed to
create_on_disk_object,_get_existing_object_file_path, andremove_object_from_disk_by_idinBaseApiManagerwere used directly inos.path.joinwithout sanitization, allowing a crafted ID like../../etc/passwdto read or write files anywhere on the filesystem. This fix adds_sanitize_id()usingos.path.basenameplus a prefix assertion to reject traversal sequences.Security Impact
Severity: High — An authenticated attacker could craft an object ID containing path traversal sequences (
../) to read arbitrary files (e.g., system credentials) or overwrite arbitrary files on the server, potentially achieving remote code execution.Changes
app/api/v2/managers/base_api_manager.py: Added_sanitize_id()that appliesos.path.basenameand asserts the resolved path stays within the expected directory prefix; applied tocreate_on_disk_object,_get_existing_object_file_path, andremove_object_from_disk_by_id; covers objectives, adversaries, fact sources, abilities, and plannersTests
tests/security/test_path_traversal.py: 14 tests, all passingTest plan
../sequences — confirm a 400 error is returned