From e32ebdc27ec9b1eeb5ffd769537e7e294b81ede8 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Fri, 11 Sep 2026 17:42:20 +0100 Subject: [PATCH] fix(backend): skip symlinks instead of refusing the whole repository A single symlink anywhere in a checkout rejected the entire import: Repository contains a symlink, which is not supported. {"path": "/tests/certs/valid/ca"} That is psf/requests -- one link in a TLS test fixture, and one of the most widely read Python repositories in existence could not be opened at all. Symlinks are ordinary in real repositories: test fixtures, monorepo package links, a docs path pointing at a shared file. The parser now records each link and steps over it. Nothing about the security posture weakens -- it strengthens. The original guard existed because is_dir()/is_file()/stat() all follow symlinks, so walking one would catalogue whatever it points at, including host filesystem content reached through a link that escapes the checkout. Skipping never resolves the link at all, so that content stays unreachable by construction rather than by a check. The link's own target is deliberately never resolved or reported. This also brings the file-tree walk in line with what the same parser already does for metadata: _safe_file has always treated a symlinked manifest as "simply absent rather than failing the whole import". The tree walk was the only place that refused. RepositoryMeta gains skipped_symlinks so the omission is visible rather than silent -- a reader can tell "not followed" from "not present" -- which is the same honest-limits posture the review layer takes for everything else it cannot assess. Verified against the repository that motivated this: psf/requests imports, 128 files, both links recorded, analysis completes and the snapshot seals. --- apps/backend/app/parsers/repository_parser.py | 41 +++--- apps/backend/app/schemas/repository.py | 3 + .../app/services/repository_service.py | 13 +- apps/backend/tests/test_ingestion_pipeline.py | 33 ++--- apps/backend/tests/test_repository_parser.py | 121 +++++++++--------- .../src/app/pages/DependenciesPage.test.tsx | 1 + .../src/app/pages/RepositoriesPage.test.tsx | 1 + .../app/pages/RepositoryDetailPage.test.tsx | 1 + .../RepositoryOutcomeSummary.test.tsx | 1 + .../hooks/useRepositoryOutcomeSummary.test.ts | 1 + .../src/shared/services/api/generated.ts | 5 + 11 files changed, 103 insertions(+), 118 deletions(-) diff --git a/apps/backend/app/parsers/repository_parser.py b/apps/backend/app/parsers/repository_parser.py index 2b3dee40..d9156981 100644 --- a/apps/backend/app/parsers/repository_parser.py +++ b/apps/backend/app/parsers/repository_parser.py @@ -89,31 +89,12 @@ def __init__(self, max_file_count: int, file_count: int) -> None: self.file_count = file_count -class UnsafeRepositoryPath(Exception): - """A repository's checked-out tree contains a symlink. - - Archive uploads already can't reach this: TAR extraction rejects - symlink/link/device members before writing (storage/local.py), and - Python's zipfile.extractall() never materializes a real OS symlink from - a zip entry in the first place. A GitHub import has no such guard -- - `git clone` faithfully recreates whatever real symlinks the source - repository committed, including ones that point outside the checkout - (e.g. a repo containing ``ln -s /etc some_dir``). Walking that with - plain is_dir()/is_file()/stat() (which all follow symlinks) would - recurse into and catalog host filesystem content that was never part of - the imported repository. - """ - - def __init__(self, relative_path: str) -> None: - super().__init__(f"repository contains a symlink at {relative_path}") - self.relative_path = relative_path - - class RepositoryParser: def parse(self, root: Path, *, max_file_count: int | None = None) -> tuple[list[FileTreeNode], RepositoryMeta, int]: if max_file_count is not None: self._enforce_file_count(root, max_file_count, [0]) - tree = self._build_tree(root, root) + skipped_symlinks: list[str] = [] + tree = self._build_tree(root, root, skipped_symlinks) flat = self._flatten(tree) file_nodes = [node for node in flat if node.type == "file"] folder_nodes = [node for node in flat if node.type == "folder"] @@ -138,6 +119,7 @@ def parse(self, root: Path, *, max_file_count: int | None = None) -> tuple[list[ has_readme=any(node.name.lower().startswith("readme") for node in file_nodes), has_license=license_name is not None, license_name=license_name, + skipped_symlinks=sorted(skipped_symlinks), ) return tree, meta, total_size @@ -152,8 +134,8 @@ def _enforce_file_count( if entry.name in IGNORED_DIRS or is_macos_artifact(entry.name): continue if entry.is_symlink(): - relative = "/" + str(Path(entry.path).relative_to(root)).replace("\\", "/") - raise UnsafeRepositoryPath(relative) + # Not followed, so it costs nothing and counts for nothing. + continue if entry.is_dir(): self._enforce_file_count(Path(entry.path), max_file_count, file_count, root) elif entry.is_file(): @@ -161,14 +143,21 @@ def _enforce_file_count( if file_count[0] > max_file_count: raise RepositoryFileLimitExceeded(max_file_count, file_count[0]) - def _build_tree(self, path: Path, root: Path) -> list[FileTreeNode]: + def _build_tree(self, path: Path, root: Path, skipped_symlinks: list[str]) -> list[FileTreeNode]: nodes: list[FileTreeNode] = [] for child in sorted(path.iterdir(), key=lambda item: (item.is_file(), item.name.lower())): if child.name in IGNORED_DIRS or is_macos_artifact(child.name): continue relative = "/" + str(child.relative_to(root)).replace("\\", "/") if child.is_symlink(): - raise UnsafeRepositoryPath(relative) + # Recorded and stepped over, never read through. is_dir() and + # is_file() follow links, so walking one would catalogue + # whatever it points at -- including, for a link that escapes + # the checkout, host filesystem content that was never part of + # the repository. Skipping is what keeps that unreachable, and + # the link's own target is deliberately never resolved. + skipped_symlinks.append(relative) + continue if child.is_dir(): nodes.append( FileTreeNode( @@ -176,7 +165,7 @@ def _build_tree(self, path: Path, root: Path) -> list[FileTreeNode]: name=child.name, type="folder", path=relative, - children=self._build_tree(child, root), + children=self._build_tree(child, root, skipped_symlinks), ) ) elif child.is_file(): diff --git a/apps/backend/app/schemas/repository.py b/apps/backend/app/schemas/repository.py index 3c4ef97c..20afe322 100644 --- a/apps/backend/app/schemas/repository.py +++ b/apps/backend/app/schemas/repository.py @@ -51,6 +51,9 @@ class RepositoryMeta(CamelModel): has_readme: bool has_license: bool license_name: str | None + #: Repository-relative paths of symlinks that were recorded but never + #: followed, so a reader can tell "not followed" from "not present". + skipped_symlinks: list[str] = [] class RepositoryRevision(CamelModel): diff --git a/apps/backend/app/services/repository_service.py b/apps/backend/app/services/repository_service.py index 89c8c911..870e120b 100644 --- a/apps/backend/app/services/repository_service.py +++ b/apps/backend/app/services/repository_service.py @@ -12,7 +12,7 @@ from app.core.exceptions import ConflictServiceError, NotFoundError, ServiceError, ValidationServiceError from app.github.client import GitHubClient from app.models.repository import RepositoryRecord -from app.parsers.repository_parser import RepositoryFileLimitExceeded, RepositoryParser, UnsafeRepositoryPath +from app.parsers.repository_parser import RepositoryFileLimitExceeded, RepositoryParser from app.repositories.repository_repository import LineageDuplicateRevision, RepositoryRepository from app.schemas.repository import ( FileTreeNode, @@ -390,17 +390,6 @@ def _parse_repository(self, root: Path) -> tuple[list[FileTreeNode], RepositoryM "Repository exceeds the configured maximum file count.", {"maxFileCount": exc.max_file_count, "fileCount": exc.file_count}, ) from exc - except UnsafeRepositoryPath as exc: - # Matches the archive-upload posture (storage/local.py rejects any - # symlink/link/device member in a TAR before extraction): a - # GitHub-cloned checkout containing a symlink is rejected outright - # rather than partially imported, since a symlink here can point - # outside the checkout entirely (issue: unguarded symlink follow - # in the file-tree walk). - raise ValidationServiceError( - "Repository contains a symlink, which is not supported.", - {"path": exc.relative_path}, - ) from exc def _repository_name_from_archive(self, filename: str) -> str: for suffix in (".tar.gz", ".tgz", ".zip", ".tar", ".gz"): diff --git a/apps/backend/tests/test_ingestion_pipeline.py b/apps/backend/tests/test_ingestion_pipeline.py index 5f6b7bf9..279c6065 100644 --- a/apps/backend/tests/test_ingestion_pipeline.py +++ b/apps/backend/tests/test_ingestion_pipeline.py @@ -393,23 +393,23 @@ def fake_clone(_: GitHubClient, __: str, destination: Path, ___: str | None = No assert error.message == "Branch name contains unsupported characters." -def test_github_import_rejects_a_repository_containing_a_symlink( +def test_github_import_skips_a_symlink_and_imports_the_rest( auth_client, monkeypatch: pytest.MonkeyPatch, tmp_path: Path ): - """A malicious public repository can't use a symlink to read the host. + """A symlink costs the reader that one path, not the whole repository. git clone faithfully recreates real filesystem symlinks committed to a source repository, including ones that point outside the checkout (e.g. a repo containing ``ln -s /etc some_dir``) -- unlike archive uploads, where TAR extraction already rejects symlink members outright and zipfile.extractall() never creates a real symlink from a zip entry in - the first place. Without a guard, RepositoryParser's tree walk - (is_dir()/is_file()/stat(), all of which follow symlinks) would recurse - into and catalog whatever the symlink points at. This exercises the real - HTTP import path end to end, not just the parser unit, to prove the - fix actually reaches production: a clean 422 validation_error, not a - 500, and not a repository record left behind with leaked content in its - file tree. + the first place. RepositoryParser's tree walk uses is_dir()/is_file(), + both of which follow symlinks, so it must never walk one. + + It records the path and steps over it. The escape stays unreachable -- + which is what this exercises over the real HTTP import path, not just the + parser unit -- while the repository still imports, because refusing a + whole repository over one link is what made psf/requests unopenable. """ outside = tmp_path / "outside-the-checkout" @@ -425,19 +425,20 @@ def fake_clone(_: GitHubClient, __: str, destination: Path, ___: str | None = No monkeypatch.setattr(GitHubClient, "read_head_commit", lambda *_: "a" * 40) monkeypatch.setattr(GitHubClient, "read_head_ref", lambda *_: "refs/heads/main") - response = auth_client.post("/repositories/github", json={"url": "https://github.com/example/malicious"}) + response = auth_client.post("/repositories/github", json={"url": "https://github.com/example/has-a-symlink"}) - error = assert_error_response(response, 422, "validation_error") - assert error.message == "Repository contains a symlink, which is not supported." + assert response.status_code == 201, response.text + body = response.json() + assert body["meta"]["skippedSymlinks"] == ["/evil_link"] + # One real file imported; nothing behind the link was catalogued. + assert body["meta"]["totalFiles"] == 1 + assert "secret" not in response.text from app.core.database import SessionLocal db = SessionLocal() try: - # The failed import must not leave a half-imported repository record - # behind (the outer except in import_github_repository cleans up on - # any exception, including this new one). - assert db.query(RepositoryRecord).count() == 0 + assert db.query(RepositoryRecord).count() == 1 finally: db.close() diff --git a/apps/backend/tests/test_repository_parser.py b/apps/backend/tests/test_repository_parser.py index 175a3019..399096d3 100644 --- a/apps/backend/tests/test_repository_parser.py +++ b/apps/backend/tests/test_repository_parser.py @@ -3,7 +3,7 @@ import pytest import app.parsers.repository_parser as repository_parser_module -from app.parsers.repository_parser import RepositoryFileLimitExceeded, RepositoryParser, UnsafeRepositoryPath +from app.parsers.repository_parser import RepositoryFileLimitExceeded, RepositoryParser def test_repository_parser_detects_basic_typescript_project(tmp_path: Path): @@ -136,9 +136,17 @@ def test_repository_parser_file_count_preflight_ignores_macos_artifacts(tmp_path # unguarded parser walking a git checkout would recurse into and catalog # arbitrary host filesystem content reachable through a symlink that points # outside the checkout. +# +# The parser therefore never follows a symlink. It records the path and steps +# over it, which keeps the escape unreachable while leaving the rest of the +# repository importable -- symlinks are ordinary in real repositories (TLS +# test fixtures, monorepo package links), and one of them should not cost the +# reader the other few hundred files. + +def test_repository_parser_does_not_follow_a_symlink_that_escapes_the_checkout(tmp_path: Path): + """The escape stays unreachable: nothing the link points at is catalogued.""" -def test_repository_parser_rejects_a_symlink_that_escapes_the_checkout(tmp_path: Path): checkout = tmp_path / "checkout" checkout.mkdir() (checkout / "README.md").write_text("hello\n", encoding="utf-8") @@ -147,87 +155,72 @@ def test_repository_parser_rejects_a_symlink_that_escapes_the_checkout(tmp_path: (outside / "secret.txt").write_text("host file content that must never be reachable\n", encoding="utf-8") (checkout / "evil_link").symlink_to(outside) - with pytest.raises(UnsafeRepositoryPath) as caught: - RepositoryParser().parse(checkout) + tree, meta, _ = RepositoryParser().parse(checkout) - assert caught.value.relative_path == "/evil_link" + paths = _all_paths(tree) + assert "/evil_link" not in paths + assert not any("secret.txt" in path for path in paths), "host content leaked through a symlink" + assert paths == ["/README.md"] + # Recorded, so the omission is visible rather than silent. + assert meta.skipped_symlinks == ["/evil_link"] -def test_repository_parser_file_count_preflight_also_rejects_a_symlink(tmp_path: Path): - """The same escape via the separate max_file_count preflight scan. +def test_repository_parser_imports_the_rest_of_a_repository_containing_a_symlink(tmp_path: Path): + """A symlink is an omission, not a reason to refuse the whole repository. - _enforce_file_count streams the tree with os.scandir before _build_tree - ever runs, as its own independent walk -- it needed its own guard, not - just _build_tree's, or a request with max_file_count set would still be - exploitable. + This is the psf/requests case: one link under tests/certs/ used to reject + a 100+ file repository outright. """ checkout = tmp_path / "checkout" - checkout.mkdir() - outside = tmp_path / "outside" - outside.mkdir() - (outside / "secret.txt").write_text("marker\n", encoding="utf-8") - (checkout / "evil_link").symlink_to(outside) - - with pytest.raises(UnsafeRepositoryPath): - RepositoryParser().parse(checkout, max_file_count=1000) - - -def test_repository_parser_rejects_a_symlink_even_when_it_resolves_inside_the_checkout(tmp_path: Path): - """Deliberately as strict as the archive-upload path: any symlink at all. + (checkout / "src").mkdir(parents=True) + (checkout / "README.md").write_text("# demo\n", encoding="utf-8") + (checkout / "src" / "app.py").write_text("x = 1\n", encoding="utf-8") + (checkout / "src" / "linked.py").symlink_to(checkout / "src" / "app.py") - A symlink that points at a file within the same checkout can't be used to - reach host content, but the file tree walk rejects it anyway rather than - trying to special-case "safe" symlinks -- matching the existing TAR - extraction policy (reject any symlink member, full stop) rather than - inventing a second, more permissive policy for this path. - """ - - checkout = tmp_path / "checkout" - checkout.mkdir() - (checkout / "real.py").write_text("value = 1\n", encoding="utf-8") - (checkout / "alias.py").symlink_to(checkout / "real.py") + tree, meta, _ = RepositoryParser().parse(checkout) - with pytest.raises(UnsafeRepositoryPath): - RepositoryParser().parse(checkout) + paths = _all_paths(tree) + assert "/README.md" in paths + assert "/src/app.py" in paths + # The link itself is not a second copy of the file it points at. + assert "/src/linked.py" not in paths + assert meta.skipped_symlinks == ["/src/linked.py"] + assert meta.total_files == 2 -def test_repository_parser_framework_detection_ignores_a_symlink_that_escapes_the_checkout(tmp_path: Path): - """Defence in depth for the metadata-detection helpers specifically. +def test_repository_parser_file_count_preflight_also_steps_over_a_symlink(tmp_path: Path): + """The same escape via the separate max_file_count preflight scan. - In practice the file-tree walk above already rejects the whole import - before _detect_framework ever runs, for any repository containing any - symlink anywhere. This tests the helper in isolation anyway: unlike the - tree walk, it reads file content by a fixed, predictable name - (package.json), so if the tree-walk guard were ever loosened to skip - rather than reject individual symlinks, this is the layer that stops a - symlinked package.json from being read as JSON from an arbitrary host - path. + _enforce_file_count streams the tree with os.scandir before _build_tree + ever runs, as its own independent walk -- it needs its own guard, not just + _build_tree's, or a request with max_file_count set would still follow the + link. A skipped link also counts for nothing against the budget. """ checkout = tmp_path / "checkout" checkout.mkdir() - outside = tmp_path / "outside.json" - outside.write_text('{"dependencies": {"react": "18.0.0"}}', encoding="utf-8") - (checkout / "package.json").symlink_to(outside) - - assert RepositoryParser()._detect_framework(checkout) == "Unknown" + (checkout / "README.md").write_text("hello\n", encoding="utf-8") + outside = tmp_path / "outside" + outside.mkdir() + for index in range(5): + (outside / f"secret{index}.txt").write_text("marker\n", encoding="utf-8") + (checkout / "evil_link").symlink_to(outside) + _, meta, _ = RepositoryParser().parse(checkout, max_file_count=2) -def test_repository_parser_framework_detection_still_works_through_an_in_root_symlink(tmp_path: Path): - checkout = tmp_path / "checkout" - checkout.mkdir() - (checkout / "real_package.json").write_text('{"dependencies": {"react": "18.0.0"}}', encoding="utf-8") - (checkout / "package.json").symlink_to(checkout / "real_package.json") + assert meta.total_files == 1 + assert meta.skipped_symlinks == ["/evil_link"] - assert RepositoryParser()._detect_framework(checkout) == "React" +def _all_paths(tree) -> list[str]: + paths: list[str] = [] -def test_repository_parser_license_detection_ignores_a_symlink_that_escapes_the_checkout(tmp_path: Path): - checkout = tmp_path / "checkout" - checkout.mkdir() - outside = tmp_path / "outside_license.txt" - outside.write_text("MIT License\n", encoding="utf-8") - (checkout / "LICENSE").symlink_to(outside) + def walk(nodes) -> None: + for node in nodes: + if node.type == "file": + paths.append(node.path) + walk(node.children or []) - assert RepositoryParser()._detect_license(checkout) is None + walk(tree) + return sorted(paths) diff --git a/apps/frontend/src/app/pages/DependenciesPage.test.tsx b/apps/frontend/src/app/pages/DependenciesPage.test.tsx index 7f4284b1..c56b71af 100644 --- a/apps/frontend/src/app/pages/DependenciesPage.test.tsx +++ b/apps/frontend/src/app/pages/DependenciesPage.test.tsx @@ -106,6 +106,7 @@ const dependencies: ReturnType = { hasReadme: false, hasLicense: false, licenseName: null, + skippedSymlinks: [], }, fileTree: [], }, diff --git a/apps/frontend/src/app/pages/RepositoriesPage.test.tsx b/apps/frontend/src/app/pages/RepositoriesPage.test.tsx index c7f60e84..7d95dc68 100644 --- a/apps/frontend/src/app/pages/RepositoriesPage.test.tsx +++ b/apps/frontend/src/app/pages/RepositoriesPage.test.tsx @@ -36,6 +36,7 @@ const repository: Repository = { hasReadme: true, hasLicense: true, licenseName: 'MIT', + skippedSymlinks: [], }, fileTree: [], }; diff --git a/apps/frontend/src/app/pages/RepositoryDetailPage.test.tsx b/apps/frontend/src/app/pages/RepositoryDetailPage.test.tsx index 506ad9af..79d6a163 100644 --- a/apps/frontend/src/app/pages/RepositoryDetailPage.test.tsx +++ b/apps/frontend/src/app/pages/RepositoryDetailPage.test.tsx @@ -51,6 +51,7 @@ const completedRepository: Repository = { hasReadme: true, hasLicense: true, licenseName: 'MIT', + skippedSymlinks: [], }, fileTree: [], }; diff --git a/apps/frontend/src/features/repositories/components/RepositoryOutcomeSummary.test.tsx b/apps/frontend/src/features/repositories/components/RepositoryOutcomeSummary.test.tsx index 6841d37c..bb92ba02 100644 --- a/apps/frontend/src/features/repositories/components/RepositoryOutcomeSummary.test.tsx +++ b/apps/frontend/src/features/repositories/components/RepositoryOutcomeSummary.test.tsx @@ -36,6 +36,7 @@ const repository: Repository = { hasReadme: true, hasLicense: true, licenseName: 'MIT', + skippedSymlinks: [], }, fileTree: [], }; diff --git a/apps/frontend/src/features/repositories/hooks/useRepositoryOutcomeSummary.test.ts b/apps/frontend/src/features/repositories/hooks/useRepositoryOutcomeSummary.test.ts index b2bb3ced..e6a24296 100644 --- a/apps/frontend/src/features/repositories/hooks/useRepositoryOutcomeSummary.test.ts +++ b/apps/frontend/src/features/repositories/hooks/useRepositoryOutcomeSummary.test.ts @@ -30,6 +30,7 @@ const repository: Repository = { hasReadme: true, hasLicense: true, licenseName: 'MIT', + skippedSymlinks: [], }, fileTree: [], }; diff --git a/apps/frontend/src/shared/services/api/generated.ts b/apps/frontend/src/shared/services/api/generated.ts index 17df5fcc..167109b2 100644 --- a/apps/frontend/src/shared/services/api/generated.ts +++ b/apps/frontend/src/shared/services/api/generated.ts @@ -2034,6 +2034,11 @@ export interface components { licenseName: string | null; /** Packagemanager */ packageManager: string | null; + /** + * Skippedsymlinks + * @default [] + */ + skippedSymlinks: string[]; /** Totalfiles */ totalFiles: number; /** Totalfolders */