Fix root-level GH API endpoints (search/issues/releases/git) from repo-key check and /repos/ mangling - #489
Conversation
d10401e to
932eea6
Compare
932eea6 to
aa816b3
Compare
…api validator (Fixes #469) The validator used to compute repo_key = parts[0]/parts[1] BEFORE the prefix allowlist check, so: - /search/code?q=foo with an empty allowlist failed with "Repo not allowed: search/code?q=foo" - /search/code?q=foo under "*" passed but produced full_path = /repos/search/code?q=foo (URL-mangled, 404) The same bug made the Forgejo translator unreachable for /search/ and /releases/ at the root, and the root prefixes in GH_API_ALLOWED_PREFIXES were effectively dead code. Split GH_API_ALLOWED_PREFIXES into: - GH_API_ALLOWED_PREFIXES = ("/repos/",) # repo-scoped - GH_API_ROOT_PREFIXES = ("/issues/", "/search/", "/releases/", "/git/") # API-root In _validate_endpoint, detect root-level endpoints by first path segment BEFORE the repo-key check, bypass the allowlist, and build the URL without prepending /repos/ (issue #469). Deny substrings still apply to root paths; repo_key is returned empty so the Forgejo translator can dispatch correctly. In _forgejo_translate, when repo_key is empty: - /search/ → /api/v1/search/... (parity with GitHub) - /issues/, /releases/, /git/ → None (no Forgejo equivalent, fail closed with a clear "Endpoint not supported" error) Replaced the prior weak `assert "prefix not allowed" not in err.lower()` guards with explicit `assert "error" not in result` plus exact `result["full_path"] == ...` and `result["repo_key"] == ...` assertions (no `if 'full_path' in result:` pattern) — the AI PR review's specific recommendation. Added TestGhApiRootEndpoints covering /search/, /git/, /issues/, /releases/ under wildcard, empty allowlist, and explicit allowlist; added forgejo parity tests asserting the exact URL issued for /search/ and that /git/, /issues/, /releases/ at the root fail closed. Fixes #469 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
aa816b3 to
cb1a312
Compare
Reason: Review comment on tests/test_gh_api.py:356 Latest note: foreman fix Workload prfix-misospace-pr-reviewer-action-489 succeeded but PR is still not mergeable after 3/3 attempts Posted automatically by Dispatch on 2026-08-12T03:30:04.423Z |
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — routed smart (risk match: public_route_changes)
Recommendation
Approve. This PR correctly fixes issue PR 469: root-level GitHub API endpoints (/search/, /issues/, /releases/, /git/) were unreachable and URL-mangled. The fix is minimal, well-tested, and maintains Forgejo parity.
Change-by-Change Findings
pr_reviewer/platform.py
_validate_endpoint rewrite (core fix): The function now detects root-level endpoints via GH_API_ROOT_PREFIXES before the repo-key check. This means /search/, /issues/, /releases/, /git/ bypass the repo allowlist (they aren't tied to any repo) and return a full_path without the /repos/ prefix being prepended. Previously, a wildcard allowlist would still mangle /search/code?q=foo into /repos/search/code?q=foo (a 404). The fix correctly routes to /search/code?q=foo for GitHub and /api/v1/search/code?q=foo for Forgejo.
_forgejo_translate update: With an empty repo_key, the translator now handles root endpoints deterministically: /search/ is mapped to /api/v1, while /issues/, /releases/, and /git/ return None (failing closed with "Endpoint not supported on PLATFORM=forgejo"), rather than silently routing to the wrong URL. This is the correct Forgejo behavior for endpoints that have no equivalent on that platform.
tests/test_gh_api.py
Test refactor from gh_api → _validate_endpoint: Tests now exercise the validator directly, removing the HTTP-layer dependency. All existing repo-scoped tests (wildcard, allowlist, deny substrings) are preserved with tighter assertions on full_path and repo_key.
New TestGhApiRootEndpoints class: 12 new test cases cover the full surface area of root-level endpoint handling, including the pre-fix mangling behavior, empty allowlist behavior, subpaths, dot-segment rejection in root context, and the repo_key == "" invariant.
tests/test_platform_gh_api_forgejo.py
Forgejo parity tests: 5 new tests confirm that /search/ routes to /api/v1 on Forgejo and that /git/, /releases/, and /issues/ fail closed rather than silently routing to the wrong endpoint.
Required Checks
✅ verify route access controls are in place
Root-level endpoints (/search/, /issues/, /releases/, /git/) are gated by GH_API_ROOT_PREFIXES. They bypass the repo-key allowlist because they are not repo-scoped — a user permitted to call gh_api at all can reach them. Repo-scoped endpoints (/repos/owner/repo/...) still require the repo to be in the allowlist. The GH_DENY_SUBSTRINGS check (secrets, dispatches, environments) still runs for root endpoints, ensuring security boundaries are not bypassed.
✅ check for unintended public endpoints
No new public endpoints are introduced. Root-level endpoints were always documented in GH_API_ALLOWED_PREFIXES; the bug was that they were unreachable. The fix makes them correctly reachable with the right URL shape. No additional URL patterns are opened.
✅ review for path traversal vulnerabilities
The dot-segment check (if part in ("", ".", "..")) runs for all paths including root-level ones. The existing test test_root_endpoint_with_dot_segment_still_rejected explicitly verifies that /search/./code is rejected. The GH_SAFE_PATH_RE regex also guards against null bytes and other control characters before any splitting occurs. The fix does not weaken any existing path-traversal guards.
✅ test with edge-case paths (null bytes, symlinks)
test_null_byte_rejected verifies that a null byte in the endpoint is caught by GH_SAFE_PATH_RE before splitting. Symlinks are not relevant here — _validate_endpoint operates on URL path strings, not filesystem paths, so there is no symlink resolution surface. The existing dot-segment tests (.. and .) cover the traversal-relevant cases.
Linked Issue Fit
Issue PR 469 acceptance criteria are fully satisfied:
- (a) Root-level prefixes validate correctly —
_validate_endpointnow detects root-level paths viaGH_API_ROOT_PREFIXESand builds the correct URL without prepending/repos/. E.g.,/search/code?q=foo→full_path='/search/code?q=foo'(GitHub) or/api/v1/search/code?q=foo(Forgejo). ✅ - (b)
/issues/,/releases/,/git/at the root — These pass validation but fail closed on Forgejo (return None→ "Endpoint not supported"), which is the correct parity behavior. ✅ - Unit tests cover both non-wildcard and wildcard cases for
/search/and/git/—test_search_under_wildcard,test_search_under_empty_allowlist,test_git_refs_under_wildcard, andtest_git_refs_without_wildcard_also_passesassert exact URLs. ✅
Standards Compliance
This PR follows the repository's established patterns:
- Test structure (
tests/test_gh_api.py): Tests the validator in isolation without HTTP mocking, matching the "security boundary" testing philosophy described in the module docstring. - Forgejo parity: The test suite enforces both backends receive the same validator behavior, with Forgejo-specific translation tests in
test_platform_gh_api_forgejo.py. - No new dependencies or behavioral surface added beyond the fix.
- CI validate check passed (commit
cb1a312).
Unknowns / Needs Verification
- Linked sources from the PR body (GitHub API URL fixtures) failed to fetch, but this is not required for review: the test suite exercises the exact URL shapes and the
validateCI check passed. - No evidence provider output or tool harness findings were produced (no tool calls were needed).
No findings.
The branch fails to address the issue of supporting Forgejo GitHub API endpoints, instead only implementing GitHub support. Tests for Forgejo functionality are missing or failing.
Fixes #469
Opened by foreman on review GO (workload wl-misospace-pr-reviewer-action-469).