fix: harden local-admin lookup against is_local-flip tamper (SEC_REVIEW F12)

`UserRepository::findLocal()` now also requires `subject IS NULL`, so a
row whose `is_local` was flipped to 1 by direct DB tampering or a
compromised "data-fix" script cannot bind the local-admin password to
the hijacked OIDC identity.

Migration 20260505100000_add_users_local_subject_invariant enforces the
same predicate at the storage layer: MySQL via a CHECK constraint;
SQLite via paired BEFORE INSERT / BEFORE UPDATE triggers that
RAISE(ABORT) on `is_local = 1 AND subject IS NOT NULL`. So an
`UPDATE users SET is_local = 1` against an OIDC row fails at the DB
before any login can bind.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

api/db/migrations/20260505100000_add_users_local_subject_invariant.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Infrastructure\Db\Migrations\BaseMigration;
+
+/**
+ * Defense-in-depth for SEC_REVIEW F12: enforce at the DB layer that a
+ * row with `is_local = 1` always has `subject IS NULL`. SPEC §6 / §8
+ * documents this invariant — local-admin rows have no OIDC subject —
+ * and `UserRepository::upsertLocal` honours it in code, but a
+ * compromised IdP that pushed an OIDC user with the local admin's
+ * `display_name` plus a later "data fix" flipping `is_local=1` would
+ * otherwise let `findLocal()` bind the local-admin password to that
+ * hijacked OIDC identity.
+ *
+ * Implementation:
+ *  - MySQL 8: a CHECK constraint added via ALTER TABLE.
+ *  - SQLite: a pair of BEFORE INSERT and BEFORE UPDATE triggers that
+ *    RAISE(ABORT) when the predicate is violated. SQLite cannot ALTER
+ *    TABLE ADD CHECK without a full table rebuild, and triggers give
+ *    equivalent runtime enforcement.
+ *
+ * If a deployment already has a row violating the invariant
+ * (is_local=1 AND subject IS NOT NULL), this migration will fail. That
+ * is the desired signal: the operator must inspect and consolidate
+ * before applying the constraint.
+ */
+final class AddUsersLocalSubjectInvariant extends BaseMigration
+{
+    private const CONSTRAINT_NAME = 'chk_users_local_subject_null';
+    private const TRIGGER_INSERT = 'trg_users_local_subject_null_insert';
+    private const TRIGGER_UPDATE = 'trg_users_local_subject_null_update';
+
+    public function up(): void
+    {
+        if ($this->isMysql()) {
+            $this->execute(
+                'ALTER TABLE users ADD CONSTRAINT ' . self::CONSTRAINT_NAME . ' '
+                . 'CHECK (NOT (is_local = 1 AND subject IS NOT NULL))'
+            );
+
+            return;
+        }
+
+        $this->execute(<<<'SQL'
+            CREATE TRIGGER trg_users_local_subject_null_insert
+            BEFORE INSERT ON users
+            FOR EACH ROW
+            WHEN NEW.is_local = 1 AND NEW.subject IS NOT NULL
+            BEGIN
+                SELECT RAISE(ABORT, 'local user must have null subject');
+            END
+        SQL);
+
+        $this->execute(<<<'SQL'
+            CREATE TRIGGER trg_users_local_subject_null_update
+            BEFORE UPDATE ON users
+            FOR EACH ROW
+            WHEN NEW.is_local = 1 AND NEW.subject IS NOT NULL
+            BEGIN
+                SELECT RAISE(ABORT, 'local user must have null subject');
+            END
+        SQL);
+    }
+
+    public function down(): void
+    {
+        if ($this->isMysql()) {
+            $this->execute(
+                'ALTER TABLE users DROP CONSTRAINT ' . self::CONSTRAINT_NAME
+            );
+
+            return;
+        }
+
+        $this->execute('DROP TRIGGER IF EXISTS ' . self::TRIGGER_INSERT);
+        $this->execute('DROP TRIGGER IF EXISTS ' . self::TRIGGER_UPDATE);
+    }
+}

api/src/Infrastructure/Auth/UserRepository.php

@@ -60,13 +60,23 @@ final class UserRepository
      * rename the local admin via `LOCAL_ADMIN_USERNAME`) and matching on
      * it would otherwise let a service-token holder mint additional Admin
      * users by varying the `username` payload (SEC_REVIEW F3).
