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
41 changes: 15 additions & 26 deletions apps/backend/app/parsers/repository_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand All @@ -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

Expand All @@ -152,31 +134,38 @@ 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():
file_count[0] += 1
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(
id=str(uuid4()),
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():
Expand Down
3 changes: 3 additions & 0 deletions apps/backend/app/schemas/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
13 changes: 1 addition & 12 deletions apps/backend/app/services/repository_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"):
Expand Down
33 changes: 17 additions & 16 deletions apps/backend/tests/test_ingestion_pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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()

Expand Down
121 changes: 57 additions & 64 deletions apps/backend/tests/test_repository_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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")
Expand All @@ -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)
1 change: 1 addition & 0 deletions apps/frontend/src/app/pages/DependenciesPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ const dependencies: ReturnType<typeof useDependencies> = {
hasReadme: false,
hasLicense: false,
licenseName: null,
skippedSymlinks: [],
},
fileTree: [],
},
Expand Down
1 change: 1 addition & 0 deletions apps/frontend/src/app/pages/RepositoriesPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const repository: Repository = {
hasReadme: true,
hasLicense: true,
licenseName: 'MIT',
skippedSymlinks: [],
},
fileTree: [],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const completedRepository: Repository = {
hasReadme: true,
hasLicense: true,
licenseName: 'MIT',
skippedSymlinks: [],
},
fileTree: [],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const repository: Repository = {
hasReadme: true,
hasLicense: true,
licenseName: 'MIT',
skippedSymlinks: [],
},
fileTree: [],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const repository: Repository = {
hasReadme: true,
hasLicense: true,
licenseName: 'MIT',
skippedSymlinks: [],
},
fileTree: [],
};
Expand Down
5 changes: 5 additions & 0 deletions apps/frontend/src/shared/services/api/generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2034,6 +2034,11 @@ export interface components {
licenseName: string | null;
/** Packagemanager */
packageManager: string | null;
/**
* Skippedsymlinks
* @default []
*/
skippedSymlinks: string[];
/** Totalfiles */
totalFiles: number;
/** Totalfolders */
Expand Down
Loading