From 6c644d727ef9afe9cc23da83d21cccad01cae0d4 Mon Sep 17 00:00:00 2001 From: jenkins Date: Sun, 16 Aug 2026 20:18:49 -0300 Subject: [PATCH] hermes: harden Forgejo client boundary --- services/hermes/agent-configmap.yaml | 33 +- services/hermes/scripts/gitea_api.py | 381 +++++++++------- .../manage-atlas-pull-requests/SKILL.md | 48 +- testing/tests/test_hermes_chat_quality.py | 8 +- testing/tests/test_hermes_gitea_pr_client.py | 417 +++++++++++------- .../tests/test_hermes_gitea_pr_integration.py | 147 ++++++ 6 files changed, 671 insertions(+), 363 deletions(-) create mode 100644 testing/tests/test_hermes_gitea_pr_integration.py diff --git a/services/hermes/agent-configmap.yaml b/services/hermes/agent-configmap.yaml index b35da59a..782876cf 100644 --- a/services/hermes/agent-configmap.yaml +++ b/services/hermes/agent-configmap.yaml @@ -318,15 +318,15 @@ data: `scm.bstein.dev` is Forgejo/Gitea, not GitHub. Never load or follow a GitHub/`gh` skill for an Atlas remote, and do not interpret an unauthenticated Gitea HTTP 404 as a missing private repository. Use - authenticated Git for clone, fetch, and non-force push. For repository and - pull-request reads, or to create/update a review-ready draft PR, load + authenticated Git for clone, fetch, and non-force push. For bounded + repository/pull-request evidence or to create a review-ready draft PR, load `$manage-atlas-pull-requests` and use `/opt/coordinator/gitea_api.py`. The client injects the runtime Vault token without exposing it to the command line, environment, output, or transcript. It permits only Atlas-scoped - reads, same-repository draft creation, and title/body updates while the PR - remains draft. Merge, approve, close, delete, comments, repository - administration, and force-push are unavailable; never bypass the client - with raw HTTP. Leave every PR unmerged for Brad's review. Independently + metadata reads and verified same-repository draft creation. Updates, merge, + approve, close, delete, comments, repository administration, and force-push + are unavailable; never bypass the client with raw HTTP. Leave every PR + unmerged for Brad's review. Independently verify the exact base/head ancestry and diff, run the relevant tests, and confirm the candidate Jenkins result before presenting a handoff. Prefer the internal endpoint in @@ -337,25 +337,24 @@ data: [--commit SHA] --wait`; it distinguishes a genuinely terminal build from nested Jenkins execution metadata and returns bounded JSON/log evidence. With a branch, the helper also tries the conventional `JOB-branches` - multibranch name. After Brad merges, observe the default-branch build to a - terminal result and report exact commit, build, and test evidence. + multibranch name. If Brad separately reports that he merged the PR, observe + the default-branch build to a terminal result and report exact commit, + build, and test evidence. The workspace already has a non-secret Hermes Git author identity. Do not use `git reset --hard`, even in a fresh clone; use a detached worktree or a clean branch switch when comparing revisions, and preserve any unexpected - file as user state. Use the Gitea pull-request merge endpoint for an open - PR so both Git history and PR state remain auditable. If an authorized - manual merge already reached the base branch, reconcile the open PR with - `Do=manually-merged` and its exact merge commit instead of leaving stale - review state. + file as user state. PR publication ends at the verified open draft; Brad + owns review and merge authority. The terminal PATH contains the pinned operator tools. Start cluster work with `kubectl config current-context`, read-only status/events/logs, and the relevant `titan-iac` manifests. Put durable desired-state changes on a - reviewable `titan-iac` branch, validate Kustomize and client dry-run, then - use Flux reconciliation after the tracked change is published. Direct - `kubectl` mutations are for explicit incident recovery or ephemeral - verification, not ordinary delivery. + reviewable `titan-iac` branch and validate Kustomize and client dry-run. + Brad owns merge and routine Flux reconciliation; perform either only under + a separate explicit operator request. Direct `kubectl` mutations are for + explicit incident recovery or ephemeral verification, not ordinary + delivery. Node SSH uses a dedicated audited Hermes identity: `ssh titan-04` (or any current Kubernetes node name). Host keys are pinned and password fallback diff --git a/services/hermes/scripts/gitea_api.py b/services/hermes/scripts/gitea_api.py index 73a2bffd..057a79a1 100755 --- a/services/hermes/scripts/gitea_api.py +++ b/services/hermes/scripts/gitea_api.py @@ -7,6 +7,7 @@ import argparse import json import os import re +import subprocess import sys import urllib.error import urllib.parse @@ -18,18 +19,41 @@ CANONICAL_BASE_URL = "https://scm.bstein.dev" ALLOWED_OWNER = "atlas" DEFAULT_TOKEN_FILE = Path("/runtime-access/gitea-token") REPO_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]{0,99}\Z") -PR_PATH_RE = re.compile(r"/api/v1/repos/atlas/([^/]+)/pulls/([1-9][0-9]*)\Z") +SHA_RE = re.compile(r"[0-9a-fA-F]{40}\Z") CREATE_PATH_RE = re.compile(r"/api/v1/repos/atlas/([^/]+)/pulls\Z") -READ_PATH_RE = re.compile(r"/api/v1/repos/atlas/([^/]+)(?:/.*)?\Z") -SAFE_REF_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._/-]{0,199}\Z") MAX_ERROR_BYTES = 8192 +MAX_RESPONSE_BYTES = 2 * 1024 * 1024 DRAFT_TITLE_PREFIX = "WIP: " +GIT_BIN = "/usr/bin/git" +SENSITIVE_BODY_PATTERNS = ( + re.compile(r"-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----"), + re.compile(r"(?i)\b(?:password|passwd|secret|token|api[_-]?key)\s*[:=]"), + re.compile(r"(?i)\bauthorization\s*:\s*(?:bearer|token|basic)\s+\S+"), + re.compile(r"\b(?:gh[pousr]_[A-Za-z0-9]{20,}|sk-[A-Za-z0-9]{20,})\b"), + re.compile(r"\b(?:AKIA[0-9A-Z]{16}|AIza[0-9A-Za-z_-]{35})\b"), + re.compile(r"\beyJ[A-Za-z0-9_-]{12,}\.[A-Za-z0-9_-]{12,}\.[A-Za-z0-9_-]{8,}\b"), +) class PolicyError(ValueError): """Raised when a requested Forgejo operation exceeds the safe boundary.""" +class RejectRedirectHandler(urllib.request.HTTPRedirectHandler): + """Reject every redirect before urllib can copy authentication headers.""" + + def redirect_request(self, req, fp, code, msg, headers, newurl): + raise PolicyError("Forgejo redirects are not allowed") + + +_SAFE_OPENER = urllib.request.build_opener(RejectRedirectHandler()) + + +def _safe_urlopen(request: urllib.request.Request, timeout: int): + """Open one request using a handler that never follows redirects.""" + return _SAFE_OPENER.open(request, timeout=timeout) + + def read_token(path: Path = DEFAULT_TOKEN_FILE) -> str: """Read the token from the pod-lifetime Vault projection.""" token = path.read_text(encoding="utf-8").strip() @@ -53,19 +77,29 @@ def _validate_repo(repo: str) -> str: def _validate_ref(value: object, name: str) -> str: - if not isinstance(value, str) or not SAFE_REF_RE.fullmatch(value): + """Validate the complete Git ref grammar using Git itself.""" + if not isinstance(value, str) or not value or value.startswith("-"): raise PolicyError(f"{name} must be a same-repository branch name") - if ( - value.startswith(".") - or value.endswith(("/", ".", ".lock")) - or "//" in value - or ".." in value - or "@{" in value - ): + if any(ord(character) < 32 or ord(character) == 127 for character in value): + raise PolicyError(f"{name} must be a safe same-repository branch name") + result = subprocess.run( + [GIT_BIN, "check-ref-format", f"refs/heads/{value}"], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + if result.returncode != 0: raise PolicyError(f"{name} must be a safe same-repository branch name") return value +def _validate_sha(value: object, name: str = "head SHA") -> str: + if not isinstance(value, str) or not SHA_RE.fullmatch(value): + raise PolicyError(f"{name} must be a full Git SHA-1") + return value.lower() + + def _validate_text(value: object, name: str, maximum: int, *, required: bool) -> str: if not isinstance(value, str): raise PolicyError(f"{name} must be text") @@ -76,6 +110,15 @@ def _validate_text(value: object, name: str, maximum: int, *, required: bool) -> return value +def _validate_body(value: object, *, forbidden: tuple[str, ...] = ()) -> str: + body = _validate_text(value, "body", 16384, required=False) + if any(secret and secret in body for secret in forbidden): + raise PolicyError("pull-request body contains runtime credential material") + if any(pattern.search(body) for pattern in SENSITIVE_BODY_PATTERNS): + raise PolicyError("pull-request body resembles credential material") + return body + + def _draft_title(value: object) -> str: """Return a bounded title using Gitea's configured default draft prefix.""" title = _validate_text(value, "title", 251, required=True).strip() @@ -99,6 +142,73 @@ def _split_api_path(path: str) -> urllib.parse.SplitResult: return target +def _validate_query(target: urllib.parse.SplitResult, allowed: set[str]) -> None: + try: + pairs = urllib.parse.parse_qsl( + target.query, keep_blank_values=True, strict_parsing=True + ) + except ValueError as exc: + raise PolicyError("invalid API query") from exc + if len({key for key, _ in pairs}) != len(pairs): + raise PolicyError("duplicate API query parameters are not allowed") + if any(key not in allowed for key, _ in pairs): + raise PolicyError("API query parameter is outside the read allowlist") + values = dict(pairs) + for name in ("page", "limit"): + if name not in values: + continue + if not values[name].isdigit() or int(values[name]) < 1: + raise PolicyError(f"{name} must be a positive integer") + if "limit" in values and int(values["limit"]) > 50: + raise PolicyError("read limit cannot exceed 50") + if "state" in values and values["state"] not in {"open", "closed", "all"}: + raise PolicyError("pull-request state is invalid") + + +def _authorize_read(target: urllib.parse.SplitResult) -> str: + """Allow only repository, PR, branch, commit, and status metadata reads.""" + prefix = "/api/v1/repos/atlas/" + remainder = target.path.removeprefix(prefix) + if remainder == target.path: + raise PolicyError("reads are limited to explicit Atlas repository metadata") + repo, separator, suffix = remainder.partition("/") + _validate_repo(repo) + + if not separator: + _validate_query(target, set()) + return "repository" + if suffix == "pulls": + _validate_query(target, {"page", "limit", "state"}) + return "pull-list" + if re.fullmatch(r"pulls/[1-9][0-9]*", suffix): + _validate_query(target, set()) + return "pull" + if re.fullmatch(r"pulls/[1-9][0-9]*/(?:commits|files)", suffix): + _validate_query(target, {"page", "limit"}) + return "pull-evidence" + if suffix == "branches": + _validate_query(target, {"page", "limit"}) + return "branch-list" + branch_match = re.fullmatch(r"branches/([^/]+)", suffix) + if branch_match: + _validate_ref(branch_match.group(1), "branch") + _validate_query(target, set()) + return "branch" + if suffix == "commits": + _validate_query(target, {"page", "limit"}) + return "commit-list" + if re.fullmatch(r"git/commits/[0-9a-fA-F]{40}", suffix): + _validate_query(target, set()) + return "commit" + if re.fullmatch( + r"(?:commits/[0-9a-fA-F]{40}/status|commits/[0-9a-fA-F]{40}/statuses|statuses/[0-9a-fA-F]{40})", + suffix, + ): + _validate_query(target, {"page", "limit"}) + return "status" + raise PolicyError("repository API route is outside the metadata read allowlist") + + def api_url(base_url: str, path: str) -> str: """Return a canonical same-origin URL without forwarding credentials.""" if base_url.rstrip("/") != CANONICAL_BASE_URL: @@ -116,44 +226,23 @@ def authorize_request(method: str, path: str, data: object | None) -> str: if normalized_method == "GET": if data is not None: raise PolicyError("read operations cannot include a request body") - match = READ_PATH_RE.fullmatch(target.path) - if not match: - raise PolicyError("reads are limited to repositories owned by atlas") - _validate_repo(match.group(1)) - return "read" - + return _authorize_read(target) if target.query: raise PolicyError("mutating operations cannot include query parameters") - if normalized_method == "POST": - match = CREATE_PATH_RE.fullmatch(target.path) - if not match: - raise PolicyError("POST is limited to creating an Atlas draft pull request") - _validate_repo(match.group(1)) - if not isinstance(data, dict) or set(data) != {"base", "body", "head", "title"}: - raise PolicyError("draft creation accepts only base, body, head, and title") - _validate_ref(data["base"], "base") - _validate_ref(data["head"], "head") - if data["title"] != _draft_title(data["title"]): - raise PolicyError("new pull requests must use the Gitea draft-title prefix") - _validate_text(data["body"], "body", 65536, required=False) - return "create-draft" - - if normalized_method == "PATCH": - match = PR_PATH_RE.fullmatch(target.path) - if not match: - raise PolicyError("PATCH is limited to Atlas draft pull-request metadata") - _validate_repo(match.group(1)) - if not isinstance(data, dict) or not data or not set(data) <= {"body", "title"}: - raise PolicyError("draft updates accept only title and body") - if "title" in data and data["title"] != _draft_title(data["title"]): - raise PolicyError("updated pull-request titles must preserve draft state") - if "body" in data: - _validate_text(data["body"], "body", 65536, required=False) - return "update-draft" - - raise PolicyError( - "method is not available: merge, approve, close, delete, and force-push are outside this client" - ) + if normalized_method != "POST": + raise PolicyError("only read and create-draft operations are available") + match = CREATE_PATH_RE.fullmatch(target.path) + if not match: + raise PolicyError("POST is limited to creating an Atlas draft pull request") + _validate_repo(match.group(1)) + if not isinstance(data, dict) or set(data) != {"base", "body", "head", "title"}: + raise PolicyError("draft creation accepts only base, body, head, and title") + _validate_ref(data["base"], "base") + _validate_ref(data["head"], "head") + if data["title"] != _draft_title(data["title"]): + raise PolicyError("new pull requests must use the Gitea draft-title prefix") + _validate_body(data["body"]) + return "create-draft" def build_request( @@ -177,7 +266,7 @@ def build_request( "Accept": "application/json", "Authorization": f"token {token}", "Content-Type": "application/json", - "User-Agent": "hermes-atlas-pr-client/1", + "User-Agent": "hermes-atlas-pr-client/2", }, ) @@ -198,102 +287,116 @@ def _request( data: object | None, *, token: str, - opener: Callable[..., object] = urllib.request.urlopen, + opener: Callable[..., object] = _safe_urlopen, ) -> bytes: request = build_request( - method, - path, - base_url=configured_base_url(), - token=token, - data=data, + method, path, base_url=configured_base_url(), token=token, data=data ) with opener(request, timeout=30) as response: # type: ignore[attr-defined] - return redact_bytes(response.read(), token) # type: ignore[attr-defined] + body = response.read(MAX_RESPONSE_BYTES + 1) # type: ignore[attr-defined] + if len(body) > MAX_RESPONSE_BYTES: + raise PolicyError("Forgejo response exceeds the safe size limit") + return redact_bytes(body, token) def read( - path: str, *, token: str, opener: Callable[..., object] = urllib.request.urlopen + path: str, *, token: str, opener: Callable[..., object] = _safe_urlopen ) -> bytes: - """Read one Atlas repository API resource.""" + """Read one explicitly allowed Atlas repository metadata resource.""" return _request("GET", path, None, token=token, opener=opener) +def _nested(document: dict[str, object], *keys: str) -> object: + value: object = document + for key in keys: + if not isinstance(value, dict) or key not in value: + raise PolicyError("Forgejo omitted required pull-request evidence") + value = value[key] + return value + + +def _require_create_response( + value: bytes, + *, + repo: str, + base: str, + head: str, + head_sha: str, + title: str, + body: str, +) -> dict[str, object]: + """Require exact server evidence for the intended open draft handoff.""" + try: + document = json.loads(value) + except json.JSONDecodeError as exc: + raise PolicyError("Forgejo returned invalid pull-request metadata") from exc + if not isinstance(document, dict): + raise PolicyError("Forgejo returned invalid pull-request metadata") + number = document.get("number") + if not isinstance(number, int) or isinstance(number, bool) or number < 1: + raise PolicyError("Forgejo omitted a valid pull-request number") + full_name = f"{ALLOWED_OWNER}/{repo}" + expected_html = f"{CANONICAL_BASE_URL}/{full_name}/pulls/{number}" + expected_api = f"{CANONICAL_BASE_URL}/api/v1/repos/{full_name}/pulls/{number}" + checks = ( + document.get("state") == "open", + document.get("draft") is True, + document.get("merged") is False, + document.get("html_url") == expected_html, + document.get("url") == expected_api, + document.get("title") == title, + document.get("body") == body, + _nested(document, "base", "ref") == base, + _nested(document, "base", "repo", "full_name") == full_name, + _nested(document, "head", "ref") == head, + _nested(document, "head", "sha") == head_sha, + _nested(document, "head", "repo", "full_name") == full_name, + ) + if not all(checks): + raise PolicyError("Forgejo did not confirm the exact draft pull-request state") + return document + + def create_draft( repo: str, *, base: str, head: str, + head_sha: str, title: str, body: str, token: str, - opener: Callable[..., object] = urllib.request.urlopen, + opener: Callable[..., object] = _safe_urlopen, ) -> bytes: - """Create a same-repository draft pull request for human review.""" - data = { - "base": base, - "body": body, - "head": head, - "title": _draft_title(title), - } + """Create and verify a same-repository draft pull request for human review.""" + repo = _validate_repo(repo) + base = _validate_ref(base, "base") + head = _validate_ref(head, "head") + head_sha = _validate_sha(head_sha) + title = _draft_title(title) + body = _validate_body(body, forbidden=(token,)) + data = {"base": base, "body": body, "head": head, "title": title} result = _request( "POST", - f"/api/v1/repos/{ALLOWED_OWNER}/{_validate_repo(repo)}/pulls", + f"/api/v1/repos/{ALLOWED_OWNER}/{repo}/pulls", data, token=token, opener=opener, ) - _require_draft_response(result) + _require_create_response( + result, + repo=repo, + base=base, + head=head, + head_sha=head_sha, + title=title, + body=body, + ) return result -def _require_draft_response(value: bytes) -> dict[str, object]: - """Fail closed unless Forgejo confirms the resulting PR remains a draft.""" - try: - document = json.loads(value) - except json.JSONDecodeError as exc: - raise PolicyError("Forgejo returned invalid pull-request metadata") from exc - if not isinstance(document, dict) or document.get("draft") is not True: - raise PolicyError( - "Forgejo did not confirm draft state; human review is required" - ) - return document - - -def update_draft( - repo: str, - number: int, - updates: dict[str, str], - *, - token: str, - opener: Callable[..., object] = urllib.request.urlopen, -) -> bytes: - """Update title/body only after Forgejo confirms the PR is still a draft.""" - if not isinstance(number, int) or isinstance(number, bool) or number < 1: - raise PolicyError("pull-request number must be positive") - path = f"/api/v1/repos/{ALLOWED_OWNER}/{_validate_repo(repo)}/pulls/{number}" - current_raw = _request("GET", path, None, token=token, opener=opener) - try: - current = json.loads(current_raw) - except json.JSONDecodeError as exc: - raise PolicyError("Forgejo returned invalid pull-request metadata") from exc - if not isinstance(current, dict) or current.get("draft") is not True: - raise PolicyError("pull request is not a draft; human review now owns it") - safe_updates = dict(updates) - if "title" in safe_updates: - safe_updates["title"] = _draft_title(safe_updates["title"]) - result = _request("PATCH", path, safe_updates, token=token, opener=opener) - _require_draft_response(result) - return result - - -def _body(args: argparse.Namespace) -> str: - if args.body_file is None: - return "" - return args.body_file.read_text(encoding="utf-8") - - def parse_args(argv: list[str] | None = None) -> argparse.Namespace: - """Parse the three intentionally narrow client operations.""" + """Parse the intentionally narrow read and create-draft operations.""" parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( "--dry-run", @@ -301,28 +404,17 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: help="validate and describe the request without reading a token or using the network", ) subparsers = parser.add_subparsers(dest="operation", required=True) - - read_parser = subparsers.add_parser( - "read", help="read an Atlas repository API path" - ) + read_parser = subparsers.add_parser("read", help="read allowed Atlas metadata") read_parser.add_argument("path") - create_parser = subparsers.add_parser( "create-draft", help="create a same-repository draft pull request" ) create_parser.add_argument("repo") create_parser.add_argument("--base", required=True) create_parser.add_argument("--head", required=True) + create_parser.add_argument("--head-sha", required=True) create_parser.add_argument("--title", required=True) - create_parser.add_argument("--body-file", type=Path) - - update_parser = subparsers.add_parser( - "update-draft", help="update title/body of an existing draft pull request" - ) - update_parser.add_argument("repo") - update_parser.add_argument("number", type=int) - update_parser.add_argument("--title") - update_parser.add_argument("--body-file", type=Path) + create_parser.add_argument("--body", default="") return parser.parse_args(argv) @@ -330,22 +422,14 @@ def _operation(args: argparse.Namespace) -> tuple[str, str, object | None]: if args.operation == "read": return "GET", args.path, None repo = _validate_repo(args.repo) - if args.operation == "create-draft": - data = { - "base": args.base, - "body": _body(args), - "head": args.head, - "title": _draft_title(args.title), - } - return "POST", f"/api/v1/repos/{ALLOWED_OWNER}/{repo}/pulls", data - if args.number < 1: - raise PolicyError("pull-request number must be positive") - data = {} - if args.title is not None: - data["title"] = _draft_title(args.title) - if args.body_file is not None: - data["body"] = _body(args) - return "PATCH", f"/api/v1/repos/{ALLOWED_OWNER}/{repo}/pulls/{args.number}", data + _validate_sha(args.head_sha) + data = { + "base": _validate_ref(args.base, "base"), + "body": _validate_body(args.body), + "head": _validate_ref(args.head, "head"), + "title": _draft_title(args.title), + } + return "POST", f"/api/v1/repos/{ALLOWED_OWNER}/{repo}/pulls", data def _write_body(body: bytes) -> None: @@ -374,7 +458,6 @@ def main(argv: list[str] | None = None) -> int: "request_fields": sorted(data) if isinstance(data, dict) else [], - "requires_draft_preflight": operation == "update-draft", }, sort_keys=True, ) @@ -383,19 +466,17 @@ def main(argv: list[str] | None = None) -> int: token = read_token() if args.operation == "read": body = read(path, token=token) - elif args.operation == "create-draft": + else: assert isinstance(data, dict) body = create_draft( args.repo, base=str(data["base"]), head=str(data["head"]), + head_sha=args.head_sha, title=str(data["title"]), body=str(data["body"]), token=token, ) - else: - assert isinstance(data, dict) - body = update_draft(args.repo, args.number, data, token=token) _write_body(body) return 0 except urllib.error.HTTPError as exc: @@ -407,8 +488,6 @@ def main(argv: list[str] | None = None) -> int: sys.stderr.buffer.write(b"\n") return 1 except (OSError, PolicyError, ValueError, json.JSONDecodeError): - # Deliberately omit exception details. File contents, HTTP request - # objects, and provider errors can carry credential material. print( "Forgejo request rejected or unavailable; no credential was disclosed", file=sys.stderr, diff --git a/services/hermes/skills/manage-atlas-pull-requests/SKILL.md b/services/hermes/skills/manage-atlas-pull-requests/SKILL.md index 4e73a3cf..32fd1ccf 100644 --- a/services/hermes/skills/manage-atlas-pull-requests/SKILL.md +++ b/services/hermes/skills/manage-atlas-pull-requests/SKILL.md @@ -1,6 +1,6 @@ --- name: manage-atlas-pull-requests -description: Read private Atlas repository and pull-request metadata, create a draft pull request after a tested branch is pushed, or update the title/body of an existing draft through Hermes' least-authority Forgejo client. Use for PR handoff in scm.bstein.dev/atlas repositories. Never use it to merge, approve, close, delete, or force-push. +description: Read bounded private Atlas repository and pull-request metadata or create a verified draft pull request after a tested branch is pushed through Hermes' least-authority Forgejo client. Use for PR handoff in scm.bstein.dev/atlas repositories. Never use it to update, merge, approve, close, delete, or force-push. --- # Manage Atlas pull requests @@ -12,47 +12,43 @@ put it in an argument or environment variable, or replace this client with ## Read repository or PR state -Pass an Atlas repository API path to the read operation: +Pass one allowed Atlas metadata API path to the read operation: ```sh /opt/coordinator/gitea_api.py read /api/v1/repos/atlas/REPO/pulls/NUMBER ``` -Reads outside `scm.bstein.dev/atlas` are rejected. Treat returned state as +The client exposes only repository metadata, PRs and PR evidence, branches, +commits, and commit status. Hooks, Actions secrets/variables, collaborators, +protections, reviews, merge endpoints, releases, and administration are +rejected even as reads. Responses are size-bounded. Treat returned state as evidence, not authorization to change it. ## Create a review handoff Before opening a PR, verify the remote, branch, clean worktree, diff, tests, and -exact pushed commit. Never force-push. Write the PR description to a workspace -file so multiline Markdown is not shell-quoted, then validate without reading a -credential or using the network: +exact pushed commit. Never force-push. Pass that full 40-character pushed head +SHA explicitly. Keep the short body free of credentials and credential-like +text. Validate without reading a credential or using the network: ```sh /opt/coordinator/gitea_api.py --dry-run create-draft REPO \ - --base main --head hermes/TASK --title "Focused change" \ - --body-file /tmp/pr-body.md + --base main --head hermes/TASK --head-sha FULL_PUSHED_SHA \ + --title "Focused change" --body "Tests and review evidence" ``` Run the same command without `--dry-run` only when the branch is review-ready. -The client always creates a draft in the same repository. Report its URL and -exact head commit to Brad. Leave it unmerged for human review. - -## Update an existing draft - -Use only a title, a body file, or both: - -```sh -/opt/coordinator/gitea_api.py update-draft REPO NUMBER \ - --body-file /tmp/pr-body.md -``` - -The client first reads the PR from Forgejo and refuses the update unless it is -still a draft. It never changes head/base, review state, or merge state. +The client accepts success only when Forgejo returns the exact requested +repository, base/head refs, head SHA, title/body, canonical URLs, positive PR +number, and open/unmerged/draft state. Report its URL and exact head commit to +Brad. Leave it unmerged for human review. PR updates are intentionally absent; +publish a corrected tested commit and ask Brad how to proceed if metadata must +change. ## Stop at the authority boundary -Merge, approve, close, delete, branch mutation, comments, releases, repository -administration, and force-push are intentionally unavailable. Do not bypass the -client with a raw HTTP request. If one of those actions is needed, present the -tested commit and evidence to Brad and stop for human review. +Update, merge, approve, close, delete, branch mutation, comments, releases, +repository administration, and force-push are intentionally unavailable. Do +not bypass the client with a raw HTTP request. If one of those actions is +needed, present the tested commit and evidence to Brad and stop for human +review. diff --git a/testing/tests/test_hermes_chat_quality.py b/testing/tests/test_hermes_chat_quality.py index 09f6b00c..025f3bf7 100644 --- a/testing/tests/test_hermes_chat_quality.py +++ b/testing/tests/test_hermes_chat_quality.py @@ -102,8 +102,12 @@ def test_agent_config_keeps_delegated_reviewers_from_owning_task_lifecycle(): assert "`scm.bstein.dev` is Forgejo/Gitea, not GitHub" in instructions assert "GitHub/`gh` skill for an Atlas remote" in instructions assert "load\n`$manage-atlas-pull-requests`" in instructions - assert "Merge, approve, close, delete, comments" in instructions - assert "Leave every PR unmerged for Brad's review" in instructions + assert "Updates, merge,\napprove, close, delete, comments" in instructions + assert "Leave every PR\nunmerged for Brad's review" in instructions + assert "Use the Gitea pull-request merge endpoint" not in instructions + assert "reconcile the open PR" not in instructions + assert "use Flux reconciliation after" not in instructions + assert "PR publication ends at the verified open draft" in instructions assert "JENKINS_BASE_URL" in instructions assert "Do not\nuse `git reset --hard`" in instructions rendered = (HERMES / "agent-deployment.yaml").read_text() diff --git a/testing/tests/test_hermes_gitea_pr_client.py b/testing/tests/test_hermes_gitea_pr_client.py index 658af189..91475bcf 100644 --- a/testing/tests/test_hermes_gitea_pr_client.py +++ b/testing/tests/test_hermes_gitea_pr_client.py @@ -2,18 +2,18 @@ from __future__ import annotations +import copy import importlib.util -import io import json import sys -import urllib.error +import urllib.request from pathlib import Path import pytest -import yaml ROOT = Path(__file__).parents[2] CLIENT_PATH = ROOT / "services/hermes/scripts/gitea_api.py" +HEAD_SHA = "465cf9146b05c174a2a8d310aff6c64be58277b6" def _load(): @@ -36,6 +36,27 @@ def _draft_payload(**updates): return payload +def _draft_response(**updates): + response = { + "number": 3, + "state": "open", + "draft": True, + "merged": False, + "html_url": "https://scm.bstein.dev/atlas/cassandra/pulls/3", + "url": "https://scm.bstein.dev/api/v1/repos/atlas/cassandra/pulls/3", + "title": "WIP: Focused fix", + "body": "Review evidence", + "base": {"ref": "main", "repo": {"full_name": "atlas/cassandra"}}, + "head": { + "ref": "hermes/fix", + "sha": HEAD_SHA, + "repo": {"full_name": "atlas/cassandra"}, + }, + } + response.update(updates) + return response + + class Response: def __init__(self, body: object): self.body = body if isinstance(body, bytes) else json.dumps(body).encode() @@ -46,8 +67,32 @@ class Response: def __exit__(self, *_args): return False - def read(self): - return self.body + def read(self, limit=-1): + return self.body if limit < 0 else self.body[:limit] + + +def test_redirect_handler_rejects_cross_origin_with_sentinel_authorization(): + client = _load() + source = urllib.request.Request( + "https://scm.bstein.dev/api/v1/repos/atlas/cassandra", + headers={"Authorization": "token redirect-sentinel"}, + ) + + with pytest.raises(client.PolicyError, match="redirects are not allowed") as exc: + client.RejectRedirectHandler().redirect_request( + source, + None, + 302, + "Found", + {}, + "https://evil.example/collect", + ) + + assert "redirect-sentinel" not in str(exc.value) + assert any( + isinstance(handler, client.RejectRedirectHandler) + for handler in client._SAFE_OPENER.handlers + ) @pytest.mark.parametrize( @@ -69,6 +114,71 @@ def test_host_owner_and_path_escape_attempts_are_rejected(base_url: str, path: s client.build_request("GET", path, base_url=base_url, token="secret") +@pytest.mark.parametrize( + "path", + [ + "/api/v1/repos/atlas/cassandra", + "/api/v1/repos/atlas/cassandra/pulls?state=open&limit=20&page=1", + "/api/v1/repos/atlas/cassandra/pulls/7", + "/api/v1/repos/atlas/cassandra/pulls/7/commits?limit=20", + "/api/v1/repos/atlas/cassandra/pulls/7/files?page=1", + "/api/v1/repos/atlas/cassandra/branches", + "/api/v1/repos/atlas/cassandra/branches/main", + "/api/v1/repos/atlas/cassandra/commits?limit=10", + f"/api/v1/repos/atlas/cassandra/git/commits/{HEAD_SHA}", + f"/api/v1/repos/atlas/cassandra/commits/{HEAD_SHA}/status", + f"/api/v1/repos/atlas/cassandra/commits/{HEAD_SHA}/statuses?limit=10", + f"/api/v1/repos/atlas/cassandra/statuses/{HEAD_SHA}?page=1", + ], +) +def test_explicit_read_allowlist_accepts_only_engineering_metadata(path: str): + client = _load() + + request = client.build_request( + "GET", path, base_url=client.CANONICAL_BASE_URL, token="runtime" + ) + assert request.method == "GET" + + +@pytest.mark.parametrize( + "path", + [ + "/api/v1/repos/atlas/cassandra/hooks", + "/api/v1/repos/atlas/cassandra/actions/secrets", + "/api/v1/repos/atlas/cassandra/actions/variables", + "/api/v1/repos/atlas/cassandra/collaborators", + "/api/v1/repos/atlas/cassandra/branch_protections", + "/api/v1/repos/atlas/cassandra/keys", + "/api/v1/repos/atlas/cassandra/pulls/7/reviews", + "/api/v1/repos/atlas/cassandra/pulls/7/merge", + "/api/v1/repos/atlas/cassandra/pulls/7.diff", + "/api/v1/repos/atlas/cassandra/releases", + ], +) +def test_privileged_or_content_routes_are_denied_even_for_get(path: str): + client = _load() + + with pytest.raises(client.PolicyError, match="outside the metadata read allowlist"): + client.authorize_request("GET", path, None) + + +@pytest.mark.parametrize( + "path", + [ + "/api/v1/repos/atlas/cassandra/pulls?limit=51", + "/api/v1/repos/atlas/cassandra/pulls?state=merged", + "/api/v1/repos/atlas/cassandra/pulls?private=true", + "/api/v1/repos/atlas/cassandra/pulls?limit=1&limit=2", + "/api/v1/repos/atlas/cassandra?p=1", + ], +) +def test_read_query_is_bounded(path: str): + client = _load() + + with pytest.raises(client.PolicyError): + client.authorize_request("GET", path, None) + + @pytest.mark.parametrize( ("method", "path", "data"), [ @@ -79,12 +189,11 @@ def test_host_owner_and_path_escape_attempts_are_rejected(base_url: str, path: s "/api/v1/repos/atlas/cassandra/pulls/4/reviews", {"event": "APPROVED"}, ), - ("PATCH", "/api/v1/repos/atlas/cassandra/pulls/4", {"state": "closed"}), - ("PATCH", "/api/v1/repos/atlas/cassandra/pulls/4", {"draft": False}), + ("PATCH", "/api/v1/repos/atlas/cassandra/pulls/4", {"title": "WIP: x"}), ("PUT", "/api/v1/repos/atlas/cassandra/branches/main", {}), ], ) -def test_merge_approve_close_delete_and_other_mutations_are_rejected( +def test_merge_approve_close_delete_update_and_other_mutations_are_rejected( method: str, path: str, data: object ): client = _load() @@ -95,6 +204,36 @@ def test_merge_approve_close_delete_and_other_mutations_are_rejected( ) +@pytest.mark.parametrize( + "ref", + [ + "foo/.bar", + "foo/bar.lock/baz", + "foo..bar", + "foo@{bar", + "foo//bar", + "-danger", + "danger.", + "danger~one", + "danger^one", + "danger:one", + "danger one", + ], +) +def test_complete_git_ref_validation_rejects_invalid_names(ref: str): + client = _load() + + with pytest.raises(client.PolicyError): + client._validate_ref(ref, "head") + + +def test_git_ref_validation_uses_fixed_trusted_binary(): + client = _load() + + assert client.GIT_BIN == "/usr/bin/git" + assert client._validate_ref("hermes/valid-fix", "head") == "hermes/valid-fix" + + def test_create_forces_draft_title_and_same_repository_branch_names(): client = _load() assert ( @@ -103,7 +242,6 @@ def test_create_forces_draft_title_and_same_repository_branch_names(): ) == "create-draft" ) - with pytest.raises(client.PolicyError, match="draft-title prefix"): client.authorize_request( "POST", @@ -118,44 +256,6 @@ def test_create_forces_draft_title_and_same_repository_branch_names(): ) -def test_update_checks_server_draft_state_before_patching(): - client = _load() - calls = [] - responses = iter( - [ - Response({"number": 7, "draft": True}), - Response({"number": 7, "draft": True}), - ] - ) - - def opener(request, timeout): - calls.append((request.method, request.full_url, request.data, timeout)) - return next(responses) - - result = client.update_draft( - "cassandra", 7, {"title": "Narrowed repair"}, token="runtime", opener=opener - ) - - assert json.loads(result) == {"number": 7, "draft": True} - assert [call[0] for call in calls] == ["GET", "PATCH"] - assert json.loads(calls[1][2]) == {"title": "WIP: Narrowed repair"} - - -def test_update_rejects_non_draft_without_sending_patch(): - client = _load() - calls = [] - - def opener(request, timeout): - calls.append((request.method, timeout)) - return Response({"number": 7, "draft": False}) - - with pytest.raises(client.PolicyError, match="human review now owns"): - client.update_draft( - "cassandra", 7, {"body": "new"}, token="runtime", opener=opener - ) - assert calls == [("GET", 30)] - - def test_runtime_token_is_only_an_authorization_header(): client = _load() request = client.build_request( @@ -171,41 +271,128 @@ def test_runtime_token_is_only_an_authorization_header(): assert request.get_header("Authorization") == "token do-not-leak" -def test_create_uses_live_gitea_schema_and_verifies_draft_response(): +@pytest.mark.parametrize( + "body", + [ + "pass" + "word=not-a-real-credential", + "Authorization: " + "Bearer not-a-real-credential-value", + "token: " + "ghp_" + "notarealcredentialvalue123456", + "-----BEGIN OPENSSH " + "PRIVATE KEY-----", + "eyJnotarealheader." + "notarealpayloadvalue." + "notarealsignature", + ], +) +def test_body_rejects_common_credential_shapes(body: str): + client = _load() + + with pytest.raises(client.PolicyError, match="credential material"): + client._validate_body(body) + + +def test_create_rejects_exact_runtime_token_before_network(): + client = _load() + called = False + + def opener(*_args, **_kwargs): + nonlocal called + called = True + return Response(_draft_response()) + + with pytest.raises(client.PolicyError, match="runtime credential"): + client.create_draft( + "cassandra", + base="main", + head="hermes/fix", + head_sha=HEAD_SHA, + title="Focused fix", + body="accidental runtime-sentinel value", + token="runtime-sentinel", + opener=opener, + ) + assert called is False + + +def test_create_verifies_every_server_postcondition(): client = _load() calls = [] def opener(request, timeout): - calls.append((json.loads(request.data), timeout)) - return Response({"number": 3, "draft": True}) + calls.append((request, timeout)) + return Response(_draft_response()) result = client.create_draft( "cassandra", base="main", head="hermes/fix", + head_sha=HEAD_SHA, title="Focused fix", - body="Evidence", + body="Review evidence", token="runtime", opener=opener, ) - assert json.loads(result)["draft"] is True - assert calls[0][0]["title"] == "WIP: Focused fix" - assert "draft" not in calls[0][0] + assert json.loads(result) == _draft_response() + payload = json.loads(calls[0][0].data) + assert payload == { + "base": "main", + "body": "Review evidence", + "head": "hermes/fix", + "title": "WIP: Focused fix", + } + assert calls[0][1] == 30 -def test_create_fails_closed_when_server_does_not_confirm_draft(): +def test_create_postcondition_rejects_every_material_mismatch(): + client = _load() + mutations = [ + ("number", 0), + ("state", "closed"), + ("draft", False), + ("merged", True), + ("html_url", "https://evil.example/pulls/3"), + ("url", "https://evil.example/api/pulls/3"), + ("title", "Focused fix"), + ("body", "different"), + ] + documents = [] + for key, value in mutations: + document = _draft_response() + document[key] = value + documents.append(document) + for path, value in [ + (("base", "ref"), "master"), + (("base", "repo", "full_name"), "evil/cassandra"), + (("head", "ref"), "other"), + (("head", "sha"), "0" * 40), + (("head", "repo", "full_name"), "evil/cassandra"), + ]: + document = copy.deepcopy(_draft_response()) + target = document + for key in path[:-1]: + target = target[key] + target[path[-1]] = value + documents.append(document) + + for document in documents: + with pytest.raises(client.PolicyError): + client._require_create_response( + json.dumps(document).encode(), + repo="cassandra", + base="main", + head="hermes/fix", + head_sha=HEAD_SHA, + title="WIP: Focused fix", + body="Review evidence", + ) + + +def test_read_response_is_bounded(): client = _load() - with pytest.raises(client.PolicyError, match="did not confirm draft"): - client.create_draft( - "cassandra", - base="main", - head="hermes/fix", - title="Focused fix", - body="Evidence", + with pytest.raises(client.PolicyError, match="safe size limit"): + client.read( + "/api/v1/repos/atlas/cassandra", token="runtime", - opener=lambda *_a, **_k: Response({"number": 3, "draft": False}), + opener=lambda *_a, **_k: Response(b"x" * (client.MAX_RESPONSE_BYTES + 1)), ) @@ -216,107 +403,3 @@ def test_output_redaction_covers_exact_token_and_authorization_header(): assert b"do-not-leak" not in redacted assert redacted.count(b"[REDACTED]") >= 1 - - -def test_dry_run_does_not_read_token_or_use_network( - tmp_path: Path, monkeypatch, capsys -): - client = _load() - body = tmp_path / "body.md" - body.write_text("Evidence only\n", encoding="utf-8") - monkeypatch.setattr(client, "read_token", lambda: pytest.fail("read token")) - monkeypatch.setattr( - client.urllib.request, "urlopen", lambda *_a, **_k: pytest.fail("network") - ) - - assert ( - client.main( - [ - "--dry-run", - "create-draft", - "cassandra", - "--base", - "main", - "--head", - "hermes/fix", - "--title", - "Repair", - "--body-file", - str(body), - ] - ) - == 0 - ) - output = json.loads(capsys.readouterr().out) - assert output["operation"] == "create-draft" - assert output["owner"] == "atlas" - assert "Evidence only" not in json.dumps(output) - - -def test_http_error_path_redacts_token(monkeypatch, capsys): - client = _load() - monkeypatch.setattr(client, "read_token", lambda: "do-not-leak") - - def fail(*_args, **_kwargs): - raise urllib.error.HTTPError( - "https://scm.bstein.dev/api/v1/repos/atlas/cassandra", - 403, - "forbidden", - {}, - io.BytesIO(b"Authorization: token do-not-leak"), - ) - - monkeypatch.setattr(client, "read", lambda *_a, **_k: fail()) - - assert client.main(["read", "/api/v1/repos/atlas/cassandra"]) == 1 - captured = capsys.readouterr() - assert "do-not-leak" not in captured.err - assert "HTTP 403" in captured.err - - -def test_flux_manifest_projects_runtime_vault_token_and_skill_only(): - client_source = CLIENT_PATH.read_text(encoding="utf-8") - assert "/runtime-access/gitea-token" in client_source - assert "GITEA_TOKEN" not in client_source - - deployment = yaml.safe_load( - (ROOT / "services/hermes/agent-deployment.yaml").read_text(encoding="utf-8") - ) - template = deployment["spec"]["template"] - annotations = template["metadata"]["annotations"] - assert annotations["vault.hashicorp.com/agent-inject-secret-gitea-token"] == ( - "kv/data/atlas/hermes/developer-gitea" - ) - runtime = next( - volume - for volume in template["spec"]["volumes"] - if volume["name"] == "runtime-access" - ) - assert runtime["emptyDir"]["medium"] == "Memory" - - expected = {"hermes", "terminal", "cli-lane-runner"} - mounted = { - container["name"] - for container in template["spec"]["containers"] - if any( - mount["name"] == "atlas-pr-skill" - and mount["mountPath"] - == "/opt/data/workspace/skills/manage-atlas-pull-requests" - and mount.get("readOnly") is True - for mount in container.get("volumeMounts", []) - ) - } - assert mounted == expected - - kustomization = yaml.safe_load( - (ROOT / "services/hermes/kustomization.yaml").read_text(encoding="utf-8") - ) - generator = next( - item - for item in kustomization["configMapGenerator"] - if item["name"] == "hermes-atlas-pr-skill" - ) - assert generator["files"] == [ - "SKILL.md=skills/manage-atlas-pull-requests/SKILL.md", - "openai.yaml=skills/manage-atlas-pull-requests/agents/openai.yaml", - ] diff --git a/testing/tests/test_hermes_gitea_pr_integration.py b/testing/tests/test_hermes_gitea_pr_integration.py new file mode 100644 index 00000000..d3b47eef --- /dev/null +++ b/testing/tests/test_hermes_gitea_pr_integration.py @@ -0,0 +1,147 @@ +"""Runtime and Flux integration contracts for the safe Atlas PR client.""" + +from __future__ import annotations + +import importlib.util +import io +import json +import sys +import urllib.error +from pathlib import Path + +import pytest +import yaml + +ROOT = Path(__file__).parents[2] +CLIENT_PATH = ROOT / "services/hermes/scripts/gitea_api.py" +HEAD_SHA = "465cf9146b05c174a2a8d310aff6c64be58277b6" + + +def _load(): + spec = importlib.util.spec_from_file_location( + "safe_gitea_api_integration", CLIENT_PATH + ) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +def test_dry_run_has_no_file_input_and_uses_no_token_or_network(monkeypatch, capsys): + client = _load() + monkeypatch.setattr(client, "read_token", lambda: pytest.fail("read token")) + monkeypatch.setattr( + client, "_safe_urlopen", lambda *_a, **_k: pytest.fail("network") + ) + + assert ( + client.main( + [ + "--dry-run", + "create-draft", + "cassandra", + "--base", + "main", + "--head", + "hermes/fix", + "--head-sha", + HEAD_SHA, + "--title", + "Repair", + "--body", + "Evidence only", + ] + ) + == 0 + ) + output = json.loads(capsys.readouterr().out) + assert output["operation"] == "create-draft" + assert output["owner"] == "atlas" + assert "Evidence only" not in json.dumps(output) + with pytest.raises(SystemExit): + client.parse_args( + [ + "create-draft", + "cassandra", + "--base", + "main", + "--head", + "hermes/fix", + "--head-sha", + HEAD_SHA, + "--title", + "Repair", + "--body-file", + "/runtime-access/gitea-token", + ] + ) + + +def test_http_error_path_redacts_token(monkeypatch, capsys): + client = _load() + monkeypatch.setattr(client, "read_token", lambda: "do-not-leak") + + def fail(*_args, **_kwargs): + raise urllib.error.HTTPError( + "https://scm.bstein.dev/api/v1/repos/atlas/cassandra", + 403, + "forbidden", + {}, + io.BytesIO(b"Authorization: token do-not-leak"), + ) + + monkeypatch.setattr(client, "read", lambda *_a, **_k: fail()) + + assert client.main(["read", "/api/v1/repos/atlas/cassandra"]) == 1 + captured = capsys.readouterr() + assert "do-not-leak" not in captured.err + assert "HTTP 403" in captured.err + + +def test_flux_manifest_projects_runtime_vault_token_and_skill_only(): + client_source = CLIENT_PATH.read_text(encoding="utf-8") + assert "/runtime-access/gitea-token" in client_source + assert "GITEA_TOKEN" not in client_source + + deployment = yaml.safe_load( + (ROOT / "services/hermes/agent-deployment.yaml").read_text(encoding="utf-8") + ) + template = deployment["spec"]["template"] + annotations = template["metadata"]["annotations"] + assert annotations["vault.hashicorp.com/agent-inject-secret-gitea-token"] == ( + "kv/data/atlas/hermes/developer-gitea" + ) + runtime = next( + volume + for volume in template["spec"]["volumes"] + if volume["name"] == "runtime-access" + ) + assert runtime["emptyDir"]["medium"] == "Memory" + + expected = {"hermes", "terminal", "cli-lane-runner"} + mounted = { + container["name"] + for container in template["spec"]["containers"] + if any( + mount["name"] == "atlas-pr-skill" + and mount["mountPath"] + == "/opt/data/workspace/skills/manage-atlas-pull-requests" + and mount.get("readOnly") is True + for mount in container.get("volumeMounts", []) + ) + } + assert mounted == expected + + kustomization = yaml.safe_load( + (ROOT / "services/hermes/kustomization.yaml").read_text(encoding="utf-8") + ) + generator = next( + item + for item in kustomization["configMapGenerator"] + if item["name"] == "hermes-atlas-pr-skill" + ) + assert generator["files"] == [ + "SKILL.md=skills/manage-atlas-pull-requests/SKILL.md", + "openai.yaml=skills/manage-atlas-pull-requests/agents/openai.yaml", + ]