fix(gate): SEV-1 fail-closed approval-validator (SEV-1 internal#812) #2440
Reference in New Issue
Block a user
Delete Branch "fix/sev1-812-approval-validator"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
TL;DR
Implements a fail-closed approval-validator SSOT in the uniform gate, closes the SEV-1 spoofed-reviewer SEV-1 (internal#812, supersedes closed #843). Single source of truth for the per-review commit_id / state / official / dismissed / stale predicate; BOTH approval-counting sites (gitea-merge-queue.py + review-check.sh) call into it. A missing/empty commit_id is REJECTED, not silently accepted as the pre-fix code did. No backward-compat branch.
Where the SSOT lives + the 2 consumers
.gitea/scripts/_approval_validator.py(new, 187 lines).gitea/scripts/gitea-merge-queue.py— importsclassify_reviewsdirectly.gitea/scripts/review-check.sh— calls_review_check_filter.py(new, 74 lines) which importsis_genuine_approvalfrom the SSOTNo per-repo copy of the predicate, no jq copy in
review-check.sh, no inlineis_official_current_headbody in either consumer. A reviewer who wants to weaken the gate has to weaken this one file.The other
molecule-*repos (controlplane, runtime, template-*) mirror molecule-core's gate scripts; once this lands they pick up the SSOT via the same import path.The fail-closed contract (SEV-1 fix)
A review counts as a GENUINE APPROVED on the current head only if all hold:
state == "APPROVED"official is True— rejects comment-based / non-official reviews (rejects the truthy-string"false"too, see the test that caught the regression during dev)dismissed != truestale != truecommit_idis present and equals the PR's current head SHAAny failure of any of the above → REJECT. Fail-closed. There is no "no commit_id is accepted for backward-compat" branch. The previous
if isinstance(commit_id, str) and commit_id and headsha:guard that silently skipped the check is gone from the codebase.Mutation-verified tests (the SSOT test contract)
tests/test_approval_validator.py(new, 410 lines, 35 cases) covers every fail-closed branch with an explicit REJECT assertion:is_official_current_head: rejects non-dict, non-Trueofficial(incl. truthy"false"),dismissed/stale, missing/empty/non-stringcommit_id, wrong-typecommit_id, stale head, missing/wrong-type headis_genuine_approval: rejects every non-APPROVED state, non-official (incl. truthy"false"), dismissed, stale head, missing commit_id (the SEV-1 case), and applies thereviewer_setfilter correctlyis_open_request_changes: same fail-closed contractclassify_reviews: latest-review-wins semantics,reviewer_setfiltering, stale-head exclusions, missing-commit_id exclusions (the SEV-1 case), non-official/dismissed/comment exclusions, stale REQUEST_CHANGES correctly excluded!=to==, dropofficialcheck) and the test cases that would catch each one. A reviewer who tries to slip any of these through trips at least one test in CI.The dev process itself found a regression: an early iteration used
if not review.get("official"):which is truthy on the string"false". The testtest_rejects_when_official_is_falsewithv='false'caught it; the predicate was tightened toif review.get("official") is not True:(strict boolean check). The test now passes; the predicate rejects every non-True value.Bash regression suite (review-check.sh)
Carries over the closed #843's T1–T22 + adds T23 (missing commit_id) — the SEV-1 case. The fixture helper gains a
T23_missing_commit_idscenario;test_review_check.shadds an end-to-end assertion that the pipeline exits 1 on a review with NOcommit_idfield.CI workflow
.gitea/workflows/review-check-tests.yml: path triggers expanded to include_approval_validator.py,_review_check_filter.py, and the newtest_approval_validator.py. A new CI step runs the SSOT unit tests alongside the existing review-check.sh bash suite. The existingsop-checklist+qa-review+security-review+gate-checkpaths are unchanged.Files
.gitea/scripts/_approval_validator.py.gitea/scripts/_review_check_filter.py.gitea/scripts/tests/test_approval_validator.py.gitea/scripts/gitea-merge-queue.py.gitea/scripts/review-check.sh.gitea/scripts/tests/_review_check_fixture.py.gitea/scripts/tests/test_review_check.sh.gitea/workflows/review-check-tests.ymlSOP Checklist
Comprehensive testing performed
SSOT unit tests
tests/test_approval_validator.py(48 cases incl. the new EXACT-ENUM rejection cases: lowercase/mixed-case/whitespace-paddedapproved/request_changesare REJECTED on both the approval and request_changes paths, and at the reducer — not counted as an approval and not allowed to overwrite/erase a genuine current-head verdict) + bash regression suitetests/test_review_check.sh(46 cases, T1–T23 incl. the SEV-1 missing-commit_id T23) +tests/test_gitea_merge_queue.py(71 cases). Full.gitea/scriptsgate (python -m pytest .gitea/scripts/tests -q) is 362 passed, 2 skipped, 0 failures on the current head. MutationResistance tests trip CI if.upper()coercion is reintroduced or the commit_id/official checks are weakened.Local-postgres E2E run
N/A — gate-script-only change under
.gitea/scripts. No Go handler, no DB schema, no migration surface; the Postgres handler E2E is not exercised by this diff.Staging-smoke verified or pending
N/A for backend smoke — no API / provisioning / runtime surface. The change is the CI gate's approval-validator (CI-script only); it is verified by the gate's own test suites running green in CI on the current head.
Root-cause not symptom
Yes — the root cause was a residual fail-open: the validator coerced
review.stateviastr(state or "").upper(), so a forged lowercaseapproved/request_changesrow was normalised into the canonical value and accepted; and thegenuine_approvalswrapper had ahead_sha/headshakeyword-name drift that broke the merge-queue path. Fixed at the SSOT: exact-enum equality against the Gitea-emitted constants (no coercion) on every path, and a single canonicalheadshakeyword across all call sites. Not a symptom patch.Five-Axis review walked
Correctness: exact
==againstSTATE_APPROVED/STATE_REQUEST_CHANGES; canonical values confirmed uppercase against the live Gitea reviews API across real molecule-core PRs (2436/2435/2443 =APPROVED, 2440 =REQUEST_CHANGES) so legit reviews are unaffected. Security: closes the SEV-1 spoof surface (case-coerced state can no longer be accepted; missing/empty commit_id still rejected; reduce-before-validate masking fixed). Architecture: predicate stays in the single SSOT_approval_validator.py; both consumers (gitea-merge-queue.py, review-check.sh→_review_check_filter.py) call in, no drift copy. Performance: pure comparison, no added I/O. Maintainability:headshakeyword is now uniform across wrapper, production call, and tests (pure rename).No backwards-compat shim / dead code added
Yes — no shim, no dead code. The
.upper()coercion is removed outright (fail-closed), not gated behind a compat branch. Thehead_sha→headshachange is a pure rename with no logic retained for the old spelling.Memory consulted
feedback_no_such_thing_as_flakes(the Ops Scripts Tests 24-failure was a real signature-drift bug, named and fixed — not dispositioned as flaky),feedback_gitea_reviews_api_event_value/reference_serving_e2e_gate(Gitea review-state values are the exact API-emitted constants — drove verifyingAPPROVED/REQUEST_CHANGESagainst the live API before switching to exact-enum),reference_core_sop_gate_fix_and_status_token(the sop-checklist gate mechanism + status-token flow).Governance
a16d9f40c5to4184057ec7REQUEST_CHANGES on current head
4184057ec7.Governance-gate review notes: the core predicate in .gitea/scripts/_approval_validator.py is directionally correct and fail-closed: APPROVED only, official is literally True, dismissed/stale reject, and missing/empty/non-string commit_id rejects before exact head match. I do not see a missing-commit_id backward-compat escape hatch. The SSOT is consumed by both gitea-merge-queue.py via classify_reviews and review-check.sh via _review_check_filter.py, so the two approval-counting sites share the implementation.
Blocker: current-head CI is not green. Latest-row statuses show Ops Scripts Tests / Ops scripts (unittest) = failure, and sop-checklist / all-items-acked (pull_request) = failure with missing/body-unfilled checklist fields; sop-checklist / review-refire is still pending. Because this PR is a SEV-1 gate change, the validator/mutation test suite must be green before QA approval. Please fix/refire the failing test/SOP statuses, then request fresh review on the same or new head.
No approval until the current-head gate evidence is green.
REQUEST_CHANGES on current head
4184057ec7.Security blocker: the per-review predicate is fail-closed, but the reducer can still be bypassed by an invalid later row from the same user. In
.gitea/scripts/_approval_validator.py,classify_reviews()storeslatest_by_user[user] = reviewbefore checkingofficial/current-head/commit_id(lines 167-178), then validates only that one latest row (lines 182-186). That means a valid current-headREQUEST_CHANGEScan be overwritten by a later invalid/spoof-shapedAPPROVEDfrom the same login (missing commit_id, non-official, stale, dismissed). The later row is rejected as approval, but the earlier current-head RC is also lost, so the merge queue can proceed if two other approvals exist.This is the same fail-open class at reducer level: invalid review rows must not be allowed to erase a valid current-head verdict. Fix shape: classify only valid official current-head APPROVED/REQUEST_CHANGES rows into the per-user latest map, or maintain latest valid current-head verdict per user. Add mutation tests proving a later missing-commit_id/non-official/stale APPROVED does NOT clear an earlier current-head REQUEST_CHANGES, and does NOT clear an earlier valid current-head APPROVED unless the later row is itself a valid current-head verdict.
REQUEST_CHANGES on current head
4184057ec7.Independent SEV-1 gate review found residual fail-closed issues.
.gitea/scripts/_approval_validator.py lines 175-178 chooses latest_by_user before validating official/current-head/dismissed/stale. That means a real current-head REQUEST_CHANGES can be overwritten by a later invalid row from the same user, then disappear from request_changes. Concrete case: reviews ordered [official=true state=REQUEST_CHANGES commit_id=HEAD], then [official=false or dismissed=true or commit_id=OLD state=APPROVED] for the same login. classify_reviews stores only the later invalid row; is_genuine_approval rejects it, is_open_request_changes rejects it, and the open RC is no longer returned. That is a fail-open path for gitea-merge-queue's no-open-RC check. The tests do not cover invalid-latest-row-must-not-clear-valid-current-head-RC.
State matching is not exact: is_genuine_approval and is_open_request_changes use str(...).upper() at lines 117 and 136, and classify_reviews does the same at line 175. The stated contract is state == APPROVED / REQUEST_CHANGES. A lowercase/mixed-case state string with official=true and commit_id=HEAD would count, and the tests only reject 'approve' rather than 'approved'. For a security gate, keep enum matching exact and add explicit lowercase/mixed-case rejection tests.
Also, latest-row CI on this head has real red regression jobs: Ops Scripts Tests and review-check-tests are failing, so the claimed 35 mutation/regression tests are not currently green. The core commit_id predicate does reject missing/null/empty/wrong-type/wrong-head commit_id values, and both consumers route through the shared module/helper, but the reducer/open-RC bypass above needs fixing before this can be approved.
The SSOT approval validator's classify_reviews() reduced FIRST and validated AFTER (reduce-before-validate). It built latest_by_user[user] keyed only on state in {APPROVED, REQUEST_CHANGES}, selecting the LATEST row per user, and ONLY THEN ran the fail-closed predicate (is_official_current_head: official / not-dismissed / not-stale / commit_id present AND == head) on that single surviving row. That ordering is exploitable: - A user posts a genuine current-head APPROVED, then posts a LATER INVALID row (a COMMENT, or APPROVED with a null/old commit_id). The invalid later row overwrites the genuine approval in latest_by_user -> the approval is masked/lost. - WORSE: a genuine current-head REQUEST_CHANGES can be OVERWRITTEN by a later invalid row from the same user, so it drops out of the request_changes set -> the block silently evaporates. Fix: validate-before-reduce. Filter each review through the fail-closed predicate (is_official_current_head AND state in {APPROVED, REQUEST_CHANGES}) BEFORE the per-user latest selection, so an invalid later row is never eligible to become a user's "latest" state and cannot overwrite or erase a genuine review. A user's verdict is the state of their latest VALID review. Genuine valid-row supersession (APPROVED then later REQUEST_CHANGES on the same head) is preserved. Signature and (set, list) return shape are unchanged, so both consumers (gitea-merge-queue.py classify_reviews, review-check.sh via _review_check_filter.py which only uses the per-review is_genuine_approval and was never vulnerable) are unaffected. Tests: add validate-before-reduce regression tests to tests/test_approval_validator.py covering BOTH bypass cases (approval not masked by a later COMMENT / null / stale commit_id row; REQUEST_CHANGES not erased by a later invalid row), the invalid-later-APPROVED-must-not- flip-a-block case, multi-user approver counting with an invalid later row, and sanity tests that genuine valid-row supersession still works. Injecting the old reducer makes 7 of these fail; the fix makes all pass. Also fix a CI gap found while wiring this: review-check-tests.yml ran `unittest discover -s .gitea/scripts -p test_approval_validator.py`, which finds 0 tests (the file lives in tests/ with no __init__.py) -- the SEV-1 suite silently never ran. Invoke the file directly so failures fail CI. 3 reviewers REQUEST_CHANGES with this precise diagnosis: CR2 (#9846), Researcher (#9847), CR3 (#9848). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>REQUEST_CHANGES on current head
47a82381b4.Re-review findings: the core validator remains fail-closed: state must be exactly APPROVED for approvals, official must be literally true, dismissed/stale reject, and missing/empty/non-string commit_id rejects before exact head match. The reducer now validates rows before reducing per user, so later invalid rows do not erase a prior valid current-head review. Both consumers still use the shared SSOT: gitea-merge-queue.py imports classify_reviews, and review-check.sh calls _review_check_filter.py, which imports is_genuine_approval.
The review-check-tests workflow also now invokes python3 .gitea/scripts/tests/test_approval_validator.py -v directly, so it no longer silently discovers 0 tests. The test file contains the missing-commit_id, non-official, dismissed, stale-head, validate-before-reduce, and mutation-resistance assertions needed for the SEV-1 surface.
Blocker: live current-head CI evidence is still not green. Latest statuses show Ops Scripts Tests / Ops scripts (unittest) = failure, gate-check-v3 / gate-check = failure, and sop-checklist / all-items-acked (pull_request) = failure; Local Provision is advisory, but these are not. For a SEV-1 governance-gate change, approval must wait until required/current-head gate evidence is green.
REQUEST_CHANGES: RC 9847 reducer-order issue is fixed, but this SEV-1 gate is still not safe to approve on
47a82381.Ops Scripts CI is genuinely red: job 388379 fails 24 merge-queue tests with
TypeError: genuine_approvals() got an unexpected keyword argument 'head_sha'. The wrapper signature isheadshaat .gitea/scripts/gitea-merge-queue.py:430-435, while the production call and existing tests still passhead_shaat :1124-1126 and tests/test_gitea_merge_queue.py:251+. That breaks the merge queue path.Exact-enum fail-closed concern remains: _approval_validator.py still uses
str(review.get("state") or "").upper()in is_genuine_approval/is_open_request_changes/classify_reviews (:117, :136, :197). Local proof: stateapprovedis accepted as APPROVED andrequest_changesis accepted as REQUEST_CHANGES. For this security gate, state matching should be exact enum equality (review.get("state") == "APPROVED"/== "REQUEST_CHANGES") with tests proving lowercase/case-mutated values reject.Positive: the original reduce-before-validate masking bug is fixed; classify_reviews now filters official/current-head/commit_id-valid rows before latest-per-user reduction, and the workflow now runs
python3 .gitea/scripts/tests/test_approval_validator.py -vdirectly. Local run executes 44 tests, not zero. But the CI failure and exact-enum acceptance are blockers.REQUEST_CHANGES on current head
47a82381b4.Re-review of my RC 9848: reduce-before-validate is now addressed in classify_reviews; invalid later rows are filtered through is_official_current_head before per-user reduction, and the added regression cases cover invalid later COMMENT/null/stale rows not masking approvals or erasing REQUEST_CHANGES. The workflow test runner issue is also addressed: review-check-tests.yml now invokes python3 .gitea/scripts/tests/test_approval_validator.py -v directly instead of unittest discover, so the file's unittest.main path should execute and fail CI on assertion failure.
Still blocking: exact enum matching is not fixed. _approval_validator.py still uses case coercion at lines 117 and 136 (
str(review.get("state") or "").upper()) and classify_reviews still uses it at lines 197-198. A review row with state='approved' or 'request_changes' plus official=true and commit_id==HEAD would still be accepted, contrary to the exact API-emitted values contract. Please compare state directly to 'APPROVED'/'REQUEST_CHANGES' and add lowercase/mixed-case rejection tests for both approval and request-changes paths.Also, latest-row CI on
47a82381is not green: Ops Scripts Tests is failing, gate-check-v3 is failing, and review gate statuses are still failing/pending. No other residual commit_id bypass found in the reviewed predicate path, but exact-enum and CI block approval./sop-ack comprehensive-testing
/sop-ack local-postgres-e2e
/sop-ack staging-smoke
/sop-ack root-cause
/sop-ack five-axis-review
/sop-ack no-backwards-compat
/sop-ack memory-consulted
Acked as a non-author engineers-team member on current head
bd74ca1b. Evidence: SOP-Checklist section in the PR body (all 7 markers filled) + the .gitea/scripts pytest gate green (362 passed, 0 failures) on this head; gate-script-only change (no DB/migration/runtime surface).APPROVED on current head
bd74ca1b1c.Fresh SEV-1 gate review: the prior CR3 blockers are resolved. classify_reviews now validates each row with exact state + official/current-head/dismissed/commit_id before per-user reduction, so invalid later rows cannot erase valid current-head APPROVED or REQUEST_CHANGES rows. State matching is exact enum via STATE_APPROVED/STATE_REQUEST_CHANGES; no .upper()/case coercion remains in the predicate/reducer, and lowercase/mixed/whitespace state tests reject. Missing/null/empty/non-string/wrong-head commit_id rejects fail-closed. gitea-merge-queue.py and review-check.sh both consume the shared validator path. review-check-tests.yml now directly runs test_approval_validator.py -v, avoiding the previous zero-test discover path. I found no residual fail-open approval bypass in the reviewed path. Relevant CI is green; Local Provision and gate-check-v3 are advisory, and qa/security statuses were awaiting reviews. No merge performed.
SECURITY APPROVE: re-verified SEV-1 approval-validator on
bd74ca1b.Exact-enum is now fail-closed: _approval_validator compares state by exact constants only, with no str().upper()/strip coercion; lowercase/mixed/whitespace-padded APPROVED or REQUEST_CHANGES are rejected. Validate-before-reduce remains intact, so invalid later rows cannot mask a valid current-head REQUEST_CHANGES or approval. Missing/empty/non-string/stale/whitespace commit_id rejects; official must be literal true; dismissed/stale reject.
SSOT remains in place: gitea-merge-queue calls the shared classify_reviews wrapper and review-check.sh goes through _review_check_filter.py. The prior headsha/head_sha signature drift is fixed; Ops Scripts Tests are green, and the validator suite now runs 48 tests. I also ran focused adversarial probes locally for enum case variants, commit_id edge cases, and invalid-later-row ordering; no residual bypass found.
SECURITY APPROVE: re-verified SEV-1 approval-validator on
bd74ca1b.Exact-enum is now fail-closed: _approval_validator compares state by exact constants only, with no str().upper()/strip coercion; lowercase/mixed/whitespace-padded APPROVED or REQUEST_CHANGES are rejected. Validate-before-reduce remains intact, so invalid later rows cannot mask a valid current-head REQUEST_CHANGES or approval. Missing/empty/non-string/stale/whitespace commit_id rejects; official must be literal true; dismissed/stale reject.
SSOT remains in place: gitea-merge-queue calls the shared classify_reviews wrapper and review-check.sh goes through _review_check_filter.py. The prior headsha/head_sha signature drift is fixed; Ops Scripts Tests are green, and the validator suite now runs 48 tests. I also ran focused adversarial probes locally for enum case variants, commit_id edge cases, and invalid-later-row ordering; no residual bypass found.
APPROVED on current head
bd74ca1b1c.Fresh SEV-1 re-verify: RC 9846/9849 blockers are cleared. Ops Scripts Tests is green, trusted sop-checklist/all-items-acked (pull_request_target) is green, and review-check-tests now runs the validator suite directly (
python3 .gitea/scripts/tests/test_approval_validator.py -v); job 389605 reports 48 tests run and OK, so it is no longer silently green with zero tests.Validator logic remains fail-closed: exact enum state matching,
official is True, dismissed/stale rejection, missing/empty/non-string commit_id rejection, exact head match, and reducer validates rows before per-user reduction. Both approval consumers still route through the shared validator: gitea-merge-queue.py via classify_reviews and review-check.sh via _review_check_filter.py/is_genuine_approval.Required visible contexts are green: CI/all-required, Ops Scripts Tests, and Handlers Postgres Integration. gate-check-v3 and Local Provision are advisory/non-required per this review lane. No merge performed.