From 2bb3ac4549d64975a310bfa8bb98ab5cca1a2a2c Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Tue, 21 Jul 2026 20:49:48 -0500 Subject: [PATCH] fix(git-tools): env-robust token parse, host-bound creds, real Gitea URL verify (#865 round-5) CI-red root cause (classification a: my round-4 change fails in the clean/cold CI env): get_gitea_token_for_login hard-required PyYAML (`import yaml`), which is absent on CI's node:24-alpine (python3 without py3-yaml). Round-4's --login override cases were the first to exercise that path, turning the mosaic package test (test:framework-shell -> test-pr-review-gitea-comment.sh) RED. Fix: add an indentation-aware line-parser fallback that resolves the SAME per-name token PyYAML would from tea's flat `logins:` list; PyYAML stays the fast path. This also repairs a latent production defect (--login overrides were silently unusable on any PyYAML-less host). Auditor blockers folded into the same round-5: 1. issue_url vs pull_request_url shape (correctness): Gitea populates WEB (html) URLs in issue_url/pull_request_url, not API paths, and a PR-conversation comment carries pull_request_url (issue_url empty). Verification now accepts either web shape scoped to the repo slug + number, so a durable write is never rejected for URL shape. Test stubs now emit the REAL Gitea web shapes. 2. Cross-host credential binding (security): get_gitea_token_for_login now takes the repo host and requires the matched login's configured URL host to equal it; an override login configured for a different host FAILS CLOSED instead of sending a cross-host credential. Regression tests added to both suites. 3. Non-exhaustive enumeration (false-fail): removed the redundant, non-exhaustive post-verification list enumeration (gitea_fetch_all + confirm_*_enumerable) from both wrappers; the exact-id GET is authoritative. Pagination cases dropped; a guard asserts no list enumeration is performed. 4. Trap clobbering / temp-file leak (security/hygiene): removing the nested enumeration eliminates the RETURN-trap nesting that clobbered caller cleanup; remaining RETURN traps are single/non-nested and clean up on all exit paths. Temp-file leak regression tests (success + failure paths) added to both suites. 5. README: corrected the exhaustive-pagination claim and documented host-bound --login selection. Preserves every round-2/3/4 fix (explicit --login fail-closed at all write sites, token->identity attribution seam). Gates: cold `pnpm turbo run test --filter=@mosaicstack/mosaic` green (14/14); full test-*.sh suite green with AND without PyYAML; bash -n, shellcheck -x -S warning, prettier --check README clean. Co-Authored-By: Claude Opus 4.8 --- packages/mosaic/framework/tools/git/README.md | 4 +- .../framework/tools/git/detect-platform.sh | 147 ++++++++++++++--- .../framework/tools/git/issue-comment.sh | 134 ++------------- .../mosaic/framework/tools/git/pr-review.sh | 135 ++------------- .../tools/git/test-issue-comment-readback.sh | 155 +++++++++++++----- .../tools/git/test-pr-review-gitea-comment.sh | 135 +++++++++++---- 6 files changed, 373 insertions(+), 337 deletions(-) diff --git a/packages/mosaic/framework/tools/git/README.md b/packages/mosaic/framework/tools/git/README.md index 8cf68280..6ffaf45f 100644 --- a/packages/mosaic/framework/tools/git/README.md +++ b/packages/mosaic/framework/tools/git/README.md @@ -15,7 +15,7 @@ A successful provider write command—or a wrapper message based only on that co **This closes the concurrency window rather than documenting it.** Because verification keys on the id the create returned, a no-op create yields no id and fails closed with no list-scan fallback, and a _concurrent_ record — even one written by the _same_ identity with an identical body/state — has a _different_ id and cannot be mistaken for this write. There is no residual same-identity window: the earlier boundary-and-author heuristic (accept any `id > pre-write-max` with a matching author) is replaced entirely by exact-id attribution. -**Full pagination.** After the exact-id read-back, each wrapper also confirms the created id is enumerable in the record list, walking every page (`?limit=&page=1,2,…` until a short/empty page) so a record that lands beyond the first page is still found regardless of how many comments or reviews already exist. +**Exact-id read-back is the sole authority.** Verification is a direct `GET` of the one record the create returned; there is no follow-up list enumeration. An earlier redundant pass that re-listed the record's page (`?limit=&page=1,2,…`) was removed: server-capped page sizes and list-pagination quirks made it a false-failure source (a durable, exact-id-verified record could be missed by a non-exhaustive enumeration), and it added nothing over the authoritative exact-id `GET`. ## `tea` invocation notes (Gitea) @@ -24,6 +24,6 @@ A successful provider write command—or a wrapper message based only on that co ### `--login` override -Both `pr-review.sh` and `issue-comment.sh` accept an optional `--login ` flag that overrides the automatically detected Gitea login for that single invocation. The override selects **which credential the REST write, the `/user` identity lookup, and the read-back all use** — its token is resolved from the tea config for that login name (`get_gitea_token_for_login`), falling back to the repo host's credential when no login is named. Resolving the acting identity and the read-back from the _same_ login that performs the write is essential: a write performed under an overridden login must be verified against that login's identity, not the host default's. Callers who need a different login than the host default should pass `--login `. +Both `pr-review.sh` and `issue-comment.sh` accept an optional `--login ` flag that overrides the automatically detected Gitea login for that single invocation. The override selects **which credential the REST write, the `/user` identity lookup, and the read-back all use** — its token is resolved from the tea config for that login name (`get_gitea_token_for_login`), falling back to the repo host's credential when no login is named. The resolved login is **host-bound**: the login's configured URL host must match the repo remote's host, so a login name shared across hosts (or an override configured for a different Gitea) can never send one host's credential to another — a host mismatch fails closed rather than leaking a cross-host token. Resolving the acting identity and the read-back from the _same_ login that performs the write is essential: a write performed under an overridden login must be verified against that login's identity, not the host default's. Callers who need a different login than the host default should pass `--login `. As a durable successor to this mechanism, consider giving each reviewer/approver slot its own dedicated Gitea login credential, so that author≠reviewer holds at the credential level rather than relying on wrapper-level `--login` bookkeeping. This is a recommendation for future hardening, not something implemented by this flag. diff --git a/packages/mosaic/framework/tools/git/detect-platform.sh b/packages/mosaic/framework/tools/git/detect-platform.sh index 5ee119e2..2e0a5194 100755 --- a/packages/mosaic/framework/tools/git/detect-platform.sh +++ b/packages/mosaic/framework/tools/git/detect-platform.sh @@ -569,44 +569,137 @@ get_gitea_token() { # per-login tokens by `name` in $XDG_CONFIG_HOME/tea/config.yml (default # ~/.config/tea/config.yml), exactly as the `tea` CLI resolves them, so a # --login override and its REST read-back bind to the SAME credential/identity. -# Prints the token on success; returns non-zero (no output) if the config or a -# matching login token cannot be found. Callers must not log the result. +# +# $2 (repo host) binds the selected credential to the TARGET host: a tea login +# also records the `url` it authenticates against, and the matched login's URL +# host MUST equal the repo host. This fails closed when an override login is +# configured for a DIFFERENT host than the repo remote, so a login name shared +# across hosts (or a mistargeted override) can never send one host's credential +# to another host (cross-host credential leak). When $2 is empty the host bind +# is skipped (host-agnostic lookup) — callers that write should always pass it. +# +# Prints the token on success; returns non-zero (no output) if the config, a +# matching login token, or the host bind cannot be satisfied. Callers must not +# log the result. get_gitea_token_for_login() { - local login_name="$1" config_file + local login_name="$1" repo_host="${2:-}" config_file [[ -n "$login_name" ]] || return 1 config_file="${XDG_CONFIG_HOME:-$HOME/.config}/tea/config.yml" [[ -f "$config_file" ]] || return 1 - LOGIN_NAME="$login_name" python3 - "$config_file" <<'PY' + LOGIN_NAME="$login_name" REPO_HOST="$repo_host" python3 - "$config_file" <<'PY' import os +import re import sys - -try: - import yaml -except ImportError: - raise SystemExit(1) - -try: - with open(sys.argv[1], encoding="utf-8") as handle: - config = yaml.safe_load(handle) -except (OSError, yaml.YAMLError): - raise SystemExit(1) +from urllib.parse import urlparse wanted = os.environ["LOGIN_NAME"] -logins = config.get("logins") if isinstance(config, dict) else None -if not isinstance(logins, list): +repo_host = os.environ.get("REPO_HOST", "").strip().lower() +config_path = sys.argv[1] + + +def _strip_scalar(value): + value = value.strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in ("'", '"'): + value = value[1:-1] + return value + + +def _host_of(url): + if not isinstance(url, str) or not url: + return None + parsed = urlparse(url if "//" in url else f"//{url}") + host = parsed.hostname + return host.lower() if host else None + + +def _accept(token, url): + # Enforce the host bind before surfacing a token. When a repo host is given, + # the login's recorded URL host must match it exactly; a login with no + # usable URL (or a mismatched one) is rejected (fail closed) so a cross-host + # credential is never emitted. + if not isinstance(token, str) or not token: + return None + if repo_host: + if _host_of(url) != repo_host: + return None + return token + + +def _token_via_pyyaml(): + # Preferred, fully general path when PyYAML is installed. Raises ImportError + # (caught by the caller) when the module is unavailable so the environment + # -robust fallback can take over instead of failing closed on every host + # that lacks PyYAML. + import yaml + + with open(config_path, encoding="utf-8") as handle: + config = yaml.safe_load(handle) + logins = config.get("logins") if isinstance(config, dict) else None + if not isinstance(logins, list): + return None + for login in logins: + if isinstance(login, dict) and str(login.get("name") or "") == wanted: + return _accept(login.get("token"), login.get("url")) + return None + + +def _token_via_lines(): + # Conservative fallback for hosts without PyYAML. tea writes config.yml in a + # fixed, flat shape (a `logins:` list of maps with scalar name/url/token + # fields), so a small indentation-aware scan resolves the SAME token PyYAML + # would. It only ever returns the `token` of the entry whose `name` EXACTLY + # equals the requested login AND whose url host matches the repo host, so it + # cannot misattribute to another identity or host; anything it cannot parse + # yields None (fail closed). + with open(config_path, encoding="utf-8") as handle: + lines = handle.read().splitlines() + + logins_indent = None + start = len(lines) + for index, line in enumerate(lines): + match = re.match(r"^(\s*)logins\s*:\s*$", line) + if match: + logins_indent = len(match.group(1)) + start = index + 1 + break + if logins_indent is None: + return None + + entries = [] + current = None + for line in lines[start:]: + if not line.strip() or line.lstrip().startswith("#"): + continue + indent = len(line) - len(line.lstrip(" ")) + if indent <= logins_indent: + break + item = re.match(r"^\s*-\s*(.*)$", line) + rest = item.group(1) if item else line + if item: + current = {} + entries.append(current) + pair = re.match(r"^([A-Za-z0-9_]+)\s*:\s*(.*)$", rest.strip()) + if pair and current is not None: + current[pair.group(1)] = _strip_scalar(pair.group(2)) + + for entry in entries: + if str(entry.get("name") or "") == wanted: + return _accept(entry.get("token"), entry.get("url")) + return None + + +try: + try: + token = _token_via_pyyaml() + except ImportError: + token = _token_via_lines() +except Exception: raise SystemExit(1) -for login in logins: - if not isinstance(login, dict): - continue - if str(login.get("name") or "") == wanted: - token = login.get("token") - if isinstance(token, str) and token: - print(token) - raise SystemExit(0) - break - +if isinstance(token, str) and token: + print(token) + raise SystemExit(0) raise SystemExit(1) PY } diff --git a/packages/mosaic/framework/tools/git/issue-comment.sh b/packages/mosaic/framework/tools/git/issue-comment.sh index f225844b..08fd8738 100755 --- a/packages/mosaic/framework/tools/git/issue-comment.sh +++ b/packages/mosaic/framework/tools/git/issue-comment.sh @@ -97,12 +97,12 @@ gitea_resolve_api_for_login() { host=$(get_remote_host) if [[ -n "$override_explicit" ]]; then - GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login") || { - echo "Error: could not resolve a Gitea token for --login '$effective_login'; refusing to fall back to the host default identity (comment write/read-back)" >&2 + GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login" "$host") || { + echo "Error: could not resolve a host-matched Gitea token for --login '$effective_login' on host '$host'; refusing to fall back to the host default identity or a cross-host credential (comment write/read-back)" >&2 return 1 } else - GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login") \ + GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login" "$host") \ || GITEA_API_TOKEN=$(get_gitea_token "$host") || { echo "Error: Gitea token not found for login '$effective_login' (comment write/read-back)" >&2 return 1 @@ -121,63 +121,6 @@ gitea_resolve_api_for_login() { return 0 } -# Fetch every page of a Gitea list endpoint into $2 (merged into one JSON -# array). Gitea paginates list responses, so a single-page read would -# false-negative once a newly created record lands beyond page 1. Walks -# page=1,2,… until a short page (fewer than the requested limit) or an empty -# page is returned, so the merged array is exhaustive. $1 is the endpoint URL -# with NO query string. Returns non-zero (clear stderr) on any transport / -# HTTP / parse failure. -gitea_fetch_all() { - local base_url="$1" dest="$2" page=1 limit=50 status page_file count - - printf '[]' > "$dest" - while :; do - page_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-issue-comment-page.XXXXXX") - if ! status=$(curl -sS -o "$page_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ - "${base_url}?limit=${limit}&page=${page}"); then - rm -f "$page_file" - echo "Error: Gitea list read transport failed" >&2 - return 1 - fi - if [[ "$status" != "200" ]]; then - rm -f "$page_file" - echo "Error: Gitea list read failed with HTTP $status" >&2 - return 1 - fi - count=$(DEST="$dest" python3 - "$page_file" <<'PY' -import json -import os -import sys - -try: - with open(os.environ["DEST"], encoding="utf-8") as merged_file: - merged = json.load(merged_file) - with open(sys.argv[1], encoding="utf-8") as page_file: - page = json.load(page_file) - if not isinstance(page, list): - raise ValueError("page response is not a list") - merged.extend(item for item in page if isinstance(item, dict)) - with open(os.environ["DEST"], "w", encoding="utf-8") as merged_file: - json.dump(merged, merged_file) -except (OSError, json.JSONDecodeError, TypeError, ValueError) as error: - print(f"Error: could not merge Gitea list page: {error}", file=sys.stderr) - raise SystemExit(1) -print(len(page)) -PY -) || { rm -f "$page_file"; return 1; } - rm -f "$page_file" - [[ "$count" -lt "$limit" ]] && break - page=$((page + 1)) - if [[ "$page" -gt 1000 ]]; then - echo "Error: Gitea list pagination exceeded 1000 pages" >&2 - return 1 - fi - done - return 0 -} - # Resolve the login of the identity the API token authenticates as (GET # /user). Used to attribute a read-back record to THIS invocation's writer so # a concurrent write from a DIFFERENT identity cannot satisfy verification. @@ -216,54 +159,6 @@ print(login) PY } -# Confirm that the comment CREATED by this invocation ($2 = its provider id) is -# enumerable in the issue's full, paginated comment listing and is authored by -# the acting identity. Gitea paginates list responses, so a comment created -# beyond page 1 must still be found; walking every page also proves the created -# id is durably indexed against THIS issue rather than merely retrievable by id. -# Returns non-zero (clear stderr) if the exact created id is not present with a -# matching author. -gitea_confirm_comment_enumerable() { - local issue_number="$1" created_id="$2" acting_login="$3" merged_file - - merged_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-issue-comment-readback.XXXXXX") - trap 'rm -f "$merged_file"' RETURN - - gitea_fetch_all "$GITEA_API_BASE/issues/$issue_number/comments" "$merged_file" || return 1 - - CREATED_COMMENT_ID="$created_id" ACTING_LOGIN="$acting_login" \ - python3 - "$merged_file" <<'PY' -import json -import os -import sys - -try: - with open(sys.argv[1], encoding="utf-8") as response: - comments = json.load(response) - if not isinstance(comments, list): - raise ValueError("response is not a comment list") - created_id = int(os.environ["CREATED_COMMENT_ID"]) - acting_login = os.environ["ACTING_LOGIN"] - match = next( - ( - c for c in comments - if isinstance(c, dict) - and c.get("id") == created_id - and (c.get("user") or {}).get("login") == acting_login - ), - None, - ) - if match is None: - raise ValueError( - f"created comment id {created_id} is not enumerable in the issue's " - "paginated comment list under the acting identity" - ) -except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: - print(f"Error: Gitea comment enumeration check failed: {error}", file=sys.stderr) - raise SystemExit(1) -PY -} - # Post a comment to a Gitea issue via the supported REST API and verify it # durably against a PROVIDER-RETURNED created id — never trust an exit code # (#865 defect class: tea's non-existent `tea issue comment` no-ops yet exits @@ -335,7 +230,7 @@ PY EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \ - EXPECTED_ISSUE_NUMBER="$issue_number" \ + EXPECTED_NUMBER="$issue_number" \ python3 - "$readback_file" <<'PY' || return 1 import json import os @@ -350,26 +245,31 @@ try: expected_id = int(os.environ["EXPECTED_COMMENT_ID"]) expected_body = os.environ["EXPECTED_COMMENT_BODY"] acting_login = os.environ["ACTING_LOGIN"] - expected_suffix = ( - f"/repos/{os.environ['EXPECTED_REPO_SLUG']}" - f"/issues/{os.environ['EXPECTED_ISSUE_NUMBER']}" - ) - issue_path = urlparse(comment.get("issue_url", "")).path.rstrip("/") + slug = os.environ["EXPECTED_REPO_SLUG"] + number = os.environ["EXPECTED_NUMBER"] + # Gitea populates WEB (html) URLs here, not API paths. A plain issue comment + # carries issue_url = ///issues/ (pull_request_url + # empty); a comment posted to a PR's conversation carries + # pull_request_url = ///pulls/ (issue_url empty). + # Accept whichever the provider populated — scoped to THIS repo slug and + # number — so a genuine write is never rejected merely for URL shape. + issue_suffix = f"/{slug}/issues/{number}" + pr_suffix = f"/{slug}/pulls/{number}" + issue_path = urlparse(comment.get("issue_url") or "").path.rstrip("/") + pr_path = urlparse(comment.get("pull_request_url") or "").path.rstrip("/") if comment.get("id") != expected_id: raise ValueError("read-back id does not match the created id") if (comment.get("user") or {}).get("login") != acting_login: raise ValueError("created comment is not authored by the acting identity") if comment.get("body") != expected_body: raise ValueError("created comment body does not match") - if not issue_path.endswith(expected_suffix): + if not (issue_path.endswith(issue_suffix) or pr_path.endswith(pr_suffix)): raise ValueError("created comment does not belong to this issue") except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) raise SystemExit(1) PY - gitea_confirm_comment_enumerable "$issue_number" "$created_id" "$acting_login" || return 1 - echo "$created_id" return 0 } diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index 555c6e6a..4c7bddb3 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -146,7 +146,7 @@ PY EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \ - EXPECTED_PR_NUMBER="$pr_number" \ + EXPECTED_NUMBER="$pr_number" \ python3 - "$readback_file" <<'PY' || return 1 import json import os @@ -161,18 +161,25 @@ try: expected_id = int(os.environ["EXPECTED_COMMENT_ID"]) expected_body = os.environ["EXPECTED_COMMENT_BODY"] acting_login = os.environ["ACTING_LOGIN"] - expected_suffix = ( - f"/repos/{os.environ['EXPECTED_REPO_SLUG']}" - f"/issues/{os.environ['EXPECTED_PR_NUMBER']}" - ) - issue_path = urlparse(comment.get("issue_url", "")).path.rstrip("/") + slug = os.environ["EXPECTED_REPO_SLUG"] + number = os.environ["EXPECTED_NUMBER"] + # Gitea populates WEB (html) URLs here, not API paths. A PR-conversation + # comment carries pull_request_url = ///pulls/ (with + # issue_url empty), while a plain issue comment carries + # issue_url = ///issues/ (with pull_request_url empty). + # Accept whichever the provider populated — scoped to THIS repo slug and + # number — so a genuine write is never rejected merely for URL shape. + issue_suffix = f"/{slug}/issues/{number}" + pr_suffix = f"/{slug}/pulls/{number}" + issue_path = urlparse(comment.get("issue_url") or "").path.rstrip("/") + pr_path = urlparse(comment.get("pull_request_url") or "").path.rstrip("/") if comment.get("id") != expected_id: raise ValueError("read-back id does not match the created id") if (comment.get("user") or {}).get("login") != acting_login: raise ValueError("created comment is not authored by the acting identity") if comment.get("body") != expected_body: raise ValueError("created comment body does not match") - if not issue_path.endswith(expected_suffix): + if not (issue_path.endswith(issue_suffix) or pr_path.endswith(pr_suffix)): raise ValueError("created comment does not belong to this PR") except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) @@ -203,12 +210,12 @@ gitea_resolve_api_for_login() { host=$(get_remote_host) if [[ -n "$override_explicit" ]]; then - GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login") || { - echo "Error: could not resolve a Gitea token for --login '$effective_login'; refusing to fall back to the host default identity (review write/read-back)" >&2 + GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login" "$host") || { + echo "Error: could not resolve a host-matched Gitea token for --login '$effective_login' on host '$host'; refusing to fall back to the host default identity or a cross-host credential (review write/read-back)" >&2 return 1 } else - GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login") \ + GITEA_API_TOKEN=$(get_gitea_token_for_login "$effective_login" "$host") \ || GITEA_API_TOKEN=$(get_gitea_token "$host") || { echo "Error: Gitea token not found for login '$effective_login' (review write/read-back)" >&2 return 1 @@ -227,62 +234,6 @@ gitea_resolve_api_for_login() { return 0 } -# Fetch every page of a Gitea list endpoint into $2 (merged into one JSON -# array). Gitea paginates list responses, so a single-page read would -# false-negative once a newly created review/comment lands beyond page 1. -# Walks page=1,2,… until a short or empty page is returned so the merged array -# is exhaustive. $1 is the endpoint URL with NO query string. Returns non-zero -# (clear stderr) on any transport / HTTP / parse failure. -gitea_fetch_all() { - local base_url="$1" dest="$2" page=1 limit=50 status page_file count - - printf '[]' > "$dest" - while :; do - page_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-page.XXXXXX") - if ! status=$(curl -sS -o "$page_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ - "${base_url}?limit=${limit}&page=${page}"); then - rm -f "$page_file" - echo "Error: Gitea list read transport failed" >&2 - return 1 - fi - if [[ "$status" != "200" ]]; then - rm -f "$page_file" - echo "Error: Gitea list read failed with HTTP $status" >&2 - return 1 - fi - count=$(DEST="$dest" python3 - "$page_file" <<'PY' -import json -import os -import sys - -try: - with open(os.environ["DEST"], encoding="utf-8") as merged_file: - merged = json.load(merged_file) - with open(sys.argv[1], encoding="utf-8") as page_file: - page = json.load(page_file) - if not isinstance(page, list): - raise ValueError("page response is not a list") - merged.extend(item for item in page if isinstance(item, dict)) - with open(os.environ["DEST"], "w", encoding="utf-8") as merged_file: - json.dump(merged, merged_file) -except (OSError, json.JSONDecodeError, TypeError, ValueError) as error: - print(f"Error: could not merge Gitea list page: {error}", file=sys.stderr) - raise SystemExit(1) -print(len(page)) -PY -) || { rm -f "$page_file"; return 1; } - rm -f "$page_file" - [[ "$count" -lt "$limit" ]] && break - page=$((page + 1)) - if [[ "$page" -gt 1000 ]]; then - echo "Error: Gitea list pagination exceeded 1000 pages" >&2 - return 1 - fi - done - return 0 -} - # Resolve the login of the identity the API token authenticates as (GET # /user). Used to attribute a read-back review to THIS action's reviewer so a # concurrent review from a DIFFERENT identity cannot satisfy verification. @@ -358,56 +309,6 @@ print(head_sha) PY } -# Confirm that the review CREATED by this action ($2 = its provider id) is -# enumerable in the PR's full, paginated review listing, authored by the acting -# identity, in the expected state. Gitea paginates review lists, so a review -# created beyond page 1 must still be found; walking every page also proves the -# created id is durably indexed against THIS PR rather than merely retrievable -# by id. Returns non-zero (clear stderr) if the exact created id is absent or -# does not match author/state. -gitea_confirm_review_enumerable() { - local pr_number="$1" created_id="$2" expected_state="$3" acting_login="$4" merged_file - - merged_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-list.XXXXXX") - trap 'rm -f "$merged_file"' RETURN - - gitea_fetch_all "$GITEA_API_BASE/pulls/$pr_number/reviews" "$merged_file" || return 1 - - CREATED_REVIEW_ID="$created_id" EXPECTED_STATE="$expected_state" ACTING_LOGIN="$acting_login" \ - python3 - "$merged_file" <<'PY' -import json -import os -import sys - -try: - with open(sys.argv[1], encoding="utf-8") as response: - reviews = json.load(response) - if not isinstance(reviews, list): - raise ValueError("response is not a review list") - created_id = int(os.environ["CREATED_REVIEW_ID"]) - expected_state = os.environ["EXPECTED_STATE"] - acting_login = os.environ["ACTING_LOGIN"] - match = next( - ( - r for r in reviews - if isinstance(r, dict) - and r.get("id") == created_id - and (r.get("user") or {}).get("login") == acting_login - and r.get("state") == expected_state - ), - None, - ) - if match is None: - raise ValueError( - f"created review id {created_id} is not enumerable in the PR's " - "paginated review list under the acting identity/state" - ) -except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: - print(f"Error: Gitea review enumeration check failed: {error}", file=sys.stderr) - raise SystemExit(1) -PY -} - # Submit a review to a Gitea PR via the supported REST API and verify it against # a PROVIDER-RETURNED created id. tea 0.11.1's `pr approve`/`reject` cannot emit # the id of the review it created and can silently no-op while exiting 0 (#865 @@ -513,8 +414,6 @@ except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error raise SystemExit(1) PY - gitea_confirm_review_enumerable "$pr_number" "$created_id" "$event" "$acting_login" || return 1 - echo "$created_id" return 0 } diff --git a/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh b/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh index 6eb3bdfe..a232de43 100755 --- a/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh +++ b/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh @@ -26,15 +26,22 @@ # concurrency window — no fallback list scan can rescue a no-op); # 5. fails closed when the created record is not authored by the acting # identity; -# 6. enumerates the created id in the issue's FULLY PAGINATED comment list, -# finding it even when it lands beyond page 1; +# 6. treats the exact-id GET as the SOLE authority — it performs NO follow-up +# list enumeration (the stub exposes no comment-list endpoint, so any +# residual enumeration attempt would fail the run); # 7. with a RESOLVABLE --login override, performs the write, the /user identity # lookup, and the read-back ALL under THAT login's token/identity — never # the host default; # 8. with an UNRESOLVABLE --login override, FAILS CLOSED (nonzero, no write, no # success line) instead of silently downgrading to the host default # identity — the token seam maps each bearer token to the identity it -# authenticates as, so a misattributed write is caught. +# authenticates as, so a misattributed write is caught; +# 9. with a --login override whose tea config URL is a DIFFERENT host than the +# repo remote, FAILS CLOSED (host-bound token selection) rather than sending +# that other host's credential cross-host; +# 10. leaves NO temp files behind (POST/GET bodies + metadata) on either the +# success or the failure path — nested function-scoped RETURN traps do not +# clobber each other and every scratch file is removed on all exit paths. set -euo pipefail @@ -49,13 +56,16 @@ AUTH_LOG="$WORK_DIR/auth.log" OUTPUT_FILE="$WORK_DIR/output.log" CREDENTIALS_FILE="$WORK_DIR/credentials.json" STATE_FILE="$WORK_DIR/comments.json" +# A dedicated scratch dir the wrapper is pointed at via TMPDIR, so the leak +# check can assert every POST/GET body + metadata temp file is cleaned up. +TMP_SCRATCH="$WORK_DIR/scratch" cleanup() { rm -rf "$WORK_DIR" } trap cleanup EXIT -mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" +mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$TMP_SCRATCH" git -C "$REPO_DIR" init -q git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git @@ -71,13 +81,22 @@ FOREIGN_LOGIN="other-writer" OVERRIDE_LOGIN="delegated-reviewer" DEFAULT_TOKEN="test-only-placeholder" OVERRIDE_TOKEN="override-token-placeholder" +# A --login override whose tea config URL points at a DIFFERENT Gitea host than +# the repo remote (git.mosaicstack.dev). Its token must NEVER be sent to the +# repo host: host-bound selection must fail closed on the host mismatch. +CROSS_HOST_LOGIN="foreign-host-reviewer" +CROSS_HOST_TOKEN="cross-host-token-placeholder" # tea config: the override login has its own token here (as tea itself stores # per-login tokens). The default login name ("mosaicstack") is deliberately NOT # present, so the no-override default path resolves via the host credential # fallback while an explicit --login must resolve from this file or fail closed. +# A second login is configured for a DIFFERENT host to exercise host-bound +# rejection. mkdir -p "$XDG_DIR/tea" -OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" python3 - "$XDG_DIR/tea/config.yml" <<'PY' +OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \ + CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \ + python3 - "$XDG_DIR/tea/config.yml" <<'PY' import os import sys @@ -86,6 +105,9 @@ with open(sys.argv[1], "w", encoding="utf-8") as handle: handle.write(f" - name: {os.environ['OVERRIDE_LOGIN']}\n") handle.write(" url: https://git.mosaicstack.dev\n") handle.write(f" token: {os.environ['OVERRIDE_TOKEN']}\n") + handle.write(f" - name: {os.environ['CROSS_HOST_LOGIN']}\n") + handle.write(" url: https://git.uscllc.com\n") + handle.write(f" token: {os.environ['CROSS_HOST_TOKEN']}\n") PY CONFIGURED_GITEA_URL="https://git.mosaicstack.dev" python3 - "$CREDENTIALS_FILE" <<'PY' @@ -123,10 +145,12 @@ SH chmod +x "$BIN_DIR/tea" # curl stub: a small REST server backed by persistent on-disk comment state. -# GET /user -> acting identity -# POST /issues/7/comments -> CREATE + PERSIST, return created object -# GET /issues/comments/{id} -> read the persisted record by exact id -# GET /issues/7/comments?page=&.. -> paginated listing of persisted state +# GET /user -> acting identity +# POST /issues/7/comments -> CREATE + PERSIST, return created object +# GET /issues/comments/{id} -> read the persisted record by exact id +# There is deliberately NO comment-LIST endpoint: exact-id read-back is the sole +# authority, so any residual list enumeration attempt hits the unexpected-request +# guard and fails the test. cat > "$BIN_DIR/curl" <<'SH' #!/usr/bin/env bash set -euo pipefail @@ -164,6 +188,7 @@ acting_identity="" case "$auth_token" in "$ISSUE_COMMENT_DEFAULT_TOKEN") acting_identity="$ISSUE_COMMENT_ACTING_LOGIN" ;; "$ISSUE_COMMENT_OVERRIDE_TOKEN") acting_identity="$ISSUE_COMMENT_OVERRIDE_LOGIN" ;; + "$ISSUE_COMMENT_CROSS_HOST_TOKEN") acting_identity="$ISSUE_COMMENT_CROSS_HOST_LOGIN" ;; esac printf '%s %s %s\n' "$method" "$path" "${acting_identity:-}" >> "$ISSUE_COMMENT_AUTH_LOG" @@ -211,7 +236,10 @@ record = { "id": new_id, "body": body, "user": {"login": author}, - "issue_url": f"https://git.mosaicstack.dev/api/v1/repos/{repo}/issues/7", + # REAL Gitea comment shape: issue_url is the WEB (html) path, not an API + # path, and a plain issue comment leaves pull_request_url empty. + "issue_url": f"https://git.mosaicstack.dev/{repo}/issues/7", + "pull_request_url": "", } comments.append(record) with open(state_path, "w", encoding="utf-8") as handle: @@ -238,24 +266,6 @@ else: print("200") print(json.dumps(match)) PY -) - write_response "$(printf '%s' "$result" | head -n1)" "$(printf '%s' "$result" | tail -n +2)" -elif [[ "$method" == "GET" && "$path" == "$ISSUE_COMMENT_API_BASE/issues/7/comments" ]]; then - result=$(ISSUE_COMMENT_QUERY="$query" python3 - <<'PY' -import json -import os -from urllib.parse import parse_qs - -state_path = os.environ["ISSUE_COMMENT_STATE"] -params = parse_qs(os.environ["ISSUE_COMMENT_QUERY"]) -limit = int(params.get("limit", ["50"])[0]) -page = int(params.get("page", ["1"])[0]) -with open(state_path, encoding="utf-8") as handle: - comments = json.load(handle) -start = (page - 1) * limit -print("200") -print(json.dumps(comments[start:start + limit])) -PY ) write_response "$(printf '%s' "$result" | head -n1)" "$(printf '%s' "$result" | tail -n +2)" else @@ -279,23 +289,30 @@ mode = os.environ["ISSUE_COMMENT_SEED_MODE"] body = os.environ["ISSUE_COMMENT_SEED_BODY"] acting = os.environ["ISSUE_COMMENT_SEED_ACTING"] repo = os.environ["ISSUE_COMMENT_SEED_REPO"] -issue_url = f"https://git.mosaicstack.dev/api/v1/repos/{repo}/issues/7" +# REAL Gitea comment shape: issue_url is the WEB path, pull_request_url empty. +issue_url = f"https://git.mosaicstack.dev/{repo}/issues/7" + + +def comment(cid, text, author): + return { + "id": cid, + "body": text, + "user": {"login": author}, + "issue_url": issue_url, + "pull_request_url": "", + } + if mode == "fresh-success": - # 50 pre-existing comments fill page 1 (limit 50); the comment this run - # creates becomes id 51 and lands ALONE on page 2, exercising >page-1 - # pagination in the enumeration check. - comments = [ - {"id": i, "body": f"prior {i}", "user": {"login": acting}, "issue_url": issue_url} - for i in range(1, 51) - ] + # 50 pre-existing comments already exist; the comment this run creates + # becomes id 51, proving exact-id read-back works regardless of how many + # comments precede it (no list enumeration is involved). + comments = [comment(i, f"prior {i}", acting) for i in range(1, 51)] elif mode == "no-op-concurrent": # A concurrent SAME-IDENTITY comment with the IDENTICAL body already exists. # The wrapper's own write will be a no-op; it must still fail closed because # no created id is returned — it must not scan and accept this record. - comments = [ - {"id": 55, "body": body, "user": {"login": acting}, "issue_url": issue_url} - ] + comments = [comment(55, body, acting)] else: # author-mismatch comments = [] @@ -315,6 +332,7 @@ run_comment() { ( cd "$REPO_DIR" PATH="$BIN_DIR:$PATH" \ + TMPDIR="$TMP_SCRATCH" \ XDG_CONFIG_HOME="$XDG_DIR" \ MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \ ISSUE_COMMENT_TEA_LOG="$TEA_LOG" \ @@ -325,8 +343,10 @@ run_comment() { ISSUE_COMMENT_ACTING_LOGIN="$ACTING_LOGIN" \ ISSUE_COMMENT_FOREIGN_LOGIN="$FOREIGN_LOGIN" \ ISSUE_COMMENT_OVERRIDE_LOGIN="$OVERRIDE_LOGIN" \ + ISSUE_COMMENT_CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" \ ISSUE_COMMENT_DEFAULT_TOKEN="$DEFAULT_TOKEN" \ ISSUE_COMMENT_OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \ + ISSUE_COMMENT_CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \ ISSUE_COMMENT_REPO_SLUG="$REPO_SLUG" \ ISSUE_COMMENT_API_BASE="$API_BASE" \ ISSUE_COMMENT_API_ROOT="$API_ROOT" \ @@ -334,8 +354,21 @@ run_comment() { ) > "$OUTPUT_FILE" 2>&1 } +# Assert the wrapper left no scratch temp files behind in TMPDIR (POST/GET +# request bodies + metadata). Called after both success and failure paths so a +# clobbered/leaked RETURN trap is caught on every exit route. +assert_no_temp_leak() { + local context="$1" leaked + leaked=$(find "$TMP_SCRATCH" -type f -name 'mosaic-issue-comment-*' 2>/dev/null || true) + if [[ -n "$leaked" ]]; then + echo "FAIL: issue-comment temp files leaked ($context):" >&2 + printf '%s\n' "$leaked" >&2 + exit 1 + fi +} + # Case 1: a genuine REST create (id 51) is verified end to end via its exact -# provider-returned id and enumerated on page 2 of the paginated listing. +# provider-returned id — no list enumeration is involved. run_comment fresh-success grep -q 'Added and verified comment on Gitea issue #7 (comment ID 51)' "$OUTPUT_FILE" # The write is a REST POST, never a tea comment. @@ -348,12 +381,17 @@ fi grep -q "^GET $API_BASE/issues/comments/51$" "$CURL_LOG" # Acting identity resolved via GET /user. grep -q "^GET $API_ROOT/user$" "$CURL_LOG" -# Enumeration paginated beyond page 1 to find the created comment. -grep -q "^GET $API_BASE/issues/7/comments?limit=[0-9]*&page=2$" "$CURL_LOG" +# No comment-list enumeration is performed — the exact-id GET is authoritative. +if grep -Eq "^GET $API_BASE/issues/7/comments(\?|$)" "$CURL_LOG"; then + echo "FAIL: wrapper performed a redundant comment-list enumeration" >&2 + exit 1 +fi # Default path (no --login): the host credential fallback resolves, and the # write is performed AND self-verified under the host-default acting identity. grep -q "^POST $API_BASE/issues/7/comments $ACTING_LOGIN$" "$AUTH_LOG" grep -q "^GET $API_BASE/issues/comments/51 $ACTING_LOGIN$" "$AUTH_LOG" +# Success path leaves no scratch temp files behind. +assert_no_temp_leak "fresh-success" # Case 2: a no-op write with a concurrent SAME-IDENTITY, same-body comment # already present must FAIL CLOSED — the closed concurrency window. @@ -382,6 +420,9 @@ if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then echo "FAIL: read-back did not enforce acting-identity authorship" >&2 exit 1 fi +# Failure-after-read-back path must ALSO leave no scratch temp files behind +# (proves the RETURN traps clean up on the error-return route, not just success). +assert_no_temp_leak "author-mismatch" # Case 4: a RESOLVABLE --login override — the write, the /user identity lookup, # and the read-back must ALL be performed under THAT login's token/identity, not @@ -420,4 +461,34 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then exit 1 fi +# Case 6: a --login override that IS present in tea config but whose URL is a +# DIFFERENT host than the repo remote must FAIL CLOSED (host-bound selection). +# The cross-host token must NEVER be sent to the repo host, and no write occurs. +if run_comment cross-host --login "$CROSS_HOST_LOGIN"; then + echo "FAIL: cross-host --login override did not fail closed" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then + echo "FAIL: cross-host --login override reported success" >&2 + exit 1 +fi +# The cross-host credential must not have performed ANY request against the repo +# host — no request may be attributed to the cross-host identity. +if grep -q " $CROSS_HOST_LOGIN\$" "$AUTH_LOG"; then + echo "FAIL: cross-host credential was sent to the repo host (cross-host leak)" >&2 + cat "$AUTH_LOG" >&2 + exit 1 +fi +if grep -q "^POST $API_BASE/issues/7/comments" "$CURL_LOG"; then + echo "FAIL: cross-host --login override still performed a write" >&2 + exit 1 +fi +# It must not have silently downgraded to the host default identity either. +if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then + echo "FAIL: cross-host --login override fell back to the host default identity" >&2 + exit 1 +fi +assert_no_temp_leak "cross-host" + echo "issue-comment.sh REST create + exact-id read-back regression passed" diff --git a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh index 3108235e..bd2e03de 100644 --- a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh +++ b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh @@ -26,6 +26,13 @@ # CLOSED (unresolvable case) — never silently downgrade to the host-default # identity. The host-default best-effort fallback is reserved for the # no-override default path. +# +# #865 Round-5: the exact-id read-back is the SOLE authority — the wrapper does +# NO follow-up list enumeration (the stub exposes no review/comment list +# endpoint, so a residual enumeration would fail the run). A --login override is +# host-bound: an override configured for a DIFFERENT host than the repo remote +# FAILS CLOSED rather than leaking a cross-host credential. And every run leaves +# no scratch temp files behind on any exit path (POST/GET bodies + metadata). set -euo pipefail @@ -43,6 +50,9 @@ CURL_LOG="$WORK_DIR/curl.log" AUTH_LOG="$WORK_DIR/auth.log" OUTPUT_FILE="$WORK_DIR/output.log" CREDENTIALS_FILE="$WORK_DIR/credentials.json" +# A dedicated scratch dir the wrapper is pointed at via TMPDIR, so the leak +# check can assert every POST/GET body + metadata temp file is cleaned up. +TMP_SCRATCH="$WORK_DIR/scratch" cleanup() { rm -rf "$WORK_DIR" @@ -57,17 +67,25 @@ HEAD_SHA="HEADSHA_FEEDFACE" OVERRIDE_LOGIN="primary-reviewer" DEFAULT_TOKEN="test-only-placeholder" OVERRIDE_TOKEN="override-token-placeholder" +# A --login override whose tea config URL points at a DIFFERENT Gitea host than +# the repo remote (git.mosaicstack.dev). Host-bound selection must reject it +# rather than send its token cross-host. +CROSS_HOST_LOGIN="foreign-host-reviewer" +CROSS_HOST_TOKEN="cross-host-token-placeholder" -mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR" +mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR" "$TMP_SCRATCH" git -C "$REPO_DIR" init -q git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git # tea config: the override login carries its own token here. The default login # name ("mosaicstack") is deliberately absent, so the no-override default path # resolves via the host credential fallback while an explicit --login must -# resolve from this file or fail closed. +# resolve from this file or fail closed. A second login is configured for a +# DIFFERENT host to exercise host-bound rejection. mkdir -p "$XDG_DIR/tea" -OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" python3 - "$XDG_DIR/tea/config.yml" <<'PY' +OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \ + CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \ + python3 - "$XDG_DIR/tea/config.yml" <<'PY' import os import sys @@ -76,6 +94,9 @@ with open(sys.argv[1], "w", encoding="utf-8") as handle: handle.write(f" - name: {os.environ['OVERRIDE_LOGIN']}\n") handle.write(" url: https://git.mosaicstack.dev\n") handle.write(f" token: {os.environ['OVERRIDE_TOKEN']}\n") + handle.write(f" - name: {os.environ['CROSS_HOST_LOGIN']}\n") + handle.write(" url: https://git.uscllc.com\n") + handle.write(f" token: {os.environ['CROSS_HOST_TOKEN']}\n") PY write_credentials() { @@ -155,6 +176,7 @@ acting_identity="" case "$auth_token" in "$PR_REVIEW_DEFAULT_TOKEN") acting_identity="$PR_REVIEW_ACTING_LOGIN" ;; "$PR_REVIEW_OVERRIDE_TOKEN") acting_identity="$PR_REVIEW_OVERRIDE_LOGIN" ;; + "$PR_REVIEW_CROSS_HOST_TOKEN") acting_identity="$PR_REVIEW_CROSS_HOST_LOGIN" ;; esac printf '%s %s %s\n' "$method" "$path" "${acting_identity:-}" >> "$PR_REVIEW_AUTH_LOG" @@ -245,23 +267,6 @@ else: print(json.dumps(match)) PY )" -elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then - emit "$(PR_REVIEW_QUERY="$query" python3 - <<'PY' -import json -import os -from urllib.parse import parse_qs - -state_path = os.environ["PR_REVIEW_REVIEWS"] -params = parse_qs(os.environ["PR_REVIEW_QUERY"]) -limit = int(params.get("limit", ["50"])[0]) -page = int(params.get("page", ["1"])[0]) -with open(state_path, encoding="utf-8") as handle: - reviews = json.load(handle) -start = (page - 1) * limit -print("200") -print(json.dumps(reviews[start:start + limit])) -PY -)" elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/issues/123/comments" ]]; then case "$mode" in write-transport-failure) @@ -278,13 +283,18 @@ import os state_path = os.environ["PR_REVIEW_COMMENTS"] acting = os.environ["PR_REVIEW_ACTING_LOGIN"] -base = os.environ["PR_REVIEW_EXPECTED_API_BASE"] +web_base = os.environ["PR_REVIEW_WEB_BASE"] body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body") +# REAL Gitea shape for a comment posted to a PR's conversation +# (/issues/{n}/comments on a PR): pull_request_url is the WEB pulls path and +# issue_url is left empty. This is what the wrapper must tolerate — it must NOT +# require an API-shaped issue_url. record = { "id": 456, "body": body, "user": {"login": acting}, - "issue_url": f"{base}/issues/123", + "issue_url": "", + "pull_request_url": f"{web_base}/pulls/123", } with open(state_path, "w", encoding="utf-8") as handle: json.dump([record], handle) @@ -344,10 +354,10 @@ def review(rid, state, commit, login): return {"id": rid, "state": state, "commit_id": commit, "user": {"login": login}} -if mode == "paginated-approve": - # 50 pre-existing reviews fill page 1 (limit 50); the review this run submits - # becomes id 51 and lands ALONE on page 2, exercising >page-1 pagination in - # the enumeration check. +if mode == "many-prior-approve": + # 50 pre-existing reviews already exist; the review this run submits becomes + # id 51, proving exact-id read-back works regardless of how many reviews + # precede it (no list enumeration is involved). reviews = [review(i, "COMMENT", "oldsha0000", acting) for i in range(1, 51)] elif mode == "no-op-concurrent-review": # A concurrent SAME-IDENTITY APPROVED review at the CURRENT head already @@ -370,6 +380,7 @@ run_review() { local login_override="${7:-}" local expected_api_base="${configured_url%/}/api/v1/repos/$expected_repo" local expected_api_root="${configured_url%/}/api/v1" + local expected_web_base="${configured_url%/}/$expected_repo" git -C "$REPO_DIR" remote set-url origin "$remote_url" write_credentials "$configured_url" : > "$TEA_LOG" @@ -380,6 +391,7 @@ run_review() { ( cd "$REPO_DIR" PATH="$BIN_DIR:$PATH" \ + TMPDIR="$TMP_SCRATCH" \ XDG_CONFIG_HOME="$XDG_DIR" \ MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \ PR_REVIEW_TEA_LOG="$TEA_LOG" \ @@ -393,16 +405,32 @@ run_review() { PR_REVIEW_EXPECTED_BODY="$comment" \ PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \ PR_REVIEW_API_ROOT="$expected_api_root" \ + PR_REVIEW_WEB_BASE="$expected_web_base" \ PR_REVIEW_HEAD_SHA="$HEAD_SHA" \ PR_REVIEW_ACTING_LOGIN="$ACTING_LOGIN" \ PR_REVIEW_FOREIGN_LOGIN="$FOREIGN_LOGIN" \ PR_REVIEW_OVERRIDE_LOGIN="$OVERRIDE_LOGIN" \ + PR_REVIEW_CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" \ PR_REVIEW_DEFAULT_TOKEN="$DEFAULT_TOKEN" \ PR_REVIEW_OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \ + PR_REVIEW_CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \ "$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"} ${login_override:+--login "$login_override"} ) > "$OUTPUT_FILE" 2>&1 } +# Assert the wrapper left no scratch temp files behind in TMPDIR (POST/GET +# request bodies + metadata). Called after both success and failure paths so a +# clobbered/leaked RETURN trap is caught on every exit route. +assert_no_temp_leak() { + local context="$1" leaked + leaked=$(find "$TMP_SCRATCH" -type f -name 'mosaic-pr-review-*' 2>/dev/null || true) + if [[ -n "$leaked" ]]; then + echo "FAIL: pr-review temp files leaked ($context):" >&2 + printf '%s\n' "$leaked" >&2 + exit 1 + fi +} + assert_no_tea_write() { # tea must only ever be used for the login list, never to write. if grep -qvE '^login list --output json$' "$TEA_LOG"; then @@ -421,12 +449,17 @@ grep -q '^GET https://git.mosaicstack.dev/api/v1/user$' "$CURL_LOG" grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG" grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews$' "$CURL_LOG" grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/101$' "$CURL_LOG" -grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=1$' "$CURL_LOG" +# No review-list enumeration is performed — the exact-id GET is authoritative. +if grep -Eq '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews(\?|$)' "$CURL_LOG"; then + echo "FAIL: wrapper performed a redundant review-list enumeration" >&2 + exit 1 +fi # No-override default path: the write, /user lookup, and read-back all resolve # via the host-default credential and authenticate as the acting identity. grep -q "^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews $ACTING_LOGIN\$" "$AUTH_LOG" grep -q "^GET https://git.mosaicstack.dev/api/v1/user $ACTING_LOGIN\$" "$AUTH_LOG" assert_no_tea_write +assert_no_temp_leak "approve" # The submitted review payload carries the event and the PR head commit_id. PR_REVIEW_HEAD_SHA="$HEAD_SHA" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY' import json @@ -454,6 +487,8 @@ if grep -q 'Approved and verified' "$OUTPUT_FILE"; then echo "FAIL: read-back did not enforce acting-identity authorship" >&2 exit 1 fi +# Failure-after-read-back path must ALSO leave no scratch temp files behind. +assert_no_temp_leak "author-mismatch-review" # Case 3: a no-op submit with a concurrent SAME-IDENTITY, same-state review at # the current head already present must FAIL CLOSED — the closed concurrency @@ -472,12 +507,16 @@ if grep -q '/pulls/123/reviews/77$' "$CURL_LOG"; then exit 1 fi -# Case 4: a genuine matching review that lands beyond page 1 of the reviews list -# must still be found by the fully-paginating enumeration check. -run_review paginated-approve approve +# Case 4: a genuine matching review (id 51) created after 50 pre-existing reviews +# is still verified by its EXACT provider-returned id — no list enumeration is +# needed regardless of how many reviews precede it. +run_review many-prior-approve approve grep -q 'Approved and verified Gitea PR #123 (review ID 51)' "$OUTPUT_FILE" grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/51$' "$CURL_LOG" -grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=2$' "$CURL_LOG" +if grep -Eq '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews(\?|$)' "$CURL_LOG"; then + echo "FAIL: wrapper performed a redundant review-list enumeration" >&2 + exit 1 +fi # Case 5: an approve WITH a body carries that body in the review submit itself — # there is no separate detached comment POST. @@ -522,6 +561,7 @@ grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/comments/456$' "$CURL_LOG" grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE" assert_no_tea_write +assert_no_temp_leak "comment-success" run_review http-success comment durable-body http://git.mosaicstack.dev grep -q '^POST http://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments$' "$CURL_LOG" @@ -620,4 +660,37 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then exit 1 fi +# Case 10 (#865 Round-5): a --login override that IS present in tea config but +# whose URL is a DIFFERENT host than the repo remote must FAIL CLOSED (host-bound +# selection). The cross-host token must NEVER be sent to the repo host, and no +# review POST occurs. +if run_review cross-host approve "" https://git.mosaicstack.dev \ + https://git.mosaicstack.dev/mosaicstack/stack.git mosaicstack/stack "$CROSS_HOST_LOGIN"; then + echo "FAIL: cross-host --login override did not fail closed" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +if grep -q 'Approved and verified' "$OUTPUT_FILE"; then + echo "FAIL: cross-host --login override reported success" >&2 + exit 1 +fi +# The cross-host credential must not have performed ANY request against the repo +# host — no request may be attributed to the cross-host identity. +if grep -q " $CROSS_HOST_LOGIN\$" "$AUTH_LOG"; then + echo "FAIL: cross-host credential was sent to the repo host (cross-host leak)" >&2 + cat "$AUTH_LOG" >&2 + exit 1 +fi +if grep -qE '^POST .*/pulls/123/reviews ' "$AUTH_LOG"; then + echo "FAIL: cross-host --login override performed a review POST" >&2 + cat "$AUTH_LOG" >&2 + exit 1 +fi +if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then + echo "FAIL: cross-host --login override fell back to the host-default identity" >&2 + cat "$AUTH_LOG" >&2 + exit 1 +fi +assert_no_temp_leak "cross-host" + echo "pr-review.sh REST review + comment create/read-back regression passed"