diff --git a/CLAUDE.md b/CLAUDE.md index a0ea9dd..d7d19ea 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -148,6 +148,7 @@ notify: devdb: # planned — design spec complete, impl in SP1–SP6 PRs provider: null # ghost | docker | null (default: null = disabled) template: "" # source DB to fork from (required when provider != null) + function_denylist_extra: [] # extra side-effecting function names `vxd db sql` rejects without --write (adds to built-in pg_terminate_backend/lo_import/... list) on_failure: keep_db: false retain_hours: 24 @@ -219,7 +220,7 @@ dashboard: | `vxd db` | Manage ephemeral story databases (devdb). Provider set via `devdb.provider` in vxd.yaml. | | `vxd db list` | List all DBs known to the devdb provider | | `vxd db connect ` | Print psql connect command + DSN for a DB (alias: `psql`) | -| `vxd db sql ""` | Run a one-shot SQL query against a DB (read-only by default; pass `--write` to allow INSERT/UPDATE/DELETE/DDL; multi-statement queries are always rejected) | +| `vxd db sql ""` | Run a one-shot SQL query against a DB (read-only by default; pass `--write` to allow INSERT/UPDATE/DELETE/DDL; multi-statement queries are always rejected; known side-effecting functions — `pg_terminate_backend`, `pg_cancel_backend`, `lo_import`, `lo_export`, `pg_read_file`, `pg_ls_dir`, `pg_reload_conf`, `pg_stat_file` — are denied without `--write`, extendable via `devdb.function_denylist_extra`) | | `vxd db schema ` | Print agent-friendly schema dump for a DB | | `vxd db delete ` | Delete a DB permanently (requires `--confirm`) | | `vxd db gc` | Run orphan recovery — scan for stale DBs and release old ones | diff --git a/README.md b/README.md index 0a3d537..71198dd 100644 --- a/README.md +++ b/README.md @@ -432,9 +432,10 @@ devdb: provider: ghost # or docker, or null template: my-prod-snapshot ghost: { api_key_env: GHOST_API_KEY } + function_denylist_extra: [] # extra function names `vxd db sql` rejects without --write ``` -Agents read `DATABASE_URL` from `.vxd-db/connect.env` (auto-injected into the worktree). Humans use `vxd db list/connect/logs/delete` plus the dashboard's per-story DB column. +Agents read `DATABASE_URL` from `.vxd-db/connect.env` (auto-injected into the worktree). Humans use `vxd db list/connect/logs/delete` plus the dashboard's per-story DB column. `vxd db sql` without `--write` statically rejects known side-effecting functions (`pg_terminate_backend`, `pg_cancel_backend`, `lo_import`, `lo_export`, `pg_read_file`, `pg_ls_dir`, `pg_reload_conf`, `pg_stat_file`) that a `READ ONLY` transaction cannot block; extend the list per project with `devdb.function_denylist_extra`. For runtime usage run `vxd db --help`. diff --git a/internal/cli/db.go b/internal/cli/db.go index 94f5a69..e2f9c70 100644 --- a/internal/cli/db.go +++ b/internal/cli/db.go @@ -53,6 +53,19 @@ func dbProviderFor(cmd *cobra.Command) (devdb.Provider, error) { return newDevDBProvider(cfg) } +// devdbFunctionDenylistExtra returns the operator-supplied additions to the +// SQL side-effecting-function denylist. Best-effort: a config-load failure +// returns nil — the built-in denylist still applies, and the provider load +// that follows will surface the real error. +func devdbFunctionDenylistExtra(cmd *cobra.Command) []string { + s, err := loadStores(cmd) + if err != nil { + return nil + } + defer s.Close() + return projectRuntimeConfig(s).DevDB.FunctionDenylistExtra +} + // findDBByNameOrID looks up a DB from the provider list by name or ID. func findDBByNameOrID(dbs []devdb.DB, nameOrID string) (*devdb.DB, error) { for i := range dbs { @@ -208,15 +221,21 @@ Defense layers (in order): transaction and ROLLBACKs at the end, so Postgres itself rejects any data-modifying statement that slipped past the static check. -Caveat: a READ ONLY transaction does NOT block side-effecting function -calls (e.g. pg_terminate_backend, lo_unlink, user functions that write). -Audit the connection's privileges accordingly.`, + 4. Known side-effecting functions that READ ONLY does not block + (pg_terminate_backend, pg_cancel_backend, lo_import, lo_export, + pg_read_file, pg_ls_dir, pg_reload_conf, pg_stat_file) are rejected + without --write. Extend the list per project via + devdb.function_denylist_extra in vxd.yaml. + +Caveat: user-defined functions that write are NOT auto-detected — add +them to devdb.function_denylist_extra and audit the connection's +privileges accordingly.`, Args: cobra.ExactArgs(2), SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { writeFlag, _ := cmd.Flags().GetBool("write") query := args[1] - if err := sqlsafety.ValidateSQLForReadOnly(query, writeFlag); err != nil { + if err := sqlsafety.ValidateSQL(query, writeFlag, devdbFunctionDenylistExtra(cmd)); err != nil { return err } diff --git a/internal/config/config.go b/internal/config/config.go index 9c4d950..3becf50 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -419,6 +419,11 @@ type DevDBConfig struct { OnFailure DevDBFailurePolicy `yaml:"on_failure"` Ghost DevDBGhostConfig `yaml:"ghost"` Docker DevDBDockerConfig `yaml:"docker"` + // FunctionDenylistExtra appends operator-supplied function names to the + // built-in side-effecting-function denylist enforced by `vxd db sql` + // without --write (pg_terminate_backend, lo_import, pg_read_file, …). + // Use it to block site-specific functions that write despite READ ONLY. + FunctionDenylistExtra []string `yaml:"function_denylist_extra,omitempty"` } // DevDBFailurePolicy controls behaviour when a story finishes with an error. diff --git a/internal/sqlsafety/sql_safety.go b/internal/sqlsafety/sql_safety.go index 29ffe1b..fa1c374 100644 --- a/internal/sqlsafety/sql_safety.go +++ b/internal/sqlsafety/sql_safety.go @@ -2,7 +2,9 @@ package sqlsafety import ( "fmt" + "regexp" "strings" + "sync" ) // QueryKind classifies the leading statement of a SQL query for safety @@ -29,8 +31,10 @@ const ( // Even for the keywords below, the read-only path executes inside a // `BEGIN READ ONLY ... ROLLBACK` transaction so Postgres enforces the // promise at the protocol level. Side-effecting *function calls* (e.g. -// `SELECT pg_terminate_backend(...)`) are NOT blocked by READ ONLY; -// the --write flag and SQL parser don't catch those either, and the +// `SELECT pg_terminate_backend(...)`) are NOT blocked by READ ONLY — +// the known dangerous ones are rejected statically instead (see +// defaultFunctionDenylist); anything not on the denylist (user-defined +// functions that write) remains the operator's responsibility, and the // command's Long help calls this out explicitly. var readOnlyLeadingKeywords = map[string]struct{}{ "SELECT": {}, @@ -39,6 +43,70 @@ var readOnlyLeadingKeywords = map[string]struct{}{ "TABLE": {}, // TABLE foo; is shorthand for SELECT * FROM foo } +// defaultFunctionDenylist names Postgres functions with side effects that a +// READ ONLY transaction does NOT block: session termination, large-object +// and server-filesystem access, and config reload. Calling any of these from +// `vxd db sql` without --write is rejected by the static classifier — on a +// shared devdb host they let one local user disrupt or read another user's +// session even though the transaction never "writes". +var defaultFunctionDenylist = []string{ + "pg_terminate_backend", + "pg_cancel_backend", + "lo_import", + "lo_export", + "pg_read_file", + "pg_ls_dir", + "pg_reload_conf", + "pg_stat_file", +} + +// denyPatternCache holds compiled per-function regexes. Patterns are tiny and +// the set is small; the cache just avoids recompiling on every CLI query. +var ( + denyPatternMu sync.Mutex + denyPatternCache = map[string]*regexp.Regexp{} +) + +// denyPattern returns a case-insensitive pattern matching a *call* of fn: +// the function name as a whole identifier (so `my_pg_terminate_backend(` does +// not match, while `pg_catalog.pg_terminate_backend (` does) followed by an +// opening parenthesis with optional whitespace. +func denyPattern(fn string) *regexp.Regexp { + denyPatternMu.Lock() + defer denyPatternMu.Unlock() + if re, ok := denyPatternCache[fn]; ok { + return re + } + re := regexp.MustCompile(`(?i)(^|[^a-zA-Z0-9_])` + regexp.QuoteMeta(strings.ToLower(fn)) + `[\s]*\(`) + denyPatternCache[fn] = re + return re +} + +// ContainsDeniedFunction reports the first denied side-effecting function +// called in the query, checking the built-in denylist plus any extra names +// supplied by the operator (devdb.function_denylist_extra). Comments and +// string literals are stripped first, so a `pg_terminate/**/_backend(...)` +// comment ambush reassembles into the plain call and is caught, while a +// quoted mention ('pg_terminate_backend') does not false-positive. +func ContainsDeniedFunction(query string, extra []string) (string, bool) { + stripped := stripSQLCommentsAndStrings(query) + for _, fn := range defaultFunctionDenylist { + if denyPattern(fn).MatchString(stripped) { + return fn, true + } + } + for _, fn := range extra { + fn = strings.TrimSpace(fn) + if fn == "" { + continue + } + if denyPattern(fn).MatchString(stripped) { + return fn, true + } + } + return "", false +} + // IsMultiStatement reports whether the query contains more than one // top-level statement separated by `;`. Trailing semicolons are tolerated. // Detection is intentionally simple: any `;` followed by non-whitespace, @@ -77,6 +145,16 @@ func ClassifyQuery(query string) QueryKind { // in that case multi-statement is still rejected so a typo can't silently // run extra statements, but the leading-keyword check is skipped. func ValidateSQLForReadOnly(query string, writeFlag bool) error { + return ValidateSQL(query, writeFlag, nil) +} + +// ValidateSQL is ValidateSQLForReadOnly plus the side-effecting-function +// denylist: in read-only mode (writeFlag=false) a call to any function in +// defaultFunctionDenylist or extraDeny is rejected, because BEGIN READ ONLY +// does not stop pg_terminate_backend / lo_import / pg_read_file-class calls. +// Operators explicitly opting into mutations with --write skip the denylist, +// same as they skip the leading-keyword check. +func ValidateSQL(query string, writeFlag bool, extraDeny []string) error { if strings.TrimSpace(query) == "" { return fmt.Errorf("query is empty") } @@ -86,6 +164,9 @@ func ValidateSQLForReadOnly(query string, writeFlag bool) error { if writeFlag { return nil } + if fn, hit := ContainsDeniedFunction(query, extraDeny); hit { + return fmt.Errorf("query calls side-effecting function %s(), which READ ONLY transactions do not block; re-run with --write if this is intentional", fn) + } switch ClassifyQuery(query) { case QueryReadOnly: return nil diff --git a/internal/sqlsafety/sql_safety_test.go b/internal/sqlsafety/sql_safety_test.go index eeab07f..3847c6c 100644 --- a/internal/sqlsafety/sql_safety_test.go +++ b/internal/sqlsafety/sql_safety_test.go @@ -143,3 +143,98 @@ func TestStripSQLCommentsAndStrings(t *testing.T) { } } } + +// TestSQLSafety_FunctionDenylist pins the side-effecting-function denylist: +// every built-in denied function is rejected in read-only mode, matching is +// case-insensitive and whitespace-invariant, comment ambushes reassemble and +// are caught, and near-miss identifiers do NOT false-positive. +func TestSQLSafety_FunctionDenylist(t *testing.T) { + denied := []struct { + name string + query string + }{ + {"pg_terminate_backend", "SELECT pg_terminate_backend(1234)"}, + {"pg_cancel_backend", "SELECT pg_cancel_backend(1234)"}, + {"lo_import", "SELECT lo_import('/etc/passwd')"}, + {"lo_export", "SELECT lo_export(12345, '/tmp/out')"}, + {"pg_read_file", "SELECT pg_read_file('postgresql.conf')"}, + {"pg_ls_dir", "SELECT pg_ls_dir('.')"}, + {"pg_reload_conf", "SELECT pg_reload_conf()"}, + {"pg_stat_file", "SELECT pg_stat_file('postgresql.conf')"}, + {"case-insensitive", "SELECT PG_TERMINATE_BACKEND(1)"}, + {"mixed-case", "SELECT Pg_Terminate_Backend(1)"}, + {"whitespace before paren", "SELECT pg_terminate_backend (1)"}, + {"newline before paren", "SELECT pg_terminate_backend\n(1)"}, + {"schema-qualified", "SELECT pg_catalog.pg_terminate_backend(1)"}, + {"comment ambush reassembles", "SELECT pg_terminate/**/_backend(1)"}, + {"nested in expression", "SELECT 1 WHERE pg_terminate_backend(pid) IS NOT NULL"}, + } + for _, tt := range denied { + t.Run("denied/"+tt.name, func(t *testing.T) { + err := ValidateSQL(tt.query, false, nil) + if err == nil { + t.Fatalf("ValidateSQL(%q, false) = nil, want denylist rejection", tt.query) + } + if !strings.Contains(err.Error(), "side-effecting function") { + t.Errorf("error %v does not mention side-effecting function", err) + } + }) + } + + allowed := []struct { + name string + query string + }{ + {"prefix identifier is a different function", "SELECT my_pg_terminate_backend(1)"}, + {"column named like function, no call", "SELECT pg_terminate_backend FROM audit_log"}, + {"quoted string mention", "SELECT * FROM logs WHERE msg = 'pg_terminate_backend(1)'"}, + {"comment mention", "SELECT 1 -- pg_terminate_backend(1)"}, + {"plain select", "SELECT id, name FROM users"}, + } + for _, tt := range allowed { + t.Run("allowed/"+tt.name, func(t *testing.T) { + if err := ValidateSQL(tt.query, false, nil); err != nil { + t.Errorf("ValidateSQL(%q, false) = %v, want nil", tt.query, err) + } + }) + } + + // --write skips the denylist — the operator explicitly opted in. + if err := ValidateSQL("SELECT pg_terminate_backend(1)", true, nil); err != nil { + t.Errorf("ValidateSQL with --write = %v, want nil (denylist skipped)", err) + } +} + +// TestSQLSafety_DenylistExtraConfig pins the operator-extended denylist +// (devdb.function_denylist_extra): extra names are denied with the same +// call-shape matching, and blank entries are ignored. +func TestSQLSafety_DenylistExtraConfig(t *testing.T) { + extra := []string{"dangerous_write_fn", " audit_purge ", ""} + + for _, q := range []string{ + "SELECT dangerous_write_fn(1)", + "SELECT DANGEROUS_WRITE_FN (1)", + "SELECT audit_purge()", + } { + if err := ValidateSQL(q, false, extra); err == nil { + t.Errorf("ValidateSQL(%q) = nil, want extra-denylist rejection", q) + } + } + + // Non-listed functions still pass; extra list doesn't over-match. + for _, q := range []string{ + "SELECT count(*) FROM t", + "SELECT not_dangerous_write_fn2(1)", + "SELECT dangerous_write_fn_v2(1)", + } { + if err := ValidateSQL(q, false, extra); err != nil { + t.Errorf("ValidateSQL(%q) = %v, want nil", q, err) + } + } + + // ContainsDeniedFunction reports which function fired. + fn, hit := ContainsDeniedFunction("SELECT audit_purge()", extra) + if !hit || fn != "audit_purge" { + t.Errorf("ContainsDeniedFunction = (%q, %v), want (audit_purge, true)", fn, hit) + } +}