From 719c6ac3db6fce8134a57dcb6c90f777c264f15e Mon Sep 17 00:00:00 2001 From: "jason.woltje" Date: Thu, 18 Jun 2026 21:35:32 +0000 Subject: [PATCH] fix(framework/tools): eval injection, broken JSON, tmpfile leak (#549) --- .../t-a292e96f-gitea-pr-metadata.md | 45 ++++++ .../framework/tools/git/issue-assign.sh | 21 ++- .../mosaic/framework/tools/git/issue-edit.sh | 32 ++-- .../framework/tools/git/milestone-create.sh | 13 +- .../mosaic/framework/tools/git/pr-metadata.sh | 16 +- .../tools/git/test-pr-metadata-gitea.sh | 149 +++++++++++++++++- 6 files changed, 244 insertions(+), 32 deletions(-) diff --git a/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md b/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md index 7ae4dfd..b12cddb 100644 --- a/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md +++ b/docs/scratchpads/t-a292e96f-gitea-pr-metadata.md @@ -51,3 +51,48 @@ This repository currently has no root `CHANGELOG.md`; the scratchpad and `docs/T - PR #1908: `Dry run: would merge PR #1908 on git.uscllc.com with authenticated Gitea API fallback (base=main, method=squash).` - PR: `https://git.mosaicstack.dev/mosaicstack/stack/pulls/518`, branch `fix/t-a292e96f-gitea-pr-metadata`. - CI: Recent PR/push pipelines failed before clone/test execution due Woodpecker/Kubernetes PVC API timeout: `dial tcp 10.43.0.1:443: i/o timeout`. No repository test step executed in CI; local targeted verification above remains clean. + +## 2026-06-18 — PR #549 functional blocker remediation + +### Assignment + +Coordinator `mos-claude` assigned remediation for PR #549: fix `packages/mosaic/framework/tools/git/pr-metadata.sh` tmpfile cleanup where an `EXIT` trap references function-local `body_file` after the function returns inside `RAW=$(...)`, producing `body_file: unbound variable` on the authenticated success path and failing to clean up safely on early `set -e` exits. + +### Plan + +1. Add a non-vacuous Gitea test that exercises `curl_gitea_pull` with stubbed `curl` and `GITEA_TOKEN` instead of `MOSAIC_GITEA_PR_METADATA_RAW_FILE`. +2. Prove the new test is RED against the current PR head. +3. Replace the function-local `EXIT` cleanup with robust function-scoped tmpfile cleanup. +4. Re-run targeted tests, `bash -n`, and review gates; commit and push branch only. Do not merge. + +### Constraints / assumptions + +- Do not modify prior injection/JSON fixes in `issue-edit`, `issue-assign`, or `milestone-create`. +- Worker role: do not modify `docs/TASKS.md`; orchestrator remains the single writer. +- Budget: no explicit token cap provided; keep scope to shell wrapper + targeted regression harness. + +### Remediation results + +- Rebased `fix/tooling-eval-injection-jq-json` onto `origin/main`; branch was already current. +- Added a curl-stub regression path that does not use `MOSAIC_GITEA_PR_METADATA_RAW_FILE`, so it exercises `curl_gitea_pull` and its temp body file. +- RED evidence: copied the new harness next to the pre-fix `HEAD` version of `pr-metadata.sh`; `MOSAIC_TEST_WORK_DIR=$PWD/.mosaic-test-work/pr-metadata-red-work .../test-pr-metadata-gitea.sh` failed with `body_file: unbound variable` on the curl success path. +- Fix: replaced `EXIT` temp-file cleanup with a `RETURN`-scoped cleanup function that removes the body file while the function-local variable is still in scope, preserves the original return status, and clears the `RETURN` trap. +- GREEN evidence: + - `MOSAIC_TEST_WORK_DIR=$PWD/.mosaic-test-work/pr-metadata-gitea-current packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed. + - `bash -n packages/mosaic/framework/tools/git/pr-metadata.sh packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed. + - `shellcheck -x -P . -e SC1090 packages/mosaic/framework/tools/git/pr-metadata.sh packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh` passed. + +### Review remediation + +- Codex review returned one should-fix: the early-exit test used `chmod 000`, which is not root-safe in container CI. +- Remediation: changed the stubbed 2xx/cat-failure mode to replace the curl output with a broken symlink, which fails deterministically even as root and still validates cleanup via `rm -f -- "$body_file"`. + +### Second review remediation + +- Codex review found the 2xx `cat "$body_file"` read could be masked under command substitution semantics because the branch returned 0 unconditionally. +- Remediation: both authenticated 2xx branches now use `cat "$body_file" || return $?` before returning success. +- Strengthened the broken-symlink test to require the body-read failure and reject the later `Gitea API returned non-JSON` parse-failure path, so the test verifies the helper-level failure propagation rather than eventual downstream failure. + +### Final review gate + +- Codex review after remediation: approved (`0 blockers, 0 should-fix, 0 suggestions`). diff --git a/packages/mosaic/framework/tools/git/issue-assign.sh b/packages/mosaic/framework/tools/git/issue-assign.sh index f3a15cb..6151b0e 100755 --- a/packages/mosaic/framework/tools/git/issue-assign.sh +++ b/packages/mosaic/framework/tools/git/issue-assign.sh @@ -98,27 +98,32 @@ case "$PLATFORM" in ;; gitea) # tea issue edit syntax - REPO_ARGS=$(get_gitea_repo_args) || { - echo "Error: Could not resolve Gitea repo/login args for remote host" >&2 + REPO_SLUG=$(get_repo_slug) || { + echo "Error: Could not resolve Gitea repo slug from remote" >&2 exit 1 } - CMD="tea issue edit $ISSUE $REPO_ARGS" + REPO_LOGIN=$(get_gitea_login) || { + echo "Error: Could not resolve Gitea login for remote host" >&2 + exit 1 + } + REPO_ARGS=(--repo "$REPO_SLUG" --login "$REPO_LOGIN") + CMD=(tea issue edit "$ISSUE" "${REPO_ARGS[@]}") NEEDS_EDIT=false if [[ -n "$ASSIGNEE" ]]; then # tea uses --assignees flag - CMD="$CMD --assignees \"$ASSIGNEE\"" + CMD+=(--assignees "$ASSIGNEE") NEEDS_EDIT=true fi if [[ -n "$LABELS" ]]; then # tea uses --labels flag (replaces existing) - CMD="$CMD --labels \"$LABELS\"" + CMD+=(--labels "$LABELS") NEEDS_EDIT=true fi if [[ -n "$MILESTONE" ]]; then - MILESTONE_ID=$(tea milestones list $REPO_ARGS 2>/dev/null | grep -E "^\s*[0-9]+" | grep "$MILESTONE" | awk '{print $1}' | head -1) + MILESTONE_ID=$(tea milestones list "${REPO_ARGS[@]}" 2>/dev/null | grep -E "^\s*[0-9]+" | grep "$MILESTONE" | awk '{print $1}' | head -1) if [[ -n "$MILESTONE_ID" ]]; then - CMD="$CMD --milestone $MILESTONE_ID" + CMD+=(--milestone "$MILESTONE_ID") NEEDS_EDIT=true else echo "Warning: Could not find milestone '$MILESTONE'" >&2 @@ -126,7 +131,7 @@ case "$PLATFORM" in fi if [[ "$NEEDS_EDIT" == true ]]; then - eval "$CMD" + "${CMD[@]}" echo "Issue #$ISSUE updated successfully" else echo "No changes specified" diff --git a/packages/mosaic/framework/tools/git/issue-edit.sh b/packages/mosaic/framework/tools/git/issue-edit.sh index 0ffcc0b..865af4f 100755 --- a/packages/mosaic/framework/tools/git/issue-edit.sh +++ b/packages/mosaic/framework/tools/git/issue-edit.sh @@ -63,24 +63,28 @@ fi detect_platform >/dev/null if [[ "$PLATFORM" == "github" ]]; then - CMD="gh issue edit $ISSUE_NUMBER" - [[ -n "$TITLE" ]] && CMD="$CMD --title \"$TITLE\"" - [[ -n "$BODY" ]] && CMD="$CMD --body \"$BODY\"" - [[ -n "$LABELS" ]] && CMD="$CMD --add-label \"$LABELS\"" - [[ -n "$MILESTONE" ]] && CMD="$CMD --milestone \"$MILESTONE\"" - eval $CMD + CMD=(gh issue edit "$ISSUE_NUMBER") + [[ -n "$TITLE" ]] && CMD+=(--title "$TITLE") + [[ -n "$BODY" ]] && CMD+=(--body "$BODY") + [[ -n "$LABELS" ]] && CMD+=(--add-label "$LABELS") + [[ -n "$MILESTONE" ]] && CMD+=(--milestone "$MILESTONE") + "${CMD[@]}" echo "Updated GitHub issue #$ISSUE_NUMBER" elif [[ "$PLATFORM" == "gitea" ]]; then - REPO_ARGS=$(get_gitea_repo_args) || { - echo "Error: Could not resolve Gitea repo/login args for remote host" >&2 + REPO_SLUG=$(get_repo_slug) || { + echo "Error: Could not resolve Gitea repo slug from remote" >&2 exit 1 } - CMD="tea issue edit $ISSUE_NUMBER $REPO_ARGS" - [[ -n "$TITLE" ]] && CMD="$CMD --title \"$TITLE\"" - [[ -n "$BODY" ]] && CMD="$CMD --description \"$BODY\"" - [[ -n "$LABELS" ]] && CMD="$CMD --add-labels \"$LABELS\"" - [[ -n "$MILESTONE" ]] && CMD="$CMD --milestone \"$MILESTONE\"" - eval $CMD + REPO_LOGIN=$(get_gitea_login) || { + echo "Error: Could not resolve Gitea login for remote host" >&2 + exit 1 + } + CMD=(tea issue edit "$ISSUE_NUMBER" --repo "$REPO_SLUG" --login "$REPO_LOGIN") + [[ -n "$TITLE" ]] && CMD+=(--title "$TITLE") + [[ -n "$BODY" ]] && CMD+=(--description "$BODY") + [[ -n "$LABELS" ]] && CMD+=(--add-labels "$LABELS") + [[ -n "$MILESTONE" ]] && CMD+=(--milestone "$MILESTONE") + "${CMD[@]}" echo "Updated Gitea issue #$ISSUE_NUMBER" else echo "Error: Unknown platform" diff --git a/packages/mosaic/framework/tools/git/milestone-create.sh b/packages/mosaic/framework/tools/git/milestone-create.sh index c5ffb52..8f5c371 100755 --- a/packages/mosaic/framework/tools/git/milestone-create.sh +++ b/packages/mosaic/framework/tools/git/milestone-create.sh @@ -99,10 +99,15 @@ fi case "$PLATFORM" in github) # GitHub uses the API for milestone creation - JSON_PAYLOAD="{\"title\":\"$TITLE\"" - [[ -n "$DESCRIPTION" ]] && JSON_PAYLOAD="$JSON_PAYLOAD,\"description\":\"$DESCRIPTION\"" - [[ -n "$DUE_DATE" ]] && JSON_PAYLOAD="$JSON_PAYLOAD,\"due_on\":\"${DUE_DATE}T00:00:00Z\"" - JSON_PAYLOAD="$JSON_PAYLOAD}" + # Use jq to safely construct JSON so titles/descriptions containing + # quotes or special characters do not corrupt the payload (F-07). + JSON_PAYLOAD=$(jq -n \ + --arg t "$TITLE" \ + --arg d "$DESCRIPTION" \ + --arg due "${DUE_DATE}" \ + '{"title": $t} + + (if $d != "" then {"description": $d} else {} end) + + (if $due != "" then {"due_on": ($due + "T00:00:00Z")} else {} end)') gh api repos/:owner/:repo/milestones --method POST --input - <<< "$JSON_PAYLOAD" echo "Milestone '$TITLE' created successfully" diff --git a/packages/mosaic/framework/tools/git/pr-metadata.sh b/packages/mosaic/framework/tools/git/pr-metadata.sh index b6373bf..5521150 100755 --- a/packages/mosaic/framework/tools/git/pr-metadata.sh +++ b/packages/mosaic/framework/tools/git/pr-metadata.sh @@ -57,12 +57,20 @@ curl_gitea_pull() { local token basic_auth raw_code body_file http_code body_file=$(mktemp) + # shellcheck disable=SC2329 # Invoked by the RETURN trap below. + cleanup_gitea_pull_body() { + local status=$? + rm -f -- "$body_file" + trap - RETURN + return "$status" + } + trap cleanup_gitea_pull_body RETURN + token=$(get_gitea_token "$HOST" || true) if [[ -n "$token" ]]; then raw_code=$(curl -sS -w '%{http_code}' -o "$body_file" -H "User-Agent: curl/8" -H "Authorization: token $token" "$api_url" || true) if [[ "$raw_code" =~ ^2 ]]; then - cat "$body_file" - rm -f "$body_file" + cat "$body_file" || return $? return 0 fi http_code="$raw_code" @@ -72,8 +80,7 @@ curl_gitea_pull() { if [[ -n "$basic_auth" ]]; then raw_code=$(curl -sS -w '%{http_code}' -o "$body_file" -u "$basic_auth" -H "User-Agent: curl/8" "$api_url" || true) if [[ "$raw_code" =~ ^2 ]]; then - cat "$body_file" - rm -f "$body_file" + cat "$body_file" || return $? return 0 fi http_code="$raw_code" @@ -96,7 +103,6 @@ except Exception: message = open(path, encoding="utf-8", errors="replace").read()[:200] or "empty response" print(f"Error: Gitea pull request API request failed with HTTP {code}: {message}") PY - rm -f "$body_file" return 1 } diff --git a/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh b/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh index e98b2e7..41ebefa 100755 --- a/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh +++ b/packages/mosaic/framework/tools/git/test-pr-metadata-gitea.sh @@ -7,9 +7,10 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" WORK_DIR="${MOSAIC_TEST_WORK_DIR:-$PWD/.mosaic-test-work/pr-metadata-gitea}" REPO_DIR="$WORK_DIR/repo" FIXTURE_DIR="$WORK_DIR/fixtures" +STUB_DIR="$WORK_DIR/stubs" rm -rf "$WORK_DIR" -mkdir -p "$REPO_DIR" "$FIXTURE_DIR" +mkdir -p "$REPO_DIR" "$FIXTURE_DIR" "$STUB_DIR" git -C "$REPO_DIR" init -q git -C "$REPO_DIR" remote add origin https://git.uscllc.com/USC/uconnect.git @@ -56,6 +57,150 @@ cat > "$FIXTURE_DIR/gitea-error.json" <<'JSON' {"message": "user does not exist [uid: 0, name: ]", "url": "https://git.uscllc.com/api/swagger"} JSON +cat > "$STUB_DIR/curl" <<'SH' +#!/usr/bin/env bash +set -euo pipefail + +output_file="" +while [[ $# -gt 0 ]]; do + case "$1" in + -o) + output_file="$2" + shift 2 + ;; + -w|-H|-u) + shift 2 + ;; + -s|-S|-sS) + shift + ;; + *) + shift + ;; + esac +done + +if [[ -z "$output_file" ]]; then + echo "curl stub expected -o " >&2 + exit 2 +fi + +case "${MOSAIC_STUB_CURL_MODE:-success}" in + success) + cat > "$output_file" <<'JSON' +{ + "number": 1910, + "title": "Live curl path", + "state": "open", + "user": {"login": "edith"}, + "head": {"ref": "fix/live-curl-path"}, + "base": {"ref": "main"}, + "html_url": "https://git.example.test/acme/widgets/pulls/1910" +} +JSON + printf '200' + ;; + cat-fails-after-2xx) + rm -f -- "$output_file" + ln -s /nonexistent/pr-metadata-body "$output_file" + printf '200' + ;; + *) + echo "unknown MOSAIC_STUB_CURL_MODE=${MOSAIC_STUB_CURL_MODE:-}" >&2 + exit 2 + ;; +esac +SH +chmod +x "$STUB_DIR/curl" + +assert_tmpdir_empty() { + local tmpdir="$1" leftover + leftover=$(find "$tmpdir" -mindepth 1 -print -quit) + if [[ -n "$leftover" ]]; then + echo "Expected tmpfile cleanup, found leftover: $leftover" >&2 + find "$tmpdir" -mindepth 1 -maxdepth 1 -ls >&2 + exit 1 + fi +} + +run_curl_success_case() { + local tmpdir="$WORK_DIR/tmp-success" stderr_file="$WORK_DIR/curl-success.stderr" + local output status + mkdir -p "$tmpdir" + + set +e + output=$(cd "$REPO_DIR" && \ + PATH="$STUB_DIR:$PATH" \ + TMPDIR="$tmpdir" \ + GITEA_TOKEN="stub-token" \ + GITEA_URL="https://git.example.test" \ + MOSAIC_STUB_CURL_MODE="success" \ + "$SCRIPT_DIR/pr-metadata.sh" -n 1910 2>"$stderr_file") + status=$? + set -e + + if [[ "$status" -ne 0 ]]; then + echo "Expected curl success path to pass, got status $status" >&2 + cat "$stderr_file" >&2 + exit 1 + fi + if grep -q "unbound variable" "$stderr_file"; then + echo "curl success path emitted unbound-variable cleanup noise" >&2 + cat "$stderr_file" >&2 + exit 1 + fi + assert_tmpdir_empty "$tmpdir" + + PR_METADATA_OUTPUT="$output" python3 - <<'PY' +import json +import os + +data = json.loads(os.environ["PR_METADATA_OUTPUT"]) +assert data["number"] == 1910, data +assert data["baseRefName"] == "main", data +assert data["headRefName"] == "fix/live-curl-path", data +PY +} + +run_curl_early_exit_cleanup_case() { + local tmpdir="$WORK_DIR/tmp-early-exit" stderr_file="$WORK_DIR/curl-early-exit.stderr" + local output status + mkdir -p "$tmpdir" + + set +e + output=$(cd "$REPO_DIR" && \ + PATH="$STUB_DIR:$PATH" \ + TMPDIR="$tmpdir" \ + GITEA_TOKEN="stub-token" \ + GITEA_URL="https://git.example.test" \ + MOSAIC_STUB_CURL_MODE="cat-fails-after-2xx" \ + "$SCRIPT_DIR/pr-metadata.sh" -n 1910 2>"$stderr_file") + status=$? + set -e + + if [[ "$status" -eq 0 ]]; then + echo "Expected unreadable 2xx body path to fail" >&2 + printf '%s\n' "$output" >&2 + exit 1 + fi + if grep -q "unbound variable" "$stderr_file"; then + echo "curl early-exit path emitted unbound-variable cleanup noise" >&2 + cat "$stderr_file" >&2 + exit 1 + fi + if ! grep -q "No such file or directory" "$stderr_file"; then + echo "Expected body-read failure from broken symlink path" >&2 + cat "$stderr_file" >&2 + exit 1 + fi + if grep -q "Gitea API returned non-JSON" "$stderr_file"; then + echo "curl helper masked body-read failure as later JSON parsing failure" >&2 + cat "$stderr_file" >&2 + exit 1 + fi + assert_tmpdir_empty "$tmpdir" +} + run_case() { local fixture="$1" expected_number="$2" expected_head="$3" local output @@ -77,6 +222,8 @@ PY run_case "$FIXTURE_DIR/gitea-standard.json" 1905 edith/t_39ce717c-authentik-smoke-gate run_case "$FIXTURE_DIR/gitea-fallback.json" 1908 fix/fallback-head run_case "$FIXTURE_DIR/gitea-refs-pull-label.json" 1908 fix/t_23fa9e1d-portal-health-backend +run_curl_success_case +run_curl_early_exit_cleanup_case if cd "$REPO_DIR" && MOSAIC_GITEA_PR_METADATA_RAW_FILE="$FIXTURE_DIR/gitea-error.json" "$SCRIPT_DIR/pr-metadata.sh" -n 1909 >/dev/null 2>"$WORK_DIR/error.log"; then echo "Expected API error fixture to fail" >&2