[MAJOR] §8: cross-repo-dispatch.yml passes untrusted client_payload.host to dispatch-apply.sh without validation

[mvillmow]

Finding

Severity: MAJOR

Evidence: .github/workflows/cross-repo-dispatch.yml:35-36

Principle: SOLID

cross-repo-dispatch.yml reads github.event.client_payload.host and passes it directly as $HOST to dispatch-apply.sh. Any entity that can send a repository_dispatch event of type image-pushed to ProjectProteus can set host to an arbitrary value, potentially causing Myrmidons to apply against an unintended host. The payload field is untrusted and unvalidated.

---

Part of #81

[mvillmow]

Implementation Plan

I have everything I need. Outputting the plan now.

Implementation Plan

Objective

Eliminate the unvalidated forwarding of client_payload.host from .github/workflows/cross-repo-dispatch.yml:35-36 into scripts/dispatch-apply.sh. Today, any actor authorized to send an image-pushed dispatch can supply an arbitrary host string (including hosts that exist in the Myrmidons inventory but were not the intended target, or shell-metacharacter / control-character payloads). Introduce a strict, schema-defined hostname format plus a finite allowlist enforced in both the workflow step and dispatch-apply.sh (defence in depth), fail closed on any violation, and back the change with regression tests run in CI.

Approach

Decision 1 — Validate in two layers, not one. The workflow Require client_payload.host step (cross-repo-dispatch.yml:31-43) is the primary trust boundary (it sees the raw untrusted payload first), but scripts/dispatch-apply.sh is also invoked from the justfile (justfile:51-52 — just dispatch-apply HOST) where the same HOST value could come from a local operator typo. Per tests/dispatch-apply.test.sh Case 3 the script already fails closed on empty HOST; I'll extend the same pattern to also fail closed on malformed or non-allowlisted HOST. Evidence: grep -n "dispatch-apply" justfile → two call sites (line 31, line 52), so script-side validation is load-bearing.

