diff --git a/docs/scratchpads/t_5aab9cc8-pr-merge-eval-injection.md b/docs/scratchpads/t_5aab9cc8-pr-merge-eval-injection.md new file mode 100644 index 0000000..23ce55c --- /dev/null +++ b/docs/scratchpads/t_5aab9cc8-pr-merge-eval-injection.md @@ -0,0 +1,48 @@ +# t_5aab9cc8 — pr-merge.sh eval injection remediation + +## Objective + +Remediate PR #521 review blocker: `packages/mosaic/framework/tools/git/pr-merge.sh` must reject non-numeric PR numbers before metadata lookup/merge and must not use `eval` for GitHub merge execution. + +## Scope + +- Shell wrapper only: `packages/mosaic/framework/tools/git/pr-merge.sh` +- Focused regression harness: `packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` +- No API/frontend/infra surfaces. + +## Acceptance Criteria + +- AC1: `PR_NUMBER` is validated as digits-only immediately after required-argument parsing, before metadata lookup. +- AC2: GitHub merge path uses a quoted argv array, not command-string construction plus `eval`. +- AC3: Focused tests prove PR-number metacharacters are rejected and cannot execute injected shell commands on GitHub path. +- AC4: Focused tests prove PR-number metacharacters are rejected on Gitea path before tea/curl merge calls. +- AC5: Existing Gitea empty-uid fallback behavior remains green. +- AC6: Syntax, shellcheck where available, focused harness, and relevant repo gates are rerun or absence documented. + +## Plan + +1. Add failing regression tests for GitHub eval injection and Gitea invalid PR rejection. +2. Implement fail-closed PR number validation before metadata lookup. +3. Replace GitHub `eval` command with argv array execution. +4. Run required validation and update this scratchpad with evidence. +5. Commit, queue-guard, push branch, update PR #521. + +## TDD Log + +- RED: `AGENT_WORK_ROOT="$HERMES_KANBAN_WORKSPACE/work" bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` failed on vulnerable code with `Expected GitHub metacharacter PR number to be rejected` and showed the injected PR number reached the GitHub merge path. +- GREEN: Added digits-only validation before metadata lookup and replaced GitHub `eval` with an argv array. The focused harness now passes and verifies invalid PR numbers are rejected before GitHub `gh` calls and before Gitea `tea`/`curl` calls. + +## Validation Evidence + +- PASS: `AGENT_WORK_ROOT="$HERMES_KANBAN_WORKSPACE/work" bash -n packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` +- PASS: `shellcheck -x packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` +- PASS: `AGENT_WORK_ROOT="$HERMES_KANBAN_WORKSPACE/work" bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` +- PASS: `pnpm --filter @mosaicstack/mosaic... build` +- PASS: `pnpm --filter @mosaicstack/mosaic lint` +- PASS: `pnpm --filter @mosaicstack/mosaic typecheck` +- PASS: `pnpm --filter @mosaicstack/mosaic test` — 32 files / 291 tests passed. +- REVIEW: `/home/hermes/.config/mosaic/tools/codex/codex-code-review.sh --uncommitted` could not run due Codex 401 Unauthorized. Independent delegate review completed read-only with PASS / no blockers; non-blocking suggestion to assert GitHub mock log remains empty was applied. + +## Risks / Blockers + +- No active blockers. diff --git a/packages/mosaic/framework/tools/git/pr-merge.sh b/packages/mosaic/framework/tools/git/pr-merge.sh index 8e472f9..dd94d14 100755 --- a/packages/mosaic/framework/tools/git/pr-merge.sh +++ b/packages/mosaic/framework/tools/git/pr-merge.sh @@ -70,6 +70,11 @@ if [[ -z "$PR_NUMBER" ]]; then usage fi +if [[ ! "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "Error: Invalid PR number '$PR_NUMBER'. PR number must contain digits only." >&2 + exit 1 +fi + if [[ "$MERGE_METHOD" != "squash" ]]; then echo "Error: Mosaic policy enforces squash merge only. Received '$MERGE_METHOD'." >&2 exit 1 @@ -190,9 +195,9 @@ PY case "$PLATFORM" in github) - CMD="gh pr merge $PR_NUMBER --squash" - [[ "$DELETE_BRANCH" == true ]] && CMD="$CMD --delete-branch" - eval "$CMD" + cmd=(gh pr merge "$PR_NUMBER" --squash) + [[ "$DELETE_BRANCH" == true ]] && cmd+=(--delete-branch) + "${cmd[@]}" ;; gitea) HOST=$(get_remote_host) || { diff --git a/packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh b/packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh index 9a3e8a9..d64aa0e 100755 --- a/packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh +++ b/packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh @@ -142,4 +142,75 @@ if ! grep -q '/api/v1/repos/mosaicstack/stack/pulls/123/merge' "$LOG_FILE"; then exit 1 fi +SENTINEL="$SANDBOX/injected-sentinel" +INJECTION="123; touch $SENTINEL #" + +cat > "$MOCK_BIN/gh" <<'EOF' +#!/bin/bash +set -euo pipefail +printf 'gh %q ' "$@" >> "$PR_MERGE_TEST_LOG" +printf '\n' >> "$PR_MERGE_TEST_LOG" +if [[ "$*" == *"pr view"* ]]; then + cat <<'JSON' +{"number":123,"title":"mock","baseRefName":"main","headRefName":"feature/mock"} +JSON + exit 0 +fi +if [[ "$*" == *"pr merge"* ]]; then + exit 0 +fi +echo "unexpected gh invocation: $*" >&2 +exit 98 +EOF +chmod +x "$MOCK_BIN/gh" + +cd "$REPO_DIR" +git remote set-url origin https://github.com/mosaicstack/stack.git +: > "$LOG_FILE" +rm -f "$SENTINEL" +if "$SCRIPT_DIR/pr-merge.sh" -n "$INJECTION" -m squash --skip-queue-guard > "$OUTPUT" 2>&1; then + echo "Expected GitHub metacharacter PR number to be rejected." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$OUTPUT" >&2 + exit 1 +fi +if [[ -e "$SENTINEL" ]]; then + echo "GitHub metacharacter PR number executed injected shell command." >&2 + exit 1 +fi +if [[ -s "$LOG_FILE" ]]; then + echo "GitHub metacharacter PR number should be rejected before gh calls." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$LOG_FILE" >&2 + exit 1 +fi +if ! grep -q 'Invalid PR number' "$OUTPUT"; then + echo "Expected invalid PR number error for GitHub metacharacter input." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$OUTPUT" >&2 + exit 1 +fi + +cd "$REPO_DIR" +git remote set-url origin https://git.mosaicstack.dev/mosaicstack/stack.git +export GITEA_LOGIN="git.mosaicstack.dev" +: > "$LOG_FILE" +rm -f "$SENTINEL" +if "$SCRIPT_DIR/pr-merge.sh" -n "$INJECTION" -m squash --skip-queue-guard > "$OUTPUT" 2>&1; then + echo "Expected Gitea metacharacter PR number to be rejected." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$OUTPUT" >&2 + exit 1 +fi +if [[ -e "$SENTINEL" ]]; then + echo "Gitea metacharacter PR number executed injected shell command." >&2 + exit 1 +fi +if [[ -s "$LOG_FILE" ]]; then + echo "Gitea metacharacter PR number should be rejected before tea/curl calls." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$LOG_FILE" >&2 + exit 1 +fi +if ! grep -q 'Invalid PR number' "$OUTPUT"; then + echo "Expected invalid PR number error for Gitea metacharacter input." >&2 + sed 's/redacted-test-token/***REDACTED***/g' "$OUTPUT" >&2 + exit 1 +fi + echo "pr-merge.sh Gitea fallback regression passed"