From 9a7957cb181940dbcce441fcf9e6d33ec6f8ce85 Mon Sep 17 00:00:00 2001 From: Kostas Pyliouras Date: Sun, 3 May 2026 11:01:51 +0200 Subject: [PATCH 1/3] Harden tenant isolation SQL boundaries --- packages/voight/src/binder/expression.ts | 44 +++ packages/voight/src/catalog/index.ts | 15 +- packages/voight/src/emitter/index.ts | 49 ++- packages/voight/src/policies/max-limit.ts | 15 +- .../voight/src/policies/tenant-scoping.ts | 15 +- .../integration/compiler/pipeline.test.ts | 6 +- .../security/compile-hardening.test.ts | 41 +++ .../project-id-isolation-sqlite.test.ts | 318 ++++++++++++++++++ .../schema-qualified-catalogs.test.ts | 83 ++++- ...ma-qualified-tenant-scoping-probes.test.ts | 16 + .../voight/tests/unit/catalog/core.test.ts | 16 + .../voight/tests/unit/emitter/core.test.ts | 22 ++ .../tests/unit/emitter/escaping.test.ts | 13 + .../voight/tests/unit/enforcer/core.test.ts | 53 ++- .../unit/enforcer/policy-hardening.test.ts | 8 +- 15 files changed, 685 insertions(+), 29 deletions(-) create mode 100644 packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts diff --git a/packages/voight/src/binder/expression.ts b/packages/voight/src/binder/expression.ts index 97bdef6..405befb 100644 --- a/packages/voight/src/binder/expression.ts +++ b/packages/voight/src/binder/expression.ts @@ -28,9 +28,11 @@ import type { BoundWindowSpecification, CaseExpressionNode, CastExpressionNode, + CastTypeNode, ExistsExpressionNode, ExpressionNode, GroupingExpressionNode, + IdentifierNode, IdentifierExpressionNode, InListExpressionNode, InSubqueryExpressionNode, @@ -54,6 +56,8 @@ import { import { stageFailure, stageSuccess, type StageResult } from "../core/result"; import type { SourceSpan } from "../core/source"; +const RAW_SQL_IDENTIFIER_PATTERN = /^[a-z_][a-z0-9_$]*$/; + export type BindResult = StageResult; export interface BinderExpressionContext { @@ -239,6 +243,14 @@ function bindFunction( node: BoundFunctionCall["ast"], ): BindResult { const callee = normalizeIdentifier(node.callee.name); + if (!isSafeRawSqlIdentifier(node.callee, callee)) { + return context.fail( + DiagnosticCode.UnsupportedConstruct, + "Function names must be unquoted simple identifiers.", + node.callee.span, + ); + } + const args: BoundExpression[] = []; for (const arg of node.arguments) { const bound = context.bindExpression(arg); @@ -350,6 +362,15 @@ function bindCast( context: BinderExpressionContext, node: CastExpressionNode, ): BindResult { + const unsafeTypeIdentifier = findUnsafeCastTypeIdentifier(node.targetType); + if (unsafeTypeIdentifier) { + return context.fail( + DiagnosticCode.UnsupportedConstruct, + "CAST target type names must be unquoted simple identifiers.", + unsafeTypeIdentifier.span, + ); + } + const expression = context.bindExpression(node.expression); if (!expression.ok) { return expression; @@ -368,6 +389,29 @@ function bindCast( ); } +function findUnsafeCastTypeIdentifier(type: CastTypeNode): IdentifierNode | undefined { + for (const part of type.name.parts) { + if (!isSafeRawSqlIdentifier(part, normalizeIdentifier(part.name))) { + return part; + } + } + + for (const argument of type.arguments) { + if (argument.kind === "CastType") { + const unsafe = findUnsafeCastTypeIdentifier(argument); + if (unsafe) { + return unsafe; + } + } + } + + return undefined; +} + +function isSafeRawSqlIdentifier(identifier: IdentifierNode, normalized: string): boolean { + return !identifier.quoted && RAW_SQL_IDENTIFIER_PATTERN.test(normalized); +} + function bindCase( context: BinderExpressionContext, node: CaseExpressionNode, diff --git a/packages/voight/src/catalog/index.ts b/packages/voight/src/catalog/index.ts index eb74c27..ddfe383 100644 --- a/packages/voight/src/catalog/index.ts +++ b/packages/voight/src/catalog/index.ts @@ -43,7 +43,7 @@ export class InMemoryCatalog implements Catalog { export function createIdentifierPath(...parts: string[]): IdentifierPath { return { - parts: parts.map(normalizeIdentifier), + parts: parts.map((part) => normalizeIdentifierPathSegment(part)), }; } @@ -57,7 +57,7 @@ export function createTableSchema(input: { } )[]; }): TableSchema { - const normalizedPath = input.path.map(normalizeIdentifier); + const normalizedPath = input.path.map((part) => normalizeIdentifierPathSegment(part)); if (normalizedPath.length === 0 || normalizedPath.some((part) => part.length === 0)) { throw new Error("createTableSchema requires a non-empty path."); } @@ -95,8 +95,17 @@ export function normalizeIdentifier(value: string): string { return value.toLowerCase(); } +function normalizeIdentifierPathSegment(value: string): string { + const normalized = normalizeIdentifier(value); + if (!normalized || normalized.includes(".")) { + throw new Error("Catalog identifier path segments cannot be empty or contain dots."); + } + + return normalized; +} + function normalizeIdentifierPath(path: IdentifierPath): string { - return path.parts.map(normalizeIdentifier).join("."); + return JSON.stringify(path.parts.map(normalizeIdentifier)); } export class AliasCatalog implements Catalog { diff --git a/packages/voight/src/emitter/index.ts b/packages/voight/src/emitter/index.ts index 2416c9d..dc5fe07 100644 --- a/packages/voight/src/emitter/index.ts +++ b/packages/voight/src/emitter/index.ts @@ -164,9 +164,19 @@ function emitBoundExpression(expression: BoundExpression, parameterIndices: numb : `-${emitBoundExpression(expression.operand, parameterIndices)}`; case "BoundBinaryExpression": return emitBinary( - emitBoundBinaryOperand(expression.left, expression.operator, parameterIndices), + emitBoundBinaryOperand( + expression.left, + expression.operator, + parameterIndices, + "left", + ), expression.operator, - emitBoundBinaryOperand(expression.right, expression.operator, parameterIndices), + emitBoundBinaryOperand( + expression.right, + expression.operator, + parameterIndices, + "right", + ), ); case "BoundFunctionCall": return `${expression.callee}(${expression.distinct ? "DISTINCT " : ""}${expression.arguments.map((arg) => emitBoundExpression(arg, parameterIndices)).join(", ")})${expression.over ? ` ${emitWindowSpecification(expression.over, parameterIndices)}` : ""}`; @@ -333,10 +343,11 @@ function emitBoundBinaryOperand( expression: BoundExpression, parentOperator: BinaryExpressionNode["operator"], parameterIndices: number[], + side: "left" | "right", ): string { const emitted = emitBoundExpression(expression, parameterIndices); return expression.kind === "BoundBinaryExpression" && - shouldParenthesizeBinary(expression.operator, parentOperator) + shouldParenthesizeBinary(expression.operator, parentOperator, side) ? `(${emitted})` : emitted; } @@ -344,8 +355,38 @@ function emitBoundBinaryOperand( function shouldParenthesizeBinary( childOperator: BinaryExpressionNode["operator"], parentOperator: BinaryExpressionNode["operator"], + side: "left" | "right", ): boolean { - return binaryPrecedence(childOperator) < binaryPrecedence(parentOperator); + const childPrecedence = binaryPrecedence(childOperator); + const parentPrecedence = binaryPrecedence(parentOperator); + if (childPrecedence < parentPrecedence) { + return true; + } + + return ( + side === "right" && + childPrecedence === parentPrecedence && + !canFlattenRightBinaryOperand(parentOperator, childOperator) + ); +} + +function canFlattenRightBinaryOperand( + parentOperator: BinaryExpressionNode["operator"], + childOperator: BinaryExpressionNode["operator"], +): boolean { + if (parentOperator === "AND" || parentOperator === "OR") { + return childOperator === parentOperator; + } + + if (parentOperator === "+") { + return childOperator === "+"; + } + + if (parentOperator === "*") { + return childOperator === "*"; + } + + return false; } function binaryPrecedence(operator: BinaryExpressionNode["operator"]): number { diff --git a/packages/voight/src/policies/max-limit.ts b/packages/voight/src/policies/max-limit.ts index 2a91327..91e1e50 100644 --- a/packages/voight/src/policies/max-limit.ts +++ b/packages/voight/src/policies/max-limit.ts @@ -1,4 +1,6 @@ import type { BoundExpression, BoundQuery, BoundSelectStatement, QueryAst } from "../ast"; +import { collectBoundPolicyDiagnostics } from "../ast/bound-policy-traversal"; +import { mapQueryAst } from "../ast/query-ast-traversal"; import { CompilerStage, DiagnosticCode, @@ -44,18 +46,19 @@ class MaxLimitPolicy implements CompilerPolicy { } rewrite(query: QueryAst): QueryAst { - if (typeof this.#defaultLimit === "undefined" || query.body.limit) { + if (typeof this.#defaultLimit === "undefined") { return query; } - return { - ...query, - body: addDefaultLimit(query.body, this.#defaultLimit), - }; + return mapQueryAst(query, (select) => + select.limit ? select : addDefaultLimit(select, this.#defaultLimit!), + ); } enforce(bound: BoundQuery): readonly Diagnostic[] { - return this.#validateSelectLimit(bound.body) ?? []; + return collectBoundPolicyDiagnostics(bound, { + select: (select) => this.#validateSelectLimit(select), + }); } #validateSelectLimit(select: BoundSelectStatement): readonly Diagnostic[] | void { diff --git a/packages/voight/src/policies/tenant-scoping.ts b/packages/voight/src/policies/tenant-scoping.ts index 26b6283..128bcf5 100644 --- a/packages/voight/src/policies/tenant-scoping.ts +++ b/packages/voight/src/policies/tenant-scoping.ts @@ -585,6 +585,7 @@ class TenantScopingPolicy implements CompilerPolicy { return undefined; } + const originalPath = table.name.parts.map((part) => normalizeIdentifier(part.name)); const originalName = normalizeIdentifier( table.name.parts[table.name.parts.length - 1]?.name ?? "", ); @@ -608,7 +609,7 @@ class TenantScopingPolicy implements CompilerPolicy { this.#assertUnambiguousScopeMatch(matchedRules, table.span, match.names); - if (visibleCtes.has(originalName)) { + if (originalPath.length === 1 && visibleCtes.has(originalName)) { throw new PolicyDiagnosticError( createDiagnostic({ code: DiagnosticCode.PolicyViolation, @@ -1149,10 +1150,12 @@ function collectTenantScopeMatchFromAst( const originalQualified = originalPath.join("."); const originalName = originalPath[originalPath.length - 1] ?? originalQualified; const resolved = catalog.getTable({ parts: originalPath }); - const canonicalName = resolved?.path.parts.join(".") ?? originalQualified; + const canonicalPath = resolved?.path.parts ?? originalPath; + const canonicalName = canonicalPath.join("."); + const canonicalShortName = canonicalPath[canonicalPath.length - 1] ?? canonicalName; const schemaQualified = (resolved?.path.parts.length ?? originalPath.length) > 1; const exactNames = uniqueNames([originalQualified, canonicalName]); - const shortNames = schemaQualified ? uniqueNames([originalName]) : []; + const shortNames = schemaQualified ? uniqueNames([originalName, canonicalShortName]) : []; return { exactRules: matchTenantScopeRules(rules, exactNames), @@ -1167,7 +1170,9 @@ function collectTenantScopeMatchFromBound( table: BoundTableReference, rules: readonly TenantScopeRule[], ): TenantScopeMatch { - const canonicalName = table.table.path.parts.map((part) => normalizeIdentifier(part)).join("."); + const canonicalPath = table.table.path.parts.map((part) => normalizeIdentifier(part)); + const canonicalName = canonicalPath.join("."); + const canonicalShortName = canonicalPath[canonicalPath.length - 1] ?? canonicalName; const astName = table.ast.kind === "TableReference" ? table.ast.name.parts.map((part) => normalizeIdentifier(part.name)).join(".") @@ -1182,7 +1187,7 @@ function collectTenantScopeMatchFromBound( const exactNames = uniqueNames( schemaQualified ? [canonicalName, astName] : [canonicalName, astName, shortName], ); - const shortNames = schemaQualified ? uniqueNames([shortName]) : []; + const shortNames = schemaQualified ? uniqueNames([shortName, canonicalShortName]) : []; return { exactRules: matchTenantScopeRules(rules, exactNames), diff --git a/packages/voight/tests/integration/compiler/pipeline.test.ts b/packages/voight/tests/integration/compiler/pipeline.test.ts index 48b9a0f..6f2e1e0 100644 --- a/packages/voight/tests/integration/compiler/pipeline.test.ts +++ b/packages/voight/tests/integration/compiler/pipeline.test.ts @@ -173,7 +173,7 @@ describe("compile", () => { ); }); - test("injects the configured default limit only on the outermost select", () => { + test("injects the configured default limit into every select", () => { const result = compile( "WITH recent_orders AS (SELECT user_id FROM orders) SELECT id FROM users WHERE id IN (SELECT user_id FROM recent_orders)", { @@ -189,13 +189,13 @@ describe("compile", () => { } expect(result.emitted?.sql).toBe( - "WITH `recent_orders` AS (SELECT `orders`.`user_id` FROM `orders`) SELECT `users`.`id` FROM `users` WHERE `users`.`id` IN (SELECT `recent_orders`.`user_id` FROM `recent_orders`) LIMIT 25", + "WITH `recent_orders` AS (SELECT `orders`.`user_id` FROM `orders` LIMIT 25) SELECT `users`.`id` FROM `users` WHERE `users`.`id` IN (SELECT `recent_orders`.`user_id` FROM `recent_orders` LIMIT 25) LIMIT 25", ); expect(result.rewrittenAst?.body.limit?.count.kind).toBe("Literal"); if (result.rewrittenAst?.body.limit?.count.kind === "Literal") { expect(result.rewrittenAst.body.limit.count.value).toBe("25"); } - expect(result.rewrittenAst?.with?.ctes[0]?.query.body.limit).toBeUndefined(); + expect(result.rewrittenAst?.with?.ctes[0]?.query.body.limit?.count.kind).toBe("Literal"); }); test("does not apply any function policy unless one is configured", () => { diff --git a/packages/voight/tests/integration/security/compile-hardening.test.ts b/packages/voight/tests/integration/security/compile-hardening.test.ts index 65b1be2..bbbbf85 100644 --- a/packages/voight/tests/integration/security/compile-hardening.test.ts +++ b/packages/voight/tests/integration/security/compile-hardening.test.ts @@ -19,6 +19,45 @@ describe("compile hardening", () => { "SELECT 1; SELECT 2", "SET @a = 1", "SELECT id FROM users UNION SELECT id FROM orders", + "SELECT id FROM users INTERSECT SELECT user_id FROM orders", + "SELECT id FROM users EXCEPT SELECT user_id FROM orders", + ]) { + expectBlocked(sql); + } + }); + + test("rejects quoted callable and cast-type injection surfaces", () => { + for (const sql of [ + "SELECT `SLEEP(1); DROP TABLE users; -- `(0)", + "SELECT `GET_LOCK('voight', 10); DROP TABLE users; -- `(0)", + "SELECT CAST(name AS `CHAR); DROP TABLE users; -- `) FROM users", + "SELECT CAST(name AS `CHAR CHARACTER SET utf8mb4 COLLATE utf8mb4_bin`) FROM users", + ]) { + expectBlocked(sql); + } + }); + + test("rejects advanced SQL forms outside the supported SELECT subset", () => { + for (const sql of [ + "WITH RECURSIVE r(n) AS (SELECT 1) SELECT n FROM r", + "SELECT * FROM LATERAL (SELECT id FROM users) AS x", + "SELECT * FROM JSON_TABLE('[1]', '$[*]' COLUMNS(n INT PATH '$')) AS jt", + "TABLE users", + "VALUES ROW(1), ROW(2)", + "SELECT * FROM (VALUES ROW(1)) AS v(id)", + "SELECT id INTO OUTFILE '/tmp/x' FROM users", + "SELECT id FROM users FOR UPDATE", + "SELECT id FROM users LOCK IN SHARE MODE", + "SELECT id FROM users USE INDEX (idx_users_tenant)", + "SELECT name COLLATE utf8mb4_bin FROM users", + "SELECT id FROM users WHERE profile->'$.tenant_id' = 'x'", + "SELECT id FROM users WHERE profile->>'$.tenant_id' = 'x'", + "SELECT id FROM users WHERE (id, tenant_id) IN ((1, 't'))", + "SELECT id FROM users WHERE id > ALL (SELECT user_id FROM orders)", + "SELECT id FROM users WHERE id = ANY (SELECT user_id FROM orders)", + "SELECT SUM(total) OVER (ORDER BY created_at ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM orders", + "SELECT SUM(total) OVER w FROM orders WINDOW w AS (PARTITION BY user_id)", + "SELECT metric, SUM(value) FROM timeseries GROUP BY metric WITH ROLLUP", ]) { expectBlocked(sql); } @@ -29,6 +68,8 @@ describe("compile hardening", () => { "SELECT id FROM users -- where 1=1", "SELECT id FROM users # comment", "SELECT id FROM users /* comment */", + "SELECT /*!80408 SQL_NO_CACHE */ id FROM users", + "SELECT /*+ MAX_EXECUTION_TIME(1) */ id FROM users", "SELECT id FROM users\uFF1B", ]) { expectBlocked(sql); diff --git a/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts b/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts new file mode 100644 index 0000000..8d5d985 --- /dev/null +++ b/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts @@ -0,0 +1,318 @@ +import { DatabaseSync } from "node:sqlite"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; + +import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; +import { compile } from "../../../src/compiler"; +import { maxLimitPolicy, tenantScopingPolicy } from "../../../src/policies"; + +const ATTACKER_PROJECT_ID = "project-alpha"; +const VICTIM_PROJECT_ID = "project-bravo"; + +const catalog = new InMemoryCatalog([ + createTableSchema({ + path: ["events"], + columns: ["id", "project_id", "actor_id", "metric", "value"], + }), +]); + +const policies = [ + maxLimitPolicy({ + maxLimit: 100, + defaultLimit: 25, + maxOffset: 1_000, + }), + tenantScopingPolicy({ + tables: ["events"], + scopeColumn: "project_id", + contextKey: "projectId", + }), +]; + +let db: DatabaseSync; + +type QueryRow = Record; + +function compileScoped(sql: string) { + return compile(sql, { + catalog, + policies, + policyContext: { + projectId: ATTACKER_PROJECT_ID, + }, + debug: true, + }); +} + +function executeScoped(sql: string) { + const result = compileScoped(sql); + expect(result.ok, JSON.stringify(result.diagnostics, null, 2)).toBe(true); + if (!result.ok) { + throw new Error("Compilation unexpectedly failed."); + } + + return { + result, + rows: db.prepare(result.emitted!.sql).all() as QueryRow[], + }; +} + +function projectIds(rows: readonly QueryRow[]): string[] { + return rows + .flatMap((row) => Object.entries(row)) + .filter(([key, value]) => key.toLowerCase().includes("project_id") && value !== null) + .map(([, value]) => String(value)); +} + +function expectNoVictimProjectIds(rows: readonly QueryRow[]): void { + expect(projectIds(rows)).not.toContain(VICTIM_PROJECT_ID); +} + +beforeAll(() => { + db = new DatabaseSync(":memory:"); + db.exec(`CREATE TABLE events ( + id INTEGER PRIMARY KEY, + project_id TEXT NOT NULL, + actor_id INTEGER NOT NULL, + metric TEXT NOT NULL, + value INTEGER NOT NULL + )`); + + db.exec(`INSERT INTO events VALUES + (1, '${ATTACKER_PROJECT_ID}', 10, 'login', 5), + (2, '${ATTACKER_PROJECT_ID}', 11, 'export', 2), + (3, '${ATTACKER_PROJECT_ID}', 12, 'login', 8), + (4, '${VICTIM_PROJECT_ID}', 20, 'login', 999), + (5, '${VICTIM_PROJECT_ID}', 21, 'export', 777), + (6, '${VICTIM_PROJECT_ID}', 22, 'billing', 555) + `); +}); + +afterAll(() => { + db.close(); +}); + +describe("project_id isolation against mixed-project rows", () => { + test("base table reads return only the configured project", () => { + const { result, rows } = executeScoped( + "SELECT id, project_id, metric, value FROM events ORDER BY id LIMIT 10", + ); + + expect(result.emitted?.sql).toContain("WHERE `events`.`project_id` = 'project-alpha'"); + expect(projectIds(rows)).toEqual([ + ATTACKER_PROJECT_ID, + ATTACKER_PROJECT_ID, + ATTACKER_PROJECT_ID, + ]); + expectNoVictimProjectIds(rows); + }); + + test("an explicit victim project predicate returns no rows", () => { + const { result, rows } = executeScoped( + `SELECT id, project_id, metric + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' + ORDER BY id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("`events`.`project_id` = 'project-alpha'"); + expect(rows).toEqual([]); + expectNoVictimProjectIds(rows); + }); + + test("derived tables with victim predicates cannot resurrect victim rows", () => { + const { rows } = executeScoped( + `SELECT d.project_id, d.metric + FROM ( + SELECT project_id, metric + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' + LIMIT 10 + ) AS d + LIMIT 10`, + ); + + expect(rows).toEqual([]); + expectNoVictimProjectIds(rows); + }); + + test("CTEs with victim predicates cannot resurrect victim rows", () => { + const { rows } = executeScoped( + `WITH victim_events AS ( + SELECT project_id, metric + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' + LIMIT 10 + ) + SELECT project_id, metric + FROM victim_events + LIMIT 10`, + ); + + expect(rows).toEqual([]); + expectNoVictimProjectIds(rows); + }); + + test("scalar subqueries cannot leak a victim project id", () => { + const { rows } = executeScoped( + `SELECT ( + SELECT project_id + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' + LIMIT 1 + ) AS leaked_project_id + FROM events + ORDER BY id + LIMIT 1`, + ); + + expect(rows).toEqual([{ leaked_project_id: null }]); + expectNoVictimProjectIds(rows); + }); + + test("default limits are injected into NOT EXISTS subqueries", () => { + const { result, rows } = executeScoped( + `SELECT e.id + FROM events AS e + WHERE NOT EXISTS ( + SELECT 1 + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + AND victim.metric = e.metric + ) + ORDER BY e.id`, + ); + + expect(result.emitted?.sql).toContain("NOT EXISTS (SELECT 1 FROM `events` AS `victim`"); + expect(result.emitted?.sql).toContain("`victim`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("LIMIT 25"); + expect(rows).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); + expectNoVictimProjectIds(rows); + }); + + test("CASE scalar subqueries get tenant guards and default limits", () => { + const { result, rows } = executeScoped( + `SELECT CASE + WHEN e.id > 0 THEN ( + SELECT victim.project_id + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + LIMIT 1 + ) + ELSE NULL + END AS leaked_project_id + FROM events AS e + ORDER BY e.id`, + ); + + expect(result.emitted?.sql).toContain("`victim`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("LIMIT 25"); + expect(rows[0]).toEqual({ leaked_project_id: null }); + expectNoVictimProjectIds(rows); + }); + + test("self joins scope every table touch independently", () => { + const { result, rows } = executeScoped( + `SELECT e.id, other.id AS other_id + FROM events AS e + INNER JOIN events AS other + ON other.metric = e.metric + AND other.project_id = '${VICTIM_PROJECT_ID}' + ORDER BY e.id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); + expect(rows).toEqual([]); + expectNoVictimProjectIds(rows); + }); + + test("OR predicates cannot widen past the injected project guard", () => { + const { result, rows } = executeScoped( + `SELECT id, project_id + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 + ORDER BY id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("OR 1 = 1"); + expect(result.emitted?.sql).toContain("AND `events`.`project_id` = 'project-alpha'"); + expect(rows).toEqual([ + { id: 1, project_id: ATTACKER_PROJECT_ID }, + { id: 2, project_id: ATTACKER_PROJECT_ID }, + { id: 3, project_id: ATTACKER_PROJECT_ID }, + ]); + expectNoVictimProjectIds(rows); + }); + + test("left joins with always-true join predicates cannot pull victim rows", () => { + const { result, rows } = executeScoped( + `SELECT e.project_id, other.project_id AS other_project_id + FROM events AS e + LEFT JOIN events AS other + ON other.project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 + ORDER BY e.id, other.id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); + expectNoVictimProjectIds(rows); + }); + + test("IN-list scalar subqueries cannot smuggle victim ids", () => { + const { rows } = executeScoped( + `SELECT id, project_id + FROM events + WHERE id IN ( + (SELECT id FROM events WHERE project_id = '${VICTIM_PROJECT_ID}' LIMIT 1), + 1 + ) + ORDER BY id + LIMIT 10`, + ); + + expect(rows).toEqual([{ id: 1, project_id: ATTACKER_PROJECT_ID }]); + expectNoVictimProjectIds(rows); + }); + + test("window expression subqueries cannot read victim project ids", () => { + const { rows } = executeScoped( + `SELECT project_id, + COUNT(id) OVER ( + PARTITION BY ( + SELECT project_id + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' + LIMIT 1 + ) + ) AS grouped_count + FROM events + ORDER BY id + LIMIT 10`, + ); + + expect(rows).toEqual([ + { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, + { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, + { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, + ]); + expectNoVictimProjectIds(rows); + }); + + test("quoted aliases containing SQL syntax do not break out of project guards", () => { + const { result, rows } = executeScoped( + "SELECT `e --`.`project_id` FROM events AS `e --` WHERE `e --`.`project_id` = 'project-bravo' OR 1 = 1 ORDER BY `e --`.`id` LIMIT 10", + ); + + expect(result.emitted?.sql).toContain("`e --`.`project_id` = 'project-alpha'"); + expect(projectIds(rows)).toEqual([ + ATTACKER_PROJECT_ID, + ATTACKER_PROJECT_ID, + ATTACKER_PROJECT_ID, + ]); + expectNoVictimProjectIds(rows); + }); +}); diff --git a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts index d837f5d..e9a01bc 100644 --- a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts +++ b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts @@ -1,6 +1,11 @@ import { describe, expect, test } from "vitest"; -import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; +import { + AliasCatalog, + InMemoryCatalog, + createCatalogAlias, + createTableSchema, +} from "../../../src/catalog"; import { DiagnosticCode } from "../../../src/core/diagnostics"; import { tenantScopingPolicy } from "../../../src/policies"; import { compileStrict } from "../../_support/compile"; @@ -22,6 +27,17 @@ describe("schema-qualified catalogs", () => { } }); + test("does not resolve a quoted dotted identifier as a schema-qualified path", () => { + const result = compileStrict("SELECT metric FROM `tracking.time_series_stats`", { + catalog, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.UnknownTable); + } + }); + test("emits schema-qualified tables and scopes them when the policy uses the full name", () => { const result = compileStrict("SELECT ts.metric FROM tracking.time_series_stats AS ts", { catalog, @@ -72,4 +88,69 @@ describe("schema-qualified catalogs", () => { ).toBe(true); } }); + + test("fails closed when a logical alias targets a schema-qualified table scoped by short physical name", () => { + const aliasCatalog = new AliasCatalog(catalog, [ + createCatalogAlias({ + from: ["public_stats"], + to: ["tracking", "time_series_stats"], + }), + ]); + + const result = compileStrict("SELECT metric FROM public_stats", { + catalog: aliasCatalog, + policies: [ + tenantScopingPolicy({ + tables: ["time_series_stats"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + }), + ], + policyContext: { + tenantId: "tenant-123", + }, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect( + result.diagnostics.some( + (diagnostic) => + diagnostic.code === DiagnosticCode.InvalidPolicyConfiguration && + diagnostic.message.includes("tracking.time_series_stats"), + ), + ).toBe(true); + } + }); + + test("scopes a logical alias to a schema-qualified table when the logical alias is configured", () => { + const aliasCatalog = new AliasCatalog(catalog, [ + createCatalogAlias({ + from: ["public_stats"], + to: ["tracking", "time_series_stats"], + }), + ]); + + const result = compileStrict("SELECT metric FROM public_stats", { + catalog: aliasCatalog, + policies: [ + tenantScopingPolicy({ + tables: ["public_stats"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + }), + ], + policyContext: { + tenantId: "tenant-123", + }, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.emitted?.sql).toContain( + "FROM `tracking`.`time_series_stats` AS `public_stats`", + ); + expect(result.emitted?.sql).toContain("`public_stats`.`tenant_id` = 'tenant-123'"); + } + }); }); diff --git a/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts b/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts index fc3a359..cee4be2 100644 --- a/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts +++ b/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts @@ -174,6 +174,22 @@ describe("schema-qualified tenant scoping probes", () => { expect(rows).toEqual([{ series_name: "planted-row" }]); }); + test("a short-name CTE does not shadow an explicit schema-qualified catalog table", () => { + const { result, rows } = executeScoped( + `WITH event_rollups AS ( + SELECT '${VICTIM_WORKSPACE_ID}' AS workspace_id, 'planted-row' AS series_name + ) + SELECT r.workspace_id + FROM analytics.event_rollups AS r + ORDER BY r.id + LIMIT 1`, + ); + + expect(result.emitted?.sql).toContain("FROM `analytics`.`event_rollups` AS `r`"); + expect(result.emitted?.sql).toContain("`r`.`workspace_id` = 'workspace-alpha'"); + expect(rows).toEqual([{ workspace_id: ATTACKER_WORKSPACE_ID }]); + }); + test("CASE-wrapped scalar subqueries cannot exfiltrate a victim workspace id", () => { const { rows } = executeScoped( `SELECT CASE diff --git a/packages/voight/tests/unit/catalog/core.test.ts b/packages/voight/tests/unit/catalog/core.test.ts index 5985ecd..2f3826c 100644 --- a/packages/voight/tests/unit/catalog/core.test.ts +++ b/packages/voight/tests/unit/catalog/core.test.ts @@ -72,4 +72,20 @@ describe("catalog", () => { expect(table.id).toBe("analytics.users"); expect(table.name).toBe("users"); }); + + test("rejects dotted catalog path segments", () => { + expect(() => + createTableSchema({ + path: ["analytics.users"], + columns: ["id"], + }), + ).toThrow("cannot be empty or contain dots"); + + expect(() => + createCatalogAlias({ + from: ["analytics.users"], + to: ["analytics", "users"], + }), + ).toThrow("cannot be empty or contain dots"); + }); }); diff --git a/packages/voight/tests/unit/emitter/core.test.ts b/packages/voight/tests/unit/emitter/core.test.ts index 91f3374..7cc4b23 100644 --- a/packages/voight/tests/unit/emitter/core.test.ts +++ b/packages/voight/tests/unit/emitter/core.test.ts @@ -148,4 +148,26 @@ describe("emit", () => { "SELECT `users`.`id` FROM `users` WHERE `users`.`age` NOT BETWEEN 18 AND 65 ORDER BY `users`.`created_at` BETWEEN '2024-01-01' AND '2024-12-31' DESC", ); }); + + test("preserves non-associative select-alias expressions with parentheses", () => { + const subtraction = emit(bindStatement("SELECT age - 1 AS x FROM users ORDER BY 1 - x")); + expect(subtraction.ok).toBe(true); + if (subtraction.ok) { + expect(subtraction.value.sql).toContain("ORDER BY 1 - (`users`.`age` - 1) ASC"); + } + + const division = emit(bindStatement("SELECT age / 2 AS x FROM users ORDER BY 1 / x")); + expect(division.ok).toBe(true); + if (division.ok) { + expect(division.value.sql).toContain("ORDER BY 1 / (`users`.`age` / 2) ASC"); + } + + const comparison = emit(bindStatement("SELECT age = 1 AS x FROM users ORDER BY id = x")); + expect(comparison.ok).toBe(true); + if (comparison.ok) { + expect(comparison.value.sql).toContain( + "ORDER BY `users`.`id` = (`users`.`age` = 1) ASC", + ); + } + }); }); diff --git a/packages/voight/tests/unit/emitter/escaping.test.ts b/packages/voight/tests/unit/emitter/escaping.test.ts index 04c4a50..e7e8a16 100644 --- a/packages/voight/tests/unit/emitter/escaping.test.ts +++ b/packages/voight/tests/unit/emitter/escaping.test.ts @@ -70,6 +70,19 @@ describe("emitter literal escaping", () => { expect(escaped.emitted?.sql).toContain("'tenant\\\\'"); } }); + + test("contains policy-injected SQL payloads inside one MySQL string literal", () => { + const result = compileTenantScoped( + "SELECT metric FROM timeseries", + "tenant-123' OR 1=1 /*", + ); + expect(result.ok).toBe(true); + if (!result.ok) { + return; + } + + expect(result.emitted?.sql).toContain("'tenant-123'' OR 1=1 /*'"); + }); }); describe("emitter identifier escaping", () => { diff --git a/packages/voight/tests/unit/enforcer/core.test.ts b/packages/voight/tests/unit/enforcer/core.test.ts index f0fb700..360a745 100644 --- a/packages/voight/tests/unit/enforcer/core.test.ts +++ b/packages/voight/tests/unit/enforcer/core.test.ts @@ -116,7 +116,31 @@ describe("enforce", () => { expect(functionLimit.ok).toBe(false); }); - test("does not require nested subqueries to carry their own limit", () => { + test("rejects negative LIMIT and OFFSET expressions", () => { + const negativeLimit = enforce(bindStatement("SELECT id FROM users LIMIT -1"), { + policies: [maxLimitPolicy({ maxLimit: 100 })], + }); + expect(negativeLimit.ok).toBe(false); + + const negativeOffset = enforce(bindStatement("SELECT id FROM users LIMIT 1 OFFSET -1"), { + policies: [maxLimitPolicy({ maxLimit: 100, maxOffset: 100 })], + }); + expect(negativeOffset.ok).toBe(false); + }); + + test("enforces comma-form LIMIT count and offset positions", () => { + const excessiveOffset = enforce(bindStatement("SELECT id FROM users LIMIT 1000, 1"), { + policies: [maxLimitPolicy({ maxLimit: 100, maxOffset: 100 })], + }); + expect(excessiveOffset.ok).toBe(false); + + const excessiveCount = enforce(bindStatement("SELECT id FROM users LIMIT 1, 1000"), { + policies: [maxLimitPolicy({ maxLimit: 100, maxOffset: 100 })], + }); + expect(excessiveCount.ok).toBe(false); + }); + + test("requires nested subqueries to carry their own limit", () => { const bound = bindStatement( "SELECT users.id FROM users WHERE users.id IN (SELECT orders.id FROM orders) LIMIT 10", ); @@ -124,10 +148,28 @@ describe("enforce", () => { policies: [maxLimitPolicy({ maxLimit: 100 })], }); - expect(result.ok).toBe(true); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); + } }); - test("ignores nested subquery limit sizes when enforcing the outer result size", () => { + test("requires limits across scalar, exists, not exists, and in subqueries", () => { + for (const sql of [ + "SELECT id FROM users WHERE id = (SELECT user_id FROM orders) LIMIT 10", + "SELECT id FROM users WHERE EXISTS (SELECT user_id FROM orders) LIMIT 10", + "SELECT id FROM users WHERE NOT EXISTS (SELECT user_id FROM orders) LIMIT 10", + "SELECT id FROM users WHERE id IN (SELECT user_id FROM orders) LIMIT 10", + ]) { + const result = enforce(bindStatement(sql), { + policies: [maxLimitPolicy({ maxLimit: 100 })], + }); + + expect(result.ok, `Expected nested limit rejection for ${sql}`).toBe(false); + } + }); + + test("rejects nested subquery limit sizes above the configured maximum", () => { const bound = bindStatement( "SELECT users.id FROM users WHERE users.id IN (SELECT orders.id FROM orders LIMIT 999999) LIMIT 10", ); @@ -135,7 +177,10 @@ describe("enforce", () => { policies: [maxLimitPolicy({ maxLimit: 100 })], }); - expect(result.ok).toBe(true); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); + } }); test("allows approved functions and nested expressions", () => { diff --git a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts index 0a77ca1..4fa0fea 100644 --- a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts +++ b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts @@ -96,7 +96,7 @@ describe("maxLimitPolicy abuse probes", () => { ); }); - test("ignores nested OFFSET values because only the outer result size is constrained", () => { + test("rejects nested OFFSET values above the configured maximum", () => { const result = compile( "SELECT id FROM users WHERE id IN (SELECT user_id FROM orders LIMIT 1 OFFSET 999999999) LIMIT 1", { @@ -106,8 +106,10 @@ describe("maxLimitPolicy abuse probes", () => { }, ); - expect(result.ok).toBe(true); - expect(result.emitted?.sql).toContain("OFFSET 999999999"); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); + } }); test("rejects non-literal outer OFFSET expressions when maxOffset is configured", () => { From e72fa5eb92fecd7ddd2d4b8c5417fe2786a06e43 Mon Sep 17 00:00:00 2001 From: Kostas Pyliouras Date: Sun, 3 May 2026 14:34:54 +0200 Subject: [PATCH 2/3] Harden project isolation edge cases --- README.md | 6 + packages/voight/README.md | 5 + packages/voight/src/compiler/enforcer.ts | 3 + packages/voight/src/compiler/index.ts | 1 + packages/voight/src/policies/index.ts | 2 + packages/voight/src/policies/shared.ts | 1 + .../voight/src/policies/tenant-scoping.ts | 308 +++++++++++------ .../mysql84-project-isolation.test.ts | 321 ++++++++++++++++++ .../project-id-isolation-sqlite.test.ts | 88 +++++ .../schema-qualified-catalogs.test.ts | 32 ++ .../security/tenant-scoping-shadowing.test.ts | 51 +++ .../tests/unit/emitter/tenant-scoping.test.ts | 59 +++- .../unit/enforcer/policy-hardening.test.ts | 22 +- 13 files changed, 788 insertions(+), 111 deletions(-) create mode 100644 packages/voight/tests/integration/security/mysql84-project-isolation.test.ts diff --git a/README.md b/README.md index 25af0af..ab1ae2e 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,12 @@ By default, `compile(...)` returns a public-safe result surface: emitted SQL on - `allowedFunctionsPolicy(...)` to restrict callable SQL functions and `CURRENT_*` keywords - `supportedOperatorsPolicy()` to reject operators outside the supported policy surface +`tenantScopingPolicy(...)` treats scope values as strings by default. If a scope column is +numeric or boolean, configure `scopeValueType` explicitly, for example +`scopeValueType: "bigint"` for a `BIGINT project_id`. For MySQL string scope columns, +use binary or case-sensitive collation/comparison semantics so `project-alpha` cannot also +match case/accent variants such as `PROJECT-ALPHA`. + ## Example ```ts diff --git a/packages/voight/README.md b/packages/voight/README.md index 166b513..b208830 100644 --- a/packages/voight/README.md +++ b/packages/voight/README.md @@ -137,6 +137,11 @@ Use `AliasCatalog` and `createCatalogAlias(...)` if your public logical table na - `allowedFunctionsPolicy(...)` allowlists function calls and `CURRENT_*` keywords. - `supportedOperatorsPolicy()` rejects operators outside the supported policy surface. +`tenantScopingPolicy(...)` defaults scope values to strings. If the scoped column is numeric or +boolean, set `scopeValueType` explicitly, for example `scopeValueType: "bigint"` for a +`BIGINT project_id`. On MySQL, string scope columns should use binary or case-sensitive +collation/comparison semantics so equivalent case/accent variants do not cross tenant boundaries. + ## Repository The source repository lives at [github.com/lukaskratzel/voight](https://github.com/lukaskratzel/voight). The workspace README has more detail on the parser stack, development workflow, and release process. diff --git a/packages/voight/src/compiler/enforcer.ts b/packages/voight/src/compiler/enforcer.ts index 6a7b0cc..4a4dbd5 100644 --- a/packages/voight/src/compiler/enforcer.ts +++ b/packages/voight/src/compiler/enforcer.ts @@ -1,4 +1,5 @@ import type { BoundQuery } from "../ast"; +import type { Catalog } from "../catalog"; import { CompilerStage, type Diagnostic } from "../core/diagnostics"; import { type CompilerPolicy, type PolicyContext, resolvePolicies } from "../policies"; import { stageFailure, stageSuccess, type StageResult } from "../core/result"; @@ -6,6 +7,7 @@ import { stageFailure, stageSuccess, type StageResult } from "../core/result"; export interface EnforcementOptions { readonly policies?: readonly CompilerPolicy[]; readonly policyContext?: PolicyContext; + readonly catalog?: Catalog; } export type EnforcementResult = StageResult< @@ -19,6 +21,7 @@ export function enforce(bound: BoundQuery, options: EnforcementOptions = {}): En const policies = resolvePolicies(options); const context = { context: options.policyContext ?? {}, + catalog: options.catalog, }; policies.forEach((policy) => { diff --git a/packages/voight/src/compiler/index.ts b/packages/voight/src/compiler/index.ts index d1ebf7b..0197588 100644 --- a/packages/voight/src/compiler/index.ts +++ b/packages/voight/src/compiler/index.ts @@ -110,6 +110,7 @@ function compileInternal(source: string, options: CompileOptions): CompileResult const enforced = enforce(bound.value, { policies: options.policies, policyContext: options.policyContext, + catalog: options.catalog, }); if (!enforced.ok) { return { diff --git a/packages/voight/src/policies/index.ts b/packages/voight/src/policies/index.ts index d83a292..3de8e5d 100644 --- a/packages/voight/src/policies/index.ts +++ b/packages/voight/src/policies/index.ts @@ -14,6 +14,7 @@ import { tenantScopingPolicy, type TenantScopingPolicyOptions, type TenantScopingScopeOptions, + type TenantScopeValueType, } from "./tenant-scoping"; export type { @@ -29,6 +30,7 @@ export type { MaxLimitPolicyOptions, TenantScopingPolicyOptions, TenantScopingScopeOptions, + TenantScopeValueType, }; export { diff --git a/packages/voight/src/policies/shared.ts b/packages/voight/src/policies/shared.ts index bd43dcc..f64de82 100644 --- a/packages/voight/src/policies/shared.ts +++ b/packages/voight/src/policies/shared.ts @@ -11,6 +11,7 @@ export interface PolicyRewriteContext { export interface PolicyEnforcementContext { readonly context: PolicyContext; + readonly catalog?: Catalog; } export interface CompilerPolicy { diff --git a/packages/voight/src/policies/tenant-scoping.ts b/packages/voight/src/policies/tenant-scoping.ts index 128bcf5..0b62656 100644 --- a/packages/voight/src/policies/tenant-scoping.ts +++ b/packages/voight/src/policies/tenant-scoping.ts @@ -37,10 +37,13 @@ const TENANT_SCOPING_POLICY_NAME = "tenant-scoping"; const MAX_UINT64 = (1n << 64n) - 1n; const MIN_POSITIVE_UINT64 = 1n; +export type TenantScopeValueType = "string" | "number" | "bigint" | "boolean"; + export interface TenantScopingScopeOptions { readonly tables: readonly string[]; readonly scopeColumn: string; readonly contextKey: string; + readonly scopeValueType?: TenantScopeValueType; } export type TenantScopingPolicyOptions = @@ -57,6 +60,7 @@ interface TenantScopeRule { readonly tables: ReadonlySet; readonly scopeColumn: string; readonly contextKey: string; + readonly scopeValueType: TenantScopeValueType; } interface TenantScopeMatch { @@ -67,6 +71,24 @@ interface TenantScopeMatch { readonly schemaQualified: boolean; } +type NormalizedPolicyValue = + | { + readonly literalType: "string"; + readonly value: string; + } + | { + readonly literalType: "integer" | "decimal"; + readonly value: string; + } + | { + readonly literalType: "boolean"; + readonly value: boolean; + } + | { + readonly literalType: "null"; + readonly value: null; + }; + const POLICY_IDENTIFIER_SEGMENT_PATTERN = /^[a-z_][a-z0-9_$]*$/; class TenantScopingPolicy implements CompilerPolicy { @@ -99,6 +121,7 @@ class TenantScopingPolicy implements CompilerPolicy { this.#enforceSelect( select, resolveTenantScopeContextValues(this.#scopeRules, context.context, "enforce"), + context.catalog, ), }); } @@ -217,6 +240,7 @@ class TenantScopingPolicy implements CompilerPolicy { scope.alias, scope.rule.scopeColumn, getTenantScopeContextValue(contextValues, scope.rule.contextKey), + scope.rule.scopeValueType, scope.span, ); return { @@ -234,6 +258,7 @@ class TenantScopingPolicy implements CompilerPolicy { fromScope.alias, fromScope.rule.scopeColumn, getTenantScopeContextValue(contextValues, fromScope.rule.contextKey), + fromScope.rule.scopeValueType, fromScope.span, ), ]) @@ -241,6 +266,7 @@ class TenantScopingPolicy implements CompilerPolicy { fromScope.alias, fromScope.rule.scopeColumn, getTenantScopeContextValue(contextValues, fromScope.rule.contextKey), + fromScope.rule.scopeValueType, fromScope.span, ) : rewritten.where; @@ -256,11 +282,12 @@ class TenantScopingPolicy implements CompilerPolicy { #enforceSelect( select: BoundSelectStatement, contextValues: ReadonlyMap, + catalog: Catalog | undefined, ): readonly Diagnostic[] { const diagnostics: Diagnostic[] = []; for (const table of select.scope.tables.values()) { - const classification = this.#classifyBoundTable(table); + const classification = this.#classifyBoundTable(table, catalog); if (!classification.matched) { continue; } @@ -309,6 +336,7 @@ class TenantScopingPolicy implements CompilerPolicy { const expectedLiteral = normalizePolicyValue( getTenantScopeContextValue(contextValues, classification.rule.contextKey), + classification.rule.scopeValueType, ); const guardExpression = @@ -589,6 +617,28 @@ class TenantScopingPolicy implements CompilerPolicy { const originalName = normalizeIdentifier( table.name.parts[table.name.parts.length - 1]?.name ?? "", ); + const aliasName = normalizeIdentifier(table.alias?.name ?? originalName); + if (originalPath.length === 1 && visibleCtes.has(originalName)) { + const shadowedName = findScopedTableName( + [originalName, aliasName], + this.#scopeRules, + catalog, + ); + if (shadowedName) { + throw new PolicyDiagnosticError( + createDiagnostic({ + code: DiagnosticCode.PolicyViolation, + stage: CompilerStage.Rewriter, + message: `Policy "${this.name}" rejects CTE shadowing for scoped table "${shadowedName}".`, + primarySpan: table.span, + visibility: DiagnosticVisibility.PublicRedacted, + publicMessage: "Query violates tenant scoping requirements.", + }), + { policyName: this.name }, + ); + } + } + const match = collectTenantScopeMatchFromAst(table, catalog, this.#scopeRules); const matchedRules = dedupeTenantScopeRules([...match.exactRules, ...match.shortRules]); const matchedRule = match.exactRules[0]; @@ -609,28 +659,17 @@ class TenantScopingPolicy implements CompilerPolicy { this.#assertUnambiguousScopeMatch(matchedRules, table.span, match.names); - if (originalPath.length === 1 && visibleCtes.has(originalName)) { - throw new PolicyDiagnosticError( - createDiagnostic({ - code: DiagnosticCode.PolicyViolation, - stage: CompilerStage.Rewriter, - message: `Policy "${this.name}" rejects CTE shadowing for scoped table "${originalName}".`, - primarySpan: table.span, - visibility: DiagnosticVisibility.PublicRedacted, - publicMessage: "Query violates tenant scoping requirements.", - }), - { policyName: this.name }, - ); - } - return { - alias: normalizeIdentifier(table.alias?.name ?? originalName), + alias: aliasName, span: table.span, rule: matchedRule, }; } - #classifyBoundTable(table: BoundTableReference): + #classifyBoundTable( + table: BoundTableReference, + catalog: Catalog | undefined, + ): | { matched: false; shadowed: false } | { matched: true; @@ -640,7 +679,7 @@ class TenantScopingPolicy implements CompilerPolicy { ambiguous?: readonly string[]; requiresQualifiedName?: boolean; } { - const match = collectTenantScopeMatchFromBound(table, this.#scopeRules); + const match = collectTenantScopeMatchFromBound(table, this.#scopeRules, catalog); const matchedRules = dedupeTenantScopeRules([...match.exactRules, ...match.shortRules]); const matchedRule = match.exactRules[0]; if (!matchedRule) { @@ -666,7 +705,9 @@ class TenantScopingPolicy implements CompilerPolicy { }; } - const matchedName = match.names.find((name) => matchedRule.tables.has(name)); + const matchedName = match.names.find((name) => + tenantScopeRuleMatchesName(matchedRule, name, catalog, "exact"), + ); return { matched: true, @@ -701,10 +742,12 @@ function createTenantPredicate( alias: string, column: string, value: unknown, + scopeValueType: TenantScopeValueType, span: SourceSpan, ): ExpressionNode { const left = createQualifiedReference(alias, column, span); - if (value === null) { + const normalized = normalizePolicyValue(value, scopeValueType); + if (normalized.value === null) { return { kind: "IsNullExpression", span: left.span, @@ -713,7 +756,7 @@ function createTenantPredicate( } satisfies IsNullExpressionNode; } - const right = createPolicyValueExpression(value, span); + const right = createPolicyValueExpression(normalized, span); return { kind: "BinaryExpression", span: mergeSpans(left.span, right.span), @@ -768,72 +811,22 @@ function createIdentifier(name: string, span: SourceSpan): IdentifierNode { } as IdentifierNode); } -function createPolicyValueExpression(value: unknown, span: SourceSpan): LiteralNode { - if ( - typeof value === "string" || - typeof value === "boolean" || - value === null || - typeof value === "number" || - typeof value === "bigint" - ) { - return createLiteral(value, span); - } - - throw new PolicyUsageError( - 'Policy "tenant-scoping" only supports string, number, bigint, boolean, or null tenant values.', - { policyName: "tenant-scoping" }, - ); -} - -function createLiteral( - value: string | number | bigint | boolean | null, - span: SourceSpan, -): LiteralNode { - if (typeof value === "bigint") { - assertPositiveUint64(value); - return { - kind: "Literal", - span, - literalType: "integer", - value: value.toString(), - }; - } - - if (typeof value === "number") { - assertFiniteNumber(value); - assertSafeIntegerLiteral(value); - return { - kind: "Literal", - span, - literalType: Number.isInteger(value) ? "integer" : "decimal", - value: String(value), - }; - } - - if (typeof value === "string") { - return { - kind: "Literal", - span, - literalType: "string", - value, - }; - } - - if (typeof value === "boolean") { +function createPolicyValueExpression(value: NormalizedPolicyValue, span: SourceSpan): LiteralNode { + if (value.literalType === "null") { return { kind: "Literal", span, - literalType: "boolean", - value, + literalType: "null", + value: null, }; } return { kind: "Literal", span, - literalType: "null", - value: null, - }; + literalType: value.literalType, + value: value.value, + } as LiteralNode; } function assertPositiveUint64(value: bigint): void { @@ -867,7 +860,7 @@ function hasRequiredTenantScope( expression: BoundExpression | undefined, alias: string, column: string, - expectedValue: string | boolean | null, + expectedValue: NormalizedPolicyValue, ): boolean { if (!expression) { return false; @@ -905,9 +898,9 @@ function isTenantComparison( expression: BoundExpression, alias: string, column: string, - expectedValue: string | boolean | null, + expectedValue: NormalizedPolicyValue, ): boolean { - if (expectedValue === null) { + if (expectedValue.literalType === "null") { return ( expression.kind === "BoundIsNullExpression" && !expression.negated && @@ -941,36 +934,86 @@ function isScopedColumnReference( function isExpectedLiteral( expression: BoundExpression, - expectedValue: string | boolean | null, + expectedValue: NormalizedPolicyValue, ): boolean { return ( expression.kind === "BoundLiteral" && - normalizePolicyValue(expression.value) === expectedValue + expression.literalType === expectedValue.literalType && + expression.value === expectedValue.value ); } -function normalizePolicyValue(value: unknown): string | boolean | null { - if (typeof value === "bigint") { +function normalizePolicyValue( + value: unknown, + scopeValueType: TenantScopeValueType, +): NormalizedPolicyValue { + if (value === null) { + return { + literalType: "null", + value: null, + }; + } + + if (scopeValueType === "string") { + if (typeof value === "string") { + return { + literalType: "string", + value, + }; + } + + throw createTenantScopeValueTypeError(scopeValueType, value); + } + + if (scopeValueType === "bigint") { + if (typeof value !== "bigint") { + throw createTenantScopeValueTypeError(scopeValueType, value); + } + assertPositiveUint64(value); - return value.toString(); + return { + literalType: "integer", + value: value.toString(), + }; } - if (typeof value === "number") { + if (scopeValueType === "number") { + if (typeof value !== "number") { + throw createTenantScopeValueTypeError(scopeValueType, value); + } + assertFiniteNumber(value); assertSafeIntegerLiteral(value); - return String(value); + return { + literalType: Number.isInteger(value) ? "integer" : "decimal", + value: String(value), + }; } - if (typeof value === "string" || typeof value === "boolean" || value === null) { - return value; + if (typeof value === "boolean") { + return { + literalType: "boolean", + value, + }; } - throw new PolicyUsageError( - 'Policy "tenant-scoping" only supports string, number, bigint, boolean, or null tenant values.', + throw createTenantScopeValueTypeError(scopeValueType, value); +} + +function createTenantScopeValueTypeError( + scopeValueType: TenantScopeValueType, + value: unknown, +): PolicyUsageError { + return new PolicyUsageError( + `Policy "${TENANT_SCOPING_POLICY_NAME}" requires ${scopeValueType} tenant values; received ${describePolicyValueType(value)}.`, { policyName: TENANT_SCOPING_POLICY_NAME }, ); } +function describePolicyValueType(value: unknown): string { + return value === null ? "null" : typeof value; +} + function validateTenantScopeRules( options: TenantScopingPolicyOptions, policyName: string, @@ -999,6 +1042,7 @@ function validateTenantScopeRules( { allowQualified: false }, ), contextKey: validateContextKey(scope.contextKey, policyName), + scopeValueType: validateScopeValueType(scope.scopeValueType, policyName), }; }); } @@ -1091,6 +1135,24 @@ function validateContextKey(value: string, policyName: string): string { return trimmed; } +function validateScopeValueType( + value: TenantScopingScopeOptions["scopeValueType"], + policyName: string, +): TenantScopeValueType { + if (typeof value === "undefined") { + return "string"; + } + + if (value === "string" || value === "number" || value === "bigint" || value === "boolean") { + return value; + } + + throw new PolicyConfigurationError( + `Policy "${policyName}" requires scopeValueType to be string, number, bigint, or boolean.`, + { policyName }, + ); +} + function resolveTenantScopeContextValues( rules: readonly TenantScopeRule[], context: Readonly>, @@ -1137,8 +1199,50 @@ function getTenantScopeContextValue( function matchTenantScopeRules( rules: readonly TenantScopeRule[], names: readonly string[], + catalog: Catalog | undefined, + mode: "exact" | "short", ): readonly TenantScopeRule[] { - return rules.filter((rule) => names.some((name) => rule.tables.has(name))); + return rules.filter((rule) => + names.some((name) => tenantScopeRuleMatchesName(rule, name, catalog, mode)), + ); +} + +function tenantScopeRuleMatchesName( + rule: TenantScopeRule, + name: string, + catalog: Catalog | undefined, + mode: "exact" | "short", +): boolean { + if (rule.tables.has(name)) { + return true; + } + + if (!catalog || mode === "short") { + return false; + } + + for (const table of rule.tables) { + const resolved = catalog.getTable({ parts: table.split(".") }); + if (resolved?.path.parts.map((part) => normalizeIdentifier(part)).join(".") === name) { + return true; + } + } + + return false; +} + +function findScopedTableName( + names: readonly string[], + rules: readonly TenantScopeRule[], + catalog: Catalog | undefined, +): string | undefined { + for (const name of names) { + if (rules.some((rule) => tenantScopeRuleMatchesName(rule, name, catalog, "exact"))) { + return name; + } + } + + return undefined; } function collectTenantScopeMatchFromAst( @@ -1158,8 +1262,8 @@ function collectTenantScopeMatchFromAst( const shortNames = schemaQualified ? uniqueNames([originalName, canonicalShortName]) : []; return { - exactRules: matchTenantScopeRules(rules, exactNames), - shortRules: matchTenantScopeRules(rules, shortNames), + exactRules: matchTenantScopeRules(rules, exactNames, catalog, "exact"), + shortRules: matchTenantScopeRules(rules, shortNames, catalog, "short"), names: uniqueNames([...exactNames, ...shortNames]), canonicalName, schemaQualified, @@ -1169,6 +1273,7 @@ function collectTenantScopeMatchFromAst( function collectTenantScopeMatchFromBound( table: BoundTableReference, rules: readonly TenantScopeRule[], + catalog: Catalog | undefined, ): TenantScopeMatch { const canonicalPath = table.table.path.parts.map((part) => normalizeIdentifier(part)); const canonicalName = canonicalPath.join("."); @@ -1185,13 +1290,20 @@ function collectTenantScopeMatchFromBound( : normalizeIdentifier(table.table.name); const schemaQualified = table.source === "catalog" && table.table.path.parts.length > 1; const exactNames = uniqueNames( - schemaQualified ? [canonicalName, astName] : [canonicalName, astName, shortName], + schemaQualified + ? [canonicalName, astName] + : [ + canonicalName, + astName, + shortName, + table.source === "catalog" ? undefined : table.alias, + ], ); const shortNames = schemaQualified ? uniqueNames([shortName, canonicalShortName]) : []; return { - exactRules: matchTenantScopeRules(rules, exactNames), - shortRules: matchTenantScopeRules(rules, shortNames), + exactRules: matchTenantScopeRules(rules, exactNames, catalog, "exact"), + shortRules: matchTenantScopeRules(rules, shortNames, catalog, "short"), names: uniqueNames([...exactNames, ...shortNames]), canonicalName, schemaQualified, diff --git a/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts b/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts new file mode 100644 index 0000000..bc6011c --- /dev/null +++ b/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts @@ -0,0 +1,321 @@ +import { + createConnection, + type Connection, + type ConnectionOptions, + type RowDataPacket, +} from "mysql2/promise"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; + +import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; +import { compile } from "../../../src/compiler"; +import { maxLimitPolicy, tenantScopingPolicy, type CompilerPolicy } from "../../../src/policies"; + +const ATTACKER_PROJECT_ID = "project-alpha"; +const VICTIM_PROJECT_ID = "project-bravo"; +const MYSQL_CONFIG = getMysqlConfig(); +const describeMysql84 = MYSQL_CONFIG ? describe : describe.skip; + +const eventsCatalog = new InMemoryCatalog([ + createTableSchema({ + path: ["events"], + columns: ["id", "project_id", "actor_id", "metric", "value"], + }), +]); + +const collationCatalog = new InMemoryCatalog([ + createTableSchema({ + path: ["collation_events"], + columns: ["id", "project_id", "label"], + }), +]); + +const numericCatalog = new InMemoryCatalog([ + createTableSchema({ + path: ["numeric_events"], + columns: ["id", "project_id", "metric"], + }), +]); + +const basePolicies: CompilerPolicy[] = [ + maxLimitPolicy({ + maxLimit: 100, + defaultLimit: 25, + maxOffset: 1_000, + }), +]; + +const eventsPolicies: CompilerPolicy[] = [ + ...basePolicies, + tenantScopingPolicy({ + tables: ["events"], + scopeColumn: "project_id", + contextKey: "projectId", + }), +]; + +const collationPolicies: CompilerPolicy[] = [ + ...basePolicies, + tenantScopingPolicy({ + tables: ["collation_events"], + scopeColumn: "project_id", + contextKey: "projectId", + }), +]; + +const numericPolicies: CompilerPolicy[] = [ + ...basePolicies, + tenantScopingPolicy({ + tables: ["numeric_events"], + scopeColumn: "project_id", + contextKey: "projectId", + scopeValueType: "bigint", + }), +]; + +let db: Connection; + +type QueryRow = Record; + +function getMysqlConfig(): ConnectionOptions | string | undefined { + const url = process.env.VOIGHT_MYSQL84_URL; + if (url) { + return url; + } + + const port = process.env.VOIGHT_MYSQL84_PORT; + if (!port) { + return undefined; + } + + return { + host: process.env.VOIGHT_MYSQL84_HOST ?? "127.0.0.1", + port: Number(port), + user: process.env.VOIGHT_MYSQL84_USER ?? "root", + password: process.env.VOIGHT_MYSQL84_PASSWORD ?? "voightpass", + database: process.env.VOIGHT_MYSQL84_DATABASE ?? "voight", + }; +} + +function compileScoped( + sql: string, + options: { + catalog?: InMemoryCatalog; + policies?: readonly CompilerPolicy[]; + projectId?: unknown; + } = {}, +) { + return compile(sql, { + catalog: options.catalog ?? eventsCatalog, + policies: options.policies ?? eventsPolicies, + policyContext: { + projectId: Object.hasOwn(options, "projectId") + ? options.projectId + : ATTACKER_PROJECT_ID, + }, + debug: true, + }); +} + +async function executeScoped( + sql: string, + options: { + catalog?: InMemoryCatalog; + policies?: readonly CompilerPolicy[]; + projectId?: unknown; + } = {}, +) { + const result = compileScoped(sql, options); + expect(result.ok, JSON.stringify(result.diagnostics, null, 2)).toBe(true); + if (!result.ok) { + throw new Error("Compilation unexpectedly failed."); + } + + const [rows] = await db.query(result.emitted!.sql); + return { + result, + rows: rows as QueryRow[], + }; +} + +function projectIds(rows: readonly QueryRow[]): string[] { + return rows + .flatMap((row) => Object.entries(row)) + .filter(([key, value]) => key.toLowerCase().includes("project_id") && value !== null) + .map(([, value]) => String(value)); +} + +function expectNoVictimProjectIds(rows: readonly QueryRow[]): void { + expect(projectIds(rows)).not.toContain(VICTIM_PROJECT_ID); +} + +describeMysql84("MySQL 8.4 project_id isolation against mixed-project rows", () => { + beforeAll(async () => { + db = + typeof MYSQL_CONFIG === "string" + ? await createConnection(MYSQL_CONFIG) + : await createConnection(MYSQL_CONFIG!); + + await db.query("DROP TABLE IF EXISTS events"); + await db.query("DROP TABLE IF EXISTS collation_events"); + await db.query("DROP TABLE IF EXISTS numeric_events"); + await db.query(`CREATE TABLE events ( + id BIGINT PRIMARY KEY, + project_id VARCHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL, + actor_id BIGINT NOT NULL, + metric VARCHAR(64) NOT NULL, + value BIGINT NOT NULL + )`); + await db.query(`CREATE TABLE collation_events ( + id BIGINT PRIMARY KEY, + project_id VARCHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci NOT NULL, + label VARCHAR(64) NOT NULL + )`); + await db.query(`CREATE TABLE numeric_events ( + id BIGINT PRIMARY KEY, + project_id BIGINT NOT NULL, + metric VARCHAR(64) NOT NULL + )`); + await db.query("INSERT INTO events (id, project_id, actor_id, metric, value) VALUES ?", [ + [ + [1, ATTACKER_PROJECT_ID, 10, "login", 5], + [2, ATTACKER_PROJECT_ID, 11, "export", 2], + [3, ATTACKER_PROJECT_ID, 12, "login", 8], + [4, VICTIM_PROJECT_ID, 20, "login", 999], + [5, VICTIM_PROJECT_ID, 21, "export", 777], + [6, VICTIM_PROJECT_ID, 22, "billing", 555], + ], + ]); + await db.query("INSERT INTO collation_events (id, project_id, label) VALUES ?", [ + [ + [1, ATTACKER_PROJECT_ID, "lowercase-project"], + [2, ATTACKER_PROJECT_ID.toUpperCase(), "uppercase-project"], + [3, VICTIM_PROJECT_ID, "different-project"], + ], + ]); + await db.query("INSERT INTO numeric_events (id, project_id, metric) VALUES ?", [ + [ + [1, 1, "alpha-login"], + [2, 1, "alpha-export"], + [3, 2, "bravo-secret"], + ], + ]); + }); + + afterAll(async () => { + await db?.end(); + }); + + test("OR predicates and direct victim filters cannot widen past the project guard", async () => { + const { result, rows } = await executeScoped( + `SELECT id, project_id + FROM events + WHERE project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 + ORDER BY id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("AND `events`.`project_id` = 'project-alpha'"); + expect(rows).toEqual([ + { id: 1, project_id: ATTACKER_PROJECT_ID }, + { id: 2, project_id: ATTACKER_PROJECT_ID }, + { id: 3, project_id: ATTACKER_PROJECT_ID }, + ]); + expectNoVictimProjectIds(rows); + }); + + test("LEFT JOIN predicates cannot pull victim-side rows", async () => { + const { result, rows } = await executeScoped( + `SELECT e.project_id, other.project_id AS other_project_id + FROM events AS e + LEFT JOIN events AS other + ON other.project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 + ORDER BY e.id, other.id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); + expectNoVictimProjectIds(rows); + }); + + test("scalar, EXISTS, and correlated aggregate subqueries do not expose victim data", async () => { + const { rows } = await executeScoped( + `SELECT COALESCE(( + SELECT victim.project_id + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + LIMIT 1 + ), 'none') AS leaked_project_id, + EXISTS ( + SELECT 1 + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + ) AS has_victim_project, + ( + SELECT COUNT(victim.id) + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + AND victim.metric = e.metric + ) AS victim_metric_count + FROM events AS e + ORDER BY e.id + LIMIT 1`, + ); + + expect(rows).toEqual([ + { + leaked_project_id: "none", + has_victim_project: 0, + victim_metric_count: 0, + }, + ]); + expectNoVictimProjectIds(rows); + }); + + test("documents why project_id needs a binary or case-sensitive MySQL collation", async () => { + const { result, rows } = await executeScoped( + `SELECT id, project_id + FROM collation_events + ORDER BY id + LIMIT 10`, + { + catalog: collationCatalog, + policies: collationPolicies, + }, + ); + + expect(result.emitted?.sql).toContain( + "WHERE `collation_events`.`project_id` = 'project-alpha'", + ); + expect(projectIds(rows)).toEqual([ATTACKER_PROJECT_ID, ATTACKER_PROJECT_ID.toUpperCase()]); + }); + + test("rejects numeric and boolean context values for string project_id guards", () => { + for (const projectId of [0, false]) { + const result = compileScoped("SELECT id, project_id FROM events", { projectId }); + + expect(result.ok, `Unexpected success for ${String(projectId)}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.message).toContain("requires string tenant values"); + } + } + }); + + test("numeric project ids are isolated only through explicit numeric scope typing", async () => { + const { result, rows } = await executeScoped( + `SELECT id, project_id, metric + FROM numeric_events + WHERE project_id = 2 OR 1 = 1 + ORDER BY id + LIMIT 10`, + { + catalog: numericCatalog, + policies: numericPolicies, + projectId: 1n, + }, + ); + + expect(result.emitted?.sql).toContain("`numeric_events`.`project_id` = 1"); + expect(projectIds(rows)).toEqual(["1", "1"]); + }); +}); diff --git a/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts b/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts index 8d5d985..095dc2d 100644 --- a/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts +++ b/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts @@ -211,6 +211,78 @@ describe("project_id isolation against mixed-project rows", () => { expectNoVictimProjectIds(rows); }); + test("function-argument scalar subqueries cannot leak victim project ids", () => { + const { rows } = executeScoped( + `SELECT COALESCE(( + SELECT victim.project_id + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + LIMIT 1 + ), 'none') AS leaked_project_id + FROM events AS e + ORDER BY e.id + LIMIT 1`, + ); + + expect(rows).toEqual([{ leaked_project_id: "none" }]); + expectNoVictimProjectIds(rows); + }); + + test("EXISTS projection subqueries cannot reveal victim project existence", () => { + const { rows } = executeScoped( + `SELECT EXISTS ( + SELECT 1 + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + ) AS has_victim_project + FROM events AS e + ORDER BY e.id + LIMIT 1`, + ); + + expect(rows).toEqual([{ has_victim_project: 0 }]); + expectNoVictimProjectIds(rows); + }); + + test("correlated aggregate subqueries cannot reveal victim metric distribution", () => { + const { rows } = executeScoped( + `SELECT e.metric, + ( + SELECT COUNT(victim.id) + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + AND victim.metric = e.metric + ) AS victim_metric_count + FROM events AS e + ORDER BY e.id + LIMIT 3`, + ); + + expect(rows).toEqual([ + { metric: "login", victim_metric_count: 0 }, + { metric: "export", victim_metric_count: 0 }, + { metric: "login", victim_metric_count: 0 }, + ]); + expectNoVictimProjectIds(rows); + }); + + test("HAVING subqueries cannot reveal victim project existence", () => { + const { rows } = executeScoped( + `SELECT metric, COUNT(id) AS event_count + FROM events + GROUP BY metric + HAVING ( + SELECT COUNT(victim.id) + FROM events AS victim + WHERE victim.project_id = '${VICTIM_PROJECT_ID}' + ) > 0 + ORDER BY metric`, + ); + + expect(rows).toEqual([]); + expectNoVictimProjectIds(rows); + }); + test("self joins scope every table touch independently", () => { const { result, rows } = executeScoped( `SELECT e.id, other.id AS other_id @@ -228,6 +300,22 @@ describe("project_id isolation against mixed-project rows", () => { expectNoVictimProjectIds(rows); }); + test("cross joins scope the joined table even without an original ON clause", () => { + const { result, rows } = executeScoped( + `SELECT e.project_id, other.project_id AS other_project_id + FROM events AS e + CROSS JOIN events AS other + WHERE other.project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 + ORDER BY e.id, other.id + LIMIT 10`, + ); + + expect(result.emitted?.sql).toContain("INNER JOIN `events` AS `other` ON TRUE"); + expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); + expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); + expectNoVictimProjectIds(rows); + }); + test("OR predicates cannot widen past the injected project guard", () => { const { result, rows } = executeScoped( `SELECT id, project_id diff --git a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts index e9a01bc..c3ee346 100644 --- a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts +++ b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts @@ -153,4 +153,36 @@ describe("schema-qualified catalogs", () => { expect(result.emitted?.sql).toContain("`public_stats`.`tenant_id` = 'tenant-123'"); } }); + + test("scopes direct physical access when the schema-qualified table is protected by logical alias", () => { + const aliasCatalog = new AliasCatalog(catalog, [ + createCatalogAlias({ + from: ["public_stats"], + to: ["tracking", "time_series_stats"], + }), + ]); + + const result = compileStrict( + "SELECT metric FROM tracking.time_series_stats WHERE tenant_id = 'tenant-999'", + { + catalog: aliasCatalog, + policies: [ + tenantScopingPolicy({ + tables: ["public_stats"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + }), + ], + policyContext: { + tenantId: "tenant-123", + }, + }, + ); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.emitted?.sql).toContain("FROM `tracking`.`time_series_stats` WHERE"); + expect(result.emitted?.sql).toContain("`time_series_stats`.`tenant_id` = 'tenant-123'"); + } + }); }); diff --git a/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts b/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts index d0f7bb6..2f55d90 100644 --- a/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts +++ b/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts @@ -61,6 +61,36 @@ describe("tenant scoping security probes", () => { ); }); + test("scopes direct physical access when the logical alias is configured", () => { + const aliasCatalog = new AliasCatalog(createTestCatalog(), [ + createCatalogAlias({ + from: ["projects"], + to: ["internal_projects"], + }), + ]); + + const result = compile( + "SELECT id, name FROM internal_projects WHERE tenant_id = 'tenant-999'", + { + catalog: aliasCatalog, + policies: [ + tenantScopingPolicy({ + tables: ["projects"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + }), + ], + policyContext: { tenantId: "tenant-123" }, + debug: true, + }, + ); + + expect(result.ok).toBe(true); + expect(result.emitted?.sql).toBe( + "SELECT `internal_projects`.`id`, `internal_projects`.`name` FROM `internal_projects` WHERE `internal_projects`.`tenant_id` = 'tenant-999' AND `internal_projects`.`tenant_id` = 'tenant-123'", + ); + }); + test("fails closed for derived-table shadowing of a scoped table name", () => { const result = compile( "SELECT metric FROM (SELECT id, name AS metric, 'tenant-123' AS tenant_id FROM users) AS timeseries", @@ -123,4 +153,25 @@ describe("tenant scoping vulnerability probes", () => { expect(result.ok).toBe(false); expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.PolicyViolation); }); + + test("rejects CTE reference aliases that shadow a scoped table name", () => { + const result = compile( + "WITH planted AS (SELECT id, name AS metric, 'tenant-A' AS tenant_id FROM users) SELECT metric FROM planted AS timeseries", + { + catalog: createTestCatalog(), + policies: [ + tenantScopingPolicy({ + tables: ["timeseries"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + }), + ], + policyContext: { tenantId: "tenant-A" }, + debug: true, + }, + ); + + expect(result.ok).toBe(false); + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.PolicyViolation); + }); }); diff --git a/packages/voight/tests/unit/emitter/tenant-scoping.test.ts b/packages/voight/tests/unit/emitter/tenant-scoping.test.ts index 75ee137..4ce1095 100644 --- a/packages/voight/tests/unit/emitter/tenant-scoping.test.ts +++ b/packages/voight/tests/unit/emitter/tenant-scoping.test.ts @@ -9,6 +9,27 @@ const tenantPolicy = tenantScopingPolicy({ contextKey: "tenantId", }); +const bigintTenantPolicy = tenantScopingPolicy({ + tables: ["timeseries", "orders"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "bigint", +}); + +const numberTenantPolicy = tenantScopingPolicy({ + tables: ["timeseries", "orders"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "number", +}); + +const booleanTenantPolicy = tenantScopingPolicy({ + tables: ["timeseries", "orders"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "boolean", +}); + function compileTenantScoped(sql: string, tenantId: unknown = "tenant-123") { return compileStrict(sql, { policies: [tenantPolicy], @@ -16,9 +37,16 @@ function compileTenantScoped(sql: string, tenantId: unknown = "tenant-123") { }); } +function compileWithPolicy(sql: string, policy = tenantPolicy, tenantId: unknown = "tenant-123") { + return compileStrict(sql, { + policies: [policy], + policyContext: { tenantId }, + }); +} + describe("emitter tenant scoping output", () => { test("emits bigint tenant predicates as exact integer literals", () => { - const result = compileTenantScoped("SELECT metric FROM timeseries", 42n); + const result = compileWithPolicy("SELECT metric FROM timeseries", bigintTenantPolicy, 42n); expect(result.ok).toBe(true); if (!result.ok) { return; @@ -28,7 +56,11 @@ describe("emitter tenant scoping output", () => { }); test("emits the largest supported uint64 tenant value without precision loss", () => { - const result = compileTenantScoped("SELECT metric FROM timeseries", 18446744073709551615n); + const result = compileWithPolicy( + "SELECT metric FROM timeseries", + bigintTenantPolicy, + 18446744073709551615n, + ); expect(result.ok).toBe(true); if (!result.ok) { return; @@ -76,18 +108,29 @@ describe("emitter tenant scoping output", () => { test("renders tenant scope values according to their literal kind", () => { const cases = [ - { tenantId: "my-tenant", fragment: "'my-tenant'" }, - { tenantId: 42, fragment: "= 42" }, - { tenantId: true, fragment: "= TRUE" }, - { tenantId: null, fragment: "IS NULL" }, + { policy: tenantPolicy, tenantId: "my-tenant", fragment: "'my-tenant'" }, + { policy: numberTenantPolicy, tenantId: 42, fragment: "= 42" }, + { policy: booleanTenantPolicy, tenantId: true, fragment: "= TRUE" }, + { policy: tenantPolicy, tenantId: null, fragment: "IS NULL" }, ] as const; - for (const { tenantId, fragment } of cases) { - const result = compileTenantScoped("SELECT metric FROM timeseries", tenantId); + for (const { policy, tenantId, fragment } of cases) { + const result = compileWithPolicy("SELECT metric FROM timeseries", policy, tenantId); expect(result.ok, `Failed for tenant value ${String(tenantId)}`).toBe(true); if (result.ok) { expect(result.emitted?.sql).toContain(fragment); } } }); + + test("rejects numeric and boolean values for default string tenant guards", () => { + for (const tenantId of [42, 42n, true, false]) { + const result = compileTenantScoped("SELECT metric FROM timeseries", tenantId); + + expect(result.ok, `Unexpected success for ${String(tenantId)}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.message).toContain("requires string tenant values"); + } + } + }); }); diff --git a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts index 4fa0fea..3937fb6 100644 --- a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts +++ b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts @@ -178,6 +178,18 @@ describe("tenantScopingPolicy boundaries", () => { scopeColumn: "tenant_id", contextKey: "tenantId", }); + const bigintPolicy = tenantScopingPolicy({ + tables: ["timeseries"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "bigint", + }); + const numberPolicy = tenantScopingPolicy({ + tables: ["timeseries"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "number", + }); test("enforcement rejects semantically equivalent but non-canonical tenant predicates", () => { // Enforcement is intentionally syntactic. This query is logically scoped, but @@ -370,7 +382,7 @@ describe("tenantScopingPolicy boundaries", () => { test("rejects bigint tenant values at zero", () => { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), - policies: [policy], + policies: [bigintPolicy], policyContext: { tenantId: 0n }, debug: true, }); @@ -385,7 +397,7 @@ describe("tenantScopingPolicy boundaries", () => { test("rejects negative bigint tenant values", () => { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), - policies: [policy], + policies: [bigintPolicy], policyContext: { tenantId: -1n }, debug: true, }); @@ -400,7 +412,7 @@ describe("tenantScopingPolicy boundaries", () => { test("rejects bigint tenant values above uint64", () => { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), - policies: [policy], + policies: [bigintPolicy], policyContext: { tenantId: 18446744073709551616n }, debug: true, }); @@ -415,7 +427,7 @@ describe("tenantScopingPolicy boundaries", () => { test("rejects unsafe integer number tenant values that should be bigint", () => { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), - policies: [policy], + policies: [numberPolicy], policyContext: { tenantId: Number.MAX_SAFE_INTEGER + 1 }, debug: true, }); @@ -433,7 +445,7 @@ describe("tenantScopingPolicy boundaries", () => { for (const tenantId of cases) { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), - policies: [policy], + policies: [numberPolicy], policyContext: { tenantId }, debug: true, }); From bd20b5d6817f1f718eb6618c9bcd5536eb91469c Mon Sep 17 00:00:00 2001 From: lukaskratzel Date: Wed, 13 May 2026 09:56:06 +0200 Subject: [PATCH 3/3] Refine tenant isolation hardening --- README.md | 20 +- packages/voight/README.md | 19 +- .../voight/src/policies/allowed-functions.ts | 4 +- packages/voight/src/policies/index.ts | 23 +- packages/voight/src/policies/max-limit.ts | 29 +- .../src/policies/supported-operators.ts | 4 +- .../voight/src/policies/tenant-scoping.ts | 9 +- .../voight/tests/e2e/tenant-scoping.test.ts | 84 +++- .../integration/compiler/pipeline.test.ts | 78 +++- .../compiler/public-surface.test.ts | 1 + .../security/compile-hardening.test.ts | 70 ++- .../mysql84-project-isolation.test.ts | 10 +- .../security/policy-guardrails.test.ts | 1 + .../project-id-isolation-sqlite.test.ts | 406 ------------------ .../schema-qualified-catalogs.test.ts | 5 + ...ma-qualified-tenant-scoping-probes.test.ts | 1 + .../tenant-scoping-regression.test.ts | 8 +- .../security/tenant-scoping-shadowing.test.ts | 7 + .../wildcard-projection-regression.test.ts | 4 +- .../tests/unit/emitter/escaping.test.ts | 14 +- .../tests/unit/emitter/parameters.test.ts | 1 + .../tests/unit/emitter/tenant-scoping.test.ts | 7 +- .../voight/tests/unit/enforcer/core.test.ts | 35 +- .../unit/enforcer/policy-hardening.test.ts | 119 ++++- .../unit/enforcer/policy-resolution.test.ts | 6 +- .../unit/enforcer/tenant-scoping.test.ts | 3 + .../tests/unit/parser/ambiguity.test.ts | 6 +- .../tests/unit/parser/structure.test.ts | 6 +- .../unit/rewriter/tenant-scoping.test.ts | 13 +- 29 files changed, 472 insertions(+), 521 deletions(-) delete mode 100644 packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts diff --git a/README.md b/README.md index ab1ae2e..15df63a 100644 --- a/README.md +++ b/README.md @@ -80,15 +80,22 @@ By default, `compile(...)` returns a public-safe result surface: emitted SQL on `voight` currently ships with these policy helpers: - `tenantScopingPolicy(...)` to inject and enforce tenant filters on configured tables -- `maxLimitPolicy(...)` to cap `LIMIT`, optionally cap `OFFSET`, and optionally add a default `LIMIT` +- `maxLimitPolicy(...)` to cap the outer `LIMIT`, optionally cap outer `OFFSET`, and optionally add a default outer `LIMIT` - `allowedFunctionsPolicy(...)` to restrict callable SQL functions and `CURRENT_*` keywords - `supportedOperatorsPolicy()` to reject operators outside the supported policy surface -`tenantScopingPolicy(...)` treats scope values as strings by default. If a scope column is -numeric or boolean, configure `scopeValueType` explicitly, for example -`scopeValueType: "bigint"` for a `BIGINT project_id`. For MySQL string scope columns, -use binary or case-sensitive collation/comparison semantics so `project-alpha` cannot also -match case/accent variants such as `PROJECT-ALPHA`. +`maxLimitPolicy(...)` constrains the final result set by default, so nested selects are not +limited unless `recursive: true` is configured. + +`tenantScopingPolicy(...)` requires `scopeValueType` on every scope rule and enforces that type +at runtime. Use `scopeValueType: "string"` for string scope columns, or configure the matching +numeric or boolean type, for example `scopeValueType: "bigint"` for a `BIGINT project_id`. + +String tenant scopes require careful database configuration. MySQL in particular has many +string comparison gotchas around implicit casts, collations, charsets, padding, and +case/accent equivalence. Avoid string tenant identifiers unless those semantics are deliberate; +if you use them, choose binary or otherwise case-sensitive comparison semantics so values such +as `project-alpha` and `PROJECT-ALPHA` cannot share a scope unintentionally. ## Example @@ -118,6 +125,7 @@ const result = compile( tables: ["tracking.time_series_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { diff --git a/packages/voight/README.md b/packages/voight/README.md index b208830..7aa0809 100644 --- a/packages/voight/README.md +++ b/packages/voight/README.md @@ -57,6 +57,7 @@ const result = compile( tables: ["tracking.time_series_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -133,14 +134,22 @@ Use `AliasCatalog` and `createCatalogAlias(...)` if your public logical table na ## Built-in Policies - `tenantScopingPolicy(...)` injects tenant predicates during rewrite and verifies them during enforcement. -- `maxLimitPolicy(...)` caps `LIMIT`, can cap `OFFSET`, and can add a default `LIMIT`. +- `maxLimitPolicy(...)` caps the outer `LIMIT`, can cap the outer `OFFSET`, and can add a default outer `LIMIT`. - `allowedFunctionsPolicy(...)` allowlists function calls and `CURRENT_*` keywords. - `supportedOperatorsPolicy()` rejects operators outside the supported policy surface. -`tenantScopingPolicy(...)` defaults scope values to strings. If the scoped column is numeric or -boolean, set `scopeValueType` explicitly, for example `scopeValueType: "bigint"` for a -`BIGINT project_id`. On MySQL, string scope columns should use binary or case-sensitive -collation/comparison semantics so equivalent case/accent variants do not cross tenant boundaries. +`maxLimitPolicy(...)` constrains the final result set by default, so nested selects are not +limited unless `recursive: true` is configured. + +`tenantScopingPolicy(...)` requires `scopeValueType` on every scope rule and enforces that type +at runtime. Use `scopeValueType: "string"` for string scope columns, or configure the matching +numeric or boolean type, for example `scopeValueType: "bigint"` for a `BIGINT project_id`. + +String tenant scopes require careful database configuration. MySQL in particular has many string +comparison gotchas around implicit casts, collations, charsets, padding, and case/accent +equivalence. Avoid string tenant identifiers unless those semantics are deliberate; if you use +them, choose binary or otherwise case-sensitive comparison semantics so values such as +`project-alpha` and `PROJECT-ALPHA` cannot share a scope unintentionally. ## Repository diff --git a/packages/voight/src/policies/allowed-functions.ts b/packages/voight/src/policies/allowed-functions.ts index 89fe4e7..100edd4 100644 --- a/packages/voight/src/policies/allowed-functions.ts +++ b/packages/voight/src/policies/allowed-functions.ts @@ -17,8 +17,10 @@ export function allowedFunctionsPolicy(options: AllowedFunctionsPolicyOptions): return new AllowedFunctionsPolicy(options); } +export const ALLOWED_FUNCTIONS_POLICY_NAME = "allowed-functions"; + class AllowedFunctionsPolicy implements CompilerPolicy { - readonly name = "allowed-functions"; + readonly name = ALLOWED_FUNCTIONS_POLICY_NAME; readonly #allowedFunctions: ReadonlySet; constructor(options: AllowedFunctionsPolicyOptions) { diff --git a/packages/voight/src/policies/index.ts b/packages/voight/src/policies/index.ts index 3de8e5d..e5048dc 100644 --- a/packages/voight/src/policies/index.ts +++ b/packages/voight/src/policies/index.ts @@ -1,6 +1,10 @@ -import { allowedFunctionsPolicy, type AllowedFunctionsPolicyOptions } from "./allowed-functions"; -import { maxLimitPolicy, type MaxLimitPolicyOptions } from "./max-limit"; -import { supportedOperatorsPolicy } from "./supported-operators"; +import { + ALLOWED_FUNCTIONS_POLICY_NAME, + allowedFunctionsPolicy, + type AllowedFunctionsPolicyOptions, +} from "./allowed-functions"; +import { MAX_LIMIT_POLICY_NAME, maxLimitPolicy, type MaxLimitPolicyOptions } from "./max-limit"; +import { SUPPORTED_OPERATORS_POLICY_NAME, supportedOperatorsPolicy } from "./supported-operators"; import { PolicyConflictError, PolicyConfigurationError, @@ -11,6 +15,7 @@ import { type PolicySelectionOptions, } from "./shared"; import { + TENANT_SCOPING_POLICY_NAME, tenantScopingPolicy, type TenantScopingPolicyOptions, type TenantScopingScopeOptions, @@ -34,6 +39,8 @@ export type { }; export { + ALLOWED_FUNCTIONS_POLICY_NAME, + MAX_LIMIT_POLICY_NAME, allowedFunctionsPolicy, maxLimitPolicy, PolicyConflictError, @@ -41,10 +48,18 @@ export { PolicyDiagnosticError, PolicyError, PolicyUsageError, + SUPPORTED_OPERATORS_POLICY_NAME, + TENANT_SCOPING_POLICY_NAME, supportedOperatorsPolicy, tenantScopingPolicy, }; export function resolvePolicies(options: PolicySelectionOptions = {}) { - return dedupePoliciesByName(options.policies ?? []); + const policies = dedupePoliciesByName(options.policies ?? []); + if (policies.some((policy) => policy.name === ALLOWED_FUNCTIONS_POLICY_NAME)) { + return policies; + } + + // If no allowed functions policy is provided, add a default one that disallows all functions. + return [allowedFunctionsPolicy({ allowedFunctions: new Set() }), ...policies]; } diff --git a/packages/voight/src/policies/max-limit.ts b/packages/voight/src/policies/max-limit.ts index 91e1e50..23646f6 100644 --- a/packages/voight/src/policies/max-limit.ts +++ b/packages/voight/src/policies/max-limit.ts @@ -14,17 +14,26 @@ export interface MaxLimitPolicyOptions { readonly maxLimit: number; readonly maxOffset?: number; readonly defaultLimit?: number; + /** + * When false, the policy controls only the final result set returned by the + * compiled query. Enable this to require every nested SELECT to carry its + * own bounded LIMIT/OFFSET as well. + */ + readonly recursive?: boolean; } +export const MAX_LIMIT_POLICY_NAME = "max-limit"; + export function maxLimitPolicy(options: MaxLimitPolicyOptions): CompilerPolicy { return new MaxLimitPolicy(options); } class MaxLimitPolicy implements CompilerPolicy { - readonly name = "max-limit"; + readonly name = MAX_LIMIT_POLICY_NAME; readonly #maxLimit: number; readonly #maxOffset?: number; readonly #defaultLimit?: number; + readonly #recursive: boolean; constructor(options: MaxLimitPolicyOptions) { this.#maxLimit = validateNonNegativeInteger(options.maxLimit, "maxLimit"); @@ -36,11 +45,12 @@ class MaxLimitPolicy implements CompilerPolicy { typeof options.defaultLimit === "undefined" ? undefined : validateNonNegativeInteger(options.defaultLimit, "defaultLimit"); + this.#recursive = options.recursive ?? false; if (typeof this.#defaultLimit !== "undefined" && this.#defaultLimit > this.#maxLimit) { throw new PolicyConfigurationError( - `Policy "max-limit" requires defaultLimit (${this.#defaultLimit}) to be less than or equal to maxLimit (${this.#maxLimit}).`, - { policyName: "max-limit" }, + `Policy "${MAX_LIMIT_POLICY_NAME}" requires defaultLimit (${this.#defaultLimit}) to be less than or equal to maxLimit (${this.#maxLimit}).`, + { policyName: MAX_LIMIT_POLICY_NAME }, ); } } @@ -50,12 +60,25 @@ class MaxLimitPolicy implements CompilerPolicy { return query; } + if (!this.#recursive) { + return query.body.limit + ? query + : { + ...query, + body: addDefaultLimit(query.body, this.#defaultLimit), + }; + } + return mapQueryAst(query, (select) => select.limit ? select : addDefaultLimit(select, this.#defaultLimit!), ); } enforce(bound: BoundQuery): readonly Diagnostic[] { + if (!this.#recursive) { + return this.#validateSelectLimit(bound.body) ?? []; + } + return collectBoundPolicyDiagnostics(bound, { select: (select) => this.#validateSelectLimit(select), }); diff --git a/packages/voight/src/policies/supported-operators.ts b/packages/voight/src/policies/supported-operators.ts index 9db7638..6b385b3 100644 --- a/packages/voight/src/policies/supported-operators.ts +++ b/packages/voight/src/policies/supported-operators.ts @@ -29,12 +29,14 @@ const SUPPORTED_BINARY_OPERATORS = new Set([ const SUPPORTED_UNARY_OPERATORS = new Set(["-", "NOT"]); +export const SUPPORTED_OPERATORS_POLICY_NAME = "supported-operators"; + export function supportedOperatorsPolicy(): CompilerPolicy { return new SupportedOperatorsPolicy(); } class SupportedOperatorsPolicy implements CompilerPolicy { - readonly name = "supported-operators"; + readonly name = SUPPORTED_OPERATORS_POLICY_NAME; enforce(bound: BoundQuery): readonly Diagnostic[] { return collectBoundPolicyDiagnostics(bound, { diff --git a/packages/voight/src/policies/tenant-scoping.ts b/packages/voight/src/policies/tenant-scoping.ts index 0b62656..f37b0a3 100644 --- a/packages/voight/src/policies/tenant-scoping.ts +++ b/packages/voight/src/policies/tenant-scoping.ts @@ -33,7 +33,7 @@ import { type PolicyRewriteContext, } from "./shared"; -const TENANT_SCOPING_POLICY_NAME = "tenant-scoping"; +export const TENANT_SCOPING_POLICY_NAME = "tenant-scoping"; const MAX_UINT64 = (1n << 64n) - 1n; const MIN_POSITIVE_UINT64 = 1n; @@ -43,7 +43,7 @@ export interface TenantScopingScopeOptions { readonly tables: readonly string[]; readonly scopeColumn: string; readonly contextKey: string; - readonly scopeValueType?: TenantScopeValueType; + readonly scopeValueType: TenantScopeValueType; } export type TenantScopingPolicyOptions = @@ -1140,7 +1140,10 @@ function validateScopeValueType( policyName: string, ): TenantScopeValueType { if (typeof value === "undefined") { - return "string"; + throw new PolicyConfigurationError( + `Policy "${policyName}" requires scopeValueType to be configured explicitly.`, + { policyName }, + ); } if (value === "string" || value === "number" || value === "bigint" || value === "boolean") { diff --git a/packages/voight/tests/e2e/tenant-scoping.test.ts b/packages/voight/tests/e2e/tenant-scoping.test.ts index 9a910f4..034cdc3 100644 --- a/packages/voight/tests/e2e/tenant-scoping.test.ts +++ b/packages/voight/tests/e2e/tenant-scoping.test.ts @@ -3,7 +3,7 @@ import { describe, expect, test, beforeAll, afterAll } from "vitest"; import { compile } from "../../src/compiler"; import { InMemoryCatalog, createTableSchema } from "../../src/catalog"; -import { tenantScopingPolicy } from "../../src/policies"; +import { allowedFunctionsPolicy, tenantScopingPolicy } from "../../src/policies"; /** * END-TO-END VERIFICATION: Tenant scoping now works for all query forms. @@ -38,12 +38,14 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["metrics"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const joinTenantPolicy = tenantScopingPolicy({ tables: ["users", "orders"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const mixedColumnTenantPolicy = tenantScopingPolicy({ @@ -52,19 +54,24 @@ const mixedColumnTenantPolicy = tenantScopingPolicy({ tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }, { tables: ["subscriptions"], scopeColumn: "account_id", contextKey: "tenantId", + scopeValueType: "string", }, ], }); +const allowedFunctionPolicy = allowedFunctionsPolicy({ + allowedFunctions: new Set(["coalesce", "count"]), +}); function compileTenantScoped(sql: string, tenantId = "tenant-A") { return compile(sql, { catalog, - policies: [tenantPolicy], + policies: [tenantPolicy, allowedFunctionPolicy], policyContext: { tenantId }, debug: true, }); @@ -73,7 +80,7 @@ function compileTenantScoped(sql: string, tenantId = "tenant-A") { function compileJoinTenantScoped(sql: string, tenantId = "tenant-A") { return compile(sql, { catalog, - policies: [joinTenantPolicy], + policies: [joinTenantPolicy, allowedFunctionPolicy], policyContext: { tenantId }, debug: true, }); @@ -82,7 +89,7 @@ function compileJoinTenantScoped(sql: string, tenantId = "tenant-A") { function compileMixedColumnTenantScoped(sql: string, tenantId = "tenant-A") { return compile(sql, { catalog, - policies: [mixedColumnTenantPolicy], + policies: [mixedColumnTenantPolicy, allowedFunctionPolicy], policyContext: { tenantId }, debug: true, }); @@ -155,6 +162,24 @@ describe("E2E: safe queries return only tenant-A data", () => { }); describe("E2E FIXED: expression subqueries no longer leak cross-tenant data", () => { + test("explicit victim predicates on the scoped table return no rows", () => { + const result = compileTenantScoped( + "SELECT metric_name FROM metrics WHERE tenant_id = 'tenant-B' ORDER BY id", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all() as { metric_name: string }[]; + expect(rows).toEqual([]); + }); + + test("CTEs with victim predicates cannot resurrect victim rows", () => { + const result = compileTenantScoped( + "WITH victim_metrics AS (SELECT metric_name FROM metrics WHERE tenant_id = 'tenant-B') SELECT metric_name FROM victim_metrics ORDER BY metric_name", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all() as { metric_name: string }[]; + expect(rows).toEqual([]); + }); + test("EXISTS subquery is now scoped — cannot detect tenant-B data", () => { const result = compileTenantScoped( "SELECT name FROM users WHERE EXISTS (SELECT 1 FROM metrics WHERE metrics.tenant_id = 'tenant-B')", @@ -193,6 +218,17 @@ describe("E2E FIXED: expression subqueries no longer leak cross-tenant data", () expect(values).not.toContain(99.9); }); + test("function-argument scalar subqueries cannot extract tenant-B data", () => { + const result = compileTenantScoped( + "SELECT COALESCE((SELECT metric_name FROM metrics WHERE metrics.tenant_id = 'tenant-B' LIMIT 1), 'none') AS leaked_metric FROM users LIMIT 1", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all() as Array<{ + leaked_metric: string; + }>; + expect(rows).toEqual([{ leaked_metric: "none" }]); + }); + test("IN subquery cannot enumerate tenant-B IDs", () => { const result = compileTenantScoped( "SELECT users.id, users.name FROM users WHERE users.id IN (SELECT metrics.id FROM metrics WHERE metrics.tenant_id = 'tenant-B')", @@ -204,6 +240,27 @@ describe("E2E FIXED: expression subqueries no longer leak cross-tenant data", () expect(rows.length).toBe(0); }); + test("IN-list scalar subqueries cannot smuggle tenant-B IDs", () => { + const result = compileTenantScoped( + "SELECT id, metric_name FROM metrics WHERE id IN ((SELECT id FROM metrics WHERE tenant_id = 'tenant-B' LIMIT 1), 1) ORDER BY id", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all() as Array<{ + id: number; + metric_name: string; + }>; + expect(rows).toEqual([{ id: 1, metric_name: "cpu" }]); + }); + + test("HAVING subqueries cannot reveal tenant-B data", () => { + const result = compileTenantScoped( + "SELECT metric_name, COUNT(id) AS metric_count FROM metrics GROUP BY metric_name HAVING (SELECT COUNT(id) FROM metrics WHERE tenant_id = 'tenant-B') > 0 ORDER BY metric_name", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all(); + expect(rows).toEqual([]); + }); + test("LIMIT/OFFSET iteration cannot extract tenant-B data", () => { const leakedNames: string[] = []; @@ -257,6 +314,25 @@ describe("E2E: tenant scoping across joined tables", () => { ]); }); + test("CROSS JOIN scopes the joined table even when WHERE tries to widen it", () => { + const result = compileTenantScoped( + "SELECT m.metric_name, other.metric_name AS other_metric_name FROM metrics AS m CROSS JOIN metrics AS other WHERE other.tenant_id = 'tenant-B' OR 1 = 1 ORDER BY m.id, other.id LIMIT 10", + ); + expect(result.ok).toBe(true); + const rows = db.prepare(toSQLite(result.emitted!.sql)).all() as Array<{ + metric_name: string; + other_metric_name: string; + }>; + expect(rows.length).toBe(4); + expect( + rows.every( + (row) => + !row.metric_name.startsWith("SECRET") && + !row.other_metric_name.startsWith("SECRET"), + ), + ).toBe(true); + }); + test("explicit scope rules can scope joined tables that use different column names", () => { const result = compileMixedColumnTenantScoped( "SELECT u.name, s.plan_name FROM users AS u LEFT JOIN subscriptions AS s ON s.user_id = u.id ORDER BY u.id", diff --git a/packages/voight/tests/integration/compiler/pipeline.test.ts b/packages/voight/tests/integration/compiler/pipeline.test.ts index 6f2e1e0..e84e6e0 100644 --- a/packages/voight/tests/integration/compiler/pipeline.test.ts +++ b/packages/voight/tests/integration/compiler/pipeline.test.ts @@ -173,7 +173,7 @@ describe("compile", () => { ); }); - test("injects the configured default limit into every select", () => { + test("injects the configured default limit only on the outermost select", () => { const result = compile( "WITH recent_orders AS (SELECT user_id FROM orders) SELECT id FROM users WHERE id IN (SELECT user_id FROM recent_orders)", { @@ -189,23 +189,46 @@ describe("compile", () => { } expect(result.emitted?.sql).toBe( - "WITH `recent_orders` AS (SELECT `orders`.`user_id` FROM `orders` LIMIT 25) SELECT `users`.`id` FROM `users` WHERE `users`.`id` IN (SELECT `recent_orders`.`user_id` FROM `recent_orders` LIMIT 25) LIMIT 25", + "WITH `recent_orders` AS (SELECT `orders`.`user_id` FROM `orders`) SELECT `users`.`id` FROM `users` WHERE `users`.`id` IN (SELECT `recent_orders`.`user_id` FROM `recent_orders`) LIMIT 25", ); expect(result.rewrittenAst?.body.limit?.count.kind).toBe("Literal"); if (result.rewrittenAst?.body.limit?.count.kind === "Literal") { expect(result.rewrittenAst.body.limit.count.value).toBe("25"); } + expect(result.rewrittenAst?.with?.ctes[0]?.query.body.limit).toBeUndefined(); + }); + + test("injects the configured default limit into nested selects when recursive", () => { + const result = compile( + "WITH recent_orders AS (SELECT user_id FROM orders) SELECT id FROM users WHERE id IN (SELECT user_id FROM recent_orders)", + { + catalog: createTestCatalog(), + policies: [maxLimitPolicy({ maxLimit: 100, defaultLimit: 25, recursive: true })], + debug: true, + }, + ); + + expect(result.ok).toBe(true); + if (!result.ok) { + return; + } + + expect(result.emitted?.sql).toBe( + "WITH `recent_orders` AS (SELECT `orders`.`user_id` FROM `orders` LIMIT 25) SELECT `users`.`id` FROM `users` WHERE `users`.`id` IN (SELECT `recent_orders`.`user_id` FROM `recent_orders` LIMIT 25) LIMIT 25", + ); expect(result.rewrittenAst?.with?.ctes[0]?.query.body.limit?.count.kind).toBe("Literal"); }); - test("does not apply any function policy unless one is configured", () => { + test("denies functions unless allowed-functions is configured", () => { const result = compile("SELECT SLEEP(10) FROM users", { catalog: createTestCatalog(), debug: true, }); - expect(result.ok).toBe(true); - expect(result.emitted?.sql).toBe("SELECT sleep(10) FROM `users`"); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.DisallowedFunction); + } }); test("rejects dangerous functions when allowed-functions is configured explicitly", () => { @@ -260,6 +283,7 @@ describe("compile", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -282,6 +306,7 @@ describe("compile", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], debug: true, @@ -300,6 +325,7 @@ describe("compile", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], }); @@ -311,17 +337,42 @@ describe("compile", () => { test("supports projection-only and current temporal selects", () => { const inputs = [ ["SELECT 1", "SELECT 1"], + ["SELECT 1;", "SELECT 1"], + ] as const; + + for (const [sql, expected] of inputs) { + const result = compile(sql, { + catalog: createTestCatalog(), + debug: true, + }); + + expect(result.ok).toBe(true); + expect(result.emitted?.sql).toBe(expected); + } + }); + + test("supports current temporal expressions when explicitly allowed", () => { + const inputs = [ ["SELECT now()", "SELECT now()"], ["SELECT (CURRENT_TIMESTAMP)", "SELECT (CURRENT_TIMESTAMP)"], ["SELECT (CURRENT_DATE)", "SELECT (CURRENT_DATE)"], ["SELECT (CURRENT_TIME)", "SELECT (CURRENT_TIME)"], ["SELECT CURRENT_TIME", "SELECT CURRENT_TIME"], - ["SELECT 1;", "SELECT 1"], ] as const; for (const [sql, expected] of inputs) { const result = compile(sql, { catalog: createTestCatalog(), + policies: [ + allowedFunctionsPolicy({ + allowedFunctions: new Set([ + "now", + "current_timestamp", + "current_date", + "current_time", + ]), + }), + ], debug: true, }); @@ -353,6 +404,11 @@ describe("compile", () => { FROM orders`, { catalog: createTestCatalog(), + policies: [ + allowedFunctionsPolicy({ + allowedFunctions: new Set(["round", "coalesce", "nullif"]), + }), + ], debug: true, }, ); @@ -379,6 +435,11 @@ FROM orders`, FROM orders`; const result = compile(sql, { catalog: createTestCatalog(), + policies: [ + allowedFunctionsPolicy({ + allowedFunctions: new Set(["date_add", "date_sub", "adddate", "subdate"]), + }), + ], debug: true, }); @@ -431,6 +492,11 @@ LIMIT 100 OFFSET 20`; const result = compile(sql, { catalog: createTestCatalog(), + policies: [ + allowedFunctionsPolicy({ + allowedFunctions: new Set(["coalesce", "count", "sum"]), + }), + ], debug: true, }); diff --git a/packages/voight/tests/integration/compiler/public-surface.test.ts b/packages/voight/tests/integration/compiler/public-surface.test.ts index a68748c..03f1e30 100644 --- a/packages/voight/tests/integration/compiler/public-surface.test.ts +++ b/packages/voight/tests/integration/compiler/public-surface.test.ts @@ -183,6 +183,7 @@ describe("public compile surface", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-A" }, diff --git a/packages/voight/tests/integration/security/compile-hardening.test.ts b/packages/voight/tests/integration/security/compile-hardening.test.ts index bbbbf85..0c4e461 100644 --- a/packages/voight/tests/integration/security/compile-hardening.test.ts +++ b/packages/voight/tests/integration/security/compile-hardening.test.ts @@ -19,45 +19,6 @@ describe("compile hardening", () => { "SELECT 1; SELECT 2", "SET @a = 1", "SELECT id FROM users UNION SELECT id FROM orders", - "SELECT id FROM users INTERSECT SELECT user_id FROM orders", - "SELECT id FROM users EXCEPT SELECT user_id FROM orders", - ]) { - expectBlocked(sql); - } - }); - - test("rejects quoted callable and cast-type injection surfaces", () => { - for (const sql of [ - "SELECT `SLEEP(1); DROP TABLE users; -- `(0)", - "SELECT `GET_LOCK('voight', 10); DROP TABLE users; -- `(0)", - "SELECT CAST(name AS `CHAR); DROP TABLE users; -- `) FROM users", - "SELECT CAST(name AS `CHAR CHARACTER SET utf8mb4 COLLATE utf8mb4_bin`) FROM users", - ]) { - expectBlocked(sql); - } - }); - - test("rejects advanced SQL forms outside the supported SELECT subset", () => { - for (const sql of [ - "WITH RECURSIVE r(n) AS (SELECT 1) SELECT n FROM r", - "SELECT * FROM LATERAL (SELECT id FROM users) AS x", - "SELECT * FROM JSON_TABLE('[1]', '$[*]' COLUMNS(n INT PATH '$')) AS jt", - "TABLE users", - "VALUES ROW(1), ROW(2)", - "SELECT * FROM (VALUES ROW(1)) AS v(id)", - "SELECT id INTO OUTFILE '/tmp/x' FROM users", - "SELECT id FROM users FOR UPDATE", - "SELECT id FROM users LOCK IN SHARE MODE", - "SELECT id FROM users USE INDEX (idx_users_tenant)", - "SELECT name COLLATE utf8mb4_bin FROM users", - "SELECT id FROM users WHERE profile->'$.tenant_id' = 'x'", - "SELECT id FROM users WHERE profile->>'$.tenant_id' = 'x'", - "SELECT id FROM users WHERE (id, tenant_id) IN ((1, 't'))", - "SELECT id FROM users WHERE id > ALL (SELECT user_id FROM orders)", - "SELECT id FROM users WHERE id = ANY (SELECT user_id FROM orders)", - "SELECT SUM(total) OVER (ORDER BY created_at ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM orders", - "SELECT SUM(total) OVER w FROM orders WINDOW w AS (PARTITION BY user_id)", - "SELECT metric, SUM(value) FROM timeseries GROUP BY metric WITH ROLLUP", ]) { expectBlocked(sql); } @@ -68,14 +29,41 @@ describe("compile hardening", () => { "SELECT id FROM users -- where 1=1", "SELECT id FROM users # comment", "SELECT id FROM users /* comment */", - "SELECT /*!80408 SQL_NO_CACHE */ id FROM users", - "SELECT /*+ MAX_EXECUTION_TIME(1) */ id FROM users", "SELECT id FROM users\uFF1B", ]) { expectBlocked(sql); } }); + test("rejects quoted callable and cast-type injection surfaces", () => { + for (const { sql, message } of [ + { + sql: "SELECT `SLEEP(1); DROP TABLE users; -- `(0)", + message: "Function names must be unquoted simple identifiers.", + }, + { + sql: "SELECT `GET_LOCK('voight', 10); DROP TABLE users; -- `(0)", + message: "Function names must be unquoted simple identifiers.", + }, + { + sql: "SELECT CAST(name AS `CHAR); DROP TABLE users; -- `) FROM users", + message: "CAST target type names must be unquoted simple identifiers.", + }, + { + sql: "SELECT CAST(name AS `CHAR CHARACTER SET utf8mb4 COLLATE utf8mb4_bin`) FROM users", + message: "CAST target type names must be unquoted simple identifiers.", + }, + ]) { + const result = expectBlocked(sql); + expect(result.diagnostics).toContainEqual( + expect.objectContaining({ + code: DiagnosticCode.UnsupportedConstruct, + message, + }), + ); + } + }); + test("rejects catalog escape attempts against system and cross-database tables", () => { const unknownTable = expectBlocked("SELECT * FROM information_schema.tables"); expect( diff --git a/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts b/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts index bc6011c..766aca1 100644 --- a/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts +++ b/packages/voight/tests/integration/security/mysql84-project-isolation.test.ts @@ -8,7 +8,12 @@ import { afterAll, beforeAll, describe, expect, test } from "vitest"; import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; import { compile } from "../../../src/compiler"; -import { maxLimitPolicy, tenantScopingPolicy, type CompilerPolicy } from "../../../src/policies"; +import { + allowedFunctionsPolicy, + maxLimitPolicy, + tenantScopingPolicy, + type CompilerPolicy, +} from "../../../src/policies"; const ATTACKER_PROJECT_ID = "project-alpha"; const VICTIM_PROJECT_ID = "project-bravo"; @@ -37,6 +42,7 @@ const numericCatalog = new InMemoryCatalog([ ]); const basePolicies: CompilerPolicy[] = [ + allowedFunctionsPolicy({ allowedFunctions: new Set(["coalesce", "count"]) }), maxLimitPolicy({ maxLimit: 100, defaultLimit: 25, @@ -50,6 +56,7 @@ const eventsPolicies: CompilerPolicy[] = [ tables: ["events"], scopeColumn: "project_id", contextKey: "projectId", + scopeValueType: "string", }), ]; @@ -59,6 +66,7 @@ const collationPolicies: CompilerPolicy[] = [ tables: ["collation_events"], scopeColumn: "project_id", contextKey: "projectId", + scopeValueType: "string", }), ]; diff --git a/packages/voight/tests/integration/security/policy-guardrails.test.ts b/packages/voight/tests/integration/security/policy-guardrails.test.ts index 4676cb3..d5a8c09 100644 --- a/packages/voight/tests/integration/security/policy-guardrails.test.ts +++ b/packages/voight/tests/integration/security/policy-guardrails.test.ts @@ -8,6 +8,7 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); function compileTenantScoped(sql: string, tenantId = "tenant-123") { diff --git a/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts b/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts deleted file mode 100644 index 095dc2d..0000000 --- a/packages/voight/tests/integration/security/project-id-isolation-sqlite.test.ts +++ /dev/null @@ -1,406 +0,0 @@ -import { DatabaseSync } from "node:sqlite"; -import { afterAll, beforeAll, describe, expect, test } from "vitest"; - -import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; -import { compile } from "../../../src/compiler"; -import { maxLimitPolicy, tenantScopingPolicy } from "../../../src/policies"; - -const ATTACKER_PROJECT_ID = "project-alpha"; -const VICTIM_PROJECT_ID = "project-bravo"; - -const catalog = new InMemoryCatalog([ - createTableSchema({ - path: ["events"], - columns: ["id", "project_id", "actor_id", "metric", "value"], - }), -]); - -const policies = [ - maxLimitPolicy({ - maxLimit: 100, - defaultLimit: 25, - maxOffset: 1_000, - }), - tenantScopingPolicy({ - tables: ["events"], - scopeColumn: "project_id", - contextKey: "projectId", - }), -]; - -let db: DatabaseSync; - -type QueryRow = Record; - -function compileScoped(sql: string) { - return compile(sql, { - catalog, - policies, - policyContext: { - projectId: ATTACKER_PROJECT_ID, - }, - debug: true, - }); -} - -function executeScoped(sql: string) { - const result = compileScoped(sql); - expect(result.ok, JSON.stringify(result.diagnostics, null, 2)).toBe(true); - if (!result.ok) { - throw new Error("Compilation unexpectedly failed."); - } - - return { - result, - rows: db.prepare(result.emitted!.sql).all() as QueryRow[], - }; -} - -function projectIds(rows: readonly QueryRow[]): string[] { - return rows - .flatMap((row) => Object.entries(row)) - .filter(([key, value]) => key.toLowerCase().includes("project_id") && value !== null) - .map(([, value]) => String(value)); -} - -function expectNoVictimProjectIds(rows: readonly QueryRow[]): void { - expect(projectIds(rows)).not.toContain(VICTIM_PROJECT_ID); -} - -beforeAll(() => { - db = new DatabaseSync(":memory:"); - db.exec(`CREATE TABLE events ( - id INTEGER PRIMARY KEY, - project_id TEXT NOT NULL, - actor_id INTEGER NOT NULL, - metric TEXT NOT NULL, - value INTEGER NOT NULL - )`); - - db.exec(`INSERT INTO events VALUES - (1, '${ATTACKER_PROJECT_ID}', 10, 'login', 5), - (2, '${ATTACKER_PROJECT_ID}', 11, 'export', 2), - (3, '${ATTACKER_PROJECT_ID}', 12, 'login', 8), - (4, '${VICTIM_PROJECT_ID}', 20, 'login', 999), - (5, '${VICTIM_PROJECT_ID}', 21, 'export', 777), - (6, '${VICTIM_PROJECT_ID}', 22, 'billing', 555) - `); -}); - -afterAll(() => { - db.close(); -}); - -describe("project_id isolation against mixed-project rows", () => { - test("base table reads return only the configured project", () => { - const { result, rows } = executeScoped( - "SELECT id, project_id, metric, value FROM events ORDER BY id LIMIT 10", - ); - - expect(result.emitted?.sql).toContain("WHERE `events`.`project_id` = 'project-alpha'"); - expect(projectIds(rows)).toEqual([ - ATTACKER_PROJECT_ID, - ATTACKER_PROJECT_ID, - ATTACKER_PROJECT_ID, - ]); - expectNoVictimProjectIds(rows); - }); - - test("an explicit victim project predicate returns no rows", () => { - const { result, rows } = executeScoped( - `SELECT id, project_id, metric - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' - ORDER BY id - LIMIT 10`, - ); - - expect(result.emitted?.sql).toContain("`events`.`project_id` = 'project-alpha'"); - expect(rows).toEqual([]); - expectNoVictimProjectIds(rows); - }); - - test("derived tables with victim predicates cannot resurrect victim rows", () => { - const { rows } = executeScoped( - `SELECT d.project_id, d.metric - FROM ( - SELECT project_id, metric - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' - LIMIT 10 - ) AS d - LIMIT 10`, - ); - - expect(rows).toEqual([]); - expectNoVictimProjectIds(rows); - }); - - test("CTEs with victim predicates cannot resurrect victim rows", () => { - const { rows } = executeScoped( - `WITH victim_events AS ( - SELECT project_id, metric - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' - LIMIT 10 - ) - SELECT project_id, metric - FROM victim_events - LIMIT 10`, - ); - - expect(rows).toEqual([]); - expectNoVictimProjectIds(rows); - }); - - test("scalar subqueries cannot leak a victim project id", () => { - const { rows } = executeScoped( - `SELECT ( - SELECT project_id - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' - LIMIT 1 - ) AS leaked_project_id - FROM events - ORDER BY id - LIMIT 1`, - ); - - expect(rows).toEqual([{ leaked_project_id: null }]); - expectNoVictimProjectIds(rows); - }); - - test("default limits are injected into NOT EXISTS subqueries", () => { - const { result, rows } = executeScoped( - `SELECT e.id - FROM events AS e - WHERE NOT EXISTS ( - SELECT 1 - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - AND victim.metric = e.metric - ) - ORDER BY e.id`, - ); - - expect(result.emitted?.sql).toContain("NOT EXISTS (SELECT 1 FROM `events` AS `victim`"); - expect(result.emitted?.sql).toContain("`victim`.`project_id` = 'project-alpha'"); - expect(result.emitted?.sql).toContain("LIMIT 25"); - expect(rows).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); - expectNoVictimProjectIds(rows); - }); - - test("CASE scalar subqueries get tenant guards and default limits", () => { - const { result, rows } = executeScoped( - `SELECT CASE - WHEN e.id > 0 THEN ( - SELECT victim.project_id - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - LIMIT 1 - ) - ELSE NULL - END AS leaked_project_id - FROM events AS e - ORDER BY e.id`, - ); - - expect(result.emitted?.sql).toContain("`victim`.`project_id` = 'project-alpha'"); - expect(result.emitted?.sql).toContain("LIMIT 25"); - expect(rows[0]).toEqual({ leaked_project_id: null }); - expectNoVictimProjectIds(rows); - }); - - test("function-argument scalar subqueries cannot leak victim project ids", () => { - const { rows } = executeScoped( - `SELECT COALESCE(( - SELECT victim.project_id - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - LIMIT 1 - ), 'none') AS leaked_project_id - FROM events AS e - ORDER BY e.id - LIMIT 1`, - ); - - expect(rows).toEqual([{ leaked_project_id: "none" }]); - expectNoVictimProjectIds(rows); - }); - - test("EXISTS projection subqueries cannot reveal victim project existence", () => { - const { rows } = executeScoped( - `SELECT EXISTS ( - SELECT 1 - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - ) AS has_victim_project - FROM events AS e - ORDER BY e.id - LIMIT 1`, - ); - - expect(rows).toEqual([{ has_victim_project: 0 }]); - expectNoVictimProjectIds(rows); - }); - - test("correlated aggregate subqueries cannot reveal victim metric distribution", () => { - const { rows } = executeScoped( - `SELECT e.metric, - ( - SELECT COUNT(victim.id) - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - AND victim.metric = e.metric - ) AS victim_metric_count - FROM events AS e - ORDER BY e.id - LIMIT 3`, - ); - - expect(rows).toEqual([ - { metric: "login", victim_metric_count: 0 }, - { metric: "export", victim_metric_count: 0 }, - { metric: "login", victim_metric_count: 0 }, - ]); - expectNoVictimProjectIds(rows); - }); - - test("HAVING subqueries cannot reveal victim project existence", () => { - const { rows } = executeScoped( - `SELECT metric, COUNT(id) AS event_count - FROM events - GROUP BY metric - HAVING ( - SELECT COUNT(victim.id) - FROM events AS victim - WHERE victim.project_id = '${VICTIM_PROJECT_ID}' - ) > 0 - ORDER BY metric`, - ); - - expect(rows).toEqual([]); - expectNoVictimProjectIds(rows); - }); - - test("self joins scope every table touch independently", () => { - const { result, rows } = executeScoped( - `SELECT e.id, other.id AS other_id - FROM events AS e - INNER JOIN events AS other - ON other.metric = e.metric - AND other.project_id = '${VICTIM_PROJECT_ID}' - ORDER BY e.id - LIMIT 10`, - ); - - expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); - expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); - expect(rows).toEqual([]); - expectNoVictimProjectIds(rows); - }); - - test("cross joins scope the joined table even without an original ON clause", () => { - const { result, rows } = executeScoped( - `SELECT e.project_id, other.project_id AS other_project_id - FROM events AS e - CROSS JOIN events AS other - WHERE other.project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 - ORDER BY e.id, other.id - LIMIT 10`, - ); - - expect(result.emitted?.sql).toContain("INNER JOIN `events` AS `other` ON TRUE"); - expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); - expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); - expectNoVictimProjectIds(rows); - }); - - test("OR predicates cannot widen past the injected project guard", () => { - const { result, rows } = executeScoped( - `SELECT id, project_id - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 - ORDER BY id - LIMIT 10`, - ); - - expect(result.emitted?.sql).toContain("OR 1 = 1"); - expect(result.emitted?.sql).toContain("AND `events`.`project_id` = 'project-alpha'"); - expect(rows).toEqual([ - { id: 1, project_id: ATTACKER_PROJECT_ID }, - { id: 2, project_id: ATTACKER_PROJECT_ID }, - { id: 3, project_id: ATTACKER_PROJECT_ID }, - ]); - expectNoVictimProjectIds(rows); - }); - - test("left joins with always-true join predicates cannot pull victim rows", () => { - const { result, rows } = executeScoped( - `SELECT e.project_id, other.project_id AS other_project_id - FROM events AS e - LEFT JOIN events AS other - ON other.project_id = '${VICTIM_PROJECT_ID}' OR 1 = 1 - ORDER BY e.id, other.id - LIMIT 10`, - ); - - expect(result.emitted?.sql).toContain("`other`.`project_id` = 'project-alpha'"); - expect(result.emitted?.sql).toContain("`e`.`project_id` = 'project-alpha'"); - expectNoVictimProjectIds(rows); - }); - - test("IN-list scalar subqueries cannot smuggle victim ids", () => { - const { rows } = executeScoped( - `SELECT id, project_id - FROM events - WHERE id IN ( - (SELECT id FROM events WHERE project_id = '${VICTIM_PROJECT_ID}' LIMIT 1), - 1 - ) - ORDER BY id - LIMIT 10`, - ); - - expect(rows).toEqual([{ id: 1, project_id: ATTACKER_PROJECT_ID }]); - expectNoVictimProjectIds(rows); - }); - - test("window expression subqueries cannot read victim project ids", () => { - const { rows } = executeScoped( - `SELECT project_id, - COUNT(id) OVER ( - PARTITION BY ( - SELECT project_id - FROM events - WHERE project_id = '${VICTIM_PROJECT_ID}' - LIMIT 1 - ) - ) AS grouped_count - FROM events - ORDER BY id - LIMIT 10`, - ); - - expect(rows).toEqual([ - { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, - { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, - { project_id: ATTACKER_PROJECT_ID, grouped_count: 3 }, - ]); - expectNoVictimProjectIds(rows); - }); - - test("quoted aliases containing SQL syntax do not break out of project guards", () => { - const { result, rows } = executeScoped( - "SELECT `e --`.`project_id` FROM events AS `e --` WHERE `e --`.`project_id` = 'project-bravo' OR 1 = 1 ORDER BY `e --`.`id` LIMIT 10", - ); - - expect(result.emitted?.sql).toContain("`e --`.`project_id` = 'project-alpha'"); - expect(projectIds(rows)).toEqual([ - ATTACKER_PROJECT_ID, - ATTACKER_PROJECT_ID, - ATTACKER_PROJECT_ID, - ]); - expectNoVictimProjectIds(rows); - }); -}); diff --git a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts index c3ee346..6572e7a 100644 --- a/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts +++ b/packages/voight/tests/integration/security/schema-qualified-catalogs.test.ts @@ -46,6 +46,7 @@ describe("schema-qualified catalogs", () => { tables: ["tracking.time_series_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -70,6 +71,7 @@ describe("schema-qualified catalogs", () => { tables: ["time_series_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -104,6 +106,7 @@ describe("schema-qualified catalogs", () => { tables: ["time_series_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -138,6 +141,7 @@ describe("schema-qualified catalogs", () => { tables: ["public_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { @@ -171,6 +175,7 @@ describe("schema-qualified catalogs", () => { tables: ["public_stats"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { diff --git a/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts b/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts index cee4be2..19c4ad1 100644 --- a/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts +++ b/packages/voight/tests/integration/security/schema-qualified-tenant-scoping-probes.test.ts @@ -37,6 +37,7 @@ const policies = [ tables: ["analytics.event_rollups", "iam.api_clients"], scopeColumn: "workspace_id", contextKey: "workspaceId", + scopeValueType: "string", }), ]; diff --git a/packages/voight/tests/integration/security/tenant-scoping-regression.test.ts b/packages/voight/tests/integration/security/tenant-scoping-regression.test.ts index 65b3688..682fc5e 100644 --- a/packages/voight/tests/integration/security/tenant-scoping-regression.test.ts +++ b/packages/voight/tests/integration/security/tenant-scoping-regression.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "vitest"; import { compile } from "../../../src/compiler"; -import { tenantScopingPolicy } from "../../../src/policies"; +import { allowedFunctionsPolicy, tenantScopingPolicy } from "../../../src/policies"; import { createTestCatalog } from "../../../src/testing"; /** @@ -17,12 +17,16 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", +}); +const allowedFunctionPolicy = allowedFunctionsPolicy({ + allowedFunctions: new Set(["count", "min", "max"]), }); function compileTenantScoped(sql: string) { return compile(sql, { catalog, - policies: [tenantPolicy], + policies: [tenantPolicy, allowedFunctionPolicy], policyContext: { tenantId: "tenant-123" }, debug: true, }); diff --git a/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts b/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts index 2f55d90..b9a26e1 100644 --- a/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts +++ b/packages/voight/tests/integration/security/tenant-scoping-shadowing.test.ts @@ -22,6 +22,7 @@ describe("tenant scoping security probes", () => { tables: ["projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -49,6 +50,7 @@ describe("tenant scoping security probes", () => { tables: ["internal_projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -78,6 +80,7 @@ describe("tenant scoping security probes", () => { tables: ["projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -101,6 +104,7 @@ describe("tenant scoping security probes", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -119,6 +123,7 @@ describe("tenant scoping security probes", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -143,6 +148,7 @@ describe("tenant scoping vulnerability probes", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-A" }, @@ -164,6 +170,7 @@ describe("tenant scoping vulnerability probes", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-A" }, diff --git a/packages/voight/tests/integration/security/wildcard-projection-regression.test.ts b/packages/voight/tests/integration/security/wildcard-projection-regression.test.ts index 6e77a68..efee1a0 100644 --- a/packages/voight/tests/integration/security/wildcard-projection-regression.test.ts +++ b/packages/voight/tests/integration/security/wildcard-projection-regression.test.ts @@ -3,7 +3,7 @@ import { describe, expect, test } from "vitest"; import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; import { compile } from "../../../src/compiler"; import { DiagnosticCode } from "../../../src/core/diagnostics"; -import { tenantScopingPolicy } from "../../../src/policies"; +import { allowedFunctionsPolicy, tenantScopingPolicy } from "../../../src/policies"; const catalog = new InMemoryCatalog([ createTableSchema({ @@ -24,6 +24,7 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["users", "orders"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); function compileScoped(sql: string) { @@ -139,6 +140,7 @@ describe("FIXED: wildcard projection respects catalog selectability", () => { test("hidden-only tables still permit COUNT(*) because no hidden columns are projected", () => { const result = compile("SELECT COUNT(*) AS row_count FROM audit_log", { catalog, + policies: [allowedFunctionsPolicy({ allowedFunctions: new Set(["count"]) })], debug: true, }); diff --git a/packages/voight/tests/unit/emitter/escaping.test.ts b/packages/voight/tests/unit/emitter/escaping.test.ts index e7e8a16..b264a2d 100644 --- a/packages/voight/tests/unit/emitter/escaping.test.ts +++ b/packages/voight/tests/unit/emitter/escaping.test.ts @@ -7,6 +7,7 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); function compileTenantScoped(sql: string, tenantId: unknown = "tenant-123") { @@ -70,19 +71,6 @@ describe("emitter literal escaping", () => { expect(escaped.emitted?.sql).toContain("'tenant\\\\'"); } }); - - test("contains policy-injected SQL payloads inside one MySQL string literal", () => { - const result = compileTenantScoped( - "SELECT metric FROM timeseries", - "tenant-123' OR 1=1 /*", - ); - expect(result.ok).toBe(true); - if (!result.ok) { - return; - } - - expect(result.emitted?.sql).toContain("'tenant-123'' OR 1=1 /*'"); - }); }); describe("emitter identifier escaping", () => { diff --git a/packages/voight/tests/unit/emitter/parameters.test.ts b/packages/voight/tests/unit/emitter/parameters.test.ts index 248e74a..7fd99d3 100644 --- a/packages/voight/tests/unit/emitter/parameters.test.ts +++ b/packages/voight/tests/unit/emitter/parameters.test.ts @@ -7,6 +7,7 @@ const tenantPolicy = tenantScopingPolicy({ tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); function compileTenantScoped(sql: string, tenantId: unknown = "tenant-123") { diff --git a/packages/voight/tests/unit/emitter/tenant-scoping.test.ts b/packages/voight/tests/unit/emitter/tenant-scoping.test.ts index 4ce1095..a4c1e6e 100644 --- a/packages/voight/tests/unit/emitter/tenant-scoping.test.ts +++ b/packages/voight/tests/unit/emitter/tenant-scoping.test.ts @@ -1,12 +1,13 @@ import { describe, expect, test } from "vitest"; -import { tenantScopingPolicy } from "../../../src/policies"; +import { allowedFunctionsPolicy, tenantScopingPolicy } from "../../../src/policies"; import { compileStrict } from "../../_support/compile"; const tenantPolicy = tenantScopingPolicy({ tables: ["timeseries", "orders"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const bigintTenantPolicy = tenantScopingPolicy({ @@ -32,7 +33,7 @@ const booleanTenantPolicy = tenantScopingPolicy({ function compileTenantScoped(sql: string, tenantId: unknown = "tenant-123") { return compileStrict(sql, { - policies: [tenantPolicy], + policies: [tenantPolicy, allowedFunctionsPolicy({ allowedFunctions: new Set(["count"]) })], policyContext: { tenantId }, }); } @@ -110,7 +111,9 @@ describe("emitter tenant scoping output", () => { const cases = [ { policy: tenantPolicy, tenantId: "my-tenant", fragment: "'my-tenant'" }, { policy: numberTenantPolicy, tenantId: 42, fragment: "= 42" }, + { policy: numberTenantPolicy, tenantId: 42.5, fragment: "= 42.5" }, { policy: booleanTenantPolicy, tenantId: true, fragment: "= TRUE" }, + { policy: booleanTenantPolicy, tenantId: false, fragment: "= FALSE" }, { policy: tenantPolicy, tenantId: null, fragment: "IS NULL" }, ] as const; diff --git a/packages/voight/tests/unit/enforcer/core.test.ts b/packages/voight/tests/unit/enforcer/core.test.ts index 360a745..2e41dd4 100644 --- a/packages/voight/tests/unit/enforcer/core.test.ts +++ b/packages/voight/tests/unit/enforcer/core.test.ts @@ -6,15 +6,18 @@ import { allowedFunctionsPolicy, maxLimitPolicy } from "../../../src/policies"; import { bindStatement } from "../../_support/bind"; describe("enforce", () => { - test("does not enforce any function policy unless one is configured", () => { + test("denies functions by default when no function policy is configured", () => { const bound = bindStatement("SELECT SLEEP(10) FROM users"); const result = enforce(bound); - expect(result.ok).toBe(true); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.DisallowedFunction); + } }); - test("allows queries with no configured policies", () => { - const bound = bindStatement("SELECT SLEEP(10) FROM users"); + test("allows function-free queries with no configured policies", () => { + const bound = bindStatement("SELECT id FROM users"); const result = enforce(bound); expect(result.ok).toBe(true); @@ -140,7 +143,7 @@ describe("enforce", () => { expect(excessiveCount.ok).toBe(false); }); - test("requires nested subqueries to carry their own limit", () => { + test("does not require nested subqueries to carry their own limit by default", () => { const bound = bindStatement( "SELECT users.id FROM users WHERE users.id IN (SELECT orders.id FROM orders) LIMIT 10", ); @@ -148,13 +151,10 @@ describe("enforce", () => { policies: [maxLimitPolicy({ maxLimit: 100 })], }); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); - } + expect(result.ok).toBe(true); }); - test("requires limits across scalar, exists, not exists, and in subqueries", () => { + test("requires limits across scalar, exists, not exists, and in subqueries when recursive", () => { for (const sql of [ "SELECT id FROM users WHERE id = (SELECT user_id FROM orders) LIMIT 10", "SELECT id FROM users WHERE EXISTS (SELECT user_id FROM orders) LIMIT 10", @@ -162,14 +162,14 @@ describe("enforce", () => { "SELECT id FROM users WHERE id IN (SELECT user_id FROM orders) LIMIT 10", ]) { const result = enforce(bindStatement(sql), { - policies: [maxLimitPolicy({ maxLimit: 100 })], + policies: [maxLimitPolicy({ maxLimit: 100, recursive: true })], }); expect(result.ok, `Expected nested limit rejection for ${sql}`).toBe(false); } }); - test("rejects nested subquery limit sizes above the configured maximum", () => { + test("ignores nested subquery limit sizes by default", () => { const bound = bindStatement( "SELECT users.id FROM users WHERE users.id IN (SELECT orders.id FROM orders LIMIT 999999) LIMIT 10", ); @@ -177,6 +177,17 @@ describe("enforce", () => { policies: [maxLimitPolicy({ maxLimit: 100 })], }); + expect(result.ok).toBe(true); + }); + + test("rejects nested subquery limit sizes above the configured maximum when recursive", () => { + const bound = bindStatement( + "SELECT users.id FROM users WHERE users.id IN (SELECT orders.id FROM orders LIMIT 999999) LIMIT 10", + ); + const result = enforce(bound, { + policies: [maxLimitPolicy({ maxLimit: 100, recursive: true })], + }); + expect(result.ok).toBe(false); if (!result.ok) { expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); diff --git a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts index 3937fb6..b355660 100644 --- a/packages/voight/tests/unit/enforcer/policy-hardening.test.ts +++ b/packages/voight/tests/unit/enforcer/policy-hardening.test.ts @@ -14,6 +14,7 @@ import { PolicyConfigurationError, supportedOperatorsPolicy, tenantScopingPolicy, + type TenantScopingPolicyOptions, } from "../../../src/policies"; import { createTestCatalog } from "../../../src/testing"; @@ -43,6 +44,20 @@ function injectUnsupportedOperator(query: T): T { } describe("allowedFunctionsPolicy hardening", () => { + test("blocks functions by default when no allowlist is configured", () => { + for (const sql of ["SELECT SLEEP(1) FROM users", "SELECT COUNT(id) FROM users"]) { + const result = compile(sql, { + catalog: createTestCatalog(), + debug: true, + }); + + expect(result.ok, `Unexpected success for ${sql}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.DisallowedFunction); + } + } + }); + test("blocks disallowed functions inside nested subqueries", () => { // The allowlist must recurse into inner queries, otherwise attackers can hide // side-effecting or expensive calls behind a subquery boundary. @@ -96,7 +111,7 @@ describe("maxLimitPolicy abuse probes", () => { ); }); - test("rejects nested OFFSET values above the configured maximum", () => { + test("ignores nested OFFSET values because only the outer result size is constrained", () => { const result = compile( "SELECT id FROM users WHERE id IN (SELECT user_id FROM orders LIMIT 1 OFFSET 999999999) LIMIT 1", { @@ -106,6 +121,20 @@ describe("maxLimitPolicy abuse probes", () => { }, ); + expect(result.ok).toBe(true); + expect(result.emitted?.sql).toContain("OFFSET 999999999"); + }); + + test("rejects nested OFFSET values above the configured maximum when recursive", () => { + const result = compile( + "SELECT id FROM users WHERE id IN (SELECT user_id FROM orders LIMIT 1 OFFSET 999999999) LIMIT 1", + { + catalog: createTestCatalog(), + policies: [maxLimitPolicy({ maxLimit: 100, maxOffset: 1000, recursive: true })], + debug: true, + }, + ); + expect(result.ok).toBe(false); if (!result.ok) { expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.LimitExceeded); @@ -177,6 +206,7 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const bigintPolicy = tenantScopingPolicy({ tables: ["timeseries"], @@ -190,6 +220,12 @@ describe("tenantScopingPolicy boundaries", () => { contextKey: "tenantId", scopeValueType: "number", }); + const booleanPolicy = tenantScopingPolicy({ + tables: ["timeseries"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "boolean", + }); test("enforcement rejects semantically equivalent but non-canonical tenant predicates", () => { // Enforcement is intentionally syntactic. This query is logically scoped, but @@ -231,6 +267,7 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }).enforce?.(result.bound, { context: { tenantId: "tenant-123" }, }); @@ -262,11 +299,13 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }, { tables: ["users"], scopeColumn: "workspace_id", contextKey: "workspaceId", + scopeValueType: "string", }, ], }), @@ -279,6 +318,7 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["`users`"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ).toThrow(PolicyConfigurationError); }); @@ -289,10 +329,32 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["users"], scopeColumn: "tenant-id", contextKey: "tenantId", + scopeValueType: "string", }), ).toThrow(PolicyConfigurationError); }); + test("requires tenant scope value types to be configured explicitly", () => { + expect(() => + tenantScopingPolicy({ + tables: ["users"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + } as unknown as TenantScopingPolicyOptions), + ).toThrow(PolicyConfigurationError); + }); + + test("rejects invalid tenant scope value type configuration", () => { + expect(() => + tenantScopingPolicy({ + tables: ["users"], + scopeColumn: "tenant_id", + contextKey: "tenantId", + scopeValueType: "uuid", + } as unknown as TenantScopingPolicyOptions), + ).toThrow(PolicyConfigurationError); + }); + test("fails closed when one table matches more than one scope rule via schema aliases", () => { const catalog = new InMemoryCatalog([ createTableSchema({ @@ -310,11 +372,13 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }, { tables: ["analytics.users"], scopeColumn: "workspace_id", contextKey: "workspaceId", + scopeValueType: "string", }, ], }), @@ -357,11 +421,13 @@ describe("tenantScopingPolicy boundaries", () => { tables: ["projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }, { tables: ["internal_projects"], scopeColumn: "workspace_id", contextKey: "workspaceId", + scopeValueType: "string", }, ], }), @@ -424,6 +490,23 @@ describe("tenantScopingPolicy boundaries", () => { } }); + test("rejects non-bigint values for bigint tenant scopes", () => { + for (const tenantId of ["42", 42, true]) { + const result = compile("SELECT metric FROM timeseries", { + catalog: createTestCatalog(), + policies: [bigintPolicy], + policyContext: { tenantId }, + debug: true, + }); + + expect(result.ok, `Unexpected success for ${String(tenantId)}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.PolicyExecutionError); + expect(result.diagnostics[0]?.message).toContain("requires bigint tenant values"); + } + } + }); + test("rejects unsafe integer number tenant values that should be bigint", () => { const result = compile("SELECT metric FROM timeseries", { catalog: createTestCatalog(), @@ -439,6 +522,23 @@ describe("tenantScopingPolicy boundaries", () => { } }); + test("rejects non-number values for number tenant scopes", () => { + for (const tenantId of ["42", 42n, false]) { + const result = compile("SELECT metric FROM timeseries", { + catalog: createTestCatalog(), + policies: [numberPolicy], + policyContext: { tenantId }, + debug: true, + }); + + expect(result.ok, `Unexpected success for ${String(tenantId)}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.PolicyExecutionError); + expect(result.diagnostics[0]?.message).toContain("requires number tenant values"); + } + } + }); + test("rejects non-finite numeric tenant values", () => { const cases = [Number.NaN, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY]; @@ -457,4 +557,21 @@ describe("tenantScopingPolicy boundaries", () => { } } }); + + test("rejects non-boolean values for boolean tenant scopes", () => { + for (const tenantId of ["true", 1, 0n]) { + const result = compile("SELECT metric FROM timeseries", { + catalog: createTestCatalog(), + policies: [booleanPolicy], + policyContext: { tenantId }, + debug: true, + }); + + expect(result.ok, `Unexpected success for ${String(tenantId)}`).toBe(false); + if (!result.ok) { + expect(result.diagnostics[0]?.code).toBe(DiagnosticCode.PolicyExecutionError); + expect(result.diagnostics[0]?.message).toContain("requires boolean tenant values"); + } + } + }); }); diff --git a/packages/voight/tests/unit/enforcer/policy-resolution.test.ts b/packages/voight/tests/unit/enforcer/policy-resolution.test.ts index 103799b..a165628 100644 --- a/packages/voight/tests/unit/enforcer/policy-resolution.test.ts +++ b/packages/voight/tests/unit/enforcer/policy-resolution.test.ts @@ -3,8 +3,8 @@ import { describe, expect, test } from "vitest"; import { PolicyConflictError, resolvePolicies, tenantScopingPolicy } from "../../../src/policies"; describe("policy resolution", () => { - test("returns an empty list when no policies are provided", () => { - expect(resolvePolicies()).toEqual([]); + test("adds a default deny-all function policy when no policies are provided", () => { + expect(resolvePolicies().map((policy) => policy.name)).toEqual(["allowed-functions"]); }); test("throws when multiple explicit policies share the same name", () => { @@ -25,11 +25,13 @@ describe("policy resolution", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const ordersPolicy = tenantScopingPolicy({ tables: ["orders"], scopeColumn: "workspace_id", contextKey: "workspaceId", + scopeValueType: "string", }); expect(() => diff --git a/packages/voight/tests/unit/enforcer/tenant-scoping.test.ts b/packages/voight/tests/unit/enforcer/tenant-scoping.test.ts index 3598268..90e050d 100644 --- a/packages/voight/tests/unit/enforcer/tenant-scoping.test.ts +++ b/packages/voight/tests/unit/enforcer/tenant-scoping.test.ts @@ -10,6 +10,7 @@ describe("tenant scoping enforcement", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); test("rejects OR-based bypasses during enforcement", () => { @@ -48,6 +49,7 @@ describe("tenant scoping enforcement", () => { tables: ["users", "orders", "internal_projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const bound = bindStatement( "SELECT u.id, o.total, p.name FROM users AS u INNER JOIN orders AS o ON o.user_id = u.id AND o.tenant_id = 'tenant-123' LEFT JOIN internal_projects AS p ON p.id = o.id AND p.tenant_id = 'tenant-123' WHERE u.tenant_id = 'tenant-123' AND u.age > 18", @@ -65,6 +67,7 @@ describe("tenant scoping enforcement", () => { tables: ["users", "orders", "internal_projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const bound = bindStatement( "SELECT u.id, o.total, p.name FROM users AS u INNER JOIN orders AS o ON o.user_id = u.id AND o.tenant_id = 'tenant-123' LEFT JOIN internal_projects AS p ON p.id = o.id WHERE u.tenant_id = 'tenant-123'", diff --git a/packages/voight/tests/unit/parser/ambiguity.test.ts b/packages/voight/tests/unit/parser/ambiguity.test.ts index 4a76994..c581432 100644 --- a/packages/voight/tests/unit/parser/ambiguity.test.ts +++ b/packages/voight/tests/unit/parser/ambiguity.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { compileStrict } from "../../_support/compile"; +import { compileStrict, compileWithAllowedFunctions } from "../../_support/compile"; describe("parser ambiguity boundaries", () => { test("treats a trailing identifier as an implicit alias", () => { @@ -15,7 +15,9 @@ describe("parser ambiguity boundaries", () => { test("distinguishes bare identifiers from function calls", () => { expect(compileStrict("SELECT count FROM users").ok).toBe(false); - expect(compileStrict("SELECT count() FROM users").ok).toBe(true); + expect( + compileWithAllowedFunctions("SELECT count() FROM users", new Set(["count"])).ok, + ).toBe(true); }); test("keeps qualified references distinct from aliases", () => { diff --git a/packages/voight/tests/unit/parser/structure.test.ts b/packages/voight/tests/unit/parser/structure.test.ts index 2cd510c..1bad7af 100644 --- a/packages/voight/tests/unit/parser/structure.test.ts +++ b/packages/voight/tests/unit/parser/structure.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { compileStrict } from "../../_support/compile"; +import { compileStrict, compileWithAllowedFunctions } from "../../_support/compile"; describe("parser structural boundaries", () => { test("rejects empty clause bodies", () => { @@ -65,9 +65,9 @@ describe("parser structural boundaries", () => { }); test("accepts window functions with OVER, PARTITION BY, and ORDER BY", () => { - expect(compileStrict("SELECT COUNT(*) OVER () FROM users").ok).toBe(true); + expect(compileWithAllowedFunctions("SELECT COUNT(*) OVER () FROM users").ok).toBe(true); expect( - compileStrict( + compileWithAllowedFunctions( "SELECT SUM(id) OVER (PARTITION BY age ORDER BY created_at DESC) FROM users", ).ok, ).toBe(true); diff --git a/packages/voight/tests/unit/rewriter/tenant-scoping.test.ts b/packages/voight/tests/unit/rewriter/tenant-scoping.test.ts index f62a1a4..2df8d3c 100644 --- a/packages/voight/tests/unit/rewriter/tenant-scoping.test.ts +++ b/packages/voight/tests/unit/rewriter/tenant-scoping.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "vitest"; import { compile } from "../../../src/compiler"; import { InMemoryCatalog, createTableSchema } from "../../../src/catalog"; -import { tenantScopingPolicy } from "../../../src/policies"; +import { allowedFunctionsPolicy, tenantScopingPolicy } from "../../../src/policies"; import { createTestCatalog } from "../../../src/testing"; describe("tenant scoping rewrite", () => { @@ -10,6 +10,7 @@ describe("tenant scoping rewrite", () => { tables: ["timeseries"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); test("rewrites aliased table scans", () => { @@ -76,11 +77,15 @@ describe("tenant scoping rewrite", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }); const result = compile("SELECT DATE_ADD(created_at, INTERVAL tenant_id DAY) FROM users", { catalog: createTestCatalog(), - policies: [scopedUsersPolicy], + policies: [ + scopedUsersPolicy, + allowedFunctionsPolicy({ allowedFunctions: new Set(["date_add"]) }), + ], policyContext: { tenantId: "tenant-123" }, debug: true, }); @@ -139,6 +144,7 @@ describe("tenant scoping rewrite", () => { tables: ["users", "orders", "internal_projects"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -172,6 +178,7 @@ describe("tenant scoping rewrite", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }), ], policyContext: { tenantId: "tenant-123" }, @@ -208,11 +215,13 @@ describe("tenant scoping rewrite", () => { tables: ["users"], scopeColumn: "tenant_id", contextKey: "tenantId", + scopeValueType: "string", }, { tables: ["subscriptions"], scopeColumn: "account_id", contextKey: "tenantId", + scopeValueType: "string", }, ], }),