diff --git a/.github/scripts/add-to-project/classify-initiative.sh b/.github/scripts/add-to-project/classify-initiative.sh index ed9c0aea1..e9b4b6bdc 100755 --- a/.github/scripts/add-to-project/classify-initiative.sh +++ b/.github/scripts/add-to-project/classify-initiative.sh @@ -276,16 +276,20 @@ sweep_project() { decided=$(decide_for_signature "${sig}") init="${decided%%$'\t'*}" theme="${decided#*$'\t'}" + # Titles are contributor-controlled: neutralise runner command markers + # before any log line below embeds them (#1384). + local log_title + log_title=$(atp_log_safe "${title}") if [ -z "${init}" ]; then unmatched=$((unmatched + 1)) - printf 'UNMATCHED %-20s «%s»\n' "${repo:-draft}" "${title}" + printf 'UNMATCHED %-20s «%s»\n' "${repo:-draft}" "${log_title}" continue fi optid="${CI_INIT_OPT[${init}]:-}" if [ -z "${optid}" ]; then - printf '::warning::rule matched Initiative %q which is not a live project option; skipping «%s»\n' "${init}" "${title}" >&2 + printf '::warning::rule matched Initiative %q which is not a live project option; skipping «%s»\n' "${init}" "${log_title}" >&2 skipped=$((skipped + 1)) continue fi @@ -296,11 +300,11 @@ sweep_project() { # (this is a fill-blanks-only sweep, so retries are idempotent). Mirrors # reconcile-backlog.sh's per-item error isolation. if ! set_item_single_select_value "${item_id}" "${CI_INIT_FIELD_ID}" "${optid}"; then - printf '::warning::failed to set Initiative %q on «%s» (transient?); will retry next sweep\n' "${init}" "${title}" >&2 + printf '::warning::failed to set Initiative %q on «%s» (transient?); will retry next sweep\n' "${init}" "${log_title}" >&2 failed=$((failed + 1)) continue fi - printf 'MATCH %-22s <- %-20s «%s»\n' "${init}" "${repo:-draft}" "${title}" + printf 'MATCH %-22s <- %-20s «%s»\n' "${init}" "${repo:-draft}" "${log_title}" matched=$((matched + 1)) # Theme is best-effort: co-assign only when the field and matching option @@ -309,7 +313,7 @@ sweep_project() { if [ -n "${theme}" ] && [ -n "${CI_THEME_FIELD_ID}" ]; then local topt="${CI_THEME_OPT[${theme}]:-}" if [ -n "${topt}" ] && ! set_item_single_select_value "${item_id}" "${CI_THEME_FIELD_ID}" "${topt}"; then - printf '::warning::failed to set Theme %q on «%s» (transient?); retry requires RECLASSIFY=all (Initiative already set)\n' "${theme}" "${title}" >&2 + printf '::warning::failed to set Theme %q on «%s» (transient?); retry requires RECLASSIFY=all (Initiative already set)\n' "${theme}" "${log_title}" >&2 fi fi done < <(printf '%s' "${items}" | jq -c '.[]') diff --git a/.github/scripts/add-to-project/lib.sh b/.github/scripts/add-to-project/lib.sh index 63d214c33..79b8872fb 100644 --- a/.github/scripts/add-to-project/lib.sh +++ b/.github/scripts/add-to-project/lib.sh @@ -29,6 +29,26 @@ _atp_require_env() { fi } +# atp_log_safe — print an untrusted value (an issue / PR / draft title) +# so it is safe to embed in a runner log line (combsmith/.github#1384). The +# Actions runner treats `::cmd::` at the start of a line, and the legacy +# `##[cmd]` marker anywhere in a line, as workflow commands, so a raw title +# could forge annotations or `stop-commands` the rest of the step. CR/LF become +# spaces (no new line can start inside the value) and each `::` / `##[` gets a +# zero-width space (U+200B) between its characters, which keeps the title +# readable in the log while no longer matching either command syntax. +atp_log_safe() { + local v="${1-}" zwsp=$'\xe2\x80\x8b' + v="${v//$'\r'/ }" + v="${v//$'\n'/ }" + v="${v//'##['/#${zwsp}#[}" + # Loop: one pass over ':::' leaves '::' at the tail. + while [[ "${v}" == *'::'* ]]; do + v="${v//'::'/:${zwsp}:}" + done + printf '%s' "${v}" +} + # Prefetched board-membership cache (optional; batch reconcile only). # reconcile-backlog.sh scans the whole fleet's open issues/PRs and would # otherwise hit the API once PER item on both hot paths — a redundant @@ -241,7 +261,7 @@ add_draft_item() { local body="$2" if [ "${DRY_RUN:-}" = "1" ]; then - printf '[dry-run] would add draft: %s\n' "${title}" + printf '[dry-run] would add draft: %s\n' "$(atp_log_safe "${title}")" return 0 fi diff --git a/.github/scripts/auto-rebase/README.md b/.github/scripts/auto-rebase/README.md index 5d246e11d..e3ecfcd22 100644 --- a/.github/scripts/auto-rebase/README.md +++ b/.github/scripts/auto-rebase/README.md @@ -37,6 +37,7 @@ Source the file, then call: | Function | Input | Returns | |----------|-------|---------| | `auto_rebase_post_comment_best_effort PR_NUMBER REPO BODY` | PR number, `owner/repo`, comment body | always `0` — posts the comment; on failure logs a `::warning::` and swallows the error | +| `auto_rebase_shell_quote VALUE` | any string (e.g. `origin/`) | prints VALUE as one POSIX-shell word: unchanged if only `[A-Za-z0-9_./+-]`, else single-quoted (`'` → `'\''`). Used for the copy-paste `git rebase` / `git merge` commands in comments (issue #1396) | ### Why best-effort (issue #594) diff --git a/.github/scripts/auto-rebase/lib/comments.sh b/.github/scripts/auto-rebase/lib/comments.sh index 6288ae85b..ac2af9539 100644 --- a/.github/scripts/auto-rebase/lib/comments.sh +++ b/.github/scripts/auto-rebase/lib/comments.sh @@ -24,3 +24,20 @@ auto_rebase_post_comment_best_effort() { || echo "::warning::could not post conflict comment on #$pr_number (comment cap / API error) — continuing" >&2 return 0 } + +# auto_rebase_shell_quote VALUE +# Prints VALUE as a single POSIX-shell word, for pasting into the manual +# recovery commands the comments publish (`git rebase `, issue #1396). +# A git branch name may legally contain $, `, ;, |, &, (, ), quotes, so a raw +# base branch could run extra commands when a maintainer copies the snippet. +# Values made only of [A-Za-z0-9_./+-] are printed unchanged so ordinary refs +# stay readable (`origin/main`); anything else is single-quoted, with each +# embedded ' written as '\'' . Works in bash, zsh and sh alike (unlike %q). +auto_rebase_shell_quote() { + local v="$1" + if [[ -n "$v" && "$v" != *[!A-Za-z0-9_./+-]* ]]; then + printf '%s' "$v" + else + printf "'%s'" "${v//\'/\'\\\'\'}" + fi +} diff --git a/.github/workflows/agent-shield-reusable.yml b/.github/workflows/agent-shield-reusable.yml index a8ed224cc..1cfb353bc 100644 --- a/.github/workflows/agent-shield-reusable.yml +++ b/.github/workflows/agent-shield-reusable.yml @@ -52,13 +52,24 @@ jobs: # --- Deep security scan via AgentShield CLI --- # Uses ecc-agentshield (https://github.com/affaan-m/agentshield) # 102 rules: secrets, permissions, hooks, MCP servers, agent config + # + # npm reads "project" config from the .npmrc of its local prefix (cwd), so + # running npx inside the PR checkout would let the scanned PR's own .npmrc + # (script-shell, registry, ...) control the scanner (#1405). Run npm from + # an empty prefix under $RUNNER_TEMP instead and pass the checkout as the + # scan target. - name: AgentShield Security Scan env: AS_VERSION: ${{ inputs.agentshield-version }} AS_SEVERITY: ${{ inputs.min-severity }} run: | - npx --ignore-scripts "ecc-agentshield@${AS_VERSION}" scan \ - --path . \ + scan_target="$PWD" + npm_prefix="$RUNNER_TEMP/agentshield-npm" + mkdir -p "$npm_prefix" + printf '{"private":true}\n' > "$npm_prefix/package.json" + cd "$npm_prefix" + npx --prefix="$npm_prefix" --ignore-scripts "ecc-agentshield@${AS_VERSION}" scan \ + --path "$scan_target" \ --min-severity "${AS_SEVERITY}" \ --format terminal diff --git a/.github/workflows/auto-rebase-reusable.yml b/.github/workflows/auto-rebase-reusable.yml index 968cbbd90..f550c005a 100644 --- a/.github/workflows/auto-rebase-reusable.yml +++ b/.github/workflows/auto-rebase-reusable.yml @@ -195,6 +195,9 @@ jobs: echo " Branch updated" else echo " Update failed: $UPDATE_OUTPUT" + # The base ref goes into copy-paste shell commands below; a branch + # name can contain shell syntax, so quote it (#1396). + MANUAL_BASE_REF=$(auto_rebase_shell_quote "origin/$BASE_BRANCH") if echo "$UPDATE_OUTPUT" | grep -qF "without \`workflows\` permission"; then # GitHub blocks update-branch when merging the base would add @@ -215,7 +218,7 @@ jobs: BLOCKED_BODY+=" but the auto-rebase workflow's token does not have that permission." BLOCKED_BODY+=$'\n\n'"Please rebase this branch manually:" BLOCKED_BODY+=$'\n'"\`\`\`"$'\n'"git fetch origin" - BLOCKED_BODY+=$'\n'"git rebase origin/$BASE_BRANCH" + BLOCKED_BODY+=$'\n'"git rebase $MANUAL_BASE_REF" BLOCKED_BODY+=$'\n'"git push --force-with-lease"$'\n'"\`\`\`" auto_rebase_post_comment_best_effort "$PR_NUMBER" "$REPO" "$BLOCKED_BODY" fi @@ -255,7 +258,7 @@ jobs: CONFLICT_BODY+=" (automatic resolution requires \`GH_PAT_WORKFLOWS\` to be configured):" fi CONFLICT_BODY+=$'\n'"\`\`\`"$'\n'"git fetch origin" - CONFLICT_BODY+=$'\n'"git merge origin/$BASE_BRANCH" + CONFLICT_BODY+=$'\n'"git merge $MANUAL_BASE_REF" CONFLICT_BODY+=$'\n'"# resolve conflicts, then:" CONFLICT_BODY+=$'\n'"git add ."$'\n'"git commit"$'\n'"git push"$'\n'"\`\`\`" # Best-effort: a comment-side failure (e.g. GitHub's 2500-comment diff --git a/scripts/lib/dev-lead-retrigger.sh b/scripts/lib/dev-lead-retrigger.sh index 33866c968..2867cad2b 100644 --- a/scripts/lib/dev-lead-retrigger.sh +++ b/scripts/lib/dev-lead-retrigger.sh @@ -23,7 +23,8 @@ # dl_dev_lead_active # Returns 0 (true) if dev-lead is ALREADY working this issue — either an open PR -# exists on a dev-lead/issue--* branch, or the issue carries the +# exists on a dev-lead/issue--* branch of this repo (not a fork) authored +# by the dev-lead identity (DEV_LEAD_LOGINS, default don-petry), or the issue carries the # `in-progress` label. In that case the caller must NOT cycle the trigger label, # to avoid interrupting or duplicating active work. dl_dev_lead_active() { @@ -31,14 +32,25 @@ dl_dev_lead_active() { # Match the dev-lead branch exactly: "dev-lead/issue--". # Require the trailing "-" so issue 1 does not match branch "issue-12-...". + # The branch name alone is NOT proof (#1391): the open-PR list includes fork + # PRs, whose head branch name the contributor picks. Only count a PR whose head + # lives in this repository (not a fork, not a deleted head repo) and whose + # author is the dev-lead agent's runtime identity (persona-standards.md §5.1: + # dev-lead acts as `don-petry`; override with DEV_LEAD_LOGINS, comma-separated). + # GitHub logins are case-insensitive, so both sides compare in lower case. + # The logins are JSON-encoded by jq, never spliced raw into the filter. # Use --paginate so repos with >100 open PRs are fully scanned. # Fail closed: if the API call itself fails (transient error, rate limit, token # scope issue), treat the issue as active so we do not cycle the label and # accidentally create duplicate work. - local pr_raw pr_count + local pr_raw pr_count logins_json + logins_json=$(printf '%s' "${DEV_LEAD_LOGINS:-don-petry}" \ + | jq -Rc 'split(",") | map(gsub("^\\s+|\\s+$"; "") | ascii_downcase) | map(select(length > 0))') if ! pr_raw=$(gh api "repos/$org/$repo/pulls?state=open&per_page=100" \ --paginate \ - --jq "[.[] | select(.head.ref | type == \"string\" and startswith(\"dev-lead/issue-${issue}-\"))] | length" \ + --jq "[.[] | select(.head.ref | type == \"string\" and startswith(\"dev-lead/issue-${issue}-\")) + | select(.head.repo != null and .head.repo.full_name == .base.repo.full_name) + | select((.user.login // \"\" | ascii_downcase) as \$l | ${logins_json} | any(.[]; . == \$l))] | length" \ 2>/dev/null); then echo "[warn] PR lookup for $org/$repo#$issue failed — treating as active to avoid duplicate work" >&2 return 0 diff --git a/test/scripts/compliance-retrigger/dev-lead-active.bats b/test/scripts/compliance-retrigger/dev-lead-active.bats new file mode 100644 index 000000000..7b81d1e08 --- /dev/null +++ b/test/scripts/compliance-retrigger/dev-lead-active.bats @@ -0,0 +1,137 @@ +#!/usr/bin/env bats +# Tests for dl_dev_lead_active (scripts/lib/dev-lead-retrigger.sh): only a PR +# opened BY the dev-lead agent FROM the repository itself counts as "dev-lead is +# already working this issue" (combsmith/.github#1391). +# +# The open-PR list includes fork PRs, and a fork contributor chooses its head +# branch name freely, so a `dev-lead/issue--*` branch name alone must not +# suppress the compliance retrigger. The `gh` stub below evaluates the real +# `--jq` filter against a fixture, so these tests exercise the actual selector. + +bats_require_minimum_version 1.5.0 + +setup() { + TT_REPO_ROOT="$(cd -- "$BATS_TEST_DIRNAME/../../.." && pwd)" + PULLS_FIXTURE="${BATS_TEST_TMPDIR}/pulls.json" + GH_FAIL_PULLS=0 + unset DEV_LEAD_LOGINS + # shellcheck source=/dev/null + . "${TT_REPO_ROOT}/scripts/lib/dev-lead-retrigger.sh" +} + +# gh stub: apply the caller's --jq filter to the pulls fixture (or to an issue +# with no labels), mimicking `gh api --jq`. +gh() { + local path="" filter="" arg prev="" + for arg in "$@"; do + case "$prev" in --jq) filter="$arg" ;; esac + case "$arg" in repos/*) path="$arg" ;; esac + prev="$arg" + done + case "$path" in + */pulls\?*) + [ "$GH_FAIL_PULLS" -eq 0 ] || return 1 + jq "$filter" "$PULLS_FIXTURE" + ;; + */issues/*/events*) printf '' ;; + */issues/*) printf '{"labels":[]}' | jq -r "$filter" ;; + *) return 1 ;; + esac +} + +# pr_json +pr_json() { + local head_repo + if [ "$2" = "null" ]; then head_repo=null; else head_repo="{\"full_name\":\"$2\"}"; fi + printf '{"number":1,"head":{"ref":"%s","repo":%s},"base":{"ref":"main","repo":{"full_name":"combsmith/widget"}},"user":{"login":"%s"}}' \ + "$1" "$head_repo" "$3" +} + +write_pulls() { + local IFS=, + printf '[%s]' "$*" >"$PULLS_FIXTURE" +} + +@test "active: same-repo dev-lead branch PR authored by the agent identity" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 combsmith/widget don-petry)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] +} + +@test "inactive: a FORK PR on a dev-lead/issue-- branch does not suppress the retrigger" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 mallory/widget mallory)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "inactive: a fork PR is rejected even when its author is the agent login" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 don-petry/widget don-petry)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "inactive: a same-repo dev-lead branch PR by someone other than the agent" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 combsmith/widget some-contributor)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "inactive: a PR whose head repo was deleted (head.repo null) does not count" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 null don-petry)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "active: the agent PR is found among fork decoys" { + write_pulls \ + "$(pr_json dev-lead/issue-7-20261027-1100 mallory/widget mallory)" \ + "$(pr_json dev-lead/issue-7-20261027-1200 combsmith/widget don-petry)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] +} + +@test "issue-number prefix stays exact: issue 1 does not match an issue-12 branch" { + write_pulls "$(pr_json dev-lead/issue-12-20261027-1200 combsmith/widget don-petry)" + run dl_dev_lead_active combsmith widget 1 + [ "$status" -eq 1 ] +} + +@test "DEV_LEAD_LOGINS overrides the trusted agent identity (comma-separated)" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 combsmith/widget other-agent)" + DEV_LEAD_LOGINS="don-petry, other-agent" run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] + DEV_LEAD_LOGINS="don-petry" run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "logins compare case-insensitively: DEV_LEAD_LOGINS casing differs from the API's" { + write_pulls "$(pr_json dev-lead/issue-7-20260101 combsmith/widget don-petry)" + DEV_LEAD_LOGINS="Don-Petry" run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] +} + +@test "logins compare case-insensitively: the API's casing differs from the default" { + write_pulls "$(pr_json dev-lead/issue-7-20260101 combsmith/widget Don-Petry)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] +} + +@test "case-insensitive compare still rejects a different login" { + write_pulls "$(pr_json dev-lead/issue-7-20260101 combsmith/widget Don-Petry2)" + run dl_dev_lead_active combsmith widget 7 + [ "$status" -ne 0 ] +} + +@test "DEV_LEAD_LOGINS values cannot inject into the jq filter" { + write_pulls "$(pr_json dev-lead/issue-7-20261027-1200 combsmith/widget mallory)" + DEV_LEAD_LOGINS='x"] or true or ["' run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 1 ] +} + +@test "fail-closed preserved: a failing PR lookup still reports active" { + write_pulls + GH_FAIL_PULLS=1 + run dl_dev_lead_active combsmith widget 7 + [ "$status" -eq 0 ] + [[ "$output" == *"treating as active"* ]] +} diff --git a/test/workflows/add-to-project/classify-initiative.bats b/test/workflows/add-to-project/classify-initiative.bats index 8bb435167..68c7409b9 100644 --- a/test/workflows/add-to-project/classify-initiative.bats +++ b/test/workflows/add-to-project/classify-initiative.bats @@ -423,3 +423,83 @@ setup_sweep_stub() { run classify_by_rules "make canary-rollout semver-major aware — major-version channels + per-major shim pins | | combsmith/.github-private" [ "$output" = "Release Strategy" ] } + +# --------------------------------------------------------------------------- +# Untrusted titles must not reach runner stdout/stderr as workflow commands +# (combsmith/.github#1384). The Actions runner parses `::cmd::` at the start of +# a line and the legacy `##[cmd]` marker ANYWHERE in a line, so a raw title +# could forge annotations or `stop-commands` the rest of the step. +# --------------------------------------------------------------------------- + +# Fails if any output line carries a runner command the TITLE injected: a legacy +# `##[` marker anywhere, or a line whose first non-blank chars are `::` other +# than the script's own ::warning:: lines. +assert_no_injected_commands() { + local line trimmed + [[ "$output" != *'##['* ]] || { printf 'legacy ##[ marker leaked:\n%s\n' "$output" >&2; return 1; } + while IFS= read -r line; do + trimmed="${line#"${line%%[![:space:]]*}"}" + case "$trimmed" in + '::warning::failed to set '*|'::warning::rule matched '*) : ;; + '::'*) printf 'injected command line: %s\n' "$line" >&2; return 1 ;; + esac + done <<<"$output" +} + +@test "atp_log_safe: an ordinary title passes through unchanged" { + run atp_log_safe "SonarCloud: fix S7635 stubs (#12)" + [ "$status" -eq 0 ] + [ "$output" = "SonarCloud: fix S7635 stubs (#12)" ] +} + +@test "atp_log_safe: CR/LF are flattened so a title cannot start a new runner line" { + run atp_log_safe $'a\n::error::x\r\nb' + [ "$status" -eq 0 ] + [ "${#lines[@]}" -eq 1 ] + [[ "$output" != *$'\r'* ]] +} + +@test "atp_log_safe: '::' and legacy '##[' command markers are neutralised" { + local v + for v in '::error::forged' 'x ##[error]forged' '##[stop-commands]tok' ':::warning:::' '###[x]' 'std::io'; do + run atp_log_safe "$v" + [ "$status" -eq 0 ] + [[ "$output" != *'::'* ]] || { echo "still has :: for $v -> $output"; return 1; } + [[ "$output" != *'##['* ]] || { echo "still has ##[ for $v -> $output"; return 1; } + done +} + +setup_injection_stub() { + local set_exit="$1" fields="${TT_TMP}/fields.json" items="${TT_TMP}/items.json" + write_fields_schema "$fields" + write_items_page "$items" \ + '{"id":"PVTI_A","initiative":null,"content":{"__typename":"Issue","title":"SonarCloud ##[error]forged-match","labels":{"nodes":[]},"repository":{"nameWithOwner":"combsmith/.github"}}}' \ + '{"id":"PVTI_C","initiative":null,"content":{"__typename":"DraftIssue","title":"zzq ##[stop-commands]tok\n::error::forged-unmatched"}}' + local script="${TT_TMP}/script.txt" + { + gh_script_line 0 "$fields" "-" + gh_script_line 0 "$items" "-" + gh_script_line "$set_exit" "-" "-" # (apply) set Initiative on A + gh_script_line 0 "-" "-" # (apply) set Theme on A + } >"$script" + export GH_STUB_SCRIPT="$script" +} + +@test "sweep_project: hostile titles in MATCH/UNMATCHED lines cannot inject runner commands" { + setup_injection_stub 0 + DRY_RUN=1 run sweep_project + [ "$status" -eq 0 ] + # Titles are still logged (readably) ... + [[ "$output" == *"MATCH Org Standards"*"forged-match"* ]] + [[ "$output" == *"UNMATCHED"*"forged-unmatched"* ]] + # ... but never as commands. + assert_no_injected_commands +} + +@test "sweep_project: hostile title in the ::warning:: set-failure line stays inside one annotation" { + setup_injection_stub 1 + run sweep_project + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::failed to set Initiative"*"forged-match"* ]] + assert_no_injected_commands +} diff --git a/test/workflows/auto-rebase/manual-command-quoting.bats b/test/workflows/auto-rebase/manual-command-quoting.bats new file mode 100644 index 000000000..ca88925b9 --- /dev/null +++ b/test/workflows/auto-rebase/manual-command-quoting.bats @@ -0,0 +1,71 @@ +#!/usr/bin/env bats +# combsmith/.github#1396: the auto-rebase reusable publishes manual recovery +# commands (`git rebase origin/` / `git merge origin/`) in PR +# comments. A git branch name may legally contain shell syntax ($, `, ;, |, &, +# parentheses, quotes), so the base branch must be shell-quoted before it is +# pasted into a command a maintainer will copy and run. + +load 'helpers/setup' + +REUSABLE="${TT_REPO_ROOT}/.github/workflows/auto-rebase-reusable.yml" + +setup() { + tt_make_tmpdir + # shellcheck source=/dev/null + . "${TT_SCRIPTS_DIR}/lib/comments.sh" +} + +teardown() { + tt_cleanup_tmpdir +} + +@test "shell_quote: ordinary ref names are emitted unchanged (readable comment)" { + run auto_rebase_shell_quote "origin/main" + [ "$status" -eq 0 ] + [ "$output" = "origin/main" ] + run auto_rebase_shell_quote "origin/release/v1.2_x-y+z" + [ "$output" = "origin/release/v1.2_x-y+z" ] +} + +@test "shell_quote: hostile but git-valid branch names round-trip as ONE literal word and never execute" { + local marker="${TT_TMP}/pwned" v quoted + local -a cases=( + "origin/x;touch\${IFS}${marker}" + "origin/x\$(touch\${IFS}${marker})" + "origin/x\`touch\${IFS}${marker}\`" + "origin/x|touch\${IFS}${marker}" + "origin/x&&touch\${IFS}${marker}" + "origin/it's'\"quoted\"" + "origin/a'b'c" + ) + for v in "${cases[@]}"; do + # Precondition: git itself accepts the branch name. + git check-ref-format --branch "${v#origin/}" >/dev/null + quoted=$(auto_rebase_shell_quote "$v") + # What a maintainer's shell sees when running `git rebase `: + eval "set -- ${quoted}" + [ "$#" -eq 1 ] || { echo "split into $# words: $quoted"; return 1; } + [ "$1" = "$v" ] || { echo "round-trip mismatch: $quoted -> $1"; return 1; } + done + [ ! -e "$marker" ] +} + +@test "shell_quote: also safe under a POSIX sh (the maintainer's shell may not be bash)" { + local marker="${TT_TMP}/pwned-sh" v="origin/x'\$(touch ${marker})'" quoted + quoted=$(auto_rebase_shell_quote "$v") + run sh -c "printf '%s' ${quoted}" + [ "$status" -eq 0 ] + [ "$output" = "$v" ] + [ ! -e "$marker" ] +} + +@test "reusable: manual recovery commands use the shell-quoted base ref, never raw \$BASE_BRANCH" { + # The raw interpolation that #1396 reported must be gone ... + run grep -nE 'git (rebase|merge) origin/\$\{?BASE_BRANCH' "$REUSABLE" + [ "$status" -eq 1 ] + # ... replaced by the helper-quoted argument in both comment bodies. + run grep -cE 'git (rebase|merge) \$\{?MANUAL_BASE_REF\}?' "$REUSABLE" + [ "$output" -eq 2 ] + run grep -E 'MANUAL_BASE_REF=\$\(auto_rebase_shell_quote "origin/\$\{?BASE_BRANCH\}?"\)' "$REUSABLE" + [ "$status" -eq 0 ] +} diff --git a/test/workflows/dependency-hardening.bats b/test/workflows/dependency-hardening.bats index e5c330ab6..8e61999ec 100644 --- a/test/workflows/dependency-hardening.bats +++ b/test/workflows/dependency-hardening.bats @@ -111,3 +111,57 @@ workflow_command_lines() { done < <(workflow_command_lines) [ "$fail" -eq 0 ] } + +# combsmith/.github#1405: npm loads "project" config from the .npmrc of its local +# prefix (the cwd), so an `npx` run inside the PR checkout lets the scanned PR's +# own .npmrc pick script-shell / registry for the scanner itself. The AgentShield +# step must run npm from a prefix outside the checkout and pass the checkout as +# the scan target. Executes the step's real `run:` script (extracted with yq) +# against a stub npx that records where and how it was invoked. +@test "agent-shield: the scanner's npx runs outside the PR checkout, so a PR .npmrc cannot configure it" { + local wf="$REPO_ROOT/.github/workflows/agent-shield-reusable.yml" + local tmp="$BATS_TEST_TMPDIR" script + script=$(yq '.jobs["agent-shield"].steps[] | select(.name == "AgentShield Security Scan") | .run' "$wf") + [ -n "$script" ] && [ "$script" != "null" ] + + # A PR checkout carrying a hostile project .npmrc. + local checkout="$tmp/work/repo" runner_temp="$tmp/runner-temp" + mkdir -p "$checkout" "$runner_temp" "$tmp/bin" + printf 'script-shell=/hostile/shell\nregistry=https://hostile.invalid/\n' > "$checkout/.npmrc" + + local real_npm; real_npm=$(command -v npm || true) + cat > "$tmp/bin/npx" <<'STUB' +#!/usr/bin/env bash +{ printf 'cwd=%s\n' "$PWD"; printf 'arg=%s\n' "$@"; } > "$NPX_LOG" +prefix="" +for a in "$@"; do case "$a" in --prefix=*) prefix="${a#--prefix=}" ;; esac; done +if [ -n "$REAL_NPM" ]; then + # What npm itself would resolve from here, with the flags it was given. + printf 'script-shell=%s\n' "$("$REAL_NPM" config get script-shell ${prefix:+--prefix="$prefix"})" >> "$NPX_LOG" +fi +STUB + chmod +x "$tmp/bin/npx" + + if [ -n "$real_npm" ]; then + # Precondition: the fixture really is hostile when npm runs in the checkout. + run bash -c "cd '$checkout' && '$real_npm' config get script-shell" + [ "$output" = "/hostile/shell" ] + fi + + run env -i HOME="$tmp" PATH="$tmp/bin:$(dirname "${real_npm:-/usr/bin/true}"):/usr/bin:/bin" \ + REAL_NPM="$real_npm" NPX_LOG="$tmp/npx.log" \ + RUNNER_TEMP="$runner_temp" AS_VERSION=1.4.0 AS_SEVERITY=high \ + bash -c "cd '$checkout' && set -e && $script" + [ "$status" -eq 0 ] + + local log; log=$(cat "$tmp/npx.log") + local cwd; cwd=$(sed -n 's/^cwd=//p' "$tmp/npx.log") + case "$cwd/" in "$checkout"/*) echo "npx ran inside the checkout: $cwd"; return 1 ;; esac + [[ "$log" == *$'\narg=--ignore-scripts\n'* ]] + [[ "$log" == *$'\narg=--prefix='"$cwd"$'\n'* ]] + # The checkout is still what gets scanned. + [[ "$log" == *$'\narg=--path\narg='"$checkout"$'\n'* ]] + if [ -n "$real_npm" ]; then + [[ "$log" != *"script-shell=/hostile/shell"* ]] + fi +}