From e53b063e047fbd48d0c8b442e2a1fa309b4c5e01 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Fri, 7 Aug 2026 08:17:50 +0200 Subject: [PATCH 1/2] fix(gate-9): the admin rule matched no real isAdmin() call, and prose bought the self-auth exemption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, found while working two openconnector findings. 1. `no-admin-required-annotation-with-admin-body` was blind to the only form the guard is ever written in. The condition matcher used `\bisAdmin\b[^)]*===\s*false`, and `isAdmin($this->userId) === false` puts a `)` between the name and the comparison, so the character class could not reach it. The same over-restrictive-class mistake as the `[^}]*` body regex this module was written to replace (W28). The condition slice is already bounded by its own matching paren, so `.*?` is safe there; the Yoda form is matched too. That one character class was hiding 25 findings across 10 repos — #[NoAdminRequired] on methods that enforce admin in the body, i.e. endpoints whose attribute says the opposite of what they do. Every one was reported PASS by every run of this gate until now. Sampled and confirmed real: openregister FederatedConfigController::trust/setTrust, softwarecatalog SettingsController ×8, pipelinq CtiController ×4. 2. The self-auth exemption was matched against raw source, so a docblock describing a credential check exempted a method that performed none. The 2026-08-06 gate-64 shape, where a commented-out call counted as a real one. Matched against comment-stripped source now. Comments only — NOT string literals. `'Bearer '`, `'HTTP_AUTHORIZATION'` and the header name passed to getHeader() are literals in every real handler, and blanking those would manufacture exactly the false positives this gate was rewritten to stop. 3. Two endpoints were passing only BECAUSE of (2), and would have turned red the moment it was fixed: hermiq McpRunController::handle and EgressAuthorizeController::authorize resolve their credential through a helper (`bearerToken()`) passed as a named argument (`token:`), neither of which the six-verb pattern list covered. It only ever matched the word "bearer" in their comments. Widened to any `->…Token…(` call and to named credential arguments. Also adds username/password credentials to the exemption. 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. The token-only list reported openconnector UserController::login as an unsourced denial although it resolves $username/$password from the request and calls checkPassword() in the body; there was no correct action a developer could take on that finding. Measured over 761 fleet controllers, old gate vs new: 2 false positives removed, 25 previously invisible true positives revealed. Adds test_check_semantic_auth.py — the module had none, and it is picked up automatically by the discovery runner. 21 assertions, of which 9 are failure-demonstrating: every exemption is paired with the same PHP shape minus the one thing that earns it, so the suite cannot go quiet by accident. Three tests specifically pin (2) and (3) against each other — prose must not exempt, literals must still count. --- .../scripts/lib/check_semantic_auth.py | 116 +++- .../scripts/lib/test_check_semantic_auth.py | 546 ++++++++++++------ 2 files changed, 490 insertions(+), 172 deletions(-) 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..1828c632 100644 --- a/hydra-gates/scripts/lib/test_check_semantic_auth.py +++ b/hydra-gates/scripts/lib/test_check_semantic_auth.py @@ -4,22 +4,24 @@ python3 scripts/lib/test_check_semantic_auth.py -WHY THIS SUITE EXISTS ---------------------- -gate-9 shipped with NO tests. Its `public-page-annotation-with-auth-body` -rule produced 39 fleet findings of which 36 were false, and its remediation -text — *"remove `#[PublicPage]` or remove body auth check"* — would have -INTRODUCED a vulnerability in every one of those 36: the first half breaks -the endpoint (Nextcloud middleware rejects the remote caller before the -controller runs), the second half deletes its only authentication. - -The fixtures below are the real call shapes, verbatim in structure: -openconnector's HMAC webhook, portaliq's portal `subject()` inbox, -openregister's bearer-share-token federation reader — against decidesk's -actual defect, a `#[PublicPage]` method that tests the SESSION. - -Both ways, in the same class: the self-authenticating shapes must go quiet, -and the session-dependent shape must still fire. +or via pytest: + + python3 -m pytest scripts/lib/test_check_semantic_auth.py + +Gate-9 has two exemptions, and an exemption is a way for a real mismatch +to slip through. So every "must not fire" test below is paired with a +"must still fire" test built from the same PHP shape minus the one thing +that earns the exemption. A suite in which nothing fails proves nothing. + +:class:`ProseDoesNotEarnTheExemption` is the one that matters most. The +self-auth exemption used to be matched against the raw source, so a +docblock reading "callers present a bearer token" exempted a method that +checked no such thing — the 2026-08-06 gate-64 failure mode, where a +commented-out call counted as a real one. Those tests fail if comment +stripping is ever removed. + +Fixtures are written as realistic Nextcloud controllers rather than as +minimal echoes of the gate's own regexes (the gate-56 lesson). """ from __future__ import annotations @@ -35,217 +37,429 @@ import check_semantic_auth as csa # noqa: E402 -def _scan(php: str) -> list[str]: - with tempfile.TemporaryDirectory() as d: - p = Path(d) / "Controller.php" - p.write_text(php, encoding="utf-8") +def _scan(src: str) -> list[str]: + """Write *src* to a throwaway .php file and return the finding lines.""" + with tempfile.TemporaryDirectory() as root: + path = Path(root) / "SomeController.php" + path.write_text(src, encoding="utf-8") buf = io.StringIO() with redirect_stdout(buf): - csa.scan_file(str(p)) - return [ln for ln in buf.getvalue().splitlines() if ln.strip()] - + csa.scan_file(str(path)) + return [ln for ln in buf.getvalue().splitlines() if ln.strip()] -def _rules(findings: list[str]) -> list[str]: - return [f.split("rule=", 1)[1].split(" ", 1)[0] for f in findings] +_PROLOGUE = """\ + str: + return _PROLOGUE + methods + "\n}\n" + + +# --------------------------------------------------------------------------- +# The session-mismatch rule — a session gate hiding behind a public attribute. +# --------------------------------------------------------------------------- + +class SessionGateBehindPublicPage(unittest.TestCase): + """#[PublicPage] whose denial reads the session can only ever deny.""" + + def test_user_session_null_check_is_flagged(self): + src = _controller(""" #[PublicPage] - #[NoCSRFRequired] - public function webhook(): JSONResponse - { - $signature = $this->request->getHeader('X-Mollie-Signature'); - $expected = hash_hmac('sha256', $this->request->getContent(), $this->secret); - if (hash_equals($expected, $signature) === false) { - return new JSONResponse(['error' => 'bad signature'], Http::STATUS_UNAUTHORIZED); + public function summary(): JSONResponse { + if ($this->userSession->getUser() === null) { + return new JSONResponse(['error' => 'sign in first'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse(['ok' => true]); + + return new JSONResponse($this->dashboardService->summarise()); } -""" +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) + + def test_require_admin_is_flagged(self): + src = _controller(""" + #[PublicPage] + public function purgeCaches(): JSONResponse { + $this->requireAdmin(); -PORTAL_INBOX = CLASS % """ + $this->cacheService->purgeAll(); + return new JSONResponse(['purged' => true]); + } +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) + + def test_a_bearer_token_does_not_launder_a_session_gate(self): + """Self-auth must not buy an exemption from the session rule. + + The session branch is checked first and has no exemption; a method + that authenticates a token AND then consults the session is still + contradicting its own attribute. + """ + src = _controller(""" #[PublicPage] - public function inbox(): JSONResponse - { - $subject = $this->portal->subject(); - if ($subject === null) { - return new JSONResponse(['error' => 'no portal session'], Http::STATUS_UNAUTHORIZED); + public function dump(): JSONResponse { + $presentedToken = (string) $this->request->getHeader('Authorization'); + if (hash_equals($this->expectedToken(), $presentedToken) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse($this->service->listFor($subject)); + + $this->requireAdmin(); + + return new JSONResponse($this->exportService->dumpEverything()); } -""" +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) + + +# --------------------------------------------------------------------------- +# The unsourced-denial rule — 401/403 with no credential in the request. +# --------------------------------------------------------------------------- -FEDERATION_BEARER = CLASS % """ +class DenialWithoutACredential(unittest.TestCase): + """A public endpoint that denies on app state, not on anything sent.""" + + def test_feature_flag_denial_is_flagged(self): + src = _controller(""" #[PublicPage] - #[NoCSRFRequired] - public function objects(string $shareToken): JSONResponse - { - $share = $this->shares->findByToken($shareToken); - if ($share === null || $share->isRevoked()) { - return new JSONResponse(['error' => 'unknown share'], Http::STATUS_FORBIDDEN); + public function index(): JSONResponse { + if ($this->config->getAppValue('demo', 'portal_enabled', 'no') !== 'yes') { + return new JSONResponse(['error' => 'portal disabled'], Http::STATUS_FORBIDDEN); } - return new JSONResponse($this->service->objectsFor($share)); + + return new JSONResponse($this->portalService->landingPage()); } -""" +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) -# decidesk's real defect shape: annotated public, but the body tests the -# SESSION — a test that can only ever fail for the callers the annotation -# admits. -SESSION_DEPENDENT = CLASS % """ + def test_published_flag_denial_is_flagged(self): + src = _controller(""" #[PublicPage] - public function load(): JSONResponse - { - $this->requireAdmin(); - return new JSONResponse($this->settings->all()); + public function download(int $id): JSONResponse { + if ($this->exportService->isPublished($id) === false) { + return new JSONResponse(['error' => 'not available'], Http::STATUS_FORBIDDEN); + } + + return new JSONResponse($this->exportService->payload($id)); } -""" +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) + -SESSION_NULL_CHECK = CLASS % """ +class CredentialInTheRequestIsNotAMismatch(unittest.TestCase): + """Public by necessity, 401 by correctness — the 36-of-39 shape.""" + + def test_hmac_signed_webhook_is_not_flagged(self): + src = _controller(""" #[PublicPage] - public function mine(): JSONResponse - { - if ($this->userSession->getUser() === null) { - return new JSONResponse([], Http::STATUS_UNAUTHORIZED); + public function webhook(): JSONResponse { + $payload = file_get_contents('php://input'); + $expected = 'sha256=' . hash_hmac('sha256', $payload, $this->sharedSecret()); + + if (hash_equals($expected, (string) $this->request->getHeader('X-Hub-Signature-256')) === false) { + return new JSONResponse(['error' => 'bad signature'], Http::STATUS_FORBIDDEN); } - return new JSONResponse($this->service->forCurrentUser()); + + return new JSONResponse([], 202); } -""" +""") + self.assertEqual(_scan(src), []) -# 401 with no credential source anywhere: the shape that remains worth -# reporting, because nothing in the method says what it authenticates with. -UNSOURCED_DENIAL = CLASS % """ + def test_route_share_token_is_not_flagged(self): + src = _controller(""" #[PublicPage] - public function report(): JSONResponse - { - if ($this->flags->closed()) { - return new JSONResponse(['error' => 'closed'], Http::STATUS_FORBIDDEN); + public function share(string $shareToken): JSONResponse { + $share = $this->shareMapper->findByToken($shareToken); + if ($share === null || $share->getExpired() === true) { + return new JSONResponse(['error' => 'unknown share'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse($this->service->report()); + + return new JSONResponse($this->shareService->contents($share)); } -""" +""") + self.assertEqual(_scan(src), []) + + def test_login_endpoint_is_not_flagged(self): + """The credential is a password, not a token. + + openconnector UserController::login (2026-08-07): correct code that + the token-only pattern list reported as an unsourced denial. + """ + src = _controller(""" + #[NoCSRFRequired] + #[PublicPage] + public function login(): JSONResponse { + $data = $this->request->getParams(); + $credentials = $this->securityService->validateLoginCredentials($data); + $username = $credentials['username']; + $password = $credentials['password']; + + $user = $this->userManager->checkPassword($username, $password); + if ($user === false) { + $this->securityService->recordFailedLoginAttempt($username, $this->clientIp()); + return new JSONResponse(['error' => 'invalid credentials'], Http::STATUS_UNAUTHORIZED); + } + + return new JSONResponse(['uid' => $user->getUID()]); + } +""") + self.assertEqual(_scan(src), []) + + def test_named_argument_and_helper_resolved_token_is_not_flagged(self): + """hermiq McpRunController::handle / EgressAuthorizeController::authorize. + + The credential is resolved by a helper (`bearerToken()`) and handed + over as a named argument (`token:`). Neither shape is one of the + six verbs the pattern list started with, and the only reason these + two used to pass was the word "bearer" in their own comments — so + they are the pair that would silently break if the comment-stripping + change were made without widening the idioms alongside it. + """ + src = _controller(""" + #[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->mcpService->dispatch($binding, $this->readRawBody())); + } +""") + self.assertEqual(_scan(src), []) + def test_password_protected_share_is_not_flagged(self): + src = _controller(""" + #[PublicPage] + public function unlock(string $slug): JSONResponse { + $folder = $this->folderMapper->findBySlug($slug); -class SelfAuthenticatingPublicEndpoints(unittest.TestCase): - """The 36 false positives.""" + if (password_verify((string) $this->request->getParam('password'), $folder->getPasswordHash()) === false) { + return new JSONResponse(['error' => 'wrong password'], Http::STATUS_UNAUTHORIZED); + } - def test_fp_hmac_signed_webhook_is_not_a_finding(self): - self.assertEqual(_scan(HMAC_WEBHOOK), []) + return new JSONResponse($this->folderService->listing($folder)); + } +""") + self.assertEqual(_scan(src), []) + + +class ProseDoesNotEarnTheExemption(unittest.TestCase): + """Only executable code counts as authenticating a credential.""" + + def test_docblock_describing_a_token_check_is_still_flagged(self): + src = _controller(""" + /** + * Download an export. + * + * Callers must present a signed capability token in the Authorization + * header. $presentedToken is compared against the stored secret with + * hash_equals() before any payload is returned. + */ + #[PublicPage] + public function download(int $id): JSONResponse { + if ($this->exportService->isPublished($id) === false) { + return new JSONResponse(['error' => 'not available'], Http::STATUS_FORBIDDEN); + } - def test_fp_portal_subject_resolution_is_not_a_finding(self): - self.assertEqual(_scan(PORTAL_INBOX), []) + return new JSONResponse($this->exportService->payload($id)); + } +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) - def test_fp_bearer_share_token_is_not_a_finding(self): - self.assertEqual(_scan(FEDERATION_BEARER), []) + def test_commented_out_credential_check_is_still_flagged(self): + src = _controller(""" + #[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->importService->isAcceptingUploads() === false) { + return new JSONResponse(['error' => 'closed'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse([], 202); + } +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) -class SessionDependentPublicPage(unittest.TestCase): - """The 3 true positives — these MUST still fire.""" - def test_tp_require_admin_under_public_page_still_fires(self): - self.assertEqual(_rules(_scan(SESSION_DEPENDENT)), - ["public-page-annotation-with-session-auth-body"]) +class StringLiteralsStillCount(unittest.TestCase): + """Comment stripping must not take the literals the idioms live in. - def test_tp_user_session_null_check_under_public_page_still_fires(self): - self.assertEqual(_rules(_scan(SESSION_NULL_CHECK)), - ["public-page-annotation-with-session-auth-body"]) + ``'Bearer '``, ``'HTTP_AUTHORIZATION'`` and the header name handed to + ``getHeader()`` are string literals in every real handler. Blanking them + alongside comments would turn correct code into findings — the exact + failure gate-9 was rewritten to stop. + """ - def test_tp_a_session_check_fires_even_alongside_a_token(self): - # Self-authentication excuses an UNSOURCED denial, never a session - # test. A method that reads a token AND requires a session is still - # contradicting its own annotation. - php = CLASS % """ + def test_bearer_prefix_in_a_literal_is_not_flagged(self): + src = _controller(""" #[PublicPage] - public function mixed(string $shareToken): JSONResponse - { - $share = $this->shares->findByToken($shareToken); - $this->requireAdmin(); - return new JSONResponse($share); + public function ingest(): JSONResponse { + $header = (string) $this->request->getHeader('Authorization'); + $presented = str_replace('Bearer ', '', $header); + + if ($this->apiKeyService->authorize($presented) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + } + + return new JSONResponse([], 202); } -""" - self.assertEqual(_rules(_scan(php)), - ["public-page-annotation-with-session-auth-body"]) +""") + self.assertEqual(_scan(src), []) - def test_tp_a_denial_with_no_credential_source_is_reported(self): - self.assertEqual(_rules(_scan(UNSOURCED_DENIAL)), - ["public-page-annotation-with-unsourced-denial"]) + def test_server_superglobal_header_is_not_flagged(self): + src = _controller(""" + #[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(src), []) -class RemediationTextIsSafe(unittest.TestCase): - """The advice itself is part of the gate. If it tells a developer to open - the endpoint, the gate is a vulnerability generator regardless of its - precision. These assertions are on the STRING, deliberately.""" + def test_a_url_with_a_double_slash_does_not_swallow_the_rest(self): + """`//` inside a literal must not be read as a comment start. - def _advice(self, php: str) -> str: - found = _scan(php) - self.assertTrue(found, "expected a finding to inspect the advice of") - return found[0] + If it were, everything after it — including the credential check — + would be blanked and the method would be reported unsourced. + """ + src = _controller(""" + #[PublicPage] + public function callback(): JSONResponse { + $issuer = 'https://idp.example.org/realms/demo'; + $presented = (string) $this->request->getHeader('Authorization'); - def test_advice_never_says_to_remove_the_public_page_annotation(self): - for php in (SESSION_DEPENDENT, SESSION_NULL_CHECK, UNSOURCED_DENIAL): - with self.subTest(php=php[:60]): - advice = self._advice(php) - self.assertNotIn("remove #[PublicPage] or", advice) + if ($this->oidcService->verifyIdToken($presented, $issuer) === false) { + return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + } - def test_advice_never_says_to_remove_the_auth_check(self): - for php in (SESSION_DEPENDENT, SESSION_NULL_CHECK, UNSOURCED_DENIAL): - with self.subTest(php=php[:60]): - advice = self._advice(php) - self.assertNotIn("remove body auth check", advice) - self.assertIn("Do NOT", advice) + return new JSONResponse(['issuer' => $issuer]); + } +""") + self.assertEqual(_scan(src), []) - def test_advice_names_the_request_borne_alternative(self): - advice = self._advice(SESSION_DEPENDENT) - self.assertIn("route token", advice) +# --------------------------------------------------------------------------- +# The #[NoAdminRequired] rule, and silence on ordinary controllers. +# --------------------------------------------------------------------------- -class NoAdminRequiredRuleUnchanged(unittest.TestCase): - """The rule gate-9 was actually built for. It was RIGHT — 3 of 3 real — - and nothing above may weaken it.""" +class NoAdminRequiredWithAdminBody(unittest.TestCase): - def test_tp_no_admin_required_with_an_admin_body_still_fires(self): - php = CLASS % """ + def test_require_admin_under_no_admin_required_is_flagged(self): + src = _controller(""" #[NoAdminRequired] - public function load(): JSONResponse - { + public function destroy(int $id): JSONResponse { $this->requireAdmin(); - return new JSONResponse($this->settings->all()); + + $this->registerService->delete($id); + return new JSONResponse([], 204); } -""" - self.assertEqual(_rules(_scan(php)), - ["no-admin-required-annotation-with-admin-body"]) +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) - def test_fp_a_plain_no_admin_required_method_is_clean(self): - php = CLASS % """ + def test_is_admin_guard_with_forbidden_return_is_flagged(self): + src = _controller(""" #[NoAdminRequired] - public function index(): JSONResponse - { - return new JSONResponse($this->service->listForCurrentUser()); + public function settings(): JSONResponse { + if ($this->groupManager->isAdmin($this->userId) === false) { + return new JSONResponse(['error' => 'admins only'], Http::STATUS_FORBIDDEN); + } + + return new JSONResponse($this->settingsService->all()); } -""" - self.assertEqual(_scan(php), []) +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) + def test_yoda_comparison_is_flagged(self): + src = _controller(""" + #[NoAdminRequired] + public function purge(): JSONResponse { + if (false === $this->groupManager->isAdmin($this->userId)) { + throw new OCSForbiddenException('admins only'); + } -class GateIsNotBlind(unittest.TestCase): - def test_the_scanner_still_reads_methods_at_all(self): - # If `_find_method_bodies` ever returns nothing, every `assertEqual([])` - # above passes. This asserts the floor directly. - php = CLASS % """ - #[PublicPage] - public function a(): JSONResponse { $this->requireAdmin(); return new JSONResponse([]); } + $this->cacheService->purgeAll(); + return new JSONResponse(['purged' => true]); + } +""") + findings = _scan(src) + self.assertEqual(len(findings), 1, findings) + self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) + + def test_a_non_admin_predicate_is_not_flagged(self): + """The guard has to be about being an admin, not merely an `if`.""" + src = _controller(""" + #[NoAdminRequired] + public function publish(int $id): JSONResponse { + if ($this->publicationService->isReviewed($id) === false) { + return new JSONResponse(['error' => 'not reviewed yet'], Http::STATUS_FORBIDDEN); + } - #[PublicPage] - public function b(): JSONResponse { $this->requireAdmin(); return new JSONResponse([]); } -""" - self.assertEqual(len(_scan(php)), 2) + return new JSONResponse($this->publicationService->publish($id)); + } +""") + self.assertEqual(_scan(src), []) + + +class OrdinaryControllersProduceNoFindings(unittest.TestCase): + + def test_plain_authenticated_endpoints_are_silent(self): + src = _controller(""" + #[NoAdminRequired] + public function index(): JSONResponse { + return new JSONResponse($this->objectService->findAll()); + } + + #[NoAdminRequired] + public function show(int $id): JSONResponse { + return new JSONResponse($this->objectService->find($id)); + } +""") + self.assertEqual(_scan(src), []) + + def test_an_admin_endpoint_with_no_attribute_is_silent(self): + src = _controller(""" + public function reindex(): JSONResponse { + $this->searchService->reindexEverything(); + return new JSONResponse(['queued' => true]); + } +""") + self.assertEqual(_scan(src), []) if __name__ == "__main__": From 023959d9e89f44974b053ebd1cd7bf59b60f4a11 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Fri, 7 Aug 2026 10:04:52 +0200 Subject: [PATCH 2/2] test(gate-9): restore the suite the previous commit overwrote, and add to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit said this module shipped without tests and wrote a new file. It did not — 252 lines of suite already existed, and the write replaced them. Restored verbatim and extended instead. What came back matters more than what I added. `RemediationTextIsSafe` asserts on the FINDING STRING itself: that the advice never tells a developer to remove #[PublicPage] or delete the auth check, and that it names the request-borne alternative. That is the assertion standing between this gate and its own history of telling people to open an endpoint. `GateIsNotBlind` asserts the scanner reads methods at all, without which every `assertEqual([])` in the file passes on nothing. Added on top, one class per defect the previous commit fixed: AdminGuardsWithArguments isAdmin($uid) === false, and the Yoda form, against a non-admin predicate that must stay clean PasswordsAreCredentialsToo login, password_verify share unlock, and the helper-resolved token passed as a named argument ProseDoesNotEarnTheExemption a docblock and a commented-out check must still fire StringLiteralsStillCount 'Bearer ', $_SERVER['HTTP_…'] and a `//` inside a URL must still count The last two are each other's control: strip too little and prose exempts, strip too much and correct handlers become findings. 24 tests, up from 13. --- .../scripts/lib/test_check_semantic_auth.py | 674 +++++++++--------- 1 file changed, 350 insertions(+), 324 deletions(-) diff --git a/hydra-gates/scripts/lib/test_check_semantic_auth.py b/hydra-gates/scripts/lib/test_check_semantic_auth.py index 1828c632..e1db746e 100644 --- a/hydra-gates/scripts/lib/test_check_semantic_auth.py +++ b/hydra-gates/scripts/lib/test_check_semantic_auth.py @@ -4,24 +4,36 @@ python3 scripts/lib/test_check_semantic_auth.py -or via pytest: - - python3 -m pytest scripts/lib/test_check_semantic_auth.py - -Gate-9 has two exemptions, and an exemption is a way for a real mismatch -to slip through. So every "must not fire" test below is paired with a -"must still fire" test built from the same PHP shape minus the one thing -that earns the exemption. A suite in which nothing fails proves nothing. - -:class:`ProseDoesNotEarnTheExemption` is the one that matters most. The -self-auth exemption used to be matched against the raw source, so a -docblock reading "callers present a bearer token" exempted a method that -checked no such thing — the 2026-08-06 gate-64 failure mode, where a -commented-out call counted as a real one. Those tests fail if comment -stripping is ever removed. - -Fixtures are written as realistic Nextcloud controllers rather than as -minimal echoes of the gate's own regexes (the gate-56 lesson). +WHY THIS SUITE EXISTS +--------------------- +gate-9 shipped with NO tests. Its `public-page-annotation-with-auth-body` +rule produced 39 fleet findings of which 36 were false, and its remediation +text — *"remove `#[PublicPage]` or remove body auth check"* — would have +INTRODUCED a vulnerability in every one of those 36: the first half breaks +the endpoint (Nextcloud middleware rejects the remote caller before the +controller runs), the second half deletes its only authentication. + +The fixtures below are the real call shapes, verbatim in structure: +openconnector's HMAC webhook, portaliq's portal `subject()` inbox, +openregister's bearer-share-token federation reader — against decidesk's +actual defect, a `#[PublicPage]` method that tests the SESSION. + +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 @@ -37,429 +49,443 @@ import check_semantic_auth as csa # noqa: E402 -def _scan(src: str) -> list[str]: - """Write *src* to a throwaway .php file and return the finding lines.""" - with tempfile.TemporaryDirectory() as root: - path = Path(root) / "SomeController.php" - path.write_text(src, encoding="utf-8") +def _scan(php: str) -> list[str]: + with tempfile.TemporaryDirectory() as d: + p = Path(d) / "Controller.php" + p.write_text(php, encoding="utf-8") buf = io.StringIO() with redirect_stdout(buf): - csa.scan_file(str(path)) - return [ln for ln in buf.getvalue().splitlines() if ln.strip()] - + csa.scan_file(str(p)) + return [ln for ln in buf.getvalue().splitlines() if ln.strip()] -_PROLOGUE = """\ - list[str]: + return [f.split("rule=", 1)[1].split(" ", 1)[0] for f in findings] -class DemoController extends Controller { -""" - - -def _controller(methods: str) -> str: - return _PROLOGUE + methods + "\n}\n" +CLASS = """request->getHeader('X-Mollie-Signature'); + $expected = hash_hmac('sha256', $this->request->getContent(), $this->secret); + if (hash_equals($expected, $signature) === false) { + return new JSONResponse(['error' => 'bad signature'], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse(['ok' => true]); + } +""" - def test_user_session_null_check_is_flagged(self): - src = _controller(""" +PORTAL_INBOX = CLASS % """ #[PublicPage] - public function summary(): JSONResponse { - if ($this->userSession->getUser() === null) { - return new JSONResponse(['error' => 'sign in first'], Http::STATUS_UNAUTHORIZED); + public function inbox(): JSONResponse + { + $subject = $this->portal->subject(); + if ($subject === null) { + return new JSONResponse(['error' => 'no portal session'], Http::STATUS_UNAUTHORIZED); } + return new JSONResponse($this->service->listFor($subject)); + } +""" - return new JSONResponse($this->dashboardService->summarise()); +FEDERATION_BEARER = CLASS % """ + #[PublicPage] + #[NoCSRFRequired] + public function objects(string $shareToken): JSONResponse + { + $share = $this->shares->findByToken($shareToken); + if ($share === null || $share->isRevoked()) { + return new JSONResponse(['error' => 'unknown share'], Http::STATUS_FORBIDDEN); + } + return new JSONResponse($this->service->objectsFor($share)); } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) +""" - def test_require_admin_is_flagged(self): - src = _controller(""" +# decidesk's real defect shape: annotated public, but the body tests the +# SESSION — a test that can only ever fail for the callers the annotation +# admits. +SESSION_DEPENDENT = CLASS % """ #[PublicPage] - public function purgeCaches(): JSONResponse { + public function load(): JSONResponse + { $this->requireAdmin(); + return new JSONResponse($this->settings->all()); + } +""" - $this->cacheService->purgeAll(); - return new JSONResponse(['purged' => true]); +SESSION_NULL_CHECK = CLASS % """ + #[PublicPage] + public function mine(): JSONResponse + { + if ($this->userSession->getUser() === null) { + return new JSONResponse([], Http::STATUS_UNAUTHORIZED); + } + return new JSONResponse($this->service->forCurrentUser()); } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) - - def test_a_bearer_token_does_not_launder_a_session_gate(self): - """Self-auth must not buy an exemption from the session rule. - - The session branch is checked first and has no exemption; a method - that authenticates a token AND then consults the session is still - contradicting its own attribute. - """ - src = _controller(""" +""" + +# 401 with no credential source anywhere: the shape that remains worth +# reporting, because nothing in the method says what it authenticates with. +UNSOURCED_DENIAL = CLASS % """ #[PublicPage] - public function dump(): JSONResponse { - $presentedToken = (string) $this->request->getHeader('Authorization'); - if (hash_equals($this->expectedToken(), $presentedToken) === false) { - return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); + public function report(): JSONResponse + { + if ($this->flags->closed()) { + return new JSONResponse(['error' => 'closed'], Http::STATUS_FORBIDDEN); } + return new JSONResponse($this->service->report()); + } +""" - $this->requireAdmin(); - return new JSONResponse($this->exportService->dumpEverything()); - } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-session-auth-body", findings[0]) +class SelfAuthenticatingPublicEndpoints(unittest.TestCase): + """The 36 false positives.""" + def test_fp_hmac_signed_webhook_is_not_a_finding(self): + self.assertEqual(_scan(HMAC_WEBHOOK), []) -# --------------------------------------------------------------------------- -# The unsourced-denial rule — 401/403 with no credential in the request. -# --------------------------------------------------------------------------- + def test_fp_portal_subject_resolution_is_not_a_finding(self): + self.assertEqual(_scan(PORTAL_INBOX), []) -class DenialWithoutACredential(unittest.TestCase): - """A public endpoint that denies on app state, not on anything sent.""" + def test_fp_bearer_share_token_is_not_a_finding(self): + self.assertEqual(_scan(FEDERATION_BEARER), []) - def test_feature_flag_denial_is_flagged(self): - src = _controller(""" - #[PublicPage] - public function index(): JSONResponse { - if ($this->config->getAppValue('demo', 'portal_enabled', 'no') !== 'yes') { - return new JSONResponse(['error' => 'portal disabled'], Http::STATUS_FORBIDDEN); - } - return new JSONResponse($this->portalService->landingPage()); - } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) +class SessionDependentPublicPage(unittest.TestCase): + """The 3 true positives — these MUST still fire.""" - def test_published_flag_denial_is_flagged(self): - src = _controller(""" - #[PublicPage] - public function download(int $id): JSONResponse { - if ($this->exportService->isPublished($id) === false) { - return new JSONResponse(['error' => 'not available'], Http::STATUS_FORBIDDEN); - } + def test_tp_require_admin_under_public_page_still_fires(self): + self.assertEqual(_rules(_scan(SESSION_DEPENDENT)), + ["public-page-annotation-with-session-auth-body"]) + + def test_tp_user_session_null_check_under_public_page_still_fires(self): + self.assertEqual(_rules(_scan(SESSION_NULL_CHECK)), + ["public-page-annotation-with-session-auth-body"]) - return new JSONResponse($this->exportService->payload($id)); + def test_tp_a_session_check_fires_even_alongside_a_token(self): + # Self-authentication excuses an UNSOURCED denial, never a session + # test. A method that reads a token AND requires a session is still + # contradicting its own annotation. + php = CLASS % """ + #[PublicPage] + public function mixed(string $shareToken): JSONResponse + { + $share = $this->shares->findByToken($shareToken); + $this->requireAdmin(); + return new JSONResponse($share); } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) +""" + self.assertEqual(_rules(_scan(php)), + ["public-page-annotation-with-session-auth-body"]) + def test_tp_a_denial_with_no_credential_source_is_reported(self): + self.assertEqual(_rules(_scan(UNSOURCED_DENIAL)), + ["public-page-annotation-with-unsourced-denial"]) -class CredentialInTheRequestIsNotAMismatch(unittest.TestCase): - """Public by necessity, 401 by correctness — the 36-of-39 shape.""" - def test_hmac_signed_webhook_is_not_flagged(self): - src = _controller(""" - #[PublicPage] - public function webhook(): JSONResponse { - $payload = file_get_contents('php://input'); - $expected = 'sha256=' . hash_hmac('sha256', $payload, $this->sharedSecret()); +class RemediationTextIsSafe(unittest.TestCase): + """The advice itself is part of the gate. If it tells a developer to open + the endpoint, the gate is a vulnerability generator regardless of its + precision. These assertions are on the STRING, deliberately.""" - if (hash_equals($expected, (string) $this->request->getHeader('X-Hub-Signature-256')) === false) { - return new JSONResponse(['error' => 'bad signature'], Http::STATUS_FORBIDDEN); - } + def _advice(self, php: str) -> str: + found = _scan(php) + self.assertTrue(found, "expected a finding to inspect the advice of") + return found[0] - return new JSONResponse([], 202); + def test_advice_never_says_to_remove_the_public_page_annotation(self): + for php in (SESSION_DEPENDENT, SESSION_NULL_CHECK, UNSOURCED_DENIAL): + with self.subTest(php=php[:60]): + advice = self._advice(php) + self.assertNotIn("remove #[PublicPage] or", advice) + + def test_advice_never_says_to_remove_the_auth_check(self): + for php in (SESSION_DEPENDENT, SESSION_NULL_CHECK, UNSOURCED_DENIAL): + with self.subTest(php=php[:60]): + advice = self._advice(php) + self.assertNotIn("remove body auth check", advice) + self.assertIn("Do NOT", advice) + + def test_advice_names_the_request_borne_alternative(self): + advice = self._advice(SESSION_DEPENDENT) + self.assertIn("route token", advice) + + +class NoAdminRequiredRuleUnchanged(unittest.TestCase): + """The rule gate-9 was actually built for. It was RIGHT — 3 of 3 real — + and nothing above may weaken it.""" + + def test_tp_no_admin_required_with_an_admin_body_still_fires(self): + php = CLASS % """ + #[NoAdminRequired] + public function load(): JSONResponse + { + $this->requireAdmin(); + return new JSONResponse($this->settings->all()); } -""") - self.assertEqual(_scan(src), []) +""" + self.assertEqual(_rules(_scan(php)), + ["no-admin-required-annotation-with-admin-body"]) - def test_route_share_token_is_not_flagged(self): - src = _controller(""" - #[PublicPage] - public function share(string $shareToken): JSONResponse { - $share = $this->shareMapper->findByToken($shareToken); - if ($share === null || $share->getExpired() === true) { - return new JSONResponse(['error' => 'unknown share'], Http::STATUS_UNAUTHORIZED); + def test_fp_a_plain_no_admin_required_method_is_clean(self): + php = CLASS % """ + #[NoAdminRequired] + public function index(): JSONResponse + { + return new JSONResponse($this->service->listForCurrentUser()); + } +""" + 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"]) - return new JSONResponse($this->shareService->contents($share)); + 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(src), []) +""" + self.assertEqual(_scan(php), []) + + +class PasswordsAreCredentialsToo(unittest.TestCase): + """A login endpoint is the self-authenticating shape with a password. - def test_login_endpoint_is_not_flagged(self): - """The credential is a password, not a token. + 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. + """ - openconnector UserController::login (2026-08-07): correct code that - the token-only pattern list reported as an unsourced denial. - """ - src = _controller(""" + def test_fp_login_is_not_a_finding(self): + php = CLASS % """ #[NoCSRFRequired] #[PublicPage] - public function login(): JSONResponse { + public function login(): JSONResponse + { $data = $this->request->getParams(); - $credentials = $this->securityService->validateLoginCredentials($data); + $credentials = $this->security->validateLoginCredentials($data); $username = $credentials['username']; $password = $credentials['password']; $user = $this->userManager->checkPassword($username, $password); if ($user === false) { - $this->securityService->recordFailedLoginAttempt($username, $this->clientIp()); + $this->security->recordFailedLoginAttempt($username, $this->clientIp()); return new JSONResponse(['error' => 'invalid credentials'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse(['uid' => $user->getUID()]); } -""") - self.assertEqual(_scan(src), []) - - def test_named_argument_and_helper_resolved_token_is_not_flagged(self): - """hermiq McpRunController::handle / EgressAuthorizeController::authorize. - - The credential is resolved by a helper (`bearerToken()`) and handed - over as a named argument (`token:`). Neither shape is one of the - six verbs the pattern list started with, and the only reason these - two used to pass was the word "bearer" in their own comments — so - they are the pair that would silently break if the comment-stripping - change were made without widening the idioms alongside it. - """ - src = _controller(""" +""" + 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 { + 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->mcpService->dispatch($binding, $this->readRawBody())); - } -""") - self.assertEqual(_scan(src), []) - - def test_password_protected_share_is_not_flagged(self): - src = _controller(""" - #[PublicPage] - public function unlock(string $slug): JSONResponse { - $folder = $this->folderMapper->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->folderService->listing($folder)); + return new JSONResponse($this->mcp->dispatch($binding, $this->readRawBody())); } -""") - self.assertEqual(_scan(src), []) +""" + self.assertEqual(_scan(php), []) class ProseDoesNotEarnTheExemption(unittest.TestCase): - """Only executable code counts as authenticating a credential.""" + """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_docblock_describing_a_token_check_is_still_flagged(self): - src = _controller(""" + 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. $presentedToken is compared against the stored secret with - * hash_equals() before any payload is returned. + * 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->exportService->isPublished($id) === false) { + 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->exportService->payload($id)); + return new JSONResponse($this->exports->payload($id)); } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) +""" + self.assertEqual(_rules(_scan(php)), + ["public-page-annotation-with-unsourced-denial"]) - def test_commented_out_credential_check_is_still_flagged(self): - src = _controller(""" + def test_tp_a_commented_out_credential_check_still_fires(self): + php = CLASS % """ #[PublicPage] - public function receive(): JSONResponse { + 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->importService->isAcceptingUploads() === false) { + if ($this->imports->isAcceptingUploads() === false) { return new JSONResponse(['error' => 'closed'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse([], 202); } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=public-page-annotation-with-unsourced-denial", findings[0]) +""" + self.assertEqual(_rules(_scan(php)), + ["public-page-annotation-with-unsourced-denial"]) class StringLiteralsStillCount(unittest.TestCase): - """Comment stripping must not take the literals the idioms live in. + """The control on ProseDoesNotEarnTheExemption. - ``'Bearer '``, ``'HTTP_AUTHORIZATION'`` and the header name handed to - ``getHeader()`` are string literals in every real handler. Blanking them - alongside comments would turn correct code into findings — the exact - failure gate-9 was rewritten to stop. + 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_bearer_prefix_in_a_literal_is_not_flagged(self): - src = _controller(""" + def test_fp_a_bearer_prefix_stripped_from_a_literal_is_not_a_finding(self): + php = CLASS % """ #[PublicPage] - public function ingest(): JSONResponse { + public function ingest(): JSONResponse + { $header = (string) $this->request->getHeader('Authorization'); $presented = str_replace('Bearer ', '', $header); - if ($this->apiKeyService->authorize($presented) === false) { + if ($this->apiKeys->authorize($presented) === false) { return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse([], 202); } -""") - self.assertEqual(_scan(src), []) +""" + self.assertEqual(_scan(php), []) - def test_server_superglobal_header_is_not_flagged(self): - src = _controller(""" + def test_fp_the_server_superglobal_header_key_is_not_a_finding(self): + php = CLASS % """ #[PublicPage] - public function ping(): JSONResponse { + 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(src), []) - - def test_a_url_with_a_double_slash_does_not_swallow_the_rest(self): - """`//` inside a literal must not be read as a comment start. +""" + self.assertEqual(_scan(php), []) - If it were, everything after it — including the credential check — - would be blanked and the method would be reported unsourced. - """ - src = _controller(""" + 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 { + public function callback(): JSONResponse + { $issuer = 'https://idp.example.org/realms/demo'; $presented = (string) $this->request->getHeader('Authorization'); - if ($this->oidcService->verifyIdToken($presented, $issuer) === false) { + if ($this->oidc->verifyIdToken($presented, $issuer) === false) { return new JSONResponse(['error' => 'unauthorized'], Http::STATUS_UNAUTHORIZED); } - return new JSONResponse(['issuer' => $issuer]); } -""") - self.assertEqual(_scan(src), []) - - -# --------------------------------------------------------------------------- -# The #[NoAdminRequired] rule, and silence on ordinary controllers. -# --------------------------------------------------------------------------- - -class NoAdminRequiredWithAdminBody(unittest.TestCase): - - def test_require_admin_under_no_admin_required_is_flagged(self): - src = _controller(""" - #[NoAdminRequired] - public function destroy(int $id): JSONResponse { - $this->requireAdmin(); - - $this->registerService->delete($id); - return new JSONResponse([], 204); - } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) - - def test_is_admin_guard_with_forbidden_return_is_flagged(self): - src = _controller(""" - #[NoAdminRequired] - public function settings(): JSONResponse { - if ($this->groupManager->isAdmin($this->userId) === false) { - return new JSONResponse(['error' => 'admins only'], Http::STATUS_FORBIDDEN); - } - - return new JSONResponse($this->settingsService->all()); - } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) - - def test_yoda_comparison_is_flagged(self): - src = _controller(""" - #[NoAdminRequired] - public function purge(): JSONResponse { - if (false === $this->groupManager->isAdmin($this->userId)) { - throw new OCSForbiddenException('admins only'); - } - - $this->cacheService->purgeAll(); - return new JSONResponse(['purged' => true]); - } -""") - findings = _scan(src) - self.assertEqual(len(findings), 1, findings) - self.assertIn("rule=no-admin-required-annotation-with-admin-body", findings[0]) - - def test_a_non_admin_predicate_is_not_flagged(self): - """The guard has to be about being an admin, not merely an `if`.""" - src = _controller(""" - #[NoAdminRequired] - public function publish(int $id): JSONResponse { - if ($this->publicationService->isReviewed($id) === false) { - return new JSONResponse(['error' => 'not reviewed yet'], Http::STATUS_FORBIDDEN); - } - - return new JSONResponse($this->publicationService->publish($id)); - } -""") - self.assertEqual(_scan(src), []) - +""" + self.assertEqual(_scan(php), []) -class OrdinaryControllersProduceNoFindings(unittest.TestCase): - def test_plain_authenticated_endpoints_are_silent(self): - src = _controller(""" - #[NoAdminRequired] - public function index(): JSONResponse { - return new JSONResponse($this->objectService->findAll()); - } +class GateIsNotBlind(unittest.TestCase): + def test_the_scanner_still_reads_methods_at_all(self): + # If `_find_method_bodies` ever returns nothing, every `assertEqual([])` + # above passes. This asserts the floor directly. + php = CLASS % """ + #[PublicPage] + public function a(): JSONResponse { $this->requireAdmin(); return new JSONResponse([]); } - #[NoAdminRequired] - public function show(int $id): JSONResponse { - return new JSONResponse($this->objectService->find($id)); - } -""") - self.assertEqual(_scan(src), []) - - def test_an_admin_endpoint_with_no_attribute_is_silent(self): - src = _controller(""" - public function reindex(): JSONResponse { - $this->searchService->reindexEverything(); - return new JSONResponse(['queued' => true]); - } -""") - self.assertEqual(_scan(src), []) + #[PublicPage] + public function b(): JSONResponse { $this->requireAdmin(); return new JSONResponse([]); } +""" + self.assertEqual(len(_scan(php)), 2) if __name__ == "__main__":