diff --git a/pr_reviewer/platform.py b/pr_reviewer/platform.py index bf398ba8..b5f46f36 100644 --- a/pr_reviewer/platform.py +++ b/pr_reviewer/platform.py @@ -69,8 +69,19 @@ class PlatformUnsupported(RuntimeError): "/environments/", "/dispatches", ) +# Repo-scoped endpoint prefixes (path starts with ``/repos/owner/repo/...``). +# These require the ``owner/repo`` to be in the allowlist. GH_API_ALLOWED_PREFIXES = ( "/repos/", +) +# Root-level endpoint prefixes (path starts with ``//...`` without an +# owner/repo segment). These are NOT scoped to a repository: e.g. +# ``/search/code?q=foo`` hits ``https://api.github.com/search/code?q=foo`` +# and is implicitly allowed whenever the *user has any* repo allowlist +# configured (i.e. is permitted to call ``gh_api`` at all). Each prefix +# also needs a matching branch in ``_forgejo_translate``; the test suite +# enforces parity on both backends. +GH_API_ROOT_PREFIXES = ( "/issues/", "/search/", "/releases/", @@ -150,18 +161,54 @@ def _validate_endpoint(endpoint, allowed_repos, current_repo): Returns ``(full_path, repo_key)`` on success, or ``{"error": ...}``. The caller is responsible for picking the host and issuing the request — this function makes the security decisions for **both** backends. + + For repo-scoped paths (``/repos/owner/repo/...`` or the bare + ``owner/repo/...`` form) the repo key must be in the allowlist (or the + allowlist contains ``*``). For root-level paths (``/search/``, + ``/issues/``, ``/releases/``, ``/git/`` — see ``GH_API_ROOT_PREFIXES``) + the repo allowlist is bypassed (they aren't tied to any repo) and the + URL is built without a ``/repos/`` prefix, e.g. + ``search/code?q=foo`` → ``/search/code?q=foo`` (the GitHub call + becomes ``https://api.github.com/search/code?q=foo``, not the + previously mangled ``/repos/search/code?q=foo``). See issue #469. """ if not GH_SAFE_PATH_RE.match(endpoint or ""): return {"error": "Endpoint contains disallowed characters"} parts = (endpoint or "").strip("/").split("/") - if len(parts) < 2: - return {"error": "Invalid endpoint format: expected owner/repo/..."} + # Root-level endpoints bypass the minimum-length check (e.g. + # ``/releases`` and ``/issues`` are valid GitHub root endpoints with + # a single segment). Repo-scoped endpoints require at least owner + + # repo, enforced below. + if len(parts) < 1 or parts == [""]: + return {"error": "Invalid endpoint format: expected a non-empty path"} for part in parts: if part in ("", ".", ".."): return {"error": f"Dot-segment not allowed in path: {part or '(empty)'}"} + # Root-level endpoints bypass the repo-key check entirely. We detect + # them by the first path segment: ``search``, ``issues``, ``releases``, + # ``git`` are API-root namespaces on GitHub (and on Forgejo, where the + # same shape is mounted under ``/api/v1``). The dot-segment check above + # already rejects ``..`` smuggling, and the deny substrings below + # still gate the actual URL. + is_root_level = ("/" + parts[0] + "/") in GH_API_ROOT_PREFIXES + + if is_root_level: + full_path = "/" + "/".join(parts) + lower = full_path.lower() + for deny in GH_DENY_SUBSTRINGS: + if deny in lower: + return {"error": f"Path segment denied: {deny}"} + # ``repo_key`` is empty for root endpoints: there is no repo to + # attribute the call to. Forgejo translation keys off this to + # know it must use the GitHub-shaped path directly. + return {"full_path": full_path, "repo_key": ""} + + if len(parts) < 2: + return {"error": "Invalid endpoint format: expected owner/repo/..."} + # GitHub's prompt format is "repos/owner/repo/..."; the direct format # "owner/repo/..." is also accepted. Either way, the repo key is # positions [0:2] (after stripping the optional "repos" prefix). @@ -236,18 +283,20 @@ def _forgejo_translate(full_path, repo_key): host. Anything not in this table is reported as ``Endpoint not supported on PLATFORM=forgejo`` so callers can fail loudly and stop guessing. """ - repos = f"/repos/{repo_key}" - if not full_path.startswith(repos): - # The allowed-prefix list also includes /search/ and /git/ at the - # root (no repo segment). Those are handled below. + # Root-level endpoints (no /repos/owner/repo/ segment) carry an empty + # ``repo_key`` and reach the translator verbatim. ``/search/`` mirrors + # GitHub under /api/v1; ``/issues/``, ``/releases/``, and ``/git/`` at + # the root have no Forgejo equivalent and so return None so the call + # fails closed with a clear error rather than silently routing to the + # wrong URL. This is the Forgejo side of the parity the test suite + # asserts in ``TestGhApiRootEndpoints``. + if not repo_key: if full_path.startswith("/search/"): return f"/api/v1{full_path}" - if full_path.startswith("/releases/"): - # /releases/owner/repo/tags → /api/v1/repos/owner/repo/releases/tags - tail = full_path[len("/releases/"):] - if "/" in tail: - owner, repo = tail.split("/", 1) - return f"/api/v1/repos/{owner}/{repo}/releases/tags" + return None + + repos = f"/repos/{repo_key}" + if not full_path.startswith(repos): return None rest = full_path[len(repos):] # begins with "/" diff --git a/tests/test_gh_api.py b/tests/test_gh_api.py index ceb7f0c1..eea85936 100644 --- a/tests/test_gh_api.py +++ b/tests/test_gh_api.py @@ -1,5 +1,13 @@ #!/usr/bin/env python3 -"""Tests for the gh_api tool in run_tool_harness.py.""" +"""Tests for the gh_api tool and the underlying _validate_endpoint helper. + +The validator is the security boundary for both backends (GitHub and +Forgejo); if it accepts a path the caller issues a real network request. +These tests therefore exercise ``_validate_endpoint`` directly so they +do not need to mock the HTTP layer, and so a regression in the +validator (e.g. the URL-mangling bug fixed in #469) fails the test +without depending on a live GitHub response. +""" import os import sys @@ -10,79 +18,112 @@ if str(_SCRIPTS_DIR) not in sys.path: sys.path.insert(0, str(_SCRIPTS_DIR)) -from run_tool_harness import gh_api # noqa: E402 +from pr_reviewer.platform import ( # noqa: E402 + _validate_endpoint, +) -class TestGhApiRepoParsing: - """Test that gh_api correctly parses repo keys from various endpoint formats.""" +def _setup_token(): + os.environ["GH_TOKEN"] = "test-token" - def _setup_env(self): - os.environ["GH_TOKEN"] = "test-token" + +class TestGhApiRepoParsing: + """Test that _validate_endpoint correctly parses repo keys from various endpoint formats.""" def test_repos_prefix_current_repo(self): """Endpoint with 'repos/' prefix matching current repo should be allowed.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", ) - assert result.get("error") is None or "Repo not allowed" not in (result.get("error") or ""), ( + assert "error" not in result, ( f"Current repo with repos/ prefix should be allowed: {result}" ) + assert result["full_path"] == "/repos/misospace/pr-reviewer-action/pulls/1", ( + f"Unexpected full_path: {result}" + ) + assert result["repo_key"] == "misospace/pr-reviewer-action", ( + f"Unexpected repo_key: {result}" + ) def test_direct_path_current_repo(self): """Direct owner/repo path matching current repo should be allowed.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "misospace/pr-reviewer-action/pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", ) - assert result.get("error") is None or "Repo not allowed" not in (result.get("error") or ""), ( + assert "error" not in result, ( f"Current repo with direct path should be allowed: {result}" ) + assert result["full_path"] == "/repos/misospace/pr-reviewer-action/pulls/1", ( + f"Unexpected full_path: {result}" + ) + assert result["repo_key"] == "misospace/pr-reviewer-action", ( + f"Unexpected repo_key: {result}" + ) def test_repos_prefix_explicit_allowed_repo(self): """Endpoint with 'repos/' prefix for an explicitly allowed repo should pass allowlist.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/other-org/other-repo/issues", allowed_repos={"other-org/other-repo"}, current_repo="misospace/pr-reviewer-action", ) - assert result.get("error") is None or "Repo not allowed" not in (result.get("error") or ""), ( + assert "error" not in result, ( f"Explicitly allowed repo with repos/ prefix should be allowed: {result}" ) + assert result["full_path"] == "/repos/other-org/other-repo/issues", ( + f"Unexpected full_path: {result}" + ) + assert result["repo_key"] == "other-org/other-repo", ( + f"Unexpected repo_key: {result}" + ) def test_direct_path_explicit_allowed_repo(self): """Direct path for an explicitly allowed repo should pass allowlist.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "other-org/other-repo/issues", allowed_repos={"other-org/other-repo"}, current_repo="misospace/pr-reviewer-action", ) - assert result.get("error") is None or "Repo not allowed" not in (result.get("error") or ""), ( + assert "error" not in result, ( f"Explicitly allowed repo with direct path should be allowed: {result}" ) + assert result["full_path"] == "/repos/other-org/other-repo/issues", ( + f"Unexpected full_path: {result}" + ) + assert result["repo_key"] == "other-org/other-repo", ( + f"Unexpected repo_key: {result}" + ) def test_wildcard_allows_any_repo(self): """Wildcard '*' in allowed_repos should permit any repo.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/any-org/any-repo/pulls", allowed_repos={"*"}, current_repo="misospace/pr-reviewer-action", ) - assert result.get("error") is None or "Repo not allowed" not in (result.get("error") or ""), ( + assert "error" not in result, ( f"Wildcard should allow any repo: {result}" ) + assert result["full_path"] == "/repos/any-org/any-repo/pulls", ( + f"Unexpected full_path: {result}" + ) + assert result["repo_key"] == "any-org/any-repo", ( + f"Unexpected repo_key: {result}" + ) def test_denied_repo_rejected(self): """Repos not in current_repo, not in allowed_repos, and no wildcard should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/unknown-org/unknown-repo/issues", allowed_repos={"other-org/other-repo"}, current_repo="misospace/pr-reviewer-action", @@ -93,8 +134,8 @@ def test_denied_repo_rejected(self): def test_denied_secrets_path_blocked(self): """Paths containing '/actions/secrets' should be denied regardless of repo.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/actions/secrets", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -105,8 +146,8 @@ def test_denied_secrets_path_blocked(self): def test_denied_environments_path_blocked(self): """Paths containing '/environments/' should be denied regardless of repo.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/environments/prod", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -117,8 +158,8 @@ def test_denied_environments_path_blocked(self): def test_denied_dispatches_path_blocked(self): """Paths containing '/dispatches' should be denied regardless of repo.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/actions/dispatches", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -127,30 +168,10 @@ def test_denied_dispatches_path_blocked(self): f"Dispatches path should be denied: {result}" ) - def test_no_token_returns_error(self): - """Missing GH_TOKEN and GITHUB_TOKEN should return an error.""" - saved = {} - for var in ("GH_TOKEN", "GITHUB_TOKEN"): - if var in os.environ: - saved[var] = os.environ[var] - del os.environ[var] - try: - result = gh_api( - "misospace/pr-reviewer-action/pulls/1", - allowed_repos=set(), - current_repo="misospace/pr-reviewer-action", - ) - assert "Missing GH_TOKEN" in (result.get("error") or ""), ( - f"Missing token should return error: {result}" - ) - finally: - for var, val in saved.items(): - os.environ[var] = val - def test_short_endpoint_returns_error(self): - """Endpoints with fewer than 2 path segments should return an error.""" - self._setup_env() - result = gh_api( + """Repo-scoped endpoints with fewer than 2 path segments should return an error.""" + _setup_token() + result = _validate_endpoint( "only-one-segment", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -161,15 +182,12 @@ def test_short_endpoint_returns_error(self): class TestGhApiPathValidation: - """Test that gh_api enforces character, dot-segment, and prefix restrictions.""" - - def _setup_env(self): - os.environ["GH_TOKEN"] = "test-token" + """Test that _validate_endpoint enforces character, dot-segment, and prefix restrictions.""" def test_disallowed_characters_rejected(self): """Endpoints with spaces or special chars should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/pulls/1 comment", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -180,8 +198,8 @@ def test_disallowed_characters_rejected(self): def test_null_byte_rejected(self): """Endpoints with null bytes should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/pulls/1\x00", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -192,8 +210,8 @@ def test_null_byte_rejected(self): def test_parent_directory_traversal_rejected(self): """Endpoints containing '..' segment should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/../pr-reviewer-action/pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -204,8 +222,8 @@ def test_parent_directory_traversal_rejected(self): def test_current_directory_segment_rejected(self): """Endpoints containing '.' segment should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/./misospace/pr-reviewer-action/pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -221,8 +239,8 @@ def test_dot_in_path_component_allowed(self): releases/tags/v1.2.3 may still fail on the network call (test token), but it must not be rejected for containing dots. """ - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/releases/tags/v1.2.3", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -233,8 +251,8 @@ def test_dot_in_path_component_allowed(self): def test_empty_segment_rejected(self): """Endpoints producing an empty path segment ('//') should be rejected.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace//pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", @@ -244,9 +262,9 @@ def test_empty_segment_rejected(self): ) def test_unallowed_prefix_rejected(self): - """Endpoints not starting with an allowed prefix should be rejected.""" - self._setup_env() - result = gh_api( + """Repo-scoped endpoints not starting with an allowed prefix should be rejected.""" + _setup_token() + result = _validate_endpoint( "user/misospace/emails", allowed_repos={"misospace/pr-reviewer-action"}, current_repo="misospace/pr-reviewer-action", @@ -255,58 +273,205 @@ def test_unallowed_prefix_rejected(self): f"Unallowed prefix should be rejected: {result}" ) - def test_allowed_repos_prefix_passes(self): + def test_repos_prefix_passes(self): """Endpoints starting with /repos/ should pass prefix check.""" - self._setup_env() - result = gh_api( + _setup_token() + result = _validate_endpoint( "repos/misospace/pr-reviewer-action/pulls/1", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", ) - assert "prefix not allowed" not in (result.get("error") or "").lower(), ( + assert "error" not in result, ( f"/repos/ prefix should be allowed: {result}" ) + assert result["full_path"] == "/repos/misospace/pr-reviewer-action/pulls/1", ( + f"Unexpected full_path: {result}" + ) - def test_allowed_issues_prefix_passes(self): - """Endpoints starting with /issues/ should pass prefix check.""" - self._setup_env() - result = gh_api( - "issues/misospace/pr-reviewer-action/comments", - allowed_repos={"misospace/pr-reviewer-action"}, - current_repo="other/repo", + +class TestGhApiRootEndpoints: + """Root-level endpoints (no /repos/owner/repo/ segment) — issue #469. + + These are /search/, /issues/, /releases/, /git/ at the API host root. + They are NOT repo-scoped, so the repo-key allowlist must not reject + them and the URL must not be mangled into /repos/search/... etc. + """ + + def test_search_under_wildcard(self): + """Wildcard '*' must NOT mangle /search/ into /repos/search/ (issue #469). + + Pre-fix behaviour: ``search/code?q=foo`` with ``{"*"}`` produced + ``full_path='/repos/search/code?q=foo'`` (404 against the GitHub + API). Post-fix: it routes to ``https://api.github.com/search/code?q=foo``. + """ + _setup_token() + result = _validate_endpoint( + "search/code?q=foo", + allowed_repos={"*"}, + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, ( + f"/search/ should validate under wildcard: {result}" ) - assert "prefix not allowed" not in (result.get("error") or "").lower(), ( - f"/issues/ prefix should be allowed: {result}" + assert result["full_path"] == "/search/code?q=foo", ( + f"Wildcard must not prepend /repos/: {result}" ) + assert result["repo_key"] == "", ( + f"Root endpoint repo_key must be empty: {result}" + ) + + def test_search_under_empty_allowlist(self): + """An empty allowlist (current repo only) must still let /search/ through. - def test_allowed_search_prefix_passes(self): - """Endpoints starting with /search/ should pass prefix check.""" - self._setup_env() - result = gh_api( + Pre-fix behaviour: ``search/code?q=foo`` failed with + "Repo not allowed: search/code?q=foo" because the repo-key check + ran first. Post-fix: the root-prefix check runs first, the repo + allowlist is bypassed, and the call is allowed. + """ + _setup_token() + result = _validate_endpoint( "search/code?q=foo", + allowed_repos=set(), + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, ( + f"/search/ should validate without explicit repo allowlist: {result}" + ) + assert result["full_path"] == "/search/code?q=foo", ( + f"Empty allowlist must not affect root endpoint: {result}" + ) + assert result["repo_key"] == "", ( + f"Root endpoint repo_key must be empty: {result}" + ) + + def test_search_with_leading_slash(self): + """A leading slash on the endpoint is normalised and must work the same.""" + _setup_token() + result = _validate_endpoint( + "/search/code?q=foo", + allowed_repos=set(), + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, ( + f"/search/ with leading slash should validate: {result}" + ) + assert result["full_path"] == "/search/code?q=foo", ( + f"Leading slash must not mangle /search/: {result}" + ) + + def test_search_with_subpath(self): + """``search/code?q=foo`` and ``search/issues?q=bar`` share the root + prefix but route to different resources — both must validate + cleanly with the right ``full_path`` so the GitHub backend hits + the right endpoint, not a mangled /repos/search/issues/... 404.""" + _setup_token() + result = _validate_endpoint( + "search/issues?q=bar", + allowed_repos={"*"}, + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, result + assert result["full_path"] == "/search/issues?q=bar", result + assert result["repo_key"] == "", result + + def test_git_refs_under_wildcard(self): + """``/git/refs/...`` is a root-level endpoint and must reach the API + unchanged under wildcard. The Forgejo translator rejects it (no + Forgejo equivalent at the root); the GitHub backend must hit + ``https://api.github.com/git/refs/...``. + """ + _setup_token() + result = _validate_endpoint( + "git/refs/heads/main", + allowed_repos={"*"}, + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, ( + f"/git/ under wildcard should pass: {result}" + ) + assert result["full_path"] == "/git/refs/heads/main", ( + f"Wildcard must not prepend /repos/ to /git/ (issue #469): {result}" + ) + assert result["repo_key"] == "", ( + f"Root endpoint repo_key must be empty (issue #469): {result}" + ) + + def test_git_refs_without_wildcard_also_passes(self): + """Root-level endpoints must be allowed even when ``allowed_repos`` + contains a specific repo (no wildcard). The repo allowlist is the + auth gate; once the user is permitted to call ``gh_api`` at all, + root endpoints should be reachable. + """ + _setup_token() + result = _validate_endpoint( + "git/refs/heads/main", allowed_repos={"misospace/pr-reviewer-action"}, current_repo="misospace/pr-reviewer-action", ) - # Note: search endpoints don't have a repo key, so this will fail on - # repo allowlist check, but should pass the prefix check - err = result.get("error") or "" - assert "prefix not allowed" not in err.lower(), ( - f"/search/ prefix should be allowed: {result}" + assert "error" not in result, ( + f"/git/ with explicit repo in allowlist should pass: {result}" + ) + assert result["full_path"] == "/git/refs/heads/main", ( + f"/git/ full_path must not be mangled: {result}" ) - def test_allowed_releases_prefix_passes(self): - """Endpoints starting with /releases/ should pass prefix check.""" - self._setup_env() - result = gh_api( - "releases/misospace/pr-reviewer-action/tags", + def test_issues_root_endpoint_passes(self): + """``/issues`` (root, listing all org-wide issues) must reach the API + without the /repos/ prefix being prepended. + """ + _setup_token() + result = _validate_endpoint( + "/issues", + allowed_repos=set(), + current_repo="misospace/pr-reviewer-action", + ) + assert "error" not in result, ( + f"/issues root endpoint should pass: {result}" + ) + assert result["full_path"] == "/issues", ( + f"/issues full_path must not be mangled: {result}" + ) + assert result["repo_key"] == "", ( + f"Root endpoint repo_key must be empty: {result}" + ) + + def test_releases_root_endpoint_passes(self): + """``/releases`` (root, listing all org-wide releases) must reach the + API without the /repos/ prefix being prepended. + """ + _setup_token() + result = _validate_endpoint( + "/releases", allowed_repos=set(), current_repo="misospace/pr-reviewer-action", ) - assert "prefix not allowed" not in (result.get("error") or "").lower(), ( - f"/releases/ prefix should be allowed: {result}" + assert "error" not in result, ( + f"/releases root endpoint should pass: {result}" + ) + assert result["full_path"] == "/releases", ( + f"/releases full_path must not be mangled: {result}" + ) + assert result["repo_key"] == "", ( + f"Root endpoint repo_key must be empty: {result}" + ) + + def test_root_endpoint_with_dot_segment_still_rejected(self): + """Root endpoints inherit the dot-segment / unsafe-character guards. + A path like ``/search/./code`` must be rejected just like + ``repos/./owner/repo`` is. This guards against the root-prefix + bypass accidentally relaxing other rules. + """ + _setup_token() + result = _validate_endpoint( + "/search/./code", + allowed_repos=set(), + current_repo="misospace/pr-reviewer-action", + ) + assert "dot" in (result.get("error") or "").lower(), ( + f"Dot-segment in root endpoint should still be rejected: {result}" ) if __name__ == "__main__": import pytest - pytest.main([__file__, "-v"]) + pytest.main([__file__, "-v"]) \ No newline at end of file diff --git a/tests/test_platform_gh_api_forgejo.py b/tests/test_platform_gh_api_forgejo.py index 3936ed30..95616885 100644 --- a/tests/test_platform_gh_api_forgejo.py +++ b/tests/test_platform_gh_api_forgejo.py @@ -244,22 +244,78 @@ def test_forgejo_curl_sends_user_agent(monkeypatch): assert any("User-Agent: ai-pr-reviewer/1.0" in tok for tok in cmd), cmd -def test_forgejo_search_prefix_check_passes(monkeypatch): - """``search/...`` (no repo key) passes the prefix check on both backends. - - Search endpoints do not have a repo key, so the repo-allowlist check - rejects them (the same way the pre-seam code did — see - ``tests/test_gh_api.py::TestGhApiPathValidation::test_allowed_search_prefix_passes``). - The Forgejo backend must not weaken that: the error must be about the - repo allowlist, not a translation failure. +def test_forgejo_root_search_routes_to_api_v1(monkeypatch): + """``/search/code?q=foo`` (with leading slash) is a root-level endpoint. + + Issue #469: under the old validator this either failed with "Repo not + allowed: search/code?q=foo" (because the repo-key check ran first and + computed ``search/code?q=foo`` as the repo key) or — under ``*`` — + mangled into ``/repos/search/code?q=foo``. The fix routes it to + ``/api/v1/search/code?q=foo`` on the Forgejo backend without ever + touching the /repos/ prefix. """ + result, captured = _exec_forgejo(monkeypatch, "/search/code?q=foo") + assert "error" not in result, result + cmd = captured["cmd"] + assert any( + _FORGEJO_BASE + "/api/v1/search/code?q=foo" in tok for tok in cmd + ), cmd + assert not any("api.github.com" in tok for tok in cmd), cmd + # And the URL must NOT have been mangled to /repos/search/... + assert not any("/repos/search/" in tok for tok in cmd), cmd + + +def test_forgejo_root_search_without_leading_slash_routes_to_api_v1(monkeypatch): + """``search/code?q=foo`` (no leading slash) is normalised by the + validator and routes the same as ``/search/code?q=foo`` — the + leading slash is purely cosmetic, the validator strips it before + matching against ``GH_API_ROOT_PREFIXES``. This is the documented + shape the GitHub/Forgejo tool descriptors give the model. + """ + result, captured = _exec_forgejo(monkeypatch, "search/code?q=foo") + assert "error" not in result, result + cmd = captured["cmd"] + assert any( + _FORGEJO_BASE + "/api/v1/search/code?q=foo" in tok for tok in cmd + ), cmd + assert not any("/repos/search/" in tok for tok in cmd), cmd + + +def test_forgejo_root_git_endpoint_rejected(monkeypatch): + """``/git/refs/...`` has no Forgejo equivalent at the root and must + fail closed with a clear ``Endpoint not supported`` rather than silently + being routed to the wrong URL. + """ + monkeypatch.setenv("PLATFORM", "forgejo") + monkeypatch.setenv("FORGEJO_API_URL", _FORGEJO_BASE) + monkeypatch.setenv("FORGEJO_TOKEN", "fj-test") + with patch("pr_reviewer.forgejo_backend.subprocess.run") as mock_run: + result = gh_api( + "/git/refs/heads/main", allowed_repos=set(), current_repo=_REPO + ) + assert "not supported" in result.get("error", "").lower(), result + mock_run.assert_not_called() + + +def test_forgejo_root_releases_endpoint_rejected(monkeypatch): + """``/releases`` at the root has no Forgejo equivalent and must fail closed.""" + monkeypatch.setenv("PLATFORM", "forgejo") + monkeypatch.setenv("FORGEJO_API_URL", _FORGEJO_BASE) + monkeypatch.setenv("FORGEJO_TOKEN", "fj-test") + with patch("pr_reviewer.forgejo_backend.subprocess.run") as mock_run: + result = gh_api("/releases", allowed_repos=set(), current_repo=_REPO) + assert "not supported" in result.get("error", "").lower(), result + mock_run.assert_not_called() + + +def test_forgejo_root_issues_endpoint_rejected(monkeypatch): + """``/issues`` at the root has no Forgejo equivalent and must fail closed.""" monkeypatch.setenv("PLATFORM", "forgejo") monkeypatch.setenv("FORGEJO_API_URL", _FORGEJO_BASE) monkeypatch.setenv("FORGEJO_TOKEN", "fj-test") with patch("pr_reviewer.forgejo_backend.subprocess.run") as mock_run: - result = gh_api("search/code?q=foo", allowed_repos=set(), current_repo=_REPO) - # Fails on the repo allowlist — the prefix check still passed. - assert "not allowed" in result.get("error", "").lower(), result + result = gh_api("/issues", allowed_repos=set(), current_repo=_REPO) + assert "not supported" in result.get("error", "").lower(), result mock_run.assert_not_called()