diff --git a/hydra-gates/scripts/lib/check_no_admin_idor.py b/hydra-gates/scripts/lib/check_no_admin_idor.py index c6e09222..29131d93 100644 --- a/hydra-gates/scripts/lib/check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/check_no_admin_idor.py @@ -744,6 +744,44 @@ def _top_level_split(text: str, sep: str) -> list: return [p.strip() for p in parts] +# A LEADING SCALAR CAST, stripped before the expression is classified +# (`ConductionNL/.github#414`). +# +# πŸ”΄ THE SPELLING MOVED THE VERDICT, AND THE GUARD DID NOT. Control 2 below +# rejects "any call with arguments" with `\(\s*[^)\s]` β€” and a cast is written +# with exactly those bytes, so `(string)$user->getUID()` read as a call taking +# an argument and stopped being an identity. Measured on openbuild's +# `AppOverrideController::getUser()`, a method whose uid is never a request +# parameter, on one file with one token varying: +# +# uid: (string)$user->getUID() FAIL +# uid: $user->getUID() PASS +# $uid = (string)$user->getUID(); … FAIL <- through a local, too +# appId: (string)$appId, uid: $user… PASS <- an unrelated cast is fine +# +# That is a FALSE POSITIVE WHOSE RECOMMENDED REPAIR IS WRONG: gate-7's FAIL +# text says "scope the object to the caller", and the object already was. And +# the cast is not a quirk β€” `IUser::getUID()` carries no PHP return type, only +# `@return string`, so Psalm/PHPStan actively encourage writing it. +# +# ⚠️ WHAT IS *NOT* STRIPPED, and why the list is short. `(array)` and +# `(object)` would launder a payload into an identity, so they are absent. The +# two surviving controls do the rest of the work AFTER the strip, which is what +# keeps this from widening anything: `(string)canAccess($id, $uid)` still has a +# real argument list and `(string)$account['ownerId']` still has a subscript, +# so both stay non-identities. +_LEADING_SCALAR_CAST_RE = re.compile(r"^\(\s*(?:string|int|integer)\s*\)\s*") + + +def _strip_leading_scalar_casts(expr: str) -> str: + """Remove leading ``(string)`` / ``(int)`` casts β€” see `#414` above.""" + while True: + stripped = _LEADING_SCALAR_CAST_RE.sub("", expr, count=1) + if stripped == expr: + return expr + expr = stripped + + def _is_identity_expression(expr: str, context: str = "") -> bool: """True when *expr* reads as "the caller's identity" and nothing else. @@ -751,8 +789,11 @@ def _is_identity_expression(expr: str, context: str = "") -> bool: ARGUMENTS and no array subscript, so a guard call (``canAccess($id, $uid)``) and object data (``$account['ownerId']``) can never be read as an identity however their names are spelled. + + A leading scalar cast is normalised away first (`#414`) β€” it is a SPELLING + of the same expression, and control 2 could not tell it from a call. """ - expr = expr.strip() + expr = _strip_leading_scalar_casts(expr.strip()) if expr == "": return False if expr[0] in ("'", '"'): @@ -1075,6 +1116,15 @@ def _argument_is_session_identity(arg: str, declared: set, session: set) -> bool named = re.match(r"^[A-Za-z_][A-Za-z0-9_]*\s*:\s*(?!:)(.*)$", arg, re.S) if named is not None: arg = named.group(1).strip() + # `#414`: normalise the cast HERE TOO, and before the bare-variable test β€” + # not only inside `_is_identity_expression`. `(string)$otherUid` must still + # reach the `in declared` branch below and be refused as caller-supplied; + # stripping the cast only in `_is_identity_expression` would have routed it + # past that check and let a caller-chosen parameter whose NAME happens to + # contain "uid" pass for a session identity. That is the direction this + # gate must never move in, so the strip is applied where the declared-name + # veto can still see it. + arg = _strip_leading_scalar_casts(arg) m = re.fullmatch(r"\$([A-Za-z_][A-Za-z0-9_]*)", arg) if m is not None: if m.group(1) in session: diff --git a/hydra-gates/scripts/lib/test_check_no_admin_idor.py b/hydra-gates/scripts/lib/test_check_no_admin_idor.py index 2b17beac..8aed99a2 100644 --- a/hydra-gates/scripts/lib/test_check_no_admin_idor.py +++ b/hydra-gates/scripts/lib/test_check_no_admin_idor.py @@ -2756,6 +2756,95 @@ def test_raw_body_with_an_inline_401_clears(self): self.assertEqual(out, [], out) +def _cast_arm(body: str, sig: str = "string $appId") -> str: + """One `#[NoAdminRequired]` method whose body is given verbatim. + + Unlike `_method()` above, nothing is appended: every `#414` arm differs + from its neighbours by ONE TOKEN, so the body has to be the whole variable. + """ + return ( + "userSession->getUser();\n" + " return new JSONResponse($this->svc->getUserDelta(" + "appId: $appId, uid: (string)$user->getUID()));\n" + ) + _PLAIN_ARG = ( + " $user = $this->userSession->getUser();\n" + " return new JSONResponse($this->svc->getUserDelta(" + "appId: $appId, uid: $user->getUID()));\n" + ) + _CAST_LOCAL = ( + " $user = $this->userSession->getUser();\n" + " $uid = (string)$user->getUID();\n" + " return new JSONResponse($this->svc->getUserDelta(" + "appId: $appId, uid: $uid));\n" + ) + + def test_cast_on_the_identity_argument_still_clears(self): + """Arm A β€” the SHIPPED openbuild spelling. FAIL before `#414`.""" + self.assertEqual(_scan(_cast_arm(self._CAST_ARG)), []) + + def test_uncast_arm_is_the_control(self): + """Arm B. It passed before `#414` and after β€” it is what made the + one-token difference visible in the first place.""" + self.assertEqual(_scan(_cast_arm(self._PLAIN_ARG)), []) + + def test_cast_through_a_local_clears_too(self): + """Arm C, the one whose failure was NOT predicted: hoisting into a + variable is the obvious workaround and it did not work, because the + assignment's right-hand side is classified by the same predicate. A fix + that normalises only the argument position leaves this one red.""" + self.assertEqual(_scan(_cast_arm(self._CAST_LOCAL)), []) + + def test_cast_on_a_caller_supplied_value_is_still_reported(self): + """πŸ”‘ THE ABUSE CONTROL. Stripping the cast must not route a value the + CALLER chose past the declared-parameter veto just because its name + contains "uid". This must stay a finding, or `#414` would have turned a + false positive into a false NEGATIVE β€” the direction that leaves no log + to notice.""" + out = _scan(_cast_arm( + " return new JSONResponse($this->svc->getUserDelta(" + "appId: $appId, uid: (string)$targetUid));\n", + sig="string $appId, string $targetUid")) + self.assertEqual(len(out), 1, out) + + def test_cast_does_not_make_object_data_an_identity(self): + """The subscript control survives the strip: `(string)$row['ownerId']` + is the server's data about an object the caller named, not the caller.""" + self.assertFalse( + cni._is_identity_expression("(string)$row['ownerId']")) + + def test_cast_does_not_make_a_predicate_call_an_identity(self): + """And so does the argument control: after the cast is removed there is + still a real argument list.""" + self.assertFalse( + cni._is_identity_expression("(string)canAccess($id, $uid)")) + + def test_array_and_object_casts_are_not_stripped(self): + """`(array)` / `(object)` would launder a payload into an identity, so + they are deliberately absent from the cast list.""" + self.assertEqual( + cni._strip_leading_scalar_casts("(array)$user->getUID()"), + "(array)$user->getUID()") + + class HelperGuardEvidenceTest(unittest.TestCase): """A gate that can be silenced by a sentence in a comment is not measuring the code.""" diff --git a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/routes.php b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/routes.php index a055bce2..290426c7 100644 --- a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/routes.php +++ b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/routes.php @@ -10,5 +10,7 @@ ['name' => 'ledger#tenancy404', 'url' => '/api/entries/{entryId}/tenancy', 'verb' => 'GET'], ['name' => 'ledger#collaborator', 'url' => '/api/entries/{entryId}/collaborator', 'verb' => 'GET'], ['name' => 'ledger#handoff', 'url' => '/api/entries/{entryId}/handoff', 'verb' => 'GET'], + ['name' => 'ledger#handoffCastIdentity', 'url' => '/api/entries/{entryId}/cast', 'verb' => 'GET'], + ['name' => 'ledger#handoffCastLocal', 'url' => '/api/entries/{entryId}/castlocal', 'verb' => 'GET'], ], ]; diff --git a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Controller/LedgerController.php b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Controller/LedgerController.php index 3832f2e7..c388974c 100644 --- a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Controller/LedgerController.php +++ b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Controller/LedgerController.php @@ -114,6 +114,58 @@ public function handoff(string $entryId): JSONResponse { return new JSONResponse($this->ledger->findOwned(entryId: $entryId, userId: $userId)); } + /** + * Shape 5 β€” THE SAME HAND-OFF, SPELLED WITH A CAST + * (`ConductionNL/.github#414`). + * + * `handoff()` above and this method are the same guard. The only + * difference is `(string)` on the identity, and until `#414` that one + * token moved the verdict: Pattern 6's identity recogniser rejects "any + * call with arguments" with `\(\s*[^)\s]`, and a cast is written with + * exactly those bytes. + * + * IT BELONGS IN THE CLEAN ARM BECAUSE IT WAS A FALSE POSITIVE WHOSE + * RECOMMENDED REPAIR WAS WRONG. gate-7's FAIL text says "scope the object + * to the caller"; the object already was. Following the guidance means + * adding a redundant guard to an endpoint that already had one β€” and then + * believing the gate about it. + * + * The cast is not an odd spelling either: `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. + */ + #[NoAdminRequired] + public function handoffCastIdentity(string $entryId): JSONResponse { + $user = $this->userSession->getUser(); + if ($user === null) { + return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse( + $this->ledger->findOwned(entryId: $entryId, userId: (string)$user->getUID()) + ); + } + + /** + * Shape 6 β€” the same cast, hoisted into a LOCAL. + * + * This arm exists because it was the one whose failure was not predicted. + * "Hoist it into a variable" is the obvious workaround for shape 5, and it + * did not work: the cast defeated the recogniser through the local trace + * too, because `_session_identity_names()` classifies the assignment's + * right-hand side with the same predicate. A fix that normalises only the + * argument position passes shape 5 and still fails here. + */ + #[NoAdminRequired] + public function handoffCastLocal(string $entryId): JSONResponse { + $user = $this->userSession->getUser(); + if ($user === null) { + return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED); + } + $uid = (string)$user->getUID(); + return new JSONResponse($this->ledger->findOwned(entryId: $entryId, userId: $uid)); + } + private function sessionUserId(): ?string { $user = $this->userSession->getUser(); if ($user === null) { diff --git a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/expect.conf b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/expect.conf index 15777ddf..6bbc91c7 100644 --- a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/expect.conf +++ b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/expect.conf @@ -44,3 +44,18 @@ gate 7 hydra-gate-no-admin-idor.log FAIL PASS method=bare rule=no-auth-guard-in-body gate 7 hydra-gate-no-admin-idor.log FAIL PASS method=preamble rule=no-auth-guard-in-body gate 7 hydra-gate-no-admin-idor.log FAIL PASS method=preambleForbiddenCode rule=no-auth-guard-in-body +# +# `ConductionNL/.github#414` β€” A `(string)` CAST ON THE SESSION IDENTITY. +# +# The clean arm gained `handoffCastIdentity()` and `handoffCastLocal()`: the +# same Pattern 6 hand-off `handoff()` already proves, with one token added. +# Before `#414` both were FINDINGS, so the CLEAN column below is where that +# defect shows up β€” a false positive is a clean arm that fails. +# +# `castCallerValue()` is the planted counterpart and it is the load-bearing +# half. Normalising the cast away must not route a caller-supplied `$targetUid` +# past the declared-parameter veto just because `_IDENTITY_TOKEN_RE` matches +# "uid". Without this row, "strip every cast and classify what is left" would +# pass the whole bundle while silencing every endpoint that takes another +# user's id as a parameter. +gate 7 hydra-gate-no-admin-idor.log FAIL PASS method=castCallerValue rule=no-auth-guard-in-body diff --git a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/routes.php b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/routes.php index 3ba7ac66..7268ca05 100644 --- a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/routes.php +++ b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/routes.php @@ -9,6 +9,7 @@ ['name' => 'ledger#bare', 'url' => '/api/entries/{entryId}/bare', 'verb' => 'GET'], ['name' => 'ledger#preamble', 'url' => '/api/entries/{entryId}/preamble', 'verb' => 'GET'], ['name' => 'ledger#preambleForbiddenCode', 'url' => '/api/entries/{entryId}/forbidden', 'verb' => 'GET'], + ['name' => 'ledger#castCallerValue', 'url' => '/api/entries/{entryId}/cast/{targetUid}', 'verb' => 'GET'], ['name' => 'ledger#readAsOwner', 'url' => '/api/entries/{entryId}', 'verb' => 'GET'], ], ]; diff --git a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/lib/Controller/LedgerController.php b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/lib/Controller/LedgerController.php index a10efd4e..f3b4ce15 100644 --- a/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/lib/Controller/LedgerController.php +++ b/hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/lib/Controller/LedgerController.php @@ -91,6 +91,31 @@ public function preambleForbiddenCode(string $entryId): JSONResponse { return new JSONResponse($entry); } + /** + * ARM 4 β€” THE ABUSE CONTROL FOR `ConductionNL/.github#414`. + * + * `#414` normalises a leading `(string)` cast away before an expression is + * classified as the caller's identity, so that the clean arm's + * `handoffCastIdentity()` stops being a false positive. This method is the + * shape that normalisation must NOT clear: the cast sits on a value the + * CALLER chose, whose name merely happens to contain "uid". + * + * Without it, "strip the cast, then classify" would route `(string)$targetUid` + * past the declared-parameter veto β€” `_IDENTITY_TOKEN_RE` matches "uid" β€” + * and every endpoint that takes someone else's user id as a parameter would + * go silent. That is a false NEGATIVE on a security gate, which leaves no + * log to notice. + * + * No session value reaches the lookup here. It must report in both + * directions, before and after `#414`. + */ + #[NoAdminRequired] + public function castCallerValue(string $entryId, string $targetUid): JSONResponse { + return new JSONResponse( + $this->ledger->findOwned(entryId: $entryId, userId: (string)$targetUid) + ); + } + /** * NOT planted β€” a real per-object ownership check. Keeps the planted arm * from being uniformly guilty.