Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/basic_memory/api/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async def lifespan(app: FastAPI): # pragma: no cover
# Instrument httpx client for distributed tracing when in cloud mode
if app_config.cloud_mode_enabled:
try:
import logfire
import logfire # pyright: ignore[reportMissingImports]
from basic_memory.mcp.async_client import client as httpx_client

logger.info("Cloud mode enabled - instrumenting httpx client for distributed tracing")
Expand Down
2 changes: 1 addition & 1 deletion src/basic_memory/schemas/project_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def permalink(self) -> str: # pragma: no cover

@property
def home(self) -> Path: # pragma: no cover
return Path(self.name)
return Path(self.path).expanduser()

@property
def project_url(self) -> str: # pragma: no cover
Expand Down
22 changes: 7 additions & 15 deletions src/basic_memory/services/project_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,21 +138,13 @@ async def add_project(self, name: str, path: str, set_default: bool = False) ->
if project_root:
base_path = Path(project_root)

# Sanitize the input path
# Strip leading slashes, home directory references, and parent directory references
clean_path = path.lstrip("/").replace("~/", "").replace("~", "")

# Remove any parent directory traversal attempts and normalize to lowercase
# to prevent case-sensitivity issues on Linux filesystems
path_parts = []
for part in clean_path.split("/"):
if part and part != "." and part != "..":
# Convert to lowercase to ensure case-insensitive consistency
path_parts.append(part.lower())
clean_path = "/".join(path_parts) if path_parts else ""

# Construct path relative to project_root
resolved_path = (base_path / clean_path).resolve().as_posix()
# In cloud mode (when project_root is set), ignore user's path completely
# and use sanitized project name as the directory name
# This ensures flat structure: /app/data/test-bisync instead of /app/data/documents/test bisync
sanitized_name = generate_permalink(name)

# Construct path using sanitized project name only
resolved_path = (base_path / sanitized_name).resolve().as_posix()

# Verify the resolved path is actually under project_root
if not resolved_path.startswith(base_path.resolve().as_posix()):
Expand Down
58 changes: 58 additions & 0 deletions test-int/mcp/test_write_note_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -432,3 +432,61 @@ async def test_write_note_file_path_os_path_join(mcp_server, test_project):

# Restore original config value
config.kebab_filenames = curr_config_val


@pytest.mark.asyncio
async def test_write_note_project_path_validation(mcp_server, test_project):
"""Test that ProjectItem.home uses expanded path, not name (Issue #340).

Regression test verifying that:
1. ProjectItem.home returns Path(self.path).expanduser()
2. Not Path(self.name) which was the bug

This test verifies the fix works correctly even though in the test environment
the project name and path happen to be the same. The fix in src/basic_memory/schemas/project_info.py:186
ensures .expanduser() is called, which is critical for paths with ~ like "~/Documents/Test BiSync".
"""
from basic_memory.schemas.project_info import ProjectItem
from pathlib import Path

# Test the fix directly: ProjectItem.home should expand tilde paths
project_with_tilde = ProjectItem(
id=1,
name="Test BiSync", # Name differs from path structure
description="Test",
path="~/Documents/Test BiSync", # Path with tilde
is_active=True,
is_default=False,
)

# Before fix: Path("Test BiSync") - wrong!
# After fix: Path("~/Documents/Test BiSync").expanduser() - correct!
home_path = project_with_tilde.home

# Verify it's a Path object
assert isinstance(home_path, Path)

# Verify tilde was expanded (won't contain ~)
assert "~" not in str(home_path)

# Verify it ends with the expected structure (use Path.parts for cross-platform)
assert home_path.parts[-2:] == ("Documents", "Test BiSync")

# Also test that write_note works with regular project
async with Client(mcp_server) as client:
result = await client.call_tool(
"write_note",
{
"project": test_project.name,
"title": "Validation Test",
"folder": "documents",
"content": "Testing path validation",
"tags": "test",
},
)

response_text = result.content[0].text # pyright: ignore [reportAttributeAccessIssue]

