fix(framework/tools): wrapper body-safety + login-resolution hardening (#559, #560)
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful

#559 — Markdown body safety / eval removal:
- Add test-issue-create-body-safety.sh: feeds a hostile Markdown body
  ($(...), backticks, quotes, $vars, pipes) through issue-create.sh and
  asserts no command substitution runs and the body reaches tea verbatim.
- Convert issue-comment.sh from unquoted $(get_gitea_repo_args) word-splitting
  to an argv array with an explicit loud login-resolution error.
- Confirmed: zero eval usages remain across tools/git/*.sh; the other
  body-carrying wrappers (issue-create, pr-create, issue-edit, issue-assign)
  already use argv arrays.

#560 — host-derived Gitea login + loud failure:
- detect-platform.sh: add print_gitea_login_diagnostic and emit it on the
  get_gitea_login_for_host failure path (stderr only) — names the unresolved
  host, lists available tea logins, and gives the GITEA_LOGIN override +
  tea-login-add fix. Replaces the previous silent failure.
- Extend test-gitea-login-resolution.sh: assert the diagnostic fires and lists
  logins, login is derived from origin host for both mosaicstack and usc (scoped
  second tea mock), and a valid GITEA_LOGIN override is honored.

Also gitignore the .mosaic-test-work/ shell-harness scratch dir.
Scope: wrapper surface only. All wrapper test harnesses pass locally.
This commit is contained in:
jason.woltje
2026-06-20 04:51:54 -05:00
parent 9b7e63f6c3
commit feb0d8a58b
6 changed files with 316 additions and 1 deletions

3
.gitignore vendored
View File

@@ -12,3 +12,6 @@ docs/reports/
# Step-CA dev password — real file is gitignored; commit only the .example # Step-CA dev password — real file is gitignored; commit only the .example
infra/step-ca/dev-password infra/step-ca/dev-password
# Scratch dirs created by the framework git-wrapper shell test harnesses
.mosaic-test-work/

View File

@@ -0,0 +1,87 @@
# Wrapper hardening fold-in: #559 (eval removal) + #560 (host-derived login)
**Branch:** `fix/wrapper-hardening-tls-credpath-cicwait` (PR #551)
**Worker:** coderlite0 (Sonnet lane) · coordinated by mos-claude
**Date:** 2026-06-20
**Scope:** `packages/mosaic/framework/tools/git/*.sh` only
## What the issues asked for vs. what was already landed
Both issues were largely satisfied by prior merged work; this fold-in closes the
remaining gaps (regression tests + a loud diagnostic + one residual word-split site)
rather than re-implementing finished functionality.
### #559 — remove `eval` from issue-create.sh (and siblings)
- `eval`-based command construction was already removed across the wrapper surface
(landed in #549). A full scan of `tools/git/*.sh` finds **zero** `eval` usages.
- `issue-create.sh`, `pr-create.sh`, `issue-edit.sh`, `issue-assign.sh` already build
their `tea`/`gh` invocations as argv arrays (`CMD=(...)`, `"${CMD[@]}"`), so Markdown
bodies pass through verbatim.
- **Residual found & fixed:** `issue-comment.sh` still used unquoted
`$(get_gitea_repo_args)` word-splitting (the comment body itself was already safely
quoted, so no injection bug — but it was the inconsistent, fragile pattern #559 targets,
and it failed silently when no login resolved). Converted to an argv array with an
explicit, loud login-resolution error.
- **Added regression test:** `test-issue-create-body-safety.sh` — feeds a hostile
Markdown body (`$(touch SENTINEL)`, backticks, single/double quotes, `$HOME`/`${PATH}`,
pipes/`&&`/`;`) through `issue-create.sh` and asserts (1) no command substitution
executes (sentinel file never created) and (2) the `--description` `tea` receives is
byte-for-byte the original body.
### #560 — auto-detect Gitea `--login` from repo origin host
- Centralized host→login resolution already exists in `detect-platform.sh`
(`get_gitea_login_for_host``find_tea_login_for_host`, matching `urlparse(url).hostname`).
Every wrapper routes through it (or `get_gitea_login` / `get_gitea_login_for_repo_override`);
**no wrapper hardcodes `${GITEA_LOGIN:-mosaicstack}`**. Explicit `GITEA_LOGIN` wins only
when it matches the host (`tea_login_matches_host`), so stale overrides are rejected.
- **Gap fixed — silent failure → loud diagnostic:** the failure path of
`get_gitea_login_for_host` returned non-zero with no message. Added
`print_gitea_login_diagnostic`, emitted to **stderr** on resolution failure: names the
unresolved host, lists available tea logins (name + host), and gives the `GITEA_LOGIN`
override + `tea login add` fix. Stderr-only, so it never contaminates stdout (the
resolved login name) or the log-grep assertions in the existing harnesses. Callers with
an API fallback (pr-merge, issue-close, pr-create, issue-create) still follow with their
own "using API fallback" line, giving a clear "no login → fallback" trail.
- **Extended test:** `test-gitea-login-resolution.sh` now also asserts (a) the loud
diagnostic fires and lists available logins for an unresolved host, (b) login is derived
from origin host for **both** instances (mosaicstack + usc) via a scoped second `tea`
mock, and (c) a valid `GITEA_LOGIN` override is honored. The scoped mock keeps the
existing API-fallback assertions (which require mosaicstack to have _no_ tea login) valid.
## Files changed (wrapper surface only)
- `detect-platform.sh` — add `print_gitea_login_diagnostic`; call it on the
`get_gitea_login_for_host` failure path.
- `issue-comment.sh` — argv array + loud login-resolution error (was unquoted
`$(get_gitea_repo_args)`).
- `test-issue-create-body-safety.sh`**new** (#559 regression).
- `test-gitea-login-resolution.sh` — extended (#560 diagnostic + both-host + override).
## Verification
All wrapper harnesses pass locally:
- `test-issue-create-body-safety.sh` — PASS
- `test-gitea-login-resolution.sh` — PASS
- `test-pr-merge-gitea-empty-uid.sh` — PASS
- `test-pr-metadata-gitea.sh` — PASS
- `test-lane-brief-pr-linkage.sh` — PASS
## Open items flagged to mos-claude (orchestrator decisions)
1. **CHANGELOG absent.** The task said "update CHANGELOG (append-only), keep the existing
#550/#551 entry." No CHANGELOG file exists anywhere in the repo, and #550/#551 are not
recorded in one. **ASSUMPTION:** documenting #559/#560 in this scratchpad + the PR
description (`Closes #559 Closes #560`) follows the repo's actual convention
(`docs/scratchpads/`). Did not invent a new CHANGELOG structure.
2. **`docs/TASKS.md` is orchestrator single-writer.** It carries a "Workers read but never
modify" banner. As a worker I did **not** edit it; task tracking is via the linked Gitea
issues #559/#560 + this scratchpad. Orchestrator may add a rollup row if desired.
3. **Wrapper `test-*.sh` are not CI-wired.** `.woodpecker/ci.yml` runs `pnpm
typecheck/lint/format:check/test` (`turbo run test`); the framework dir has no
`package.json`, so these shell harnesses run **locally/manually only** — they do not gate
the PR in Woodpecker. **ASSUMPTION:** out of scope to wire a shell-test step into CI in
this PR (would broaden the diff beyond the wrapper surface). Flagging for a follow-up if
the fleet wants these gated.

View File

@@ -169,6 +169,43 @@ raise SystemExit(1)
PY PY
} }
# Emit an actionable diagnostic to stderr when no tea login resolves for a host.
# Callers that have a working API fallback may ignore the non-zero return of
# get_gitea_login_for_host; this turns the previously SILENT failure into a loud,
# greppable hint (available logins + override + add-login instructions). Printed to
# stderr only, so it never contaminates stdout (the resolved login name) or log
# assertions that capture tea/curl invocations.
print_gitea_login_diagnostic() {
local host="${1:-<unknown>}"
local available
available=$(
command -v tea >/dev/null 2>&1 || { echo "(tea CLI not installed)"; exit 0; }
logins_json=$(tea login list --output json 2>/dev/null) || { echo "(could not query tea login list)"; exit 0; }
TEA_LOGINS_JSON="$logins_json" python3 - <<'PY'
import json, os
from urllib.parse import urlparse
try:
logins = json.loads(os.environ.get("TEA_LOGINS_JSON", "[]"))
except Exception:
logins = []
rows = []
for login in logins if isinstance(logins, list) else []:
name = str(login.get("name") or login.get("Name") or "")
url = str(login.get("url") or login.get("URL") or "")
host = urlparse(url).hostname or "?"
if name:
rows.append(f"{name} (host: {host})")
print("; ".join(rows) if rows else "(none configured)")
PY
)
{
echo "Error: no Gitea tea login matches host '$host'."
echo " Available tea logins: ${available}"
echo " Fix: set GITEA_LOGIN to a login whose URL host is '$host',"
echo " or add one: tea login add --name <name> --url https://$host --token <token>"
} >&2
}
get_gitea_login_for_host() { get_gitea_login_for_host() {
local host="${1:-}" local host="${1:-}"
local login local login
@@ -190,6 +227,7 @@ get_gitea_login_for_host() {
return 0 return 0
fi fi
print_gitea_login_diagnostic "$host"
return 1 return 1
} }

View File

@@ -53,7 +53,15 @@ if [[ "$PLATFORM" == "github" ]]; then
gh issue comment "$ISSUE_NUMBER" --body "$COMMENT" gh issue comment "$ISSUE_NUMBER" --body "$COMMENT"
echo "Added comment to GitHub issue #$ISSUE_NUMBER" echo "Added comment to GitHub issue #$ISSUE_NUMBER"
elif [[ "$PLATFORM" == "gitea" ]]; then elif [[ "$PLATFORM" == "gitea" ]]; then
tea issue comment "$ISSUE_NUMBER" "$COMMENT" $(get_gitea_repo_args) # Build the invocation as an argv array (not unquoted $(get_gitea_repo_args)
# word-splitting) so the comment body — including Markdown backticks, $(...),
# and quotes — is passed verbatim and never re-split or shell-evaluated.
REPO_SLUG=$(get_repo_slug)
GITEA_LOGIN_NAME=$(get_gitea_login) || {
echo "Error: could not resolve a Gitea login for this repo; cannot comment on issue #$ISSUE_NUMBER." >&2
exit 1
}
tea issue comment "$ISSUE_NUMBER" "$COMMENT" --repo "$REPO_SLUG" --login "$GITEA_LOGIN_NAME"
echo "Added comment to Gitea issue #$ISSUE_NUMBER" echo "Added comment to Gitea issue #$ISSUE_NUMBER"
else else
echo "Error: Unknown platform" echo "Error: Unknown platform"

View File

@@ -230,4 +230,81 @@ if grep -q -- 'tea issue close 536 .*--login mosaicstack' "$LOG_FILE"; then
exit 1 exit 1
fi fi
# ---------------------------------------------------------------------------
# #560: loud diagnostic + host-derived login for BOTH instances + override-wins
# ---------------------------------------------------------------------------
# Loud diagnostic: a host with no matching tea login must emit an actionable
# error to stderr (the previous behavior was a SILENT failure). The original
# mock defines only usc/evil-usc logins, so mosaicstack resolution fails here.
git -C "$REPO_DIR" remote set-url origin https://git.mosaicstack.dev/mosaicstack/stack.git
diag_stderr=$(run_in_repo bash -c '
source "'"$SCRIPT_DIR"'/detect-platform.sh"
get_gitea_login_for_host git.mosaicstack.dev
' 2>&1 1>/dev/null || true)
if ! grep -q "no Gitea tea login matches host 'git.mosaicstack.dev'" <<<"$diag_stderr"; then
echo "Expected loud diagnostic naming the unresolved host; got: $diag_stderr" >&2
exit 1
fi
if ! grep -q "Available tea logins:" <<<"$diag_stderr"; then
echo "Expected diagnostic to list available tea logins; got: $diag_stderr" >&2
exit 1
fi
# Both-instance host derivation + override-wins, using a mock that DOES define a
# mosaicstack login. Scoped to this section so the API-fallback assertions above
# (which rely on mosaicstack having NO tea login) remain valid.
BIN_DIR2="$WORK_DIR/bin2"
mkdir -p "$BIN_DIR2"
cp "$BIN_DIR/curl" "$BIN_DIR2/curl"
cat > "$BIN_DIR2/tea" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
if [[ "$*" == "login list --output json" ]]; then
cat <<'JSON'
[
{"name":"mosaicstack","url":"https://git.mosaicstack.dev","user":"jason.woltje"},
{"name":"usc","url":"https://git.uscllc.com","user":"jason.woltje"}
]
JSON
exit 0
fi
printf 'tea %s\n' "$*" >> "$MOSAIC_TEST_LOG"
exit 0
SH
chmod +x "$BIN_DIR2/tea"
run_in_repo2() {
(
cd "$REPO_DIR"
PATH="$BIN_DIR2:$PATH" \
MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
MOSAIC_TEST_LOG="$LOG_FILE" \
"$@"
)
}
git -C "$REPO_DIR" remote set-url origin https://git.mosaicstack.dev/mosaicstack/stack.git
mosaic_login=$(run_in_repo2 bash -c 'source "'"$SCRIPT_DIR"'/detect-platform.sh"; get_gitea_login')
if [[ "$mosaic_login" != "mosaicstack" ]]; then
echo "Expected mosaicstack origin to derive login 'mosaicstack'; got '$mosaic_login'" >&2
exit 1
fi
git -C "$REPO_DIR" remote set-url origin https://git.uscllc.com/USC/uconnect.git
usc_login_derived=$(run_in_repo2 bash -c 'source "'"$SCRIPT_DIR"'/detect-platform.sh"; get_gitea_login')
if [[ "$usc_login_derived" != "usc" ]]; then
echo "Expected usc origin to derive login 'usc'; got '$usc_login_derived'" >&2
exit 1
fi
# Explicit GITEA_LOGIN override is honored when it matches the host.
git -C "$REPO_DIR" remote set-url origin https://git.mosaicstack.dev/mosaicstack/stack.git
override_wins=$(run_in_repo2 bash -c 'export GITEA_LOGIN=mosaicstack; source "'"$SCRIPT_DIR"'/detect-platform.sh"; get_gitea_login')
if [[ "$override_wins" != "mosaicstack" ]]; then
echo "Expected valid GITEA_LOGIN override to win on mosaicstack host; got '$override_wins'" >&2
exit 1
fi
git -C "$REPO_DIR" remote set-url origin https://git.uscllc.com/USC/uconnect.git
echo "Gitea login resolution regression harness passed" echo "Gitea login resolution regression harness passed"

View File

@@ -0,0 +1,102 @@
#!/usr/bin/env bash
# Regression harness for issue-create.sh Markdown-body safety (#559).
#
# Guards against reintroduction of eval-based command construction. The wrapper
# builds its tea/gh invocation as an argv array, so a body containing command
# substitution ($(...)), backticks, quotes, and dollar signs MUST reach tea
# verbatim and MUST NOT be shell-evaluated. This test asserts both:
# 1. No command-substitution side effect (an injected `touch SENTINEL` never runs).
# 2. The --description value tea receives is byte-for-byte the original body.
set -euo pipefail
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
WORK_DIR="${MOSAIC_TEST_WORK_DIR:-$PWD/.mosaic-test-work/issue-create-body-safety}"
REPO_DIR="$WORK_DIR/repo"
BIN_DIR="$WORK_DIR/bin"
SENTINEL="$WORK_DIR/INJECTION_SENTINEL"
BODY_FILE="$WORK_DIR/body.txt"
RECEIVED_FILE="$WORK_DIR/received-description.txt"
rm -rf "$WORK_DIR"
mkdir -p "$REPO_DIR" "$BIN_DIR"
git -C "$REPO_DIR" init -q
git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
# Hostile Markdown body. The unquoted heredoc expands $SENTINEL (a real path we
# want embedded) but every shell metacharacter we care about is backslash-escaped
# so the TEST shell writes them literally into the file — the bytes the wrapper
# must then preserve.
cat > "$BODY_FILE" <<EOF
# Release notes
Inline code: \`rm -rf /\` must stay literal.
Command sub attempt: \$(touch $SENTINEL)
Backtick cmd attempt: \`touch $SENTINEL\`
Dollars: \$HOME \${PATH} \$5.00 and 100% done
Quotes: "double" and 'single' and \`mixed\`
Trailing pipe-ish: foo | bar && baz ; qux
EOF
BODY="$(cat "$BODY_FILE")"
# Mock tea: resolve a mosaicstack login, then capture the --description verbatim.
cat > "$BIN_DIR/tea" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
if [[ "$*" == "login list --output json" ]]; then
cat <<'JSON'
[
{"name":"mosaicstack","url":"https://git.mosaicstack.dev","user":"jason.woltje"}
]
JSON
exit 0
fi
if [[ "${1:-}" == "issue" && "${2:-}" == "create" ]]; then
desc=""
while [[ $# -gt 0 ]]; do
case "$1" in
--description) desc="$2"; shift 2 ;;
*) shift ;;
esac
done
printf '%s' "$desc" > "$MOSAIC_TEST_RECEIVED"
echo "#1 created"
exit 0
fi
exit 0
SH
chmod +x "$BIN_DIR/tea"
(
cd "$REPO_DIR"
PATH="$BIN_DIR:$PATH" \
MOSAIC_TEST_RECEIVED="$RECEIVED_FILE" \
"$SCRIPT_DIR/issue-create.sh" -t "Body safety test" -b "$BODY"
) >/dev/null
# 1. No command substitution executed anywhere in the pipeline.
if [[ -e "$SENTINEL" ]]; then
echo "FAIL: injected command substitution executed (sentinel file created): $SENTINEL" >&2
exit 1
fi
# 2. tea actually received the body (issue create path taken, not silently dropped).
if [[ ! -f "$RECEIVED_FILE" ]]; then
echo "FAIL: tea issue create was never invoked with a --description" >&2
exit 1
fi
# 3. The description tea received is byte-for-byte the original body.
if [[ "$(cat "$RECEIVED_FILE")" != "$BODY" ]]; then
echo "FAIL: body was not preserved verbatim through issue-create.sh" >&2
echo "--- expected ---" >&2; printf '%s\n' "$BODY" >&2
echo "--- received ---" >&2; cat "$RECEIVED_FILE" >&2
exit 1
fi
echo "issue-create.sh Markdown body-safety regression harness passed"