fix(gate-7): Pattern 8 is keyed on the signature, so a SOAP endpoint is invisible to it (#413) - #426
Merged
Merged
Conversation
…is invisible to it (#413) Pattern 8 decides `#[PublicPage]` scope by asking whether a caller-supplied SCALAR reaches a selection call, and every source of such a scalar it knows is either a declared parameter or a `->getParam()`-family read. That is right for REST. It has no opinion at all about the shape where the method takes NO PARAMETERS and the selector arrives inside the request body. REPRODUCED FIRST — three arms in one file, varying only the selector's SOURCE: armVisible(string $id) -> $this->dispatcher->find($id) 1 finding armRawBody() -> ->dispatch(rawBody: file_get_contents( 'php://input'), service: 'zaken') 0 findings armRawBodyVerified() -> ->dispatchVerified(rawBody: …) 0 findings The middle arm is the defect: the same delegate, the same unscoped resolution, invisible. The live instance is procest `StufController::{zaken,personen}` — `#[PublicPage]` + `#[NoCSRFRequired]`, reaching a responder that dispatches zakLk01, zakLv01, npsLv01 (person query by BSN) and edcLk01 with no authentication of any kind. gate-7 reported ZERO findings on that app's 79 public methods. The silence was right 77 times and wrong twice, which is exactly the ratio that makes a silence persuasive. WHAT CHANGES — SCOPE ONLY, NO NEW CLEAR Pattern 8b puts a `#[PublicPage]` method in scope when it reads the raw request body and hands a value derived from it to a `->`/`::` call. Everything that already clears then applies unchanged: the 401/403 body guard, Pattern 7's ownership comparison, Pattern 1's same-class guard helper and Pattern 4's RESOLVED collaborator guard. All five fleet apps that get this right are cleared by machinery that was already here — `checkSignature()`/`verifyWsse()` by the Pattern 1/4 name rules, `validateSignature()` by its inline 401, and procest's repaired `dispatch()` by Pattern 4 reading `authenticateSender()`'s STATUS_UNAUTHORIZED out of the collaborator's own file. One credential clear IS added, and it is Pattern 8's own: the scalar half already holds that "a $token / $secret IS the authorisation", so a webhook signature travelling with the bytes it authenticates clears the same way. It was added because it was MEASURED to be needed — see below. `IRequest::getContent()` is also added to `_REQUEST_INPUT_RE`, which already carried `php://input`. Without it Pattern 3b cleared the same method as a "zero-input READ" one branch after Pattern 8b had put it in scope; two clauses disagreeing about whether a line is caller input is how a widening buys nothing. It is anchored on the request receiver, so `$file->getContent()` is untouched. MEASURED, gate-7 on lib/Controller across all 18 core apps decidesk 15->15 docudesk 23->23 doriath 4->4 hermiq 17->17 larpingapp 1->1 launchpad 1->1 nldesign 0->0 openbuild 6->6 opencatalogi 14->14 openconnector 80->80 openregister 95->95 pipelinq 50->50 portaliq 1->1 procest 27->27 scholiq 4->4 shillinq 18->18 softwarecatalog 30->30 zaakafhandelapp 89->89 TOTAL 475 -> 475 (+0) The first draft, without the credential clear, produced exactly TWO new findings across those 18 apps and both were false positives of the same shape: pipelinq's BRP and CTI webhooks, each reading an `X-…-Signature` header and forwarding it to the verifier. Neither was reachable by any existing clear (the CTI one refuses on `$result['valid'] === false`, a bare field comparison `_public_refuses_on_scope_predicate` deliberately does not read), so their only remedy would have been an exempt tag. Characterised, then closed properly. TWO-DIRECTIONAL CONTROL ON THE REAL REPO. procest with `authenticateSender()` removed — the state it shipped before ConductionNL/procest#828 — reports 27 findings without this change and 29 with it, the two extra being `zaken` and `personen`. procest as it ships TODAY reports 27 either way: the fix does not punish the repair. Same file, same checker, one hop of difference. ACCEPTANCE ARMS, and which are evidence publicpage-scope/planted `rawBodyDispatch()` — EVIDENCE. Reverting the checker and re-running: "gate-7 failed but 'method=rawBodyDispatch rule=publicpage-unscoped-object-lookup' is not in the log — it failed for some OTHER reason". With the fix: PASS. publicpage-scope/clean `rawBodyDispatch()` — CONTROL. Byte-identical apart from `EnvelopeService::authenticateSender()`. Silent before AND after, so "flag every public method that reads the body" is not a passing repair. publicpage-scope/planted `arbitraryId()` — CONTROL, unchanged. Suite: 137 passed / 0 failed with the fix; 134/0 on this branch alone (the other 3 belong to #414). 148 unit tests pass. `test_gate_orphaned_capability_ fixtures.sh` fails identically on pristine main (missing fixture dirs). Closes #413
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #413.
Reproduced first
Three arms in one file, varying only the selector's source:
armVisible(string $id)->find($id)armRawBody()->dispatch(rawBody: file_get_contents('php://input'), service: 'zaken')armRawBodyVerified()The middle arm is the defect: same delegate, same unscoped resolution, invisible. Pattern 8 decides scope by asking whether a caller-supplied scalar reaches a selection call, and every source of such a scalar it knows about is a declared parameter or a
->getParam()-family read. It has no opinion at all about a method that takes no parameters and carries its selector in the body.The live instance is procest
StufController::{zaken,personen}—#[PublicPage]+#[NoCSRFRequired], reaching a responder that dispatcheszakLk01,zakLv01,npsLv01(person query by BSN) andedcLk01with no authentication of any kind. gate-7 reported zero findings on that app's 79 public methods. Right 77 times, wrong twice — the ratio that makes a silence persuasive.What changes — scope only, no new clear
Pattern 8b puts a
#[PublicPage]method in scope when it reads the raw request body and hands a derived value to a->/::call. Everything that already clears then applies unchanged (401/403 body guard, Pattern 7 ownership, Pattern 1 same-class helper, Pattern 4 resolved collaborator guard). Every fleet app that gets this right is cleared by machinery that was already here —checkSignature()/verifyWsse()by the Pattern 1/4 name rules,validateSignature()by its inline 401, and procest's repaireddispatch()by Pattern 4 readingauthenticateSender()'sSTATUS_UNAUTHORIZEDout of the collaborator's own file.One clear is added and it is Pattern 8's own: the scalar half already holds that "a
$token/$secretis the authorisation", so a webhook signature travelling with the bytes it authenticates clears the same way. It was added because it was measured to be needed — see below.IRequest::getContent()is added to_REQUEST_INPUT_RE, which already carriedphp://input. Without it Pattern 3b cleared the same method as a "zero-input READ" one branch after Pattern 8b had put it in scope. Anchored on the request receiver, so$file->getContent()is untouched.Measured before/after — gate-7 on
lib/Controller, all 18 core appsThe first draft, without the credential clear, produced exactly two new findings across those 18 apps, and both were false positives of one shape: pipelinq's BRP and CTI webhooks, each reading an
X-…-Signatureheader and forwarding it to the verifier. Neither was reachable by any existing clear (the CTI one refuses on$result['valid'] === false, a bare field comparison_public_refuses_on_scope_predicatedeliberately does not read), so their only remedy would have been an exempt tag. Characterised, then closed properly rather than shipped.Residual risk, stated rather than hidden: an app that collects a signature and never verifies it clears here. That is the risk Pattern 8 already accepts for
$token, and it is smaller than the raw-body blindness this closes.Two-directional control on the real repo
procest with
authenticateSender()removed — the state it shipped before ConductionNL/procest#828 — reports 27 findings without this change and 29 with it, the two extra beingmethod=zakenandmethod=personen. procest as it ships today reports 27 either way: the fix does not punish the repair. Same file, same checker, one hop of difference.Revert control — which arms are evidence
Suite re-run with the fixtures kept and the checker reverted:
publicpage-scope/plantedrawBodyDispatch():FAIL — gate-7 failed but 'method=rawBodyDispatch rule=publicpage-unscoped-object-lookup' is not in hydra-gate-no-admin-idor.log — it failed for some OTHER reason. With the fix: PASS.publicpage-scope/cleanrawBodyDispatch(): byte-identical apart fromEnvelopeService::authenticateSender(). Silent before and after, so "flag every public method that reads the body" is not a passing repair.publicpage-scope/plantedarbitraryId(): unchanged in both directions.test_gate_acceptance_matrix.sh: 134 passed / 0 failed on this branch (137/0 with #414 also applied).test_check_no_admin_idor.py: 148 pass.test_gate_orphaned_capability_fixtures.shfails identically on pristinemain(missing fixture dirs) — pre-existing, not this branch.🤖 Generated with Claude Code