fix(security): workflow/CI hardening batch (#1384, #1391, #1396, #1405)

Merged
DO
6d ago
+423-13

Summary

Workflow/CI hardening batch for four CodeRabbit security findings. Each one was checked against the code and reproduced locally before fixing. Each fix comes with a regression test that fails on origin/main and passes on this branch. There is one commit per issue.

IssueAreaVerdict
#1384add-to-project classify-initiative.sh log linesConfirmed
#1391scripts/lib/dev-lead-retrigger.sh activity checkConfirmed
#1396auto-rebase-reusable.yml manual recovery commandsConfirmed
#1405agent-shield-reusable.yml scanner invocationConfirmed

Fixes #1384: untrusted titles written raw to runner logs

How it was confirmed: sweep_project printed «${title}» raw at classify-initiative.sh:282, 288, 299, 303 and also at 312 (the Theme warning). The runner parser (actions/runner ActionCommand.TryParse) matches the legacy ##[ prefix with IndexOf, so it fires anywhere in a line. TryParseV2 (::cmd::) only fires at the start of a line, after leading whitespace is trimmed. A title such as x ##[error]… or ##[stop-commands]… in an UNMATCHED/MATCH line was therefore read as a command. The new sweep tests reproduced it: the forged markers showed up verbatim in the output on main.

Fix: added atp_log_safe to the shared .github/scripts/add-to-project/lib.sh:

  • CR and LF become spaces.
  • Every :: and ##[ gets a zero-width space (U+200B) between its characters, so the title stays readable but no longer matches either command syntax. The :: replacement loops, so ::: is handled too.

All five title log sites in the sweep go through it. So does add_draft_item's dry-run line, which is the only other title echo among the sibling scripts. add-issue-or-pr.sh and reconcile-backlog.sh only log GitHub-generated URLs, repo names and config-defined labels.

Regression tests (test/workflows/add-to-project/classify-initiative.bats):

  • Unit tests for atp_log_safe.
  • Two sweep_project tests with hostile titles: dry-run MATCH/UNMATCHED, and the apply-mode ::warning:: set-failure path. They check that no ##[ and no injected :: line reaches the output.

Fixes #1391: a fork PR branch name could fake dev-lead activity

How it was confirmed: at dev-lead-retrigger.sh:39-47, the pulls?state=open filter only checked .head.ref | startswith("dev-lead/issue-<n>-"). That list includes fork PRs, and fork authors pick their own branch names. A fixture fork PR on dev-lead/issue-7-… made dl_dev_lead_active return "active" on main.

Fix: a PR now counts only when all of these hold:

  • the branch prefix matches (unchanged);
  • .head.repo != null and .head.repo.full_name == .base.repo.full_name, i.e. the PR comes from this repo, not a fork or a deleted head repo;
  • .user.login is the dev-lead runtime identity.

That identity is don-petry, as documented in standards/persona-standards.md §5.1. All 154 dev-lead/ PRs in this repo were authored by that login. It can be overridden with DEV_LEAD_LOGINS (comma-separated). The logins are JSON-encoded with jq -R, not spliced into the filter as raw strings. Fail-closed behaviour on API errors is unchanged. I also ran a read-only smoke test of the new --jq filter against real gh/gojq on this repo: open PR #1383 (dev-lead/issue-1361-…) is still detected as active.

Regression tests (new test/scripts/compliance-retrigger/dev-lead-active.bats, 10 tests). The gh stub applies the real --jq filter to fixtures. Cases covered:

  • fork PR
  • fork PR authored by the agent login
  • same-repo PR by a non-agent author
  • head.repo: null
  • agent PR among fork decoys
  • exact issue-number prefix
  • DEV_LEAD_LOGINS override and injection attempt
  • fail-closed behaviour

The suite is already run by compliance-retrigger-tests.yml, which runs the test/scripts/compliance-retrigger/ directory on scripts/lib/** changes.

Fixes #1396: base branch pasted unescaped into published shell commands

How it was confirmed: auto-rebase-reusable.yml:218 (git rebase origin/$BASE_BRANCH) and :258 (git merge origin/$BASE_BRANCH) interpolate .base.ref raw into the copy-paste snippets in PR comments. git check-ref-format --branch accepts names containing ;, $(…), backticks, |, && and quotes. The tests below assert that precondition for each case.

Fix: added auto_rebase_shell_quote to .github/scripts/auto-rebase/lib/comments.sh, which the reusable already sources from its own commit.

  • Refs made only of [A-Za-z0-9_./+-] are printed unchanged, so origin/main stays readable.
  • Anything else is single-quoted, with ' written as '\''. That form is valid in bash, zsh and sh, unlike %q.

The reusable computes MANUAL_BASE_REF once and uses it in both snippets. The README function table is updated.

Regression tests (new test/workflows/auto-rebase/manual-command-quoting.bats):

  • Round-trip and no-execution checks for hostile but git-valid names, under bash eval and sh.
  • A grep guard confirming the reusable no longer pastes origin/$BASE_BRANCH into a git command, and that both snippets use the quoted ref.

Fixes #1405: the PR's .npmrc could configure the AgentShield scanner

How it was confirmed: agent-shield-reusable.yml:60 runs npx --ignore-scripts ecc-agentshield@… scan --path . with the PR checkout as cwd. npm loads project config from <localPrefix>/.npmrc, and localPrefix is cwd-based. I reproduced this locally with npm 11.11.0. In a directory whose .npmrc sets script-shell=/usr/bin/true, npx --ignore-scripts -c 'exit 3' exits 0, so the command is silently skipped. Run from a clean prefix, the same command exits 3. npm config get registry similarly showed the hostile registry from inside the directory.

Fix: the scan step now:

  • creates $RUNNER_TEMP/agentshield-npm containing a minimal package.json, which pins localPrefix;
  • cds there;
  • runs npx --prefix=<that dir> --ignore-scripts ecc-agentshield@${AS_VERSION} scan --path "<absolute checkout path>".

The version pin and --ignore-scripts are unchanged. I checked in the ecc-agentshield v1.4.0 source that scan --path resolves its argument, so an absolute target works. The change is limited to the scan step, so it does not conflict with #1378, which edits the inputs and the cross-reference steps of this file.

Regression test (test/workflows/dependency-hardening.bats, which already runs on every .github/workflows/** change): it extracts the step's real run: script with yq and executes it inside a fake checkout that has a hostile .npmrc, with a stub npx that records its cwd and args. It asserts:

  • npx runs outside the checkout;
  • --prefix equals that cwd;
  • --path is the checkout;
  • --ignore-scripts is kept;
  • when npm is available, npm config get script-shell with the recorded flags does not resolve the hostile value. A precondition first confirms the fixture is hostile when npm runs inside the checkout.

Tests run (locally, bats 1.13 with GNU bash 5)

  • bats test/workflows/add-to-project/: 88/88 pass
  • bats test/scripts/compliance-retrigger/: 22/22 pass
  • bats test/workflows/auto-rebase/: 52/52 pass
  • bats test/workflows/dependency-hardening.bats: 5/5 pass
  • bats test/scripts/compliance-audit/ (it also sources dev-lead-retrigger.sh): 298/298 pass
  • Red check: all 16 new tests run against an origin/main tree fail as expected.
  • shellcheck -x on the changed scripts: clean apart from one existing SC2016 info in lib.sh (delete_project_item's GraphQL string, not touched here; CI runs at -S warning).
  • actionlint on both changed workflows: only an existing github.job_workflow_sha false positive from my local actionlint version, on a line not touched here.
  • gitleaks detect --no-git: no leaks.

Release notes / follow-ups

  • #1396 and #1405 change reusable workflows. Callers pick them up only when the channels are promoted: auto-rebase/stable and agent-shield/stable, after the release cut and canary rings. No tags were moved in this PR.
  • #1384 (add-to-project-reconcile.yml) and #1391 (compliance-retrigger.yml / compliance audit) run from this repo's main and take effect on merge.
  • #1391 adds an optional DEV_LEAD_LOGINS env (default don-petry). If the dev-lead persona's runtime.identity.account ever changes, set this or update the default.
  • Not changed here: auto-rebase-reusable.yml also uses BASE_BRANCH as an associative-array key and in API paths. Neither is a copy-paste shell command, so both are outside #1396.

Needs a human decision

  • None blocking. Please double-check one behaviour change in #1391: a same-repo dev-lead/issue-<n>-* PR opened by anyone other than don-petry no longer suppresses the retrigger. That matches every existing dev-lead PR in this repo.

🤖 Generated with Claude Code

Problem

See the linked issue.

Risk

Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.

Test plan

Tests added/updated: test/scripts/compliance-retrigger/dev-lead-active.bats,test/workflows/add-to-project/classify-initiative.bats test/workflows/auto-rebase/manual-command-quoting.bats,test/workflows/dependency-hardening.bats. Verification: bash scripts/dev-lead-lint.sh (shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.

Rollback

Revert this PR. No non-revertible side effects (no tags, migrations, or external state).

Monitoring

Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.