Skip to content

fix(sql): harden risk classification and fail closed - #26

Open
nevlate wants to merge 5 commits into
OtterMind:mainfrom
nevlate:codex/sql-risk-hardening
Open

nevlate wants to merge 5 commits into
OtterMind:mainfrom
nevlate:codex/sql-risk-hardening

Conversation

@nevlate

@nevlate nevlate commented Aug 12, 2026 •

Copy link
Copy Markdown

Summary

  • replace prefix-only SQL classification with quote-, comment-, dollar-quote-, and parenthesis-aware scanning in both TypeScript and Java
  • keep UNKNOWN as a sticky analysis signal and fail closed with UNCLASSIFIED_SQL_BLOCKED, while reusing NUBASE_ALLOW_DANGEROUS_SQL as the explicit override
  • enforce the same DANGEROUS / UNKNOWN gate in the Java MCP executeSql path so direct server calls cannot bypass the CLI policy
  • share regression fixtures across TypeScript and Java for CTEs, DO, EXPLAIN ANALYZE, full-table DELETE ... RETURNING, parenthesized reads, maintenance commands, and PREPARE / EXECUTE

Root cause

The previous implementation split statements on raw semicolons and classified mostly from the first keyword. That made dollar-quoted bodies unreliable, hid writes behind wrappers such as CTEs and EXPLAIN ANALYZE, and allowed UNKNOWN statements to disappear when multi-statement risk was folded to a single maximum severity. The Java execution path also returned a risk label without enforcing it.

Behavior

  • DO $$ BEGIN DELETE ...; END $$ -> DANGEROUS
  • EXPLAIN ANALYZE DELETE ... -> DANGEROUS
  • DELETE FROM ... RETURNING * -> DANGEROUS
  • (SELECT 1) -> READ
  • SELECT ... INTO ... -> SCHEMA_WRITE
  • any unclassified statement remains sticky in multi-statement input and is blocked by default
  • transaction/session commands and common maintenance statements are explicitly categorized to limit false-positive blocking
  • idempotent destructive statements such as DROP POLICY IF EXISTS ... remain DANGEROUS; both CLI and direct MCP execution require the explicit NUBASE_ALLOW_DANGEROUS_SQL=true override

Validation

  • MCP bridge/CLI: 122 tests passed
  • Java: 946 tests passed, 22 skipped
  • Spotless passed for all changed Java files
  • git diff --check passed

Follow-up boundary

This PR keeps static classification as the policy authority. A follow-up PR can make PostgreSQL transaction probing (BEGIN READ ONLY / existing dry-run rollback flow) authoritative, including the required asynchronous CLI call-chain changes.

@nevlate
nevlate marked this pull request as ready for review August 12, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant