Skip to content

v2.1.0 - The gate asked the wrong question, and one route never reached it

Choose a tag to compare

@CarlLee1983 CarlLee1983 released this 16 Aug 07:35
· 405 commits to main since this release
c2693f6

2.0.0 put a two-tier gate in front of raw SQL. This release is what measuring that gate against real servers found: one entire route into the database bypassed it, its qualification criterion asked whether a WHERE existed rather than whether it narrowed anything, and its notion of "what kind of statement is this" was the leading keyword — so a full-table delete wearing an INSERT or WITH in front of it was treated as routine. Each of the three is a way a statement that empties a table reached the database unattended, and each is closed here. The measurement the gate's own ADR bets on is now a command rather than a jq invocation somebody has to remember.

Changed

  • The gate's qualification criterion now requires positive evidence that the WHERE narrows the table being written. It was statement.where != null, which asks whether a WHERE exists; the question that matters is whether this WHERE restricts the target. UPDATE p SET c = (SELECT … WHERE …) has one and rewrites every row. A WHERE must now reference a column of the table being written — a correlated reference back to the target counts, unless the name is bound by the subquery itself, since DELETE FROM sessions WHERE EXISTS (SELECT 1 FROM sessions WHERE …) speaks about the subquery's own rows and was measured emptying the table. This is a lower bound, not a proof: WHERE id IS NOT NULL names the target and touches every row. WHERE 1=1 remains the supported escape hatch (#80, #93).

  • A multi-table write is tier two regardless of its WHERE, under the new reason multi_table. DELETE p FROM p JOIN o ON p.id = o.ref WHERE o.x > 0 deleted 2 of 5 rows on one dataset and all 2000 on another — the same statement, a different answer, so scope is not decidable from the text. A join's ON always mentions the target table, so treating it as evidence would readmit the entire class 2.0.0 exists to stop, UPDATE p SET … FROM o WHERE p.id = o.ref included. Four criteria were tried and each was broken by the next ordinary statement, so the tier is now decided by what the statement is — which is known — rather than by what it will touch, which is not. The remedy in the refusal says "rewrite as a single-table statement or have somebody confirm it", not "add a WHERE" (#80).

  • When the parser cannot read a statement, tier one is now an allowlist rather than a denylist. Enumerating the keywords that introduce a second table lost one round of adversarial review each: USING, JOIN, CTEs, subqueries, TABLE as a subquery. A statement now has to read as "writes one named table, has a WHERE or LIMIT" and contain no SELECT / TABLE / VALUES / JOIN / USING, no statement-level WITH, and no UPDATE … FROM to qualify. Keywords inside parentheses are not statement-level, so SUBSTRING(x FROM 2) and AGAINST ('a' WITH QUERY EXPANSION) are not misread (#80).

  • Statement type is decided by shape, not by the leading keyword, so a write hidden in front of one no longer buys tier-one treatment. Measured on PostgreSQL 16 against a 2000-row table: WITH moved AS (DELETE FROM p RETURNING *) INSERT INTO archive … deleted 2000 rows and MERGE INTO p … WHEN MATCHED THEN DELETE deleted 2000 — both tier one, both skipped by --yes. The same CTE under a CREATE TABLE … AS head and MERGE … WHEN MATCHED THEN UPDATE behave the same way. The criterion is now a nested write anywhere inside parentheses, which holds because a parenthesised expression that is not the statement body cannot contain a write: INSERT / UPDATE / DELETE are reserved words in both dialects, so a column named that way must be quoted, and quoting is stripped before the read. MERGE is the exception, so only MERGE INTO is matched. Locking clauses (SELECT … FOR UPDATE) and foreign-key referential actions (ON DELETE / ON UPDATE) are stripped first — the latter after a review round found ON DELETE CASCADE blocking every table creation with a foreign key, while the same constraint added via ALTER TABLE was allowed, one meaning with two answers. A MERGE is classified by its WHEN … THEN actions: THEN DELETE / THEN UPDATE are tier-two multi_table because MERGE reads its rows from USING, while a pure-insert or DO NOTHING merge stays tier one, which is what keeps ordinary upserts working (#94, #95).

  • Every gate decision is audited as db-write, so filtering the audit log by tier no longer misses two thirds of them. side_effect_tier was read from the command's capability table, so the same DROP TABLE users decision was recorded readonly when it arrived through query, db-write through delete, and interactive through shell — and tier is the first filter any audit consumer reaches for. AuditOutcome gained sideEffectTier so a caller that knows more than the capability table can state what the statement actually does; the type deliberately accepts only db-write / local-write, because this opening exists to be more accurate, not to downgrade a write. --dry-run / --plan still win, being an explicit execution mode rather than an outcome. The capability table is unchanged — interactive remains a correct description of the shell command itself (#83).

Automation affected

Three shapes that ran unattended before now exit 1, all of them full-table writes that 2.0.0 intended to stop and mis-tiered:

Invocation Now Remedy
UPDATE p SET c = (SELECT … WHERE …) — WHERE only inside a subquery reason=no_where add a WHERE on the target table, or WHERE 1=1
DELETE p FROM p JOIN o … / UPDATE p SET … FROM o … reason=multi_table rewrite as a single-table statement, or run it where a person can confirm
data-modifying CTEs and MERGE … THEN DELETE/UPDATE reason=multi_table as above; pure-insert MERGE upserts are unaffected

Added

  • dbcli shell now applies the tier-two gate, the last SQL route that bypassed it. The REPL called the adapter directly with only permission and blacklist checks in the way, so DELETE FROM users demanded a typed table name under dbcli query and asked nothing at all one prompt over — protection that depended on which entrance the user picked, which is the thing #70 set out to remove. Only tier two is wired in: every line in a shell is hand-typed, so a per-statement y/N would become reflex, and tier one exists for batches of routine writes. A refusal prints to stderr and returns to the prompt with the session, connection and buffer intact; piped input (dbcli shell < script.sql) has nobody to answer, so that statement is refused and the rest of the file runs. Every tier-two evaluation is audited. Core still writes to neither stdout nor stderr (ADR 0009): ReplEngine takes a ReplWriteGate callback and src/commands/shell.ts supplies it, asking through the REPL's own readline interface — inquirer opens a second reader and was measured consuming each keypress twice. Line handling is queued, because readline emits every line of a paste before the first await returns (#78).

  • dbcli audit write-gate turns the measurement ADR 0010 bets on into one command. The ADR does not justify tier two by argument; it bets that in six months the records will say whether it stopped anything. The data was being written all along (write_gate_tier / write_gate_outcome / write_gate_reason) but reading it meant hand-writing jq over .dbcli/audit/*.jsonl — a measurement nobody performs is a falsification condition that never fires. The summary answers three questions: how often tier two was reached, grouped by reason; how many were allowed, cancelled and refused; and over what span those numbers were measured. Zero is reported as a conclusion rather than an empty table — it is evidence the criterion is wrong, not that the gate was unnecessary — while an empty audit log is stated as yielding no conclusion at all. Reasons that never fired stay on the table showing 0, since an omitted row can answer "how often" but not "at all". Tier one is counted separately with its outcome distribution rather than a hardcoded label. Counting is a pure core function; reading files and formatting stay in the command layer. A compile-time guard in src/commands/audit.ts fails in both directions if the reason or outcome list drifts from the gate's own (#79).

  • \ calls a subcommand from inside dbcli shell, and SQL keywords win the name collision. Typing delete users --where status=active was classified as SQL on its leading keyword, and with no semicolon it entered multiline mode silently — every subsequent line, .quit included, was swallowed into the buffer, leaving a shell that could only be killed from outside. A shell is for typing SQL, so DELETE FROM users WHERE … must stay typeable; subcommands moved to their own namespace instead. \ is separate from the existing meta . — the latter is implemented by the shell, the former runs a dbcli subcommand in its own process — and it works for every subcommand, so what has to be remembered is a rule rather than a list of the four that actually collide (insert, update, delete, explain). The prefix is checked before the SQL rule, so a trailing ; typed out of muscle memory does not turn it back into SQL. When a line is classified as SQL but starts with a subcommand name and carries a double-dash option, one line says which prefix would have reached the subcommand — unless the second word is a keyword like FROM / SET / INTO, since -- also opens a SQL comment (#88).

Fixed

  • Multiline mode no longer hijacks the shell, and Ctrl-C now cancels what it says it cancels. While a statement is buffering, the shell's own meta commands are handled instead of being appended, and .quit / .clear also discard the buffer — but not inside an unterminated string literal, where the line is text rather than a command and lifting it out would split the statement; MultilineBuffer is the only place that knows the quoting state, so it answers isInsideLiteral(). Separately, pressing Ctrl-C at the gate's "type the table name" question invoked the SIGINT listener while readline's question stayed mounted, so the next line typed was consumed as the answer. Each question now gets an AbortController; SIGINT aborts it and resolves null, which prints "cancelled" rather than "input did not match X" — the latter describes an answer that was never given. The audit still records declined, because a decision was made. And cancelling a multiline statement only ever printed a message: MultilineBuffer.reset() existed with no caller in the repository, so the next statement was appended to the abandoned half and came back as a syntax error pointing at a keyword the user never typed (#85, #88).

  • A subcommand refused inside dbcli shell said something untrue, and before that it could not run at all. Bun.spawn gives a child no stdin by default, so every subcommand was judged unattended: tier two was refused with "nobody can confirm this right now, run it in an interactive terminal", said to a person sitting at one. The child now carries DBCLI_SHELL_SUBCOMMAND=1 and the refusal states the actual situation and offers a route that works — type the statement at the dbcli> prompt, where dbcli will ask for the table name. code and reason are untouched, since agents branch on those; only the prose for humans changed. Verifying it surfaced two defects further upstream that made the situation unreachable: --config was placed after the subcommand, but it is a program-level option that query / insert / update / delete do not declare, so every write subcommand ended in unknown option; and the tokenizer left matched quotes inside tokens, which go straight into Bun.spawn's argv with no shell to strip them, so query "DELETE FROM users" arrived as one quoted identifier (#84).

  • dbcli q now enforces permission itself instead of relying on another module to do it. q.ts never called enforcePermission and the adapter does not check either; nothing broke today only because saved-queries/parser.ts rejects snippets at load time that are not SELECT / WITH, contain write keywords, or hold multiple statements. A property held jointly by two modules, one of which does not know it is holding it, comes apart the first time somebody reasonably asks for snippets that write. This is defence in depth rather than a bug fix — under the current contract every executable snippet still passes. The check reads prepared.rewrittenSql rather than the driver SQL, which is a size-guard wrapper and would describe dbcli's wrapping instead of the requested statement, and it runs before the --dry-run branch, so a refusal is not worded differently for a statement that was not going to execute anyway. checkTablesBlacklist now receives the real statement type instead of a hardcoded 'SELECT' (#81).

  • 518 test files had never been type-checked, and now 519 are. tsconfig.json matched them with "tests/**/*.{ts,tsx}", and TypeScript's include globs support *, ? and **/ but do not expand braces — tsc --listFiles | grep -c "/tests/" returned 0. Every type-level assertion written in a test file was decoration: a compile-time guard was found still passing after a union member was removed from under it. The pattern was replaced with tsconfig.tests.json and typecheck:tests, initially covering only the directories that were clean and ratcheting outward over five batches (339 errors across 174 files, now zero), and typecheck:tests is in CI and the release checklist. Roughly six in ten were unchecked indexed access, but the pass also found tests that were wrong: an import from a module that does not exist (Bun strips type-only imports without resolving them), buildSchemaContext() called with no argument, a makeEntry helper whose ...overrides silently overwrote the two fields written above it, spies typed as bare ReturnType<typeof spyOn> and thereby erasing 13 parameters into any, mock adapters missing four methods QueryableAdapter requires, and permission: 'read-only' — not one of the four legal values — standing in for "insufficient permission" and reaching the right verdict for the wrong reason. tests/helpers/test-config.ts was producing a value that was not a valid DbcliConfig at all, its Partial overrides spread after the required fields turning them all optional (#93, #97).

  • The inspect leak check no longer fails at random. It asserted expect(stdout).not.toContain('5432') over the whole serialized output; --no-connect emits no port field, but an audit entry's UUID ended in 5432 and reddened CI once. Three UUIDs of 32 hex characters each puts this at roughly one run in 700 — rare enough not to look like a real problem, frequent enough to burn a CI run and an investigation now and then. The check now walks the parsed structure: a value must be the port, a string must contain the host, and credential field names count wherever they appear (the fixture password is the single character p, so value matching cannot help there). The connection object's key set is pinned to name / database / version, which is stricter than before — any added field fails — where the old check exploded on the substring "host" appearing anywhere (#92).


Install: npm install -g @carllee1983/dbcli@2.1.0

Full diff: v2.0.0...v2.1.0