diff --git a/.github/workflows/slopsearx-pilot.yml b/.github/workflows/slopsearx-pilot.yml index 3aadd53f..7652ebf4 100644 --- a/.github/workflows/slopsearx-pilot.yml +++ b/.github/workflows/slopsearx-pilot.yml @@ -122,10 +122,10 @@ jobs: checks: read pull-requests: read actions: read - uses: groktopus/codereview/.github/workflows/pr-analysis.yml@537418ca9f542ba403b6bc6b8280eacb52d1ce0f + uses: groktopus/codereview/.github/workflows/pr-analysis.yml@23852759ed3cfb1e6964b7438141e293508aa178 with: harness_repository: groktopus/codereview - harness_sha: 537418ca9f542ba403b6bc6b8280eacb52d1ce0f + harness_sha: 23852759ed3cfb1e6964b7438141e293508aa178 target_repository: magnus919/SlopSearX pull_request_number: ${{ fromJSON(inputs.pull_request_number) }} base_ref: ${{ needs.resolve-target.outputs.base_ref }} diff --git a/docs/protected-publication-runtime.md b/docs/protected-publication-runtime.md index d841b2a5..19875967 100644 --- a/docs/protected-publication-runtime.md +++ b/docs/protected-publication-runtime.md @@ -8,9 +8,9 @@ When enabled by a protected configuration, the runtime verifies its clean checke The target is restricted to PRs whose base repository is the protected workflow’s repository and whose current base ref is the configured default branch. A fork head remains untrusted source data; it cannot select the publisher repository, credentials, or executable runtime. Before reading the artifact or App key, the runtime fetches the configured default branch from GitHub's repository API and binds the manifest's caller workflow SHA to that reported tip. It keeps the reviewed PR base/head SHAs separate: an older open PR may have a base commit behind the current default-branch tip. The App identity canary independently re-fetches the branch tip before reading the key. Before receipt persistence and again after receipt acknowledgement, the runtime rechecks that the tip is unchanged and that the PR still has the candidate base/head. These are fail-closed API observations; the source-context probe records `GITHUB_SHA`, `GITHUB_WORKFLOW_SHA`, the default-branch API tip, run API head, and PR base/head separately. Do not assume unobserved context equality or treat the probe as identity proof. The producer must independently bind its caller workflow SHA to the protected source revision. Manual dispatch and cross-repository publication remain outside this runtime. -The policy file is a versioned contract, not an input to the PR analysis. Schema v1 is App-only and remains unchanged. Schema v2 explicitly selects the App route or the target job's Actions token route and requires `authorization_mode: STANDING_BOUNDED`; this records narrow owner authorization for every result matching the exact policy envelope, without per-result human approval. Both templates remain disabled. The policy pins the repository and numeric ID, source and publisher workflow IDs/paths/refs, called harness commit, profile and provider-configuration identities, allowed disposition/event scope, credential route and expected actor, artifact redirect hosts, and upper bounds for run/page/review counts and wall-clock time. V2 also pins the exact publisher workflow file SHA-256; the runtime compares that digest with both the checked-out file and the file at `GITHUB_SHA` before accessing GitHub or the token. A target-local owner must review every identity and limit against authoritative repository configuration while keeping `enabled` false. The runtime also requires the workflow checkout to be clean and its HEAD to equal `GITHUB_SHA`; it rejects an uncommitted or mismatched policy file. +The policy file is a versioned contract, not an input to the PR analysis. Schema v1 is App-only and remains unchanged. Schema v2 explicitly selects the App route or the target job's Actions token route and requires `authorization_mode: STANDING_BOUNDED`; this records narrow owner authorization for every result matching the exact policy envelope, without per-result human approval. Schemas v1 and v2 retain their static artifact redirect host allowlist. Schema v3 keeps the bounded authorization and credential contract, pins the exact publisher workflow SHA-256, and replaces the host list with `artifact_download_mode: AUTHENTICATED_API_LOCATION_NO_AUTH`. In that mode, the runtime accepts only a well-formed HTTPS signed location obtained from the authenticated GitHub artifact API response for the already-bound artifact ID; the bearer credential is used only for the API request and never forwarded to the signed download. The downloader does not follow a second redirect. The location is used ephemerally and is not written to logs or receipts. V3 still validates the API artifact digest, bounded archive, manifest identity, and source-run bindings. The validator rejects malformed URLs and obvious local/private IP destinations; it does not perform DNS resolution or claim protection against DNS rebinding. All three policy templates remain disabled. The policy pins the repository and numeric ID, source and publisher workflow IDs/paths/refs, called harness commit, profile and provider-configuration identities, allowed disposition/event scope, credential route and expected actor, artifact-download mode, and upper bounds for run/page/review counts and wall-clock time. V2 and v3 pin the exact publisher workflow file SHA-256; the runtime compares that digest with both the checked-out file and the file at `GITHUB_SHA` before accessing GitHub or the token. A target-local owner must review every identity and limit against authoritative repository configuration while keeping `enabled` false. The runtime also requires the workflow checkout to be clean and its HEAD to equal `GITHUB_SHA`; it rejects an uncommitted or mismatched policy file. -For v2 `ACTIONS_TOKEN`, the runtime consumes only the protected workflow's fixed `${{ github.token }}` binding and validates the publisher workflow bytes against the policy's SHA-256 pin before any GitHub API request or token read. It also validates source/run/repository facts through bounded API admission. `PLATFORM_BOUND` means token provenance is bound to that exact protected workflow and those API facts; it does not mean the opaque token was independently inspected or that GitHub's effective write scope was observed. The v2 pre-POST receipt records the complete declared writer-job permission map and workflow digest with `permission_basis: WORKFLOW_DECLARATION` and `write_capability: NOT_TESTED`. The adapter's narrower `actions`/`pull-requests` tuple describes the operations it needs, not the full effective token permissions. Only the actual review response and later history readback showing the expected `github-actions[bot]` actor can establish that exact effect as confirmed. Actor mismatch or missing readback after submission leaves the result `UNKNOWN`; there is no App-key fallback or automatic retry. The existing App route remains `VERIFIED` through its separate App/installation APIs and token scope response. +For v2/v3 `ACTIONS_TOKEN`, the runtime consumes only the protected workflow's fixed `${{ github.token }}` binding and validates the publisher workflow bytes against the policy's SHA-256 pin before any GitHub API request or token read. It also validates source/run/repository facts through bounded API admission. `PLATFORM_BOUND` means token provenance is bound to that exact protected workflow and those API facts; it does not mean the opaque token was independently inspected or that GitHub's effective write scope was observed. The v2/v3 pre-POST receipt records the complete declared writer-job permission map and workflow digest with `permission_basis: WORKFLOW_DECLARATION` and `write_capability: NOT_TESTED`. The adapter's narrower `actions`/`pull-requests` tuple describes the operations it needs, not the full effective token permissions. Only the actual review response and later history readback showing the expected `github-actions[bot]` actor can establish that exact effect as confirmed. Actor mismatch or missing readback after submission leaves the result `UNKNOWN`; there is no App-key fallback or automatic retry. The existing App route remains `VERIFIED` through its separate App/installation APIs and token scope response. The writer adapter uses repository-wide concurrency (`pr-review-publish-`) so two PRs in the same repository cannot overlap receipt and review effects. The workflow template queues runs and does not cancel an in-progress publisher. The App route requires its named `code-review-publication` environment and App key secret; the Actions-token route uses no App key or installation and instead relies on the target workflow's per-job token. In either route, the YAML declaration does not prove configured protections or effective write capability. diff --git a/scripts/central_pilot_admission.py b/scripts/central_pilot_admission.py index 33819daa..e6a5b90b 100644 --- a/scripts/central_pilot_admission.py +++ b/scripts/central_pilot_admission.py @@ -37,7 +37,7 @@ TARGET = "magnus919/SlopSearX" BASE_REF = "main" REUSABLE_PATH = ".github/workflows/pr-analysis.yml" -REUSABLE_SHA = "537418ca9f542ba403b6bc6b8280eacb52d1ce0f" +REUSABLE_SHA = "23852759ed3cfb1e6964b7438141e293508aa178" MAX_EVENT_BYTES = 64 * 1024 SHA1 = re.compile(r"^[0-9a-f]{40}$") RUN_ID = re.compile(r"^[1-9][0-9]{0,19}$") diff --git a/src/pr_review_harness/actions_publication.py b/src/pr_review_harness/actions_publication.py index 5e8f315e..038fc254 100644 --- a/src/pr_review_harness/actions_publication.py +++ b/src/pr_review_harness/actions_publication.py @@ -16,6 +16,7 @@ import hashlib import io +import ipaddress import json import math import re @@ -105,6 +106,60 @@ class ArtifactTrustMode(StrEnum): SIGNED_ATTESTATION = "SIGNED_ATTESTATION" +class ArtifactDownloadMode(StrEnum): + """How a trusted GitHub artifact API redirect location is admitted.""" + + STATIC_HOST_ALLOWLIST = "STATIC_HOST_ALLOWLIST" + AUTHENTICATED_API_LOCATION_NO_AUTH = "AUTHENTICATED_API_LOCATION_NO_AUTH" + + +def validate_api_issued_artifact_location(location: str) -> bool: + """Validate URL syntax for an ephemeral location already received from GitHub's artifact API.""" + if not isinstance(location, str) or not location or len(location) > 16_384: + return False + try: + parsed = urllib.parse.urlsplit(location) + hostname = parsed.hostname + port = parsed.port + except ValueError: + return False + if ( + parsed.scheme.lower() != "https" + or not hostname + or parsed.username is not None + or parsed.password is not None + or parsed.fragment + or "#" in location + or not parsed.query + or port not in (None, 443) + or "\\" in location + or any(character.isspace() or ord(character) < 0x21 or ord(character) == 0x7F for character in location) + ): + return False + normalized_host = hostname.rstrip(".").lower() + if ( + not normalized_host + or normalized_host == "localhost" + or normalized_host.endswith((".localhost", ".local", ".internal")) + ): + return False + try: + address = ipaddress.ip_address(normalized_host) + except ValueError: + labels = normalized_host.split(".") + if ( + len(normalized_host) > 253 + or len(labels) < 2 + or all(re.fullmatch(r"(?:0x[0-9a-f]+|[0-9]+)", label) for label in labels) + ): + return False + return all( + re.fullmatch(r"[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?", label) is not None + for label in labels + ) + return address.is_global + + @dataclass(frozen=True) class VerifiedPublisherIdentity: """Writer identity bound by the trusted runtime adapter/canary.""" @@ -367,6 +422,7 @@ class GitHubPublicationPolicy: provider_configuration_identity: str allowed_actor_login: str artifact_trust_mode: ArtifactTrustMode + artifact_download_mode: ArtifactDownloadMode = ArtifactDownloadMode.STATIC_HOST_ALLOWLIST result_artifact_name: str = "pr-review-result" receipt_artifact_prefix: str = "pr-review-attempt" api_base_url: str = "https://api.github.com" @@ -426,6 +482,8 @@ def __post_init__(self) -> None: raise GitHubPublicationError("publication_actor_policy_invalid") if not isinstance(self.artifact_trust_mode, ArtifactTrustMode): raise GitHubPublicationError("artifact_trust_mode_invalid") + if not isinstance(self.artifact_download_mode, ArtifactDownloadMode): + raise GitHubPublicationError("artifact_download_mode_invalid") if not isinstance(self.concurrency_scope, str) or self.concurrency_scope not in {"pull_request", "repository"}: raise GitHubPublicationError("publication_concurrency_scope_invalid") if self.credential_provenance is not None: @@ -453,6 +511,14 @@ def __post_init__(self) -> None: for host in self.artifact_redirect_hosts ): raise GitHubPublicationError("artifact_redirect_host_invalid") + if self.artifact_download_mode is ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH: + if ( + parsed.hostname != "api.github.com" + or parsed.port not in (None, 443) + or parsed.path not in {"", "/"} + or self.artifact_redirect_hosts + ): + raise GitHubPublicationError("artifact_download_mode_invalid") def receipt_artifact_name(self, slot: EffectSlot) -> str: return f"{self.receipt_artifact_prefix}-{slot.pull_request_number}-{slot.head_sha}" @@ -844,17 +910,21 @@ def _download_artifact(self, artifact: dict, timeout_seconds: float, cap: int) - if response.status != 302: raise GitHubPublicationError("artifact_download_redirect_missing") redirect = response.headers.get("location") - parsed = urllib.parse.urlsplit(redirect or "") - allowed = {host.lower() for host in self.policy.artifact_redirect_hosts} - if ( - parsed.scheme != "https" - or not parsed.hostname - or parsed.hostname.lower() not in allowed - or parsed.username - or parsed.password - or not parsed.query - or parsed.fragment - ): + if self.policy.artifact_download_mode is ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH: + trusted_location = validate_api_issued_artifact_location(redirect) + else: + parsed = urllib.parse.urlsplit(redirect or "") + allowed = {host.lower() for host in self.policy.artifact_redirect_hosts} + trusted_location = ( + parsed.scheme == "https" + and bool(parsed.hostname) + and parsed.hostname.lower() in allowed + and not parsed.username + and not parsed.password + and bool(parsed.query) + and not parsed.fragment + ) + if not trusted_location: raise GitHubPublicationError("artifact_download_location_untrusted") remaining = self._deadline(timeout_seconds) if remaining <= 0: diff --git a/src/pr_review_harness/protected_publication_runtime.py b/src/pr_review_harness/protected_publication_runtime.py index efcfce12..27abb9d0 100644 --- a/src/pr_review_harness/protected_publication_runtime.py +++ b/src/pr_review_harness/protected_publication_runtime.py @@ -22,6 +22,7 @@ from urllib.parse import quote, urlsplit from .actions_publication import ( + ArtifactDownloadMode, ArtifactTrustMode, CredentialKind, GitHubActionsPublicationAdapter, @@ -31,6 +32,7 @@ IdentityState, UrllibGitHubTransport, VerifiedPublisherIdentity, + validate_api_issued_artifact_location, ) from .actions_runtime import OfficialActionsArtifactUploader from .artifact_intake import ( @@ -77,6 +79,7 @@ "artifact_redirect_hosts", } _V2_CONFIG_KEYS = (_CONFIG_KEYS - {"app"}) | {"credential", "authorization_mode"} +_V3_CONFIG_KEYS = (_V2_CONFIG_KEYS - {"artifact_redirect_hosts"}) | {"artifact_download_mode"} class ProtectedRuntimeError(ValueError): @@ -166,6 +169,23 @@ def parse(cls, value: dict[str, object]) -> "ProtectedPublicationPolicy": raise ProtectedRuntimeError("protected_policy_credential_invalid") else: raise ProtectedRuntimeError("protected_policy_credential_invalid") + elif schema == "pr-review-protected-publication-policy.v3": + if set(value) != _V3_CONFIG_KEYS or value.get("authorization_mode") != "STANDING_BOUNDED": + raise ProtectedRuntimeError("protected_policy_schema_invalid") + credential = value.get("credential") + if not isinstance(credential, dict): + raise ProtectedRuntimeError("protected_policy_credential_invalid") + if credential.get("kind") == "APP_INSTALLATION": + if set(credential) != {"kind", "app"}: + raise ProtectedRuntimeError("protected_policy_credential_invalid") + elif credential.get("kind") == "ACTIONS_TOKEN": + if ( + set(credential) != {"kind", "actor_login"} + or credential.get("actor_login") != "github-actions[bot]" + ): + raise ProtectedRuntimeError("protected_policy_credential_invalid") + else: + raise ProtectedRuntimeError("protected_policy_credential_invalid") else: raise ProtectedRuntimeError("protected_policy_schema_invalid") if not isinstance(value.get("enabled"), bool): @@ -184,7 +204,10 @@ def parse(cls, value: dict[str, object]) -> "ProtectedPublicationPolicy": workflow = value.get(key) expected_path = ".github/workflows/pr-publish.yml" if key == "publisher_workflow" else None expected_workflow_fields = {"id", "path", "ref"} - if schema == "pr-review-protected-publication-policy.v2" and key == "publisher_workflow": + if schema in { + "pr-review-protected-publication-policy.v2", + "pr-review-protected-publication-policy.v3", + } and key == "publisher_workflow": expected_workflow_fields.add("sha256") if not isinstance(workflow, dict) or set(workflow) != expected_workflow_fields: raise ProtectedRuntimeError("protected_policy_workflow_invalid") @@ -267,15 +290,19 @@ def parse(cls, value: dict[str, object]) -> "ProtectedPublicationPolicy": raise ProtectedRuntimeError("protected_policy_limits_invalid") if value.get("artifact_trust_mode") != "API_BOUND_SHA256": raise ProtectedRuntimeError("protected_policy_artifact_trust_invalid") - hosts = value.get("artifact_redirect_hosts") - if ( - not isinstance(hosts, list) - or not hosts - or len(hosts) > 10 - or any(not isinstance(host, str) or not re.fullmatch(r"[A-Za-z0-9.-]{1,253}", host) for host in hosts) - or len(set(hosts)) != len(hosts) - ): - raise ProtectedRuntimeError("protected_policy_artifact_hosts_invalid") + if schema == "pr-review-protected-publication-policy.v3": + if value.get("artifact_download_mode") != ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH.value: + raise ProtectedRuntimeError("protected_policy_artifact_download_mode_invalid") + else: + hosts = value.get("artifact_redirect_hosts") + if ( + not isinstance(hosts, list) + or not hosts + or len(hosts) > 10 + or any(not isinstance(host, str) or not re.fullmatch(r"[A-Za-z0-9.-]{1,253}", host) for host in hosts) + or len(set(hosts)) != len(hosts) + ): + raise ProtectedRuntimeError("protected_policy_artifact_hosts_invalid") return cls(raw=value) @property @@ -359,7 +386,10 @@ def _policy_from_workspace(environ: Mapping[str, str]) -> ProtectedPublicationPo policy = ProtectedPublicationPolicy.parse( _json_object(raw, limit=_POLICY_MAX_BYTES, code="protected_policy_invalid") ) - if policy.raw["schema"] == "pr-review-protected-publication-policy.v2": + if policy.raw["schema"] in { + "pr-review-protected-publication-policy.v2", + "pr-review-protected-publication-policy.v3", + }: workflow = policy.raw["publisher_workflow"] workflow_path = root / workflow["path"] try: @@ -562,7 +592,30 @@ def _publication_policy( concurrency_scope="repository", credential_provenance=credential_provenance, artifact_trust_mode=ArtifactTrustMode.API_BOUND_SHA256, - artifact_redirect_hosts=tuple(protected.raw["artifact_redirect_hosts"]), + artifact_download_mode=( + ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH + if protected.raw["schema"] == "pr-review-protected-publication-policy.v3" + else ArtifactDownloadMode.STATIC_HOST_ALLOWLIST + ), + artifact_redirect_hosts=tuple(protected.raw.get("artifact_redirect_hosts", ())), + ) + + +def _trusted_artifact_location(protected: ProtectedPublicationPolicy, location: object) -> bool: + if protected.raw["schema"] == "pr-review-protected-publication-policy.v3": + return validate_api_issued_artifact_location(location) + if not isinstance(location, str): + return False + try: + parsed = urlsplit(location) + except ValueError: + return False + return ( + parsed.scheme == "https" + and bool(parsed.hostname) + and parsed.hostname in protected.raw["artifact_redirect_hosts"] + and parsed.username is None + and parsed.password is None ) @@ -705,15 +758,7 @@ def request(method: str, url: str, *, expected_status: int = 200, body=None): raise ProtectedRuntimeError("source_artifact_metadata_invalid") redirect = request("GET", f"{root}/actions/artifacts/{artifact['id']}/zip", expected_status=302) location = redirect.headers.get("location") if isinstance(redirect.headers, dict) else None - parsed = urlsplit(location) if isinstance(location, str) else None - if ( - parsed is None - or parsed.scheme != "https" - or not parsed.hostname - or parsed.hostname not in protected.raw["artifact_redirect_hosts"] - or parsed.username is not None - or parsed.password is not None - ): + if not _trusted_artifact_location(protected, location): raise ProtectedRuntimeError("source_artifact_redirect_untrusted") remaining = deadline - clock() if remaining <= 0: diff --git a/templates/protected-publisher-policy-v3.json b/templates/protected-publisher-policy-v3.json new file mode 100644 index 00000000..69c84b11 --- /dev/null +++ b/templates/protected-publisher-policy-v3.json @@ -0,0 +1,50 @@ +{ + "schema": "pr-review-protected-publication-policy.v3", + "authorization_mode": "STANDING_BOUNDED", + "enabled": false, + "repository": { + "name": "REPLACE_WITH_OWNER_AND_REPOSITORY", + "id": 0 + }, + "default_branch": "REPLACE_WITH_PROTECTED_DEFAULT_BRANCH", + "publisher_workflow": { + "id": 0, + "path": ".github/workflows/pr-publish.yml", + "ref": "REPLACE_WITH_OWNER_AND_REPOSITORY/.github/workflows/pr-publish.yml@refs/heads/REPLACE_WITH_PROTECTED_DEFAULT_BRANCH", + "sha256": "REPLACE_WITH_REVIEWED_PUBLISHER_WORKFLOW_SHA256" + }, + "source_workflow": { + "id": 0, + "path": ".github/workflows/REPLACE_WITH_PROTECTED_ANALYSIS_WORKFLOW.yml", + "ref": "REPLACE_WITH_OWNER_AND_REPOSITORY/.github/workflows/REPLACE_WITH_PROTECTED_ANALYSIS_WORKFLOW.yml@refs/heads/REPLACE_WITH_PROTECTED_DEFAULT_BRANCH" + }, + "called_harness": { + "repository": "REPLACE_WITH_OWNER_AND_HARNESS_REPOSITORY", + "path": ".github/workflows/pr-analysis.yml", + "sha": "REPLACE_WITH_VERIFIED_40_CHARACTER_HARNESS_COMMIT_SHA" + }, + "credential": { + "kind": "ACTIONS_TOKEN", + "actor_login": "github-actions[bot]" + }, + "profile": { + "version": "REPLACE_WITH_REVIEW_PROFILE_VERSION", + "sha256": "REPLACE_WITH_64_CHARACTER_PROFILE_SHA256", + "provider_configuration_identity": "provider-identity-sha256:REPLACE_WITH_64_CHARACTER_PROVIDER_IDENTITY_SHA256" + }, + "publication": { + "allowed_dispositions": [ + "COMMENT" + ], + "max_review_body_bytes": 60000, + "effect_policy": "PUBLISH_REVIEW" + }, + "limits": { + "max_runs": 20, + "max_pages": 2, + "max_reviews": 100, + "deadline_seconds": 25 + }, + "artifact_trust_mode": "API_BOUND_SHA256", + "artifact_download_mode": "AUTHENTICATED_API_LOCATION_NO_AUTH" +} diff --git a/tests/test_actions_publication.py b/tests/test_actions_publication.py index 4e9385e0..b12eac12 100644 --- a/tests/test_actions_publication.py +++ b/tests/test_actions_publication.py @@ -7,6 +7,7 @@ import pytest from pr_review_harness.actions_publication import ( + ArtifactDownloadMode, ArtifactTrustMode, ArtifactUploadResult, CredentialKind, @@ -16,7 +17,9 @@ HTTPResponse, IdentityState, PublisherCredential, + UrllibGitHubTransport, VerifiedPublisherIdentity, + validate_api_issued_artifact_location, ) from pr_review_harness.artifact_intake import MANIFEST_FILENAME, RESULT_FILENAME from pr_review_harness.github_actions_credentials import ( @@ -427,6 +430,126 @@ def responder(method, url, body): assert all(call[2] != "test-secret-token" for call in transport.download_calls) +@pytest.mark.parametrize( + "location", + [ + "http://productionresultssa99.blob.core.windows.net/file?sig=x", + "https://user:pass@storage.example.test/file?sig=x", + "https://storage.example.test/file", + "https://storage.example.test/file?sig=x#fragment", + "https://storage.example.test/file?sig=x#", + "https://storage.example.test:444/file?sig=x", + "https://localhost/file?sig=x", + "https://private.local/file?sig=x", + "https://127.0.0.1/file?sig=x", + "https://127.1/file?sig=x", + "https://2130706433/file?sig=x", + "https://0x7f000001/file?sig=x", + "https://[::1]/file?sig=x", + "https://storage.example.test/file?sig=x\n", + ], +) +def test_authenticated_api_location_validator_rejects_unsafe_redirects(location): + assert not validate_api_issued_artifact_location(location) + + +def test_authenticated_api_location_validator_accepts_unlisted_public_signed_location(): + assert validate_api_issued_artifact_location( + "https://productionresultssa99.blob.core.windows.net/container/file?sig=opaque" + ) + assert not validate_api_issued_artifact_location("https://" + "x" * 16_385 + "?sig=x") + + +@pytest.mark.parametrize( + ("download_response", "expected_error"), + [ + (HTTPResponse(302, {"location": "https://second.example.test/file?sig=two"}, b""), "artifact_download_failed"), + (HTTPResponse(200, {}, b"wrong archive"), "artifact_transport_digest_mismatch"), + ], +) +def test_authenticated_api_location_mode_downloads_without_host_allowlist_or_redirect_following( + download_response, expected_error +): + archive = b"archive bytes that pass minimum" + transport = FakeTransport( + lambda *_: HTTPResponse( + 302, + {"location": "https://productionresultssa99.blob.core.windows.net/file?sig=one"}, + b"", + ) + ) + def download(url, *, timeout_seconds, max_response_bytes): + transport.download_calls.append((url, timeout_seconds, max_response_bytes)) + return download_response + + transport.download = download + policy = make_policy( + artifact_download_mode=ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH, + artifact_redirect_hosts=(), + ) + client = GitHubActionsPublicationAdapter(policy, Provider(), transport=transport) + artifact = { + "id": 456, + "name": "review-bundle", + "size_in_bytes": len(archive), + "digest": "sha256:" + hashlib.sha256(archive).hexdigest(), + "expired": False, + } + with pytest.raises(GitHubPublicationError, match=expected_error): + client._download_artifact(artifact, 4, 1024) + assert len(transport.download_calls) == 1 + assert transport.calls[0][2] == "test-secret-token" + assert len(transport.download_calls[0]) == 3 + + +def test_authenticated_api_location_mode_accepts_public_unlisted_host_and_keeps_download_tokenless(): + archive = b"archive bytes that pass minimum" + transport = FakeTransport( + lambda *_: HTTPResponse( + 302, + {"location": "https://productionresultssa99.blob.core.windows.net/file?sig=one"}, + b"", + ) + ) + def download(url, *, timeout_seconds, max_response_bytes): + transport.download_calls.append((url, timeout_seconds, max_response_bytes)) + return HTTPResponse(200, {}, archive) + + transport.download = download + policy = make_policy( + artifact_download_mode=ArtifactDownloadMode.AUTHENTICATED_API_LOCATION_NO_AUTH, + artifact_redirect_hosts=(), + ) + client = GitHubActionsPublicationAdapter(policy, Provider(), transport=transport) + artifact = { + "id": 456, + "name": "review-bundle", + "size_in_bytes": len(archive), + "digest": "sha256:" + hashlib.sha256(archive).hexdigest(), + "expired": False, + } + assert client._download_artifact(artifact, 4, 1024).archive == archive + assert transport.download_calls[0][0].startswith("https://productionresultssa99.") + + +def test_urllib_signed_artifact_download_request_has_no_bearer_header(monkeypatch): + transport = UrllibGitHubTransport() + captured = {} + + def open_request(request, timeout_seconds, max_response_bytes): + captured.update({key.lower(): value for key, value in request.header_items()}) + return HTTPResponse(200, {}, b"archive") + + monkeypatch.setattr(transport, "_open", open_request) + result = transport.download( + "https://productionresultssa99.blob.core.windows.net/file?sig=opaque", + timeout_seconds=3, + max_response_bytes=1024, + ) + assert result.status == 200 + assert "authorization" not in captured + + def make_valid_result(): value = { "run_id": "review-run-2026-09-26", diff --git a/tests/test_pilot_workflow.py b/tests/test_pilot_workflow.py index d7bb7056..cc3cd8ce 100644 --- a/tests/test_pilot_workflow.py +++ b/tests/test_pilot_workflow.py @@ -9,7 +9,7 @@ import pytest WORKFLOW = Path(__file__).parents[1] / ".github/workflows/slopsearx-pilot.yml" -PIN = "537418ca9f542ba403b6bc6b8280eacb52d1ce0f" +PIN = "23852759ed3cfb1e6964b7438141e293508aa178" SECRETS = ( "LLM_BASE_URL", "LLM_MODEL", diff --git a/tests/test_protected_actions_token_template.py b/tests/test_protected_actions_token_template.py index 2e9507f9..0754989c 100644 --- a/tests/test_protected_actions_token_template.py +++ b/tests/test_protected_actions_token_template.py @@ -55,3 +55,34 @@ def test_actions_token_templates_are_bounded_inert_and_match_policy(): runtime_step = next(step for step in steps if step.get("name") == "Run the protected publication runtime") assert runtime_step["env"]["GITHUB_TOKEN"] == "${{ github.token }}" assert "PR_REVIEW_GITHUB_APP_PRIVATE_KEY" not in runtime_step["env"] + + +def test_v3_template_selects_authenticated_api_location_mode_without_host_pool(): + policy = json.loads((ROOT / "templates/protected-publisher-policy-v3.json").read_text(encoding="utf-8")) + assert policy["schema"] == "pr-review-protected-publication-policy.v3" + assert policy["enabled"] is False + assert policy["artifact_download_mode"] == "AUTHENTICATED_API_LOCATION_NO_AUTH" + assert "artifact_redirect_hosts" not in policy + + policy.update( + repository={"name": "owner/repo", "id": 8123}, + default_branch="main", + publisher_workflow={ + "id": 1, + "path": ".github/workflows/pr-publish.yml", + "ref": "owner/repo/.github/workflows/pr-publish.yml@refs/heads/main", + "sha256": "d" * 64, + }, + source_workflow={ + "id": 2, + "path": ".github/workflows/pr-analysis.yml", + "ref": "owner/repo/.github/workflows/pr-analysis.yml@refs/heads/main", + }, + called_harness={"repository": "harness/repo", "path": ".github/workflows/pr-analysis.yml", "sha": "a" * 40}, + profile={ + "version": "v1", + "sha256": "b" * 64, + "provider_configuration_identity": "provider-identity-sha256:" + "c" * 64, + }, + ) + assert not ProtectedPublicationPolicy.parse(policy).enabled diff --git a/tests/test_protected_publication_runtime.py b/tests/test_protected_publication_runtime.py index bcbe9f78..2be15124 100644 --- a/tests/test_protected_publication_runtime.py +++ b/tests/test_protected_publication_runtime.py @@ -74,6 +74,14 @@ def actions_token_policy_document(*, enabled=False): return value +def api_location_policy_document(*, enabled=False): + value = actions_token_policy_document(enabled=enabled) + value["schema"] = "pr-review-protected-publication-policy.v3" + value.pop("artifact_redirect_hosts") + value["artifact_download_mode"] = "AUTHENTICATED_API_LOCATION_NO_AUTH" + return value + + def protected_checkout(tmp_path, *, enabled=True, policy_value=None): workspace = tmp_path / "protected" (workspace / ".github/workflows").mkdir(parents=True) @@ -295,6 +303,35 @@ def test_actions_token_v2_policy_is_strict_and_keeps_standing_authorization_expl runtime.ProtectedPublicationPolicy.parse(unspecified) +def test_v3_policy_selects_closed_api_location_mode_and_preserves_v2_static_hosts(): + v2 = runtime.ProtectedPublicationPolicy.parse(actions_token_policy_document()) + v3_value = api_location_policy_document() + v3 = runtime.ProtectedPublicationPolicy.parse(v3_value) + assert v2.raw["artifact_redirect_hosts"] == ["downloads.example.test"] + assert v3.raw["artifact_download_mode"] == "AUTHENTICATED_API_LOCATION_NO_AUTH" + assert runtime._trusted_artifact_location( + v3, "https://productionresultssa99.blob.core.windows.net/file?sig=opaque" + ) + assert runtime._trusted_artifact_location(v3, "https://unlisted.example.test/file?sig=opaque") + assert not runtime._trusted_artifact_location(v3, "https://127.1/file?sig=opaque") + assert runtime._trusted_artifact_location( + v2, "https://downloads.example.test/file?sig=opaque" + ) + assert not runtime._trusted_artifact_location( + v2, "https://productionresultssa99.blob.core.windows.net/file?sig=opaque" + ) + + for invalid in ("STATIC_HOST_ALLOWLIST", "AUTHENTICATED_API_LOCATION_NO_AUTH_EXTRA", None): + malformed = api_location_policy_document() + malformed["artifact_download_mode"] = invalid + with pytest.raises(runtime.ProtectedRuntimeError, match="protected_policy_artifact_download_mode_invalid"): + runtime.ProtectedPublicationPolicy.parse(malformed) + with_hosts = api_location_policy_document() + with_hosts["artifact_redirect_hosts"] = ["downloads.example.test"] + with pytest.raises(runtime.ProtectedRuntimeError, match="protected_policy_schema_invalid"): + runtime.ProtectedPublicationPolicy.parse(with_hosts) + + @pytest.mark.parametrize("schema", ["v1", "v2"]) @pytest.mark.parametrize("app_value", [None, "missing"]) def test_app_routes_require_a_real_app_policy_object(schema, app_value): @@ -405,14 +442,18 @@ def request(self, *_args, **_kwargs): for association in ("empty", "omitted", "row-with-older-pr-base") ] + [("success", association, "ACTIONS_TOKEN") for association in ("empty", "omitted", "row-with-older-pr-base")] + + [("success", "empty", "ACTIONS_TOKEN_V3")] + [("post_forbidden", "empty", "ACTIONS_TOKEN")], ) def test_runtime_composes_api_bound_intake_receipt_ack_fresh_head_and_single_post( tmp_path, monkeypatch, failure_mode, source_association, credential_route ): + actions_token_route = credential_route.startswith("ACTIONS_TOKEN") policy_value = ( - actions_token_policy_document(enabled=True) - if credential_route == "ACTIONS_TOKEN" + api_location_policy_document(enabled=True) + if credential_route == "ACTIONS_TOKEN_V3" + else actions_token_policy_document(enabled=True) + if actions_token_route else policy_document(enabled=True) ) workspace, revision, event_path = protected_checkout(tmp_path, policy_value=policy_value) @@ -555,15 +596,20 @@ def api_response(method, url, body): return response({"total_count": 0, "workflow_runs": []}) if path.endswith("/pulls/44/reviews") and method == "GET": return response(stored_reviews) + artifact_host = ( + "productionresultssa99.blob.core.windows.net" + if credential_route == "ACTIONS_TOKEN_V3" + else "downloads.example.test" + ) if path.endswith("/actions/artifacts/123/zip"): - return HTTPResponse(302, {"location": "https://downloads.example.test/result?signature=one-use"}, b"") + return HTTPResponse(302, {"location": f"https://{artifact_host}/result?signature=one-use"}, b"") if path.endswith("/actions/artifacts/991/zip"): - return HTTPResponse(302, {"location": "https://downloads.example.test/receipt?signature=one-use"}, b"") + return HTTPResponse(302, {"location": f"https://{artifact_host}/receipt?signature=one-use"}, b"") if path.endswith("/pulls/44/reviews") and method == "POST": post_bodies.append(body) if failure_mode == "post_forbidden": return response({"message": "permission denied"}, status=403) - post_actor = "github-actions[bot]" if credential_route == "ACTIONS_TOKEN" else WRITER + post_actor = "github-actions[bot]" if actions_token_route else WRITER return response({"id": 777, "commit_id": HEAD, "state": "COMMENTED", "body": body["body"], "user": {"login": post_actor}}) raise AssertionError(f"unexpected fake API request: {method} {url}") @@ -622,7 +668,7 @@ def admit(self, result, config, limits): assert outcome["schema"] == "pr-review-protected-publication.v1" assert outcome["status"] == "CONFIRMED", (outcome, events, admitted) assert outcome["reason"] is None - if credential_route == "ACTIONS_TOKEN": + if actions_token_route: assert outcome["effect_observation"] == { "actor_login": "github-actions[bot]", "review_state": "COMMENTED", @@ -693,7 +739,7 @@ def admit(self, result, config, limits): assert any("/actions/artifacts/991/zip" in call[1] for call in transport.calls) assert pull_reads >= 3 assert all(actual_key not in repr(call) for call in transport.calls) - if credential_route == "ACTIONS_TOKEN": + if actions_token_route: assert all(call[2] == "test-actions-read-token" for call in transport.calls) assert all("/app" not in call[1] for call in transport.calls) else: @@ -703,7 +749,7 @@ def admit(self, result, config, limits): else: assert post_bodies == [] assert events.count("review_post") == 0 - assert len(key_calls) == (0 if credential_route == "ACTIONS_TOKEN" else ( + assert len(key_calls) == (0 if actions_token_route else ( 0 if failure_mode in {"caller_workflow_sha_mismatch", "provider_identity_mismatch", "duplicate_source_artifact", "stale_base", "stale_head", "deadline"} else 1 @@ -721,7 +767,7 @@ def admit(self, result, config, limits): assert events.count("receipt_upload") == 0 if failure_mode == "receipt_ack_missing": assert events.count("receipt_upload") == 1 - if credential_route == "ACTIONS_TOKEN": + if actions_token_route: assert key_calls == [] assert events.count("write_token_mint") == 0 assert all(call[2] == "test-actions-read-token" for call in transport.calls)