fix(authz): close fail-open db store defects from review round 1

C1/C2: grant() wrapped its delete+insert in db.tx on a shared,
unserialized sqlite connection, so a concurrent bare write from another
method (e.g. revokeRole) got swept into the open transaction and
discarded on rollback - a revoke could report success while the
privilege survived. Also broke concurrent grants on distinct keys
("cannot start a transaction within a transaction"). Replaced with
single-statement upserts (ON CONFLICT / ON DUPLICATE KEY UPDATE),
atomic without a transaction.

I1: assignRole's check-then-act SELECT lost 19/20 concurrent identical
calls to a UNIQUE violation; switched to ON CONFLICT DO NOTHING.

I2: an unrecognised `effect` value was dropped from both the grant and
deny buckets on read. Added a CHECK constraint and made anything not
literally "allow" count as a deny (fail closed).

I3: ensureAuthzTables defaulted to sqlite instead of the Db's own
dialect. I4: scopeKey now refuses an explicitly empty tenantId rather
than treating it as global (shared with the memory adapter). I5: added
migrations.test.ts asserting the generated DDL per dialect, including
MySQL's binary collation on identity columns. M1: DDL is now a
statement list instead of a blob split on a formatting-dependent
separator. M3: declared @wrnexus/db as a workspace dependency.

Extends the conformance suite with four concurrency/empty-scope tests
(23 total, up from 19) that all three adapters now pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

packages/authz/package.json

@@ -7,5 +7,8 @@
   "exports": {
     ".": "./src/index.ts",
     "./db": "./src/db.ts"
+  },
+  "dependencies": {
+    "@wrnexus/db": "workspace:*"
   }
 }

packages/authz/src/db.ts

@@ -1,6 +1,6 @@
 import type { Db, Dialect } from "@wrnexus/db";
 import { authzMigrationSql } from "./migrations.ts";
-import { scopeKey, type GrantEffect, type PermissionStore } from "./store.ts";
+import { scopeKey, type PermissionStore } from "./store.ts";
 import type { AuthzScope, SubjectAssignments } from "./types.ts";
 
 // Re-exported so `@wrnexus/authz/db` is the single entry point for everything
@@ -8,72 +8,84 @@ import type { AuthzScope, SubjectAssignments } from "./types.ts";
 export { authzMigrationSql } from "./migrations.ts";
 
 /** Create the tables if absent. Production apps should use a real migration. */
