Skip to content

Commit 18ad8e4

Browse files
phernandezclaude
andcommitted
fix(api): stop canonical path matching at the project boundary
_canonical_file_path followed escaping symlinks with os.scandir before the post-canonicalization containment check rejected the request: on a case-sensitive filesystem a wrong-cased request like 'LINK/secret.md' passes the pre-check (the path does not exist, so resolve() never follows the real 'link' symlink), and segment matching then descended through 'link' and scanned the outside directory — an information touch beyond the project boundary even though the request was ultimately rejected. Re-check the boundary at every traversal step: before scanning the current directory, its fully-resolved path must still be contained in the resolved project home; bail with None the moment resolution escapes. The post-canonicalization checks remain as defense in depth, and symlinks whose targets stay inside the project keep canonicalizing as before. Regression tests spy on os.scandir to prove the outside directory is never scanned (meaningful on case-insensitive macOS too, where the pre-check already rejects the request), and exercise the boundary bail directly so it is covered on every filesystem. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent b4c044a commit 18ad8e4

2 files changed

Lines changed: 105 additions & 11 deletions

File tree

src/basic_memory/api/v2/routers/knowledge_router.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,23 @@ def _canonical_file_path(home: pathlib.Path, segments: list[str]) -> str | None:
260260
Outcome: each segment is matched against real directory entries — exact name first
261261
(so distinct case-variant files on case-sensitive filesystems stay distinct),
262262
then a unique case-insensitive match. Returns None when any segment cannot be
263-
matched to exactly one entry, including missing files.
263+
matched to exactly one entry, including missing files. Traversal stops at the
264+
project boundary: a directory whose resolved path escapes the project home is
265+
never scanned.
264266
"""
267+
resolved_home = home.resolve()
265268
current = home
266269
canonical_segments: list[str] = []
267270
for segment in segments:
271+
# Trigger: a previously matched segment may be a symlink whose target lies
272+
# outside the project root (e.g. wrong-cased 'LINK' matched the on-disk
273+
# 'link' -> /tmp/outside on a case-sensitive filesystem).
274+
# Why: os.scandir follows symlinked directories, so continuing would read
275+
# directory contents outside the project boundary even though the
276+
# post-canonicalization containment check rejects the request later.
277+
# Outcome: bail before scanning the moment resolution escapes the home.
278+
if not current.resolve().is_relative_to(resolved_home):
279+
return None
268280
try:
269281
with os.scandir(current) as entries_iter:
270282
entries = [entry.name for entry in entries_iter]

tests/api/v2/test_knowledge_router.py

Lines changed: 92 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
"""Tests for V2 knowledge graph API routes (ID-based endpoints)."""
22

3+
import os
34
from datetime import datetime, timezone
45
from pathlib import Path
56
import uuid
67

78
import pytest
89
from httpx import AsyncClient
910

11+
from basic_memory.api.v2.routers.knowledge_router import _canonical_file_path
1012
from basic_memory.ignore_utils import get_bmignore_path
1113
from basic_memory.models import Entity as EntityModel, Project
1214
from basic_memory.repository.entity_repository import EntityRepository
@@ -1045,9 +1047,9 @@ async def test_sync_file_rejects_symlink_escape(
10451047
boundary check: the path exists, so resolve() follows the symlink and detects the
10461048
escape. The wrong-cased request ('LINK/secret.md') is the regression case — on a
10471049
case-sensitive filesystem that path does not exist, the pre-check resolves it
1048-
lexically and passes, and only canonicalization rewrites it to the real 'link'
1049-
segment; the post-canonicalization containment check must reject it. On
1050-
case-insensitive filesystems the pre-check catches both. Either way: 400.
1050+
lexically and passes; canonicalization then matches the real 'link' segment but
1051+
stops at the project boundary and reports the path as not found (404). On
1052+
case-insensitive filesystems the pre-check catches both with 400.
10511053
"""
10521054
project_path = Path(test_project.path)
10531055
outside_dir = project_path.parent / "sync-file-outside"
@@ -1057,18 +1059,98 @@ async def test_sync_file_rejects_symlink_escape(
10571059
)
10581060
(project_path / "link").symlink_to(outside_dir, target_is_directory=True)
10591061

