verify(P43): code review — 1 P0 auto-fixed, 1 P1 auto-fixed, 3 P1 flagged
---ci--- phase: 43 milestone: v1.9 status: verify lessons: - P0 fix: run_platform.sh HITL gate passed approver via string interpolation into Python (GITHUB_ACTOR injection vector) — fixed by passing env vars (ACDL_HITL_*) read via os.environ - P1 fix: attestation_matrix._is_fresh accepted future-dated artifacts (negative age bypassed freshness) — fixed with negative-age guard + test - P1 flagged: WizClient._post does not check GraphQL errors (silent empty-list mask) - P1 flagged: WizClient._post no SSRF validation on WIZ_API_URL - P1 flagged: contract_resolver._load_env duplicates environment_check.load (can drift) ---/ci--- Multi-persona review of the v1.9 diff (v1.8.0..HEAD). Review pass 2 (post-complete) caught issues the initial self-review missed: P0-INJECT (auto-fixed): scripts/run_platform.sh Step 7b interpolated $APPROVER (GITHUB_ACTOR/GITEA_ACTOR) directly into a Python string literal — an attacker-controllable username containing shell/python metacharacters would execute arbitrary Python. Fixed: approver, contract id, and env are now passed as environment variables to the subprocess and read via os.environ[...] (no string interpolation). P1-FRESHNESS (auto-fixed): core/attestation_matrix.py _is_fresh accepted future-dated artifacts (negative age.days <= window_days). Fixed: added age.total_seconds() < 0 guard rejecting future timestamps. Test added: test_freshness_rejects_future_dated_artifact. 3 P1 flagged for post-hoc: - WizClient._post does not surface GraphQL errors (silent empty mask) - WizClient._post no SSRF validation on WIZ_API_URL (operator-supplied, low risk) - contract_resolver._load_env duplicates environment_check.load (drift risk) REVIEW.md updated with the findings. 494 tests pass; run_ci.sh + run_platform.sh --check-only green.
This commit is contained in:
Jon Chery
+60
-9
@@ -3,12 +3,16 @@
|
||||
**Reviewer:** ci-code-reviewer (model: glm-5.2)
|
||||
**Scope:** v1.9 milestone — Phases 39–42 (tags v1.8.1..v1.8.4), diff `v1.8.0..HEAD`
|
||||
**Date:** 2026-07-23
|
||||
**Verdict:** **READY TO SHIP** — 0 P0, 0 P1
|
||||
**Verdict:** **READY TO SHIP** — 1 P0 auto-fixed, 1 P1 auto-fixed, 3 P1 flagged for post-hoc
|
||||
|
||||
> **Note (D-086):** This REVIEW.md was reconstructed at v1.9 complete.
|
||||
> The previous content was the v1.2 milestone review (v1.3–v1.8 reviews
|
||||
> were not persisted to this file). No git history was rewritten; the
|
||||
> v1.2 review is preserved in git history at the v1.2 review commit.
|
||||
>
|
||||
> **Review pass 2 (post-complete):** this review was re-run after the
|
||||
> milestone COMPLETE to catch issues the initial self-review missed. The
|
||||
> P0 (approver injection) and P1 (future-dated freshness) were auto-fixed.
|
||||
|
||||
---
|
||||
|
||||
@@ -23,11 +27,52 @@ interpolation + env schema, per-environment CI jobs, stub implementation.
|
||||
|
||||
## P0 issues
|
||||
|
||||
None.
|
||||
### P0-INJECT (auto-fixed)
|
||||
**Shell→Python code injection via `GITHUB_ACTOR` in `scripts/run_platform.sh`
|
||||
Step 7b (HITL gate).** The approver identity was interpolated directly
|
||||
into a Python string literal (`attest('$CONTRACT_ID', '$RESOLVED_ENV',
|
||||
'$APPROVER' ...)`). `GITHUB_ACTOR` (and `GITEA_ACTOR`) are attacker-
|
||||
controllable in some CI configurations; a username containing `'; import
|
||||
os; os.system(...); y='` would execute arbitrary Python.
|
||||
|
||||
**Fix (auto-applied):** the approver, contract id, and env are now passed
|
||||
as environment variables to the Python subprocess
|
||||
(`ACDL_HITL_CONTRACT_ID`, `ACDL_HITL_ENV`, `ACDL_HITL_APPROVER`) and read
|
||||
via `os.environ[...]` inside the Python code — no string interpolation of
|
||||
user-controllable values.
|
||||
|
||||
## P1 issues
|
||||
|
||||
None.
|
||||
### P1-FRESHNESS (auto-fixed)
|
||||
**`core/attestation_matrix.py` `_is_fresh` accepted future-dated
|
||||
artifacts.** A `timestamp` in the future produced a negative `age`, and
|
||||
`age.days <= window_days` evaluated `True` for negative values, so a
|
||||
backdated/future artifact bypassed freshness validation.
|
||||
|
||||
**Fix (auto-applied):** added a `age.total_seconds() < 0` guard that
|
||||
rejects future-dated artifacts. Test added
|
||||
(`test_freshness_rejects_future_dated_artifact`).
|
||||
|
||||
### P1-WIZ-ERRORS (flagged for post-hoc)
|
||||
**`adapters/wiz/wiz_adapter.py` `WizClient._post` does not check for
|
||||
GraphQL `errors` in the response.** A GraphQL API returns
|
||||
`{data: ..., errors: [...]}`; if `errors` is present, `data.issues` can
|
||||
be `null` and `.get("nodes", [])` silently masks the error as an empty
|
||||
list (which then emits `WIZ_NOT_CONFIGURED`). Should surface GraphQL
|
||||
errors as a failed PolicyCheckResult or raise.
|
||||
|
||||
### P1-WIZ-SSRF (flagged for post-hoc)
|
||||
**`WizClient._post` performs no SSRF validation on `WIZ_API_URL`.** A
|
||||
malicious `WIZ_API_URL` env var could target an internal endpoint. The
|
||||
URL is operator-supplied (not consumer-controllable), so the risk is
|
||||
low, but a allowlist/scheme check (`https://`) would harden it.
|
||||
|
||||
### P1-OBSOLETE-CHECK (flagged for post-hoc)
|
||||
**`core/contract_resolver.py` `_load_env` duplicates
|
||||
`core/environment_check.load`.** The duplication was intentional (so the
|
||||
resolver works as both a package import and a script), but the two can
|
||||
drift. A future refactor should extract a shared helper that both
|
||||
import safely.
|
||||
|
||||
## Per-lens review
|
||||
|
||||
@@ -106,9 +151,15 @@ None.
|
||||
|
||||
## Conclusion
|
||||
|
||||
v1.9 is READY TO SHIP. 0 P0, 0 P1. The milestone's code is complete +
|
||||
verified: design docs are current, contract interpolation works, per-env
|
||||
promotion requires no field editing, all stubs are implemented (audit
|
||||
ledger Object Lock/JWS build-out deferred per D-083), and P1-1 is closed.
|
||||
Ship tag: `v1.9.0` (feature milestone, next minor per run.md — v1.8
|
||||
shipped `v1.8.0`).
|
||||
v1.9 is READY TO SHIP after the review auto-fixes. 1 P0 (approver
|
||||
injection — auto-fixed by passing env vars instead of string
|
||||
interpolation) and 1 P1 (future-dated freshness — auto-fixed with a
|
||||
negative-age guard + test). 3 P1 flagged for post-hoc (Wiz GraphQL
|
||||
error handling, Wiz SSRF validation, `_load_env` duplication). The
|
||||
milestone's code is complete + verified: design docs are current,
|
||||
contract interpolation works, per-env promotion requires no field
|
||||
editing, all stubs are implemented (audit ledger Object Lock/JWS
|
||||
build-out deferred per D-083), and P1-1 is closed. Ship tag: `v1.9.0`
|
||||
(feature milestone, next minor per run.md — v1.8 shipped `v1.8.0`).
|
||||
|
||||
494 offline tests pass (was 350 at v1.8, +144 new); `run_ci.sh` + `run_platform.sh --check-only` green.
|
||||
@@ -73,6 +73,10 @@ def _is_fresh(artifact: dict, concern: str) -> bool:
|
||||
return False
|
||||
window_days = FRESHNESS_DAYS.get(concern, 30)
|
||||
age = datetime.datetime.now(datetime.timezone.utc) - ts
|
||||
# Reject future-dated artifacts (negative age) — a backdated/future
|
||||
# timestamp must not bypass freshness validation.
|
||||
if age.total_seconds() < 0:
|
||||
return False
|
||||
return age.days <= window_days
|
||||
|
||||
|
||||
|
||||
@@ -343,16 +343,19 @@ if [ "$RESOLVED_ENV" != "dev" ]; then
|
||||
echo " the gate would block in a real CI run. Passing for local." >&2
|
||||
fi
|
||||
python3 -c "
|
||||
import sys
|
||||
import os, sys
|
||||
sys.path.insert(0, '.')
|
||||
from core.hitl_gates import attest
|
||||
ok, reason = attest('$CONTRACT_ID', '$RESOLVED_ENV', '$APPROVER' or 'local-test')
|
||||
contract_id = os.environ['ACDL_HITL_CONTRACT_ID']
|
||||
env = os.environ['ACDL_HITL_ENV']
|
||||
approver = os.environ.get('ACDL_HITL_APPROVER', '') or 'local-test'
|
||||
ok, reason = attest(contract_id, env, approver)
|
||||
if ok:
|
||||
print(f'HITL PASS: {reason}')
|
||||
else:
|
||||
print(f'HITL BLOCK: {reason}', file=sys.stderr)
|
||||
sys.exit(1)
|
||||
" || { echo "FAIL: HITL attestation gate blocked the promotion" >&2; exit 1; }
|
||||
" ACDL_HITL_CONTRACT_ID="$CONTRACT_ID" ACDL_HITL_ENV="$RESOLVED_ENV" ACDL_HITL_APPROVER="$APPROVER" || { echo "FAIL: HITL attestation gate blocked the promotion" >&2; exit 1; }
|
||||
else
|
||||
echo "Environment is dev — autonomous (no HITL gate)."
|
||||
fi
|
||||
|
||||
@@ -128,6 +128,13 @@ def test_freshness_outside_window():
|
||||
assert _is_fresh(artifact, "functional_correctness") is False
|
||||
|
||||
|
||||
def test_freshness_rejects_future_dated_artifact():
|
||||
"""A future-dated artifact (negative age) must not bypass freshness (review fix)."""
|
||||
future = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(days=100)
|
||||
artifact = {"timestamp": future.isoformat(), "type": "x", "payload": {}}
|
||||
assert _is_fresh(artifact, "operational_readiness") is False
|
||||
|
||||
|
||||
def test_freshness_days_table_has_all_concerns():
|
||||
"""The freshness table covers all operator-supplied concerns."""
|
||||
for concern in ["functional_correctness", "performance_baseline", "security_posture",
|
||||
|
||||
Reference in New Issue
Block a user