-export async function ensureAuthzTables(db: Db, dialect: Dialect = "sqlite"): Promise<void> {
-  for (const statement of authzMigrationSql(dialect).up.split(";\n\n")) {
-    const sql = statement.trim();
-    if (sql) await db.exec(sql.endsWith(";") ? sql : `${sql};`);
-  }
+export async function ensureAuthzTables(
+  db: Db,
+  dialect: Dialect = db.driver.dialect,
+): Promise<void> {
+  for (const statement of authzMigrationSql(dialect).up) await db.exec(statement);
+}
+
+/** Positional placeholder for the dialect: postgres numbers them, others use "?". */
+function ph(dialect: Dialect, index: number): string {
+  return dialect === "postgres" ? `$${index}` : "?";
 }
 
 export function dbPermissionStore(db: Db): PermissionStore {
+  const dialect = db.driver.dialect;
+  const p = (n: number) => ph(dialect, n);
+  // Single-statement upserts. A transaction here would be worse than useless:
+  // the drivers run BEGIN on one shared connection, so an open transaction
+  // swallows any concurrent write from another method and discards it on
+  // rollback - a revoke would resolve successfully while the role survived.
+  const onConflict = (columns: string, update: string) =>
+    dialect === "mysql"
+      ? ` ON DUPLICATE KEY UPDATE ${update}`
+      : ` ON CONFLICT (${columns}) DO UPDATE SET ${update}`;
+  const onConflictIgnore = (columns: string) =>
+    dialect === "mysql"
+      ? " ON DUPLICATE KEY UPDATE id = id"
+      : ` ON CONFLICT (${columns}) DO NOTHING`;
+
   return {
     async assignmentsFor(subjectId: string, scope?: AuthzScope): Promise<SubjectAssignments> {
       const key = scopeKey(scope);
       // A request inside a tenant sees global rows plus that tenant's rows.
       const roleRows = await db.all<{ role: string }>(
-        "SELECT role FROM _wrn_authz_assignment WHERE subject_id = ? AND (scope = '' OR scope = ?)",
+        `SELECT role FROM _wrn_authz_assignment WHERE subject_id = ${p(1)} AND (scope = '' OR scope = ${p(2)})`,
         [subjectId, key],
       );
-      const grantRows = await db.all<{ permission: string; effect: GrantEffect }>(
-        "SELECT permission, effect FROM _wrn_authz_grant WHERE subject_id = ? AND (scope = '' OR scope = ?)",
+      const grantRows = await db.all<{ permission: string; effect: string }>(
+        `SELECT permission, effect FROM _wrn_authz_grant WHERE subject_id = ${p(1)} AND (scope = '' OR scope = ${p(2)})`,
         [subjectId, key],
       );
       return {
         roles: roleRows.map((row) => row.role),
         grants: grantRows.filter((r) => r.effect === "allow").map((r) => r.permission),
-        denies: grantRows.filter((r) => r.effect === "deny").map((r) => r.permission),
+        // Anything that is not literally "allow" counts as a deny, so a
+        // corrupted or mis-cased effect fails closed rather than vanishing.
+        denies: grantRows.filter((r) => r.effect !== "allow").map((r) => r.permission),
       };
     },
 
     async assignRole(subjectId, role, scope) {
-      const key = scopeKey(scope);
-      const existing = await db.all<{ id: number }>(
-        "SELECT id FROM _wrn_authz_assignment WHERE subject_id = ? AND scope = ? AND role = ?",
-        [subjectId, key, role],
-      );
-      if (existing.length) return;
       await db.exec(
-        "INSERT INTO _wrn_authz_assignment (subject_id, scope, role) VALUES (?, ?, ?)",
-        [subjectId, key, role],
+        `INSERT INTO _wrn_authz_assignment (subject_id, scope, role) VALUES (${p(1)}, ${p(2)}, ${p(3)})` +
+          onConflictIgnore("subject_id, scope, role"),
+        [subjectId, scopeKey(scope), role],
       );
     },
 
     async revokeRole(subjectId, role, scope) {
       await db.exec(
-        "DELETE FROM _wrn_authz_assignment WHERE subject_id = ? AND scope = ? AND role = ?",
+        `DELETE FROM _wrn_authz_assignment WHERE subject_id = ${p(1)} AND scope = ${p(2)} AND role = ${p(3)}`,
         [subjectId, scopeKey(scope), role],
       );
     },
 
     async grant(subjectId, permission, effect, scope) {
-      const key = scopeKey(scope);
-      // Re-granting replaces the effect. Do the delete+insert inside a
-      // transaction so a failed insert can't leave the row missing.
-      await db.tx(async (tx) => {
-        await tx.exec(
-          "DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?",
-          [subjectId, key, permission],
-        );
-        await tx.exec(
-          "INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (?, ?, ?, ?)",
-          [subjectId, key, permission, effect],
-        );
-      });
+      await db.exec(
+        `INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (${p(1)}, ${p(2)}, ${p(3)}, ${p(4)})` +
+          onConflict(
+            "subject_id, scope, permission",
+            "effect = " + (dialect === "mysql" ? "VALUES(effect)" : "excluded.effect"),
+          ),
+        [subjectId, scopeKey(scope), permission, effect],
+      );
     },
 
     async revokeGrant(subjectId, permission, scope) {
       await db.exec(
-        "DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?",
+        `DELETE FROM _wrn_authz_grant WHERE subject_id = ${p(1)} AND scope = ${p(2)} AND permission = ${p(3)}`,
         [subjectId, scopeKey(scope), permission],
       );
     },
@@ -81,8 +93,8 @@ export function dbPermissionStore(db: Db): PermissionStore {
     async listSubjects(scope) {
       const key = scopeKey(scope);
       const rows = await db.all<{ subject_id: string }>(
-        "SELECT subject_id FROM _wrn_authz_assignment WHERE scope = ? " +
-          "UNION SELECT subject_id FROM _wrn_authz_grant WHERE scope = ?",
+        `SELECT subject_id FROM _wrn_authz_assignment WHERE scope = ${p(1)} ` +
+          `UNION SELECT subject_id FROM _wrn_authz_grant WHERE scope = ${p(2)}`,
         [key, key],
       );
       return [...new Set(rows.map((row) => row.subject_id))];

packages/authz/src/migrations.ts

@@ -1,11 +1,17 @@
 import type { Dialect } from "@wrnexus/db";
 
 /**
- * DDL for the two assignment tables. `scope` holds a tenant id, or the empty
- * string for a global assignment, so the unique constraints work on every
- * dialect (NULL is not comparable in a UNIQUE index).
+ * DDL for the two assignment tables, as a list of statements rather than one
+ * blob: splitting a blob on a separator makes runtime correctness depend on
+ * source formatting, and only the sqlite driver accepts multi-statement exec.
+ *
+ * `scope` holds a tenant id, or the empty string for a global assignment, so
+ * the unique constraints work on every dialect (NULL is not comparable in a
+ * UNIQUE index). `effect` is CHECK-constrained: an unrecognised value would
+ * otherwise be dropped from both the grant and deny buckets on read, silently
+ * turning a deny into a no-op.
  */
-export function authzMigrationSql(dialect: Dialect): { up: string; down: string } {
+export function authzMigrationSql(dialect: Dialect): { up: string[]; down: string[] } {
   const id =
     dialect === "postgres"
       ? "SERIAL PRIMARY KEY"
@@ -13,31 +19,31 @@ export function authzMigrationSql(dialect: Dialect): { up: string; down: string
         ? "INT AUTO_INCREMENT PRIMARY KEY"
         : "INTEGER PRIMARY KEY AUTOINCREMENT";
   const timestamp = dialect === "sqlite" ? "TEXT" : "TIMESTAMP";
-  const now = "CURRENT_TIMESTAMP";
+  // MySQL's default collation is case- and accent-insensitive, which would let
+  // tenant "T1" match "t1" and collapse roles "admin"/"Admin" onto one row.
+  const exact = dialect === "mysql" ? " COLLATE utf8mb4_bin" : "";
+  const key = `VARCHAR(255)${exact} NOT NULL`;
 
-  const up = `CREATE TABLE IF NOT EXISTS _wrn_authz_assignment (
+  return {
+    up: [
+      `CREATE TABLE IF NOT EXISTS _wrn_authz_assignment (
   id ${id},
-  subject_id VARCHAR(255) NOT NULL,
-  scope VARCHAR(255) NOT NULL DEFAULT '',
-  role VARCHAR(255) NOT NULL,
-  granted_by VARCHAR(255),
-  created_at ${timestamp} NOT NULL DEFAULT ${now},
+  subject_id ${key},
+  scope ${key} DEFAULT '',
+  role ${key},
+  created_at ${timestamp} NOT NULL DEFAULT CURRENT_TIMESTAMP,
   CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)
-);
-
-CREATE TABLE IF NOT EXISTS _wrn_authz_grant (
+)`,
+      `CREATE TABLE IF NOT EXISTS _wrn_authz_grant (
   id ${id},
-  subject_id VARCHAR(255) NOT NULL,
-  scope VARCHAR(255) NOT NULL DEFAULT '',
-  permission VARCHAR(255) NOT NULL,
-  effect VARCHAR(16) NOT NULL,
-  granted_by VARCHAR(255),
-  created_at ${timestamp} NOT NULL DEFAULT ${now},
+  subject_id ${key},
+  scope ${key} DEFAULT '',
+  permission ${key},
+  effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny')),
+  created_at ${timestamp} NOT NULL DEFAULT CURRENT_TIMESTAMP,
   CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)
-);`;
-
-  const down = `DROP TABLE IF EXISTS _wrn_authz_grant;
-DROP TABLE IF EXISTS _wrn_authz_assignment;`;
-
-  return { up, down };
+)`,
+    ],
+    down: ["DROP TABLE IF EXISTS _wrn_authz_grant", "DROP TABLE IF EXISTS _wrn_authz_assignment"],
+  };
 }

packages/authz/src/store.ts

@@ -16,9 +16,21 @@ export interface PermissionStore {
   listSubjects(scope?: AuthzScope): Promise<string[]>;
 }
 
-/** Global assignments are stored under the empty-string scope key. */
+/**
+ * Global assignments are stored under the empty-string scope key. An OMITTED
+ * scope means global; an explicitly EMPTY tenantId is refused, because it is
+ * indistinguishable from global and would let a caller who controls the tenant
+ * id read and write global assignments.
+ */
 export function scopeKey(scope?: AuthzScope): string {
-  return scope?.tenantId ?? "";
+  const tenantId = scope?.tenantId;
+  if (tenantId === undefined) return "";
+  if (tenantId === "") {
+    throw new Error(
+      "WRN-AUTHZ-SCOPE: tenantId must not be empty; omit the scope for a global assignment.",
+    );
+  }
+  return tenantId;
 }
 
 interface Row {

packages/authz/test/migrations.test.ts

@@ -0,0 +1,75 @@
+import { describe, expect, test } from "bun:test";
+import { authzMigrationSql } from "../src/migrations.ts";
+
+/**
+ * The postgres/mysql DDL is generated but never exercised against a real
+ * server in this repo, so it has to be asserted statically: the id column
+ * type, the `effect` CHECK constraint (an unrecognised value must not vanish
+ * from both the grant and deny buckets), the MySQL binary collation (so
+ * tenant "T1" cannot match "t1" and role "admin" cannot collapse with
+ * "Admin"), and both UNIQUE constraints, per dialect.
+ */
+describe("authzMigrationSql", () => {
+  test("sqlite: autoincrement id, no collation, both constraints", () => {
+    const { up, down } = authzMigrationSql("sqlite");
+    expect(up).toHaveLength(2);
+    const [assignment, grant] = up;
+
+    expect(assignment).toContain("id INTEGER PRIMARY KEY AUTOINCREMENT");
+    expect(assignment).toContain(
+      "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)",
+    );
+    expect(assignment).not.toContain("COLLATE");
+
+    expect(grant).toContain("id INTEGER PRIMARY KEY AUTOINCREMENT");
+    expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))");
+    expect(grant).toContain(
+      "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)",
+    );
+    expect(grant).not.toContain("COLLATE");
+
+    expect(down).toEqual([
+      "DROP TABLE IF EXISTS _wrn_authz_grant",
+      "DROP TABLE IF EXISTS _wrn_authz_assignment",
+    ]);
+  });
+
+  test("postgres: SERIAL id, no collation, both constraints", () => {
+    const { up } = authzMigrationSql("postgres");
+    const [assignment, grant] = up;
+
+    expect(assignment).toContain("id SERIAL PRIMARY KEY");
+    expect(assignment).toContain(
+      "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)",
+    );
+    expect(assignment).not.toContain("COLLATE");
+
+    expect(grant).toContain("id SERIAL PRIMARY KEY");
+    expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))");
+    expect(grant).toContain(
+      "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)",
+    );
+    expect(grant).not.toContain("COLLATE");
+  });
+
+  test("mysql: AUTO_INCREMENT id, binary collation on identity columns, both constraints", () => {
+    const { up } = authzMigrationSql("mysql");
+    const [assignment, grant] = up;
+
+    expect(assignment).toContain("id INT AUTO_INCREMENT PRIMARY KEY");
+    expect(assignment).toContain("subject_id VARCHAR(255) COLLATE utf8mb4_bin NOT NULL");
+    expect(assignment).toContain("scope VARCHAR(255) COLLATE utf8mb4_bin NOT NULL DEFAULT ''");
+    expect(assignment).toContain("role VARCHAR(255) COLLATE utf8mb4_bin NOT NULL");
+    expect(assignment).toContain(
+      "CONSTRAINT _wrn_authz_assignment_unique UNIQUE (subject_id, scope, role)",
+    );
+
+    expect(grant).toContain("id INT AUTO_INCREMENT PRIMARY KEY");
+    expect(grant).toContain("subject_id VARCHAR(255) COLLATE utf8mb4_bin NOT NULL");
+    expect(grant).toContain("permission VARCHAR(255) COLLATE utf8mb4_bin NOT NULL");
+    expect(grant).toContain("effect VARCHAR(16) NOT NULL CHECK (effect IN ('allow', 'deny'))");
+    expect(grant).toContain(
+      "CONSTRAINT _wrn_authz_grant_unique UNIQUE (subject_id, scope, permission)",
+    );
+  });
+});

packages/authz/test/store-conformance.ts

@@ -129,6 +129,42 @@ export function runStoreConformance(name: string, makeStore: () => Promise<Permi
       expect((await store.listSubjects({ tenantId: "t1" })).sort()).toEqual(["u1", "u2"]);
     });
 