1060-
for requested in ("link/secret.md", "LINK/secret.md"):
1061-
response = await client.post(
1062-
f"{v2_project_url}/knowledge/sync-file",
1063-
json={"file_path": requested},
1064-
)
1065-
assert response.status_code == 400, requested
1066-
assert "project boundaries" in response.json()["detail"]
1062+
response = await client.post(
1063+
f"{v2_project_url}/knowledge/sync-file",
1064+
json={"file_path": "link/secret.md"},
1065+
)
1066+
assert response.status_code == 400
1067+
assert "project boundaries" in response.json()["detail"]
1068+
1069+
# 400 (pre-check, case-insensitive FS) or 404 (canonicalization stops at the
1070+
# boundary, case-sensitive FS) — either way the escape is rejected.
1071+
response = await client.post(
1072+
f"{v2_project_url}/knowledge/sync-file",
1073+
json={"file_path": "LINK/secret.md"},
1074+
)
1075+
assert response.status_code in (400, 404)
10671076

10681077
# Nothing outside the project root was indexed
10691078
assert await entity_repository.find_all() == []
10701079

10711080

1081+
@pytest.mark.asyncio
1082+
async def test_sync_file_symlink_escape_never_scans_outside_directory(
1083+
client: AsyncClient,
1084+
v2_project_url,
1085+
test_project: Project,
1086+
entity_repository,
1087+
monkeypatch: pytest.MonkeyPatch,
1088+
):
1089+
"""Canonicalization never scans directories outside the project root.
1090+
1091+
Rejecting the request is not enough: before this fix, the wrong-cased request
1092+
('LINK/secret.md') passed the pre-check on a case-sensitive filesystem, and
1093+
canonicalization followed the escaping 'link' symlink with os.scandir before the
1094+
post-canonicalization containment check fired — an information touch outside the
1095+
boundary. We spy on os.scandir and assert the outside directory is never scanned.
1096+
The spy is what makes this test meaningful on case-insensitive macOS too, where
1097+
the request is already rejected by the pre-check: the assertion proves no layer
1098+
scanned past the boundary either way.
1099+
"""
1100+
project_path = Path(test_project.path)
1101+
outside_dir = (project_path.parent / "sync-file-outside-scan").resolve()
1102+
outside_dir.mkdir(parents=True, exist_ok=True)
1103+
(outside_dir / "secret.md").write_text(
1104+
"# Outside\n\nMust never be scanned.\n", encoding="utf-8"
1105+
)
1106+
(project_path / "link").symlink_to(outside_dir, target_is_directory=True)
1107+
1108+
real_scandir = os.scandir
1109+
scanned: list[Path] = []
1110+
1111+
def recording_scandir(path=".", *args, **kwargs):
1112+
# Record the resolved path so a scandir on the symlinked 'link' directory
1113+
# shows up as the outside directory it actually reads.
1114+
scanned.append(Path(path).resolve())
1115+
return real_scandir(path, *args, **kwargs)
1116+
1117+
monkeypatch.setattr(os, "scandir", recording_scandir)
1118+
1119+
response = await client.post(
1120+
f"{v2_project_url}/knowledge/sync-file",
1121+
json={"file_path": "LINK/secret.md"},
1122+
)
1123+
assert response.status_code in (400, 404)
1124+
assert outside_dir not in scanned
1125+
1126+
assert await entity_repository.find_all() == []
1127+
1128+
1129+
def test_canonical_file_path_stops_at_project_boundary(tmp_path: Path):
1130+
"""_canonical_file_path bails before descending past the resolved project home.
1131+
1132+
Exercised directly (not via the endpoint) so the boundary bail is covered on
1133+
case-insensitive filesystems too, where the endpoint pre-check rejects the
1134+
request before canonicalization runs.
1135+
"""
1136+
home = tmp_path / "project"
1137+
home.mkdir()
1138+
outside_dir = tmp_path / "outside"
1139+
outside_dir.mkdir()
1140+
(outside_dir / "secret.md").write_text("# Outside\n", encoding="utf-8")
1141+
(home / "link").symlink_to(outside_dir, target_is_directory=True)
1142+
1143+
assert _canonical_file_path(home, ["link", "secret.md"]) is None
1144+
1145+
# Symlinks that stay inside the project keep canonicalizing as before.
1146+
real_dir = home / "real"
1147+
real_dir.mkdir()
1148+
(real_dir / "inside.md").write_text("# Inside\n", encoding="utf-8")
1149+
(home / "alias").symlink_to(real_dir, target_is_directory=True)
1150+
1151+
assert _canonical_file_path(home, ["alias", "inside.md"]) == "alias/inside.md"
1152+
1153+
10721154
@pytest.mark.asyncio
10731155
async def test_sync_file_symlink_inside_project_still_indexes(
10741156
client: AsyncClient, v2_project_url, test_project: Project

0 commit comments

Comments
 (0)