fix(gate-7): a (string) cast blinded Pattern 6 — the guard was unchanged, only its spelling (#414)

hydra-gates/scripts/lib/check_no_admin_idor.py

@@ -744,15 +744,56 @@ 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.
 
     Control 2 of the three listed above: the expression may carry no call
     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:

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 (
+        "<?php\nclass AppOverrideController {\n"
+        "    #[NoAdminRequired]\n"
+        "    public function getUser(%s): JSONResponse\n    {\n" % sig
+        + body + "    }\n}\n"
+    )
+
+
+class CastOnSessionIdentityTest(unittest.TestCase):
+    """`ConductionNL/.github#414` — a `(string)` cast blinded Pattern 6.
+
+    Control 2 of `_is_identity_expression` rejects "any call with arguments"
+    with `\\(\\s*[^)\\s]`, and a cast is written with exactly those bytes. So
+    the GUARD was unchanged and only its SPELLING moved the verdict — a false
+    positive whose recommended repair (add a guard) was wrong, on an endpoint
+    that already had one.
+    """
+
+    _CAST_ARG = (
+        "        $user = $this->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."""

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'],
 	],
 ];

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) {

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

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'],
 	],
 ];

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.