fix(authz): guard scopeKey's tenantId type, add deterministic C1/C2 guard
N1: scopeKey guarded the empty-string VALUE but not the TYPE. A
non-string tenantId (null, 0, false, an object) flowed through
un-normalised, and the adapters disagreed about the result - db
rejects null on NOT NULL, memory accepts it as an unreachable row; 0
and false stringify differently and could collide. Now
`typeof tenantId !== "string" || tenantId === ""` is refused with the
same WRN-AUTHZ-SCOPE error. Added a conformance case covering
null/0/false/{}.
N2: nothing failed if grant() were re-wrapped in db.tx, reintroducing
the shared-connection rollback from C1/C2 - timing-based tests can't
reliably prove a transaction is never opened. Added
db-no-transaction.test.ts: a fake Db with a spied driver.transaction
and statement-recording all/exec, driving every PermissionStore method
and asserting zero transaction calls and no "BEGIN" in any recorded
statement. Verified it fails when grant() is temporarily re-wrapped in
db.tx, then restored.
Also documents two things in db.ts as comments only: the UNIQUE
constraints are now load-bearing for ON CONFLICT/ON DUPLICATE KEY
target inference, and MySQL's VALUES(effect) upsert syntax is
deprecated since 8.0.20 (no MySQL server in CI to catch its removal).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,85 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import type { Db, Dialect, Driver, ExecResult, Row, TxHandle } from "@wrnexus/db";
|
||||
import { dbPermissionStore } from "../src/db.ts";
|
||||
|
||||
/**
|
||||
* A deterministic regression guard for C1/C2 (round-1 review): grant() used to
|
||||
* wrap its delete-then-insert in db.tx, and the sqlite driver runs a bare
|
||||
* BEGIN on one shared, unserialized connection - so an open transaction there
|
||||
* could sweep in and discard a concurrent bare write from another method.
|
||||
* Timing-based tests can't reliably prove the absence of that; this can,
|
||||
* because it needs no concurrency at all - it just asserts the store never
|
||||
* asks the driver to open a transaction in the first place.
|
||||
*/
|
||||
function makeFakeDb(): { db: Db; statements: string[]; transactionCalls: number } {
|
||||
const statements: string[] = [];
|
||||
const stats = { transactionCalls: 0 };
|
||||
|
||||
const driver: Driver = {
|
||||
dialect: "sqlite" as Dialect,
|
||||
async query(sql: string): Promise<Row[]> {
|
||||
statements.push(sql);
|
||||
return [];
|
||||
},
|
||||
async exec(sql: string): Promise<ExecResult> {
|
||||
statements.push(sql);
|
||||
return { changes: 0 };
|
||||
},
|
||||
async transaction<T>(fn: (tx: TxHandle) => Promise<T>): Promise<T> {
|
||||
// The spy: this must never be called by a transaction-free store.
|
||||
stats.transactionCalls++;
|
||||
statements.push("BEGIN");
|
||||
return fn(driver);
|
||||
},
|
||||
close() {},
|
||||
};
|
||||
|
||||
const db: Db = {
|
||||
driver,
|
||||
async all<T = Row>(sql: string): Promise<T[]> {
|
||||
statements.push(sql);
|
||||
return [];
|
||||
},
|
||||
async one<T = Row>(sql: string): Promise<T | null> {
|
||||
statements.push(sql);
|
||||
return null;
|
||||
},
|
||||
async exec(sql: string): Promise<ExecResult> {
|
||||
statements.push(sql);
|
||||
return { changes: 0 };
|
||||
},
|
||||
async tx<T>(fn: (tx: Db) => Promise<T>): Promise<T> {
|
||||
// Routed through the same driver.transaction spy a real Db would use.
|
||||
return driver.transaction(() => fn(db));
|
||||
},
|
||||
async createTable() {},
|
||||
close() {},
|
||||
};
|
||||
|
||||
return {
|
||||
db,
|
||||
statements,
|
||||
get transactionCalls() {
|
||||
return stats.transactionCalls;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe("dbPermissionStore opens no transaction", () => {
|
||||
test("the store opens no transaction: a shared-connection rollback would discard concurrent writes from other methods", async () => {
|
||||
const fake = makeFakeDb();
|
||||
const store = dbPermissionStore(fake.db);
|
||||
|
||||
await store.assignmentsFor("u1");
|
||||
await store.assignRole("u1", "editor");
|
||||
await store.revokeRole("u1", "editor");
|
||||
await store.grant("u1", "post:write", "allow");
|
||||
await store.revokeGrant("u1", "post:write");
|
||||
await store.listSubjects();
|
||||
|
||||
expect(fake.transactionCalls).toBe(0);
|
||||
for (const sql of fake.statements) {
|
||||
expect(sql).not.toContain("BEGIN");
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -136,6 +136,15 @@ export function runStoreConformance(name: string, makeStore: () => Promise<Permi
|
||||
await expect(store.assignRole("g1", "admin", { tenantId: "" })).rejects.toThrow(/tenantId/);
|
||||
});
|
||||
|
||||
test("a non-string tenantId is refused", async () => {
|
||||
// Same class as the empty-string case: the caller controls this value.
|
||||
for (const bad of [null, 0, false, {}]) {
|
||||
await expect(store.assignmentsFor("u1", { tenantId: bad as never })).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.
|
||||
|
||||
Reference in New Issue
Block a user