fix(git-tools): scope-aware YAML fallback, port-bound creds, origin-pinned URL + review-body verification (#865)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful

Round-6 remediation for PR #866 addressing four cross-host exact-diff audit
blockers (REQUEST_CHANGES governs):

Blocker 1 (detect-platform.sh get_gitea_token_for_login line parser): the
PyYAML-absence fallback attributed any `key: value` at any depth to the current
login, so a token from a nested sub-map or a mis-indented line could be selected
where PyYAML fails closed, and inline comments were not stripped. The fallback is
now scope-aware — a field attaches only at the entry's own direct-field
indentation, only list items at the login list's own dash indent open an entry —
and _strip_scalar strips a trailing inline comment like PyYAML. It is therefore
only ever MORE conservative than PyYAML, never less.

Blocker 2 (detect-platform.sh host bind): credential binding compared
parsed.hostname only, dropping the port, so a :9443 login satisfied a portless
host and a matching :8443 login was rejected. Binding now normalizes scheme +
host + effective port (scheme default applied symmetrically) exactly like
gitea_url_matches_host.

Blocker 3 (issue-comment.sh + pr-review.sh read-back URL check): verification
used path.endswith, accepting a look-alike host or a decoy path prefix. It now
pins the returned issue_url/pull_request_url ORIGIN (scheme+host+effective-port)
and FULL path (deployment prefix + exact owner/repo + kind + number). A new
GITEA_WEB_BASE is exported from gitea_resolve_api_for_login for this.

Blocker 4 (pr-review.sh gitea_submit_review_verified): the submitted review body
was not verified, so a finalized/reused pending review id carrying foreign
Content passed. The persisted body is now bound to the exact submitted body.

Tests: added forced-PyYAML-absence parser-equivalence fixtures (nested sub-map,
sibling, mis-indent, inline comment, tab-indent fail-closed, port match/mismatch)
to test-gitea-login-resolution.sh; URL-forgery fail-closed cases
(wrong-host/owner/repo + prefix injection) to both write suites; and a
reused-review-id body-mismatch case to the pr-review suite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Hermes Agent
2026-07-21 21:49:59 -05:00
parent 2bb3ac4549
commit 99856c5567
7 changed files with 420 additions and 51 deletions

View File

@@ -11,7 +11,7 @@ A successful provider write command—or a wrapper message based only on that co
- Comments (`issue-comment.sh`, and the `comment` action of `pr-review.sh`) `POST /api/v1/repos/{owner}/{repo}/issues/{index}/comments`, requiring a `201` and parsing the created comment's `id` from the response body. - Comments (`issue-comment.sh`, and the `comment` action of `pr-review.sh`) `POST /api/v1/repos/{owner}/{repo}/issues/{index}/comments`, requiring a `201` and parsing the created comment's `id` from the response body.
- Reviews (`approve` / `request-changes`) `POST /api/v1/repos/{owner}/{repo}/pulls/{index}/reviews` with the `event` (`APPROVED` / `REQUEST_CHANGES`), the review `body`, and `commit_id` pinned to the PR's current head, then parse the created review's `id`. The review body travels _in the review submit itself_ — there is no separate detached comment to reconcile (a Gitea `REQUEST_CHANGES` review requires a non-empty body, which the submit carries). - Reviews (`approve` / `request-changes`) `POST /api/v1/repos/{owner}/{repo}/pulls/{index}/reviews` with the `event` (`APPROVED` / `REQUEST_CHANGES`), the review `body`, and `commit_id` pinned to the PR's current head, then parse the created review's `id`. The review body travels _in the review submit itself_ — there is no separate detached comment to reconcile (a Gitea `REQUEST_CHANGES` review requires a non-empty body, which the submit carries).
**Verification keys on that exact provider-returned id.** The wrapper then `GET`s that one record directly — `GET /issues/comments/{id}` or `GET /pulls/{n}/reviews/{id}` — and requires that its `id` equals the created id, its **author login equals the acting identity** (resolved via `GET /api/v1/user` for the token in use), and, for comments, its body exactly matches what was submitted, or, for reviews, its state matches the requested action and its reviewed `commit_id` equals the PR head. The write, the `/user` identity lookup, and the read-back all use the **same** credential — the effective login's token, or the host credential when no login is named — so the write is verified against the identity that actually performed it. **Verification keys on that exact provider-returned id.** The wrapper then `GET`s that one record directly — `GET /issues/comments/{id}` or `GET /pulls/{n}/reviews/{id}` — and requires that its `id` equals the created id, its **author login equals the acting identity** (resolved via `GET /api/v1/user` for the token in use), and, for comments, its body exactly matches what was submitted **and its returned web URL belongs to this exact provider and repository** (the `issue_url` / `pull_request_url` origin — scheme, host, and effective port — and full path, i.e. deployment prefix + exact `owner/repo` + kind + number, must match; a suffix/`endsWith` test would accept a look-alike host or a decoy path prefix, so the whole normalized URL is compared), or, for reviews, its state matches the requested action, its reviewed `commit_id` equals the PR head, **and its persisted body equals the submitted body** (Gitea can finalize/reuse a pending review id whose stored content was authored elsewhere, so the body is bound too). The write, the `/user` identity lookup, and the read-back all use the **same** credential — the effective login's token, or the host credential when no login is named — so the write is verified against the identity that actually performed it.
**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. **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.
@@ -24,6 +24,6 @@ A successful provider write command—or a wrapper message based only on that co
### `--login` override ### `--login` override
Both `pr-review.sh` and `issue-comment.sh` accept an optional `--login <name>` 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 <reviewer-login>`. Both `pr-review.sh` and `issue-comment.sh` accept an optional `--login <name>` 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- and port-bound**: the login's configured URL host **and effective port** (the scheme's default port — 80 for `http`, 443 for `https` — applies when a port is omitted, symmetrically on both sides) must match the repo remote's, so a login name shared across hosts (or an override configured for a different Gitea, including one on a different port of the same host) can never send one host's credential to another — a host or port 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 <reviewer-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. 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.

View File

@@ -599,29 +599,61 @@ config_path = sys.argv[1]
def _strip_scalar(value): def _strip_scalar(value):
# Resolve a YAML flow scalar the way PyYAML would for tea's simple scalars:
# honor surrounding quotes and strip a trailing inline comment. A quoted
# scalar keeps its literal contents (any '#' inside is data, not a comment);
# an unquoted scalar ends at the first whitespace-preceded '#' (a YAML
# comment must be preceded by whitespace or line start), so "abc#def" stays
# literal while "abc # note" becomes "abc".
value = value.strip() value = value.strip()
if len(value) >= 2 and value[0] == value[-1] and value[0] in ("'", '"'): if not value:
value = value[1:-1] return value
return value if value[0] in ("'", '"'):
quote = value[0]
end = value.find(quote, 1)
if end != -1:
return value[1:end]
# Unterminated quote: PyYAML would error; return best-effort remainder so
# the (more conservative) caller still compares against the wanted name.
return value[1:]
for index, char in enumerate(value):
if char == "#" and (index == 0 or value[index - 1] in (" ", "\t")):
value = value[:index]
break
return value.strip()
def _host_of(url): def _url_matches_repo_host(url):
# Mirror gitea_url_matches_host (detect-platform.sh): the login's recorded
# URL must name the SAME host AND the SAME effective port as the repo remote
# host, not merely the same hostname. A login configured for an explicit,
# non-default provider port (e.g. :9443) must NOT satisfy a portless (default
# -port) repo host, and a login on the matching port (e.g. :8443) must NOT be
# rejected. Ports are normalized by applying the login URL's scheme default
# (80 for http, else 443) to whichever side omits the port, symmetrically, so
# an implicit port and its explicit default-port form compare equal.
if not isinstance(url, str) or not url: if not isinstance(url, str) or not url:
return None return False
parsed = urlparse(url if "//" in url else f"//{url}") configured = urlparse(url if "//" in url else f"//{url}")
host = parsed.hostname remote = urlparse(f"//{repo_host}")
return host.lower() if host else None configured_host = configured.hostname
if not configured_host or configured_host.lower() != (remote.hostname or "").lower():
return False
default_port = 80 if configured.scheme == "http" else 443
configured_port = configured.port if configured.port is not None else default_port
remote_port = remote.port if remote.port is not None else default_port
return configured_port == remote_port
def _accept(token, url): def _accept(token, url):
# Enforce the host bind before surfacing a token. When a repo host is given, # 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 # the login's recorded URL host AND port must match it; a login with no
# usable URL (or a mismatched one) is rejected (fail closed) so a cross-host # usable URL (or a mismatched host/port) is rejected (fail closed) so a
# credential is never emitted. # cross-host (or cross-port) credential is never emitted.
if not isinstance(token, str) or not token: if not isinstance(token, str) or not token:
return None return None
if repo_host: if repo_host:
if _host_of(url) != repo_host: if not _url_matches_repo_host(url):
return None return None
return token return token
@@ -647,11 +679,17 @@ def _token_via_pyyaml():
def _token_via_lines(): def _token_via_lines():
# Conservative fallback for hosts without PyYAML. tea writes config.yml in a # 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 # 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 # fields). This scan is SCOPE-AWARE: a field is attributed to a login entry
# would. It only ever returns the `token` of the entry whose `name` EXACTLY # ONLY when it sits at that entry's own direct-field indentation. A field
# equals the requested login AND whose url host matches the repo host, so it # nested inside a deeper sub-map (e.g. `extra:\n token: X`) or a mis-indented
# cannot misattribute to another identity or host; anything it cannot parse # line is NEVER attached to the entry — exactly the cases where PyYAML resolves
# yields None (fail closed). # the entry's own `token` to None (or errors) and thus fails closed. Likewise
# only list items at the login list's own dash indent open a new entry, so a
# nested list item cannot masquerade as a sibling login. It returns the
# `token` of the entry whose `name` EXACTLY equals the requested login AND
# whose url host/port matches the repo host; anything else yields None (fail
# closed). This can only ever be MORE conservative than PyYAML (it never
# selects a token where PyYAML would refuse), never less.
with open(config_path, encoding="utf-8") as handle: with open(config_path, encoding="utf-8") as handle:
lines = handle.read().splitlines() lines = handle.read().splitlines()
@@ -668,19 +706,47 @@ def _token_via_lines():
entries = [] entries = []
current = None current = None
item_indent = None # dash column of the login list's own items
field_indent = None # exact column of the current entry's direct fields
for line in lines[start:]: for line in lines[start:]:
if not line.strip() or line.lstrip().startswith("#"): if not line.strip() or line.lstrip().startswith("#"):
continue continue
indent = len(line) - len(line.lstrip(" ")) indent = len(line) - len(line.lstrip(" "))
if indent <= logins_indent: if indent <= logins_indent:
break break
item = re.match(r"^\s*-\s*(.*)$", line) item = re.match(r"^(\s*)-(\s*)(.*)$", line)
rest = item.group(1) if item else line
if item: if item:
dash_indent = len(item.group(1))
if item_indent is None:
item_indent = dash_indent
if dash_indent != item_indent:
# A more-deeply-indented (nested) or dedented list item: not a
# direct login entry. Ignore it and its scope.
continue
current = {} current = {}
entries.append(current) entries.append(current)
pair = re.match(r"^([A-Za-z0-9_]+)\s*:\s*(.*)$", rest.strip()) content = item.group(3)
if pair and current is not None: if content:
# First field shares this line; its column is the direct-field
# indent for the rest of the entry.
field_indent = dash_indent + 1 + len(item.group(2))
pair = re.match(r"^([A-Za-z0-9_]+)\s*:\s*(.*)$", content)
if pair:
current[pair.group(1)] = _strip_scalar(pair.group(2))
else:
# Bare "-": the first following field line establishes the indent.
field_indent = None
continue
if current is None:
continue
if field_indent is None:
field_indent = indent
if indent != field_indent:
# Deeper => a nested sub-map's field (not this entry's own); anything
# else at an unexpected column is not a direct field. Skip either way.
continue
pair = re.match(r"^([A-Za-z0-9_]+)\s*:\s*(.*)$", line.strip())
if pair:
current[pair.group(1)] = _strip_scalar(pair.group(2)) current[pair.group(1)] = _strip_scalar(pair.group(2))
for entry in entries: for entry in entries:

View File

@@ -118,6 +118,11 @@ gitea_resolve_api_for_login() {
} }
GITEA_API_ROOT="${configured_url%/}/api/v1" GITEA_API_ROOT="${configured_url%/}/api/v1"
GITEA_API_BASE="$GITEA_API_ROOT/repos/$repo" GITEA_API_BASE="$GITEA_API_ROOT/repos/$repo"
# The provider WEB base (scheme + host + effective port + any deployment path
# prefix) that Gitea uses to build a comment's html issue_url/pull_request_url.
# Read-back verification pins the returned URL's origin + path prefix to THIS,
# not just a repo/issue suffix.
GITEA_WEB_BASE="${configured_url%/}"
return 0 return 0
} }
@@ -230,13 +235,26 @@ PY
EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \
ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \ ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \
EXPECTED_NUMBER="$issue_number" \ EXPECTED_NUMBER="$issue_number" EXPECTED_WEB_BASE="$GITEA_WEB_BASE" \
python3 - "$readback_file" <<'PY' || return 1 python3 - "$readback_file" <<'PY' || return 1
import json import json
import os import os
import sys import sys
from urllib.parse import urlparse from urllib.parse import urlparse
def _origin_and_path(url):
# Normalize a URL to (scheme, host, effective-port) + comment path. The port
# defaults to the scheme's default (80 http / 443 otherwise) so an implicit
# port and its explicit default form compare equal.
parsed = urlparse(url or "")
scheme = (parsed.scheme or "").lower()
host = (parsed.hostname or "").lower()
default_port = 80 if scheme == "http" else 443
port = parsed.port if parsed.port is not None else default_port
return (scheme, host, port), parsed.path.rstrip("/")
try: try:
with open(sys.argv[1], encoding="utf-8") as response: with open(sys.argv[1], encoding="utf-8") as response:
comment = json.load(response) comment = json.load(response)
@@ -247,24 +265,36 @@ try:
acting_login = os.environ["ACTING_LOGIN"] acting_login = os.environ["ACTING_LOGIN"]
slug = os.environ["EXPECTED_REPO_SLUG"] slug = os.environ["EXPECTED_REPO_SLUG"]
number = os.environ["EXPECTED_NUMBER"] number = os.environ["EXPECTED_NUMBER"]
web_base = os.environ["EXPECTED_WEB_BASE"]
# Gitea populates WEB (html) URLs here, not API paths. A plain issue comment # Gitea populates WEB (html) URLs here, not API paths. A plain issue comment
# carries issue_url = <app>/<owner>/<repo>/issues/<n> (pull_request_url # carries issue_url = <web_base>/<owner>/<repo>/issues/<n> (pull_request_url
# empty); a comment posted to a PR's conversation carries # empty); a comment posted to a PR's conversation carries
# pull_request_url = <app>/<owner>/<repo>/pulls/<n> (issue_url empty). # pull_request_url = <web_base>/<owner>/<repo>/pulls/<n> (issue_url empty).
# Accept whichever the provider populated — scoped to THIS repo slug and # Pin the returned URL's ORIGIN (scheme+host+port) and its FULL path to this
# number — so a genuine write is never rejected merely for URL shape. # provider + repo + kind + number — an endswith/suffix test would accept a
issue_suffix = f"/{slug}/issues/{number}" # look-alike host (evil.example/deceptive/<slug>/issues/N) or a same-host
pr_suffix = f"/{slug}/pulls/{number}" # decoy prefix (/other/<slug>/issues/N), so compare the whole thing.
issue_path = urlparse(comment.get("issue_url") or "").path.rstrip("/") base_origin, base_path = _origin_and_path(web_base)
pr_path = urlparse(comment.get("pull_request_url") or "").path.rstrip("/") expected_issue_path = f"{base_path}/{slug}/issues/{number}"
expected_pr_path = f"{base_path}/{slug}/pulls/{number}"
def _belongs(url, expected_path):
if not url:
return False
origin, path = _origin_and_path(url)
return origin == base_origin and path == expected_path
if comment.get("id") != expected_id: if comment.get("id") != expected_id:
raise ValueError("read-back id does not match the created id") raise ValueError("read-back id does not match the created id")
if (comment.get("user") or {}).get("login") != acting_login: if (comment.get("user") or {}).get("login") != acting_login:
raise ValueError("created comment is not authored by the acting identity") raise ValueError("created comment is not authored by the acting identity")
if comment.get("body") != expected_body: if comment.get("body") != expected_body:
raise ValueError("created comment body does not match") raise ValueError("created comment body does not match")
if not (issue_path.endswith(issue_suffix) or pr_path.endswith(pr_suffix)): if not (
raise ValueError("created comment does not belong to this issue") _belongs(comment.get("issue_url"), expected_issue_path)
or _belongs(comment.get("pull_request_url"), expected_pr_path)
):
raise ValueError("created comment does not belong to this issue on this provider/repo")
except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error:
print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr)
raise SystemExit(1) raise SystemExit(1)

View File

@@ -146,13 +146,26 @@ PY
EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \
ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \ ACTING_LOGIN="$acting_login" EXPECTED_REPO_SLUG="${GITEA_API_BASE##*/repos/}" \
EXPECTED_NUMBER="$pr_number" \ EXPECTED_NUMBER="$pr_number" EXPECTED_WEB_BASE="$GITEA_WEB_BASE" \
python3 - "$readback_file" <<'PY' || return 1 python3 - "$readback_file" <<'PY' || return 1
import json import json
import os import os
import sys import sys
from urllib.parse import urlparse from urllib.parse import urlparse
def _origin_and_path(url):
# Normalize a URL to (scheme, host, effective-port) + comment path. The port
# defaults to the scheme's default (80 http / 443 otherwise) so an implicit
# port and its explicit default form compare equal.
parsed = urlparse(url or "")
scheme = (parsed.scheme or "").lower()
host = (parsed.hostname or "").lower()
default_port = 80 if scheme == "http" else 443
port = parsed.port if parsed.port is not None else default_port
return (scheme, host, port), parsed.path.rstrip("/")
try: try:
with open(sys.argv[1], encoding="utf-8") as response: with open(sys.argv[1], encoding="utf-8") as response:
comment = json.load(response) comment = json.load(response)
@@ -163,24 +176,36 @@ try:
acting_login = os.environ["ACTING_LOGIN"] acting_login = os.environ["ACTING_LOGIN"]
slug = os.environ["EXPECTED_REPO_SLUG"] slug = os.environ["EXPECTED_REPO_SLUG"]
number = os.environ["EXPECTED_NUMBER"] number = os.environ["EXPECTED_NUMBER"]
web_base = os.environ["EXPECTED_WEB_BASE"]
# Gitea populates WEB (html) URLs here, not API paths. A PR-conversation # Gitea populates WEB (html) URLs here, not API paths. A PR-conversation
# comment carries pull_request_url = <app>/<owner>/<repo>/pulls/<n> (with # comment carries pull_request_url = <web_base>/<owner>/<repo>/pulls/<n> (with
# issue_url empty), while a plain issue comment carries # issue_url empty), while a plain issue comment carries
# issue_url = <app>/<owner>/<repo>/issues/<n> (with pull_request_url empty). # issue_url = <web_base>/<owner>/<repo>/issues/<n> (with pull_request_url empty).
# Accept whichever the provider populated — scoped to THIS repo slug and # Pin the returned URL's ORIGIN (scheme+host+port) and its FULL path to this
# number — so a genuine write is never rejected merely for URL shape. # provider + repo + kind + number — an endswith/suffix test would accept a
issue_suffix = f"/{slug}/issues/{number}" # look-alike host (evil.example/deceptive/<slug>/pulls/N) or a same-host
pr_suffix = f"/{slug}/pulls/{number}" # decoy prefix (/other/<slug>/pulls/N), so compare the whole thing.
issue_path = urlparse(comment.get("issue_url") or "").path.rstrip("/") base_origin, base_path = _origin_and_path(web_base)
pr_path = urlparse(comment.get("pull_request_url") or "").path.rstrip("/") expected_issue_path = f"{base_path}/{slug}/issues/{number}"
expected_pr_path = f"{base_path}/{slug}/pulls/{number}"
def _belongs(url, expected_path):
if not url:
return False
origin, path = _origin_and_path(url)
return origin == base_origin and path == expected_path
if comment.get("id") != expected_id: if comment.get("id") != expected_id:
raise ValueError("read-back id does not match the created id") raise ValueError("read-back id does not match the created id")
if (comment.get("user") or {}).get("login") != acting_login: if (comment.get("user") or {}).get("login") != acting_login:
raise ValueError("created comment is not authored by the acting identity") raise ValueError("created comment is not authored by the acting identity")
if comment.get("body") != expected_body: if comment.get("body") != expected_body:
raise ValueError("created comment body does not match") raise ValueError("created comment body does not match")
if not (issue_path.endswith(issue_suffix) or pr_path.endswith(pr_suffix)): if not (
raise ValueError("created comment does not belong to this PR") _belongs(comment.get("issue_url"), expected_issue_path)
or _belongs(comment.get("pull_request_url"), expected_pr_path)
):
raise ValueError("created comment does not belong to this PR on this provider/repo")
except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error:
print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr)
raise SystemExit(1) raise SystemExit(1)
@@ -231,6 +256,11 @@ gitea_resolve_api_for_login() {
} }
GITEA_API_ROOT="${configured_url%/}/api/v1" GITEA_API_ROOT="${configured_url%/}/api/v1"
GITEA_API_BASE="$GITEA_API_ROOT/repos/$repo" GITEA_API_BASE="$GITEA_API_ROOT/repos/$repo"
# The provider WEB base (scheme + host + effective port + any deployment path
# prefix) that Gitea uses to build a comment's html issue_url/pull_request_url.
# Read-back verification pins the returned URL's origin + path prefix to THIS,
# not just a repo/PR suffix.
GITEA_WEB_BASE="${configured_url%/}"
return 0 return 0
} }
@@ -386,7 +416,7 @@ PY
fi fi
EXPECTED_REVIEW_ID="$created_id" EXPECTED_STATE="$event" ACTING_LOGIN="$acting_login" \ EXPECTED_REVIEW_ID="$created_id" EXPECTED_STATE="$event" ACTING_LOGIN="$acting_login" \
EXPECTED_HEAD_SHA="$head_sha" \ EXPECTED_HEAD_SHA="$head_sha" EXPECTED_REVIEW_BODY="$review_body" \
python3 - "$readback_file" <<'PY' || return 1 python3 - "$readback_file" <<'PY' || return 1
import json import json
import os import os
@@ -401,6 +431,7 @@ try:
expected_state = os.environ["EXPECTED_STATE"] expected_state = os.environ["EXPECTED_STATE"]
acting_login = os.environ["ACTING_LOGIN"] acting_login = os.environ["ACTING_LOGIN"]
expected_head = os.environ["EXPECTED_HEAD_SHA"] expected_head = os.environ["EXPECTED_HEAD_SHA"]
expected_body = os.environ["EXPECTED_REVIEW_BODY"]
if review.get("id") != expected_id: if review.get("id") != expected_id:
raise ValueError("read-back id does not match the created id") raise ValueError("read-back id does not match the created id")
if (review.get("user") or {}).get("login") != acting_login: if (review.get("user") or {}).get("login") != acting_login:
@@ -409,6 +440,13 @@ try:
raise ValueError("created review is not in the expected state") raise ValueError("created review is not in the expected state")
if review.get("commit_id") != expected_head: if review.get("commit_id") != expected_head:
raise ValueError("created review is not pinned to the PR head commit") raise ValueError("created review is not pinned to the PR head commit")
# Bind to the exact submitted body. On Gitea v1.25.4 SubmitReview may
# finalize/reuse a pending review id whose Content was authored elsewhere;
# the exact GET exposes the persisted body, so a mismatch (a reused/foreign
# review carrying different Content) fails closed even when id/author/state/
# head all line up.
if (review.get("body") or "") != expected_body:
raise ValueError("created review body does not match the submitted body")
except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error:
print(f"Error: Gitea review persistence verification failed: {error}", file=sys.stderr) print(f"Error: Gitea review persistence verification failed: {error}", file=sys.stderr)
raise SystemExit(1) raise SystemExit(1)

View File

@@ -312,4 +312,132 @@ if [[ "$override_wins" != "mosaicstack" ]]; then
fi fi
git -C "$REPO_DIR" remote set-url origin https://git.uscllc.com/USC/uconnect.git git -C "$REPO_DIR" remote set-url origin https://git.uscllc.com/USC/uconnect.git
# ---------------------------------------------------------------------------
# #865 Blocker 1 & 2: get_gitea_token_for_login must resolve the SAME token as
# PyYAML would (or fail closed identically) even when PyYAML is ABSENT, and must
# bind the credential to the repo host's scheme + host + EFFECTIVE PORT — not the
# hostname alone. These fixtures probe the ImportError-dispatched line-parser
# fallback under FORCED PyYAML absence with adversarial YAML shapes, asserting it
# NEVER misattributes a token from a nested sub-map or a mis-indented line, strips
# inline comments like PyYAML, fails closed where PyYAML errors, and rejects a
# port mismatch while accepting an exact / default-port match. When PyYAML is
# available the same fixtures also assert the PyYAML path agrees (equivalence).
# ---------------------------------------------------------------------------
FIXTURE_XDG="$WORK_DIR/tokenfix"
NOYAML_DIR="$WORK_DIR/noyaml"
mkdir -p "$FIXTURE_XDG/tea" "$NOYAML_DIR"
# A shadow `yaml` module that raises ImportError, forcing the fallback path.
printf 'raise ImportError("forced-absent for #865 fallback regression")\n' > "$NOYAML_DIR/yaml.py"
if python3 -c 'import yaml' >/dev/null 2>&1; then HAVE_PYYAML=true; else HAVE_PYYAML=false; fi
# Confirm the shim really does force ImportError, so the fallback is exercised.
if python3 -c 'import yaml' >/dev/null 2>&1; then
if PYTHONPATH="$NOYAML_DIR" python3 -c 'import yaml' >/dev/null 2>&1; then
echo "FAIL: PyYAML-absence shim did not force ImportError (fallback not exercised)" >&2
exit 1
fi
fi
write_fixture() { printf '%s' "$1" > "$FIXTURE_XDG/tea/config.yml"; }
# Resolve a token via the FORCED-fallback path (PyYAML shimmed to ImportError).
token_fallback() {
(
cd "$REPO_DIR"
XDG_CONFIG_HOME="$FIXTURE_XDG" PYTHONPATH="$NOYAML_DIR" bash -c '
source "'"$SCRIPT_DIR"'/detect-platform.sh"
get_gitea_token_for_login "$1" "$2"
' _ "$1" "$2"
) 2>/dev/null || true
}
# Resolve a token via the normal path (uses PyYAML when installed).
token_pyyaml() {
(
cd "$REPO_DIR"
XDG_CONFIG_HOME="$FIXTURE_XDG" bash -c '
source "'"$SCRIPT_DIR"'/detect-platform.sh"
get_gitea_token_for_login "$1" "$2"
' _ "$1" "$2"
) 2>/dev/null || true
}
assert_token() {
local desc="$1" expected="$2" login="$3" host="$4" got
got=$(token_fallback "$login" "$host")
if [[ "$got" != "$expected" ]]; then
echo "FAIL fallback [$desc]: expected [$expected] got [$got]" >&2
exit 1
fi
if [[ "$HAVE_PYYAML" == true ]]; then
got=$(token_pyyaml "$login" "$host")
if [[ "$got" != "$expected" ]]; then
echo "FAIL pyyaml [$desc]: expected [$expected] got [$got]" >&2
exit 1
fi
fi
}
# 1. Plain, well-formed entry resolves its token.
write_fixture 'logins:
- name: primary
url: https://git.example
token: TOK_PLAIN
'
assert_token "plain scalar" "TOK_PLAIN" primary git.example
# 2. A token nested inside a deeper SUB-MAP must NOT attach to the entry — PyYAML
# resolves the entry's own token to None here, so the fallback must too.
write_fixture 'logins:
- name: primary
url: https://git.example
extra:
token: TOK_NESTED_ATTACKER
- name: other
url: https://git.example
token: TOK_OTHER
'
assert_token "nested sub-map token is not attributed" "" primary git.example
assert_token "sibling entry still resolves its own token" "TOK_OTHER" other git.example
# 3. A MIS-INDENTED token line (deeper than the entry's fields) must not attach;
# PyYAML errors on this shape, so both fail closed.
write_fixture 'logins:
- name: primary
url: https://git.example
token: TOK_MISINDENT
'
assert_token "mis-indented token fails closed" "" primary git.example
# 4. A trailing inline comment on a scalar is stripped, exactly as PyYAML does.
write_fixture 'logins:
- name: primary
url: https://git.example
token: TOK_INLINE # trailing note
'
assert_token "inline comment stripped" "TOK_INLINE" primary git.example
# 5. A PyYAML-fail-closed case: tab indentation. PyYAML raises a scanner error;
# the fallback resolves no token. Both fail closed identically.
write_fixture "$(printf 'logins:\n - name: primary\n url: https://git.example\n\ttoken: TOK_TAB\n')"
assert_token "tab-indent fails closed like PyYAML" "" primary git.example
# 6. Host binding is scheme + host + EFFECTIVE PORT, not hostname alone.
write_fixture 'logins:
- name: ported
url: https://git.example:8443
token: TOK_PORTED
'
assert_token "explicit port exact match accepted" "TOK_PORTED" ported git.example:8443
assert_token "portless repo host rejects :8443 login" "" ported git.example
assert_token "wrong explicit port rejected" "" ported git.example:9443
# 7. An implicit (portless) login URL equals the scheme's explicit default port.
write_fixture 'logins:
- name: defported
url: https://git.example
token: TOK_DEFPORT
'
assert_token "implicit https vs explicit :443 match" "TOK_DEFPORT" defported git.example:443
assert_token "implicit https vs :8443 rejected" "" defported git.example:8443
echo "Gitea login resolution regression harness passed" echo "Gitea login resolution regression harness passed"

View File

@@ -232,13 +232,25 @@ if mode == "no-op-concurrent":
author = foreign if mode == "author-mismatch" else acting author = foreign if mode == "author-mismatch" else acting
new_id = (max((c["id"] for c in comments), default=0)) + 1 new_id = (max((c["id"] for c in comments), default=0)) + 1
# 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. The URL-injection
# modes persist a record whose id/author/body are all correct but whose
# issue_url is forged, so ONLY the origin+path verification can catch them.
issue_url = f"https://git.mosaicstack.dev/{repo}/issues/7"
if mode == "url-wrong-host":
issue_url = f"https://evil.example/{repo}/issues/7"
elif mode == "url-wrong-owner":
issue_url = "https://git.mosaicstack.dev/attacker/stack/issues/7"
elif mode == "url-wrong-repo":
issue_url = "https://git.mosaicstack.dev/mosaicstack/other/issues/7"
elif mode == "url-suffix-injection":
# Prefix-injected: a bare endswith("/<slug>/issues/7") test would ACCEPT this.
issue_url = f"https://git.mosaicstack.dev/deceptive/{repo}/issues/7"
record = { record = {
"id": new_id, "id": new_id,
"body": body, "body": body,
"user": {"login": author}, "user": {"login": author},
# REAL Gitea comment shape: issue_url is the WEB (html) path, not an API "issue_url": issue_url,
# path, and a plain issue comment leaves pull_request_url empty.
"issue_url": f"https://git.mosaicstack.dev/{repo}/issues/7",
"pull_request_url": "", "pull_request_url": "",
} }
comments.append(record) comments.append(record)
@@ -491,4 +503,27 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
fi fi
assert_no_temp_leak "cross-host" assert_no_temp_leak "cross-host"
# Cases 7-10 (#865 Blocker 3): the created record's id/author/body are all
# correct, but its provider-returned issue_url is forged. Verification pins the
# URL's ORIGIN (scheme+host+effective-port) and its FULL path (deployment prefix
# + exact owner/repo + kind + number), so each forgery must FAIL CLOSED. A bare
# endswith/suffix test would wrongly accept the look-alike-host and
# prefix-injection variants.
for bad_mode in url-wrong-host url-wrong-owner url-wrong-repo url-suffix-injection; do
if run_comment "$bad_mode"; then
echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2
cat "$OUTPUT_FILE" >&2
exit 1
fi
if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then
echo "FAIL: forged comment URL ($bad_mode) passed verification" >&2
exit 1
fi
assert_no_temp_leak "$bad_mode"
done
# Sanity: the exact same verification path still ACCEPTS a legitimate web-shaped
# issue_url (already exercised by Case 1's fresh-success), so the tightened check
# is not rejecting genuine writes.
echo "issue-comment.sh REST create + exact-id read-back regression passed" echo "issue-comment.sh REST create + exact-id read-back regression passed"

View File

@@ -233,6 +233,27 @@ if mode == "no-op-concurrent-review":
print(json.dumps({})) print(json.dumps({}))
raise SystemExit(0) raise SystemExit(0)
# review-body-reuse (#865 Blocker 4): Gitea v1.25.4's SubmitReview can finalize
# and REUSE a pending review id whose Content was authored earlier — NOT this
# submit's body. id/author/state/head all line up with the request; only the
# persisted body diverges, so only body verification catches it. The read-back
# GET returns this same divergent-body record.
if mode == "review-body-reuse":
new_id = (max((r["id"] for r in reviews), default=0)) + 1
record = {
"id": new_id,
"state": submitted.get("event"),
"commit_id": submitted.get("commit_id"),
"body": "leftover-pending-content-not-this-submit",
"user": {"login": acting},
}
reviews.append(record)
with open(state_path, "w", encoding="utf-8") as handle:
json.dump(reviews, handle)
print("201")
print(json.dumps(record))
raise SystemExit(0)
author = foreign if mode == "author-mismatch-review" else acting author = foreign if mode == "author-mismatch-review" else acting
new_id = (max((r["id"] for r in reviews), default=0)) + 1 new_id = (max((r["id"] for r in reviews), default=0)) + 1
record = { record = {
@@ -277,24 +298,42 @@ elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/issues/1
write_response 500 '{"message":"simulated rejection"}' write_response 500 '{"message":"simulated rejection"}'
;; ;;
*) *)
emit "$(PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY' emit "$(PR_REVIEW_PAYLOAD="$payload" PR_REVIEW_TEST_MODE="$mode" python3 - <<'PY'
import json import json
import os import os
from urllib.parse import urlparse
state_path = os.environ["PR_REVIEW_COMMENTS"] state_path = os.environ["PR_REVIEW_COMMENTS"]
acting = os.environ["PR_REVIEW_ACTING_LOGIN"] acting = os.environ["PR_REVIEW_ACTING_LOGIN"]
web_base = os.environ["PR_REVIEW_WEB_BASE"] web_base = os.environ["PR_REVIEW_WEB_BASE"]
mode = os.environ.get("PR_REVIEW_TEST_MODE", "")
body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body") body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body")
# REAL Gitea shape for a comment posted to a PR's conversation # 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 # (/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 # issue_url is left empty. This is what the wrapper must tolerate — it must NOT
# require an API-shaped issue_url. # require an API-shaped issue_url.
pr_url = f"{web_base}/pulls/123"
# URL-injection modes (#865 Blocker 3): id/author/body are all correct but the
# provider-returned pull_request_url is forged, so ONLY origin+full-path
# verification can catch them.
_p = urlparse(web_base)
_origin = f"{_p.scheme}://{_p.netloc}"
_slug = _p.path # /<owner>/<repo>
if mode == "comment-url-wrong-host":
pr_url = f"https://evil.example{_slug}/pulls/123"
elif mode == "comment-url-wrong-owner":
pr_url = f"{_origin}/attacker/stack/pulls/123"
elif mode == "comment-url-wrong-repo":
pr_url = f"{_origin}/mosaicstack/other/pulls/123"
elif mode == "comment-url-suffix-injection":
# Prefix-injected: a bare endswith("/<slug>/pulls/123") test would ACCEPT it.
pr_url = f"{_origin}/deceptive{_slug}/pulls/123"
record = { record = {
"id": 456, "id": 456,
"body": body, "body": body,
"user": {"login": acting}, "user": {"login": acting},
"issue_url": "", "issue_url": "",
"pull_request_url": f"{web_base}/pulls/123", "pull_request_url": pr_url,
} }
with open(state_path, "w", encoding="utf-8") as handle: with open(state_path, "w", encoding="utf-8") as handle:
json.dump([record], handle) json.dump([record], handle)
@@ -693,4 +732,37 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
fi fi
assert_no_temp_leak "cross-host" assert_no_temp_leak "cross-host"
# Case 11 (#865 Blocker 4): SubmitReview finalizes/reuses a pending review id
# whose persisted body is NOT this submit's body. id/author/state/head all match
# the request, so ONLY body verification can catch the divergence — it must FAIL
# CLOSED. (Submit a non-empty body so the mismatch is meaningful.)
if run_review review-body-reuse approve real-submitted-review-body; then
echo "FAIL: review with a reused/foreign body was accepted (body not verified)" >&2
cat "$OUTPUT_FILE" >&2
exit 1
fi
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
echo "FAIL: read-back did not enforce the submitted review body" >&2
exit 1
fi
assert_no_temp_leak "review-body-reuse"
# Cases 12-15 (#865 Blocker 3): a PR comment whose id/author/body are all correct
# but whose provider-returned pull_request_url is forged must FAIL CLOSED.
# Verification pins the URL's ORIGIN (scheme+host+effective-port) and FULL path
# (deployment prefix + exact owner/repo + kind + number); a bare endswith/suffix
# test would wrongly accept the look-alike-host and prefix-injection variants.
for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong-repo comment-url-suffix-injection; do
if run_review "$bad_mode" comment durable-body; then
echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2
cat "$OUTPUT_FILE" >&2
exit 1
fi
if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then
echo "FAIL: forged comment URL ($bad_mode) passed verification" >&2
exit 1
fi
assert_no_temp_leak "$bad_mode"
done
echo "pr-review.sh REST review + comment create/read-back regression passed" echo "pr-review.sh REST review + comment create/read-back regression passed"