# Should successfully create without path validation errors
assert "# Created note" in response_text
assert "not allowed" not in response_text
142 changes: 76 additions & 66 deletions tests/services/test_project_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,15 @@ async def test_synchronize_projects_handles_case_sensitivity_bug(project_service
async def test_add_project_with_project_root_sanitizes_paths(
project_service: ProjectService, config_manager: ConfigManager, monkeypatch
):
"""Test that BASIC_MEMORY_PROJECT_ROOT sanitizes and validates project paths."""
"""Test that BASIC_MEMORY_PROJECT_ROOT uses sanitized project name, ignoring user path.

When project_root is set (cloud mode), the system should:
1. Ignore the user's provided path completely
2. Use the sanitized project name as the directory name
3. Create a flat structure: /app/data/test-bisync instead of /app/data/documents/test bisync

This prevents the bisync auto-discovery bug where nested paths caused duplicate project creation.
"""
with tempfile.TemporaryDirectory() as temp_dir:
# Set up project root environment
project_root_path = Path(temp_dir) / "app" / "data"
Expand All @@ -770,65 +778,48 @@ async def test_add_project_with_project_root_sanitizes_paths(
config_module._CONFIG_CACHE = None

test_cases = [
# (input_path, expected_result_path, should_succeed)
("test", str(project_root_path / "test"), True), # Simple relative path
(
"~/Documents/test",
str(project_root_path / "Documents" / "test"),
True,
), # Home directory
(
"/tmp/test",
str(project_root_path / "tmp" / "test"),
True,
), # Absolute path (sanitized to relative)
(
"../../../etc/passwd",
str(project_root_path),
True,
), # Path traversal (all ../ removed, results in project_root)
(
"folder/subfolder",
str(project_root_path / "folder" / "subfolder"),
True,
), # Nested path
(
"~/folder/../test",
str(project_root_path / "test"),
True,
), # Mixed patterns (sanitized to just 'test')
# (project_name, user_path, expected_sanitized_name)
# User path is IGNORED - only project name matters
("test", "anything/path", "test"),
("Test BiSync", "~/Documents/Test BiSync", "test-bi-sync"), # BiSync -> bi-sync (dash preserved)
("My Project", "/tmp/whatever", "my-project"),
("UPPERCASE", "~", "uppercase"),
("With Spaces", "~/Documents/With Spaces", "with-spaces"),
]

for i, (input_path, expected_path, should_succeed) in enumerate(test_cases):
test_project_name = f"project-root-test-{i}"
for i, (project_name, user_path, expected_sanitized) in enumerate(test_cases):
test_project_name = f"{project_name}-{i}" # Make unique
expected_final_segment = f"{expected_sanitized}-{i}"

try:
# Add the project
await project_service.add_project(test_project_name, input_path)
# Add the project - user_path should be ignored
await project_service.add_project(test_project_name, user_path)

# Verify the path uses sanitized project name, not user path
assert test_project_name in project_service.projects
actual_path = project_service.projects[test_project_name]

# The path should be under project_root (resolve both to handle macOS /private/var)
assert (
Path(actual_path)
.resolve()
.is_relative_to(Path(project_root_path).resolve())
), (
f"Path {actual_path} should be under {project_root_path}"
)

if should_succeed:
# Verify the path was sanitized correctly
assert test_project_name in project_service.projects
actual_path = project_service.projects[test_project_name]

# The path should be under project_root (resolve both to handle macOS /private/var)
assert (
Path(actual_path)
.resolve()
.is_relative_to(Path(project_root_path).resolve())
), (
f"Path {actual_path} should be under {project_root_path} for input {input_path}"
)

# Clean up
await project_service.remove_project(test_project_name)
else:
pytest.fail(f"Expected ValueError for input path: {input_path}")
# Verify the final path segment is the sanitized project name
path_parts = Path(actual_path).parts
final_segment = path_parts[-1]
assert final_segment == expected_final_segment, (
f"Expected path segment '{expected_final_segment}', got '{final_segment}'"
)

# Clean up
await project_service.remove_project(test_project_name)

except ValueError as e:
if should_succeed:
pytest.fail(f"Unexpected ValueError for input path {input_path}: {e}")
# Expected failure - continue to next test case
pytest.fail(f"Unexpected ValueError for project {test_project_name}: {e}")


@pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems")
Expand Down Expand Up @@ -919,12 +910,17 @@ async def test_add_project_without_project_root_allows_arbitrary_paths(
await project_service.remove_project(test_project_name)


@pytest.mark.skip(reason="Obsolete: project_root mode now uses sanitized project name, not user path. See test_add_project_with_project_root_sanitizes_paths instead.")
@pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems")
@pytest.mark.asyncio
async def test_add_project_with_project_root_normalizes_case(
project_service: ProjectService, config_manager: ConfigManager, monkeypatch
):
"""Test that BASIC_MEMORY_PROJECT_ROOT normalizes paths to lowercase."""
"""Test that BASIC_MEMORY_PROJECT_ROOT normalizes paths to lowercase.

NOTE: This test is obsolete. After fixing the bisync duplicate project bug,
project_root mode now ignores the user's path and uses the sanitized project name instead.
"""
with tempfile.TemporaryDirectory() as temp_dir:
# Set up project root environment
project_root_path = Path(temp_dir) / "app" / "data"
Expand Down Expand Up @@ -966,12 +962,17 @@ async def test_add_project_with_project_root_normalizes_case(
pytest.fail(f"Unexpected ValueError for input path {input_path}: {e}")


@pytest.mark.skip(reason="Obsolete: project_root mode now uses sanitized project name, not user path.")
@pytest.mark.skipif(os.name == "nt", reason="Project root constraints only tested on POSIX systems")
@pytest.mark.asyncio
async def test_add_project_with_project_root_detects_case_collisions(
project_service: ProjectService, config_manager: ConfigManager, monkeypatch
):
"""Test that BASIC_MEMORY_PROJECT_ROOT detects case-insensitive path collisions."""
"""Test that BASIC_MEMORY_PROJECT_ROOT detects case-insensitive path collisions.

NOTE: This test is obsolete. After fixing the bisync duplicate project bug,
project_root mode now ignores the user's path and uses the sanitized project name instead.
"""
with tempfile.TemporaryDirectory() as temp_dir:
# Set up project root environment
project_root_path = Path(temp_dir) / "app" / "data"
Expand Down Expand Up @@ -1162,21 +1163,30 @@ async def test_add_project_nested_validation_with_project_root(
child_project_name = f"cloud-child-{os.urandom(4).hex()}"

try:
# Add parent project (normalized to lowercase)
# Add parent project - user path is ignored, uses sanitized project name
await project_service.add_project(parent_project_name, "parent-folder")

# Verify it was created
# Verify it was created using sanitized project name, not user path
assert parent_project_name in project_service.projects
parent_actual_path = project_service.projects[parent_project_name]
# Resolve both paths to handle macOS /private/var vs /var differences
assert (
Path(parent_actual_path).resolve()
== (project_root_path / "parent-folder").resolve()
)

# Try to add a child project nested under parent (should fail)
with pytest.raises(ValueError, match="nested within existing project"):
await project_service.add_project(child_project_name, "parent-folder/child-folder")
# Path should use sanitized project name (cloud-parent-xxx -> cloud-parent-xxx)
# NOT the user-provided path "parent-folder"
assert parent_project_name.lower() in parent_actual_path.lower()
# Resolve both to handle macOS /private/var vs /var
assert Path(parent_actual_path).resolve().is_relative_to(Path(project_root_path).resolve())

# Nested projects should still be prevented, even with user path ignored
# Since paths use project names, this won't actually be nested
# But we can test that two projects can coexist
await project_service.add_project(child_project_name, "parent-folder/child-folder")

# Both should exist with their own paths
assert child_project_name in project_service.projects
child_actual_path = project_service.projects[child_project_name]
assert child_project_name.lower() in child_actual_path.lower()

# Clean up child
await project_service.remove_project(child_project_name)

finally:
# Clean up
Expand Down
Loading