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.
|
||||
Reference in New Issue
Block a user