fix: prevent path traversal via Gitea ref/sha/base/head parameters
test / test (push) Successful in 20s
lint / lint (push) Successful in 22s
docker / lint (pull_request) Successful in 33s
docker / test (pull_request) Successful in 25s
test / test (pull_request) Successful in 38s
lint / lint (pull_request) Successful in 40s
docker / docker-test (pull_request) Successful in 15s
docker / docker-publish (pull_request) Has been skipped
test / test (push) Successful in 20s
lint / lint (push) Successful in 22s
docker / lint (pull_request) Successful in 33s
docker / test (pull_request) Successful in 25s
test / test (pull_request) Successful in 38s
lint / lint (pull_request) Successful in 40s
docker / docker-test (pull_request) Successful in 15s
docker / docker-publish (pull_request) Has been skipped
The ref-like tool arguments (ref, sha, base, head) were only length-limited and were interpolated unencoded into Gitea API URL paths (get_tree, get_commit_diff, compare_refs). Because httpx collapses ".." path segments (RFC 3986), a crafted value such as "../../../../owner/repo/contents/secret" escaped the declared owner/repo prefix. In service-PAT mode this allowed a user authorized on one repository to read arbitrary repositories the service token could reach, and in OAuth mode it bypassed the policy engine's per-repository rules (which never see ref values). Two defense layers: - arguments.py: add _validate_git_ref / GitRef that rejects ".." path segments, leading "/", backslashes, null bytes, control chars, whitespace, and "?"/"#", while preserving legitimate slash refs (feature/foo, v1.2.3). This is what actually closes the traversal. - gitea_client.py: defense-in-depth urllib.parse.quote() on owner/repo (safe="") and ref/sha/base/head/filepath (safe="/") in every repo URL builder, mirroring the existing pattern in server.py. Tests: negative cases for traversal/unsafe chars across all four fields, positive cases for slash-containing refs, length-bound regression, and a URL-layer confinement check. Full suite green (176 passed), coverage 85.64%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -5,7 +5,8 @@ from __future__ import annotations
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
from httpx import Request, Response
|
||||
from httpx import Client, Request, Response
|
||||
from pydantic import ValidationError
|
||||
|
||||
from aegis_gitea_mcp.config import reset_settings
|
||||
from aegis_gitea_mcp.gitea_client import (
|
||||
@@ -15,6 +16,11 @@ from aegis_gitea_mcp.gitea_client import (
|
||||
GiteaError,
|
||||
GiteaNotFoundError,
|
||||
)
|
||||
from aegis_gitea_mcp.tools.arguments import (
|
||||
CommitDiffArgs,
|
||||
CompareRefsArgs,
|
||||
FileTreeArgs,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
@@ -166,3 +172,94 @@ async def test_get_file_contents_blocks_oversized_payload(monkeypatch: pytest.Mo
|
||||
|
||||
with pytest.raises(GiteaError, match="exceeds limit"):
|
||||
await client.get_file_contents("acme", "demo", "big.bin")
|
||||
|
||||
|
||||
_MALICIOUS_REFS = [
|
||||
"../../../x/y",
|
||||
"..",
|
||||
"/etc/passwd",
|
||||
"a\x00b",
|
||||
"a?b",
|
||||
"a#b",
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", _MALICIOUS_REFS)
|
||||
def test_file_tree_args_reject_traversal_ref(value: str) -> None:
|
||||
"""Layer 1: FileTreeArgs.ref rejects traversal/unsafe values."""
|
||||
with pytest.raises(ValidationError):
|
||||
FileTreeArgs(owner="o", repo="r", ref=value)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", _MALICIOUS_REFS)
|
||||
def test_commit_diff_args_reject_traversal_sha(value: str) -> None:
|
||||
"""Layer 1: CommitDiffArgs.sha rejects traversal/unsafe values."""
|
||||
# ".." is shorter than the 7-char min_length; still rejected (length or ref check).
|
||||
with pytest.raises(ValidationError):
|
||||
CommitDiffArgs(owner="o", repo="r", sha=value)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", _MALICIOUS_REFS)
|
||||
def test_compare_refs_args_reject_traversal_base(value: str) -> None:
|
||||
"""Layer 1: CompareRefsArgs.base rejects traversal/unsafe values."""
|
||||
with pytest.raises(ValidationError):
|
||||
CompareRefsArgs(owner="o", repo="r", base=value, head="main")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", _MALICIOUS_REFS)
|
||||
def test_compare_refs_args_reject_traversal_head(value: str) -> None:
|
||||
"""Layer 1: CompareRefsArgs.head rejects traversal/unsafe values."""
|
||||
with pytest.raises(ValidationError):
|
||||
CompareRefsArgs(owner="o", repo="r", base="main", head=value)
|
||||
|
||||
|
||||
def test_git_refs_allow_slash_containing_refs() -> None:
|
||||
"""Legitimate refs that contain '/' validate successfully."""
|
||||
tree = FileTreeArgs(owner="o", repo="r", ref="feature/foo")
|
||||
assert tree.ref == "feature/foo"
|
||||
|
||||
compare = CompareRefsArgs(owner="o", repo="r", base="release/1.0", head="main")
|
||||
assert compare.base == "release/1.0"
|
||||
assert compare.head == "main"
|
||||
|
||||
|
||||
def test_git_ref_length_bounds_still_enforced() -> None:
|
||||
"""Field min/max length still applies alongside the AfterValidator."""
|
||||
with pytest.raises(ValidationError):
|
||||
FileTreeArgs(owner="o", repo="r", ref="")
|
||||
with pytest.raises(ValidationError):
|
||||
FileTreeArgs(owner="o", repo="r", ref="a" * 201)
|
||||
with pytest.raises(ValidationError):
|
||||
# sha below 7-char minimum
|
||||
CommitDiffArgs(owner="o", repo="r", sha="abc")
|
||||
|
||||
|
||||
def test_layer2_encoding_confines_unsafe_chars_to_path_segment() -> None:
|
||||
"""Layer 2 defense-in-depth: quote(..., safe='/') confines unsafe chars.
|
||||
|
||||
A sha containing ``?``, ``#``, whitespace, or a backslash that hypothetically
|
||||
bypassed Layer 1 is percent-encoded so it cannot split off a query string,
|
||||
fragment, or extra path component -- it stays inside the single ref segment
|
||||
under the declared ``owner/repo``. (Note: ``..`` collapse is closed by Layer 1
|
||||
validation, since ``quote`` intentionally leaves ``.`` and ``/`` literal to
|
||||
preserve refs like ``feature/foo``.)
|
||||
"""
|
||||
from urllib.parse import quote
|
||||
|
||||
owner, repo = "alice", "repoA"
|
||||
prefix = f"/api/v1/repos/{owner}/{repo}/git/commits/"
|
||||
|
||||
for malicious_sha in ["abc?injected=1", "abc#frag", "abc def", "abc\\..\\evil"]:
|
||||
endpoint = (
|
||||
f"/api/v1/repos/{quote(owner, safe='')}/{quote(repo, safe='')}"
|
||||
f"/git/commits/{quote(malicious_sha, safe='/')}"
|
||||
)
|
||||
with Client(base_url="https://gitea.example.com") as http_client:
|
||||
request = http_client.build_request("GET", endpoint)
|
||||
|
||||
# Stays scoped to declared repo, no query/fragment broke out.
|
||||
assert request.url.path.startswith(prefix)
|
||||
assert request.url.query == b""
|
||||
assert request.url.fragment == ""
|
||||
# Exactly one ref segment after the prefix (no path splitting).
|
||||
assert "/" not in request.url.path[len(prefix) :]
|
||||
|
||||
Reference in New Issue
Block a user