+     *
+     * The query additionally requires `subject IS NULL` (SEC_REVIEW F12):
+     * local-admin rows have a null OIDC subject by construction. If a row
+     * exists with `is_local=1` AND a non-null subject (impossible via
+     * application code, only reachable via direct DB tampering or a
+     * compromised "data-fix" script), refuse to bind the local-admin
+     * password to that hijacked identity. The migration
+     * 20260505100000_add_users_local_subject_invariant enforces the same
+     * invariant at the DB layer (MySQL CHECK constraint; SQLite triggers).
      */
     public function findLocal(): ?User
     {
         /** @var array<string, mixed>|false $row */
         $row = $this->connection->fetchAssociative(
             'SELECT ' . self::COLUMNS . ' FROM users '
-            . 'WHERE is_local = :local ORDER BY id ASC LIMIT 1',
+            . 'WHERE is_local = :local AND subject IS NULL '
+            . 'ORDER BY id ASC LIMIT 1',
             ['local' => 1]
         );
 

api/tests/Integration/Auth/AuthEndpointsTest.php

@@ -235,6 +235,80 @@ final class AuthEndpointsTest extends AppTestCase
         ]);
     }
 
+    /**
+     * SEC_REVIEW F12 regression. Even if direct DB tampering or a
+     * compromised "data-fix" script tries to insert a row with
+     * `is_local=1` AND a non-null `subject` (an OIDC identity flagged
+     * as local), the migration
+     * 20260505100000_add_users_local_subject_invariant rejects the
+     * write — MySQL via CHECK constraint, SQLite via BEFORE INSERT
+     * trigger.
+     */
+    public function testDbLayerRejectsInsertingLocalRowWithNonNullSubject(): void
+    {
+        $this->expectException(\Doctrine\DBAL\Exception::class);
+
+        $this->db->insert('users', [
+            'subject' => 'hijacked-oidc-sub',
+            'email' => 'hijacked@example.com',
+            'display_name' => 'admin',
+            'role' => Role::Admin->value,
+            'is_local' => 1,
+        ]);
+    }
+
+    /**
+     * SEC_REVIEW F12 regression. The classic threat model is "OIDC row
+     * already exists, attacker flips its is_local to 1 via a data-fix
+     * script". The BEFORE UPDATE trigger / CHECK constraint must catch
+     * this transition too — not only fresh inserts.
+     */
+    public function testDbLayerRejectsFlippingIsLocalOnOidcRow(): void
+    {
+        $this->db->insert('users', [
+            'subject' => 'oidc-sub',
+            'email' => 'oidc@example.com',
+            'display_name' => 'admin',
+            'role' => Role::Viewer->value,
+            'is_local' => 0,
+        ]);
+        $oidcId = (int) $this->db->lastInsertId();
+
+        $this->expectException(\Doctrine\DBAL\Exception::class);
+
+        $this->db->update('users', ['is_local' => 1], ['id' => $oidcId]);
+    }
+
+    /**
+     * SEC_REVIEW F12 regression. Belt-and-suspenders to the DB-level
+     * constraint: `findLocal()` must additionally filter by
+     * `subject IS NULL`, so that even if some future code path bypasses
+     * the constraint (or it is dropped during ops), a hijacked OIDC row
+     * cannot bind to local-admin login. We cannot exercise this via a
+     * direct insert (the DB constraint blocks it), so we drop the
+     * constraint for the duration of the test, inject the malformed
+     * row, and assert findLocal does not return it.
+     */
+    public function testFindLocalIgnoresHijackedRowEvenIfDbConstraintIsBypassed(): void
+    {
+        // Bypass the SQLite triggers so we can simulate a tampered DB.
+        $this->db->executeStatement('DROP TRIGGER IF EXISTS trg_users_local_subject_null_insert');
+        $this->db->executeStatement('DROP TRIGGER IF EXISTS trg_users_local_subject_null_update');
+
+        $this->db->insert('users', [
+            'subject' => 'oidc-sub-hijacked',
+            'email' => 'hijack@example.com',
+            'display_name' => 'admin',
+            'role' => Role::Admin->value,
+            'is_local' => 1,
+        ]);
+
+        /** @var \App\Infrastructure\Auth\UserRepository $users */
+        $users = $this->container->get(\App\Infrastructure\Auth\UserRepository::class);
+
+        self::assertNull($users->findLocal(), 'findLocal must skip rows with non-null subject');
+    }
+
     public function testUpsertLocalIsIdempotent(): void
     {
         $token = $this->createToken(TokenKind::Service);