diff --git a/hydra-gates/scripts/lib/check_semantic_auth.py b/hydra-gates/scripts/lib/check_semantic_auth.py index c15748fa..cec34037 100644 --- a/hydra-gates/scripts/lib/check_semantic_auth.py +++ b/hydra-gates/scripts/lib/check_semantic_auth.py @@ -24,7 +24,10 @@ - ``$this->requireAdmin()`` or bare ``requireAdmin()`` - ``if (... !isAdmin ...) { ... STATUS_FORBIDDEN | OCSForbidden | 403 ...}`` - ``if (... isAdmin === false ...) { ... STATUS_FORBIDDEN | OCSForbidden | 403 ...}`` - - PublicPage + ``Http::STATUS_UNAUTHORIZED|FORBIDDEN`` in body + - PublicPage + ``Http::STATUS_UNAUTHORIZED|FORBIDDEN`` in a body that + does not itself resolve a credential from the request. See + ``_SELF_AUTH_RE``; the exemption is matched against the + comment-stripped source, so only code can earn it. Prints one line per violation in the same format as the bash gate so ``run-hydra-gates.sh`` can consume it unchanged. @@ -46,6 +49,71 @@ ) +def _strip_comments(src: str) -> str: + """Replace comments with same-length whitespace, KEEPING string literals. + + Used to decide whether a method authenticates its own credential. That + question is about executable code, and a docblock is not executable: a + comment reading "callers must present a bearer token" would otherwise + buy the self-auth exemption for a method containing no such check — the + 2026-08-06 gate-64 failure mode, where a commented-out call counted as + a real one. + + String literals are deliberately preserved, unlike in + :func:`_strip_strings_and_comments`. Several of the idioms in + ``_SELF_AUTH_RE`` live inside literals by nature — the ``'Bearer '`` + prefix a handler strips, the ``'HTTP_AUTHORIZATION'`` key, the header + name passed to ``getHeader()``. Blanking those would turn correct code + into findings, which is the failure this gate already exists to avoid. + + Offsets are preserved so the result can be searched interchangeably with + the raw slice. + """ + out = [] + i = 0 + n = len(src) + while i < n: + c = src[i] + # Single-line comment // ... \n + if c == "/" and i + 1 < n and src[i + 1] == "/": + j = src.find("\n", i) + if j == -1: + j = n + out.append(" " * (j - i)) + i = j + continue + # Block comment / docblock /* ... */ + if c == "/" and i + 1 < n and src[i + 1] == "*": + j = src.find("*/", i + 2) + if j == -1: + j = n + else: + j += 2 + out.append(" " * (j - i)) + i = j + continue + # A quoted string is copied through verbatim, but must still be + # consumed here so that a `//` or `/*` inside it (a URL, a regex) + # is not mistaken for the start of a comment. + if c in ("'", '"'): + quote = c + j = i + 1 + while j < n: + if src[j] == "\\" and j + 1 < n: + j += 2 + continue + if src[j] == quote: + j += 1 + break + j += 1 + out.append(src[i:j]) + i = j + continue + out.append(c) + i += 1 + return "".join(out) + + def _strip_strings_and_comments(src: str) -> str: """Replace string literals + comments with same-length whitespace. @@ -235,6 +303,16 @@ def _find_method_bodies(src: str): # breaks the endpoint (middleware rejects the caller before the controller # runs), the second removes its only authentication. A gate that can be # closed only by weakening the code is worse than no gate. +# +# A username/password pair is the same idiom with a different credential +# shape, and the token-only list above missed it: a login endpoint is +# #[PublicPage] by necessity (the caller has no session yet — that is what +# it is asking for) and answers 401 when the password is wrong. Observed on +# openconnector UserController::login (2026-08-07), which resolves +# `$username`/`$password` from the request and calls +# `$this->userManager->checkPassword(...)` — the credential IS named, in the +# body, and the gate still reported it unsourced. Same class of finding as +# the 36 above, so it is listed here rather than argued with in the app. _SELF_AUTH_RE = re.compile( r"hash_hmac\s*\(|" r"hash_equals\s*\(|" @@ -245,10 +323,23 @@ def _find_method_bodies(src: str): r"\bhmac\b|" r"[Tt]oken\s*\)|" r"\$\w*[Tt]oken\b|" - r"->\s*(resolve|validate|verify|find|get)\w*[Tt]oken\s*\(|" + # Any call whose NAME carries the credential, not just the six verbs + # that used to be listed. hermiq EgressAuthorizeController::authorize + # (2026-08-07) resolves its credential with `$this->bearerToken()` and + # was matching only on the word "bearer" in its own docblock — so it + # went from exempt to flagged the moment comments stopped counting, + # despite being textbook-correct code. + r"->\s*\w*[Tt]oken\w*\s*\(|" + # PHP named argument: `verify(token: ...)`, `assert(apiKey: ...)`. + r"\b(token|apiKey|credential|password|signature)\s*:\s*|" r"->\s*subject\s*\(|" - r"->\s*findByToken\s*\(|" - r"[Cc]apability\s*[Tt]oken", + r"[Cc]apability\s*[Tt]oken|" + # Username/password credentials presented in the request. + r"password_verify\s*\(|" + r"->\s*checkPassword\s*\(|" + r"\$\w*[Pp]assword\b|" + r"\$\w*[Cc]redentials?\b|" + r"->\s*(resolve|validate|verify|check)\w*[Cc]redentials?\s*\(", re.IGNORECASE, ) @@ -278,9 +369,16 @@ def _has_admin_if_with_throw(body: str) -> bool: # Negated isAdmin or isAdmin === false. Character class must # include `$` (`$this->`), `>` (`->`), and word chars to span # `$this->isAdmin` / `$user->getUID()->isAdmin` etc. + # `cond` is already bounded by the matching close paren of the `if`, + # so `.*?` cannot run past the condition. It must not be `[^)]*`: + # the call being tested usually HAS arguments, and + # `isAdmin($this->userId) === false` puts a `)` between the name and + # the comparison — the same shape of over-restrictive character class + # that made the old `[^}]*` body regex miss real throws (W28). if not ( re.search(r"!\s*[\w\$\->]*isAdmin\b", cond) or - re.search(r"\bisAdmin\b[^)]*===\s*false", cond) + re.search(r"\bisAdmin\b.*?===\s*false", cond, re.DOTALL) or + re.search(r"false\s*===.*?\bisAdmin\b", cond, re.DOTALL) ): continue # Body of the if. @@ -341,7 +439,13 @@ def scan_file(path: str) -> int: f"Do NOT simply delete the check." ) violations += 1 - elif _PUBLIC_DENY_STATUS_RE.search(body) and not _SELF_AUTH_RE.search(head + body): + # Comments stripped before asking "does this authenticate its own + # credential?" — the exemption has to be earned by code, not by a + # docblock describing a check the body does not perform. + elif ( + _PUBLIC_DENY_STATUS_RE.search(body) + and not _SELF_AUTH_RE.search(_strip_comments(head + body)) + ): print( f"{path}:{line_no} method={name} " f"rule=public-page-annotation-with-unsourced-denial — " diff --git a/hydra-gates/scripts/lib/test_check_semantic_auth.py b/hydra-gates/scripts/lib/test_check_semantic_auth.py index e409043e..e1db746e 100644 --- a/hydra-gates/scripts/lib/test_check_semantic_auth.py +++ b/hydra-gates/scripts/lib/test_check_semantic_auth.py @@ -20,6 +20,20 @@ Both ways, in the same class: the self-authenticating shapes must go quiet, and the session-dependent shape must still fire. + +2026-08-07 — three more defects, and the classes guarding them: + +* :class:`AdminGuardsWithArguments` — the `#[NoAdminRequired]` rule matched + `isAdmin` only through `[^)]*`, and `isAdmin($uid) === false` puts a `)` + between the two. It had therefore never matched a real guard: 25 findings + across 10 repos were invisible while the gate reported PASS. +* :class:`ProseDoesNotEarnTheExemption` — the self-auth exemption was + matched against raw source, so a docblock describing a credential check + exempted a method performing none. The gate-64 failure mode. +* :class:`StringLiteralsStillCount` — and the fix for the above must NOT + extend to string literals. `'Bearer '`, `'HTTP_AUTHORIZATION'` and the + header name passed to getHeader() are literals in every real handler. + These two classes are each other's control. """ from __future__ import annotations @@ -234,6 +248,232 @@ def test_fp_a_plain_no_admin_required_method_is_clean(self): self.assertEqual(_scan(php), []) +class AdminGuardsWithArguments(unittest.TestCase): + """`isAdmin()` takes a UID, and the rule could not read past the `)`. + + `\\bisAdmin\\b[^)]*===\\s*false` cannot span `isAdmin($this->userId)`, + which is how the guard is written everywhere. The rule above + (``NoAdminRequiredRuleUnchanged``) passed only because its fixture calls + `requireAdmin()`, matched by a different pattern — so the whole + `if (...isAdmin...) { deny }` branch had never fired on real code. + """ + + def test_tp_is_admin_with_a_uid_argument_fires(self): + php = CLASS % """ + #[NoAdminRequired] + public function trust(): JSONResponse + { + if ($this->groupManager->isAdmin($this->userId) === false) { + return new JSONResponse(['error' => 'admins only'], Http::STATUS_FORBIDDEN); + } + return new JSONResponse($this->service->getTrustConfig()); + } +""" + self.assertEqual(_rules(_scan(php)), + ["no-admin-required-annotation-with-admin-body"]) + + def test_tp_yoda_comparison_fires(self): + php = CLASS % """ + #[NoAdminRequired] + public function purge(): JSONResponse + { + if (false === $this->groupManager->isAdmin($this->userId)) { + throw new OCSForbiddenException('admins only'); + } + return new JSONResponse($this->cache->purgeAll()); + } +""" + self.assertEqual(_rules(_scan(php)), + ["no-admin-required-annotation-with-admin-body"]) + + def test_fp_a_non_admin_predicate_is_not_a_finding(self): + # The guard has to be about being an admin, not merely an `if` that + # returns 403. Loosening the condition matcher must not turn every + # domain check into an attribute mismatch. + php = CLASS % """ + #[NoAdminRequired] + public function publish(int $id): JSONResponse + { + if ($this->publications->isReviewed($id) === false) { + return new JSONResponse(['error' => 'not reviewed yet'], Http::STATUS_FORBIDDEN); + } + return new JSONResponse($this->publications->publish($id)); + } +""" + self.assertEqual(_scan(php), []) + + +class PasswordsAreCredentialsToo(unittest.TestCase): + """A login endpoint is the self-authenticating shape with a password. + + openconnector UserController::login: #[PublicPage] by necessity — the + caller has no session yet, that is what it is asking for — resolving + $username/$password from the request and calling checkPassword() in the + body, reported as an unsourced denial by a token-only pattern list. + """ + + def test_fp_login_is_not_a_finding(self): + php = CLASS % """ + #[NoCSRFRequired] + #[PublicPage] + public function login(): JSONResponse + { + $data = $this->request->getParams(); + $credentials = $this->security->validateLoginCredentials($data); + $username = $credentials['username']; + $password = $credentials['password']; + + $user = $this->userManager->checkPassword($username, $password); + if ($user === false) { + $this->security->recordFailedLoginAttempt($username, $this->clientIp()); + return new JSONResponse(['error' => 'invalid credentials'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse(['uid' => $user->getUID()]); + } +""" + self.assertEqual(_scan(php), []) + + def test_fp_password_protected_share_is_not_a_finding(self): + php = CLASS % """ + #[PublicPage] + public function unlock(string $slug): JSONResponse + { + $folder = $this->folders->findBySlug($slug); + if (password_verify((string) $this->request->getParam('password'), $folder->getPasswordHash()) === false) { + return new JSONResponse(['error' => 'wrong password'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse($this->folders->listing($folder)); + } +""" + self.assertEqual(_scan(php), []) + + def test_fp_a_helper_resolved_token_passed_by_name_is_not_a_finding(self): + # hermiq McpRunController::handle / EgressAuthorizeController::authorize. + # The credential comes from a helper (`bearerToken()`) and is handed + # over as a named argument (`token:`). Neither is one of the verbs the + # pattern list started with — the ONLY thing that used to match was the + # word "bearer" in the method's own comments, so this pair is what + # would silently break if comment-stripping landed on its own. + php = CLASS % """ + #[PublicPage] + #[NoCSRFRequired] + public function handle(): JSONResponse + { + $binding = $this->runTokenService->verify(token: $this->bearerToken()); + if ($binding === null) { + return new JSONResponse(['error' => 'invalid_token'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse($this->mcp->dispatch($binding, $this->readRawBody())); + } +""" + self.assertEqual(_scan(php), []) + + +class ProseDoesNotEarnTheExemption(unittest.TestCase): + """Only executable code counts as authenticating a credential. + + The gate-64 shape: a checker that reads comments will accept a + commented-out call as a real one, and a docblock as a check. + """ + + def test_tp_a_docblock_describing_a_token_check_still_fires(self): + php = CLASS % """ + /** + * Download an export. + * + * Callers must present a signed capability token in the Authorization + * header; it is compared against the stored secret with hash_equals() + * before any payload is returned. + */ + #[PublicPage] + public function download(int $id): JSONResponse + { + if ($this->exports->isPublished($id) === false) { + return new JSONResponse(['error' => 'not available'], Http::STATUS_FORBIDDEN); + } + return new JSONResponse($this->exports->payload($id)); + } +""" + self.assertEqual(_rules(_scan(php)), + ["public-page-annotation-with-unsourced-denial"]) + + def test_tp_a_commented_out_credential_check_still_fires(self): + php = CLASS % """ + #[PublicPage] + public function receive(): JSONResponse + { + // TODO re-enable once the partner rotates their key: + // $presentedToken = (string) $this->request->getHeader('X-Api-Key'); + // if (hash_equals($this->expectedKey(), $presentedToken) === false) { + if ($this->imports->isAcceptingUploads() === false) { + return new JSONResponse(['error' => 'closed'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse([], 202); + } +""" + self.assertEqual(_rules(_scan(php)), + ["public-page-annotation-with-unsourced-denial"]) + + +class StringLiteralsStillCount(unittest.TestCase): + """The control on ProseDoesNotEarnTheExemption. + + Comments go; literals stay. `'Bearer '`, `'HTTP_AUTHORIZATION'` and the + header name handed to getHeader() live in literals in every real + handler, so blanking them would manufacture exactly the false positives + this gate was rewritten to stop. + """ + + def test_fp_a_bearer_prefix_stripped_from_a_literal_is_not_a_finding(self): + php = CLASS % """ + #[PublicPage] + public function ingest(): JSONResponse + { + $header = (string) $this->request->getHeader('Authorization'); + $presented = str_replace('Bearer ', '', $header); + + if ($this->apiKeys->authorize($presented) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse([], 202); + } +""" + self.assertEqual(_scan(php), []) + + def test_fp_the_server_superglobal_header_key_is_not_a_finding(self): + php = CLASS % """ + #[PublicPage] + public function ping(): JSONResponse + { + $presented = $_SERVER['HTTP_AUTHORIZATION'] ?? ''; + if (hash_equals($this->expectedKey(), (string) $presented) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse(['pong' => true]); + } +""" + self.assertEqual(_scan(php), []) + + def test_fp_a_double_slash_inside_a_url_does_not_swallow_the_method(self): + # If `//` in a literal were read as a comment start, everything after + # it — including the credential check — would be blanked and the + # method would be reported as denying on nothing. + php = CLASS % """ + #[PublicPage] + public function callback(): JSONResponse + { + $issuer = 'https://idp.example.org/realms/demo'; + $presented = (string) $this->request->getHeader('Authorization'); + + if ($this->oidc->verifyIdToken($presented, $issuer) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse(['issuer' => $issuer]); + } +""" + self.assertEqual(_scan(php), []) + + class GateIsNotBlind(unittest.TestCase): def test_the_scanner_still_reads_methods_at_all(self): # If `_find_method_bodies` ever returns nothing, every `assertEqual([])`