Decision 2 — Allowlist source of truth lives in the repo, not the workflow. Following the team-knowledge pattern (security-pep508-parser-replaces-regex → "canonical parser over regex" and ci-cd-pip-audit-ignore-vuln-allowlist-with-tracking-issue → "explicit allowlist with tracking issue"), the allowlist goes in a single plain-text file configs/allowed-hosts.txt (one host per line, # comments allowed). Both the workflow step and dispatch-apply.sh read it. Evidence: docs/dispatch-contract.md already documents the contract; adding the allowlist file (referenced from that doc) keeps the audit trail visible. No new dependency is required — pure bash grep -Fx lookup.

Decision 3 — Format check is a strict regex anchored end-to-end. Allowed character set: [A-Za-z0-9] plus internal - and ., length ≤ 253, no leading/trailing dot or dash. This is RFC 1123-compatible and excludes every shell metacharacter, control character, and whitespace. Pattern: ^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$. Format check runs before allowlist check so logs don't echo untrusted control characters. Lesson from security-pep508-parser-replaces-regex "what failed": regex alone is not enough → that's why we combine it with the allowlist.

Decision 4 — Fail-closed messages cite issue #97 and docs/dispatch-contract.md, mirroring the existing #84 citation style in cross-repo-dispatch.yml:38 and dispatch-apply.sh:14. This is the existing repo convention.

Decision 5 — Regression test extends the existing harness. tests/dispatch-apply.test.sh already exists, is plain bash, and is wired into .github/workflows/_required.yml:159 (dispatch-contract-test job). Add cases there rather than introducing a new test framework. Per CLAUDE.md "Audit Remediation Tracking" step 4: a regression test is required.

Decision 6 — Update docs/dispatch-contract.md and docs/audit-2026-04-28/remediation-plan.md:23 to reflect the new validation and tick the box, per CLAUDE.md step 5.

Files to Create

configs/allowed-hosts.txt

Plain-text host allowlist, one host per line, # comments permitted. Initial contents seeded from the only host currently referenced in the repo (hermes, from justfile:32 and docs/dispatch-contract.md example):

# Allowed Myrmidons target hosts for agamemnon-apply dispatch.
# Audited entries only. See docs/dispatch-contract.md and issue #97.
# Format: one RFC 1123 hostname per line. Lines starting with '#' are comments.
hermes

scripts/validate-host.sh

Shared bash helper, sourced by dispatch-apply.sh and invokable standalone from the workflow step. Single function validate_host that enforces format then allowlist:

#!/usr/bin/env bash
# validate-host.sh — Validate a Myrmidons dispatch host against
# RFC 1123 format AND configs/allowed-hosts.txt. Fails closed.
# See docs/dispatch-contract.md and issue #97.

set -euo pipefail

validate_host() {
    local host="${1:-}"
    local allowlist="${2:-configs/allowed-hosts.txt}"
    local pattern='^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$'

    if [[ -z "$host" ]]; then
        echo "Error: host is empty. See docs/dispatch-contract.md (#97)." >&2
        return 1
    fi
    if [[ ! "$host" =~ $pattern ]]; then
        # Do NOT echo the raw host — it may contain control characters.
        echo "Error: host failed RFC 1123 format check. See docs/dispatch-contract.md (#97)." >&2
        return 1
    fi
    if [[ ! -r "$allowlist" ]]; then
        echo "Error: allowlist file not readable: ${allowlist} (#97)." >&2
        return 1
    fi
    # grep -Fx: fixed-string, full-line match; strip comments/blanks first.
    if ! grep -vE '^\s*(#|$)' "$allowlist" | grep -Fxq -- "$host"; then
        echo "Error: host '${host}' is not in allowlist ${allowlist}. See docs/dispatch-contract.md (#97)." >&2
        return 1
    fi
    return 0
}

# Allow direct invocation: `scripts/validate-host.sh <host> [allowlist]`
if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then
    validate_host "$@"
fi

Files to Modify

.github/workflows/cross-repo-dispatch.yml

Replace the single Require client_payload.host step (lines 31-43) with a step that also validates format and allowlist by delegating to scripts/validate-host.sh. The checkout step already runs above it (line 29), so the script is available.

      - name: Validate client_payload.host
        env:
          HOST: ${{ github.event.client_payload.host }}
        run: |
          set -euo pipefail
          if [[ -z "${HOST}" ]]; then
            echo "::error title=dispatch-contract::client_payload.host is required but was empty or absent. See docs/dispatch-contract.md and issues #84, #97."
            exit 1
          fi
          if ! bash scripts/validate-host.sh "${HOST}" configs/allowed-hosts.txt; then
            echo "::error title=dispatch-contract::client_payload.host failed validation. See docs/dispatch-contract.md and issue #97."
            exit 1
          fi
          echo "host=${HOST}" >> "$GITHUB_ENV"

scripts/dispatch-apply.sh

Insert a validation call between the HOST empty-check (line 13) and the GITHUB_TOKEN check (line 18). Resolve the allowlist path relative to the script so it works from any cwd.

HOST="${1:-${HOST:-}}"
MYRMIDONS_REPO="${MYRMIDONS_REPO:-HomericIntelligence/Myrmidons}"
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
ALLOWLIST="${ALLOWED_HOSTS_FILE:-${SCRIPT_DIR}/../configs/allowed-hosts.txt}"

if [[ -z "${HOST}" ]]; then
    echo "Error: host is required (pass as \$1 or set HOST env var). See docs/dispatch-contract.md (#84, #97)." >&2
    exit 1
fi

# shellcheck source=scripts/validate-host.sh
source "${SCRIPT_DIR}/validate-host.sh"
if ! validate_host "${HOST}" "${ALLOWLIST}"; then
    echo "Error: host validation failed for '${HOST}'. See docs/dispatch-contract.md (#97)." >&2
    exit 1
fi

tests/dispatch-apply.test.sh

Append four cases after the existing Case 3 (current file ends at line 44 — echo "OK: all 3 cases passed"). Tests must point the script at a fixture allowlist so they don't depend on production contents.

# Case 4: host in allowlist — exits 0.
FIXTURE_ALLOW="$(mktemp)"; echo "hermes" >"$FIXTURE_ALLOW"
out=$(env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" hermes 2>&1) \
  || { echo "FAIL case4: expected exit 0 for allowlisted host"; echo "$out"; exit 1; }
echo "$out" | grep -q "for host: hermes" \
  || { echo "FAIL case4: expected success line"; echo "$out"; exit 1; }

# Case 5: host NOT in allowlist — must FAIL CLOSED.
if env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" attacker-host 2>err.txt; then
  echo "FAIL case5: expected nonzero exit for non-allowlisted host"; cat err.txt; exit 1
fi
grep -q "not in allowlist" err.txt \
  || { echo "FAIL case5: expected 'not in allowlist' in stderr"; cat err.txt; exit 1; }

# Case 6: host with shell metachars — must FAIL on format check, before allowlist.
if env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" 'hermes;rm -rf /' 2>err.txt; then
  echo "FAIL case6: expected nonzero exit for metacharacter host"; cat err.txt; exit 1
fi
grep -q "RFC 1123 format check" err.txt \
  || { echo "FAIL case6: expected format-check failure"; cat err.txt; exit 1; }

# Case 7: host with newline — must FAIL on format check (no raw echo of payload).
if env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" $'hermes\nevil' 2>err.txt; then
  echo "FAIL case7: expected nonzero exit for newline-containing host"; cat err.txt; exit 1
fi
grep -q "RFC 1123 format check" err.txt \
  || { echo "FAIL case7: expected format-check failure"; cat err.txt; exit 1; }

rm -f "$FIXTURE_ALLOW"
echo "OK: all 7 cases passed"

Also delete the old final line echo "OK: all 3 cases passed" to avoid double-printing.

docs/dispatch-contract.md

Update the inbound table row for host (currently states only required/non-empty) and the "Fail-Closed Behavior" section. Insert after the existing inbound table, before the "Outbound" section:

### Host validation (issue #97)

`client_payload.host` is validated in two layers:

1. **Format**: must match RFC 1123 hostname grammar
   `^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$`. Any control
   character, whitespace, or shell metacharacter fails the check.
2. **Allowlist**: must appear as an exact-match line in
   `configs/allowed-hosts.txt`. To add a host, open a PR amending that
   file with a justification linked to a tracking issue.

Both checks run in `.github/workflows/cross-repo-dispatch.yml` (primary
trust boundary) and again in `scripts/dispatch-apply.sh` (defence in
depth, also covers local `just dispatch-apply` invocations). Both
layers delegate to `scripts/validate-host.sh::validate_host`.

docs/audit-2026-04-28/remediation-plan.md

Tick the issue #97 checkbox at line 23 and update the description:

- [x] #97 Payload `host` validation — `scripts/validate-host.sh` (format + allowlist) wired into workflow + `dispatch-apply.sh` — PR: <fill in>

Implementation Order

1. Create configs/allowed-hosts.txt with the initial hermes entry.
2. Create scripts/validate-host.sh and chmod +x it; run shellcheck scripts/validate-host.sh to confirm clean (the lint-scripts CI job at ci.yml:25-37 scans ./scripts).
3. Modify scripts/dispatch-apply.sh to source validate-host.sh and call validate_host after the empty-check; re-run shellcheck.
4. Extend tests/dispatch-apply.test.sh with Cases 4–7 and bump the final OK line to 7 cases passed.
5. Run bash tests/dispatch-apply.test.sh locally; all 7 must pass.
6. Modify .github/workflows/cross-repo-dispatch.yml to call scripts/validate-host.sh in the validation step.
7. Update docs/dispatch-contract.md (new "Host validation" subsection) and tick docs/audit-2026-04-28/remediation-plan.md:23.
8. Open PR with body containing Refs #81 and Closes #97.

Verification

Acceptance criterion — "host is validated, not passed through untrusted":

bash tests/dispatch-apply.test.sh              # Cases 4–7 prove format + allowlist enforcement

Acceptance criterion — "shell-metacharacter / control-char payload is rejected at format layer":

env ALLOWED_HOSTS_FILE=<(echo hermes) bash scripts/dispatch-apply.sh 'hermes;id' 2>&1 \
  | grep -q "RFC 1123 format check" && echo PASS

Acceptance criterion — "non-allowlisted but format-valid host is rejected":

env ALLOWED_HOSTS_FILE=<(echo hermes) bash scripts/dispatch-apply.sh attacker-host 2>&1 \
  | grep -q "not in allowlist" && echo PASS

Acceptance criterion — "no shellcheck regression in scripts/":

shellcheck scripts/validate-host.sh scripts/dispatch-apply.sh

Acceptance criterion — "CI job dispatch-contract-test still wired":

grep -n "tests/dispatch-apply.test.sh" .github/workflows/_required.yml   # expect line 159 hit

Acceptance criterion — "workflow YAML stays valid":

pixi run python -c "import yaml; yaml.safe_load(open('.github/workflows/cross-repo-dispatch.yml'))"

Skills Used

• Team knowledge: security-pep508-parser-replaces-regex — informed Decision 3 (regex alone is insufficient; combine with canonical/explicit check) and the choice of an exact-match allowlist over a permissive pattern.
• Team knowledge: ci-cd-pip-audit-ignore-vuln-allowlist-with-tracking-issue — informed Decision 2 (allowlist file with tracking-issue references in comments and docs/dispatch-contract.md).
• Team knowledge: precommit-schema-validation — informed the two-layer design (format + allowlist as separate, fail-closed checks rather than a single regex).
• Team knowledge: bandit-replace-pygrep-security-hook — reinforced the fail-fast-on-untrusted-input pattern used by both the workflow step and dispatch-apply.sh.
• Planning skill: read repo state directly via Bash to ground every cited line number against the current code (no speculative line refs).

Changes from Review

N/A — initial plan

---

Final review verdict (from strict review loop)

Now I have the full picture of the current codebase. Let me perform a thorough review of the plan.

---

Review: Implementation Plan for Issue #97

Issue Requirements Analysis

The issue states:

• Finding: cross-repo-dispatch.yml reads github.event.client_payload.host and passes it directly as $HOST to dispatch-apply.sh without validation
• Risk: Any authorized repository_dispatch sender can set host to an arbitrary value
• Principle: SOLID

The core acceptance criterion is: validate client_payload.host before it's used, reject untrusted/malformed/non-allowlisted values, fail closed.

---

Dimension 1: Requirements Alignment

The plan fully addresses the issue's stated requirement: the unvalidated forwarding of client_payload.host. Both the workflow-level (primary trust boundary) and script-level (defense-in-depth) validations are planned. The CLAUDE.md audit requirement (#97 checkbox in Wave 2) is addressed.

No silently dropped criteria are evident. The plan goes beyond the issue's stated minimum (format check alone would satisfy it; the plan adds an allowlist). This is scope expansion but arguably justified by defense-in-depth (see P2/YAGNI concern below).

Alignment: Solid.

Dimension 2: Plan Completeness

Design, implementation, tests, and verification are all present. Rollback is not explicitly addressed — the plan creates new files (configs/allowed-hosts.txt, scripts/validate-host.sh) and modifies existing ones, all of which are reversible via git revert. The repo's rollback policy (line 39 of remediation-plan.md) covers this implicitly, but the plan doesn't cite it. Minor gap.

The test strategy is concrete: four new test cases (4–7) extending the existing bash test harness.

Completeness: Good, minor gap on rollback mention.

Dimension 3: Concreteness

Every step names specific files and line numbers. Examples:

• .github/workflows/cross-repo-dispatch.yml:31-43 — exact lines being replaced
• scripts/dispatch-apply.sh:13-18 — exact insertion point
• tests/dispatch-apply.test.sh — line 44 noted as the extension point
• docs/audit-2026-04-28/remediation-plan.md:23 — exact line for checkbox

The plan includes actual code for every modified file. Line numbers are grounded against the actual codebase (verified above).

Concreteness: Excellent.

Dimension 4: Risk Surface

Finding (Major) — Security risk in dispatch-apply.sh logging at line 31:

The plan modifies dispatch-apply.sh to validate HOST before use, but does not address line 31 of the current script:

--data "{\"event_type\":\"agamemnon-apply\",\"client_payload\":{\"host\":\"${HOST}\"}}"

After validation, HOST is a format-validated, allowlisted string — this is safe. However, the plan's "validate before use" approach works only if validation is genuinely in the right place. The plan inserts validation between the empty-check (line 13) and the GITHUB_TOKEN check (line 18). If shellcheck catches sourcing issues, this is fine. Risk is managed.

Finding (Minor) — validate-host.sh logs host in the allowlist-not-found error:

echo "Error: host '${host}' is not in allowlist ${allowlist}. See docs/dispatch-contract.md (#97)." >&2

The plan correctly avoids logging the raw host in the format-check error ("Do NOT echo the raw host — it may contain control characters"), but does echo ${host} in the allowlist-error path (Case 5 scenario). At the point of the allowlist check, the host has already passed the format regex — it's RFC 1123-compliant, meaning no control characters or shell metacharacters can be present. So this is safe: a format-validated hostname cannot contain $(), backticks, newlines, etc. The code is correct, but the inconsistency could confuse a future maintainer into thinking it's a bug. Minor.

Finding (Major) — $HOST injection via GITHUB_ENV into run: step:

The workflow currently uses echo "host=${HOST}" >> "$GITHUB_ENV" (line 39 of the current file), then accesses it via ${{ env.host }} at line 45 of the current Dispatch apply to Myrmidons step. The plan replaces the validation step but the Dispatch apply to Myrmidons step that calls ./scripts/dispatch-apply.sh "${{ env.host }}" is not modified. After the plan's changes, ${{ env.host }} still comes from the GITHUB_ENV set in the new step — which is now fully validated. However, the plan should explicitly state that the downstream step is left unchanged and explain why this is safe (because script-side validation is now a second layer). The plan does address this implicitly but doesn't call it out in the "Files to Modify" section for the workflow — it only modifies the validation step, not the dispatch step. This is acceptable but a slight documentation gap.

Allowlist path resolution — justified concern (from plan's own learnings):

The plan acknowledges that configs/allowed-hosts.txt is passed as a relative path in the workflow. The checkout step at line 29 runs without working-directory:, so the CWD is the repo root. This is correct today but fragile. The plan doesn't add a mitigation (e.g., using $GITHUB_WORKSPACE/configs/allowed-hosts.txt). Minor risk, acknowledged in learnings but no mitigation planned.

Risk surface: Two minor findings; overall acceptable.

Dimension 5: Verification Plan

The plan includes six concrete verification commands:

1. bash tests/dispatch-apply.test.sh — runs cases 4–7
2. Direct invocation of dispatch-apply.sh with shell metacharacter host
3. Direct invocation with non-allowlisted host
4. shellcheck scripts/validate-host.sh scripts/dispatch-apply.sh
5. grep -n "tests/dispatch-apply.test.sh" .github/workflows/_required.yml
6. YAML validity check

These are runnable commands, not vague statements. Each acceptance criterion from the issue has a corresponding check.

Verification: Excellent.

Dimension 6: Stage Handoff

The plan is extremely detailed: full code for every new file and modification is provided. An implementer could execute this without re-deriving any design decisions. The implementation order is sequential and logical.

One open question deferred: the plan's own learnings note the regex edge case for single-character hostnames. The pattern ^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$ requires at least 2 characters (the outer group has exactly one char, then optionally 0–251 more chars plus a final char). A single-character hostname like a would fail because the optional group ([A-Za-z0-9.-]{0,251}[A-Za-z0-9])? when absent leaves only one char — wait, the outer structure is ^[A-Za-z0-9] (one char) followed by an optional group ([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?. So single-char hostnames do match (the optional group is absent, leaving just the first char). The plan's own learning is incorrect about this being a problem — single-char hostnames do pass. However, hermes (7 chars) passes trivially.

Actually the learning says "rejects single-character hostnames" — let me re-analyze: ^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$. For input a: matches [A-Za-z0-9] (the a), optional group absent, end anchor. So a DOES match. The learning is wrong, but the plan code is correct. No action needed.

Handoff: Excellent.

---

Software Engineering Principles

P1 — KISS:

The plan introduces three new artifacts: configs/allowed-hosts.txt, scripts/validate-host.sh, and modifications to four existing files. The shared-helper approach (validate-host.sh sourced by the workflow and the script) is an appropriate level of DRY. The allowlist file is a simple text file. No over-engineering detected.

Finding: Clean.

P2 — YAGNI:

Finding (Major): The issue requires validating that host is not arbitrary. A format check alone (RFC 1123 regex) would close the security gap: it would reject shell metacharacters, control characters, newlines, and most injection vectors. The plan adds an allowlist on top of this.

The allowlist is not a stated requirement in the issue — the issue says "arbitrary value, potentially causing Myrmidons to apply against an unintended host." A format check doesn't prevent an attacker from sending a valid-RFC-1123 hostname that isn't in the Myrmidons inventory (e.g., attacker-node). The allowlist does address this scenario, which is implicitly part of "causing Myrmidons to apply against an unintended host."

Re-reading the issue: "any entity...can set host to an arbitrary value, potentially causing Myrmidons to apply against an unintended host." This phrasing suggests the concern is not just injection but also misdirection to a valid-looking but wrong host. The allowlist addresses exactly this. This is within scope — the issue calls out misdirection as a threat. YAGNI is not violated.

Finding: Clean.

P3 — TDD:

The plan defines tests (Cases 4–7) that cover the behavior contracts:

• Case 4: allowlisted host succeeds
• Case 5: non-allowlisted host fails
• Case 6: shell metachar fails at format layer
• Case 7: newline fails at format layer

Tests are planned alongside implementation (step 4 in the implementation order comes before step 6, the workflow modification). This is test-first in spirit.

Minor concern: The test harness uses ALLOWED_HOSTS_FILE env var override to inject fixture allowlists, but the plan's current test code for Case 4 checks grep -q "for host: hermes" — this would trigger the actual curl call (shimmed to 204). This is correct given the shim is set up at the top of the test file. The test would still hit validate_host before reaching curl. ✓

Finding: Clean.

P4 — DRY:

The plan deliberately DRYs validation into validate-host.sh rather than duplicating the logic in the workflow step and dispatch-apply.sh. This is appropriate given both consumers need identical behavior.

Finding: Clean.

P5 — SOLID:

• SRP: validate-host.sh has one job: validate a hostname. dispatch-apply.sh handles dispatching. Separation is maintained.
• DIP: The plan has dispatch-apply.sh sourcing validate-host.sh directly — a concrete file path dependency. This is acceptable for bash; no better abstraction exists in shell.

Finding: Clean.

P6 — Modularity:

validate-host.sh is designed as an independently invokable module (the BASH_SOURCE[0] guard), and as a sourceable library. The allowlist path is parameterizable via the second argument. This is well-designed.

Finding: Clean.

P7 — POLA:

The validate_host function: empty host → error, bad format → error (no raw echo), not in allowlist → error with host echoed. The inconsistency between cases (raw host printed in allowlist error but not format error) is noted above. The rationale is sound (format-validated = safe to log), but a reader encountering the code for the first time might wonder why the inconsistency exists. No comment explaining this is planned.

Minor finding: The plan should note this intent gap — either log consistently or add a comment explaining why the allowlist error can safely log the host value.

---

Critical Issue: Test Case 4 Fragility

Test Case 4 in the plan:

out=$(env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" hermes 2>&1)

This will call the modified dispatch-apply.sh, which sources validate-host.sh and calls validate_host. After validation passes, the script proceeds to the GITHUB_TOKEN check. But GITHUB_TOKEN is already set in the test environment (line 20 of the test file: export GITHUB_TOKEN="fake-token-for-test"). The script then calls the curl shim, which returns 204. So the test would pass the full flow. This is correct behavior for an integration test.

However, there's a subtle issue: the plan shows Case 4 checking:

echo "$out" | grep -q "for host: hermes"

The dispatch-apply.sh line 23 currently echoes: Dispatching agamemnon-apply to ${MYRMIDONS_REPO} for host: ${HOST}. After the plan's modifications, this line is unchanged. The grep will match. ✓

---

Critical Issue: Workflow Step Logic

The plan's new workflow step:

      - name: Validate client_payload.host
        env:
          HOST: ${{ github.event.client_payload.host }}
        run: |
          set -euo pipefail
          if [[ -z "${HOST}" ]]; then
            ...
            exit 1
          fi
          if ! bash scripts/validate-host.sh "${HOST}" configs/allowed-hosts.txt; then
            ...
            exit 1
          fi
          echo "host=${HOST}" >> "$GITHUB_ENV"

The plan calls bash scripts/validate-host.sh "${HOST}" configs/allowed-hosts.txt — this invokes validate-host.sh as a standalone script (using the BASH_SOURCE[0] == $0 guard), which calls validate_host "$@". This works correctly.

However, there's a subtle issue: the format check pattern uses [[ ! "$host" =~ $pattern ]]. In bash, the =~ operator does NOT require the pattern to be quoted (and in fact should not be when using ERE features), but the pattern is stored in a variable. This is correct bash practice. ✓

---

Summary of Findings

Finding	Severity	Dimension
Allowlist path in workflow is a relative path without mitigation (acknowledged in learnings)	Minor	Risk Surface
Inconsistency in whether raw host is logged in error paths (no comment explaining rationale)	Minor	P7/POLA
Plan doesn't explicitly state the Dispatch apply to Myrmidons step is intentionally unchanged	Minor	Stage Handoff

No critical or major findings. Three minor findings. The plan is fundamentally sound, well-grounded in the actual codebase, addresses the security issue comprehensively, includes proper tests, and provides runnable verification.

By the grading rubric:

• Zero critical findings ✓
• Zero major findings ✓
• Three minor findings (under the ≤2 threshold for A, within B range)

Grade: B — Solid plan with three minor gaps, none blocking.

Grade: B
Verdict: GO

---

Generated by Claude Code Planner (strict review loop)

[mvillmow]

🔍 Plan Review

Now I have the full picture of the current codebase. Let me perform a thorough review of the plan.

---

Review: Implementation Plan for Issue #97

Issue Requirements Analysis

The issue states:

• Finding: cross-repo-dispatch.yml reads github.event.client_payload.host and passes it directly as $HOST to dispatch-apply.sh without validation
• Risk: Any authorized repository_dispatch sender can set host to an arbitrary value
• Principle: SOLID

The core acceptance criterion is: validate client_payload.host before it's used, reject untrusted/malformed/non-allowlisted values, fail closed.

---

Dimension 1: Requirements Alignment

The plan fully addresses the issue's stated requirement: the unvalidated forwarding of client_payload.host. Both the workflow-level (primary trust boundary) and script-level (defense-in-depth) validations are planned. The CLAUDE.md audit requirement (#97 checkbox in Wave 2) is addressed.

No silently dropped criteria are evident. The plan goes beyond the issue's stated minimum (format check alone would satisfy it; the plan adds an allowlist). This is scope expansion but arguably justified by defense-in-depth (see P2/YAGNI concern below).

Alignment: Solid.

Dimension 2: Plan Completeness

Design, implementation, tests, and verification are all present. Rollback is not explicitly addressed — the plan creates new files (configs/allowed-hosts.txt, scripts/validate-host.sh) and modifies existing ones, all of which are reversible via git revert. The repo's rollback policy (line 39 of remediation-plan.md) covers this implicitly, but the plan doesn't cite it. Minor gap.

The test strategy is concrete: four new test cases (4–7) extending the existing bash test harness.

Completeness: Good, minor gap on rollback mention.

Dimension 3: Concreteness

Every step names specific files and line numbers. Examples:

• .github/workflows/cross-repo-dispatch.yml:31-43 — exact lines being replaced
• scripts/dispatch-apply.sh:13-18 — exact insertion point
• tests/dispatch-apply.test.sh — line 44 noted as the extension point
• docs/audit-2026-04-28/remediation-plan.md:23 — exact line for checkbox

The plan includes actual code for every modified file. Line numbers are grounded against the actual codebase (verified above).

Concreteness: Excellent.

Dimension 4: Risk Surface

Finding (Major) — Security risk in dispatch-apply.sh logging at line 31:

The plan modifies dispatch-apply.sh to validate HOST before use, but does not address line 31 of the current script:

--data "{\"event_type\":\"agamemnon-apply\",\"client_payload\":{\"host\":\"${HOST}\"}}"

After validation, HOST is a format-validated, allowlisted string — this is safe. However, the plan's "validate before use" approach works only if validation is genuinely in the right place. The plan inserts validation between the empty-check (line 13) and the GITHUB_TOKEN check (line 18). If shellcheck catches sourcing issues, this is fine. Risk is managed.

Finding (Minor) — validate-host.sh logs host in the allowlist-not-found error:

echo "Error: host '${host}' is not in allowlist ${allowlist}. See docs/dispatch-contract.md (#97)." >&2

The plan correctly avoids logging the raw host in the format-check error ("Do NOT echo the raw host — it may contain control characters"), but does echo ${host} in the allowlist-error path (Case 5 scenario). At the point of the allowlist check, the host has already passed the format regex — it's RFC 1123-compliant, meaning no control characters or shell metacharacters can be present. So this is safe: a format-validated hostname cannot contain $(), backticks, newlines, etc. The code is correct, but the inconsistency could confuse a future maintainer into thinking it's a bug. Minor.

Finding (Major) — $HOST injection via GITHUB_ENV into run: step:

The workflow currently uses echo "host=${HOST}" >> "$GITHUB_ENV" (line 39 of the current file), then accesses it via ${{ env.host }} at line 45 of the current Dispatch apply to Myrmidons step. The plan replaces the validation step but the Dispatch apply to Myrmidons step that calls ./scripts/dispatch-apply.sh "${{ env.host }}" is not modified. After the plan's changes, ${{ env.host }} still comes from the GITHUB_ENV set in the new step — which is now fully validated. However, the plan should explicitly state that the downstream step is left unchanged and explain why this is safe (because script-side validation is now a second layer). The plan does address this implicitly but doesn't call it out in the "Files to Modify" section for the workflow — it only modifies the validation step, not the dispatch step. This is acceptable but a slight documentation gap.

Allowlist path resolution — justified concern (from plan's own learnings):

The plan acknowledges that configs/allowed-hosts.txt is passed as a relative path in the workflow. The checkout step at line 29 runs without working-directory:, so the CWD is the repo root. This is correct today but fragile. The plan doesn't add a mitigation (e.g., using $GITHUB_WORKSPACE/configs/allowed-hosts.txt). Minor risk, acknowledged in learnings but no mitigation planned.

Risk surface: Two minor findings; overall acceptable.

Dimension 5: Verification Plan

The plan includes six concrete verification commands:

1. bash tests/dispatch-apply.test.sh — runs cases 4–7
2. Direct invocation of dispatch-apply.sh with shell metacharacter host
3. Direct invocation with non-allowlisted host
4. shellcheck scripts/validate-host.sh scripts/dispatch-apply.sh
5. grep -n "tests/dispatch-apply.test.sh" .github/workflows/_required.yml
6. YAML validity check

These are runnable commands, not vague statements. Each acceptance criterion from the issue has a corresponding check.

Verification: Excellent.

Dimension 6: Stage Handoff

The plan is extremely detailed: full code for every new file and modification is provided. An implementer could execute this without re-deriving any design decisions. The implementation order is sequential and logical.

One open question deferred: the plan's own learnings note the regex edge case for single-character hostnames. The pattern ^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$ requires at least 2 characters (the outer group has exactly one char, then optionally 0–251 more chars plus a final char). A single-character hostname like a would fail because the optional group ([A-Za-z0-9.-]{0,251}[A-Za-z0-9])? when absent leaves only one char — wait, the outer structure is ^[A-Za-z0-9] (one char) followed by an optional group ([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?. So single-char hostnames do match (the optional group is absent, leaving just the first char). The plan's own learning is incorrect about this being a problem — single-char hostnames do pass. However, hermes (7 chars) passes trivially.

Actually the learning says "rejects single-character hostnames" — let me re-analyze: ^[A-Za-z0-9]([A-Za-z0-9.-]{0,251}[A-Za-z0-9])?$. For input a: matches [A-Za-z0-9] (the a), optional group absent, end anchor. So a DOES match. The learning is wrong, but the plan code is correct. No action needed.

Handoff: Excellent.

---

Software Engineering Principles

P1 — KISS:

The plan introduces three new artifacts: configs/allowed-hosts.txt, scripts/validate-host.sh, and modifications to four existing files. The shared-helper approach (validate-host.sh sourced by the workflow and the script) is an appropriate level of DRY. The allowlist file is a simple text file. No over-engineering detected.

Finding: Clean.

P2 — YAGNI:

Finding (Major): The issue requires validating that host is not arbitrary. A format check alone (RFC 1123 regex) would close the security gap: it would reject shell metacharacters, control characters, newlines, and most injection vectors. The plan adds an allowlist on top of this.

The allowlist is not a stated requirement in the issue — the issue says "arbitrary value, potentially causing Myrmidons to apply against an unintended host." A format check doesn't prevent an attacker from sending a valid-RFC-1123 hostname that isn't in the Myrmidons inventory (e.g., attacker-node). The allowlist does address this scenario, which is implicitly part of "causing Myrmidons to apply against an unintended host."

Re-reading the issue: "any entity...can set host to an arbitrary value, potentially causing Myrmidons to apply against an unintended host." This phrasing suggests the concern is not just injection but also misdirection to a valid-looking but wrong host. The allowlist addresses exactly this. This is within scope — the issue calls out misdirection as a threat. YAGNI is not violated.

Finding: Clean.

P3 — TDD:

The plan defines tests (Cases 4–7) that cover the behavior contracts:

• Case 4: allowlisted host succeeds
• Case 5: non-allowlisted host fails
• Case 6: shell metachar fails at format layer
• Case 7: newline fails at format layer

Tests are planned alongside implementation (step 4 in the implementation order comes before step 6, the workflow modification). This is test-first in spirit.

Minor concern: The test harness uses ALLOWED_HOSTS_FILE env var override to inject fixture allowlists, but the plan's current test code for Case 4 checks grep -q "for host: hermes" — this would trigger the actual curl call (shimmed to 204). This is correct given the shim is set up at the top of the test file. The test would still hit validate_host before reaching curl. ✓

Finding: Clean.

P4 — DRY:

The plan deliberately DRYs validation into validate-host.sh rather than duplicating the logic in the workflow step and dispatch-apply.sh. This is appropriate given both consumers need identical behavior.

Finding: Clean.

P5 — SOLID:

• SRP: validate-host.sh has one job: validate a hostname. dispatch-apply.sh handles dispatching. Separation is maintained.
• DIP: The plan has dispatch-apply.sh sourcing validate-host.sh directly — a concrete file path dependency. This is acceptable for bash; no better abstraction exists in shell.

Finding: Clean.

P6 — Modularity:

validate-host.sh is designed as an independently invokable module (the BASH_SOURCE[0] guard), and as a sourceable library. The allowlist path is parameterizable via the second argument. This is well-designed.

Finding: Clean.

P7 — POLA:

The validate_host function: empty host → error, bad format → error (no raw echo), not in allowlist → error with host echoed. The inconsistency between cases (raw host printed in allowlist error but not format error) is noted above. The rationale is sound (format-validated = safe to log), but a reader encountering the code for the first time might wonder why the inconsistency exists. No comment explaining this is planned.

Minor finding: The plan should note this intent gap — either log consistently or add a comment explaining why the allowlist error can safely log the host value.

---

Critical Issue: Test Case 4 Fragility

Test Case 4 in the plan:

out=$(env ALLOWED_HOSTS_FILE="$FIXTURE_ALLOW" "$REPO_ROOT/scripts/dispatch-apply.sh" hermes 2>&1)

This will call the modified dispatch-apply.sh, which sources validate-host.sh and calls validate_host. After validation passes, the script proceeds to the GITHUB_TOKEN check. But GITHUB_TOKEN is already set in the test environment (line 20 of the test file: export GITHUB_TOKEN="fake-token-for-test"). The script then calls the curl shim, which returns 204. So the test would pass the full flow. This is correct behavior for an integration test.

However, there's a subtle issue: the plan shows Case 4 checking:

echo "$out" | grep -q "for host: hermes"

The dispatch-apply.sh line 23 currently echoes: Dispatching agamemnon-apply to ${MYRMIDONS_REPO} for host: ${HOST}. After the plan's modifications, this line is unchanged. The grep will match. ✓

---

Critical Issue: Workflow Step Logic

The plan's new workflow step:

      - name: Validate client_payload.host
        env:
          HOST: ${{ github.event.client_payload.host }}
        run: |
          set -euo pipefail
          if [[ -z "${HOST}" ]]; then
            ...
            exit 1
          fi
          if ! bash scripts/validate-host.sh "${HOST}" configs/allowed-hosts.txt; then
            ...
            exit 1
          fi
          echo "host=${HOST}" >> "$GITHUB_ENV"

The plan calls bash scripts/validate-host.sh "${HOST}" configs/allowed-hosts.txt — this invokes validate-host.sh as a standalone script (using the BASH_SOURCE[0] == $0 guard), which calls validate_host "$@". This works correctly.

However, there's a subtle issue: the format check pattern uses [[ ! "$host" =~ $pattern ]]. In bash, the =~ operator does NOT require the pattern to be quoted (and in fact should not be when using ERE features), but the pattern is stored in a variable. This is correct bash practice. ✓

---

Summary of Findings

Finding	Severity	Dimension
Allowlist path in workflow is a relative path without mitigation (acknowledged in learnings)	Minor	Risk Surface
Inconsistency in whether raw host is logged in error paths (no comment explaining rationale)	Minor	P7/POLA
Plan doesn't explicitly state the Dispatch apply to Myrmidons step is intentionally unchanged	Minor	Stage Handoff

No critical or major findings. Three minor findings. The plan is fundamentally sound, well-grounded in the actual codebase, addresses the security issue comprehensively, includes proper tests, and provides runnable verification.

By the grading rubric:

• Zero critical findings ✓
• Zero major findings ✓
• Three minor findings (under the ≤2 threshold for A, within B range)

Grade: B — Solid plan with three minor gaps, none blocking.

Grade: B
Verdict: GO

[mvillmow]

Filed consolidated follow-up issue #184 covering 1 item(s)