WorkRoot/WRNexusJS
2
0
Fork 0
Code Issues Pull Requests Actions 46 Packages Projects Releases Wiki Activity

docs: fix fail-open concurrency and effect handling in the Task 11 plan

Browse Source 
The database store review found two Critical defects and several Important
ones, all reachable in production.

grant() was wrapped in db.tx for atomicity. The sqlite driver runs a bare
BEGIN on one shared connection with no serialization, so an open transaction
swallows any concurrent write from another method and discards it on
rollback. Demonstrated: revokeRole resolved with no error while the role
survived - a security-critical revoke reporting success with the privilege
retained. Concurrent grants also rejected outright with "cannot start a
transaction within a transaction". Replaced with single-statement upserts,
which are atomic without a transaction; assignRole likewise drops its
check-then-act SELECT for ON CONFLICT DO NOTHING, which was rejecting 19 of
20 concurrent identical calls.

effect had no CHECK constraint and assignmentsFor classified by exact
equality, so a mis-cased or corrupted value was dropped from BOTH buckets -
a deny row that silently stopped denying. Added the constraint and made
anything that is not literally "allow" count as a deny.

scopeKey now refuses an explicitly empty tenantId rather than treating it as
global, which otherwise let a caller who controls the tenant id read and
write global assignments.

Also: ensureAuthzTables takes the dialect from db.driver.dialect instead of
defaulting to sqlite; the DDL is a list of statements rather than a blob
split on a formatting-dependent separator; MySQL identity columns get a
binary collation so tenant "T1" cannot match "t1"; and postgres placeholders
are numbered.

Adds four conformance tests for the concurrency and empty-scope cases. The
suite was entirely sequential and structurally could not catch any of this.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Clintchiz
2026-08-04 20:33:45 +05:30
co-authored by Claude Opus 5
parent 3fa3fce5df
commit f19462dff0
1 changed files with 129 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/2026-08-04-authz-permissions-implementation.md
+129 -54
View File
@@ -598,6 +598,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" });
@@ -646,9 +682,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 {
@@ -2645,7 +2693,18 @@ import type { Dialect } from "@wrnexus/db";
* string for a global assignment, so the unique constraints work on every
* dialect (NULL is not comparable in a UNIQUE index).
*/
export function authzMigrationSql(dialect: Dialect): { up: string; down: string } {
/**
* 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[] } {
const id =
dialect === "postgres"
? "SERIAL PRIMARY KEY"
@@ -2653,33 +2712,33 @@ 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 = dialect === "sqlite" ? "CURRENT_TIMESTAMP" : "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"],
};
}
```
@@ -2698,69 +2757,85 @@ 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};`);
}
/** Create the tables if absent. Production apps should use a real migration. */
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, so delete then insert.
await db.exec(
"DELETE FROM _wrn_authz_grant WHERE subject_id = ? AND scope = ? AND permission = ?",
[subjectId, key, permission],
);
await db.exec(
"INSERT INTO _wrn_authz_grant (subject_id, scope, permission, effect) VALUES (?, ?, ?, ?)",
[subjectId, key, permission, effect],
`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],
);
},
@@ -2768,8 +2843,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))];