From 92f5f3f80860148e135a599871d44e7be871ad3f Mon Sep 17 00:00:00 2001 From: deacon Date: Sun, 15 Mar 2026 23:08:10 -0400 Subject: [PATCH 1/8] fix(security): sanitize id field to prevent path traversal in objectives API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/api/v2/managers/base_api_manager.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index f72368790..eb07cb91c 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -64,6 +64,7 @@ def create_object_from_schema(self, schema: SchemaMeta, data: dict, access: Base async def create_on_disk_object(self, data: dict, access: dict, ram_key: str, id_property: str, obj_class: type): obj_id = data.get(id_property) or str(uuid.uuid4()) + obj_id = self._sanitize_id(obj_id) data[id_property] = obj_id file_path = await self._get_new_object_file_path(data[id_property], ram_key) @@ -121,11 +122,24 @@ async def remove_object_from_memory_by_id(self, identifier: str, ram_key: str, i await self._data_svc.remove(ram_key, {id_property: identifier}) async def remove_object_from_disk_by_id(self, identifier: str, ram_key: str): + identifier = self._sanitize_id(identifier) file_path = await self._get_existing_object_file_path(identifier, ram_key) if os.path.exists(file_path): os.remove(file_path) + @staticmethod + def _sanitize_id(obj_id: str) -> str: + """Strip any path traversal sequences from an object ID used as a filename. + + Raises ValueError if the resulting name is empty or starts with '.', + which would indicate a hidden file or a remaining traversal component. + """ + safe = os.path.basename(obj_id) + if not safe or safe.startswith('.'): + raise ValueError(f"Invalid id: {obj_id!r}") + return safe + @staticmethod async def _get_new_object_file_path(identifier: str, ram_key: str) -> str: """Create file path for new object""" @@ -133,6 +147,7 @@ async def _get_new_object_file_path(identifier: str, ram_key: str) -> str: async def _get_existing_object_file_path(self, identifier: str, ram_key: str) -> str: """Find file path for existing object (by id)""" + identifier = self._sanitize_id(identifier) _, file_path = await self._file_svc.find_file_path(f'{identifier}.yml', location=ram_key) if not file_path: file_path = await self._get_new_object_file_path(identifier, ram_key) From 244ad9b82482594df7e1f96c45366a0746a8017b Mon Sep 17 00:00:00 2001 From: deacon Date: Sun, 15 Mar 2026 23:16:21 -0400 Subject: [PATCH 2/8] test(security): add regression tests for API id path traversal sanitization 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. --- tests/security/test_path_traversal.py | 81 +++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 tests/security/test_path_traversal.py diff --git a/tests/security/test_path_traversal.py b/tests/security/test_path_traversal.py new file mode 100644 index 000000000..7c32fa2f2 --- /dev/null +++ b/tests/security/test_path_traversal.py @@ -0,0 +1,81 @@ +""" +Security regression tests for path traversal via API id fields. + +Verifies that _sanitize_id() in BaseApiManager correctly rejects +traversal sequences like '../', absolute paths, and empty ids. +""" +import pytest + +from app.api.v2.managers.base_api_manager import BaseApiManager + + +class TestSanitizeId: + + def test_normal_id_passes(self): + assert BaseApiManager._sanitize_id('abc123') == 'abc123' + + def test_uuid_passes(self): + assert BaseApiManager._sanitize_id('f489321f-31b6-ef36-3042-94562d3d4645') == 'f489321f-31b6-ef36-3042-94562d3d4645' + + def test_traversal_stripped(self): + # os.path.basename('../../etc/passwd') == 'passwd' + result = BaseApiManager._sanitize_id('../../etc/passwd') + assert result == 'passwd' + assert '..' not in result + assert '/' not in result + + def test_absolute_path_stripped(self): + # os.path.basename('/etc/passwd') == 'passwd' + result = BaseApiManager._sanitize_id('/etc/passwd') + assert result == 'passwd' + + def test_leading_dot_rejected(self): + # os.path.basename('.hidden') == '.hidden', which starts with '.' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('.hidden') + + def test_empty_id_rejected(self): + # os.path.basename('') == '' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('') + + def test_traversal_to_empty_raises(self): + # os.path.basename('../..') == '..' which starts with '.' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('../..') + + def test_traversal_to_dot_raises(self): + # os.path.basename('../../.') == '.' which starts with '.' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('../../.') + + def test_traversal_with_url_encoding_rejected(self): + # os.path.basename('..%2Fetc%2Fpasswd') returns '..%2Fetc%2Fpasswd' unchanged + # (no real '/' chars, so basename can't strip anything). The result starts with '.' + # so ValueError is raised — the id is safely rejected. + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('..%2Fetc%2Fpasswd') + + def test_windows_backslash_rejected(self): + # On Linux backslashes are not path separators; os.path.basename returns the full + # string unchanged. It starts with '.' so ValueError is raised. + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('..\\..\\windows\\system32') + + def test_trailing_slash_raises(self): + # os.path.basename('someid/') == '' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('someid/') + + def test_root_slash_raises(self): + # os.path.basename('/') == '' -> ValueError + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('/') + + def test_alphanumeric_with_hyphens_passes(self): + val = 'my-objective-2024' + assert BaseApiManager._sanitize_id(val) == val + + def test_id_with_underscores_passes(self): + val = 'some_object_id_42' + assert BaseApiManager._sanitize_id(val) == val From 45d6f8b159cac3f204d36b03838ec4050f4bc69f Mon Sep 17 00:00:00 2001 From: deacon Date: Mon, 16 Mar 2026 00:23:35 -0400 Subject: [PATCH 3/8] fix: reject traversal IDs instead of silently rewriting via basename 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. --- app/api/v2/managers/base_api_manager.py | 16 ++++++----- tests/security/test_path_traversal.py | 36 +++++++++---------------- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index eb07cb91c..4a0748a72 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -130,15 +130,19 @@ async def remove_object_from_disk_by_id(self, identifier: str, ram_key: str): @staticmethod def _sanitize_id(obj_id: str) -> str: - """Strip any path traversal sequences from an object ID used as a filename. + """Validate an object ID used as a filename, rejecting any path traversal sequences. - Raises ValueError if the resulting name is empty or starts with '.', - which would indicate a hidden file or a remaining traversal component. + Raises ValueError if the ID contains path separators, traversal sequences, + starts with '.', or is empty. The ID is returned unchanged when valid. """ - safe = os.path.basename(obj_id) - if not safe or safe.startswith('.'): + if not obj_id: raise ValueError(f"Invalid id: {obj_id!r}") - return safe + # Reject any ID that contains a path separator or traversal component + if '/' in obj_id or os.sep in obj_id or '..' in obj_id: + raise ValueError(f"Invalid id: {obj_id!r}") + if obj_id.startswith('.'): + raise ValueError(f"Invalid id: {obj_id!r}") + return obj_id @staticmethod async def _get_new_object_file_path(identifier: str, ram_key: str) -> str: diff --git a/tests/security/test_path_traversal.py b/tests/security/test_path_traversal.py index 7c32fa2f2..a54db947b 100644 --- a/tests/security/test_path_traversal.py +++ b/tests/security/test_path_traversal.py @@ -17,58 +17,48 @@ def test_normal_id_passes(self): def test_uuid_passes(self): assert BaseApiManager._sanitize_id('f489321f-31b6-ef36-3042-94562d3d4645') == 'f489321f-31b6-ef36-3042-94562d3d4645' - def test_traversal_stripped(self): - # os.path.basename('../../etc/passwd') == 'passwd' - result = BaseApiManager._sanitize_id('../../etc/passwd') - assert result == 'passwd' - assert '..' not in result - assert '/' not in result - - def test_absolute_path_stripped(self): - # os.path.basename('/etc/passwd') == 'passwd' - result = BaseApiManager._sanitize_id('/etc/passwd') - assert result == 'passwd' + def test_traversal_rejected(self): + # IDs containing traversal sequences must be rejected outright, not silently rewritten. + # Silently rewriting '../../etc/passwd' -> 'passwd' would cause ID collisions. + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('../../etc/passwd') + + def test_absolute_path_rejected(self): + # Absolute paths must be rejected rather than silently stripped to their basename. + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('/etc/passwd') def test_leading_dot_rejected(self): - # os.path.basename('.hidden') == '.hidden', which starts with '.' -> ValueError with pytest.raises(ValueError): BaseApiManager._sanitize_id('.hidden') def test_empty_id_rejected(self): - # os.path.basename('') == '' -> ValueError with pytest.raises(ValueError): BaseApiManager._sanitize_id('') - def test_traversal_to_empty_raises(self): - # os.path.basename('../..') == '..' which starts with '.' -> ValueError + def test_traversal_to_dotdot_raises(self): with pytest.raises(ValueError): BaseApiManager._sanitize_id('../..') def test_traversal_to_dot_raises(self): - # os.path.basename('../../.') == '.' which starts with '.' -> ValueError with pytest.raises(ValueError): BaseApiManager._sanitize_id('../../.') def test_traversal_with_url_encoding_rejected(self): - # os.path.basename('..%2Fetc%2Fpasswd') returns '..%2Fetc%2Fpasswd' unchanged - # (no real '/' chars, so basename can't strip anything). The result starts with '.' - # so ValueError is raised — the id is safely rejected. + # URL-encoded separators should be rejected; '..%2F' contains '..' with pytest.raises(ValueError): BaseApiManager._sanitize_id('..%2Fetc%2Fpasswd') def test_windows_backslash_rejected(self): - # On Linux backslashes are not path separators; os.path.basename returns the full - # string unchanged. It starts with '.' so ValueError is raised. + # Backslash traversal sequences contain '..' and are rejected with pytest.raises(ValueError): BaseApiManager._sanitize_id('..\\..\\windows\\system32') def test_trailing_slash_raises(self): - # os.path.basename('someid/') == '' -> ValueError with pytest.raises(ValueError): BaseApiManager._sanitize_id('someid/') def test_root_slash_raises(self): - # os.path.basename('/') == '' -> ValueError with pytest.raises(ValueError): BaseApiManager._sanitize_id('/') From 255eea7e23c3a84c9788286c15175cfd556929d2 Mon Sep 17 00:00:00 2001 From: deacon Date: Mon, 16 Mar 2026 01:27:24 -0400 Subject: [PATCH 4/8] fix: address Copilot review feedback on path-traversal sanitization - 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 --- app/api/v2/managers/base_api_manager.py | 14 ++++++++++++-- tests/security/test_path_traversal.py | 12 ++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index 4a0748a72..b6169e5a6 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -134,11 +134,20 @@ def _sanitize_id(obj_id: str) -> str: Raises ValueError if the ID contains path separators, traversal sequences, starts with '.', or is empty. The ID is returned unchanged when valid. + + Note: embedded '..' in the middle of an ID (e.g. 'version..2') is permitted + because caldera IDs are used directly as filename components, and '..'' only + enables directory traversal when it appears as a standalone path component + (i.e. with an adjacent separator). The path-separator check above ensures + that no separator can appear, making embedded '..' harmless. """ if not obj_id: raise ValueError(f"Invalid id: {obj_id!r}") - # Reject any ID that contains a path separator or traversal component - if '/' in obj_id or os.sep in obj_id or '..' in obj_id: + # Reject any ID that contains a path separator + if '/' in obj_id or os.sep in obj_id: + raise ValueError(f"Invalid id: {obj_id!r}") + # Reject '..' as a standalone token (traversal sequence without separator) + if obj_id == '..': raise ValueError(f"Invalid id: {obj_id!r}") if obj_id.startswith('.'): raise ValueError(f"Invalid id: {obj_id!r}") @@ -147,6 +156,7 @@ def _sanitize_id(obj_id: str) -> str: @staticmethod async def _get_new_object_file_path(identifier: str, ram_key: str) -> str: """Create file path for new object""" + identifier = BaseApiManager._sanitize_id(identifier) return os.path.join('data', ram_key, f'{identifier}.yml') async def _get_existing_object_file_path(self, identifier: str, ram_key: str) -> str: diff --git a/tests/security/test_path_traversal.py b/tests/security/test_path_traversal.py index a54db947b..6e2a5ca5d 100644 --- a/tests/security/test_path_traversal.py +++ b/tests/security/test_path_traversal.py @@ -69,3 +69,15 @@ def test_alphanumeric_with_hyphens_passes(self): def test_id_with_underscores_passes(self): val = 'some_object_id_42' assert BaseApiManager._sanitize_id(val) == val + + def test_embedded_dotdot_in_id_passes(self): + # Embedded '..' without a path separator is not a traversal sequence and is permitted. + # e.g. 'version..2' is a valid slug-style ID; it cannot escape the directory + # because no path separator is present. + val = 'version..2' + assert BaseApiManager._sanitize_id(val) == val + + def test_standalone_dotdot_raises(self): + # '..' alone IS a traversal token and must be rejected. + with pytest.raises(ValueError): + BaseApiManager._sanitize_id('..') From 085369cbdfea1c6707abb158d35b7368a229d213 Mon Sep 17 00:00:00 2001 From: deacon Date: Mon, 16 Mar 2026 09:57:50 -0400 Subject: [PATCH 5/8] Address Copilot review: raise DataValidationError instead of ValueError for proper HTTP 400, validate type, fix docstring --- app/api/v2/managers/base_api_manager.py | 17 ++++++++------ tests/security/test_path_traversal.py | 31 ++++++++++++++++--------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index b6169e5a6..3e2bb7fc9 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -7,6 +7,7 @@ from typing import Any, List from base64 import b64encode, b64decode +from app.api.v2.errors import DataValidationError from app.utility.base_world import BaseWorld @@ -129,28 +130,30 @@ async def remove_object_from_disk_by_id(self, identifier: str, ram_key: str): os.remove(file_path) @staticmethod - def _sanitize_id(obj_id: str) -> str: + def _sanitize_id(obj_id) -> str: """Validate an object ID used as a filename, rejecting any path traversal sequences. - Raises ValueError if the ID contains path separators, traversal sequences, + Raises DataValidationError if the ID contains path separators, traversal sequences, starts with '.', or is empty. The ID is returned unchanged when valid. Note: embedded '..' in the middle of an ID (e.g. 'version..2') is permitted - because caldera IDs are used directly as filename components, and '..'' only + because caldera IDs are used directly as filename components, and '..' only enables directory traversal when it appears as a standalone path component (i.e. with an adjacent separator). The path-separator check above ensures that no separator can appear, making embedded '..' harmless. """ + if not isinstance(obj_id, str): + raise DataValidationError(message=f"Invalid id type: expected str, got {type(obj_id).__name__}", name='id', value=obj_id) if not obj_id: - raise ValueError(f"Invalid id: {obj_id!r}") + raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) # Reject any ID that contains a path separator if '/' in obj_id or os.sep in obj_id: - raise ValueError(f"Invalid id: {obj_id!r}") + raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) # Reject '..' as a standalone token (traversal sequence without separator) if obj_id == '..': - raise ValueError(f"Invalid id: {obj_id!r}") + raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) if obj_id.startswith('.'): - raise ValueError(f"Invalid id: {obj_id!r}") + raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) return obj_id @staticmethod diff --git a/tests/security/test_path_traversal.py b/tests/security/test_path_traversal.py index 6e2a5ca5d..fbce6538b 100644 --- a/tests/security/test_path_traversal.py +++ b/tests/security/test_path_traversal.py @@ -3,9 +3,11 @@ Verifies that _sanitize_id() in BaseApiManager correctly rejects traversal sequences like '../', absolute paths, and empty ids. +Raises DataValidationError (caught by v2 middleware as HTTP 400). """ import pytest +from app.api.v2.errors import DataValidationError from app.api.v2.managers.base_api_manager import BaseApiManager @@ -20,46 +22,46 @@ def test_uuid_passes(self): def test_traversal_rejected(self): # IDs containing traversal sequences must be rejected outright, not silently rewritten. # Silently rewriting '../../etc/passwd' -> 'passwd' would cause ID collisions. - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('../../etc/passwd') def test_absolute_path_rejected(self): # Absolute paths must be rejected rather than silently stripped to their basename. - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('/etc/passwd') def test_leading_dot_rejected(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('.hidden') def test_empty_id_rejected(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('') def test_traversal_to_dotdot_raises(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('../..') def test_traversal_to_dot_raises(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('../../.') def test_traversal_with_url_encoding_rejected(self): # URL-encoded separators should be rejected; '..%2F' contains '..' - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('..%2Fetc%2Fpasswd') def test_windows_backslash_rejected(self): # Backslash traversal sequences contain '..' and are rejected - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('..\\..\\windows\\system32') def test_trailing_slash_raises(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('someid/') def test_root_slash_raises(self): - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('/') def test_alphanumeric_with_hyphens_passes(self): @@ -79,5 +81,12 @@ def test_embedded_dotdot_in_id_passes(self): def test_standalone_dotdot_raises(self): # '..' alone IS a traversal token and must be rejected. - with pytest.raises(ValueError): + with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('..') + + def test_non_string_id_rejected(self): + """Non-string IDs (e.g. int, None) should raise DataValidationError, not TypeError.""" + with pytest.raises(DataValidationError): + BaseApiManager._sanitize_id(12345) + with pytest.raises(DataValidationError): + BaseApiManager._sanitize_id(None) From 2930828e0a1bd2e492f9fa66876effa4fa2d4913 Mon Sep 17 00:00:00 2001 From: uruwhy <58484522+uruwhy@users.noreply.github.com> Date: Sat, 21 Mar 2026 12:25:50 -0400 Subject: [PATCH 6/8] fix sanitization --- app/api/v2/managers/base_api_manager.py | 24 +---- .../api/v2/managers/test_base_api_manager.py | 14 +++ tests/security/test_path_traversal.py | 92 ------------------- 3 files changed, 18 insertions(+), 112 deletions(-) delete mode 100644 tests/security/test_path_traversal.py diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index 3e2bb7fc9..f3f312938 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -1,5 +1,6 @@ import logging import os +import re import uuid import yaml @@ -131,29 +132,12 @@ async def remove_object_from_disk_by_id(self, identifier: str, ram_key: str): @staticmethod def _sanitize_id(obj_id) -> str: - """Validate an object ID used as a filename, rejecting any path traversal sequences. - - Raises DataValidationError if the ID contains path separators, traversal sequences, - starts with '.', or is empty. The ID is returned unchanged when valid. - - Note: embedded '..' in the middle of an ID (e.g. 'version..2') is permitted - because caldera IDs are used directly as filename components, and '..' only - enables directory traversal when it appears as a standalone path component - (i.e. with an adjacent separator). The path-separator check above ensures - that no separator can appear, making embedded '..' harmless. - """ + '''Removes any non-alphanumeric characters and non-hyphen/underscore.''' if not isinstance(obj_id, str): - raise DataValidationError(message=f"Invalid id type: expected str, got {type(obj_id).__name__}", name='id', value=obj_id) + raise DataValidationError(message=f'Invalid id type: expected str, got {type(obj_id).__name__}', name='id', value=obj_id) + obj_id = re.sub(r'[^a-zA-Z0-9_-]', '', obj_id) if not obj_id: raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) - # Reject any ID that contains a path separator - if '/' in obj_id or os.sep in obj_id: - raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) - # Reject '..' as a standalone token (traversal sequence without separator) - if obj_id == '..': - raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) - if obj_id.startswith('.'): - raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) return obj_id @staticmethod diff --git a/tests/api/v2/managers/test_base_api_manager.py b/tests/api/v2/managers/test_base_api_manager.py index 7a6495cf1..01d4a52f1 100644 --- a/tests/api/v2/managers/test_base_api_manager.py +++ b/tests/api/v2/managers/test_base_api_manager.py @@ -1,5 +1,7 @@ +import pytest import marshmallow as ma +from app.api.v2.errors import DataValidationError from app.api.v2.managers.base_api_manager import BaseApiManager from app.objects.interfaces.i_object import FirstClassObjectInterface from app.utility.base_object import BaseObject @@ -248,3 +250,15 @@ def test_replace_object(agent): assert len(stub_data_svc.ram['tests']) == 1 assert not stub_data_svc.ram['tests'][0].value + + +def test_sanitize_id(): + valid = '766be199-7316-4b26-b3db-e272aaf7e0d4' + assert valid == BaseApiManager._sanitize_id(valid) + assert valid.upper() == BaseApiManager._sanitize_id(valid.upper()) + assert valid == BaseApiManager._sanitize_id('../.././&%$!"#766be19:9-73[16-]4b}26-b{3d!b-e272..\\//aaf/*7e0d4') + assert 'testid123TEST' == BaseApiManager._sanitize_id('testid123TEST') + with pytest.raises(DataValidationError): + BaseApiManager._sanitize_id('../../.') + with pytest.raises(DataValidationError): + BaseApiManager._sanitize_id('') diff --git a/tests/security/test_path_traversal.py b/tests/security/test_path_traversal.py deleted file mode 100644 index fbce6538b..000000000 --- a/tests/security/test_path_traversal.py +++ /dev/null @@ -1,92 +0,0 @@ -""" -Security regression tests for path traversal via API id fields. - -Verifies that _sanitize_id() in BaseApiManager correctly rejects -traversal sequences like '../', absolute paths, and empty ids. -Raises DataValidationError (caught by v2 middleware as HTTP 400). -""" -import pytest - -from app.api.v2.errors import DataValidationError -from app.api.v2.managers.base_api_manager import BaseApiManager - - -class TestSanitizeId: - - def test_normal_id_passes(self): - assert BaseApiManager._sanitize_id('abc123') == 'abc123' - - def test_uuid_passes(self): - assert BaseApiManager._sanitize_id('f489321f-31b6-ef36-3042-94562d3d4645') == 'f489321f-31b6-ef36-3042-94562d3d4645' - - def test_traversal_rejected(self): - # IDs containing traversal sequences must be rejected outright, not silently rewritten. - # Silently rewriting '../../etc/passwd' -> 'passwd' would cause ID collisions. - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('../../etc/passwd') - - def test_absolute_path_rejected(self): - # Absolute paths must be rejected rather than silently stripped to their basename. - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('/etc/passwd') - - def test_leading_dot_rejected(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('.hidden') - - def test_empty_id_rejected(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('') - - def test_traversal_to_dotdot_raises(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('../..') - - def test_traversal_to_dot_raises(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('../../.') - - def test_traversal_with_url_encoding_rejected(self): - # URL-encoded separators should be rejected; '..%2F' contains '..' - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('..%2Fetc%2Fpasswd') - - def test_windows_backslash_rejected(self): - # Backslash traversal sequences contain '..' and are rejected - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('..\\..\\windows\\system32') - - def test_trailing_slash_raises(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('someid/') - - def test_root_slash_raises(self): - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('/') - - def test_alphanumeric_with_hyphens_passes(self): - val = 'my-objective-2024' - assert BaseApiManager._sanitize_id(val) == val - - def test_id_with_underscores_passes(self): - val = 'some_object_id_42' - assert BaseApiManager._sanitize_id(val) == val - - def test_embedded_dotdot_in_id_passes(self): - # Embedded '..' without a path separator is not a traversal sequence and is permitted. - # e.g. 'version..2' is a valid slug-style ID; it cannot escape the directory - # because no path separator is present. - val = 'version..2' - assert BaseApiManager._sanitize_id(val) == val - - def test_standalone_dotdot_raises(self): - # '..' alone IS a traversal token and must be rejected. - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id('..') - - def test_non_string_id_rejected(self): - """Non-string IDs (e.g. int, None) should raise DataValidationError, not TypeError.""" - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id(12345) - with pytest.raises(DataValidationError): - BaseApiManager._sanitize_id(None) From b116dc6598d55178c09621fb6d0378097909a05f Mon Sep 17 00:00:00 2001 From: Fiona McCrae Date: Mon, 6 Apr 2026 13:38:52 -0400 Subject: [PATCH 7/8] Added warning log for when ID is sanitized, and corresponding test --- app/api/v2/managers/base_api_manager.py | 3 +++ tests/api/v2/managers/test_base_api_manager.py | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/app/api/v2/managers/base_api_manager.py b/app/api/v2/managers/base_api_manager.py index f3f312938..83da39b04 100644 --- a/app/api/v2/managers/base_api_manager.py +++ b/app/api/v2/managers/base_api_manager.py @@ -135,9 +135,12 @@ def _sanitize_id(obj_id) -> str: '''Removes any non-alphanumeric characters and non-hyphen/underscore.''' if not isinstance(obj_id, str): raise DataValidationError(message=f'Invalid id type: expected str, got {type(obj_id).__name__}', name='id', value=obj_id) + original_id = obj_id obj_id = re.sub(r'[^a-zA-Z0-9_-]', '', obj_id) if not obj_id: raise DataValidationError(message=f"Invalid id: {obj_id!r}", name='id', value=obj_id) + if original_id != obj_id: + logging.getLogger(DEFAULT_LOGGER_NAME).warning(f"Sanitized ID: {obj_id}") return obj_id @staticmethod diff --git a/tests/api/v2/managers/test_base_api_manager.py b/tests/api/v2/managers/test_base_api_manager.py index 01d4a52f1..28aa016e2 100644 --- a/tests/api/v2/managers/test_base_api_manager.py +++ b/tests/api/v2/managers/test_base_api_manager.py @@ -262,3 +262,15 @@ def test_sanitize_id(): BaseApiManager._sanitize_id('../../.') with pytest.raises(DataValidationError): BaseApiManager._sanitize_id('') + # Non-string IDs should raise a DataValidationError + with pytest.raises(DataValidationError): + BaseApiManager._sanitize_id(12345) + +def test_sanitize_id_logs_warning_when_changed(caplog): + # Capture warnings when an ID is mutated by sanitization + caplog.set_level('WARNING') + original = 'abc/def?ghi' + sanitized = BaseApiManager._sanitize_id(original) + assert sanitized == 'abcdefghi' + # Ensure a warning was emitted that includes the sanitized ID + assert any('Sanitized ID' in rec.getMessage() and sanitized in rec.getMessage() for rec in caplog.records) From b1fc6997771cef7e3c931a863ff22b3029ba7f63 Mon Sep 17 00:00:00 2001 From: Daniel Matthews <58484522+uruwhy@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:36:24 -0400 Subject: [PATCH 8/8] style fix --- tests/api/v2/managers/test_base_api_manager.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/api/v2/managers/test_base_api_manager.py b/tests/api/v2/managers/test_base_api_manager.py index 28aa016e2..3cf4560e5 100644 --- a/tests/api/v2/managers/test_base_api_manager.py +++ b/tests/api/v2/managers/test_base_api_manager.py @@ -266,6 +266,7 @@ def test_sanitize_id(): with pytest.raises(DataValidationError): BaseApiManager._sanitize_id(12345) + def test_sanitize_id_logs_warning_when_changed(caplog): # Capture warnings when an ID is mutated by sanitization caplog.set_level('WARNING')