From 889c780ba5c2e9fdc96b86e554489edd7fe2c991 Mon Sep 17 00:00:00 2001 From: davidjiang8888 Date: Wed, 12 Aug 2026 00:04:24 -0400 Subject: [PATCH 1/5] docs: design GitHub pagination boundary --- .../2026-08-12-github-pagination-boundary.md | 380 ++++++++++++++++++ ...08-12-github-pagination-boundary-design.md | 136 +++++++ 2 files changed, 516 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-12-github-pagination-boundary.md create mode 100644 docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md diff --git a/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md b/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md new file mode 100644 index 0000000..333eef4 --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md @@ -0,0 +1,380 @@ +# GitHub Pagination and Credential Boundary Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development +> (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use +> checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make public GitHub pull-request pagination same-origin, lineage-bound, cycle-safe, and +deterministically bounded without forwarding optional credentials or permitting partial evidence to +become Ready. + +**Architecture:** `GitHubClient` owns one per-fetch request/response budget and one strict +paginated-list helper. Every `next` link is validated before use and accepted links are reissued as +relative same-origin requests. File and commit collections return bounded truncation metadata; +malformed or exhausted traversals raise a user-safe ingestion error before adapters mutate state. + +**Tech Stack:** Python 3.11+, httpx, Pydantic 2, pytest, Streamlit AppTest, uv, Ruff. + +## Global Constraints + +- ScopeProof is an evidence assistant, not a correctness oracle. +- Never execute target-repository code. +- Keep the core independent of Streamlit and GitHub UI layers. +- Persisted and exported objects remain Pydantic-validated. +- Failed ingestion does not mutate saved reviews or create exports. +- Gate behavior remains deterministic and fail closed; False Ready is more harmful than False + Blocked. +- Preserve anonymous public GitHub ingestion and session-only optional credentials. +- Use ceilings of 16 requests, 10 pagination pages, 1,000 items, 4 MiB per decoded response, + 16 MiB total decoded responses, 100 files by default, and 250 commits by default. +- Product Stage 1 stays exactly zero. Do not begin Phases 2–4, release, publish, or perform outreach. + +--- + +### Task 1: Strict traversal and shared budgets + +**Files:** +- Modify: `scopeproof_core/github/client.py` +- Modify: `scopeproof_core/github/__init__.py` +- Test: `tests/github/test_pagination_and_candidates.py` +- Test: `tests/github/test_client.py` + +**Interfaces:** +- Produces: `GitHubPaginationError(GitHubIngestionError)`. +- Produces: private `_FetchBudget.charge_request()`, `_FetchBudget.charge_response(response)`, + `_FetchBudget.charge_page()`, and `_FetchBudget.charge_items(count)` methods. +- Produces: private `_PaginatedResult(items: list[dict], truncated: bool)`. +- Produces: `_get_paginated(path, *, expected_path, per_page, retain_limit, budget)`. +- Consumes: existing `_get`, `_raise_for_pr`, and `httpx.Response` behavior. + +- [ ] **Step 1: Write failing origin, lineage, and cycle tests** + +Add a controlled transport that returns one valid first file page and a configurable `Link` header. +Capture all requests and assert, for example: + +```python +with pytest.raises(GitHubPaginationError, match="expected GitHub API origin"): + GitHubClient(token="session-secret", transport=transport).fetch_pull_request(PR_URL) + +assert [request.url.host for request in requests] == ["api.github.com", "api.github.com"] +assert requests[-1].headers["authorization"] == "Bearer session-secret" +assert all(request.url.host != "attacker.invalid" for request in requests) +``` + +Parameterize `http://api.github.com/...`, `https://attacker.invalid/...`, another repository, +another PR, and `/commits` substituted for `/files`. Add repeated canonical URL, page-one/page-two +cycle, duplicate `page`, unknown query, missing `per_page`, and two `rel="next"` cases. + +- [ ] **Step 2: Write failing budget tests** + +Construct multi-page or padded responses and lower one ceiling per test: + +```python +with pytest.raises(GitHubPaginationError, match="request budget"): + GitHubClient(transport=transport, max_requests=2).fetch_pull_request(PR_URL) + +with pytest.raises(GitHubPaginationError, match="decoded response byte budget"): + GitHubClient(transport=transport, max_response_bytes=64).fetch_pull_request(PR_URL) +``` + +Cover request, page, item, single decoded response, and cumulative decoded responses independently. + +- [ ] **Step 3: Run the focused tests and confirm intended red failures** + +Run: + +```bash +uv run pytest -q tests/github/test_pagination_and_candidates.py tests/github/test_client.py \ + -k 'pagination or budget or decoded or origin or downgrade or cycle or escape' +``` + +Expected: failures show `_get_all` follows unsafe links or lacks the new constructor ceilings and +error type. + +- [ ] **Step 4: Implement the budget and strict target validator** + +Add these private shapes in `client.py`: + +```python +@dataclass +class _FetchBudget: + requests_remaining: int + pages_remaining: int + items_remaining: int + max_response_bytes: int + total_response_bytes_remaining: int + + +@dataclass(frozen=True) +class _PaginatedResult: + items: list[dict] + truncated: bool = False +``` + +Extend `GitHubClient.__init__` with `max_commits=250`, `max_requests=16`, +`max_pagination_pages=10`, `max_pagination_items=1_000`, `max_response_bytes=4 * 1024 * 1024`, and +`max_total_response_bytes=16 * 1024 * 1024`. Reject non-positive limits with `ValueError`. + +Implement a target validator with `urlsplit` and `parse_qsl(keep_blank_values=True)`. Require HTTPS, +hostname `api.github.com`, no user information or fragment, port absent or 443, exact collection +path, and exactly `[('page', positive_integer), ('per_page', expected_size)]` after deterministic +sorting. Return a relative canonical target such as: + +```python +return f"{expected_path}?page={page}&per_page={per_page}" +``` + +Count `rel="next"` occurrences from all Link header values and require zero or one. Track the +initial and every accepted canonical target in a local `visited` set before requesting it. + +Modify `_get` to take an optional budget, charge a request before transport, and charge +`len(response.content)` before return. Export `GitHubPaginationError` from `github/__init__.py`. + +- [ ] **Step 5: Run focused tests until green** + +Run: + +```bash +uv run pytest -q tests/github/test_pagination_and_candidates.py tests/github/test_client.py +uv run ruff check scopeproof_core/github tests/github/test_pagination_and_candidates.py \ + tests/github/test_client.py +``` + +Expected: all selected tests pass with no lint output. + +- [ ] **Step 6: Commit the core boundary intentionally** + +```bash +git add scopeproof_core/github/client.py scopeproof_core/github/__init__.py \ + tests/github/test_pagination_and_candidates.py tests/github/test_client.py +git commit -m "fix: constrain GitHub pagination traversal" +``` + +### Task 2: Bounded files, commits, and fail-closed evidence + +**Files:** +- Modify: `scopeproof_core/github/client.py` +- Modify: `tests/github/test_pagination_and_candidates.py` +- Modify: `tests/github/test_client.py` +- Modify: `tests/gates/test_evaluator.py` + +**Interfaces:** +- Consumes: Task 1 `_FetchBudget` and `_get_paginated`. +- Produces: file results retained in GitHub order at `max_files` with at most one observed overflow. +- Produces: commit results retained in GitHub order at `max_commits` with at most one observed + overflow. +- Preserves: `PullRequestSnapshot.ingestion_state`, `warnings`, and `skipped_files` Pydantic rules. + +- [ ] **Step 1: Write failing early-file-stop and commit-bound tests** + +Make the transport record collection URLs. For files, use `max_files=1` and return two entries on +the first page plus a `next` link that would fail if requested: + +```python +snapshot = GitHubClient(transport=transport, max_files=1).fetch_pull_request(PR_URL) +assert [item.path for item in snapshot.files] == ["src/000.py"] +assert not any(url.params.get("page") == "2" for url in file_requests) +assert snapshot.ingestion_state is IngestionState.PARTIAL +``` + +For commits, set `max_commits=2`, return three ordered entries, and assert the first two are retained, +the next page is not requested, and a commit-history warning is present. Add a two-page ordering test +whose retained SHA list exactly matches API order. + +- [ ] **Step 2: Run the bounded-collection tests and confirm intended red failures** + +Run: + +```bash +uv run pytest -q tests/github/test_pagination_and_candidates.py tests/github/test_client.py \ + -k 'early or file_limit or commit_limit or deterministic_order' +``` + +Expected: current code requests extra pages or lacks `max_commits` and commit partial-state behavior. + +- [ ] **Step 3: Implement smallest-overflow collection behavior** + +Create one `_FetchBudget` at the start of `fetch_pull_request` and pass it to every `_get`. Request +files with `per_page=min(100, max_files + 1)` and commits with +`per_page=min(100, max_commits + 1)`. Invoke: + +```python +file_result = self._get_paginated( + f"{root}/pulls/{pr_number}/files", + expected_path=f"{root}/pulls/{pr_number}/files", + per_page=file_page_size, + retain_limit=self.max_files, + budget=budget, +) +``` + +The helper stops when it observes `retain_limit + 1` items or when it has retained exactly the limit +and a valid `next` proves more exist. It never follows that unnecessary link. For a truncated file +collection, keep only observed overflow filenames in `skipped_files` and use wording that does not +invent the number or names of unretrieved files. Apply the same bounded result to commits and mark +the snapshot partial with a commit-history warning. + +- [ ] **Step 4: Prove client-produced partial evidence cannot be Ready** + +Add a gate regression using a `Review` populated from the truncated snapshot: + +```python +review.ingestion_state = snapshot.ingestion_state +review.ingestion_warnings = snapshot.warnings +review.skipped_files = snapshot.skipped_files +decision = evaluate_gate(review, [criterion], [finding], [accepted_resolution]) +assert decision.verdict is GateVerdict.NEEDS_REVIEW +assert "partial_ingestion" in decision.reason_codes +``` + +- [ ] **Step 5: Run focused tests and commit intentionally** + +```bash +uv run pytest -q tests/github/test_pagination_and_candidates.py tests/github/test_client.py \ + tests/gates/test_evaluator.py +uv run ruff check scopeproof_core/github tests/github tests/gates/test_evaluator.py +git add scopeproof_core/github/client.py tests/github/test_pagination_and_candidates.py \ + tests/github/test_client.py tests/gates/test_evaluator.py +git commit -m "fix: bound GitHub file and commit evidence" +``` + +### Task 3: CLI and Streamlit mutation safety + +**Files:** +- Modify: `tests/cli/test_cli.py` +- Modify: `tests/apps/test_streamlit_app.py` + +**Interfaces:** +- Consumes: public `GitHubPaginationError` from Task 1. +- Preserves: CLI fetch-before-save and Streamlit assign-after-fetch behavior. +- Produces: explicit non-mutation regressions for stored records, reports, and session state. + +- [ ] **Step 1: Write the CLI non-mutation regressions** + +Patch `GitHubClient.fetch_pull_request` to raise `GitHubPaginationError("GitHub pagination target +was rejected.")`. For `review` and `alpha init`, capture existing store bytes and assert: + +```python +with pytest.raises(SystemExit) as error: + main(arguments) +assert error.value.code == 2 +assert record.read_bytes() == before +assert not report_path.exists() +``` + +Also assert the captured error omits tokens, raw rejected URLs, local paths, and traceback text. + +- [ ] **Step 2: Write the Streamlit non-mutation regression** + +Prepare criteria and capture `pr_url`, requirements text, criteria models, `review_state`, `bundle`, +and snapshot-related session keys. Raise `GitHubPaginationError` from fetch and assert those values +are unchanged, no download appears, and the rendered error contains `No review data was changed.` + +- [ ] **Step 3: Run adapter tests and make only a confirmed adapter correction if required** + +```bash +uv run pytest -q tests/cli/test_cli.py -k 'pagination or fetch_failure' +uv run pytest -q tests/apps/test_streamlit_app.py -k 'public_pr_fetch_failure or pagination' +``` + +Expected: tests pass on existing fetch-before-mutate behavior. If one fails, change only the +assignment/write ordering demonstrated by that regression; do not refactor unrelated UI or CLI code. + +- [ ] **Step 4: Lint and commit the regressions intentionally** + +```bash +uv run ruff check tests/cli/test_cli.py tests/apps/test_streamlit_app.py +git add tests/cli/test_cli.py tests/apps/test_streamlit_app.py +git commit -m "test: preserve state on pagination failure" +``` + +### Task 4: Documentation and complete verification + +**Files:** +- Modify: `CHANGELOG.md` +- Modify only confirmed defects found by verification, with a failing regression first. + +**Interfaces:** +- Consumes: completed Phase 1 implementation and tests. +- Produces: repository-truth change record and exact verification evidence. + +- [ ] **Step 1: Update the unreleased changelog truth** + +Add one `0.2.4.dev0` bullet stating that public GitHub pagination is same-origin, lineage-bound, +cycle-safe, and deterministically bounded. Do not make release, adoption, platform, accessibility, +or customer claims. + +- [ ] **Step 2: Run source verification** + +```bash +uv run ruff check . +uv run pytest -q --import-mode=importlib --cov=scopeproof_core --cov=apps \ + --cov-report=term-missing --cov-fail-under=95 +uv run pytest -q tests/test_repository_contracts.py +uv run scopeproof benchmark +uv run scopeproof comparison-benchmark +``` + +Expected: complete suite passes with at least 95% combined coverage; repository contracts pass; +acceptance benchmark has zero mismatches and zero must-have False Ready outcomes; comparison +benchmark has zero mismatches. + +- [ ] **Step 3: Run artifact and installed-runtime verification** + +Build twice in isolated directories and compare wheel bytes and SHA-256. Inventory both archives, +install one wheel in a clean environment, validate dependencies, compare source/distribution/review +schema and both CLI version surfaces, run both installed benchmarks, check exact loopback health, +run the installed-wheel Chromium regression, and run supported Python lanes 3.11, 3.12, and 3.13. + +Expected: two byte-identical wheels; no unexpected packaged files; all version surfaces equal +`0.2.4.dev0`; zero benchmark mismatches; health and browser regression pass; unsupported +environments are classified without fabrication. + +- [ ] **Step 4: Audit and commit named documentation/tests** + +```bash +git diff --check +git status --short +git diff --name-only origin/main...HEAD +git log --oneline --decorate origin/main..HEAD +``` + +Inspect generated files, secrets, absolute local paths, packaging inputs, the exact branch base, and +the preserved root `.coverage 2`. Stage only `CHANGELOG.md` and any named regression files not +already committed. + +### Task 5: Independent review and ready PR + +**Files:** +- Modify only confirmed Critical or Important defects, each with a failing regression first. + +**Interfaces:** +- Consumes: the exact verified Phase 1 branch head. +- Produces: ready PR `fix: bound GitHub pagination and credential forwarding`. + +- [ ] **Step 1: Run independent read-only review** + +```bash +codex review --base origin/main +``` + +Require zero unresolved Critical or Important findings. Reproduce every proposed defect before +changing code; fix confirmed findings test-first and repeat affected plus complete verification. + +- [ ] **Step 2: Push and open the ready PR** + +Push `codex/github-pagination-boundary` and open a ready PR against the exact verified `main` base. +The description records scope, red-green evidence, verification results, credential/evidence +boundaries, Stage 1 zeros, and unsupported environments. + +- [ ] **Step 3: Monitor every available check to terminal conclusions** + +Inspect CI, CodeQL, Pages, dependency workflows, review threads, mergeability, commit list, and final +diff. Diagnose a failed check systematically; repair only confirmed Phase 1 defects on the same +branch and rerun affected/full verification. + +- [ ] **Step 4: Stop at the owner gate** + +Do not merge the Phase 1 PR or begin Phase 2. Report exact base/head SHAs, commits, diff, checks, +review findings, unsupported environments, PR #186 automatic state, `.coverage 2` proof, Stage 1 +zero counts, and the owner's exact merge-or-hold decision. diff --git a/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md b/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md new file mode 100644 index 0000000..11c4d83 --- /dev/null +++ b/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md @@ -0,0 +1,136 @@ +# GitHub Pagination and Credential Boundary Design + +## Status and scope + +This design implements the owner-approved Phase 1 boundary from verified `main` commit +`73e3ecb76d4856a156ad23d711e585ed2af499e7`. It is limited to ScopeProof's read-only public +GitHub pull-request ingestion. It does not add private-repository support, execute repository code, +change evidence interpretation, alter criteria confirmation, or advance Product Stage 1. + +## Current problem + +`GitHubClient._get_all` accepts every absolute `rel="next"` URL supplied by a response and sends +it through an `httpx.Client` whose default headers may contain the optional GitHub token. It also +collects every file and commit page before enforcing `max_files`. There is no origin or endpoint +lineage check, cycle detection, or deterministic request, page, item, or decoded-response-size +budget. + +The result is both a credential-forwarding boundary defect and an unbounded resource-consumption +path. The supported source remains anonymous public GitHub; a token may improve rate limits but +must remain session-only and must never be forwarded outside the intended API origin. + +## Considered approaches + +### A. Strict pagination inside `GitHubClient` with one per-fetch budget — selected + +Replace `_get_all` with a small bounded collection helper. Validate each `next` URL before a +request, convert an accepted URL to a relative same-origin target, track visited targets, and share +one budget across pull-request metadata, file pages, commit pages, and CI metadata. This is the +smallest core-first correction and protects every current CLI and Streamlit caller. + +### B. Disable pagination + +Only accept the first GitHub page. This removes the forwarding risk but silently turns ordinary +multi-page public pull requests into incomplete evidence and breaks supported behavior. + +### C. Strip `Authorization` only for absolute URLs + +This prevents one credential leak but still permits off-origin traffic, endpoint escape, cycles, +and unbounded downloads. It does not satisfy the ingestion trust boundary. + +## Core architecture + +Add a private per-fetch budget object and a private paginated-collection result in +`scopeproof_core/github/client.py`. + +The budget tracks: + +- all HTTP requests made by one pull-request fetch; +- all followed pagination pages; +- all decoded list items across file and commit collections; +- decoded bytes for each response and cumulatively across the fetch. + +Default ceilings are explicit constructor values: 16 requests, 10 pagination pages, 1,000 +paginated items, 4 MiB per decoded response, 16 MiB cumulatively, 100 files, and 250 commits. +Tests can lower each ceiling without changing production policy. Exhaustion raises a deterministic +`GitHubPaginationError`, a `GitHubIngestionError` subtype. + +Every response is charged after transport decoding and before JSON parsing. Request allowance is +charged before the request. This ensures an oversized or excessive response cannot be accepted +merely because later projection would discard most of it. + +## Pagination target contract + +A `next` target is accepted only when all conditions hold: + +1. The URL is absolute HTTPS with hostname exactly `api.github.com`, no user information, no + fragment, and the default HTTPS port. +2. Its path exactly matches the collection path initially requested for that pull request. A file + page cannot become a commit page, another repository, another pull request, or another endpoint. +3. Its query contains exactly one positive integer `page` and one `per_page` matching the initial + page size; unknown and duplicate parameters are rejected. +4. The response exposes at most one unambiguous `rel="next"` target. +5. The canonical path-and-query target has not already been requested or visited. + +Validation happens before issuing the request. An accepted absolute target is rendered as a +canonical relative path before it reaches the shared client, so the optional authorization header +cannot be forwarded off-origin. Malformed, ambiguous, downgraded, off-origin, escaped, repeated, +cyclic, or over-budget traversal fails the entire fetch closed. + +## Bounded collection behavior + +Files are requested with the smallest permitted page size that can observe one item beyond +`max_files` (up to GitHub's 100-item page maximum). Collection stops immediately when either an +overflow item is observed or a valid `next` link proves more files exist after `max_files`. Only +the first `max_files` entries are analyzed, the snapshot is `partial`, and warnings truthfully say +that additional changed files were not retrieved. Observed overflow filenames may be listed, but +unretrieved names are never fabricated. + +Commits use 100-item pages and stop after observing at most one item beyond `max_commits`. The +first `max_commits` commits remain in GitHub order. Truncation marks the snapshot `partial` and adds +a bounded warning; ScopeProof does not claim complete commit history. + +All retained files and commits preserve GitHub response order. A non-list collection payload or a +non-object entry is malformed input and fails closed. Existing patch-byte and total-diff limits +remain independent, conservative partial-evidence boundaries. + +## Data flow and mutation safety + +CLI and Streamlit continue to call `GitHubClient.fetch_pull_request`. The client constructs a +Pydantic-validated `PullRequestSnapshot` only after all required ingestion succeeds. Known file, +commit, or patch truncation produces a validated partial snapshot; structural or budget failures +produce no snapshot. + +CLI review and Alpha initialization already fetch before their first store write. Streamlit already +assigns fetched state only after the client call succeeds. Focused regressions bind these adapter +properties to `GitHubPaginationError`: existing saved reviews and session values remain unchanged, +and no report/export is created. + +The deterministic gate already forbids `Ready` when ingestion is partial or warnings/skipped files +exist. A focused regression will prove that a client-produced truncated snapshot remains non-Ready +through the normal bundle and gate path. + +## Test strategy + +Tests use only `httpx.MockTransport`, local fixtures, temporary storage, and Streamlit AppTest. +They first demonstrate failures for off-origin links and token forwarding, HTTP downgrade, +repository/endpoint escape, repeated URLs and cycles, ambiguous/malformed links, request/page/item +exhaustion, per-response and cumulative decoded-size exhaustion, file early-stop behavior, bounded +commit history, and deterministic ordering. + +Adapter regressions prove pagination failure is non-mutating in CLI and Streamlit. Gate coverage +proves bounded partial evidence cannot produce an unsupported `Ready` result. Existing anonymous +public-PR, candidate-file, CI observation, packaging, browser, and benchmark coverage remains green. + +## Evidence and product boundaries + +- ScopeProof remains an evidence assistant, not a correctness oracle. +- No target-repository code is executed. +- Criteria confirmation remains mandatory. +- Persisted and exported objects remain Pydantic-validated. +- Optional GitHub credentials remain session-only and are never logged or exported. +- False Ready remains more harmful than False Blocked. +- The GitHub Action remains opt-in and informational. +- Product Stage 1 remains exactly zero across all five targets. +- Phases 2–4, R-002, R-003, releases, tags, publishing, outreach, accounts, RBAC, billing, and paid + APIs remain out of scope. From 64997b875ade9950484e415b7854fe6fa91fa6af Mon Sep 17 00:00:00 2001 From: davidjiang8888 Date: Wed, 12 Aug 2026 00:15:47 -0400 Subject: [PATCH 2/5] fix: bound GitHub pagination traversal --- scopeproof_core/github/__init__.py | 4 +- scopeproof_core/github/client.py | 352 +++++++++++++-- tests/github/test_client.py | 7 +- .../github/test_pagination_and_candidates.py | 417 +++++++++++++++++- 4 files changed, 740 insertions(+), 40 deletions(-) diff --git a/scopeproof_core/github/__init__.py b/scopeproof_core/github/__init__.py index e7843d1..05b6c1b 100644 --- a/scopeproof_core/github/__init__.py +++ b/scopeproof_core/github/__init__.py @@ -1,5 +1,5 @@ """Public GitHub pull-request ingestion.""" -from scopeproof_core.github.client import GitHubClient, parse_pr_url +from scopeproof_core.github.client import GitHubClient, GitHubPaginationError, parse_pr_url -__all__ = ["GitHubClient", "parse_pr_url"] +__all__ = ["GitHubClient", "GitHubPaginationError", "parse_pr_url"] diff --git a/scopeproof_core/github/client.py b/scopeproof_core/github/client.py index 708bb6c..dc0cc4c 100644 --- a/scopeproof_core/github/client.py +++ b/scopeproof_core/github/client.py @@ -4,9 +4,10 @@ import base64 import re +from dataclasses import dataclass from datetime import UTC, datetime from pathlib import PurePosixPath -from urllib.parse import quote, urlparse +from urllib.parse import parse_qsl, quote, urlparse import httpx @@ -27,6 +28,10 @@ _HUNK_HEADER = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") _FAILING_CONCLUSIONS = {"failure", "timed_out", "cancelled", "action_required", "startup_failure"} _PASSING_CONCLUSIONS = {"success"} +_NEXT_RELATION = re.compile( + r'\brel\s*=\s*(?:"next"|next)(?=\s*(?:;|,|$))', + flags=re.IGNORECASE, +) class GitHubIngestionError(RuntimeError): @@ -64,6 +69,56 @@ class DiffLimitExceeded(GitHubIngestionError): pass +class GitHubPaginationError(GitHubIngestionError): + """A pagination target or traversal exceeded the supported trust boundary.""" + + +@dataclass +class _FetchBudget: + max_requests: int + max_pages: int + max_items: int + max_response_bytes: int + max_total_response_bytes: int + requests_used: int = 0 + pages_used: int = 0 + items_used: int = 0 + total_response_bytes: int = 0 + + def charge_request(self) -> None: + if self.requests_used >= self.max_requests: + raise GitHubPaginationError("GitHub request budget was exhausted.") + self.requests_used += 1 + + def charge_page(self) -> None: + if self.pages_used >= self.max_pages: + raise GitHubPaginationError("GitHub pagination page budget was exhausted.") + self.pages_used += 1 + + def charge_items(self, count: int) -> None: + if self.items_used + count > self.max_items: + raise GitHubPaginationError("GitHub pagination item budget was exhausted.") + self.items_used += count + + def charge_response(self, response: httpx.Response) -> None: + decoded_bytes = len(response.content) + if decoded_bytes > self.max_response_bytes: + raise GitHubPaginationError( + "GitHub decoded response byte budget was exhausted." + ) + if self.total_response_bytes + decoded_bytes > self.max_total_response_bytes: + raise GitHubPaginationError( + "GitHub total decoded response byte budget was exhausted." + ) + self.total_response_bytes += decoded_bytes + + +@dataclass(frozen=True) +class _PaginatedResult: + items: list[dict] + truncated: bool = False + + def _reported_total_note(payload: dict, label: str, valid_entry_count: int) -> str | None: """Return a fail-closed diagnostic when a supplied GitHub total is not exact.""" if "total_count" not in payload: @@ -148,10 +203,29 @@ def __init__( max_files: int = 100, max_patch_bytes: int = 200_000, max_total_diff_bytes: int = 1_000_000, + max_commits: int = 250, max_candidate_files: int = 8, max_candidate_bytes: int = 200_000, + max_requests: int = 16, + max_pagination_pages: int = 10, + max_pagination_items: int = 1_000, + max_response_bytes: int = 4 * 1024 * 1024, + max_total_response_bytes: int = 16 * 1024 * 1024, timeout_seconds: float = 15.0, ) -> None: + configured_limits = { + "max_files": max_files, + "max_commits": max_commits, + "max_requests": max_requests, + "max_pagination_pages": max_pagination_pages, + "max_pagination_items": max_pagination_items, + "max_response_bytes": max_response_bytes, + "max_total_response_bytes": max_total_response_bytes, + } + invalid_limits = [name for name, value in configured_limits.items() if value <= 0] + if invalid_limits: + names = ", ".join(invalid_limits) + raise ValueError(f"GitHub ingestion limits must be positive: {names}") headers = { "Accept": "application/vnd.github+json", "X-GitHub-Api-Version": "2022-11-28", @@ -166,20 +240,34 @@ def __init__( timeout=timeout_seconds, ) self.max_files = max_files + self.max_commits = max_commits self.max_patch_bytes = max_patch_bytes self.max_total_diff_bytes = max_total_diff_bytes self.max_candidate_files = max_candidate_files self.max_candidate_bytes = max_candidate_bytes + self.max_requests = max_requests + self.max_pagination_pages = max_pagination_pages + self.max_pagination_items = max_pagination_items + self.max_response_bytes = max_response_bytes + self.max_total_response_bytes = max_total_response_bytes self.last_request_authorized = "Authorization" in headers def _get( - self, path: str, *, params: dict[str, str] | None = None + self, + path: str, + *, + params: dict[str, str] | None = None, + budget: _FetchBudget | None = None, ) -> httpx.Response: + if budget is not None: + budget.charge_request() try: response = self._client.get(path, params=params) except httpx.HTTPError as error: message = "Could not reach GitHub. Retry without losing criteria." raise GitHubNetworkError(message) from error + if budget is not None: + budget.charge_response(response) return response @staticmethod @@ -232,16 +320,166 @@ def _verified_repository_visibility( ) return RepositoryVisibility.VERIFIED_PUBLIC - def _get_all(self, path: str) -> list[dict]: - """Follow GitHub pagination while retaining normal HTTP error handling.""" - response = self._get(path) - self._raise_for_pr(response) - items = list(response.json()) - while next_link := response.links.get("next", {}).get("url"): - response = self._get(next_link) + @staticmethod + def _next_link(response: httpx.Response) -> str | None: + raw_link = ", ".join(response.headers.get_list("link")) + if not raw_link: + return None + next_relation_count = len(_NEXT_RELATION.findall(raw_link)) + if next_relation_count > 1: + raise GitHubPaginationError( + "GitHub pagination response contained ambiguous next links." + ) + try: + parsed_links = response.links + next_link = parsed_links.get("next", {}).get("url") + except (KeyError, TypeError, ValueError) as error: + raise GitHubPaginationError( + "GitHub pagination response contained a malformed Link header." + ) from error + if next_relation_count == 0 and next_link is None: + if not parsed_links or any( + not isinstance(link.get("rel"), str) + or not link["rel"] + or not isinstance(link.get("url"), str) + or not link["url"] + for link in parsed_links.values() + ): + raise GitHubPaginationError( + "GitHub pagination response contained a malformed Link header." + ) + return None + if next_relation_count != 1 or not isinstance(next_link, str) or not next_link: + raise GitHubPaginationError( + "GitHub pagination response contained a malformed next link." + ) + return next_link + + @staticmethod + def _validated_pagination_target( + target: str, + *, + expected_path: str, + per_page: int, + ) -> tuple[str, str]: + try: + parsed_target = urlparse(target) + target_port = parsed_target.port + query_items = parse_qsl( + parsed_target.query, + keep_blank_values=True, + strict_parsing=True, + ) + except ValueError as error: + raise GitHubPaginationError( + "GitHub pagination target is malformed." + ) from error + if ( + parsed_target.scheme != "https" + or parsed_target.hostname != "api.github.com" + or parsed_target.username is not None + or parsed_target.password is not None + or parsed_target.fragment + or target_port not in {None, 443} + ): + raise GitHubPaginationError( + "GitHub pagination target is outside the expected GitHub API origin." + ) + if parsed_target.path != expected_path: + raise GitHubPaginationError( + "GitHub pagination target escaped the expected repository endpoint." + ) + if len(query_items) != 2 or {name for name, _ in query_items} != { + "page", + "per_page", + }: + raise GitHubPaginationError( + "GitHub pagination target query is malformed or ambiguous." + ) + query = dict(query_items) + page_text = query.get("page", "") + per_page_text = query.get("per_page", "") + if not re.fullmatch(r"[1-9][0-9]*", page_text) or not re.fullmatch( + r"[1-9][0-9]*", per_page_text + ): + raise GitHubPaginationError( + "GitHub pagination target query is malformed or ambiguous." + ) + try: + page = int(page_text) + target_per_page = int(per_page_text) + except (KeyError, ValueError) as error: + raise GitHubPaginationError( + "GitHub pagination target query is malformed or ambiguous." + ) from error + if page < 1 or target_per_page != per_page: + raise GitHubPaginationError( + "GitHub pagination target query is malformed or ambiguous." + ) + canonical_target = f"{expected_path}?page={page}&per_page={per_page}" + return canonical_target, canonical_target + + def _get_paginated( + self, + path: str, + *, + expected_path: str, + per_page: int, + retain_limit: int, + budget: _FetchBudget, + ) -> _PaginatedResult: + """Return one ordered, lineage-bound collection with bounded overflow.""" + canonical_initial = f"{expected_path}?page=1&per_page={per_page}" + visited = {canonical_initial} + response = self._get( + path, + params={"per_page": str(per_page)}, + budget=budget, + ) + items: list[dict] = [] + while True: self._raise_for_pr(response) - items.extend(response.json()) - return items + budget.charge_page() + try: + page_items = response.json() + except ValueError as error: + raise GitHubPaginationError( + "GitHub pagination response was not valid JSON." + ) from error + if not isinstance(page_items, list) or any( + not isinstance(item, dict) for item in page_items + ): + raise GitHubPaginationError( + "GitHub pagination expected a list response of objects." + ) + budget.charge_items(len(page_items)) + remaining = retain_limit + 1 - len(items) + if remaining > 0: + items.extend(page_items[:remaining]) + next_link = self._next_link(response) + canonical_target: str | None = None + relative_target: str | None = None + if next_link is not None: + canonical_target, relative_target = self._validated_pagination_target( + next_link, + expected_path=expected_path, + per_page=per_page, + ) + if canonical_target in visited: + raise GitHubPaginationError( + "GitHub pagination did not advance; a cycle or repeated target " + "was rejected." + ) + if len(items) > retain_limit: + return _PaginatedResult(items=items, truncated=True) + if next_link is None: + return _PaginatedResult(items=items) + if len(items) == retain_limit: + return _PaginatedResult(items=items, truncated=True) + assert canonical_target is not None + assert relative_target is not None + visited.add(canonical_target) + response = self._get(relative_target, budget=budget) @staticmethod def _validate_candidate_path(path: str) -> str: @@ -515,7 +753,14 @@ def _check_state(check_runs: dict, commit_status: dict) -> CheckState: def fetch_pull_request(self, url: str) -> PullRequestSnapshot: owner, repository, pr_number = parse_pr_url(url) root = f"/repos/{owner}/{repository}" - pr_response = self._get(f"{root}/pulls/{pr_number}") + budget = _FetchBudget( + max_requests=self.max_requests, + max_pages=self.max_pagination_pages, + max_items=self.max_pagination_items, + max_response_bytes=self.max_response_bytes, + max_total_response_bytes=self.max_total_response_bytes, + ) + pr_response = self._get(f"{root}/pulls/{pr_number}", budget=budget) self._raise_for_pr(pr_response) pr_data = pr_response.json() repository_visibility = self._verified_repository_visibility( @@ -523,28 +768,74 @@ def fetch_pull_request(self, url: str) -> PullRequestSnapshot: expected_repository=f"{owner}/{repository}", ) - raw_files = self._get_all(f"{root}/pulls/{pr_number}/files?per_page=100") - raw_commits = self._get_all(f"{root}/pulls/{pr_number}/commits?per_page=100") + files_path = f"{root}/pulls/{pr_number}/files" + file_result = self._get_paginated( + files_path, + expected_path=files_path, + per_page=min(100, self.max_files + 1), + retain_limit=self.max_files, + budget=budget, + ) + for item in file_result.items: + filename = item.get("filename") + if not isinstance(filename, str) or not filename: + raise GitHubIngestionError("GitHub returned malformed file metadata.") + raw_files = file_result.items[: self.max_files] + observed_file_overflow = file_result.items[self.max_files :] + + commits_path = f"{root}/pulls/{pr_number}/commits" + commit_result = self._get_paginated( + commits_path, + expected_path=commits_path, + per_page=min(100, self.max_commits + 1), + retain_limit=self.max_commits, + budget=budget, + ) + all_commits: list[CommitInfo] = [] + for item in commit_result.items: + sha = item.get("sha") + commit = item.get("commit") + message = commit.get("message") if isinstance(commit, dict) else None + html_url = item.get("html_url") + if ( + not isinstance(sha, str) + or not sha + or not isinstance(message, str) + or not isinstance(html_url, str) + ): + raise GitHubIngestionError("GitHub returned malformed commit metadata.") + all_commits.append(CommitInfo(sha=sha, message=message, html_url=html_url)) head_sha = pr_data["head"]["sha"] - check_response = self._get(f"{root}/commits/{head_sha}/check-runs?per_page=100") - status_response = self._get(f"{root}/commits/{head_sha}/status?per_page=100") + check_response = self._get( + f"{root}/commits/{head_sha}/check-runs", + params={"per_page": "100"}, + budget=budget, + ) + status_response = self._get( + f"{root}/commits/{head_sha}/status", + params={"per_page": "100"}, + budget=budget, + ) check_data = check_response.json() if check_response.is_success else {} status_data = status_response.json() if status_response.is_success else {} warnings: list[str] = [] skipped_files: list[str] = [] ingestion_state = IngestionState.COMPLETE - for item in raw_files: - if not isinstance(item, dict): - raise GitHubIngestionError("GitHub returned malformed file metadata.") - filename = item.get("filename") - if not isinstance(filename, str) or not filename: - raise GitHubIngestionError("GitHub returned malformed file metadata.") - if len(raw_files) > self.max_files: - skipped_files.extend(item["filename"] for item in raw_files[self.max_files :]) - raw_files = raw_files[: self.max_files] - warnings.append(f"File limit reached; skipped {len(skipped_files)} changed files.") + if file_result.truncated: + for item in observed_file_overflow: + filename = item.get("filename") + if isinstance(filename, str) and filename: + skipped_files.append(filename) + warnings.append( + "File limit reached; additional changed files were not retrieved." + ) + ingestion_state = IngestionState.PARTIAL + if commit_result.truncated: + warnings.append( + "Commit history limit reached; additional commits were not retrieved." + ) ingestion_state = IngestionState.PARTIAL total_bytes = 0 @@ -598,14 +889,7 @@ def fetch_pull_request(self, url: str) -> PullRequestSnapshot: f"{diff_limit_skipped_count} changed files." ) - commits = [ - CommitInfo( - sha=item["sha"], - message=item.get("commit", {}).get("message", ""), - html_url=item.get("html_url", ""), - ) - for item in raw_commits - ] + commits = all_commits[: self.max_commits] ci_observation = self._check_observation( check_data, status_data, diff --git a/tests/github/test_client.py b/tests/github/test_client.py index a5b5ffa..26e2ad3 100644 --- a/tests/github/test_client.py +++ b/tests/github/test_client.py @@ -804,8 +804,11 @@ def test_file_limit_marks_snapshot_partial_and_lists_skipped_files() -> None: snapshot = client.fetch_pull_request("https://github.com/acme/widget/pull/42") assert snapshot.ingestion_state is IngestionState.PARTIAL assert len(snapshot.files) == 1 - assert snapshot.skipped_files == ["src/export_1.py", "src/export_2.py"] - assert any("file limit" in warning.lower() for warning in snapshot.warnings) + assert snapshot.skipped_files == ["src/export_1.py"] + assert any( + "additional changed files were not retrieved" in warning + for warning in snapshot.warnings + ) def test_snapshot_json_contains_no_authorization_header() -> None: diff --git a/tests/github/test_pagination_and_candidates.py b/tests/github/test_pagination_and_candidates.py index 5bcf5fa..02a4abb 100644 --- a/tests/github/test_pagination_and_candidates.py +++ b/tests/github/test_pagination_and_candidates.py @@ -8,6 +8,66 @@ from scopeproof_core.github.client import GitHubClient, GitHubIngestionError HEAD_SHA = "b" * 40 +PR_URL = "https://github.com/acme/widget/pull/42" + + +def _pull_payload() -> dict: + return { + "number": 42, + "title": "Paged export", + "body": "", + "html_url": PR_URL, + "base": { + "sha": "a" * 40, + "repo": { + "full_name": "acme/widget", + "private": False, + "visibility": "public", + }, + }, + "head": {"sha": HEAD_SHA}, + } + + +def pagination_transport( + *, + file_pages: dict[int, object] | None = None, + file_links: dict[int, str] | None = None, + commit_pages: dict[int, object] | None = None, + commit_links: dict[int, str] | None = None, + pull_payload: dict | None = None, +) -> tuple[httpx.MockTransport, list[httpx.Request]]: + requests: list[httpx.Request] = [] + file_pages = file_pages or {1: []} + file_links = file_links or {} + commit_pages = commit_pages or {1: []} + commit_links = commit_links or {} + + def response( + data: object, + *, + link: str | None = None, + ) -> httpx.Response: + headers = {"Link": link} if link is not None else None + return httpx.Response(200, json=data, headers=headers) + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + path = request.url.path + page = int(request.url.params.get("page", "1")) + if path == "/repos/acme/widget/pulls/42": + return response(pull_payload or _pull_payload()) + if path == "/repos/acme/widget/pulls/42/files": + return response(file_pages.get(page, []), link=file_links.get(page)) + if path == "/repos/acme/widget/pulls/42/commits": + return response(commit_pages.get(page, []), link=commit_links.get(page)) + if path == f"/repos/acme/widget/commits/{HEAD_SHA}/check-runs": + return response({"check_runs": []}) + if path == f"/repos/acme/widget/commits/{HEAD_SHA}/status": + return response({"state": "success"}) + return httpx.Response(404, json={"message": path}) + + return httpx.MockTransport(handler), requests def paged_transport() -> httpx.MockTransport: @@ -46,8 +106,8 @@ def handler(request: httpx.Request) -> httpx.Response: ], headers={ "Link": ( - "; " - 'rel="next"' + "; rel="next"' ) }, ) @@ -83,6 +143,359 @@ def test_files_pagination_continues_until_no_next_page() -> None: assert snapshot.skipped_files == [] +def test_pagination_rejects_off_origin_before_forwarding_token() -> None: + requests: list[httpx.Request] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + if request.url.path == "/repos/acme/widget/pulls/42": + return httpx.Response(200, json=_pull_payload()) + if request.url.path.endswith("/files") and request.url.host == "api.github.com": + return httpx.Response( + 200, + json=[], + headers={ + "Link": ( + "; rel="next"' + ) + }, + ) + if request.url.host == "attacker.invalid": + return httpx.Response(200, json=[]) + if request.url.path.endswith("/commits"): + return httpx.Response(200, json=[]) + if request.url.path.endswith("/check-runs"): + return httpx.Response(200, json={"check_runs": []}) + if request.url.path.endswith("/status"): + return httpx.Response(200, json={"state": "success"}) + return httpx.Response(404, json={"message": request.url.path}) + + client = GitHubClient( + token="session-secret", + transport=httpx.MockTransport(handler), + ) + + with pytest.raises(GitHubIngestionError, match="expected GitHub API origin"): + client.fetch_pull_request(PR_URL) + + assert [request.url.host for request in requests] == [ + "api.github.com", + "api.github.com", + ] + assert all(request.url.host != "attacker.invalid" for request in requests) + + +def test_pagination_validates_unfollowed_next_link_at_overflow_boundary() -> None: + files = [ + { + "filename": f"src/{index}.py", + "status": "modified", + "patch": f"@@ -1 +1 @@\n+value = {index}", + } + for index in range(2) + ] + transport, requests = pagination_transport( + file_pages={1: files}, + file_links={ + 1: ( + "; rel="next"' + ) + }, + ) + + with pytest.raises(GitHubIngestionError, match="expected GitHub API origin"): + GitHubClient(transport=transport, max_files=1).fetch_pull_request(PR_URL) + + assert len(requests) == 2 + + +@pytest.mark.parametrize( + "next_link", + [ + "http://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=100", + "https://attacker.invalid/repos/acme/widget/pulls/42/files?page=2&per_page=100", + "https://api.github.com/repos/other/widget/pulls/42/files?page=2&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/43/files?page=2&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/42/commits?page=2&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=100&extra=1", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&page=3&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=2", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=+2&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=02&per_page=100", + "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=0100", + ], +) +def test_pagination_rejects_downgrade_escape_and_ambiguous_queries( + next_link: str, +) -> None: + transport, requests = pagination_transport( + file_links={1: f'<{next_link}>; rel="next"'}, + ) + + with pytest.raises(GitHubIngestionError, match="pagination target"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert len(requests) == 2 + + +def test_pagination_rejects_multiple_next_relations() -> None: + transport, requests = pagination_transport( + file_links={ + 1: ( + "; rel="next", ' + "; rel="next"' + ) + }, + ) + + with pytest.raises(GitHubIngestionError, match="ambiguous"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert len(requests) == 2 + + +def test_pagination_allows_terminal_link_header_without_next_relation() -> None: + transport, _ = pagination_transport( + file_links={ + 1: ( + "; rel="first", ' + "; rel="last"' + ) + }, + ) + + snapshot = GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert snapshot.files == [] + + +def test_pagination_rejects_malformed_link_header_without_relation() -> None: + transport, _ = pagination_transport(file_links={1: "not-a-link-header"}) + + with pytest.raises(GitHubIngestionError, match="malformed Link header"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + +def test_pagination_rejects_two_page_cycle_before_repeating_request() -> None: + second_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=100" + first_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=1&per_page=100" + transport, requests = pagination_transport( + file_pages={1: [], 2: []}, + file_links={ + 1: f'<{second_page}>; rel="next"', + 2: f'<{first_page}>; rel="next"', + }, + ) + + with pytest.raises(GitHubIngestionError, match=r"cycle|repeated"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert [request.url.params.get("page") for request in requests] == [None, None, "2"] + + +def test_pagination_rejects_non_advancing_page_sequence() -> None: + second_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=100" + earlier_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=1&per_page=100" + transport, requests = pagination_transport( + file_pages={1: [], 2: []}, + file_links={ + 1: f'<{second_page}>; rel="next"', + 2: f'<{earlier_page}>; rel="next"', + }, + ) + + with pytest.raises(GitHubIngestionError, match="advance"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert [request.url.params.get("page") for request in requests] == [None, None, "2"] + + +@pytest.mark.parametrize( + ("client_kwargs", "expected_message"), + [ + ({"max_requests": 1}, "request budget"), + ({"max_pagination_pages": 1}, "page budget"), + ({"max_pagination_items": 1}, "item budget"), + ({"max_response_bytes": 64}, "decoded response byte budget"), + ], +) +def test_pagination_enforces_independent_budgets( + client_kwargs: dict[str, int], + expected_message: str, +) -> None: + item = { + "filename": "src/first.py", + "status": "modified", + "patch": "@@ -1 +1 @@\n+first", + } + second_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=100" + transport, _ = pagination_transport( + file_pages={1: [item], 2: [item]}, + file_links={1: f'<{second_page}>; rel="next"'}, + ) + + with pytest.raises(GitHubIngestionError, match=expected_message): + GitHubClient(transport=transport, **client_kwargs).fetch_pull_request(PR_URL) + + +def test_pagination_enforces_cumulative_decoded_response_budget() -> None: + payload = _pull_payload() + payload["body"] = "p" * 5_000 + file_item = { + "filename": "src/large.py", + "status": "modified", + "patch": "x" * 5_000, + } + transport, _ = pagination_transport( + pull_payload=payload, + file_pages={1: [file_item]}, + ) + + with pytest.raises(GitHubIngestionError, match="total decoded response byte budget"): + GitHubClient( + transport=transport, + max_response_bytes=7_000, + max_total_response_bytes=9_000, + ).fetch_pull_request(PR_URL) + + +def test_pagination_rejects_non_list_collection_payload() -> None: + transport, _ = pagination_transport(file_pages={1: {"filename": "src/not-a-list.py"}}) + + with pytest.raises(GitHubIngestionError, match="list response"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + +@pytest.mark.parametrize("collection", ["files", "commits"]) +def test_pagination_rejects_malformed_observed_overflow_item(collection: str) -> None: + file_item = { + "filename": "src/valid.py", + "status": "modified", + "patch": "@@ -1 +1 @@\n+valid = True", + } + commit_item = { + "sha": "1" * 40, + "commit": {"message": "Valid"}, + "html_url": "https://github.com/acme/widget/commit/1", + } + kwargs: dict[str, object] = {"max_files": 1, "max_commits": 1} + transport_kwargs = ( + {"file_pages": {1: [file_item, {}]}} + if collection == "files" + else {"commit_pages": {1: [commit_item, {}]}} + ) + transport, _ = pagination_transport(**transport_kwargs) + + with pytest.raises(GitHubIngestionError, match=f"malformed {collection[:-1]} metadata"): + GitHubClient(transport=transport, **kwargs).fetch_pull_request(PR_URL) + + +def test_file_ingestion_stops_after_smallest_observed_overflow() -> None: + files = [ + { + "filename": f"src/{index:03d}.py", + "status": "modified", + "patch": f"@@ -1 +1 @@\n+value = {index}", + } + for index in range(3) + ] + second_page = "https://api.github.com/repos/acme/widget/pulls/42/files?page=2&per_page=2" + transport, requests = pagination_transport( + file_pages={1: files[:2], 2: files[2:]}, + file_links={1: f'<{second_page}>; rel="next"'}, + ) + + snapshot = GitHubClient(transport=transport, max_files=1).fetch_pull_request(PR_URL) + + assert [item.path for item in snapshot.files] == ["src/000.py"] + assert snapshot.skipped_files == ["src/001.py"] + assert snapshot.ingestion_state.value == "partial" + assert any( + "additional changed files were not retrieved" in warning + for warning in snapshot.warnings + ) + file_requests = [request for request in requests if request.url.path.endswith("/files")] + assert [request.url.params.get("page") for request in file_requests] == [None] + + +def test_commit_ingestion_is_bounded_and_preserves_source_order() -> None: + commits = [ + { + "sha": str(index) * 40, + "commit": {"message": f"Commit {index}"}, + "html_url": f"https://github.com/acme/widget/commit/{index}", + } + for index in range(1, 5) + ] + second_page = ( + "https://api.github.com/repos/acme/widget/pulls/42/commits?page=2&per_page=3" + ) + transport, requests = pagination_transport( + commit_pages={1: commits[:3], 2: commits[3:]}, + commit_links={1: f'<{second_page}>; rel="next"'}, + ) + + snapshot = GitHubClient(transport=transport, max_commits=2).fetch_pull_request(PR_URL) + + assert [item.sha for item in snapshot.commits] == ["1" * 40, "2" * 40] + assert snapshot.ingestion_state.value == "partial" + assert any("commit history" in warning.lower() for warning in snapshot.warnings) + commit_requests = [request for request in requests if request.url.path.endswith("/commits")] + assert [request.url.params.get("page") for request in commit_requests] == [None] + + +def test_paginated_files_and_commits_keep_github_order() -> None: + first_file = { + "filename": "src/first.py", + "status": "modified", + "patch": "@@ -1 +1 @@\n+first", + } + second_file = { + "filename": "src/second.py", + "status": "modified", + "patch": "@@ -1 +1 @@\n+second", + } + commits = [ + { + "sha": character * 40, + "commit": {"message": character}, + "html_url": f"https://github.com/acme/widget/commit/{character}", + } + for character in ("a", "b") + ] + transport, _ = pagination_transport( + file_pages={1: [first_file], 2: [second_file]}, + file_links={ + 1: ( + "; rel="next"' + ) + }, + commit_pages={1: commits[:1], 2: commits[1:]}, + commit_links={ + 1: ( + "; rel="next"' + ) + }, + ) + + snapshot = GitHubClient( + transport=transport, + max_files=5, + max_commits=5, + ).fetch_pull_request(PR_URL) + + assert [item.path for item in snapshot.files] == ["src/first.py", "src/second.py"] + assert [item.sha for item in snapshot.commits] == ["a" * 40, "b" * 40] + + def test_candidate_file_is_bounded_and_anchored_to_head_sha() -> None: client = GitHubClient( transport=paged_transport(), max_candidate_files=1, max_candidate_bytes=128 From 57cd4ce2a64bdb9519fcd28dd566e73bbb06dd49 Mon Sep 17 00:00:00 2001 From: davidjiang8888 Date: Wed, 12 Aug 2026 00:15:58 -0400 Subject: [PATCH 3/5] test: preserve state on pagination failure --- tests/apps/test_streamlit_app.py | 17 ++++++++--- tests/cli/test_cli.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/tests/apps/test_streamlit_app.py b/tests/apps/test_streamlit_app.py index b9dc5e0..7420aef 100644 --- a/tests/apps/test_streamlit_app.py +++ b/tests/apps/test_streamlit_app.py @@ -17,7 +17,7 @@ ) from scopeproof_core.demo import load_demo_snapshot from scopeproof_core.gates.evaluator import evaluate_gate -from scopeproof_core.github.client import GitHubNetworkError +from scopeproof_core.github.client import GitHubNetworkError, GitHubPaginationError from scopeproof_core.reviews.lifecycle import ( append_external_verification, append_resolution, @@ -1599,7 +1599,16 @@ def test_canonical_public_pr_url_enables_fetch_without_format_warning() -> None: assert app.button(key="fetch_pr").disabled is False -def test_public_pr_fetch_failure_preserves_inputs_and_shows_retry_guidance() -> None: +@pytest.mark.parametrize( + "fetch_error", + [ + GitHubNetworkError("Could not reach GitHub."), + GitHubPaginationError("GitHub pagination target was rejected."), + ], +) +def test_public_pr_fetch_failure_preserves_inputs_and_shows_retry_guidance( + fetch_error: Exception, +) -> None: requirement = "The export error state remains visible and retryable." app = new_app() app = app.text_input(key="pr_url").set_value( @@ -1610,12 +1619,12 @@ def test_public_pr_fetch_failure_preserves_inputs_and_shows_retry_guidance() -> with patch( "scopeproof_core.github.client.GitHubClient.fetch_pull_request", - side_effect=GitHubNetworkError("Could not reach GitHub."), + side_effect=fetch_error, ): app = app.button(key="fetch_pr").click().run() rendered_errors = "\n".join(item.value for item in app.error) - assert "Could not reach GitHub." in rendered_errors + assert str(fetch_error) in rendered_errors assert "No review data was changed." in rendered_errors assert "Verify that the PR is public and try again." in rendered_errors assert app.text_input(key="pr_url").value == "https://github.com/acme/widget/pull/42" diff --git a/tests/cli/test_cli.py b/tests/cli/test_cli.py index bbe36a7..cd95a0e 100644 --- a/tests/cli/test_cli.py +++ b/tests/cli/test_cli.py @@ -17,6 +17,7 @@ from scopeproof_core.demo import build_demo_review, build_review_from_paths from scopeproof_core.evals.comparison_runner import run_bundled_comparison_benchmark from scopeproof_core.gates.evaluator import evaluate_gate +from scopeproof_core.github.client import GitHubPaginationError from scopeproof_core.reviews.lifecycle import ( append_external_verification, append_resolution, @@ -304,6 +305,53 @@ def test_live_review_rejects_unverified_snapshot_without_saving( assert not storage.exists() +def test_live_review_pagination_failure_preserves_storage_and_report( + tmp_path: Path, + capsys, + monkeypatch: pytest.MonkeyPatch, +) -> None: + requirements = tmp_path / "requirements.txt" + requirements.write_text("Export CSV\n", encoding="utf-8") + confirmation = write_requirements_confirmation(requirements) + storage = tmp_path / "reviews" + storage.mkdir() + sentinel = storage / "existing-record.json" + sentinel.write_bytes(b'{"preserved":true}\n') + before = sentinel.read_bytes() + report = tmp_path / "must-not-exist.json" + monkeypatch.setattr( + "scopeproof_core.cli.GitHubClient.fetch_pull_request", + lambda _self, _pr: (_ for _ in ()).throw( + GitHubPaginationError("GitHub pagination target was rejected.") + ), + ) + + with pytest.raises(SystemExit) as error: + main( + [ + "review", + "--pr", + "https://github.com/acme/repo/pull/7", + "--requirements", + str(requirements), + "--confirmation", + str(confirmation), + "--storage-dir", + str(storage), + "--report", + str(report), + ] + ) + + assert error.value.code == 2 + stderr = capsys.readouterr().err + assert "pagination target was rejected" in stderr + assert "Traceback" not in stderr + assert sentinel.read_bytes() == before + assert list(storage.iterdir()) == [sentinel] + assert not report.exists() + + def test_live_review_persists_verified_public_snapshot_provenance( tmp_path: Path, capsys, monkeypatch: pytest.MonkeyPatch ) -> None: From 1698f04cb066c683ea25e7af39ec6045d6307657 Mon Sep 17 00:00:00 2001 From: davidjiang8888 Date: Wed, 12 Aug 2026 00:21:15 -0400 Subject: [PATCH 4/5] docs: record bounded GitHub ingestion --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aae8a7..05859b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ Development version: `0.2.4.dev0`. Public install remains the immutable v0.2.3 r ### Post-release engineering +- Bound public GitHub file and commit pagination to the exact HTTPS API origin and expected + repository endpoint, rejecting ambiguous, escaped, cyclic, or over-budget traversal before an + optional session token can be forwarded. File and commit truncation remains ordered, explicit, + and fail-closed evidence. - Updated the transitive GitPython lock from 3.1.57 to 3.1.59 after GitHub reported six Dependabot alerts fixed in 3.1.58 and upstream documented five additional security fixes in 3.1.59. The repository contract now rejects lower versions. GitPython remains an indirect From bd39e5b5462090757c6c9e01dcb1695ec4336e96 Mon Sep 17 00:00:00 2001 From: davidjiang8888 Date: Wed, 12 Aug 2026 00:55:12 -0400 Subject: [PATCH 5/5] fix: accept verified GitHub canonical page paths --- .../2026-08-12-github-pagination-boundary.md | 13 ++-- ...08-12-github-pagination-boundary-design.md | 6 +- scopeproof_core/github/client.py | 46 +++++++++--- .../github/test_pagination_and_candidates.py | 71 ++++++++++++++++++- 4 files changed, 118 insertions(+), 18 deletions(-) diff --git a/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md b/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md index 333eef4..c128e8f 100644 --- a/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md +++ b/docs/superpowers/plans/2026-08-12-github-pagination-boundary.md @@ -44,7 +44,8 @@ malformed or exhausted traversals raise a user-safe ingestion error before adapt - Produces: private `_FetchBudget.charge_request()`, `_FetchBudget.charge_response(response)`, `_FetchBudget.charge_page()`, and `_FetchBudget.charge_items(count)` methods. - Produces: private `_PaginatedResult(items: list[dict], truncated: bool)`. -- Produces: `_get_paginated(path, *, expected_path, per_page, retain_limit, budget)`. +- Produces: `_get_paginated(path, *, expected_paths, canonical_path, per_page, retain_limit, + budget)`. - Consumes: existing `_get`, `_raise_for_pr`, and `httpx.Response` behavior. - [ ] **Step 1: Write failing origin, lineage, and cycle tests** @@ -116,9 +117,10 @@ Extend `GitHubClient.__init__` with `max_commits=250`, `max_requests=16`, `max_total_response_bytes=16 * 1024 * 1024`. Reject non-positive limits with `ValueError`. Implement a target validator with `urlsplit` and `parse_qsl(keep_blank_values=True)`. Require HTTPS, -hostname `api.github.com`, no user information or fragment, port absent or 443, exact collection -path, and exactly `[('page', positive_integer), ('per_page', expected_size)]` after deterministic -sorting. Return a relative canonical target such as: +hostname `api.github.com`, no user information or fragment, port absent or 443, the exact named +collection path or GitHub's `/repositories/ID` alias bound to verified PR metadata, and exactly +`[('page', positive_integer), ('per_page', expected_size)]` after deterministic sorting. Return a +relative request target and one named-path canonical identity such as: ```python return f"{expected_path}?page={page}&per_page={per_page}" @@ -201,7 +203,8 @@ files with `per_page=min(100, max_files + 1)` and commits with ```python file_result = self._get_paginated( f"{root}/pulls/{pr_number}/files", - expected_path=f"{root}/pulls/{pr_number}/files", + expected_paths=frozenset({named_path, verified_numeric_path}), + canonical_path=named_path, per_page=file_page_size, retain_limit=self.max_files, budget=budget, diff --git a/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md b/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md index 11c4d83..4444959 100644 --- a/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md +++ b/docs/superpowers/specs/2026-08-12-github-pagination-boundary-design.md @@ -65,8 +65,10 @@ A `next` target is accepted only when all conditions hold: 1. The URL is absolute HTTPS with hostname exactly `api.github.com`, no user information, no fragment, and the default HTTPS port. -2. Its path exactly matches the collection path initially requested for that pull request. A file - page cannot become a commit page, another repository, another pull request, or another endpoint. +2. Its path matches either the collection path initially requested for that pull request or + GitHub's numeric `/repositories/ID` canonical alias bound to the verified base-repository ID. + A file page cannot become a commit page, another repository, another pull request, or another + endpoint. 3. Its query contains exactly one positive integer `page` and one `per_page` matching the initial page size; unknown and duplicate parameters are rejected. 4. The response exposes at most one unambiguous `rel="next"` target. diff --git a/scopeproof_core/github/client.py b/scopeproof_core/github/client.py index dc0cc4c..424d085 100644 --- a/scopeproof_core/github/client.py +++ b/scopeproof_core/github/client.py @@ -359,7 +359,8 @@ def _next_link(response: httpx.Response) -> str | None: def _validated_pagination_target( target: str, *, - expected_path: str, + expected_paths: frozenset[str], + canonical_path: str, per_page: int, ) -> tuple[str, str]: try: @@ -385,7 +386,7 @@ def _validated_pagination_target( raise GitHubPaginationError( "GitHub pagination target is outside the expected GitHub API origin." ) - if parsed_target.path != expected_path: + if parsed_target.path not in expected_paths: raise GitHubPaginationError( "GitHub pagination target escaped the expected repository endpoint." ) @@ -416,20 +417,22 @@ def _validated_pagination_target( raise GitHubPaginationError( "GitHub pagination target query is malformed or ambiguous." ) - canonical_target = f"{expected_path}?page={page}&per_page={per_page}" - return canonical_target, canonical_target + canonical_target = f"{canonical_path}?page={page}&per_page={per_page}" + relative_target = f"{parsed_target.path}?page={page}&per_page={per_page}" + return canonical_target, relative_target def _get_paginated( self, path: str, *, - expected_path: str, + expected_paths: frozenset[str], + canonical_path: str, per_page: int, retain_limit: int, budget: _FetchBudget, ) -> _PaginatedResult: """Return one ordered, lineage-bound collection with bounded overflow.""" - canonical_initial = f"{expected_path}?page=1&per_page={per_page}" + canonical_initial = f"{canonical_path}?page=1&per_page={per_page}" visited = {canonical_initial} response = self._get( path, @@ -462,7 +465,8 @@ def _get_paginated( if next_link is not None: canonical_target, relative_target = self._validated_pagination_target( next_link, - expected_path=expected_path, + expected_paths=expected_paths, + canonical_path=canonical_path, per_page=per_page, ) if canonical_target in visited: @@ -767,11 +771,29 @@ def fetch_pull_request(self, url: str) -> PullRequestSnapshot: pr_data, expected_repository=f"{owner}/{repository}", ) + base = pr_data.get("base") if isinstance(pr_data, dict) else None + repository_data = base.get("repo") if isinstance(base, dict) else None + repository_id = ( + repository_data.get("id") if isinstance(repository_data, dict) else None + ) + verified_repository_id = ( + repository_id + if isinstance(repository_id, int) + and not isinstance(repository_id, bool) + and repository_id > 0 + else None + ) files_path = f"{root}/pulls/{pr_number}/files" + file_paths = {files_path} + if verified_repository_id is not None: + file_paths.add( + f"/repositories/{verified_repository_id}/pulls/{pr_number}/files" + ) file_result = self._get_paginated( files_path, - expected_path=files_path, + expected_paths=frozenset(file_paths), + canonical_path=files_path, per_page=min(100, self.max_files + 1), retain_limit=self.max_files, budget=budget, @@ -784,9 +806,15 @@ def fetch_pull_request(self, url: str) -> PullRequestSnapshot: observed_file_overflow = file_result.items[self.max_files :] commits_path = f"{root}/pulls/{pr_number}/commits" + commit_paths = {commits_path} + if verified_repository_id is not None: + commit_paths.add( + f"/repositories/{verified_repository_id}/pulls/{pr_number}/commits" + ) commit_result = self._get_paginated( commits_path, - expected_path=commits_path, + expected_paths=frozenset(commit_paths), + canonical_path=commits_path, per_page=min(100, self.max_commits + 1), retain_limit=self.max_commits, budget=budget, diff --git a/tests/github/test_pagination_and_candidates.py b/tests/github/test_pagination_and_candidates.py index 02a4abb..5a4b0da 100644 --- a/tests/github/test_pagination_and_candidates.py +++ b/tests/github/test_pagination_and_candidates.py @@ -9,6 +9,7 @@ HEAD_SHA = "b" * 40 PR_URL = "https://github.com/acme/widget/pull/42" +REPOSITORY_ID = 12_345 def _pull_payload() -> dict: @@ -21,6 +22,7 @@ def _pull_payload() -> dict: "sha": "a" * 40, "repo": { "full_name": "acme/widget", + "id": REPOSITORY_ID, "private": False, "visibility": "public", }, @@ -57,9 +59,15 @@ def handler(request: httpx.Request) -> httpx.Response: page = int(request.url.params.get("page", "1")) if path == "/repos/acme/widget/pulls/42": return response(pull_payload or _pull_payload()) - if path == "/repos/acme/widget/pulls/42/files": + if path in { + "/repos/acme/widget/pulls/42/files", + f"/repositories/{REPOSITORY_ID}/pulls/42/files", + }: return response(file_pages.get(page, []), link=file_links.get(page)) - if path == "/repos/acme/widget/pulls/42/commits": + if path in { + "/repos/acme/widget/pulls/42/commits", + f"/repositories/{REPOSITORY_ID}/pulls/42/commits", + }: return response(commit_pages.get(page, []), link=commit_links.get(page)) if path == f"/repos/acme/widget/commits/{HEAD_SHA}/check-runs": return response({"check_runs": []}) @@ -186,6 +194,65 @@ def handler(request: httpx.Request) -> httpx.Response: assert all(request.url.host != "attacker.invalid" for request in requests) +@pytest.mark.parametrize("collection", ["files", "commits"]) +def test_pagination_accepts_github_numeric_repository_canonical_path( + collection: str, +) -> None: + item = ( + { + "filename": "src/second.py", + "status": "modified", + "patch": "@@ -1 +1 @@\n+second", + } + if collection == "files" + else { + "sha": "2" * 40, + "commit": {"message": "Second"}, + "html_url": "https://github.com/acme/widget/commit/2", + } + ) + next_link = ( + f"https://api.github.com/repositories/{REPOSITORY_ID}/pulls/42/" + f"{collection}?per_page=100&page=2" + ) + transport_kwargs = ( + { + "file_pages": {1: [], 2: [item]}, + "file_links": {1: f'<{next_link}>; rel="next"'}, + } + if collection == "files" + else { + "commit_pages": {1: [], 2: [item]}, + "commit_links": {1: f'<{next_link}>; rel="next"'}, + } + ) + transport, requests = pagination_transport(**transport_kwargs) + + snapshot = GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + observed = snapshot.files if collection == "files" else snapshot.commits + assert len(observed) == 1 + assert any( + request.url.path.startswith(f"/repositories/{REPOSITORY_ID}/") + for request in requests + ) + + +def test_pagination_rejects_numeric_canonical_path_for_another_repository() -> None: + escaped_link = ( + "https://api.github.com/repositories/999999/pulls/42/" + "files?per_page=100&page=2" + ) + transport, requests = pagination_transport( + file_links={1: f'<{escaped_link}>; rel="next"'}, + ) + + with pytest.raises(GitHubIngestionError, match="expected repository endpoint"): + GitHubClient(transport=transport).fetch_pull_request(PR_URL) + + assert len(requests) == 2 + + def test_pagination_validates_unfollowed_next_link_at_overflow_boundary() -> None: files = [ {