fix(agent-team): P3-flip — address GPT-4.1 cross-review (size bound, ref-traversal guard)
- BLOCK: cap candidate diff at 40 KB in the dispatcher (the diff rides a base64 workflow_dispatch input; GitHub caps inputs at ~64 KB so an oversized diff cannot dispatch at all) + a defense-in-depth decoded-size bound in materialize. - FIX: harden the draft-PR HEAD_BRANCH guard to reject leading/trailing slash, '..' segments, and '//' (CWE-88 git ref-traversal), not just bad charset. - NIT: document the mandatory invariants on gate-and-pr (required-reviewer environment must stay; runs-on must stay GitHub-hosted). - QUESTION (lockfiles): answered in-code — the build-test sandbox is credential-less + egress-blocked, so lockfile-postinstall RCE is contained. Tests added for all guards. 1042 tests, ruff clean, YAML valid.
This commit is contained in:
parent
9091b27630
commit
a388b9c813
4 changed files with 69 additions and 4 deletions
|
|
@ -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}",
|
||||
|
|
|
|||
|
|
@ -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")"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Reference in a new issue