Skip to content

fix(hydra-gates): gate-7 follows delegation; gates 6/7 stop passing on an empty scope - #149

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/gate7-delegation-and-empty-scope
Aug 4, 2026
Merged

fix(hydra-gates): gate-7 follows delegation; gates 6/7 stop passing on an empty scope#149
rubenvdlinde merged 1 commit into
mainfrom
fix/gate7-delegation-and-empty-scope

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Two blockers to turning enable-hydra-gates: true on fleet-wide. Both live in shared infrastructure, so this PR is left for a human — a broken run-hydra-gates.sh breaks every repo's gate run.

1. Gate-7 (no-admin-idor) false-positived on ordinary delegation

It only recognised an authorization guard written in the routed method's own body. Any controller that centralises authorisation in an injected responder, or routes thin public actions through private helpers first, went red on correct code.

Measured on decidesk development: 11 findings, all 11 guarded.

shape count guard actually reached
$this->responder->staffAction() 8 requireStaff()currentUid() !== null and isStaff(), else 401/403
$this->responder->citizenAction() 3 401 for an anonymous caller (authenticated citizen by design)

validateProposal needed three hops: validateProposalapproveProposal/rejectProposalapplyProposalDecisionstaffAction. Zero real exposure.

Pattern 4 — resolved delegation

Evidence-based, not a naming convention:

