fix(gate-7): a (string) cast blinded Pattern 6 — the guard was unchanged, only its spelling (#414) - #427
Merged
Conversation
…ged, only its spelling (#414) Pattern 6's identity recogniser applies control 2 — "the expression may carry no call ARGUMENTS" — with `\(\s*[^)\s]`. A PHP cast is written with exactly those bytes, so `(string)$user->getUID()` read as a call taking an argument and stopped being an identity. The GUARD did not change; its SPELLING did. REPRODUCED FIRST, on the shipped openbuild `AppOverrideController` with one token varying and nothing else: A uid: (string)$user->getUID() FAIL <- shipped B uid: $user->getUID() PASS C $uid = (string)$user->getUID(); … uid: $uid FAIL D appId: (string)$appId, uid: $user->getUID() PASS E $uid = $user->getUID(); … uid: $uid PASS D rules out "any cast in the call defeats it" — an unrelated cast is harmless. C rules out "hoist it into a local" as the workaround: the cast defeats the recogniser through `_session_identity_names()` too, because that classifies the assignment's right-hand side with the same predicate. WHY THIS CLASS MATTERS MORE THAN ITS COUNT. It is a FALSE POSITIVE THAT RECOMMENDS THE WRONG REPAIR. gate-7's FAIL text says "the fix is to scope the object to the caller" — and the object already was: the UID is never a request parameter. Following the guidance means adding a redundant guard to an endpoint that had one, and then believing the gate about it. `IUser::getUID()` carries no PHP return type (only `@return string`), so Psalm and PHPStan actively encourage the cast: the gate and the analysers pulled in opposite directions on one line. THE FIX. A leading `(string)` / `(int)` / `(integer)` cast is normalised away before classification. `(array)` and `(object)` are deliberately absent — they would launder a payload into an identity. Both surviving controls run AFTER the strip, so nothing widens: `(string)canAccess($id, $uid)` still has a real argument list and `(string)$row['ownerId']` still has a subscript.⚠️ THE STRIP IS ALSO APPLIED IN `_argument_is_session_identity`, BEFORE the bare-variable test — not only inside `_is_identity_expression`. Doing it in one place only would have routed `(string)$targetUid` past the declared-parameter veto (`_IDENTITY_TOKEN_RE` matches "uid"), turning a false positive into a false NEGATIVE, which is the direction that leaves no log to notice. 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) Zero delta is the honest number and it is not zero effect: openbuild#196 had already dropped the cast in-app, so the only live site was gone before this ran. The two-directional control is the byte-exact pre-#196 file: WITHOUT this change it reports `method=getUser rule=no-auth-guard-in-body` (7 findings), WITH it it does not (6) — matching openbuild#196's own recorded measurement of 7 -> 6. Same file, same checker, one token. ACCEPTANCE ARMS, and which are evidence authn-vs-authz/clean `handoffCastIdentity()` / `handoffCastLocal()` — EVIDENCE. Reverting the checker and re-running: clean arm FAILS with "2 method(s) with NoAdminRequired + no guard" (130 passed / 4 failed). With the fix: 134 / 0. authn-vs-authz/planted `castCallerValue()` — CONTROL. A cast on a value the CALLER chose. Reports before AND after, so "strip every cast and classify what is left" cannot pass this bundle. The three `#365` planted arms and the four original clean shapes — CONTROLS, unchanged in both directions. 145 unit tests pass, seven of them new. `test_gate_orphaned_capability_ fixtures.sh` fails identically on pristine main (missing fixture dirs). Closes #414
…linds-pattern-6 # Conflicts: # hydra-gates/scripts/lib/test_check_no_admin_idor.py
Contributor
Author
|
Merged #426 and #427 each merged cleanly onto The conflict is purely additive, so the resolution is union, not a choice. Verified on the combined tree before pushing:
Both checkers compile. Re-running CI on the merged head now. |
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 #414.
Reproduced first — five arms on the shipped openbuild file, one token varying
uid: (string)$user->getUID()uid: $user->getUID()$uid = (string)$user->getUID();…uid: $uidappId: (string)$appId, uid: $user->getUID()$uid = $user->getUID();…uid: $uidD rules out "any cast in the call defeats it" — an unrelated cast is harmless. C rules out "hoist it into a local" as the workaround.
Mechanism
Pattern 6's identity recogniser applies control 2 — "the expression may carry no call ARGUMENTS" — with
\(\s*[^)\s]. A PHP cast is written with exactly those bytes:So the guard did not change; only its spelling did. And the cast is not an odd spelling:
IUser::getUID()carries no PHP return type (only@return string), so Psalm and PHPStan actively encourage writing it. The gate and the analysers pulled in opposite directions on the same line.This is a false positive whose recommended repair is wrong: gate-7's FAIL text says "the fix is to scope the object to the caller" and the object already was — the UID is never a request parameter. Following the guidance means adding a redundant guard to an endpoint that had one, and then believing the gate about it.
The fix
A leading
(string)/(int)/(integer)cast is normalised away before classification.(array)and(object)are deliberately absent — they would launder a payload into an identity. Both surviving controls run after the strip, so nothing widens:(string)canAccess($id, $uid)still has a real argument list,(string)$row['ownerId']still has a subscript._argument_is_session_identityas well, before the bare-variable test — not only inside_is_identity_expression. Doing it in one place only would have routed(string)$targetUidpast the declared-parameter veto (_IDENTITY_TOKEN_REmatches "uid"), turning a false positive into a false negative. That is the direction that leaves no log to notice.Measured before/after — gate-7 on
lib/Controller, all 18 core appsZero delta is the honest number, and it is not zero effect: ConductionNL/openbuild#196 had already dropped the cast in-app, so the only live site was gone before this ran.
Two-directional control on the byte-exact pre-#196 file: WITHOUT this change it reports
method=getUser rule=no-auth-guard-in-body(7 findings); WITH it, it does not (6). That matches openbuild#196's own recorded measurement of 7 → 6 exactly. Same file, same checker, one token.Revert control — which arms are evidence
Suite re-run with the fixtures kept and the checker reverted:
authn-vs-authz/cleanhandoffCastIdentity()+handoffCastLocal(): the clean arm FAILS with2 method(s) with NoAdminRequired + no guard→ 130 passed / 4 failed. With the fix: 134 / 0. (A false-positive fix shows up as a clean arm that stops failing.)authn-vs-authz/plantedcastCallerValue(): a cast on a value the caller chose. Reports before and after, so "strip every cast and classify what is left" cannot pass this bundle.#365planted arms and the four original clean shapes: unchanged in both directions.test_check_no_admin_idor.py: 145 pass, seven of them new (includingtest_cast_on_a_caller_supplied_value_is_still_reported,test_cast_does_not_make_object_data_an_identity,test_array_and_object_casts_are_not_stripped).test_gate_orphaned_capability_fixtures.shfails identically on pristinemain(missing fixture dirs) — pre-existing, not this branch.🤖 Generated with Claude Code