From 4822291707bc2aebd4b2cf37de99f4d0ddc35403 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Tue, 21 Jul 2026 23:01:21 -0500 Subject: [PATCH] =?UTF-8?q?fix(git-wrappers):=20#865=20round-7=20addendum?= =?UTF-8?q?=20=E2=80=94=20conservative=20YAML=20recognizer=20+=20review/co?= =?UTF-8?q?mment=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold the remaining round-7 audit items into the tea-CLI comment-invocation fix. Fallback token parser (detect-platform.sh, test-gitea-login-resolution.sh): Replace the line-by-line scalar fallback with a strict CONSERVATIVE block-YAML recognizer that reconstructs the same object PyYAML would or fails closed the instant it meets anything outside the tea-config subset. Closes 5 structural fail-open classes the old parser missed (nested-shadow logins, block-scalar shadow, duplicate root key / login name / token field, malformed-after-valid, extra-document / end-marker). Validated by a 360k-check differential fuzz vs real PyYAML (0 fail-open) plus explicit forced-PyYAML-absence fixtures. ITEM 1 (pr-review.sh) current-head TOCTOU: after the exact review-id read-back succeeds, re-read the live PR head and fail closed if it advanced past the submitted commit_id, so a review is never reported as covering a superseded tip. ITEM 2 (pr-review.sh comment action): require the returned resource be a pull_request (populated pull_request_url); reject a bare issue_url so a plain issue #N cannot masquerade as a verified PR comment. issue-comment.sh keeps its broader issue-or-PR acceptance. ITEM 3a (both wrappers): move the Authorization bearer OUT of curl argv into a private mode-0600 curl --config file (gitea_write_auth_config), removed on every exit path, so the token never appears in the process table. ITEM 3b (pr-review.sh): bind the review body with presence + string-type + exact equality instead of `(body or "")`, so a non-empty submitted body persisted as null/missing fails closed. Tests: add race, plain-issue, argv-capture (no token printed), and null-body fixtures; broaden temp-leak checks to the auth-config files. Full gate set green (bash -n, shellcheck -x -S warning, prettier, all 3 REST/resolution suites with PyYAML and forced-absent, cold TURBO_FORCE turbo 14/14). Co-Authored-By: Claude Opus 4.8 --- packages/mosaic/framework/tools/git/README.md | 8 +- .../framework/tools/git/detect-platform.sh | 379 +++++++++++++----- .../framework/tools/git/issue-comment.sh | 24 +- .../mosaic/framework/tools/git/pr-review.sh | 121 ++++-- .../tools/git/test-gitea-login-resolution.sh | 144 +++++++ .../tools/git/test-issue-comment-readback.sh | 44 +- .../tools/git/test-pr-review-gitea-comment.sh | 161 +++++++- 7 files changed, 740 insertions(+), 141 deletions(-) diff --git a/packages/mosaic/framework/tools/git/README.md b/packages/mosaic/framework/tools/git/README.md index 4b5e83dd..74f15b55 100644 --- a/packages/mosaic/framework/tools/git/README.md +++ b/packages/mosaic/framework/tools/git/README.md @@ -11,12 +11,18 @@ 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 **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. +**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). The `comment` action of `pr-review.sh` additionally requires the returned resource be a **pull request** (a populated `pull_request_url`); a bare `issue_url` is rejected, so if issue `#N` exists but PR `#N` does not, an issue comment cannot be reported as a verified PR comment. (`issue-comment.sh` legitimately keeps the broader issue-or-PR acceptance.) For reviews, its state matches the requested action, its reviewed `commit_id` equals the PR head, **and its persisted body equals the submitted body** — an exact, presence- and type-checked equality (a missing/`null` persisted body no longer counts as an empty match), because 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. + +**A review's pinned head is re-checked after verification (current-head TOCTOU).** The `commit_id` is pinned to the PR head read _before_ the submit; between that read and the read-back the branch could advance (a force-push or a new commit), leaving a verified review attached to a now-superseded commit while the live tip carries unreviewed code. After the exact-id read-back succeeds, the wrapper re-reads the live PR head (`GET …/pulls/{n}`) and requires it still equals the submitted SHA; if the head advanced it fails closed (non-zero, no success line) rather than reporting a review that no longer covers the PR's current commit. **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. **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`. +## Credential handling + +The Gitea API token is **never passed on a curl command line.** An `Authorization: token ` argument would be visible to any local process that can read the process table (`ps` / `/proc//cmdline`) for the lifetime of the request. Instead, every authenticated curl call writes the header into a private, mode-`0600` config file under `$TMPDIR` and passes it with `curl --config ` (`gitea_write_auth_config`), so only the file _path_ — never the token — appears in argv. Each such file is unlinked on every exit path (success and failure) by the caller's `RETURN` trap. + ## `tea` invocation notes (Gitea) - tea v0.11.1 has **no `comment` subcommand under `tea pr` or `tea issue`** — the `tea pr comment` / `tea issue comment` forms don't error, they silently fall through to a no-op and still exit 0, producing a false-success write (#865). tea's write subcommands (`tea comment`, `tea pr approve`/`reject`) also cannot report the id of the record they create, so their exit code cannot prove a durable write. These wrappers therefore do **not** write reviews or comments through `tea` at all; they use direct Gitea REST `POST`s that return the created record's id (see "Durable review provenance" above). `tea` is consulted only to enumerate the login list for host→login resolution. diff --git a/packages/mosaic/framework/tools/git/detect-platform.sh b/packages/mosaic/framework/tools/git/detect-platform.sh index b2d3179a..5ea889a7 100755 --- a/packages/mosaic/framework/tools/git/detect-platform.sh +++ b/packages/mosaic/framework/tools/git/detect-platform.sh @@ -563,6 +563,30 @@ get_gitea_token() { return 1 } +# Stage the Gitea bearer credential for curl OUTSIDE the process argument vector. +# Passing "-H 'Authorization: token '" on the curl command line exposes +# the token to anyone who can read the process table (ps / /proc//cmdline) +# for the lifetime of the request. Instead, write the header into a private +# (mode 0600) curl config file and have callers pass it with `curl --config`, so +# only the FILE PATH — never the token — appears in argv. Prints the temp file +# path on success; the caller OWNS the file and MUST remove it on every exit +# path (success and failure). $1 = bearer token. Callers must not log the token. +gitea_write_auth_config() { + local token="$1" auth_file + auth_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-gitea-auth.XXXXXX") || return 1 + # mktemp already creates the file with 0600; be explicit in case of an + # unusual umask so the credential is never briefly group/other readable. + chmod 600 "$auth_file" 2>/dev/null || true + # `header = "..."` is curl's config syntax for an extra request header. Only + # this filename reaches curl's argv; the token stays on disk, readable solely + # by this user, and is unlinked by the caller's trap after the request. + if ! printf 'header = "Authorization: token %s"\n' "$token" > "$auth_file"; then + rm -f "$auth_file" + return 1 + fi + printf '%s' "$auth_file" +} + # Resolve the API token for a SPECIFIC tea login name from tea's own config # (the same store tea itself writes/reads for `--login `). This is what # lets a REST write be performed AS the selected --login identity: tea keys its @@ -647,46 +671,255 @@ def _implicit_nonstring(text): ) -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". +# Sentinel: "outside the supported subset -> fail closed". Distinct from a +# genuine null (None), which is a valid resolved value. +_FAIL = object() +_KEY_RE = re.compile(r"^([A-Za-z0-9_][A-Za-z0-9_.\-]*)[ \t]*:(?:[ \t](.*)|)$") +_FLOW = set("[]{}*&!") + + +class _Bail(Exception): + # Raised the instant the document leaves the narrow tea-config subset this + # recognizer can prove it resolves IDENTICALLY to PyYAML. Caught by + # _safe_parse, which then fails closed (returns _FAIL) rather than guess. + pass + + +def _scalar(raw): + # Resolve a single flow scalar (quoted or plain) the way PyYAML would for + # tea's simple values, or _FAIL when it is outside the supported subset so + # the caller fails closed instead of guessing. # - # A quoted scalar is ALWAYS a string, so its contents are returned verbatim. - # An UNQUOTED scalar, however, is subject to PyYAML's implicit typing: forms - # like `12345`, `null`, `~`, `yes`/`true`, `3.14` or a timestamp resolve to a - # non-string (int/None/bool/float/date), which PyYAML's path would reject as - # a token via _accept's isinstance(str) guard. To stay faithful (and never - # fail OPEN by surfacing a stringified non-string as a credential) such a - # scalar returns None here so the caller treats it as absent (fail closed), - # exactly as the PyYAML path does. Only unquoted scalars PyYAML would type as - # a string are returned as a string. - value = value.strip() + # A quoted scalar is ALWAYS a string (its contents returned verbatim); a '#' + # inside quotes is data. 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"), and is then + # subject to PyYAML's implicit typing: forms like 12345 / null / ~ / yes / + # 3.14 / a timestamp resolve to a NON-string (int/None/bool/float/date), so + # they return None here (PyYAML's path rejects a non-str token via _accept). + value = raw.strip() if not value: - return value + return None # empty plain scalar -> null 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] + if end == -1: + return _FAIL # unterminated quote: PyYAML would error / continue + rest = value[end + 1:].strip() + if rest and not rest.startswith("#"): + return _FAIL # trailing junk after a quoted scalar + inner = value[1:end] + # Single-quote '' escaping and double-quote backslash escapes are NOT + # interpreted here; reject any scalar that uses them so we never diverge + # from PyYAML on escape handling. + if quote == "'" and "'" in inner: + return _FAIL + if quote == '"' and "\\" in inner: + return _FAIL + return inner + for i, ch in enumerate(value): + if ch == "#" and (i == 0 or value[i - 1] in (" ", "\t")): + value = value[:i] break value = value.strip() - if not value or _implicit_nonstring(value): - # Unquoted scalar that PyYAML resolves to null or another non-string - # type: fail closed rather than emit a stringified non-credential. + if not value: return None + if value[0] in _FLOW or value[0] in ("|", ">", "?", "@", "`", '"', "'"): + return _FAIL # flow / block-scalar / reserved / anchor / quote indicator + if any(ch in _FLOW for ch in value): + return _FAIL + if ": " in value or value.endswith(":") or "\t" in value: + return _FAIL # nested-mapping-in-scalar / ambiguous + if _implicit_nonstring(value): + return None # non-string implicit type -> null-equivalent for a token return value +class _Parser: + # A deliberately NARROW, conservative recognizer for the block-style YAML + # subset tea writes (mappings of scalar fields; a `logins:` block SEQUENCE of + # such mappings; optional shallow nested mappings for e.g. preferences). It + # reconstructs the SAME Python object PyYAML's SafeLoader would, but the + # instant it meets anything it cannot prove it handles identically -- a + # document marker (--- / ...), a block scalar (| / >), a flow collection, a + # duplicate mapping key, inconsistent indentation, an escape, or any line + # outside the grammar -- it raises _Bail so the whole resolution fails + # closed. This guarantees the module invariant (only ever MORE conservative + # than PyYAML, never less) at the DOCUMENT level, closing the structural + # fail-open classes (nested-logins shadow, block-scalar shadow, duplicate + # root key, malformed-after-valid, and extra-document) that a line scan that + # does not validate whole-document structure would miss. + def __init__(self, lines): + self.toks = [] + for raw in lines: + if not raw.strip(): + continue + lead = raw[: len(raw) - len(raw.lstrip(" \t"))] + if "\t" in lead: + raise _Bail() # tab in indentation: PyYAML scanner error + indent = len(lead) + body = raw[indent:] + if body.lstrip().startswith("#"): + continue + if re.match(r"^(---|\.\.\.)(\s|$)", body) or body in ("---", "..."): + raise _Bail() # document / end marker -> multi-doc -> fail closed + self.toks.append((indent, body.rstrip())) + self.i = 0 + + def peek(self): + return self.toks[self.i] if self.i < len(self.toks) else None + + def parse_document(self): + if not self.toks: + return None + node = self.parse_node(0) + if self.i != len(self.toks): + raise _Bail() # trailing unconsumed content -> malformed + return node + + def parse_node(self, min_indent): + tok = self.peek() + if tok is None: + return None + indent, body = tok + if indent < min_indent: + return None + if body.startswith("-") and (len(body) == 1 or body[1] in (" ", "\t")): + return self.parse_seq(indent) + return self.parse_map(indent) + + def parse_map(self, indent): + result = {} + while True: + tok = self.peek() + if tok is None: + break + cur_indent, body = tok + if cur_indent < indent: + break + if cur_indent > indent: + raise _Bail() # unexpected deeper line (bad indentation) + if body.startswith("-") and (len(body) == 1 or body[1] in (" ", "\t")): + raise _Bail() # sequence item where a mapping entry was expected + m = _KEY_RE.match(body) + if not m: + raise _Bail() + key = m.group(1) + inline = m.group(2) + self.i += 1 + if key in result: + raise _Bail() # duplicate mapping key (PyYAML last-wins; we bail) + if inline is not None and inline.strip() != "": + val = _scalar(inline) + if val is _FAIL: + raise _Bail() + result[key] = val + else: + result[key] = self.parse_block_value(indent) + return result + + def parse_block_value(self, key_indent): + # The value after a "key:" with no inline scalar. A block SEQUENCE may sit + # at the same indent as the key (YAML permits `- ` aligned with the key -- + # tea's own on-disk shape) or deeper; a block MAPPING must be strictly + # deeper; otherwise the value is null. + nxt = self.peek() + if nxt is None: + return None + ni, nb = nxt + is_item = nb.startswith("-") and (len(nb) == 1 or nb[1] in (" ", "\t")) + if is_item and ni >= key_indent: + return self.parse_seq(ni) + if ni > key_indent: + return self.parse_node(ni) + return None + + def parse_seq(self, indent): + result = [] + while True: + tok = self.peek() + if tok is None: + break + cur_indent, body = tok + if cur_indent < indent: + break + if cur_indent > indent: + raise _Bail() + if not (body.startswith("-") and (len(body) == 1 or body[1] in (" ", "\t"))): + break # a mapping entry at this indent ends the sequence + rest = body[1:].strip() + self.i += 1 + if rest == "": + nxt = self.peek() + if nxt is not None and nxt[0] > indent: + result.append(self.parse_node(indent + 1)) + else: + result.append(None) + continue + km = _KEY_RE.match(rest) + if km: + # "- key: value" opens a mapping whose fields continue at the + # column where the content after the dash began. + field_indent = indent + (len(body) - len(body[1:].lstrip())) + result.append(self.parse_inline_map(field_indent, km)) + else: + val = _scalar(rest) + if val is _FAIL: + raise _Bail() + result.append(val) + return result + + def parse_inline_map(self, field_indent, first_match): + result = {} + key = first_match.group(1) + inline = first_match.group(2) + if inline is not None and inline.strip() != "": + val = _scalar(inline) + if val is _FAIL: + raise _Bail() + result[key] = val + else: + result[key] = self.parse_block_value(field_indent) + while True: + tok = self.peek() + if tok is None: + break + cur_indent, body = tok + if cur_indent != field_indent: + if cur_indent > field_indent: + raise _Bail() + break + if body.startswith("-") and (len(body) == 1 or body[1] in (" ", "\t")): + raise _Bail() + m = _KEY_RE.match(body) + if not m: + raise _Bail() + k = m.group(1) + iv = m.group(2) + self.i += 1 + if k in result: + raise _Bail() + if iv is not None and iv.strip() != "": + v = _scalar(iv) + if v is _FAIL: + raise _Bail() + result[k] = v + else: + result[k] = self.parse_block_value(field_indent) + return result + + +def _safe_parse(lines): + # Return the parsed root object (dict/list/scalar/None) when the WHOLE + # document is inside the supported subset, else _FAIL (fail closed). + try: + return _Parser(lines).parse_document() + except _Bail: + return _FAIL + except Exception: + return _FAIL + + 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 @@ -741,81 +974,27 @@ 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). 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. + # Conservative fallback for hosts without PyYAML. It parses config.yml with a + # strict recognizer (_safe_parse) of the narrow block-style subset tea writes, + # which reconstructs the SAME object PyYAML would OR fails closed (_FAIL) on + # ANYTHING it cannot prove it resolves identically -- document markers, block + # scalars, flow collections, duplicate keys, inconsistent indentation, or any + # line outside the grammar. On a recognized document it then resolves the + # login EXACTLY as the PyYAML path does (root `logins` list -> first entry + # whose `name` equals the request -> host/port-bound token), so the fallback + # can only ever be MORE conservative than PyYAML, never less. 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: + config = _safe_parse(lines) + if config is _FAIL: return None - - 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) - 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) - 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: - if str(entry.get("name") or "") == wanted: - return _accept(entry.get("token"), entry.get("url")) + 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 diff --git a/packages/mosaic/framework/tools/git/issue-comment.sh b/packages/mosaic/framework/tools/git/issue-comment.sh index cbf684ad..7c17794d 100755 --- a/packages/mosaic/framework/tools/git/issue-comment.sh +++ b/packages/mosaic/framework/tools/git/issue-comment.sh @@ -131,13 +131,18 @@ gitea_resolve_api_for_login() { # a concurrent write from a DIFFERENT identity cannot satisfy verification. # Prints the login on success. gitea_authenticated_login() { - local response_file status + local response_file auth_config status response_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-issue-comment-whoami.XXXXXX") - trap 'rm -f "$response_file"' RETURN + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$response_file" + echo "Error: could not stage Gitea credential for identity read" >&2 + return 1 + } + trap 'rm -f "$response_file" "$auth_config"' RETURN if ! status=$(curl -sS -o "$response_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_ROOT/user"); then echo "Error: Gitea authenticated-identity read transport failed" >&2 return 1 @@ -179,7 +184,7 @@ PY # Args: $1 = issue number, $2 = comment body, $3 = acting identity login. gitea_create_comment_verified() { local issue_number="$1" comment_body="$2" acting_login="$3" - local payload write_file readback_file write_status readback_status created_id + local payload write_file readback_file auth_config write_status readback_status created_id payload=$(COMMENT_BODY="$comment_body" python3 -c ' import json @@ -189,11 +194,16 @@ print(json.dumps({"body": os.environ["COMMENT_BODY"]})) ') write_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-issue-comment-write.XXXXXX") readback_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-issue-comment-getid.XXXXXX") - trap 'rm -f "$write_file" "$readback_file"' RETURN + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$write_file" "$readback_file" + echo "Error: could not stage Gitea credential for comment write" >&2 + return 1 + } + trap 'rm -f "$write_file" "$readback_file" "$auth_config"' RETURN if ! write_status=$(curl -sS -o "$write_file" -w '%{http_code}' \ -X POST \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ -H 'Content-Type: application/json' \ -d "$payload" \ "$GITEA_API_BASE/issues/$issue_number/comments"); then @@ -223,7 +233,7 @@ PY ) || return 1 if ! readback_status=$(curl -sS -o "$readback_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_BASE/issues/comments/$created_id"); then echo "Error: Gitea comment read-back transport failed" >&2 return 1 diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index d652540d..54dfde97 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -90,7 +90,7 @@ detect_platform >/dev/null # Args: $1 = PR number, $2 = comment body, $3 = acting identity login. gitea_create_comment_verified() { local pr_number="$1" comment_body="$2" acting_login="$3" - local payload write_file readback_file write_status readback_status created_id + local payload write_file readback_file auth_config write_status readback_status created_id payload=$(COMMENT_BODY="$comment_body" python3 -c ' import json @@ -100,11 +100,16 @@ print(json.dumps({"body": os.environ["COMMENT_BODY"]})) ') write_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-write.XXXXXX") readback_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-getid.XXXXXX") - trap 'rm -f "$write_file" "$readback_file"' RETURN + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$write_file" "$readback_file" + echo "Error: could not stage Gitea credential for comment write" >&2 + return 1 + } + trap 'rm -f "$write_file" "$readback_file" "$auth_config"' RETURN if ! write_status=$(curl -sS -o "$write_file" -w '%{http_code}' \ -X POST \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ -H 'Content-Type: application/json' \ -d "$payload" \ "$GITEA_API_BASE/issues/$pr_number/comments"); then @@ -134,7 +139,7 @@ PY ) || return 1 if ! readback_status=$(curl -sS -o "$readback_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_BASE/issues/comments/$created_id"); then echo "Error: Gitea comment read-back transport failed" >&2 return 1 @@ -181,12 +186,17 @@ try: # comment carries pull_request_url = ///pulls/ (with # issue_url empty), while a plain issue comment carries # issue_url = ///issues/ (with pull_request_url empty). + # This is the pr-review `comment` action, so the comment MUST land on a pull + # request: require pull_request_url. A plain issue_url is REJECTED — if issue + # #N exists but PR #N does not, POST /issues/N/comments creates an issue + # comment, and accepting that issue_url would let the wrapper falsely report a + # verified PR comment (issue-comment.sh legitimately keeps the broader + # issue-or-PR acceptance; a PR review does not). # 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): @@ -201,11 +211,8 @@ try: 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 ( - _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") + if not _belongs(comment.get("pull_request_url"), expected_pr_path): + raise ValueError("claimed PR comment did not land on a pull request (kind=pulls) 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) @@ -269,13 +276,18 @@ gitea_resolve_api_for_login() { # concurrent review from a DIFFERENT identity cannot satisfy verification. # Prints the login on success. gitea_authenticated_login() { - local response_file status + local response_file auth_config status response_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-whoami.XXXXXX") - trap 'rm -f "$response_file"' RETURN + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$response_file" + echo "Error: could not stage Gitea credential for identity read" >&2 + return 1 + } + trap 'rm -f "$response_file" "$auth_config"' RETURN if ! status=$(curl -sS -o "$response_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_ROOT/user"); then echo "Error: Gitea authenticated-identity read transport failed" >&2 return 1 @@ -302,18 +314,16 @@ print(login) PY } -# Resolve the PR's current head commit SHA (GET /pulls/{n}). The review is -# submitted against — and later verified as pinned to — this exact commit, so a -# stale review left over from an earlier push cannot be mistaken for this one. -# Prints the head SHA on success. -gitea_pr_head_sha() { - local pr_number="$1" pr_file status - - pr_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-head.XXXXXX") - trap 'rm -f "$pr_file"' RETURN +# GET /pulls/{n} into a caller-owned response file and print its head commit +# SHA. This core sets NO RETURN trap and reuses a caller-provided auth config + +# response file, so it is safe to call from INSIDE another trapped function +# (the post-verify re-read below) without clobbering that function's cleanup +# trap. $1 = PR number, $2 = response file, $3 = curl auth config file. +gitea_read_pr_head_into() { + local pr_number="$1" pr_file="$2" auth_config="$3" status if ! status=$(curl -sS -o "$pr_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_BASE/pulls/$pr_number"); then echo "Error: Gitea PR head read transport failed" >&2 return 1 @@ -339,6 +349,24 @@ print(head_sha) PY } +# Resolve the PR's current head commit SHA (GET /pulls/{n}). The review is +# submitted against — and later verified as pinned to — this exact commit, so a +# stale review left over from an earlier push cannot be mistaken for this one. +# Prints the head SHA on success. +gitea_pr_head_sha() { + local pr_number="$1" pr_file auth_config + + pr_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-head.XXXXXX") + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$pr_file" + echo "Error: could not stage Gitea credential for PR head read" >&2 + return 1 + } + trap 'rm -f "$pr_file" "$auth_config"' RETURN + + gitea_read_pr_head_into "$pr_number" "$pr_file" "$auth_config" +} + # 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 @@ -356,7 +384,8 @@ PY # $5 = PR head sha. gitea_submit_review_verified() { local pr_number="$1" event="$2" review_body="$3" acting_login="$4" head_sha="$5" - local payload write_file readback_file write_status readback_status created_id + local payload write_file readback_file recheck_file auth_config + local write_status readback_status created_id live_head payload=$(REVIEW_EVENT="$event" REVIEW_BODY="$review_body" REVIEW_COMMIT="$head_sha" python3 -c ' import json @@ -370,11 +399,17 @@ print(json.dumps({ ') write_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-submit.XXXXXX") readback_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-getid.XXXXXX") - trap 'rm -f "$write_file" "$readback_file"' RETURN + recheck_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-recheck.XXXXXX") + auth_config=$(gitea_write_auth_config "$GITEA_API_TOKEN") || { + rm -f "$write_file" "$readback_file" "$recheck_file" + echo "Error: could not stage Gitea credential for review submit" >&2 + return 1 + } + trap 'rm -f "$write_file" "$readback_file" "$recheck_file" "$auth_config"' RETURN if ! write_status=$(curl -sS -o "$write_file" -w '%{http_code}' \ -X POST \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ -H 'Content-Type: application/json' \ -d "$payload" \ "$GITEA_API_BASE/pulls/$pr_number/reviews"); then @@ -405,7 +440,7 @@ PY ) || return 1 if ! readback_status=$(curl -sS -o "$readback_file" -w '%{http_code}' \ - -H "Authorization: token $GITEA_API_TOKEN" \ + --config "$auth_config" \ "$GITEA_API_BASE/pulls/$pr_number/reviews/$created_id"); then echo "Error: Gitea review read-back transport failed" >&2 return 1 @@ -444,14 +479,42 @@ try: # 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: + # head all line up. Require presence + string TYPE + exact equality rather + # than `(body or "")`: the old coalesce treated a missing/null persisted body + # as equal to an empty submitted one, so a non-empty submitted body that + # persisted as null (a suppressed/lost body) would have passed. When a + # non-empty body was submitted the persisted value MUST be that exact string; + # when an empty body was submitted the persisted value must be empty or + # absent (a non-empty persisted body is likewise a divergence — vice-versa). + persisted_body = review.get("body") + if expected_body == "": + if persisted_body not in (None, ""): + raise ValueError("created review carries a body but none was submitted") + elif not isinstance(persisted_body, str) or persisted_body != 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) PY + # Current-head TOCTOU close-out: the review verified above is pinned to + # head_sha, but that head was read BEFORE the submit. Between then and now + # the PR branch may have advanced (a force-push or a new commit), which would + # leave this verified review attached to a now-superseded commit while the + # live tip carries unreviewed code — yet the wrapper would still report + # success. Re-read the LIVE PR head and require it STILL equals the submitted + # SHA; if it advanced, fail closed (nonzero, no created id emitted, no + # success line). This reuses the submit-scoped auth config + recheck file so + # it neither leaks the token to argv nor clobbers this function's cleanup. + live_head=$(gitea_read_pr_head_into "$pr_number" "$recheck_file" "$auth_config") || { + echo "Error: could not re-read Gitea PR head after review verification" >&2 + return 1 + } + if [[ "$live_head" != "$head_sha" ]]; then + echo "Error: Gitea PR head advanced from $head_sha to $live_head between review submit and verification; refusing to report a review pinned to a superseded commit (#865 current-head TOCTOU)" >&2 + return 1 + fi + echo "$created_id" return 0 } 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 c21f9745..c18eef0c 100755 --- a/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh +++ b/packages/mosaic/framework/tools/git/test-gitea-login-resolution.sh @@ -477,4 +477,148 @@ write_fixture "logins: " assert_token "single-quoted token is a literal string" "abc" primary git.example +# assert_fallback_fails_closed: the forced-fallback path MUST resolve no token +# (fail closed). Used for STRUCTURAL cases where PyYAML would resolve a DIFFERENT +# token (e.g. duplicate-key last-wins) — the fallback must never surface the +# wrong/stale token, so it fails closed instead; when PyYAML is present we also +# confirm it really does resolve a (divergent) token, proving the fallback is the +# strictly-more-conservative side and the case is a genuine fail-open guard. +assert_fallback_fails_closed() { + local desc="$1" login="$2" host="$3" got + got=$(token_fallback "$login" "$host") + if [[ -n "$got" ]]; then + echo "FAIL fallback [$desc]: expected fail-closed, got a token" >&2 + exit 1 + fi + if [[ "$HAVE_PYYAML" == true ]]; then + got=$(token_pyyaml "$login" "$host") + if [[ -z "$got" ]]; then + echo "FAIL [$desc]: expected PyYAML to resolve a divergent token" >&2 + exit 1 + fi + fi +} + +# 10. tea's REAL on-disk shape: the `logins:` block SEQUENCE items sit at the +# SAME indentation as the key (dash at column 0), with extra scalar fields. +# The recognizer must resolve this exactly like PyYAML (regression guard so +# the stricter whole-document recognizer does not fail closed on real input). +write_fixture 'logins: +- name: primary + url: https://git.example + token: TOK_REAL + default: false + ssh_host: "" +- name: other + url: https://other.example + token: TOK_REAL_OTHER +preferences: + editor: false + flags: null +' +assert_token "tea dash-at-column-0 real shape resolves" "TOK_REAL" primary git.example +assert_token "tea real shape sibling resolves own token" "TOK_REAL_OTHER" other other.example + +# 11. NESTED-SHADOW: a nested `logins:` (NOT at root scope) must not be mistaken +# for the real root logins. The recognizer parses whole-document structure, +# so it selects the ROOT logins token exactly as PyYAML does — never the +# nested attacker token. (A prior line scan matched the FIRST logins at ANY +# indent and returned ATTACKER.) +write_fixture 'outer: + logins: + - name: primary + url: https://git.example + token: ATTACKER_NESTED +logins: + - name: primary + url: https://git.example + token: ROOT_TOK +' +assert_token "nested logins shadow selects ROOT token" "ROOT_TOK" primary git.example + +# 12. BLOCK-SCALAR-SHADOW: text inside a YAML literal/folded block ( | or > ) is +# an OPAQUE scalar to PyYAML (so `logins` is a string, not a list) and must +# not be scanned as live logins entries. Both fail closed. +write_fixture 'logins: | + - name: primary + url: https://git.example + token: ATTACKER_BLOCK +' +assert_token "block-scalar logins value fails closed" "" primary git.example +# A folded/literal block scalar anywhere is outside the recognizer's subset, so +# the fallback fails closed (conservative) even though PyYAML can still resolve +# the real root token past the opaque scalar. Fail-closed is the safe side. +write_fixture 'note: > + logins: + - name: primary + token: ATTACKER_FOLDED +logins: + - name: primary + url: https://git.example + token: ROOT_OK +' +assert_fallback_fails_closed "folded block scalar present fails closed" primary git.example + +# 13. DUPLICATE-ROOT / DUPLICATE-FIELD: a duplicated `logins:` root key (PyYAML +# last-wins) or a duplicated field within a login must fail closed rather +# than take the FIRST (stale) value. PyYAML resolves the LAST; the fallback +# refuses to guess. +write_fixture 'logins: + - name: primary + url: https://git.example + token: FIRST_DUP +logins: + - name: primary + url: https://git.example + token: LAST_DUP +' +assert_fallback_fails_closed "duplicate root logins key fails closed" primary git.example +write_fixture 'logins: + - name: primary + url: https://git.example + token: FIRST_FIELD + token: SECOND_FIELD +' +assert_fallback_fails_closed "duplicate token field fails closed" primary git.example + +# 14. MALFORMED-AFTER-VALID: a syntax error LATER in the file makes PyYAML reject +# the WHOLE document; the recognizer must too (not emit the earlier token). +write_fixture 'logins: + - name: primary + url: https://git.example + token: TOK_PLAIN +broken: a: b: c +' +assert_token "malformed line after valid login fails closed" "" primary git.example +write_fixture 'logins: + - name: primary + url: https://git.example + token: TOK_PLAIN +broken: [unclosed +' +assert_token "unclosed flow after valid login fails closed" "" primary git.example + +# 15. EXTRA-DOCUMENT: a multi-document file (--- separator, or ... end marker) +# makes PyYAML safe_load reject multi-document input; the recognizer fails +# closed on ANY document marker rather than emit the first doc's token. +write_fixture 'logins: + - name: primary + url: https://git.example + token: TOK_PLAIN +--- +logins: + - name: primary + url: https://git.example + token: SECOND_DOC +' +assert_token "second document (--- separator) fails closed" "" primary git.example +write_fixture 'logins: + - name: primary + url: https://git.example + token: TOK_PLAIN +... +trailing: 1 +' +assert_token "end marker then more content fails closed" "" primary git.example + 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 05d70ba9..b0178385 100755 --- a/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh +++ b/packages/mosaic/framework/tools/git/test-issue-comment-readback.sh @@ -52,6 +52,8 @@ BIN_DIR="$WORK_DIR/bin" XDG_DIR="$WORK_DIR/xdg" TEA_LOG="$WORK_DIR/tea.log" CURL_LOG="$WORK_DIR/curl.log" +# Full curl argv per invocation — proves the bearer token never rides in argv. +CURL_ARGV_LOG="$WORK_DIR/curl-argv.log" AUTH_LOG="$WORK_DIR/auth.log" OUTPUT_FILE="$WORK_DIR/output.log" CREDENTIALS_FILE="$WORK_DIR/credentials.json" @@ -155,17 +157,24 @@ cat > "$BIN_DIR/curl" <<'SH' #!/usr/bin/env bash set -euo pipefail +# Record the FULL argv exactly as spawned, before consumption. The bearer token +# must NOT appear here — it is delivered via a curl --config file (#865 ITEM 3a), +# so only the config file PATH may show up. +printf '%s\n' "$*" >> "$ISSUE_COMMENT_CURL_ARGV_LOG" + output_file="" method="GET" url="" data="" auth_token="" +config_file="" while [[ $# -gt 0 ]]; do case "$1" in -o) output_file="$2"; shift 2 ;; -H) [[ "$2" == Authorization:* ]] && auth_token="${2##* }" shift 2 ;; + -K|--config) config_file="$2"; shift 2 ;; -w) shift 2 ;; -X) method="$2"; shift 2 ;; -d|--data) data="$2"; shift 2 ;; @@ -175,6 +184,17 @@ while [[ $# -gt 0 ]]; do esac done +# Resolve the bearer token from the curl --config file (its real, secure source); +# fall back to an -H header only for defense in depth. The config line is +# `header = "Authorization: token "`. +if [[ -z "$auth_token" && -n "$config_file" && -f "$config_file" ]]; then + config_hdr="$(grep -i 'Authorization' "$config_file" 2>/dev/null || true)" + if [[ "$config_hdr" == *"token "* ]]; then + auth_token="${config_hdr##*token }" + auth_token="${auth_token%\"}" + fi +fi + path="${url%%\?*}" query="${url#*\?}" [[ "$query" == "$url" ]] && query="" @@ -338,6 +358,7 @@ run_comment() { shift : > "$TEA_LOG" : > "$CURL_LOG" + : > "$CURL_ARGV_LOG" : > "$AUTH_LOG" : > "$OUTPUT_FILE" seed_state "$mode" @@ -349,6 +370,7 @@ run_comment() { MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \ ISSUE_COMMENT_TEA_LOG="$TEA_LOG" \ ISSUE_COMMENT_CURL_LOG="$CURL_LOG" \ + ISSUE_COMMENT_CURL_ARGV_LOG="$CURL_ARGV_LOG" \ ISSUE_COMMENT_AUTH_LOG="$AUTH_LOG" \ ISSUE_COMMENT_STATE="$STATE_FILE" \ ISSUE_COMMENT_TEST_MODE="$mode" \ @@ -371,7 +393,9 @@ run_comment() { # 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) + # Includes the curl auth-config files (mosaic-gitea-auth-*), which carry the + # bearer token and must be unlinked on every exit path. + leaked=$(find "$TMP_SCRATCH" -type f \( -name 'mosaic-issue-comment-*' -o -name 'mosaic-gitea-auth-*' \) 2>/dev/null || true) if [[ -n "$leaked" ]]; then echo "FAIL: issue-comment temp files leaked ($context):" >&2 printf '%s\n' "$leaked" >&2 @@ -379,6 +403,21 @@ assert_no_temp_leak() { fi } +# Assert the presented bearer token NEVER appeared in curl's argv (it must travel +# via a curl --config file), and that --config auth was actually used. On the +# expected path grep matches nothing, so no token value is ever printed. +assert_token_not_in_argv() { + local context="$1" + if grep -qF -e "$DEFAULT_TOKEN" -e "$OVERRIDE_TOKEN" -e "$CROSS_HOST_TOKEN" "$CURL_ARGV_LOG"; then + echo "FAIL: a Gitea bearer token leaked into curl argv ($context)" >&2 + exit 1 + fi + if ! grep -q -- '--config' "$CURL_ARGV_LOG"; then + echo "FAIL: curl was not invoked with --config file auth ($context)" >&2 + exit 1 + fi +} + # Case 1: a genuine REST create (id 51) is verified end to end via its exact # provider-returned id — no list enumeration is involved. run_comment fresh-success @@ -404,6 +443,9 @@ 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" +# ITEM 3a: the token drove the write/read-back chain but never appeared in curl +# argv — it was passed via a curl --config file. +assert_token_not_in_argv "fresh-success default-token" # Case 2: a no-op write with a concurrent SAME-IDENTITY, same-body comment # already present must FAIL CLOSED — the closed concurrency window. 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 adef8c2c..90f57c85 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 @@ -45,8 +45,13 @@ STATE_DIR="$WORK_DIR/state" REVIEWS_FILE="$STATE_DIR/reviews.json" COMMENTS_FILE="$STATE_DIR/comments.json" SUBMIT_PAYLOAD_FILE="$STATE_DIR/review_payload.json" +# Counts GET /pulls/{n} calls within a single run so a race mode can advance the +# reported head between the pre-submit read and the post-verify re-read. +HEAD_CALLS_FILE="$STATE_DIR/head_calls" TEA_LOG="$WORK_DIR/tea.log" CURL_LOG="$WORK_DIR/curl.log" +# Full curl argv per invocation — proves the bearer token never rides in argv. +CURL_ARGV_LOG="$WORK_DIR/curl-argv.log" AUTH_LOG="$WORK_DIR/auth.log" OUTPUT_FILE="$WORK_DIR/output.log" CREDENTIALS_FILE="$WORK_DIR/credentials.json" @@ -143,17 +148,24 @@ cat > "$BIN_DIR/curl" <<'SH' #!/usr/bin/env bash set -euo pipefail +# Record the FULL argv exactly as spawned, BEFORE any consumption. The bearer +# token must NOT appear here — it is delivered via a curl --config file, so only +# the config file PATH may show up. (#865 ITEM 3a credential-in-argv exposure.) +printf '%s\n' "$*" >> "$PR_REVIEW_CURL_ARGV_LOG" + output_file="" method="GET" payload="" url="" auth_token="" +config_file="" while [[ $# -gt 0 ]]; do case "$1" in -o) output_file="$2"; shift 2 ;; -H) [[ "$2" == Authorization:* ]] && auth_token="${2##* }" shift 2 ;; + -K|--config) config_file="$2"; shift 2 ;; -w) shift 2 ;; -X) method="$2"; shift 2 ;; -d|--data) payload="$2"; shift 2 ;; @@ -163,6 +175,17 @@ while [[ $# -gt 0 ]]; do esac done +# Resolve the bearer token from the curl --config file (its real, secure source); +# only fall back to an -H header for defense in depth. The config line is +# `header = "Authorization: token "`. +if [[ -z "$auth_token" && -n "$config_file" && -f "$config_file" ]]; then + config_hdr="$(grep -i 'Authorization' "$config_file" 2>/dev/null || true)" + if [[ "$config_hdr" == *"token "* ]]; then + auth_token="${config_hdr##*token }" + auth_token="${auth_token%\"}" + fi +fi + path="${url%%\?*}" query="${url#*\?}" [[ "$query" == "$url" ]] && query="" @@ -207,7 +230,25 @@ elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123 write_response 200 "$(PR_REVIEW_HEAD_SHA="$PR_REVIEW_HEAD_SHA" python3 - <<'PY' import json import os -print(json.dumps({"head": {"sha": os.environ["PR_REVIEW_HEAD_SHA"]}})) + +head = os.environ["PR_REVIEW_HEAD_SHA"] +mode = os.environ.get("PR_REVIEW_TEST_MODE", "") +calls_path = os.environ.get("PR_REVIEW_HEAD_CALLS", "") +# Count GET /pulls/{n} calls within this run: call 1 is the pre-submit head read +# that pins the review; call 2+ is the post-verify re-read (current-head TOCTOU +# close-out). In the race mode the branch "advances" after the pin. +n = 1 +if calls_path: + try: + with open(calls_path, encoding="utf-8") as handle: + n = int(handle.read() or "0") + 1 + except (OSError, ValueError): + n = 1 + with open(calls_path, "w", encoding="utf-8") as handle: + handle.write(str(n)) +if mode == "head-advanced-race" and n >= 2: + head = "HEADSHA_ADVANCED_DEADBEEF" +print(json.dumps({"head": {"sha": head}})) PY )" elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then @@ -254,6 +295,26 @@ if mode == "review-body-reuse": print(json.dumps(record)) raise SystemExit(0) +# review-body-null (#865 ITEM 3b): a non-empty body was submitted but the +# persisted review carries body == null. id/author/state/head all line up; only +# strict presence + string-type body verification catches the lost body. The old +# `(body or "")` coalesce would have treated null as an empty string and passed. +if mode == "review-body-null": + 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": None, + "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 = { @@ -313,6 +374,23 @@ body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body") # 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" +# comment-plain-issue (#865 ITEM 2): #123 is a plain ISSUE, not a PR. POST +# /issues/123/comments lands an issue comment whose issue_url is set and +# pull_request_url is empty. The pr-review `comment` action MUST reject this — it +# claimed a PR comment, so a bare issue_url is not acceptable proof. +if mode == "comment-plain-issue": + record = { + "id": 456, + "body": body, + "user": {"login": acting}, + "issue_url": f"{web_base}/issues/123", + "pull_request_url": "", + } + with open(state_path, "w", encoding="utf-8") as handle: + json.dump([record], handle) + print("201") + print(json.dumps(record)) + raise SystemExit(0) # 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. @@ -377,7 +455,7 @@ chmod +x "$BIN_DIR/curl" seed_state() { local mode="$1" printf '[]' > "$COMMENTS_FILE" - rm -f "$SUBMIT_PAYLOAD_FILE" + rm -f "$SUBMIT_PAYLOAD_FILE" "$HEAD_CALLS_FILE" PR_REVIEW_SEED_MODE="$mode" PR_REVIEW_SEED_ACTING="$ACTING_LOGIN" \ PR_REVIEW_SEED_HEAD="$HEAD_SHA" python3 - "$REVIEWS_FILE" <<'PY' import json @@ -424,6 +502,7 @@ run_review() { write_credentials "$configured_url" : > "$TEA_LOG" : > "$CURL_LOG" + : > "$CURL_ARGV_LOG" : > "$AUTH_LOG" : > "$OUTPUT_FILE" seed_state "$mode" @@ -436,10 +515,12 @@ run_review() { PR_REVIEW_TEA_LOG="$TEA_LOG" \ PR_REVIEW_LOGIN_URL="${configured_url%/}" \ PR_REVIEW_CURL_LOG="$CURL_LOG" \ + PR_REVIEW_CURL_ARGV_LOG="$CURL_ARGV_LOG" \ PR_REVIEW_AUTH_LOG="$AUTH_LOG" \ PR_REVIEW_REVIEWS="$REVIEWS_FILE" \ PR_REVIEW_COMMENTS="$COMMENTS_FILE" \ PR_REVIEW_SUBMIT_PAYLOAD="$SUBMIT_PAYLOAD_FILE" \ + PR_REVIEW_HEAD_CALLS="$HEAD_CALLS_FILE" \ PR_REVIEW_TEST_MODE="$mode" \ PR_REVIEW_EXPECTED_BODY="$comment" \ PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \ @@ -462,7 +543,9 @@ run_review() { # 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) + # Includes the curl auth-config files (mosaic-gitea-auth-*), which carry the + # bearer token and must be unlinked on every exit path. + leaked=$(find "$TMP_SCRATCH" -type f \( -name 'mosaic-pr-review-*' -o -name 'mosaic-gitea-auth-*' \) 2>/dev/null || true) if [[ -n "$leaked" ]]; then echo "FAIL: pr-review temp files leaked ($context):" >&2 printf '%s\n' "$leaked" >&2 @@ -470,6 +553,21 @@ assert_no_temp_leak() { fi } +# Assert the presented bearer token NEVER appeared in curl's argv (it must travel +# via a curl --config file), and that --config auth was actually used. On the +# expected path grep matches nothing, so no token value is ever printed. +assert_token_not_in_argv() { + local context="$1" + if grep -qF -e "$DEFAULT_TOKEN" -e "$OVERRIDE_TOKEN" -e "$CROSS_HOST_TOKEN" "$CURL_ARGV_LOG"; then + echo "FAIL: a Gitea bearer token leaked into curl argv ($context)" >&2 + exit 1 + fi + if ! grep -q -- '--config' "$CURL_ARGV_LOG"; then + echo "FAIL: curl was not invoked with --config file auth ($context)" >&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 @@ -499,6 +597,9 @@ grep -q "^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/ grep -q "^GET https://git.mosaicstack.dev/api/v1/user $ACTING_LOGIN\$" "$AUTH_LOG" assert_no_tea_write assert_no_temp_leak "approve" +# ITEM 3a: the host-default token drove this whole chain, yet never appeared in +# any curl argv — it was passed via a curl --config file. +assert_token_not_in_argv "approve default-token" # 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 @@ -673,6 +774,8 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then exit 1 fi assert_no_tea_write +# ITEM 3a: the override token likewise never leaked into curl argv. +assert_token_not_in_argv "override-success override-token" # Case 9 (#865 Round-4): an UNRESOLVABLE explicit --login override (a name absent # from the tea config) must FAIL CLOSED — nonzero exit, no success line, no review @@ -765,4 +868,56 @@ for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong assert_no_temp_leak "$bad_mode" done +# Case 16 (#865 ITEM 1, current-head TOCTOU): the PR head advances between the +# pre-submit head read (which pins the review) and the post-verify re-read. The +# review is genuinely created and verified as pinned to the OLD head, but the +# live tip has moved on, so the wrapper must FAIL CLOSED rather than report a +# review that no longer covers the PR's current commit. +if run_review head-advanced-race approve; then + echo "FAIL: approve reported success though the PR head advanced after submit" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +if grep -q 'Approved and verified' "$OUTPUT_FILE"; then + echo "FAIL: current-head TOCTOU close-out did not fail closed on an advanced head" >&2 + exit 1 +fi +# The head was re-read after the submit/verify (2nd GET /pulls/123). +if [[ "$(grep -c '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG")" -lt 2 ]]; then + echo "FAIL: wrapper did not re-read the PR head after review verification" >&2 + cat "$CURL_LOG" >&2 + exit 1 +fi +assert_no_temp_leak "head-advanced-race" + +# Case 17 (#865 ITEM 2): a claimed PR comment that actually lands as a plain +# ISSUE comment (issue #123 exists, PR #123 does not — issue_url set, +# pull_request_url empty) must FAIL CLOSED. The pr-review `comment` verifier +# requires a pull_request_url (kind=pulls) and rejects a bare issue_url. +if run_review comment-plain-issue comment durable-body; then + echo "FAIL: pr-review accepted a plain issue comment as a verified PR comment" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then + echo "FAIL: a plain issue_url satisfied the PR comment verifier" >&2 + exit 1 +fi +assert_no_temp_leak "comment-plain-issue" + +# Case 18 (#865 ITEM 3b): a non-empty review body submitted but persisted as null +# must FAIL CLOSED. id/author/state/head all match; only strict presence + +# string-type + exact body equality (not the old `(body or "")` coalesce) catches +# the lost body. +if run_review review-body-null approve real-submitted-review-body; then + echo "FAIL: review whose non-empty body persisted as null was accepted" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +if grep -q 'Approved and verified' "$OUTPUT_FILE"; then + echo "FAIL: a null persisted body passed strict review-body verification" >&2 + exit 1 +fi +assert_no_temp_leak "review-body-null" + echo "pr-review.sh REST review + comment create/read-back regression passed"