diff --git a/agent-team/agent_team/dispatcher.py b/agent-team/agent_team/dispatcher.py index c6f7111..75f9fc6 100644 --- a/agent-team/agent_team/dispatcher.py +++ b/agent-team/agent_team/dispatcher.py @@ -46,6 +46,15 @@ WORKFLOW_FILE = "agent-team-apply-verify.yml" # steps look for ``candidate.diff`` (mirrors nodes.fixer.CANDIDATE_DIFF_FILENAME). CANDIDATE_DIFF_FILENAME = "candidate.diff" +# Max candidate-diff size. NOT arbitrary: the diff is carried as the base64 +# `diff_b64` workflow_dispatch INPUT, and GitHub caps total dispatch inputs at +# ~65,535 bytes. base64 inflates ~4/3, so a diff over ~45 KB cannot be +# dispatched at all. We cap at 40 KB (leaving headroom for the other inputs) and +# fail closed with a clear error rather than letting GitHub reject the dispatch +# opaquely — and to bound resource use on the operator host. Larger diffs need a +# branch-only transport (future), not an inline input. +MAX_DIFF_BYTES = 40_000 + # task_id must match the workflow's gate-and-pr safety guard charset (it is # embedded in the head ref and the PR text). A coordinator thread_id is a uuid, # well within this set; we validate to fail closed on anything else. @@ -123,6 +132,12 @@ def build_dispatch_inputs( ) head_branch = head_branch_for(task_id) # validates task_id raw = diff_text.encode("utf-8") + if len(raw) > MAX_DIFF_BYTES: + raise DispatcherError( + f"diff too large ({len(raw)} bytes > {MAX_DIFF_BYTES}); the diff is " + "carried as a base64 workflow_dispatch input (GitHub caps inputs at " + "~64 KB). Split the change or use a branch-only transport." + ) return DispatchInputs( task_id=task_id, diff_artifact_name=f"{artifact_prefix}-{task_id}", diff --git a/agent-team/ci/agent-team-apply-verify.yml b/agent-team/ci/agent-team-apply-verify.yml index 831035c..d74aaca 100644 --- a/agent-team/ci/agent-team-apply-verify.yml +++ b/agent-team/ci/agent-team-apply-verify.yml @@ -142,10 +142,17 @@ jobs: set -euo pipefail mkdir -p ./_out printf '%s' "$DIFF_B64" | base64 -d > ./_out/candidate.diff - # Fail-closed: non-empty + sha256 must equal the ledger-recorded hash - # the dispatcher passed (guard re-verifies this independently). An empty - # expected hash never counts as a match. + # Fail-closed: non-empty + bounded size + sha256 must equal the + # ledger-recorded hash the dispatcher passed (guard re-verifies this + # independently). An empty expected hash never counts as a match. test -s ./_out/candidate.diff + # Size bound (defense-in-depth; the dispatch input is already ~64 KB + # capped by GitHub, the dispatcher caps the diff at 40 KB): reject an + # oversized decoded diff rather than feeding it downstream. + bytes="$(wc -c < ./_out/candidate.diff)" + if [ "$bytes" -gt 49152 ]; then + echo "::error::candidate diff too large (${bytes} bytes)"; exit 1 + fi actual="$(sha256sum ./_out/candidate.diff | cut -d' ' -f1)" if [ -z "$EXPECTED_DIFF_HASH" ] || [ "$actual" != "$EXPECTED_DIFF_HASH" ]; then echo "::error::materialized diff hash ${actual} != expected '${EXPECTED_DIFF_HASH}'" @@ -998,6 +1005,11 @@ jobs: # FLIPPED LIVE 2026-06-22 (provisioning done: agent-apply env + required # reviewer amoussa1229 + the GitHub App secrets exist). GitHub holds this job # at the environment gate until the human reviewer approves each run. + # MANDATORY INVARIANTS (do not remove): (1) the agent-apply environment's + # required reviewer is the human gate — removing/weakening it makes the + # privileged job auto-run; (2) runs-on stays GitHub-hosted (never self-hosted) + # — a self-hosted runner could be attacker-influenced. Both are asserted by + # tests/test_apply_verify_workflow_hardening.py. environment: name: agent-apply permissions: @@ -1164,8 +1176,12 @@ jobs: case "$TASK_ID" in *[!A-Za-z0-9._-]* | "" ) echo "::error::unsafe task_id"; exit 1 ;; esac + # Reject not just bad chars/empty, but also a leading/trailing slash, + # any '..' segment, or '//' — so a tampered head_branch can never be a + # git ref-traversal or resolve to an unintended ref (CWE-88). case "$HEAD_BRANCH" in - *[!A-Za-z0-9._/-]* | "" ) echo "::error::unsafe head_branch"; exit 1 ;; + *[!A-Za-z0-9._/-]* | "" | /* | */ | *..* | *//* ) + echo "::error::unsafe head_branch"; exit 1 ;; esac title="$(printf 'agent-apply: %s (diff %s)' "$TASK_ID" "$DIFF_HASH")" body="$(printf 'Automated draft PR from the agent-team apply/verify pipeline.\n\nTask: %s\nDiff hash: %s\nHead: %s\n\nDRAFT ONLY — never auto-merged. Requires: green required checks, security review, Claude Code App review, and human approval (D2, boundary 5).' "$TASK_ID" "$DIFF_HASH" "$HEAD_BRANCH")" diff --git a/agent-team/tests/test_apply_verify_workflow_hardening.py b/agent-team/tests/test_apply_verify_workflow_hardening.py index f8c6578..12fd29c 100644 --- a/agent-team/tests/test_apply_verify_workflow_hardening.py +++ b/agent-team/tests/test_apply_verify_workflow_hardening.py @@ -503,3 +503,28 @@ def test_draft_pr_opens_with_head_branch_and_sanitizes_inputs() -> None: # §4.6 / defense-in-depth: task_id + head_branch are charset-validated. assert "unsafe task_id" in s["run"] assert "unsafe head_branch" in s["run"] + + +def test_head_branch_guard_rejects_ref_traversal() -> None: + # CWE-88: the draft-PR step rejects a head_branch with a leading/trailing + # slash, a '..' segment, or '//' (not just bad chars). + pr_steps = [ + s + for _, s in _iter_steps(_doc()) + if "draft" in s.get("name", "").lower() and s.get("run") + ] + assert pr_steps + for s in pr_steps: + run = s["run"] + assert "*..*" in run and "*//*" in run and "/*" in run, ( + "head_branch guard must reject .. // and leading/trailing slash" + ) + + +def test_materialize_bounds_decoded_diff_size() -> None: + steps = [ + s for _, s in _iter_steps(_doc()) if s.get("run") and "base64 -d" in s["run"] + ] + assert steps + for s in steps: + assert "too large" in s["run"], "materialize must bound the decoded diff size" diff --git a/agent-team/tests/test_dispatcher.py b/agent-team/tests/test_dispatcher.py index a45273c..a6802ef 100644 --- a/agent-team/tests/test_dispatcher.py +++ b/agent-team/tests/test_dispatcher.py @@ -12,6 +12,7 @@ import base64 import pytest from agent_team.dispatcher import ( + MAX_DIFF_BYTES, DispatcherError, DispatchInputs, build_dispatch_inputs, @@ -59,6 +60,14 @@ def test_empty_diff_rejected(bad_diff: str) -> None: build_dispatch_inputs(task_id=TASK, diff_text=bad_diff, declared_scope=SCOPE) +def test_oversized_diff_rejected() -> None: + # The diff rides a base64 workflow_dispatch input (GitHub ~64 KB cap); a diff + # over MAX_DIFF_BYTES must fail closed in the dispatcher, not be dispatched. + big = "diff --git a/x b/x\n" + "+" + ("x" * (MAX_DIFF_BYTES + 1)) + "\n" + with pytest.raises(DispatcherError): + build_dispatch_inputs(task_id=TASK, diff_text=big, declared_scope=SCOPE) + + @pytest.mark.parametrize("bad_scope", ["", " "]) def test_empty_scope_rejected(bad_scope: str) -> None: # An empty declared scope would let a diff touch ANY path — fail closed.