mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-28 06:23:01 +00:00
Takeover of community PR #2387 with the security review's required fixes applied. Original work by @harrisali0101. With GBRAIN_RLS_SCOPE_BINDING=1, source-scoped Postgres read methods wrap their queries in a transaction that binds set_config('app.scopes', $1, true) (federated sourceIds CSV > scalar sourceId > '*') so operator-managed RLS policies can filter rows at the SQL layer — defense-in-depth layer 2 under the mandatory app-layer source filters. Review fixes on top of the original PR: - Flag-off is now a TRUE pass-through: no new per-read transaction wrap (the #1794 PgBouncer pool-exhaustion class). Only the three search methods keep a transaction when off — exactly the sql.begin() + SET LOCAL statement_timeout wrap they already had on master — via the helper's alwaysTransaction option. - Preserved the PR's latent setseed fix: listCorpusSample pins setseed() + SELECT to one connection when seeded (alwaysTransaction gated on opts.seed), so the deterministic path can't split across pooled connections. - Updated the two postgres-engine shape tests to pin the new invariant (search methods route through withScopedReadTransaction with alwaysTransaction; helper owns the sql.begin(); flag-off path is callback(this.sql)). - New behavioral tests (test/postgres-engine-rls-scope.test.ts): flag-off pass-through, flag-off alwaysTransaction, flag-on set_config emission, federated > scalar > '*' precedence, and the CSV as a bound parameter (never interpolated). - Fixed the helper header comment to match the actual branching behavior. - Operator docs in docs/ENGINES.md: env var, policy SQL, the ALTER ROLE ... SET app.scopes='*' default requirement, FORCE ROW LEVEL SECURITY for owner roles, and the honest caveat about unwrapped paths. Co-authored-by: Sinabina <sinabina@Sinabinas-MacBook-Pro-4.local> Co-authored-by: Harris <79081645+harrisali0101@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
166 lines
7.4 KiB
TypeScript
166 lines
7.4 KiB
TypeScript
/**
|
|
* postgres-engine.ts source-level guardrails.
|
|
*
|
|
* Live Postgres coverage for search paths lives in test/e2e/search-quality.test.ts.
|
|
* This file stays fast and DB-free: it inspects the source of
|
|
* src/core/postgres-engine.ts to lock in decisions that protect the
|
|
* shared connection pool from per-request GUC leaks.
|
|
*
|
|
* Regression: R6-F006 / R4-F002.
|
|
* searchKeyword and searchVector used to call bare
|
|
* await sql`SET statement_timeout = '8s'`
|
|
* ...query...
|
|
* finally { await sql`SET statement_timeout = '0'` }
|
|
* against the shared pool. Each tagged template picks an arbitrary
|
|
* connection, so the SET, the query, and the reset could all land on
|
|
* DIFFERENT connections. Worst case: the 8s GUC sticks on some pooled
|
|
* connection and clips the next caller's long-running query; or the
|
|
* reset to 0 lands on a connection that other code expected to be
|
|
* protected. The fix wraps each query in sql.begin() and uses
|
|
* SET LOCAL so the GUC is transaction-scoped and auto-resets on
|
|
* COMMIT/ROLLBACK, regardless of error path.
|
|
*/
|
|
|
|
import { describe, test, expect } from 'bun:test';
|
|
import { readFileSync } from 'fs';
|
|
import { join } from 'path';
|
|
|
|
const SRC = readFileSync(
|
|
join(import.meta.dir, '..', 'src', 'core', 'postgres-engine.ts'),
|
|
'utf-8',
|
|
);
|
|
|
|
describe('postgres-engine / search path timeout isolation', () => {
|
|
test('no bare `SET statement_timeout` statement survives', () => {
|
|
// Strip comments so the commentary mentioning the anti-pattern does
|
|
// not trigger a false positive. Block-comment + line-comment strip.
|
|
const stripped = SRC
|
|
.replace(/\/\*[\s\S]*?\*\//g, '')
|
|
.replace(/(^|\s)\/\/[^\n]*/g, '$1');
|
|
|
|
// Match a tagged-template statement of the form
|
|
// sql`SET statement_timeout = ...`
|
|
// that is NOT preceded by LOCAL. This is the exact shape that bleeds
|
|
// onto pooled connections; SET LOCAL is safe inside a transaction.
|
|
const bare = stripped.match(
|
|
/sql`\s*SET\s+(?!LOCAL\s)statement_timeout\b[^`]*`/gi,
|
|
);
|
|
expect(bare).toBeNull();
|
|
});
|
|
|
|
test('searchKeyword wraps its query in a transaction (via withScopedReadTransaction alwaysTransaction)', () => {
|
|
// Post-RLS-scope-binding invariant: the search methods route through
|
|
// withScopedReadTransaction with alwaysTransaction: true, which
|
|
// guarantees a sql.begin() wrap in BOTH modes — flag off (identical to
|
|
// master's pre-helper wrap) and flag on (scoped transaction with
|
|
// set_config). See the helper tests in
|
|
// test/postgres-engine-rls-scope.test.ts for the behavioral pins.
|
|
const fn = extractMethod(SRC, 'searchKeyword');
|
|
expect(fn).toMatch(/withScopedReadTransaction\s*\(/);
|
|
expect(fn).toMatch(/alwaysTransaction:\s*true/);
|
|
});
|
|
|
|
test('searchVector wraps its query in a transaction (via withScopedReadTransaction alwaysTransaction)', () => {
|
|
const fn = extractMethod(SRC, 'searchVector');
|
|
expect(fn).toMatch(/withScopedReadTransaction\s*\(/);
|
|
expect(fn).toMatch(/alwaysTransaction:\s*true/);
|
|
});
|
|
|
|
test('withScopedReadTransaction owns the sql.begin() wrap (and only opens it when needed)', () => {
|
|
// (extractMethod can't grab this one: `private async ...<T>(`.)
|
|
const stripped = stripComments(SRC);
|
|
// The transaction lives in the helper...
|
|
expect(stripped).toMatch(/this\.sql\.begin\s*\(/);
|
|
// ...and the flag-off / non-alwaysTransaction path is a true
|
|
// pass-through on the shared pool — no per-read transaction hold.
|
|
expect(stripped).toMatch(
|
|
/if\s*\(!this\.rlsScopeBindingEnabled\s*&&\s*!opts\?\.alwaysTransaction\)\s*\{\s*return\s+await\s+callback\(this\.sql\);/,
|
|
);
|
|
});
|
|
|
|
test('both search methods use SET LOCAL for the timeout', () => {
|
|
const keyword = extractMethod(SRC, 'searchKeyword');
|
|
const vector = extractMethod(SRC, 'searchVector');
|
|
expect(keyword).toMatch(/SET\s+LOCAL\s+statement_timeout/);
|
|
expect(vector).toMatch(/SET\s+LOCAL\s+statement_timeout/);
|
|
});
|
|
|
|
test('connect() with poolSize honors resolvePrepare (PgBouncer regression guard)', () => {
|
|
// Regression: worker-instance pools were NOT honoring the prepare decision
|
|
// before v0.15.4. Module singleton connect() in db.ts was fixed by #284 but
|
|
// PostgresEngine.connect({poolSize}) (the branch used by `gbrain jobs work`)
|
|
// silently ignored it — agents running background work against Supabase
|
|
// pooler URLs still hit `prepared statement "..." does not exist` under
|
|
// load. Source-level grep is enough: runtime mocking of postgres.js's
|
|
// tagged-template interface is painful under bun ESM and the wiring is
|
|
// simple enough that if `resolvePrepare` name appears and a conditional
|
|
// `prepare` key appears in the options literal, the wire-up is live.
|
|
const stripped = stripComments(SRC);
|
|
expect(stripped).toMatch(/db\.resolvePrepare\s*\(\s*url\s*\)/);
|
|
expect(stripped).toMatch(/typeof\s+prepare\s*===\s*['"]boolean['"]/);
|
|
});
|
|
|
|
test('neither search method clears the timeout with `SET statement_timeout = 0`', () => {
|
|
// The reset-to-zero pattern was the other half of the leak: if SET
|
|
// LOCAL is in play, COMMIT handles the reset and an explicit
|
|
// `SET statement_timeout = '0'` would itself leak the GUC change
|
|
// onto the returned connection. Strip comments first so the
|
|
// commentary in the method itself (which quotes the anti-pattern
|
|
// to explain it) does not trigger a false positive.
|
|
const keyword = stripComments(extractMethod(SRC, 'searchKeyword'));
|
|
const vector = stripComments(extractMethod(SRC, 'searchVector'));
|
|
expect(keyword).not.toMatch(/SET\s+statement_timeout\s*=\s*['"]?0/);
|
|
expect(vector).not.toMatch(/SET\s+statement_timeout\s*=\s*['"]?0/);
|
|
});
|
|
});
|
|
|
|
describe('postgres-engine / search date filtering', () => {
|
|
test('search paths filter since/until on effective_date before import-time fallback', () => {
|
|
const expectedDateExpr = 'COALESCE(p.effective_date, p.updated_at, p.created_at)';
|
|
const staleDatePredicate = /COALESCE\(p\.updated_at,\s*p\.created_at\)\s*[<>]\s*\$/;
|
|
|
|
for (const methodName of ['searchKeyword', 'searchKeywordChunks', 'searchVector']) {
|
|
const fn = stripComments(extractMethod(SRC, methodName));
|
|
|
|
expect(countOccurrences(fn, expectedDateExpr)).toBe(2);
|
|
expect(fn).not.toMatch(staleDatePredicate);
|
|
}
|
|
});
|
|
});
|
|
|
|
function stripComments(s: string): string {
|
|
return s
|
|
.replace(/\/\*[\s\S]*?\*\//g, '')
|
|
.replace(/(^|\s)\/\/[^\n]*/g, '$1');
|
|
}
|
|
|
|
function countOccurrences(s: string, needle: string): number {
|
|
return s.split(needle).length - 1;
|
|
}
|
|
|
|
// extractMethod grabs the body of a class method by brace-matching from
|
|
// its opening line. Returns the method body up to the matching closing
|
|
// brace. Good enough for the small number of methods in this file.
|
|
function extractMethod(source: string, name: string): string {
|
|
// Find "async <name>(" at method-definition indentation (2 spaces).
|
|
const openRe = new RegExp(`^\\s+async\\s+${name}\\s*\\(`, 'm');
|
|
const match = openRe.exec(source);
|
|
if (!match) {
|
|
throw new Error(`method ${name} not found in postgres-engine.ts`);
|
|
}
|
|
// Scan forward balancing braces.
|
|
let i = source.indexOf('{', match.index);
|
|
if (i < 0) throw new Error(`no opening brace for ${name}`);
|
|
const start = i;
|
|
let depth = 0;
|
|
for (; i < source.length; i++) {
|
|
const c = source[i];
|
|
if (c === '{') depth++;
|
|
else if (c === '}') {
|
|
depth--;
|
|
if (depth === 0) return source.slice(start, i + 1);
|
|
}
|
|
}
|
|
throw new Error(`unbalanced braces in ${name}`);
|
|
}
|