a. Cross-class resolution. The controller's typed constructor-promoted properties give $prop -> ClassName. The class is resolved to a real file under the app's lib/ tree, confirmed by an actual class <Name> declaration, and parsed. A call $this->prop->method( clears the routed method only when method is demonstrably guard-bearing in the collaborator's own source. An unresolvable class clears nothing (fail closed).

b. Transitive closure to a fixpoint over same-class calls, so a chain of any length is followed.

Propagation uses a stricter signal than the existing one-hop Pattern 1: a bare throw and a 404 do not seed a chain. NotFoundException is not an authorisation guard, and chaining it would let "this can fail" stand in for "this checks who you are" arbitrarily far up the call graph. Every hop must also occur before the caller's first data mutation.

Both directions are tested

A clear that cannot be shown to still fail is only evidence about itself. 9 new tests — 3 assert the decidesk shapes pass, 6 assert the gate still catches:

  • a plainly unguarded method;
  • a collaborator method that exists but is not a guard (respond()) — the sharpest control, proving resolution discriminates between methods of the same collaborator;
  • an unresolvable collaborator type;
  • a chain terminating in nothing;
  • a guard that runs after the write;
  • a bare-throw collaborator.

Verified live against decidesk: 11 findings → 0, and 5 deliberately-injected unguarded shapes → all 5 still flagged.

2. Gates 6 and 7 reported PASS when the SCOPE was empty

Zero files in the diff meant an empty findings log, a count of 0, and a _pass over nothing inspected — the same failure family as #147, where a missing helper made gate-7 report PASS over 11 real unguarded endpoints.

They now _skip, which keeps them out of _EMITTED_GATES so the coverage summary lists them. Verified against decidesk with --base HEAD:

[gate-6] orphan-auth: SKIPPED — scope was empty — 0 lib/Service or lib/Controller PHP file(s) ...
[gate-7] no-admin-idor: SKIPPED — scope was empty — 0 lib/Controller PHP file(s) ...
[hydra-gates] GATES THAT DID NOT RUN: 4 6 7 24 33

Test evidence

  • test_check_no_admin_idor.py64/64 (55 pre-existing + 9 new)
  • tests/run-helper-suites.sh — 17 passed, 0 failed, 2 pre-existing quarantines unchanged
  • tests/test-hydra-gates-bin.sh25/25
  • bash -n run-hydra-gates.sh clean

🤖 Generated with Claude Code

…n an empty scope

Two blockers to enabling the Hydra gates fleet-wide.

1. gate-7 (no-admin-idor) false-positived on ordinary delegation.

It only recognised a guard written in the routed method's own body, so any
controller that centralises authorisation in an injected responder — or routes
thin public actions through private helpers first — went red on correct code.
Measured on decidesk development: 11 findings, ALL 11 guarded. 8 reach
$this->responder->staffAction() -> requireStaff() (currentUid() !== null AND
isStaff()), 3 reach citizenAction() (401 for an anonymous caller), and
validateProposal took three hops (validateProposal -> approveProposal /
rejectProposal -> applyProposalDecision -> staffAction). Zero real exposure.

Pattern 4 resolves this with evidence, not with a naming convention:

  a. Cross-class resolution. The controller's typed constructor-promoted
     properties give $prop -> ClassName; the class is resolved to a real file
     under the app's lib/ tree, confirmed by an actual `class <Name>`
     declaration, and PARSED. A call $this->prop->method( clears the routed
     method only when `method` is demonstrably guard-bearing in the
     collaborator's own source. An unresolvable class clears nothing.
  b. Transitive closure to a fixpoint over same-class calls, so a delegation
     chain of any length is followed.

Propagation uses a STRICTER signal than the existing one-hop Pattern 1: a bare
`throw` and a 404 do not seed a chain. NotFoundException is not an
authorisation guard, and chaining it would let "this can fail" stand in for
"this checks who you are" arbitrarily far up the call graph. Every hop must
also occur before the caller's first data mutation.

Both directions are covered by tests, because a clear that cannot be shown to
still fail is only evidence about itself. 9 new tests: 3 assert the decidesk
shapes now pass, 6 assert the gate still catches a plainly unguarded method, a
collaborator method that EXISTS but is not a guard (respond()), an
unresolvable collaborator type, a chain terminating in nothing, a guard that
runs after the write, and a bare-throw collaborator. 64/64 green.

2. gates 6 and 7 reported PASS when the SCOPE was empty.

Zero files in the diff meant an empty findings log, a count of 0, and a PASS
over nothing inspected — the same failure family as .github#147, where a
missing helper made gate-7 report PASS over 11 real unguarded endpoints. They
now _skip, which keeps them out of _EMITTED_GATES so the coverage summary
lists them under GATES THAT DID NOT RUN. Verified against decidesk with
--base HEAD: "GATES THAT DID NOT RUN: 4 6 7 24 33".

Full suite: 17 helper suites pass, 2 pre-existing quarantines unchanged,
25/25 bin entry-point tests pass.
@rubenvdlinde
rubenvdlinde merged commit fc70ea6 into main Aug 4, 2026
7 checks passed
This was referenced Aug 4, 2026
rubenvdlinde added a commit to ConductionNL/hrmq that referenced this pull request Aug 5, 2026
…v1.3.0 (#72)

This repo left `enable-hydra-gates` at its default `false` on purpose,
with a stated precondition written into the workflow:

  gate-7 (no-admin-idor) currently false-positives on ordinary responder
  delegation — that is being fixed separately in ConductionNL/.github#149.
  Revisit once #149 lands.

#149 has landed (`fc70ea6`) and is in the pin below, so the deferral is
spent. Enabling it now.

The pin moves in the SAME commit, and that is load-bearing. The shared
quality.yml now defaults `hydra-gates-require-full-coverage` to true
(#164), and that flag requires a gate to DECLARE itself not-applicable.
v1.0.1 contains ZERO `_skip` calls; v1.3.0 has 36. Enabling the gates on
the fleet's old pin would land this repo red on arrival for gates it has
no subject matter for — precisely the "red for a reason that is not
hrmq's" the original comment was protecting against.

Measured before enabling, in a private mount namespace with a private
tmpfs, diff-scoped exactly as CI scopes it:

  exit 0 — ALL 58 APPLICABLE GATES PASSED, and all 58 of them ran.
  NOT APPLICABLE: 4 6 7 24 33, each naming itself and its reason.

Whole-tree debt is 10 gates and is written into the workflow comment
rather than discovered later. Two findings triaged by hand:

  gate-8, 13 findings, ALL FALSE POSITIVE. The gate flags
  `catch (\Throwable) { return null; }` in authorize* methods without
  reading what the CALLER does with the null. Every caller here is
  fail-CLOSED (`if ($x === null) return 404`). The null is deliberate:
  "does not exist" and "RBAC denied" collapse to one 404 so the endpoint
  is not an existence oracle. That is the OPPOSITE of the decidesk#45
  defect gate-8 targets. Reported upstream, not suppressed.

  gate-4 is REAL: this repo ships composer.json but no composer.lock, so
  `composer audit --locked` cannot run at all and its dependencies have
  never been CVE-audited. Filed separately.

No waiver, no continue-on-error, no require-full-coverage override.

Refs ConductionNL/.github#159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant