From 8772930aff9e6898eab15f19a48d6090f0d12403 Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Sun, 2 Aug 2026 18:35:27 +0000 Subject: [PATCH] feat(safegres): source-level convention linter (C1-C4) in the constructive preset --- packages/safegres/README.md | 40 +++- .../safegres/__tests__/lint-audit.test.ts | 139 +++++++++++++ packages/safegres/__tests__/lint.test.ts | 184 ++++++++++++++++++ packages/safegres/__tests__/presets.test.ts | 5 + packages/safegres/src/commands/audit.ts | 79 +++++++- packages/safegres/src/config/presets.ts | 16 +- packages/safegres/src/lint/engine.ts | 60 ++++++ packages/safegres/src/lint/index.ts | 26 +++ packages/safegres/src/lint/parse-unit.ts | 183 +++++++++++++++++ packages/safegres/src/lint/rules/index.ts | 18 ++ .../safegres/src/lint/rules/no-dynamic-sql.ts | 29 +++ .../src/lint/rules/no-set-search-path.ts | 67 +++++++ .../src/lint/rules/no-variable-conflict.ts | 38 ++++ .../src/lint/rules/require-qualified-refs.ts | 67 +++++++ packages/safegres/src/lint/suppressions.ts | 162 +++++++++++++++ packages/safegres/src/lint/types.ts | 94 +++++++++ packages/safegres/src/lint/util.ts | 7 + packages/safegres/src/rules/registry.ts | 36 +++- packages/safegres/src/types.ts | 2 +- 19 files changed, 1242 insertions(+), 10 deletions(-) create mode 100644 packages/safegres/__tests__/lint-audit.test.ts create mode 100644 packages/safegres/__tests__/lint.test.ts create mode 100644 packages/safegres/src/lint/engine.ts create mode 100644 packages/safegres/src/lint/index.ts create mode 100644 packages/safegres/src/lint/parse-unit.ts create mode 100644 packages/safegres/src/lint/rules/index.ts create mode 100644 packages/safegres/src/lint/rules/no-dynamic-sql.ts create mode 100644 packages/safegres/src/lint/rules/no-set-search-path.ts create mode 100644 packages/safegres/src/lint/rules/no-variable-conflict.ts create mode 100644 packages/safegres/src/lint/rules/require-qualified-refs.ts create mode 100644 packages/safegres/src/lint/suppressions.ts create mode 100644 packages/safegres/src/lint/types.ts create mode 100644 packages/safegres/src/lint/util.ts diff --git a/packages/safegres/README.md b/packages/safegres/README.md index 7dcb3d52d..8bd535882 100644 --- a/packages/safegres/README.md +++ b/packages/safegres/README.md @@ -104,8 +104,8 @@ trust boundaries on the way. ## What it checks -30 rules across two dimensions. The prefix letter is a family, **not** the dimension: `P1`/`P1b` -are performance, `P5` is security. +34 rules across two dimensions, plus a source-level convention linter. The prefix letter is a +family, **not** the dimension: `P1`/`P1b` are performance, `P5` is security. ### Security (19 rules) @@ -144,6 +144,42 @@ reaches more than intended. `fail-closed` findings are *denied by Postgres at ru availability and hygiene concern, not a leak. They contribute **zero** to the score by default (`scoring.failClosedWeight`). safegres does not cry wolf about a grant the database already refuses. +### Convention (source-level lint, 4 rules — `safegres:constructive`) + +House-style rules that read function **definitions** (`pg_get_functiondef`), not the catalog. They +are pure `source → findings` — no `pg` dependency in the lint module — and are **off outside the +`safegres:constructive` preset**, which enables them. A function on a non-exposed schema costs +nothing, like every other rule. + +| Code | Severity | Direction | Check | +| --- | --- | --- | --- | +| C1 | high | fail-open | Function **sets `search_path`** (house rule: never set it — fully-qualify instead) | +| C2 | medium | neutral | Function uses a **`#variable_conflict`** directive | +| C3 | low | neutral | Function has an **unqualified relation reference** (relies on `search_path`) | +| C4 | high | fail-open | Function uses **dynamic SQL** (`EXECUTE` / `EXECUTE … USING` / `FOR … IN EXECUTE`) | + +C3 ships at `low` (adoption severity — ratchet to error once the tree is clean). C4 cannot be +statically proven read-only, so every dynamic-SQL site is flagged and must be **waived inline with a +categorized reason** (`lookup-only`, `codegen`); a reasonless waiver does not suppress it. + +**Inline suppressions** (ESLint/Prettier style), written as SQL comments inside the body: + +```sql +-- safegres-disable-next-line no-dynamic-sql -- lookup-only: building an IN-list of integers +EXECUTE format('SELECT ... WHERE id = ANY(%L)', ids); + +EXECUTE 'REFRESH MATERIALIZED VIEW app.mv'; -- safegres-disable-line no-dynamic-sql -- codegen: fixed DDL + +-- safegres-disable no-dynamic-sql -- lookup-only: this whole block probes the catalog +... +-- safegres-enable no-dynamic-sql + +-- safegres-disable-file no-set-search-path -- vendored extension shim +``` + +A directive with no rule id applies to every convention rule. Waived findings are **not dropped** — +they surface as `acknowledged` (accepted-risk) findings carrying their reason, off the score. + ### Performance (11 rules, `--perf`) | Code | Severity | Check | diff --git a/packages/safegres/__tests__/lint-audit.test.ts b/packages/safegres/__tests__/lint-audit.test.ts new file mode 100644 index 000000000..16011faa0 --- /dev/null +++ b/packages/safegres/__tests__/lint-audit.test.ts @@ -0,0 +1,139 @@ +import { getConnections, PgTestClient } from 'pgsql-test'; + +import { audit } from '../src/commands/audit'; +import { constructive, recommended } from '../src/config/presets'; +import type { Finding } from '../src/types'; + +jest.setTimeout(120000); + +let pg: PgTestClient; +let teardown: () => Promise; + +beforeAll(async () => { + ({ pg, teardown } = await getConnections()); + await pg.any('CREATE SCHEMA fx_lint'); + await pg.any('CREATE TABLE fx_lint.widgets (id int primary key)'); + + // C1: pins search_path — a house-rule violation. Body kept trivial so only + // C1 fires on it. + await pg.any(` + CREATE FUNCTION fx_lint.pinned() RETURNS int + LANGUAGE sql + SET search_path = public + AS $$ SELECT 1 $$; + `); + + // C3: an unqualified relation reference (relies on search_path). + await pg.any(` + CREATE FUNCTION fx_lint.unqualified() RETURNS bigint + LANGUAGE plpgsql + AS $$ + DECLARE n bigint; + BEGIN + SELECT count(*) INTO n FROM widgets; + RETURN n; + END; + $$; + `); + + // C4 active: dynamic SQL with no waiver. + await pg.any(` + CREATE FUNCTION fx_lint.dyn_unwaived() RETURNS void + LANGUAGE plpgsql + AS $$ + BEGIN + EXECUTE 'SELECT 1'; + END; + $$; + `); + + // C4 waived: dynamic SQL with a reasoned inline waiver — preserved as an + // acknowledged (accepted-risk) finding rather than dropped. + await pg.any(` + CREATE FUNCTION fx_lint.dyn_waived() RETURNS void + LANGUAGE plpgsql + AS $$ + BEGIN + -- safegres-disable-next-line no-dynamic-sql -- lookup-only: static probe + EXECUTE 'SELECT 1'; + END; + $$; + `); + + // Clean: fully-qualified, no search_path, no dynamic SQL. + await pg.any(` + CREATE FUNCTION fx_lint.clean() RETURNS bigint + LANGUAGE plpgsql + AS $$ + DECLARE n bigint; + BEGIN + SELECT count(*) INTO n FROM fx_lint.widgets; + RETURN n; + END; + $$; + `); +}); + +afterAll(async () => { + if (teardown) await teardown(); +}); + +function forFn(findings: Finding[], code: string, fn: string): Finding | undefined { + return findings.find( + (f) => f.code === code && (f.context as { function?: string }).function?.startsWith(`fx_lint.${fn}(`) + ); +} + +async function lintAudit(): Promise { + const report = await audit(pg.client as never, { + schemas: ['fx_lint'], + config: { rules: { C1: 'high', C2: 'medium', C3: 'low', C4: 'high' } } + }); + return report.findings.filter((f) => f.schema === 'fx_lint'); +} + +describe('audit: convention linter wiring (C*)', () => { + it('flags a function that sets search_path (C1)', async () => { + const c1 = forFn(await lintAudit(), 'C1', 'pinned'); + expect(c1).toBeDefined(); + expect(c1?.category).toBe('convention'); + expect(c1?.severity).toBe('high'); + expect(c1?.acknowledged).toBeFalsy(); + }); + + it('flags an unqualified relation reference (C3)', async () => { + const c3 = forFn(await lintAudit(), 'C3', 'unqualified'); + expect(c3).toBeDefined(); + expect(c3?.severity).toBe('low'); + }); + + it('flags dynamic SQL as active when it is not waived (C4)', async () => { + const c4 = forFn(await lintAudit(), 'C4', 'dyn_unwaived'); + expect(c4).toBeDefined(); + expect(c4?.acknowledged).toBeFalsy(); + }); + + it('keeps a reasoned dynamic-SQL waiver as an acknowledged finding (C4)', async () => { + const c4 = forFn(await lintAudit(), 'C4', 'dyn_waived'); + expect(c4).toBeDefined(); + expect(c4?.acknowledged).toBe(true); + const ctx = c4?.context as { suppressed?: boolean; reason?: string }; + expect(ctx.suppressed).toBe(true); + expect(ctx.reason).toContain('lookup-only'); + }); + + it('does not flag a clean, fully-qualified function', async () => { + const findings = await lintAudit(); + const onClean = findings.filter( + (f) => (f.context as { function?: string }).function?.startsWith('fx_lint.clean(') + ); + expect(onClean).toEqual([]); + }); + + it('runs the linter under the constructive preset but not under recommended', () => { + // recommended carries `C*` off; constructive turns them on. + expect(recommended.rules!['C*']).toBe('off'); + expect(constructive.rules!.C1).toBe('high'); + expect(constructive.rules!.C4).toBe('high'); + }); +}); diff --git a/packages/safegres/__tests__/lint.test.ts b/packages/safegres/__tests__/lint.test.ts new file mode 100644 index 000000000..0fce929be --- /dev/null +++ b/packages/safegres/__tests__/lint.test.ts @@ -0,0 +1,184 @@ +import { LINT_RULES,lintDefinition } from '../src/lint'; + +describe('lint: rule registry', () => { + it('exposes the four convention rules with stable codes', () => { + expect(LINT_RULES.map((r) => r.code).sort()).toEqual(['C1', 'C2', 'C3', 'C4']); + expect(LINT_RULES.map((r) => r.id).sort()).toEqual([ + 'no-dynamic-sql', + 'no-set-search-path', + 'no-variable-conflict', + 'require-qualified-refs' + ]); + }); + + it('only no-dynamic-sql requires a suppression reason', () => { + const reasonRequired = LINT_RULES.filter((r) => r.reasonRequired).map((r) => r.id); + expect(reasonRequired).toEqual(['no-dynamic-sql']); + }); +}); + +describe('lint: no-set-search-path (C1)', () => { + it('flags a SET search_path clause', async () => { + const def = `CREATE FUNCTION app.f() RETURNS void LANGUAGE plpgsql +SET search_path = pg_catalog, public +AS $$ +BEGIN + PERFORM app.g(); +END; +$$;`; + const { problems } = await lintDefinition(def, 'plpgsql', 'app.f()'); + const c1 = problems.filter((p) => p.ruleId === 'no-set-search-path'); + expect(c1).toHaveLength(1); + expect(c1[0].line).toBe(2); + }); + + it('flags set_config(\'search_path\', ...) in the body', async () => { + const def = `CREATE FUNCTION app.f() RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + PERFORM set_config('search_path', 'public', true); +END; +$$;`; + const { problems } = await lintDefinition(def, 'plpgsql'); + expect(problems.some((p) => p.ruleId === 'no-set-search-path')).toBe(true); + }); + + it('does not flag a function that never touches search_path', async () => { + const def = `CREATE FUNCTION app.f() RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + PERFORM app.g(); +END; +$$;`; + const { problems } = await lintDefinition(def, 'plpgsql'); + expect(problems.some((p) => p.ruleId === 'no-set-search-path')).toBe(false); + }); +}); + +describe('lint: no-variable-conflict (C2)', () => { + it('flags a #variable_conflict directive', async () => { + const def = `CREATE FUNCTION app.f() RETURNS void LANGUAGE plpgsql +AS $$ +#variable_conflict use_column +DECLARE x int; +BEGIN + x := 1; +END; +$$;`; + const { problems } = await lintDefinition(def, 'plpgsql'); + const c2 = problems.filter((p) => p.ruleId === 'no-variable-conflict'); + expect(c2).toHaveLength(1); + expect(c2[0].line).toBe(3); + expect(c2[0].message).toContain('use_column'); + }); +}); + +describe('lint: no-dynamic-sql (C4)', () => { + it('flags EXECUTE and requires a reason to waive', async () => { + const def = `CREATE FUNCTION app.f(tbl text) RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + EXECUTE format('SELECT 1 FROM %I', tbl); +END; +$$;`; + const { problems, suppressed } = await lintDefinition(def, 'plpgsql'); + expect(problems.some((p) => p.ruleId === 'no-dynamic-sql')).toBe(true); + expect(suppressed).toHaveLength(0); + }); + + it('a reasonless waiver does not suppress (reason required)', async () => { + const def = `CREATE FUNCTION app.f(tbl text) RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + -- safegres-disable-next-line no-dynamic-sql + EXECUTE format('SELECT 1 FROM %I', tbl); +END; +$$;`; + const { problems, suppressed } = await lintDefinition(def, 'plpgsql'); + const c4 = problems.filter((p) => p.ruleId === 'no-dynamic-sql'); + expect(c4).toHaveLength(1); + expect(c4[0].message).toContain('suppression ignored'); + expect(suppressed).toHaveLength(0); + }); + + it('a reasoned waiver suppresses and is retained as accepted risk', async () => { + const def = `CREATE FUNCTION app.f(tbl text) RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + -- safegres-disable-next-line no-dynamic-sql -- lookup-only: building IN-list from integers + EXECUTE format('SELECT 1 FROM %I', tbl); +END; +$$;`; + const { problems, suppressed } = await lintDefinition(def, 'plpgsql'); + expect(problems.some((p) => p.ruleId === 'no-dynamic-sql')).toBe(false); + expect(suppressed).toHaveLength(1); + expect(suppressed[0].reason).toBe('lookup-only: building IN-list from integers'); + expect(suppressed[0].scope).toBe('next-line'); + }); +}); + +describe('lint: require-qualified-refs (C3)', () => { + it('flags an unqualified relation reference', async () => { + const def = `CREATE FUNCTION app.f() RETURNS int LANGUAGE sql +AS $$ + SELECT count(*) FROM users +$$;`; + const { problems } = await lintDefinition(def, 'sql'); + const c3 = problems.filter((p) => p.ruleId === 'require-qualified-refs'); + expect(c3).toHaveLength(1); + expect(c3[0].message).toContain('users'); + }); + + it('does not flag a schema-qualified reference', async () => { + const def = `CREATE FUNCTION app.f() RETURNS int LANGUAGE sql +AS $$ + SELECT count(*) FROM app_public.users +$$;`; + const { problems } = await lintDefinition(def, 'sql'); + expect(problems.some((p) => p.ruleId === 'require-qualified-refs')).toBe(false); + }); + + it('does not flag a CTE name used as a relation', async () => { + const def = `CREATE FUNCTION app.f() RETURNS int LANGUAGE sql +AS $$ + WITH recent AS (SELECT id FROM app_public.users) + SELECT count(*) FROM recent +$$;`; + const { problems } = await lintDefinition(def, 'sql'); + expect(problems.some((p) => p.ruleId === 'require-qualified-refs')).toBe(false); + }); +}); + +describe('lint: suppression scopes', () => { + it('disable-file silences a rule for the whole definition', async () => { + const def = `CREATE FUNCTION app.f() RETURNS int LANGUAGE sql +AS $$ + -- safegres-disable-file require-qualified-refs -- legacy, tracked in #123 + SELECT count(*) FROM users +$$;`; + const { problems, suppressed } = await lintDefinition(def, 'sql'); + expect(problems.some((p) => p.ruleId === 'require-qualified-refs')).toBe(false); + expect(suppressed.some((p) => p.ruleId === 'require-qualified-refs')).toBe(true); + }); + + it('disable/enable bracket a range', async () => { + const def = `CREATE FUNCTION app.f() RETURNS void LANGUAGE plpgsql +AS $$ +BEGIN + -- safegres-disable no-dynamic-sql -- codegen block + EXECUTE 'SELECT 1'; + EXECUTE 'SELECT 2'; + -- safegres-enable no-dynamic-sql +END; +$$;`; + const { problems, suppressed } = await lintDefinition(def, 'plpgsql'); + expect(problems.some((p) => p.ruleId === 'no-dynamic-sql')).toBe(false); + expect(suppressed.filter((p) => p.ruleId === 'no-dynamic-sql')).toHaveLength(2); + }); + + it('an unparseable definition yields no findings', async () => { + const { problems, suppressed } = await lintDefinition('this is not sql', 'plpgsql'); + expect(problems).toHaveLength(0); + expect(suppressed).toHaveLength(0); + }); +}); diff --git a/packages/safegres/__tests__/presets.test.ts b/packages/safegres/__tests__/presets.test.ts index b0094e19e..21625b294 100644 --- a/packages/safegres/__tests__/presets.test.ts +++ b/packages/safegres/__tests__/presets.test.ts @@ -56,6 +56,11 @@ describe('built-in presets', () => { for (const [name, config] of Object.entries(PRESETS)) { if (name === 'safegres:minimal') continue; for (const [rule, setting] of Object.entries(settings(config))) { + // The `C*` convention linter is Constructive house style, not a + // universal fact, so the big-tent presets carry it off and only + // `constructive` turns it on — a deliberate opt-in, not a hidden + // finding. + if (rule.startsWith('C')) continue; const severity = Array.isArray(setting) ? setting[0] : setting; expect(`${name}/${rule}=${severity as string}`).not.toContain('=off'); } diff --git a/packages/safegres/src/commands/audit.ts b/packages/safegres/src/commands/audit.ts index 8ae70d9a2..ef6d8c29f 100644 --- a/packages/safegres/src/commands/audit.ts +++ b/packages/safegres/src/commands/audit.ts @@ -59,11 +59,12 @@ import { configFingerprint } from '../config/fingerprint'; import { allAstRulesDisabled, applyRulesToFindings, matchTablePattern, resolveRules, rulesForTable } from '../config/resolve'; import type { ExposureConfig, SafegresConfig } from '../config/types'; import { resolvePlaneReach, scorePlane, stampPlanes } from '../exposure/planes'; +import { LINT_RULES, LINT_RULES_BY_ID, lintDefinition, type LintProblem, type SuppressedProblem } from '../lint'; import { type ExplainReport, proveFindings } from '../perf/explain'; import { introspectRoleGraph, introspectSchemaAcls } from '../pg/acl'; import type { ResolvedExposure } from '../pg/exposure'; import { resolveExposure, resolvePlanes, resolveReach } from '../pg/exposure'; -import { introspectFunctions } from '../pg/functions'; +import { type FunctionSnapshot, introspectFunctions } from '../pg/functions'; import { introspectIndexes, introspectViews, type TableIndexSnapshot } from '../pg/indexes'; import { asExecutor, type IntrospectOptions, introspectTables, type QueryExecutor, type TableSnapshot } from '../pg/introspect'; import { type AccessPath, classifyPaths } from '../pg/paths'; @@ -140,6 +141,19 @@ export async function audit( const exec = asExecutor(client); const config = options.config ?? {}; const resolved = resolveRules(config); + + // Function definitions are read by two independent features (the convention + // linter and the call graph); introspect them at most once. + let functionsCache: FunctionSnapshot[] | undefined; + const getFunctions = async (): Promise => { + if (!functionsCache) { + functionsCache = await introspectFunctions(exec, { + schemas: options.schemas ?? config.schemas, + excludeSchemas: options.excludeSchemas ?? config.excludeSchemas + }); + } + return functionsCache; + }; const statsEnabled = options.stats ?? config.perf?.stats?.enabled ?? false; const explainEnabled = options.explain ?? config.perf?.explain?.enabled ?? false; // Both tiers are refinements of the perf dimension: asking for either turns @@ -361,6 +375,26 @@ export async function audit( findings.push(...checkStats(statsSnapshot, statsThresholds(config))); } + // --- Convention linter (C*): source-level rules over function definitions --- + const enabledLintRules = LINT_RULES.filter( + (r) => resolved.rules.get(r.code)?.enabled !== false + ); + if (enabledLintRules.length > 0) { + const lintRuleIds = enabledLintRules.map((r) => r.id); + for (const fn of await getFunctions()) { + if (!fn.definition) continue; + const subject = `${fn.schema}.${fn.name}(${fn.args})`; + const { problems, suppressed } = await lintDefinition( + fn.definition, + fn.language, + subject, + { rules: lintRuleIds } + ); + for (const p of problems) findings.push(lintFinding(fn, subject, p)); + for (const s of suppressed) findings.push(lintFinding(fn, subject, s, true)); + } + } + findings = applyRulesToFindings(resolved, findings); // Stamp direction (from the registry) and exposure on every finding. @@ -571,10 +605,7 @@ export async function audit( } if (options.callGraph) { - const functions = await introspectFunctions(exec, { - schemas: options.schemas ?? config.schemas, - excludeSchemas: options.excludeSchemas ?? config.excludeSchemas - }); + const functions = await getFunctions(); report.callGraph = await buildCallGraph({ functions, tables: snapshot, @@ -723,6 +754,44 @@ function policyReferencedRelations(tables: TableSnapshot[]): Set { return referenced; } +/** + * Map a source-level lint problem onto a `Finding`. The function's name rides + * in the `table` slot so overrides, exposure and sorting key on it the same + * way they do for table findings. Suppressed problems are emitted as + * acknowledged findings — visible as accepted risk, off the score — carrying + * their waiver reason and scope in `context`. + */ +function lintFinding( + fn: FunctionSnapshot, + subject: string, + problem: LintProblem | SuppressedProblem, + suppressed = false +): Finding { + const meta = LINT_RULES_BY_ID.get(problem.ruleId)!; + return { + code: meta.code, + severity: RULES_BY_CODE.get(meta.code)!.defaultSeverity, + category: 'convention', + schema: fn.schema, + table: fn.name, + message: `${problem.message} in ${subject}`, + ...(problem.hint ? { hint: problem.hint } : {}), + ...(suppressed ? { acknowledged: true } : {}), + context: { + ...problem.context, + function: subject, + line: problem.line, + ...(suppressed + ? { + suppressed: true, + reason: (problem as SuppressedProblem).reason, + suppressionScope: (problem as SuppressedProblem).scope + } + : {}) + } + }; +} + function compareFindings(a: Finding, b: Finding): number { const order: Record = { critical: 0, high: 1, medium: 2, low: 3, info: 4 }; if (order[a.severity] !== order[b.severity]) return order[a.severity] - order[b.severity]; diff --git a/packages/safegres/src/config/presets.ts b/packages/safegres/src/config/presets.ts index b4b768b5a..3f72b7473 100644 --- a/packages/safegres/src/config/presets.ts +++ b/packages/safegres/src/config/presets.ts @@ -16,6 +16,10 @@ import type { SafegresConfig } from './types'; */ export const recommended: SafegresConfig = { rules: { + // The convention linter (`C*`) enforces Constructive house style rather + // than a universal security fact, so the big-tent preset leaves it off; + // the `constructive` preset turns it on. + 'C*': 'off', R1: ['critical', { rolesFrom: 'anon' }], R2: ['high', { rolesFrom: 'anon' }], L5: ['info', { rolesFrom: 'anon' }], @@ -81,7 +85,17 @@ export const constructive: SafegresConfig = { R1: ['critical', { roles: ['anonymous'], rolesFrom: 'anon' }], R2: ['high', { roles: ['anonymous'], rolesFrom: 'anon' }], R3: 'medium', - L5: ['info', { roles: ['anonymous'], rolesFrom: 'anon' }] + L5: ['info', { roles: ['anonymous'], rolesFrom: 'anon' }], + // House-style convention rules, enforced here for the first time: + // never set search_path (C1), never use #variable_conflict (C2), + // schema-qualify every relation (C3, adoption severity — ratchet to + // error once clean), and no dynamic SQL (C4) unless waived inline with a + // categorized reason (`-- safegres-disable-next-line no-dynamic-sql -- + // lookup-only: …`). + C1: 'high', + C2: 'medium', + C3: 'low', + C4: 'high' }, scoring: { floorOnCritical: 'C' } }; diff --git a/packages/safegres/src/lint/engine.ts b/packages/safegres/src/lint/engine.ts new file mode 100644 index 000000000..35cf54948 --- /dev/null +++ b/packages/safegres/src/lint/engine.ts @@ -0,0 +1,60 @@ +/** + * The lint engine: parse a definition, run the rules, then apply + * suppressions. Pure `source → result`, with no `pg` dependency, so it can be + * unit-tested on string literals and lifted into a standalone package later. + */ + +import { parseUnit } from './parse-unit'; +import { LINT_RULES, LINT_RULES_BY_ID } from './rules'; +import { Suppressions } from './suppressions'; +import type { LintProblem, LintResult, LintRule, SuppressedProblem } from './types'; + +export interface LintOptions { + /** Restrict to these rule ids; omit to run all. */ + rules?: string[]; +} + +/** Lint a single function definition. */ +export async function lintDefinition( + text: string, + language: string, + name?: string, + options: LintOptions = {} +): Promise { + const active: LintProblem[] = []; + const suppressed: SuppressedProblem[] = []; + + const selected: LintRule[] = options.rules + ? options.rules.map((id) => LINT_RULES_BY_ID.get(id)).filter((r): r is LintRule => Boolean(r)) + : LINT_RULES; + if (selected.length === 0) return { problems: active, suppressed }; + + const unit = await parseUnit(text, language, name); + // An unparseable definition produces no lint findings — dynamic/opaque bodies + // are the call-graph's concern (CG5), not the linter's. + if (unit.parseError) return { problems: active, suppressed }; + + const suppressions = new Suppressions(unit.lines); + + for (const rule of selected) { + for (const problem of rule.run(unit)) { + const res = suppressions.resolve(problem.ruleId, problem.line, rule.reasonRequired); + if (res.suppressed) { + suppressed.push({ ...problem, reason: res.reason ?? null, scope: res.scope }); + continue; + } + if (res.invalidMissingReason) { + active.push({ + ...problem, + message: `${problem.message} (suppression ignored: a reason is required)`, + context: { ...problem.context, invalidSuppression: 'missing-reason' } + }); + continue; + } + active.push(problem); + } + } + + active.sort((a, b) => a.line - b.line || a.ruleId.localeCompare(b.ruleId)); + return { problems: active, suppressed }; +} diff --git a/packages/safegres/src/lint/index.ts b/packages/safegres/src/lint/index.ts new file mode 100644 index 000000000..a14d7a0af --- /dev/null +++ b/packages/safegres/src/lint/index.ts @@ -0,0 +1,26 @@ +/** + * Source-level SQL/PL/pgSQL convention linter. + * + * Distinct from safegres's catalog checks: it reasons about the *text* of a + * function definition (fully-qualified references, dynamic SQL, forbidden + * directives) rather than live-database facts, and carries no `pg` dependency. + * safegres is its first consumer; the seam is drawn so it can become a + * standalone `@pgsql/lint` package unchanged. + */ + +export type { LintOptions } from './engine'; +export { lintDefinition } from './engine'; +export { parseUnit } from './parse-unit'; +export { LINT_RULES, LINT_RULES_BY_CODE, LINT_RULES_BY_ID } from './rules'; +export { Suppressions } from './suppressions'; +export type { + DynamicSqlSite, + LintProblem, + LintResult, + LintRule, + LintRuleMeta, + LintUnit, + SqlFragment, + SuppressedProblem, + SuppressionScope +} from './types'; diff --git a/packages/safegres/src/lint/parse-unit.ts b/packages/safegres/src/lint/parse-unit.ts new file mode 100644 index 000000000..a9f80186c --- /dev/null +++ b/packages/safegres/src/lint/parse-unit.ts @@ -0,0 +1,183 @@ +/** + * Turn a `CREATE FUNCTION …` definition into a {@link LintUnit}: the parsed + * SQL statement, the embedded body fragments, and the machinery to map any + * AST location back to an absolute line in the original text. + * + * Line mapping is the whole trick. PL/pgSQL statement line numbers are + * relative to the *body* (`prosrc`), and embedded SQL expressions are parsed + * in isolation, so both have to be re-anchored to the definition text before a + * finding — or a suppression comment — can be matched to them. + */ + +import { parsePlPgSQL } from 'libpg-query'; +import { parse } from 'pgsql-parser'; + +import { findAll } from '../ast/walk'; +import type { DynamicSqlSite, LintUnit, SqlFragment } from './types'; + +/** Count newlines in `s[0..offset)` — i.e. how many lines precede `offset`. */ +function newlinesBefore(s: string, offset: number): number { + let n = 0; + const end = Math.min(offset, s.length); + for (let i = 0; i < end; i++) if (s.charCodeAt(i) === 10) n++; + return n; +} + +/** Absolute 1-based line of a char offset within `text`. */ +function lineOf(text: string, offset: number): number { + return newlinesBefore(text, offset) + 1; +} + +/** The body string of a `CreateFunctionStmt` (`AS $$ … $$`), or null. */ +function functionBody(createFnStmt: Record): string | null { + const options = createFnStmt.options; + if (!Array.isArray(options)) return null; + for (const opt of options) { + const de = (opt as Record).DefElem as Record | undefined; + if (!de || de.defname !== 'as') continue; + const arg = de.arg as Record | undefined; + const list = arg?.List as Record | undefined; + const items = list?.items; + if (!Array.isArray(items) || items.length === 0) return null; + // A two-item AS (`obj_file`, `link_symbol`) is a C function — no SQL body. + if (items.length > 1) return null; + const str = (items[0] as Record).String as Record | undefined; + const sval = str?.sval; + return typeof sval === 'string' ? sval : null; + } + return null; +} + +/** + * Walk the PL/pgSQL JSON tree, collecting (a) every embedded SQL expression + * with the line number of its enclosing statement and (b) every dynamic-SQL + * site. Line numbers here are body-relative; the caller re-anchors them. + */ +function collectPlpgsql( + node: unknown, + currentLine: number, + exprs: Array<{ query: string; parseMode: number; line: number }>, + dynamic: Array<{ line: number; form: string }> +): void { + if (Array.isArray(node)) { + for (const item of node) collectPlpgsql(item, currentLine, exprs, dynamic); + return; + } + if (!node || typeof node !== 'object') return; + const rec = node as Record; + + // A statement node carries its own line; descendants inherit it until the + // next statement re-sets it. + let line = currentLine; + for (const [key, value] of Object.entries(rec)) { + if (key.startsWith('PLpgSQL_stmt_')) { + const stmt = value as Record; + if (typeof stmt.lineno === 'number') line = stmt.lineno; + if (key === 'PLpgSQL_stmt_dynexecute') { + dynamic.push({ line, form: 'EXECUTE' }); + } else if (key === 'PLpgSQL_stmt_dynfors') { + dynamic.push({ line, form: 'FOR … IN EXECUTE' }); + } + } + } + + const expr = rec.PLpgSQL_expr as Record | undefined; + if (expr && typeof expr.query === 'string') { + exprs.push({ + query: expr.query, + parseMode: typeof expr.parseMode === 'number' ? expr.parseMode : 2, + line + }); + } + + for (const value of Object.values(rec)) collectPlpgsql(value, line, exprs, dynamic); +} + +/** Reconstruct a parseable SQL string from a PL/pgSQL embedded expression. */ +function fragmentSql(query: string, parseMode: number): string { + // parseMode 0 = full statement; 3 = assignment (strip the anchored target so + // the RHS parses); anything else is a bare expression. + if (parseMode === 0) return query; + let q = query; + if (parseMode === 3) { + q = q.replace(/^\s*[a-zA-Z_"][\w$".]*(\[[^\]]*\])*\s*:?=\s*/, ''); + } + return `SELECT ${q}`; +} + +/** + * Parse a function definition into a {@link LintUnit}. Never throws: an + * unparseable definition comes back with `parseError` set and no fragments, + * so rules that need the AST simply find nothing. + */ +export async function parseUnit( + text: string, + language: string, + name?: string +): Promise { + const lines = text.split('\n'); + const base: LintUnit = { text, lines, language, name, fragments: [], dynamicSql: [] }; + + let sqlAst: unknown; + try { + sqlAst = await parse(text); + } catch (err) { + return { ...base, parseError: `definition failed to parse: ${(err as Error).message}` }; + } + + const createFnStmt = findAll(sqlAst, 'CreateFunctionStmt')[0]; + if (!createFnStmt) return { ...base, parseError: 'not a CREATE FUNCTION statement' }; + + const body = functionBody(createFnStmt); + const bodyOffset = body !== null ? text.indexOf(body) : -1; + const bodyStartLine = bodyOffset >= 0 ? lineOf(text, bodyOffset) : undefined; + + const fragments: SqlFragment[] = []; + const dynamicSql: DynamicSqlSite[] = []; + + const lang = language.toLowerCase(); + + if (lang === 'sql' && body !== null && bodyStartLine !== undefined) { + // A SQL-language body is itself SQL: parse it whole. Locations are + // relative to the body, so re-anchor them onto the definition. + try { + const ast = await parse(body); + fragments.push({ + ast, + lineForOffset: (offset) => bodyStartLine + newlinesBefore(body, offset) + }); + } catch { + // Body may contain positional parameters etc. that don't parse alone — + // leave it as no fragment rather than erroring the whole unit. + } + } else if (lang === 'plpgsql') { + let plpgsql: unknown; + try { + plpgsql = await parsePlPgSQL(text); + } catch (err) { + return { ...base, createFnStmt, bodyStartLine, parseError: `PL/pgSQL body failed to parse: ${(err as Error).message}` }; + } + const exprs: Array<{ query: string; parseMode: number; line: number }> = []; + const dyn: Array<{ line: number; form: string }> = []; + collectPlpgsql(plpgsql, 0, exprs, dyn); + + const anchor = (bodyLine: number): number => + bodyStartLine !== undefined && bodyLine > 0 ? bodyStartLine + (bodyLine - 1) : (bodyStartLine ?? 1); + + for (const d of dyn) dynamicSql.push({ line: anchor(d.line), form: d.form }); + + for (const e of exprs) { + const sql = fragmentSql(e.query, e.parseMode); + let ast: unknown; + try { + ast = await parse(sql); + } catch { + continue; // opaque fragment — the call-graph's concern, not the linter's + } + const absLine = anchor(e.line); + fragments.push({ ast, lineForOffset: () => absLine }); + } + } + + return { ...base, createFnStmt, bodyStartLine, fragments, dynamicSql }; +} diff --git a/packages/safegres/src/lint/rules/index.ts b/packages/safegres/src/lint/rules/index.ts new file mode 100644 index 000000000..08f3bb9a2 --- /dev/null +++ b/packages/safegres/src/lint/rules/index.ts @@ -0,0 +1,18 @@ +import type { LintRule } from '../types'; +import { noDynamicSql } from './no-dynamic-sql'; +import { noSetSearchPath } from './no-set-search-path'; +import { noVariableConflict } from './no-variable-conflict'; +import { requireQualifiedRefs } from './require-qualified-refs'; + +/** The lint rules, in report order. */ +export const LINT_RULES: LintRule[] = [ + noSetSearchPath, + requireQualifiedRefs, + noVariableConflict, + noDynamicSql +]; + +export const LINT_RULES_BY_ID = new Map(LINT_RULES.map((r) => [r.id, r])); +export const LINT_RULES_BY_CODE = new Map(LINT_RULES.map((r) => [r.code, r])); + +export { noDynamicSql, noSetSearchPath, noVariableConflict, requireQualifiedRefs }; diff --git a/packages/safegres/src/lint/rules/no-dynamic-sql.ts b/packages/safegres/src/lint/rules/no-dynamic-sql.ts new file mode 100644 index 000000000..11356f772 --- /dev/null +++ b/packages/safegres/src/lint/rules/no-dynamic-sql.ts @@ -0,0 +1,29 @@ +/** + * `no-dynamic-sql` (C4): a function must not use dynamic SQL. + * + * Dynamic SQL (`EXECUTE`, `EXECUTE … USING`, `FOR … IN EXECUTE`) is permitted + * only for lookup-only or code-generation work, and never for writes — but the + * string handed to `EXECUTE` is opaque to the parser, so we cannot statically + * tell read from write. The enforceable form is therefore: flag every site, + * and require a categorized waiver. This is the one rule whose suppression + * must carry a reason (see `reasonRequired`), so an approved use always names + * *why* (`lookup-only` / `codegen`). + */ + +import type { LintRule } from '../types'; + +export const noDynamicSql: LintRule = { + id: 'no-dynamic-sql', + code: 'C4', + title: 'Function must not use dynamic SQL', + reasonRequired: true, + run(unit) { + return unit.dynamicSql.map((site) => ({ + ruleId: 'no-dynamic-sql', + line: site.line, + message: `Function uses dynamic SQL (${site.form})`, + hint: 'Avoid dynamic SQL. If it is genuinely lookup-only or code-generation (never a write), waive it with a reason: `-- safegres-disable-next-line no-dynamic-sql -- lookup-only: `.', + context: { form: site.form } + })); + } +}; diff --git a/packages/safegres/src/lint/rules/no-set-search-path.ts b/packages/safegres/src/lint/rules/no-set-search-path.ts new file mode 100644 index 000000000..1006b55b7 --- /dev/null +++ b/packages/safegres/src/lint/rules/no-set-search-path.ts @@ -0,0 +1,67 @@ +/** + * `no-set-search-path` (C1): a function must never set `search_path`. + * + * House rule: rather than pin `search_path` (the usual CWE-426 mitigation for + * SECURITY DEFINER), we fully-qualify every reference and never touch the + * setting at all. This flags both forms: + * - the declarative `CREATE FUNCTION … SET search_path = …` clause (this is + * exactly what `pg_proc.proconfig` / `searchPathPinned` records), and + * - a runtime `set_config('search_path', …)` in the body. + */ + +import type { LintProblem, LintRule, LintUnit } from '../types'; +import { lineOfOffset } from '../util'; + +function optionSites(unit: LintUnit): LintProblem[] { + const out: LintProblem[] = []; + const options = unit.createFnStmt?.options; + if (!Array.isArray(options)) return out; + for (const opt of options) { + const de = (opt as Record).DefElem as Record | undefined; + if (!de || de.defname !== 'set') continue; + const vss = (de.arg as Record | undefined)?.VariableSetStmt as + | Record + | undefined; + if (!vss || vss.name !== 'search_path') continue; + const loc = typeof de.location === 'number' ? de.location : 0; + out.push({ + ruleId: 'no-set-search-path', + line: lineOfOffset(unit.text, loc), + message: 'Function sets search_path', + hint: 'Never set search_path. Fully-qualify every relation, function and type reference instead.', + context: { form: 'SET clause' } + }); + } + return out; +} + +function setConfigSites(unit: LintUnit): LintProblem[] { + const out: LintProblem[] = []; + const re = /\bset_config\s*\(\s*'search_path'/i; + unit.lines.forEach((text, i) => { + if (re.test(text)) { + out.push({ + ruleId: 'no-set-search-path', + line: i + 1, + message: 'Function sets search_path via set_config()', + hint: 'Never set search_path. Fully-qualify references instead of relying on it.', + context: { form: 'set_config()' } + }); + } + }); + return out; +} + +export const noSetSearchPath: LintRule = { + id: 'no-set-search-path', + code: 'C1', + title: 'Function must not set search_path', + reasonRequired: false, + run(unit) { + const byLine = new Map(); + for (const p of [...optionSites(unit), ...setConfigSites(unit)]) { + if (!byLine.has(p.line)) byLine.set(p.line, p); + } + return [...byLine.values()].sort((a, b) => a.line - b.line); + } +}; diff --git a/packages/safegres/src/lint/rules/no-variable-conflict.ts b/packages/safegres/src/lint/rules/no-variable-conflict.ts new file mode 100644 index 000000000..543ff0034 --- /dev/null +++ b/packages/safegres/src/lint/rules/no-variable-conflict.ts @@ -0,0 +1,38 @@ +/** + * `no-variable-conflict` (C2): a PL/pgSQL body must not use a + * `#variable_conflict` directive. + * + * The directive papers over an ambiguity between a column name and a PL/pgSQL + * variable; the house style is to remove the ambiguity (rename the variable, + * qualify the column) rather than declare a winner. The directive is a + * compiler pragma that must start a line at the top of the body, so a line + * scan is exact — it never appears inside an expression or string. + */ + +import type { LintRule } from '../types'; + +const RE = /^\s*#variable_conflict\b\s*(\S+)?/i; + +export const noVariableConflict: LintRule = { + id: 'no-variable-conflict', + code: 'C2', + title: 'Function must not use #variable_conflict', + reasonRequired: false, + run(unit) { + if (unit.language.toLowerCase() !== 'plpgsql') return []; + const out = []; + for (let i = 0; i < unit.lines.length; i++) { + const m = RE.exec(unit.lines[i]); + if (!m) continue; + const mode = m[1] ?? ''; + out.push({ + ruleId: 'no-variable-conflict', + line: i + 1, + message: `Function uses #variable_conflict${mode ? ` ${mode}` : ''}`, + hint: 'Remove the directive and disambiguate explicitly: rename the variable or qualify the column reference.', + context: { mode } + }); + } + return out; + } +}; diff --git a/packages/safegres/src/lint/rules/require-qualified-refs.ts b/packages/safegres/src/lint/rules/require-qualified-refs.ts new file mode 100644 index 000000000..c815a5cb2 --- /dev/null +++ b/packages/safegres/src/lint/rules/require-qualified-refs.ts @@ -0,0 +1,67 @@ +/** + * `require-qualified-refs` (C3): every relation reference must be + * schema-qualified. + * + * Banning `SET search_path` (C1) only removes the footgun; it does not make + * name resolution safe on its own. This is the rule that actually enforces the + * discipline: an unqualified `FROM users` resolves against whatever + * search_path happens to be, so it must be `FROM app_public.users`. + * + * v1 covers relation references (`RangeVar`). Names introduced by a CTE in the + * same query are excluded — they are not schema objects. Unqualified *function* + * calls are deferred (they need a built-in allowlist to avoid flagging + * `now()`, `count()`, …). + */ + +import { findAll } from '../../ast/walk'; +import type { LintProblem, LintRule, SqlFragment } from '../types'; + +function cteNames(ast: unknown): Set { + const out = new Set(); + for (const cte of findAll(ast, 'CommonTableExpr')) { + if (typeof cte.ctename === 'string') out.add(cte.ctename); + } + return out; +} + +function fragmentProblems(fragment: SqlFragment): LintProblem[] { + const out: LintProblem[] = []; + const ctes = cteNames(fragment.ast); + const seen = new Set(); + for (const rv of findAll(fragment.ast, 'RangeVar')) { + const relname = typeof rv.relname === 'string' ? rv.relname : undefined; + if (!relname) continue; + if (typeof rv.schemaname === 'string' && rv.schemaname.length > 0) continue; + if (ctes.has(relname)) continue; + const loc = typeof rv.location === 'number' ? rv.location : -1; + const line = fragment.lineForOffset(loc >= 0 ? loc : 0); + const key = `${line}:${relname}`; + if (seen.has(key)) continue; + seen.add(key); + out.push({ + ruleId: 'require-qualified-refs', + line, + message: `Unqualified relation reference "${relname}"`, + hint: 'Schema-qualify the reference (e.g. `app_public.' + relname + '`). Unqualified names resolve against search_path.', + context: { relation: relname } + }); + } + return out; +} + +export const requireQualifiedRefs: LintRule = { + id: 'require-qualified-refs', + code: 'C3', + title: 'Relation references must be schema-qualified', + reasonRequired: false, + run(unit) { + const byKey = new Map(); + for (const fragment of unit.fragments) { + for (const p of fragmentProblems(fragment)) { + const key = `${p.line}:${(p.context as { relation: string }).relation}`; + if (!byKey.has(key)) byKey.set(key, p); + } + } + return [...byKey.values()].sort((a, b) => a.line - b.line); + } +}; diff --git a/packages/safegres/src/lint/suppressions.ts b/packages/safegres/src/lint/suppressions.ts new file mode 100644 index 000000000..c199fc902 --- /dev/null +++ b/packages/safegres/src/lint/suppressions.ts @@ -0,0 +1,162 @@ +/** + * ESLint / Prettier-style suppression comments, embedded in the SQL body as + * `--` line comments. Because they live in the function source, a waiver + * authored in a migration survives `pg_get_functiondef` and is visible to the + * live-database audit — the comment is the single source of truth. + * + * Grammar (a `--` comment containing): + * + * safegres-disable-next-line […] [-- ] next physical line + * safegres-disable-line […] [-- ] this physical line + * safegres-disable […] [-- ] until a matching enable + * safegres-enable […] closes a disable range + * safegres-disable-file […] [-- ] the whole definition + * + * With no rule listed a directive applies to every rule. A reason follows a + * second `--` (ESLint style) or a `:`. Rules whose metadata requires a reason + * are *not* silenced by a reasonless directive — the finding stands, so a + * waiver is never silent. + */ + +import type { SuppressionScope } from './types'; + +interface LineDirective { + scope: 'next-line' | 'line'; + targetLine: number; + rules: Set | null; + reason: string | null; +} + +interface Interval { + rule: string | null; + start: number; + end: number; + reason: string | null; +} + +interface FileDirective { + rules: Set | null; + reason: string | null; +} + +export interface SuppressionMatch { + scope: SuppressionScope; + reason: string | null; +} + +export interface SuppressionResolution { + /** The directive silences the finding. */ + suppressed: boolean; + scope?: SuppressionScope; + reason?: string | null; + /** A directive matched but lacked a required reason, so it does not apply. */ + invalidMissingReason?: boolean; +} + +const DIRECTIVE_RE = + /safegres-(disable-next-line|disable-line|disable-file|disable|enable)\b[ \t]*([^\r\n]*)/i; + +function splitReason(rest: string): { ruleSpec: string; reason: string | null } { + const dashIdx = rest.indexOf('--'); + if (dashIdx >= 0) { + return { ruleSpec: rest.slice(0, dashIdx), reason: normalizeReason(rest.slice(dashIdx + 2)) }; + } + const colonIdx = rest.indexOf(':'); + if (colonIdx >= 0) { + return { ruleSpec: rest.slice(0, colonIdx), reason: normalizeReason(rest.slice(colonIdx + 1)) }; + } + return { ruleSpec: rest, reason: null }; +} + +function normalizeReason(s: string): string | null { + const t = s.trim().replace(/\*\/\s*$/, '').trim(); + return t.length > 0 ? t : null; +} + +function parseRules(ruleSpec: string): Set | null { + const parts = ruleSpec.split(/[\s,]+/).map((p) => p.trim()).filter((p) => p.length > 0); + return parts.length > 0 ? new Set(parts) : null; +} + +/** Parsed suppression state for one definition, queryable by (rule, line). */ +export class Suppressions { + private readonly lineDirectives: LineDirective[] = []; + private readonly intervals: Interval[] = []; + private readonly fileDirectives: FileDirective[] = []; + + constructor(lines: string[]) { + // Range bookkeeping: an open disable per rule (and one for "all"). + const open = new Map(); + + lines.forEach((text, i) => { + const line = i + 1; + const m = DIRECTIVE_RE.exec(text); + if (!m) return; + const kind = m[1].toLowerCase(); + const { ruleSpec, reason } = splitReason(m[2] ?? ''); + const rules = parseRules(ruleSpec); + + switch (kind) { + case 'disable-next-line': + this.lineDirectives.push({ scope: 'next-line', targetLine: line + 1, rules, reason }); + break; + case 'disable-line': + this.lineDirectives.push({ scope: 'line', targetLine: line, rules, reason }); + break; + case 'disable-file': + this.fileDirectives.push({ rules, reason }); + break; + case 'disable': { + const keys: Array = rules ? [...rules] : [null]; + for (const k of keys) if (!open.has(k)) open.set(k, { start: line, reason }); + break; + } + case 'enable': { + const keys: Array = rules ? [...rules] : [...open.keys()]; + for (const k of keys) { + const o = open.get(k); + if (o) { + this.intervals.push({ rule: k, start: o.start, end: line, reason: o.reason }); + open.delete(k); + } + } + break; + } + } + }); + + for (const [rule, o] of open) { + this.intervals.push({ rule, start: o.start, end: Number.POSITIVE_INFINITY, reason: o.reason }); + } + } + + private match(ruleId: string, line: number): SuppressionMatch | null { + for (const f of this.fileDirectives) { + if (f.rules === null || f.rules.has(ruleId)) return { scope: 'file', reason: f.reason }; + } + for (const d of this.lineDirectives) { + if (d.targetLine === line && (d.rules === null || d.rules.has(ruleId))) { + return { scope: d.scope, reason: d.reason }; + } + } + for (const iv of this.intervals) { + if ((iv.rule === null || iv.rule === ruleId) && line >= iv.start && line < iv.end) { + return { scope: 'range', reason: iv.reason }; + } + } + return null; + } + + /** + * Resolve whether a finding is suppressed. `reasonRequired` rules are only + * silenced by a directive that carries a reason. + */ + resolve(ruleId: string, line: number, reasonRequired: boolean): SuppressionResolution { + const m = this.match(ruleId, line); + if (!m) return { suppressed: false }; + if (reasonRequired && (m.reason === null || m.reason.length === 0)) { + return { suppressed: false, invalidMissingReason: true, scope: m.scope }; + } + return { suppressed: true, scope: m.scope, reason: m.reason }; + } +} diff --git a/packages/safegres/src/lint/types.ts b/packages/safegres/src/lint/types.ts new file mode 100644 index 000000000..c1d71f046 --- /dev/null +++ b/packages/safegres/src/lint/types.ts @@ -0,0 +1,94 @@ +/** + * Source-level SQL/PL/pgSQL linter — types. + * + * This module is deliberately free of any `pg` / catalog dependency: it takes + * a function *definition* (the `CREATE FUNCTION …` text, as `pg_get_functiondef` + * returns it, or as authored in a migration) and returns findings. That keeps + * it mechanically liftable into a standalone `@pgsql/lint` package if a second + * consumer ever appears; safegres is just the first one. + */ + +/** A parsed function definition, in the coordinate space the linter reports in. */ +export interface LintUnit { + /** The full definition text — the line/column space every finding refers to. */ + text: string; + /** `text` split on `\n`, 1-based when indexed as `lines[line - 1]`. */ + lines: string[]; + /** `sql`, `plpgsql`, `c`, `internal`, … (lower-cased `pg_proc.prolang`). */ + language: string; + /** Display name for messages, e.g. `app.grant_role(text)`. */ + name?: string; + /** The `CreateFunctionStmt` AST node, when the text parsed as one. */ + createFnStmt?: Record; + /** Absolute line (1-based, within `text`) the function body's first char sits on. */ + bodyStartLine?: number; + /** Embedded SQL fragments (body statements / expressions) with a line mapper. */ + fragments: SqlFragment[]; + /** Dynamic-SQL statements found in a PL/pgSQL body, by absolute line. */ + dynamicSql: DynamicSqlSite[]; + /** True when the definition (or its body) could not be parsed. */ + parseError?: string; +} + +/** One embedded SQL statement/expression, with a char-offset → absolute-line mapper. */ +export interface SqlFragment { + /** Parsed SQL AST for this fragment. */ + ast: unknown; + /** Map a char offset within this fragment's source to an absolute line in `text`. */ + lineForOffset: (offset: number) => number; +} + +export interface DynamicSqlSite { + line: number; + /** `EXECUTE`, `EXECUTE … USING`, or `FOR … IN EXECUTE`. */ + form: string; +} + +/** A single lint finding, before suppressions are applied. */ +export interface LintProblem { + ruleId: string; + /** Absolute line (1-based) within the definition. */ + line: number; + message: string; + hint?: string; + context?: Record; +} + +/** A problem that a suppression comment silenced — reported, never dropped. */ +export interface SuppressedProblem extends LintProblem { + /** The reason text from the directive, or null when none was given. */ + reason: string | null; + scope: SuppressionScope; +} + +export type SuppressionScope = 'next-line' | 'line' | 'range' | 'file'; + +/** The result of linting one definition. */ +export interface LintResult { + /** Active findings — not suppressed. */ + problems: LintProblem[]; + /** Suppressed findings, kept for the "accepted risk" report bucket. */ + suppressed: SuppressedProblem[]; +} + +/** Static metadata for a lint rule. */ +export interface LintRuleMeta { + /** ESLint-style stable id, e.g. `no-dynamic-sql`. */ + id: string; + /** safegres registry code this rule maps to, e.g. `C4`. */ + code: string; + title: string; + /** + * Whether a suppression of this rule must carry a reason. When true, a bare + * `safegres-disable*` directive does not suppress — the finding stands — so + * a waiver is never silent. Only `no-dynamic-sql` requires it: it is the one + * rule we expect to be waived (lookup-only / codegen), and the waiver's whole + * value is the documented reason. + */ + reasonRequired: boolean; +} + +/** A lint rule: pure `unit → problems`. */ +export interface LintRule extends LintRuleMeta { + run: (unit: LintUnit) => LintProblem[]; +} diff --git a/packages/safegres/src/lint/util.ts b/packages/safegres/src/lint/util.ts new file mode 100644 index 000000000..ed5ef70b2 --- /dev/null +++ b/packages/safegres/src/lint/util.ts @@ -0,0 +1,7 @@ +/** Absolute 1-based line of a char offset within `text`. */ +export function lineOfOffset(text: string, offset: number): number { + let n = 0; + const end = Math.min(Math.max(offset, 0), text.length); + for (let i = 0; i < end; i++) if (text.charCodeAt(i) === 10) n++; + return n + 1; +} diff --git a/packages/safegres/src/rules/registry.ts b/packages/safegres/src/rules/registry.ts index a8b2c1232..8c02e2117 100644 --- a/packages/safegres/src/rules/registry.ts +++ b/packages/safegres/src/rules/registry.ts @@ -26,8 +26,10 @@ export interface RuleMeta { /** * Rules with `scope: 'policy-ast'` require parsing policy expressions. * When every one of them is disabled the audit skips AST work entirely. + * `function-src` rules are the source-level convention linter (`C*`): they + * read function definitions rather than the catalog, and run their own pass. */ - scope: 'table' | 'policy-ast' | 'index' | 'stats'; + scope: 'table' | 'policy-ast' | 'index' | 'stats' | 'function-src'; } /** The scoring axis a rule belongs to (`security` unless declared otherwise). */ @@ -354,6 +356,38 @@ export const RULES: RuleMeta[] = [ dimension: 'perf', title: 'Statement hotspot on a table in scope (pg_stat_statements)', scope: 'stats' + }, + { + code: 'C1', + category: 'convention', + defaultSeverity: 'high', + direction: 'fail-open', + title: 'Function sets search_path (house rule: never set it — fully-qualify instead)', + scope: 'function-src' + }, + { + code: 'C2', + category: 'convention', + defaultSeverity: 'medium', + direction: 'neutral', + title: 'Function uses a #variable_conflict directive', + scope: 'function-src' + }, + { + code: 'C3', + category: 'convention', + defaultSeverity: 'low', + direction: 'neutral', + title: 'Function has an unqualified relation reference (relies on search_path)', + scope: 'function-src' + }, + { + code: 'C4', + category: 'convention', + defaultSeverity: 'high', + direction: 'fail-open', + title: 'Function uses dynamic SQL (waivable with a categorized reason)', + scope: 'function-src' } ]; diff --git a/packages/safegres/src/types.ts b/packages/safegres/src/types.ts index 5c1466bdc..50639a03d 100644 --- a/packages/safegres/src/types.ts +++ b/packages/safegres/src/types.ts @@ -38,7 +38,7 @@ export interface Finding { code: string; severity: Severity; /** High-level bucket — helps renderers group findings. */ - category: 'flags' | 'coverage' | 'anti-pattern' | 'index' | 'sync' | 'match' | 'meta'; + category: 'flags' | 'coverage' | 'anti-pattern' | 'index' | 'sync' | 'match' | 'meta' | 'convention'; /** Leak vs deny vs directionless. Stamped from the rule registry. */ direction?: Direction; /** Scoring axis. Stamped from the rule registry; defaults to `security`. */