From 99856c55679c1ed7d65dca19af926df58994bf2f Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Tue, 21 Jul 2026 21:49:59 -0500 Subject: [PATCH] fix(git-tools): scope-aware YAML fallback, port-bound creds, origin-pinned URL + review-body verification (#865) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/mosaic/framework/tools/git/README.md | 4 +- .../framework/tools/git/detect-platform.sh | 108 ++++++++++++--- .../framework/tools/git/issue-comment.sh | 52 +++++-- .../mosaic/framework/tools/git/pr-review.sh | 62 +++++++-- .../tools/git/test-gitea-login-resolution.sh | 128 ++++++++++++++++++ .../tools/git/test-issue-comment-readback.sh | 41 +++++- .../tools/git/test-pr-review-gitea-comment.sh | 76 ++++++++++- 7 files changed, 420 insertions(+), 51 deletions(-) diff --git a/packages/mosaic/framework/tools/git/README.md b/packages/mosaic/framework/tools/git/README.md index 6ffaf45f..4b5e83dd 100644 --- a/packages/mosaic/framework/tools/git/README.md +++ b/packages/mosaic/framework/tools/git/README.md @@ -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. - 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. @@ -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. 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 `. +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- 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 `. 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 2e0a5194..db5b6db8 100755 --- a/packages/mosaic/framework/tools/git/detect-platform.sh +++ b/packages/mosaic/framework/tools/git/detect-platform.sh @@ -599,29 +599,61 @@ config_path = sys.argv[1] 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() - if len(value) >= 2 and value[0] == value[-1] and value[0] in ("'", '"'): - value = value[1:-1] - return value + if not 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: - return None - parsed = urlparse(url if "//" in url else f"//{url}") - host = parsed.hostname - return host.lower() if host else None + return False + configured = urlparse(url if "//" in url else f"//{url}") + remote = urlparse(f"//{repo_host}") + 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): # 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. + # the login's recorded URL host AND port must match it; a login with no + # usable URL (or a mismatched host/port) is rejected (fail closed) so a + # cross-host (or cross-port) credential is never emitted. if not isinstance(token, str) or not token: return None if repo_host: - if _host_of(url) != repo_host: + if not _url_matches_repo_host(url): return None return token @@ -647,11 +679,17 @@ def _token_via_pyyaml(): 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). + # fields). This scan is SCOPE-AWARE: a field is attributed to a login entry + # ONLY when it sits at that entry's own direct-field indentation. A field + # nested inside a deeper sub-map (e.g. `extra:\n token: X`) or a mis-indented + # line is NEVER attached to the entry — exactly the cases where PyYAML resolves + # 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: lines = handle.read().splitlines() @@ -668,19 +706,47 @@ def _token_via_lines(): entries = [] 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:]: 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 + item = re.match(r"^(\s*)-(\s*)(.*)$", line) 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 = {} entries.append(current) - pair = re.match(r"^([A-Za-z0-9_]+)\s*:\s*(.*)$", rest.strip()) - if pair and current is not None: + content = item.group(3) + 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)) for entry in entries: diff --git a/packages/mosaic/framework/tools/git/issue-comment.sh b/packages/mosaic/framework/tools/git/issue-comment.sh index 08fd8738..cbf684ad 100755 --- a/packages/mosaic/framework/tools/git/issue-comment.sh +++ b/packages/mosaic/framework/tools/git/issue-comment.sh @@ -118,6 +118,11 @@ gitea_resolve_api_for_login() { } GITEA_API_ROOT="${configured_url%/}/api/v1" 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 } @@ -230,13 +235,26 @@ PY EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ 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 import json import os import sys 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: with open(sys.argv[1], encoding="utf-8") as response: comment = json.load(response) @@ -247,24 +265,36 @@ try: acting_login = os.environ["ACTING_LOGIN"] slug = os.environ["EXPECTED_REPO_SLUG"] 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 - # carries issue_url = ///issues/ (pull_request_url + # 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("/") + # pull_request_url = ///pulls/ (issue_url empty). + # Pin the returned URL's ORIGIN (scheme+host+port) and its FULL path to this + # provider + repo + kind + number — an endswith/suffix test would accept a + # look-alike host (evil.example/deceptive//issues/N) or a same-host + # decoy prefix (/other//issues/N), so compare the whole thing. + base_origin, base_path = _origin_and_path(web_base) + 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: 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(issue_suffix) or pr_path.endswith(pr_suffix)): - raise ValueError("created comment does not belong to this issue") + if not ( + _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: print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) raise SystemExit(1) diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index 4c7bddb3..d652540d 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -146,13 +146,26 @@ PY EXPECTED_COMMENT_ID="$created_id" EXPECTED_COMMENT_BODY="$comment_body" \ 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 import json import os import sys 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: with open(sys.argv[1], encoding="utf-8") as response: comment = json.load(response) @@ -163,24 +176,36 @@ try: acting_login = os.environ["ACTING_LOGIN"] slug = os.environ["EXPECTED_REPO_SLUG"] number = os.environ["EXPECTED_NUMBER"] + web_base = os.environ["EXPECTED_WEB_BASE"] # Gitea populates WEB (html) URLs here, not API paths. A PR-conversation - # comment carries pull_request_url = ///pulls/ (with + # 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("/") + # issue_url = ///issues/ (with pull_request_url empty). + # Pin the returned URL's ORIGIN (scheme+host+port) and its FULL path to this + # provider + repo + kind + number — an endswith/suffix test would accept a + # look-alike host (evil.example/deceptive//pulls/N) or a same-host + # decoy prefix (/other//pulls/N), so compare the whole thing. + base_origin, base_path = _origin_and_path(web_base) + 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: 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(issue_suffix) or pr_path.endswith(pr_suffix)): - raise ValueError("created comment does not belong to this PR") + if not ( + _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: print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) raise SystemExit(1) @@ -231,6 +256,11 @@ gitea_resolve_api_for_login() { } GITEA_API_ROOT="${configured_url%/}/api/v1" 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 } @@ -386,7 +416,7 @@ PY fi 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 import json import os @@ -401,6 +431,7 @@ try: expected_state = os.environ["EXPECTED_STATE"] acting_login = os.environ["ACTING_LOGIN"] expected_head = os.environ["EXPECTED_HEAD_SHA"] + expected_body = os.environ["EXPECTED_REVIEW_BODY"] if review.get("id") != expected_id: raise ValueError("read-back id does not match the created id") if (review.get("user") or {}).get("login") != acting_login: @@ -409,6 +440,13 @@ try: raise ValueError("created review is not in the expected state") if review.get("commit_id") != expected_head: 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: print(f"Error: Gitea review persistence verification failed: {error}", file=sys.stderr) raise SystemExit(1) diff --git a/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh b/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh index 4eb495a4..fb7d2730 100755 --- a/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh +++ b/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh @@ -312,4 +312,132 @@ if [[ "$override_wins" != "mosaicstack" ]]; then fi 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" 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 a232de43..05d70ba9 100755 --- a/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh +++ b/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh @@ -232,13 +232,25 @@ if mode == "no-op-concurrent": author = foreign if mode == "author-mismatch" else acting 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("//issues/7") test would ACCEPT this. + issue_url = f"https://git.mosaicstack.dev/deceptive/{repo}/issues/7" record = { "id": new_id, "body": body, "user": {"login": author}, - # 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", + "issue_url": issue_url, "pull_request_url": "", } comments.append(record) @@ -491,4 +503,27 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then fi 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" 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 bd2e03de..adef8c2c 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 @@ -233,6 +233,27 @@ if mode == "no-op-concurrent-review": print(json.dumps({})) 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 new_id = (max((r["id"] for r in reviews), default=0)) + 1 record = { @@ -277,24 +298,42 @@ elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/issues/1 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 os +from urllib.parse import urlparse state_path = os.environ["PR_REVIEW_COMMENTS"] acting = os.environ["PR_REVIEW_ACTING_LOGIN"] 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") # 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. +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 # // +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("//pulls/123") test would ACCEPT it. + pr_url = f"{_origin}/deceptive{_slug}/pulls/123" record = { "id": 456, "body": body, "user": {"login": acting}, "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: json.dump([record], handle) @@ -693,4 +732,37 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then fi 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"