Skip to content

Commit 514bf4f

Browse files
phernandezclaude
andcommitted
fix: cloud mode path validation and sanitization (bmc-issue-103)
Fixes basicmachines-co/basic-memory-cloud#103 ## Problem Cloud tenants could create projects with invalid paths outside /app/data/ mount, causing silent failures during sync operations. ## Root Causes 1. Missing BASIC_MEMORY_CLOUD_MODE environment variable in tenant containers 2. Cloud mode path validation existed but wasn't executing 3. No path sanitization to prevent escape attempts ## Changes ### Enhanced Path Validation (project_service.py) - Use config_manager.config.cloud_mode_enabled for proper detection - Sanitize input paths by stripping dangerous patterns: - Leading slashes (/) - Home directory references (~/) - Parent directory traversal (../) - Construct all paths relative to BASIC_MEMORY_HOME - Validate resolved paths stay within cloud storage boundary - Raise clear ValueError for invalid paths ### API Layer Comment (project_router.py) - Document that service layer handles cloud mode validation ### Test Coverage (test_project_service.py) - test_add_project_cloud_mode_sanitizes_paths: Verify sanitization works - test_add_project_cloud_mode_rejects_escape_attempts: Security validation - test_add_project_local_mode_allows_arbitrary_paths: Backward compatibility ## Behavior After Fix ✅ Cloud mode paths constrained to /app/data/basic-memory/: - Input: "test" → /app/data/basic-memory/test - Input: "~/Documents/test" → /app/data/basic-memory/Documents/test - Input: "/tmp/test" → /app/data/basic-memory/tmp/test - Input: "../../../etc/passwd" → /app/data/basic-memory (sanitized) ✅ All 26 project service tests pass ✅ Local mode continues to work with arbitrary paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 14c1fe4 commit 514bf4f

4 files changed

Lines changed: 172 additions & 6 deletions

File tree