+    test("an explicitly empty tenantId is refused, not treated as global", async () => {
+      await store.assignRole("g1", "viewer");
+      // Otherwise a caller who controls the tenant id reaches global scope.
+      await expect(store.assignmentsFor("g1", { tenantId: "" })).rejects.toThrow(/tenantId/);
+      await expect(store.assignRole("g1", "admin", { tenantId: "" })).rejects.toThrow(/tenantId/);
+    });
+
+    test("concurrent identical assignRole calls all resolve", async () => {
+      // Check-then-act loses this race; the UNIQUE constraint then rejects
+      // every loser even though the desired end state was already reached.
+      await Promise.all(Array.from({ length: 20 }, () => store.assignRole("u1", "editor")));
+      expect((await store.assignmentsFor("u1")).roles).toEqual(["editor"]);
+    });
+
+    test("concurrent grants on distinct keys all resolve", async () => {
+      await Promise.all([
+        store.grant("u1", "post:read", "allow"),
+        store.grant("u1", "post:write", "allow"),
+        store.grant("u1", "post:delete", "deny"),
+      ]);
+      const assignments = await store.assignmentsFor("u1");
+      expect(assignments.grants.sort()).toEqual(["post:read", "post:write"]);
+      expect(assignments.denies).toEqual(["post:delete"]);
+    });
+
+    test("a concurrent write is not lost to another method's failure", async () => {
+      // A store that wraps one method in a transaction on a shared connection
+      // will roll back this unrelated write and still resolve successfully.
+      await store.assignRole("victim", "admin");
+      await Promise.all([
+        store.revokeRole("victim", "admin"),
+        store.grant("other", "post:read", "allow").catch(() => undefined),
+      ]);
+      expect((await store.assignmentsFor("victim")).roles).toEqual([]);
+    });
+
     test("listSubjects with no scope returns global assignees only", async () => {
       await store.assignRole("g1", "viewer");
       await store.assignRole("s1", "editor", { tenantId: "t1" });