fix(cli): harden SQL argument handling - #110
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /** Globals that take no value, so the token after them is a positional, not their value. */ | ||
| const VALUELESS_GLOBAL_FLAGS = new Set(["debug", "d", "help", "h", "version", "v"]) | ||
|
|
||
| /** | ||
| * Index of the top-level command token, or -1. | ||
| * | ||
| * `args.find((a) => !a.startsWith("-"))` reads as "the first positional" and is | ||
| * wrong twice over: it returns a global's VALUE (`--schema sql task list` hands | ||
| * back "sql", `--schema tabel bogus` hands back "tabel"), and it returns an | ||
| * empty-string token, which forward.ts passes through verbatim by design. Only | ||
| * globals can precede the command, so KNOWN_GLOBAL_FLAGS is the complete set of | ||
| * value-takers to step over here. Mirrors run-cli.ts's subcommandIndex. | ||
| */ | ||
| function commandTokenIndex(args: string[]): number { | ||
| for (let index = 0; index < args.length; index++) { | ||
| const value = args[index] | ||
| if (!value) continue | ||
| if (value === "--") return -1 | ||
| if (!value.startsWith("-")) return index | ||
| const flag = value.replace(/^-+/, "").split("=")[0] | ||
| if (!flag || value.includes("=") || VALUELESS_GLOBAL_FLAGS.has(flag)) continue | ||
| if (KNOWN_GLOBAL_FLAGS.includes(flag)) index++ | ||
| } | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — third copy of this walk, and the flag table it reuses is the wrong one.
const VALUELESS_GLOBAL_FLAGS = new Set(["debug", "d", "help", "h", "version", "v"])
...
if (!flag || value.includes("=") || VALUELESS_GLOBAL_FLAGS.has(flag)) continue
if (KNOWN_GLOBAL_FLAGS.includes(flag)) index++run-cli.ts already has both halves of this:
packages/cz-cli/src/run-cli.ts:137—const GLOBAL_FLAGS = new Set(["debug", "d", "help", "h", "version", "v"]), character-for-character the same set as the newVALUELESS_GLOBAL_FLAGS.packages/cz-cli/src/run-cli.ts:138—GLOBAL_FLAGS_WITH_VALUES, the curated list of globals that consume a following token.packages/cz-cli/src/run-cli.ts:457-469— the identical scan (if (!value) continue/if (value === "--") break/ strip-+, split on=,index++for value-takers), plussubcommandIndexat:403as a second copy. The docstring here even says "Mirrors run-cli.ts's subcommandIndex".
So this is the third instance of the walk and the second copy of the valueless set, which is what KNOWN_GLOBAL_FLAGS's own comment at cli.ts:36-38 ("Kept here as the single source of truth to avoid drift") was trying to prevent.
The drift is already present, not hypothetical. KNOWN_GLOBAL_FLAGS is the did-you-mean candidate list, not a list of globals: it carries "target" and "t", which are declared as subcommand options (commands/update.ts:307, commands/task.ts:1574, :1736, :2391) and appear nowhere in GLOBAL_FLAGS_WITH_VALUES. Using it as the value-taker set means commandTokenIndex(["-t", "sql", ...]) consumes sql as -t's value and reports the wrong command token (or -1). The docstring's claim that "KNOWN_GLOBAL_FLAGS is the complete set of value-takers to step over here" is therefore inaccurate — it is a superset that includes two non-globals.
A second consequence of the same choice: any flag not in that list is assumed valueless, so a misspelled valued global before the command swallows the command position — cz-cli --formt json sql SELECT CASE … resolves the command token to "json", and looksLikeShellSplitSql returns false, silently losing the split advice in exactly the mixed typo+split case the handler at :410-415 argues it must not lose.
Smaller correct change: lift GLOBAL_FLAGS / GLOBAL_FLAGS_WITH_VALUES and the scan into one module both files import (they can't live in run-cli.ts since it imports createCli from here), and have commandTokenIndex use GLOBAL_FLAGS_WITH_VALUES rather than KNOWN_GLOBAL_FLAGS.
| suggestion ? `. Did you mean '${suggestion}'?` : "", | ||
| shellSplitSql | ||
| ? ". The SQL appears to have been split by the shell — quote the whole statement, or put it in a file and run `cz-cli sql -f /tmp/query.sql` (or `cz-cli sql --stdin < /tmp/query.sql`)." | ||
| : "", |
There was a problem hiding this comment.
LOW (confidence: high) — when both parts fire the human message gets ?. run together.
suggestion ? `. Did you mean '${suggestion}'?` : "",
shellSplitSql
? ". The SQL appears to have been split by the shell — quote the whole statement, ..."The new execute.test.ts case "reports a flag suggestion and the split advice together" exercises exactly this combination, so the emitted error.message is:
Unknown arguments: CASE, WHEN, …. Did you mean '--format'?. The SQL appears to have been split by the shell — …
?. reads as a typo to a human and the two sentences are the primary output surface for this failure. The test only asserts toContain on each half, so it passes either way. Dropping the leading . from the third element when the second one ended in ? (or joining the non-empty parts with " " after normalising terminal punctuation) fixes it.
| } else if ((cmdMatch?.[1] ?? argMatch?.[1] ?? "").includes(",")) { | ||
| // Several unknown tokens at once and nothing close enough to suggest: the | ||
| // usual cause is a quoted value (DDL, a SQL predicate, a JSON blob) that the | ||
| // shell tore apart before cz-cli saw it. Say so rather than only listing the | ||
| // debris — these commands are quote-sensitive and normal shell quoting and | ||
| // escaping rules apply to their arguments. Kept as a note on the existing | ||
| // message, not a second copy of cli.ts's detector. | ||
| finalAi = | ||
| `${finalAi} Arguments here are quote-sensitive and go through the shell first: if a quoted value was split into several tokens, re-quote it as one argument (escaping any inner quotes) or pass it through a file option instead of inline.` | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — three separate concerns with this branch.
} else if ((cmdMatch?.[1] ?? argMatch?.[1] ?? "").includes(",")) {1. No test covers it. rg "quote-sensitive" matches only these two source lines — nothing in packages/cz-cli/test. The tests added in this PR (execute.test.ts, parameter-hardening.test.ts) all go through cli.ts's top-level fail handler via the sql leaf command; none reaches a commandGroup. So the one branch in this file has no regression guard, while the cli.ts detector has nine cases.
2. The gating is the opposite of the decision made in cli.ts in this same PR. cli.ts:410-415 argues, in a comment, that suppressing the split advice when a suggestion exists "costs the caller a second round trip on the same split", and deliberately emits both. Here the else if chain suppresses the quote note whenever suggestion && (badSubcommand || badFlag) — i.e. cz-cli task save-cron --cron 0 30 9 * * ? * --nam x (typo + shell-split value) gets only the typo. Either the cli.ts reasoning applies here too, or it doesn't apply there; right now the two handlers disagree.
3. .includes(",") also fires on a single unknown token that merely contains a comma. cmdMatch[1] / argMatch[1] is the raw tail of yargs' message, so cz-cli schema "a,b" yields Unknown command: a,b → one token, comma inside it → the shell-splitting note is appended to a failure that had nothing to do with quoting. Counting tokens (.split(",").length > 1) rather than testing for the character avoids that.
Also worth noting: unlike cli.ts, this branch changes only finalAi, never displayMsg, so a human running the command sees no hint at all. If that asymmetry is intentional the comment should say so.
| // Both at once is contradictory input, and returning the positional silently threw | ||
| // the operands away: `cz-cli sql "SELECT 1 AS a" -- FROM t` submitted | ||
| // `SELECT 1 AS a` and exited 0, and `cz-cli sql SELECT -- 1 AS x FROM t` submitted | ||
| // bare `SELECT`. Either way the user got results for a statement they never wrote, | ||
| // with nothing on stdout to say so. `--` exists for a statement that STARTS with | ||
| // `-`, so it is the whole statement or none of it. | ||
| if (argv.statement && operands.length > 0) { |
There was a problem hiding this comment.
MEDIUM (confidence: high) — sql --help still documents the combination this now rejects.
if (argv.statement && operands.length > 0) {The command's own epilogue at packages/cz-cli/src/commands/sql.ts:871 reads:
"SQL input priority: positional > `--` operands > -e/--execute > -f/--file > --stdin",
A documented priority between "positional" and "-- operands" means both may be supplied and the positional wins — which is precisely the behavior this PR removes. After this change that line tells the user to do something that exits 2. The comment at :240-241 ("Priority is the one this command's own --help documents") makes the epilogue the stated source of truth, so it should be updated in the same commit — e.g. positional | -- operands > -e/--execute > -f/--file > --stdin, plus a note that the two positional forms are mutually exclusive.
(I checked for other callers of the old behavior: rg "operands" finds no test or doc that passes a positional and -- operands, and parameter-hardening.test.ts:263 / :708 only use the bare sql -- <statement> form, which is unaffected. So the epilogue looks like the only stale reference.)
| `Got a SQL positional ("${argv.statement}") and operands after '--' ("${ | ||
| operands.join(" ") | ||
| }"). Pass the statement once: quote it as a single argument, or put everything after '--'.`, |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — this message carries raw SQL text into telemetry, bypassing redactSql.
`Got a SQL positional ("${argv.statement}") and operands after '--' ("${
operands.join(" ")
}"). Pass the statement once: ...`,The path: handledError → error() → output/index.ts:135 sets process.lastError = message → run-cli.ts:865-867 passes it to track(..., lastError) → trackCommand({ ..., error }). Nothing on that path redacts.
That matters because the repo deliberately redacts the same text everywhere else it reaches telemetry: telemetry.ts:141 is args["_positional"] = redactSql(positional.slice(2).join(" ")), and logger.ts:69 is entry.sql = redactSql(opts.sql). The -- operands are exactly positional[2..] — the tokens redactSql is applied to on the _positional path — and they are now interpolated verbatim into the telemetry error field. cz-cli sql "CREATE USER u IDENTIFIED BY" -- 's3cret' is the shape that leaks, and it is a plausible way for a statement to end up split across -- in the first place.
Caveat so you can judge severity: run-cli.ts:855 already records subcommand: positional[1] un-redacted, so the positional half of this message adds no new exposure. The operand half does.
Smallest fix: wrap both interpolations in redactSql (already exported from src/logger.ts), or drop the echoed text and name the counts instead ("a SQL positional and 4 operands after --") — the caller can see their own argv, so the echo is for readability rather than diagnosis.
| // EXPLAIN returns a plan and executes nothing, so unwrap it before any other rule | ||
| // looks at the statement. This used to happen after the WRITE scan, which left | ||
| // `EXPLAIN SHOW CREATE VIEW v` demanding write approval — CREATE was still in the | ||
| // string WRITE read — while plain `SHOW CREATE VIEW v` was readonly. |
There was a problem hiding this comment.
LOW (confidence: high) — the example in this comment describes a pre-state that never existed.
// `EXPLAIN SHOW CREATE VIEW v` demanding write approval — CREATE was still in the
// string WRITE read — while plain `SHOW CREATE VIEW v` was readonly.Before this PR the strip was anchored on ^SHOW\s+CREATE\s+TABLE\b, so plain SHOW CREATE VIEW v did not match it, CREATE survived into inspected, and it classified as write — not readonly. The asymmetry the comment describes (wrapped demands approval, unwrapped does not) only exists once the prefix is broadened to ^SHOW\s+CREATE\b further down in this same diff.
SHOW CREATE TABLE t is the statement that actually had that asymmetry before the change, and it makes the rationale hold on its own rather than depending on another edit in the same commit. This file's comments are the documentation for the classifier, and the readonly.test.ts cases are organised around them, so it's worth keeping the example exact.
| // `--limit 0` means "no limit" (rowLimit Infinity), which by design also skips the | ||
| // LIMIT probe injection in sql.ts. So this loop asserts one thing: introspection | ||
| // whose text contains a write keyword clears the approval gate and returns rows. | ||
| // The default-limit path, where these same statements get ` LIMIT <n+1>` appended, | ||
| // is a separate concern and is not covered here. |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — please confirm the intent: the path this comment excludes is the one the default invocation takes.
// The default-limit path, where these same statements get ` LIMIT <n+1>` appended,
// is a separate concern and is not covered here.--limit defaults to 100 (commands/sql.ts:860), so cz-cli sql "SHOW CREATE VIEW v" — no flags — is the default-limit path, not the --limit 0 path these tests pin. Tracing it in commands/sql.ts:
isSelecttrue andisShowtrue (SHOW_RE = /^\s*SHOW\b/i,:15)hasLimitfalse (LIMIT_RE = /\bLIMIT\s+\d+/i)canRewritetrue — newly true, becauseanalysis.kindflipped fromwritetoreadonlyin this PR- so
:398fires and:403submitsSHOW CREATE VIEW v LIMIT 101
Pre-PR the same command needed --write, and --write does not change analysis.kind, so canRewrite stayed false and the statement went to the server verbatim. Post-PR it goes with LIMIT 101 appended. Recovery rests on the retry at :406, which needs the server's error text to match both /syntax/i and /LIMIT/i; if ClickZetta words it differently (e.g. "Parse error near 'LIMIT'", "mismatched input"), the user gets a hard failure on a statement that worked before via --write.
SHOW CREATE TABLE t was already readonly before this PR, so whatever the server does with the appended LIMIT there is already the status quo — that's the reason I think this is probably fine. But VIEW / MATERIALIZED VIEW / PIPE / SEMANTIC VIEW are newly on this path, and nothing in the PR exercises it: every case in this loop passes --limit 0 specifically to skip it.
Could you confirm SHOW CREATE <object> LIMIT 101 is accepted by the engine (or that its rejection matches the :406 heuristic)? If neither is certain, gating the injection on !isShow || !/^\s*SHOW\s+CREATE\b/i.test(text) is cheaper than depending on error-string matching.
Review summaryA. Upstream invasiveness — no issues found. All eight changed files are in B. Clean fix vs. hole drilled around it — mostly the right fix, two findings. The three substantive changes are each aimed at their cause rather than a symptom: the Findings:
Also flagged: raw SQL and operands into the telemetry C. Regression risk — enumerated below; one item raised as a separate question.
I have not run anything — the PR body's own note that full CLI typecheck is blocked by a pre-existing |
2 participants
Summary
SHOW CREATE ..., including wrappedEXPLAINintrospection--file/stdin recovery commands--operands instead of silently truncating itValidation
packages/clickzetta-sdk: readonly tests passed, typecheck passedpackages/cz-cli: SQL readonly, execute, and shell-split unit tests passedpackages/opencode/src/bus/global.ts:14TS2416 errorFixes CZECO-343
Fixes CZECO-344