src/basic_memory/api/routers/project_router.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ async def add_project(
194194
Response confirming the project was added
195195
"""
196196
try: # pragma: no cover
197+
# The service layer now handles cloud mode validation and path sanitization
197198
await project_service.add_project(
198199
project_data.name, project_data.path, set_default=project_data.set_default
199200
)

src/basic_memory/services/project_service.py

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,30 @@ async def add_project(self, name: str, path: str, set_default: bool = False) ->
100100
ValueError: If the project already exists
101101
"""
102102
# in cloud mode, don't allow arbitrary paths.
103-
if config.cloud_mode:
103+
if self.config_manager.config.cloud_mode_enabled:
104104
basic_memory_home = os.getenv("BASIC_MEMORY_HOME")
105105
assert basic_memory_home is not None
106106
base_path = Path(basic_memory_home)
107107

108-
# Resolve to absolute path
109-
resolved_path = Path(os.path.abspath(os.path.expanduser(base_path / path))).as_posix()
108+
# Sanitize the input path for cloud mode
109+
# Strip leading slashes, home directory references, and parent directory references
110+
clean_path = path.lstrip("/").replace("~/", "").replace("~", "")
111+
112+
# Remove any parent directory traversal attempts
113+
path_parts = []
114+
for part in clean_path.split("/"):
115+
if part and part != "." and part != "..":
116+
path_parts.append(part)
117+
clean_path = "/".join(path_parts) if path_parts else ""
118+
119+
# Construct path relative to BASIC_MEMORY_HOME
120+
resolved_path = (base_path / clean_path).resolve().as_posix()
121+
122+
# Verify the resolved path is actually under BASIC_MEMORY_HOME
123+
if not resolved_path.startswith(base_path.resolve().as_posix()):
124+
raise ValueError(
125+
f"Cloud mode requires projects under {basic_memory_home}. Invalid path: {path}"
126+
)
110127
else:
111128
resolved_path = Path(os.path.abspath(os.path.expanduser(path))).as_posix()
112129

test-int/test_disable_permalinks_integration.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,9 @@
11
"""Integration tests for the disable_permalinks configuration."""
22

33
import pytest
4-
from pathlib import Path
5-
from textwrap import dedent
64

75
from basic_memory.config import BasicMemoryConfig
86
from basic_memory.markdown import EntityParser, MarkdownProcessor
9-
from basic_memory.models import Project
107
from basic_memory.repository import (
118
EntityRepository,
129
ObservationRepository,

tests/services/test_project_service.py

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -714,3 +714,154 @@ async def test_synchronize_projects_handles_case_sensitivity_bug(
714714
db_project = await project_service.repository.get_by_name(name)
715715
if db_project:
716716
await project_service.repository.delete(db_project.id)
717+
718+
719+
@pytest.mark.asyncio
720+
async def test_add_project_cloud_mode_sanitizes_paths(
721+
project_service: ProjectService, config_manager: ConfigManager, tmp_path, monkeypatch
722+
):
723+
"""Test that cloud mode sanitizes and validates project paths."""
724+
# Set up cloud mode environment
725+
cloud_home = tmp_path / "app" / "data" / "basic-memory"
726+
cloud_home.mkdir(parents=True, exist_ok=True)
727+
728+
monkeypatch.setenv("BASIC_MEMORY_HOME", str(cloud_home))
729+
monkeypatch.setenv("BASIC_MEMORY_CLOUD_MODE", "true")
730+
731+
# Force reload config to pick up cloud mode
732+
from basic_memory.services import project_service as ps_module
733+
734+
monkeypatch.setattr(ps_module, "config", config_manager.load_config())
735+
736+
test_cases = [
737+
# (input_path, expected_result_path, should_succeed)
738+
("test", str(cloud_home / "test"), True), # Simple relative path
739+
("~/Documents/test", str(cloud_home / "Documents" / "test"), True), # Home directory
740+
(
741+
"/tmp/test",
742+
str(cloud_home / "tmp" / "test"),
743+
True,
744+
), # Absolute path (sanitized to relative)
745+
(
746+
"../../../etc/passwd",
747+
str(cloud_home),
748+
True,
749+
), # Path traversal (all ../ removed, results in cloud_home)
750+
("folder/subfolder", str(cloud_home / "folder" / "subfolder"), True), # Nested path
751+
(
752+
"~/folder/../test",
753+
str(cloud_home / "test"),
754+
True,
755+
), # Mixed patterns (sanitized to just 'test')
756+
]
757+
758+
for i, (input_path, expected_path, should_succeed) in enumerate(test_cases):
759+
test_project_name = f"cloud-test-{i}"
760+
761+
try:
762+
# Add the project
763+
await project_service.add_project(test_project_name, input_path)
764+
765+
if should_succeed:
766+
# Verify the path was sanitized correctly
767+
assert test_project_name in project_service.projects
768+
actual_path = project_service.projects[test_project_name]
769+
770+
# The path should be under cloud_home
771+
assert actual_path.startswith(str(cloud_home)), (
772+
f"Path {actual_path} should start with {cloud_home} for input {input_path}"
773+
)
774+
775+
# Clean up
776+
await project_service.remove_project(test_project_name)
777+
else:
778+
pytest.fail(f"Expected ValueError for input path: {input_path}")
779+
780+
except ValueError as e:
781+
if should_succeed:
782+
pytest.fail(f"Unexpected ValueError for input path {input_path}: {e}")
783+
# Expected failure - continue to next test case
784+
785+
786+
@pytest.mark.asyncio
787+
async def test_add_project_cloud_mode_rejects_escape_attempts(
788+
project_service: ProjectService, config_manager: ConfigManager, tmp_path, monkeypatch
789+
):
790+
"""Test that cloud mode rejects paths that try to escape cloud storage."""
791+
# Set up cloud mode environment
792+
cloud_home = tmp_path / "app" / "data" / "basic-memory"
793+
cloud_home.mkdir(parents=True, exist_ok=True)
794+
795+
# Create a directory outside cloud_home to verify it's not accessible
796+
outside_dir = tmp_path / "outside"
797+
outside_dir.mkdir(parents=True, exist_ok=True)
798+
799+
monkeypatch.setenv("BASIC_MEMORY_HOME", str(cloud_home))
800+
monkeypatch.setenv("BASIC_MEMORY_CLOUD_MODE", "true")
801+
802+
# Force reload config to pick up cloud mode
803+
from basic_memory.services import project_service as ps_module
804+
805+
monkeypatch.setattr(ps_module, "config", config_manager.load_config())
806+
807+
# All of these should succeed by being sanitized to paths under cloud_home
808+
# The sanitization removes dangerous patterns, so they don't escape
809+
safe_after_sanitization = [
810+
"../../../etc/passwd",
811+
"../../.env",
812+
"../../../home/user/.ssh/id_rsa",
813+
]
814+
815+
for i, attack_path in enumerate(safe_after_sanitization):
816+
test_project_name = f"cloud-attack-test-{i}"
817+
818+
try:
819+
# Add the project
820+
await project_service.add_project(test_project_name, attack_path)
821+
822+
# Verify it was sanitized to be under cloud_home
823+
actual_path = project_service.projects[test_project_name]
824+
assert actual_path.startswith(str(cloud_home)), (
825+
f"Sanitized path {actual_path} should be under {cloud_home}"
826+
)
827+
828+
# Clean up
829+
await project_service.remove_project(test_project_name)
830+
831+
except ValueError:
832+
# If it raises ValueError, that's also acceptable for security
833+
pass
834+
835+
836+
@pytest.mark.asyncio
837+
async def test_add_project_local_mode_allows_arbitrary_paths(
838+
project_service: ProjectService, config_manager: ConfigManager, tmp_path, monkeypatch
839+
):
840+
"""Test that local mode (non-cloud) still allows arbitrary paths."""
841+
# Ensure cloud mode is disabled
842+
monkeypatch.setenv("BASIC_MEMORY_CLOUD_MODE", "false")
843+
844+
# Force reload config to pick up local mode
845+
from basic_memory.services import project_service as ps_module
846+
847+
monkeypatch.setattr(ps_module, "config", config_manager.load_config())
848+
849+
# Create a test directory
850+
test_dir = tmp_path / "arbitrary-location"
851+
test_dir.mkdir(parents=True, exist_ok=True)
852+
853+
test_project_name = "local-mode-test"
854+
855+
try:
856+
# In local mode, we should be able to use arbitrary absolute paths
857+
await project_service.add_project(test_project_name, str(test_dir))
858+
859+
# Verify the path was accepted as-is
860+
assert test_project_name in project_service.projects
861+
actual_path = project_service.projects[test_project_name]
862+
assert actual_path == str(test_dir)
863+
864+
finally:
865+
# Clean up
866+
if test_project_name in project_service.projects:
867+
await project_service.remove_project(test_project_name)

0 commit comments

Comments
 (0)