Repository navigation
refactor(sql-fs): centralize sandbox fencing policy #274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "sql-fs-api": patch | ||
| --- | ||
|
|
||
| Centralize Postgres sandbox fencing while preserving transaction context, lock order, and statement counts. Add regression coverage for stale writes and concurrent transactions, and fix verification commands that could fail under shell pipefail. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { fileURLToPath } from "node:url"; | |
| import * as ts from "typescript/unstable/sync"; | ||
| import * as ast from "typescript/unstable/ast"; | ||
|
|
||
| const { API, SymbolFlags } = ts; | ||
| const { API, SignatureKind, SymbolFlags, TypeFlags } = ts; | ||
| const { SyntaxKind } = ast; | ||
|
|
||
| const workerImportAllowlist = new Map([ | ||
|
|
@@ -44,6 +44,40 @@ for (const importer of [ | |
|
|
||
| const scopeOperations = new Set(["beginScope", "endScope", "abortScope"]); | ||
|
|
||
| const fencePolicyConsumers = new Set([ | ||
| "src/sql-fs/dialects/postgres/namespace.ts", | ||
| "src/sql-fs/dialects/postgres/ingest.ts", | ||
| ]); | ||
| const fencePolicyStatements = [/\bapp\.sandbox_epoch\b/i, /\bversion\s*=\s*(?:\w+\.)?version\s*\+/i]; | ||
|
|
||
| function forbiddenSqlApiAccess(node, checker) { | ||
| let receiver; | ||
| let name; | ||
| if (ast.isPropertyAccessExpression(node)) { | ||
| receiver = node.expression; | ||
| name = node.name.text; | ||
| } else if (ast.isElementAccessExpression(node) && ast.isStringLiteral(node.argumentExpression)) { | ||
| receiver = node.expression; | ||
| name = node.argumentExpression.text; | ||
| } else if (ast.isBindingElement(node) && ast.isObjectBindingPattern(node.parent)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Assignment destructuring bypasses this raw-SQL check. Handle assignment-pattern destructuring and validate its source type before allowing the extracted Prompt for AI agents |
||
| receiver = node.parent; | ||
| const property = node.propertyName ?? node.name; | ||
| if (ast.isIdentifier(property) || ast.isStringLiteral(property)) name = property.text; | ||
| } | ||
| if (name !== "unsafe" && name !== "file") return false; | ||
| const type = checker.getTypeAtLocation(receiver); | ||
| return ( | ||
| type === undefined || | ||
| (type.flags & (TypeFlags.Any | TypeFlags.Unknown)) !== 0 || | ||
| checker.getSignaturesOfType(type, SignatureKind.Call).length > 0 | ||
| ); | ||
| } | ||
|
|
||
| function templateLiteralText(template) { | ||
| if (ast.isNoSubstitutionTemplateLiteral(template)) return template.text; | ||
| return [template.head.text, ...template.templateSpans.map((span) => span.literal.text)].join(" ${} "); | ||
| } | ||
|
|
||
| function relative(root, filename) { | ||
| return path.relative(root, filename).split(path.sep).join("/"); | ||
| } | ||
|
|
@@ -414,6 +448,34 @@ export function checkArchitectureBoundaries(root) { | |
| } | ||
| } | ||
|
|
||
| if (fencePolicyConsumers.has(sourcePath)) { | ||
| if (forbiddenSqlApiAccess(node, checker)) { | ||
| diagnostics.push( | ||
| diagnostic( | ||
| absoluteRoot, | ||
| sourceFile, | ||
| node, | ||
| "FENCE_POLICY_RAW_SQL", | ||
| "tx.unsafe and tx.file bypass fence-policy inspection; use tagged SQL and safe value builders", | ||
| ), | ||
| ); | ||
| } | ||
| if ( | ||
| ast.isTaggedTemplateExpression(node) && | ||
| fencePolicyStatements.some((pattern) => pattern.test(templateLiteralText(node.template))) | ||
| ) { | ||
| diagnostics.push( | ||
| diagnostic( | ||
| absoluteRoot, | ||
| sourceFile, | ||
| node, | ||
| "FENCE_POLICY_OWNER", | ||
| "epoch acceptance, epoch publication, and sandboxes.version advancement belong to the fence policy in sandbox.ts; use fenceAndAdvance or compositeFenceAndAdvance", | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (ast.isCallExpression(node)) { | ||
| const expression = unwrapParentheses(node.expression); | ||
| if ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -188,3 +188,112 @@ it("permits literal non-worker imports and rejects indirect scope method calls", | |||||
| ["SCRIPT_SCOPE_COMPLETION_OWNER", "SCRIPT_SCOPE_COMPLETION_OWNER", "SCRIPT_SCOPE_COMPLETION_OWNER"], | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| const sqlTag = "declare const tx: (strings: TemplateStringsArray, ...values: unknown[]) => unknown;\n"; | ||||||
|
|
||||||
| it("rejects fence policy copied into namespace and ingest SQL", () => { | ||||||
| const root = project({ | ||||||
| "src/sql-fs/dialects/postgres/namespace.ts": `${sqlTag}export function copy(id: string, epoch: string | null) { | ||||||
| tx\`WITH ctx AS (SELECT s.version FROM sandboxes s WHERE s.id = \${id} | ||||||
| AND s.version = COALESCE(\${epoch}::bigint, NULLIF(current_setting('app.sandbox_epoch', true), '')::bigint, s.version)) | ||||||
| SELECT 1 FROM ctx\`; | ||||||
| tx\`UPDATE sandboxes s | ||||||
| SET version = s.version + 1 WHERE s.id = \${id}\`; | ||||||
| } | ||||||
| `, | ||||||
| "src/sql-fs/dialects/postgres/ingest.ts": `${sqlTag}export function publish(version: string) { | ||||||
| tx\`SELECT set_config('app.sandbox_epoch', \${version}, true)\`; | ||||||
| } | ||||||
| `, | ||||||
| }); | ||||||
| const diagnostics = checkArchitectureBoundaries(root).filter(({ code }) => code === "FENCE_POLICY_OWNER"); | ||||||
| assert.deepEqual( | ||||||
| diagnostics.map(({ file, line }) => [file, line]), | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This assertion relies on an undocumented filesystem traversal order. Sort the diagnostics before comparing them, or make checker output ordering deterministic, to prevent platform-dependent test failures. Prompt for AI agents
Suggested change
|
||||||
| [ | ||||||
| ["src/sql-fs/dialects/postgres/ingest.ts", 3], | ||||||
| ["src/sql-fs/dialects/postgres/namespace.ts", 3], | ||||||
| ["src/sql-fs/dialects/postgres/namespace.ts", 6], | ||||||
| ], | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| it("permits epoch consumers, version reads, and the fence policy owner", () => { | ||||||
| const root = project({ | ||||||
| "src/sql-fs/dialects/postgres/sandbox.ts": `${sqlTag}export function fence(id: string) { | ||||||
| return tx\`ctx AS (SELECT s.version FROM sandboxes s WHERE s.version = NULLIF(current_setting('app.sandbox_epoch', true), '')::bigint), | ||||||
| advanced AS (UPDATE sandboxes s SET version = s.version + 1 FROM ctx WHERE s.id = \${id} RETURNING s.version)\`; | ||||||
| } | ||||||
| `, | ||||||
| "src/sql-fs/dialects/postgres/namespace.ts": `${sqlTag}// app.sandbox_epoch is published by the fence policy | ||||||
| const prose = "UPDATE sandboxes SET version = version + 1"; | ||||||
| export function consume(id: string, fence: unknown) { | ||||||
| tx\`WITH \${fence}, inserted AS (INSERT INTO inodes (sandbox_id) SELECT \${id} FROM epoch RETURNING id) | ||||||
| SELECT (SELECT count(*) FROM epoch)::int AS fenced, (SELECT id FROM inserted) AS id | ||||||
| FROM sandboxes WHERE id = \${id}\`; | ||||||
| tx\`SELECT version FROM sandboxes WHERE id = \${id}\`; | ||||||
| tx\`UPDATE sandboxes SET name = \${prose} WHERE id = \${id}\`; | ||||||
| tx\`UPDATE sandboxes SET name = \${prose} WHERE version = \${id}\`; | ||||||
| } | ||||||
| `, | ||||||
| }); | ||||||
| assert.deepEqual(checkArchitectureBoundaries(root), []); | ||||||
| }); | ||||||
|
|
||||||
| const sqlCapabilities = `declare const tx: { | ||||||
| (strings: TemplateStringsArray, ...values: unknown[]): unknown; | ||||||
| (values: unknown[] | Record<string, unknown>): unknown; | ||||||
| unsafe(query: string): unknown; | ||||||
| file(path: string): unknown; | ||||||
| }; | ||||||
| `; | ||||||
|
|
||||||
| it("rejects direct, extracted, destructured, and aliased raw SQL capabilities in both fence consumers", () => { | ||||||
| const accesses = [ | ||||||
| 'tx.unsafe("UPDATE sandboxes SET version = version + 1")', | ||||||
| 'tx["unsafe"]("SELECT 1")', | ||||||
| 'const run = tx.unsafe; run("SELECT 1")', | ||||||
| 'const run = tx["unsafe"]; run("SELECT 1")', | ||||||
| 'const { unsafe: run } = tx; run("SELECT 1")', | ||||||
| 'const { "unsafe": run } = tx; run("SELECT 1")', | ||||||
| 'const db = tx; db.unsafe("SELECT 1")', | ||||||
| 'tx.file("query.sql")', | ||||||
| 'tx["file"]("query.sql")', | ||||||
| 'const run = tx.file; run("query.sql")', | ||||||
| 'const run = tx["file"]; run("query.sql")', | ||||||
| 'const { file: run } = tx; run("query.sql")', | ||||||
| 'const db = tx; db.file("query.sql")', | ||||||
| ]; | ||||||
| const body = accesses.map((access) => `{ ${access}; }`).join("\n"); | ||||||
| const files = ["namespace", "ingest"].map((name) => `src/sql-fs/dialects/postgres/${name}.ts`); | ||||||
| const root = project(Object.fromEntries(files.map((file) => [file, sqlCapabilities + body]))); | ||||||
| const diagnostics = checkArchitectureBoundaries(root); | ||||||
| assert.equal(diagnostics.length, files.length * accesses.length); | ||||||
| for (const file of files) { | ||||||
| assert.deepEqual( | ||||||
| diagnostics.filter((item) => item.file === file).map(({ code, line }) => ({ code, line })), | ||||||
| accesses.map((_, index) => ({ code: "FENCE_POLICY_RAW_SQL", line: index + 7 })), | ||||||
| ); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| it("permits safe SQL builders, unrelated file data, and raw SQL in the sandbox lifecycle owner", () => { | ||||||
| const root = project({ | ||||||
| "src/sql-fs/dialects/postgres/namespace.ts": `${sqlCapabilities} | ||||||
| const db = tx; | ||||||
| const prose = "UPDATE sandboxes SET version = version + 1"; | ||||||
| db([1, 2]); | ||||||
| db({ name: prose }); | ||||||
| db\`SELECT \${prose}\`; | ||||||
| const entry = { file: "notes.txt", unsafe: false }; | ||||||
| void entry.file; | ||||||
| void entry["file"]; | ||||||
| const { file, unsafe } = entry; | ||||||
| `, | ||||||
| "src/sql-fs/dialects/postgres/sandbox.ts": `${sqlCapabilities} | ||||||
| tx.unsafe("UPDATE sandboxes SET version = version + 1"); | ||||||
| const { file: run } = tx; | ||||||
| run("lifecycle.sql"); | ||||||
| `, | ||||||
| }); | ||||||
| assert.deepEqual(checkArchitectureBoundaries(root), []); | ||||||
| }); | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: This generic pattern rejects unrelated version counters, not only
sandboxes.versionadvancement. Scope the match to asandboxesupdate or use SQL-aware matching so namespace and ingest queries do not need workarounds.Prompt for AI agents