Skip to content

fix(providers): every MySQL read went through the prepared protocol, and four more - #476

Merged
cevheri merged 1 commit into
mainfrom
chore/backlog-round7
Aug 24, 2026
Merged

cevheri merged 1 commit into
mainfrom
chore/backlog-round7

Conversation

@cevheri

@cevheri cevheri commented Aug 24, 2026

Copy link
Copy Markdown
Member

Round 7 of the BACKLOG sweep. Five entries closed, each measured against a live server before the change and re-measured in a browser on a production build after it. BACKLOG 113 -> 110 (five removed, two follow-ups recorded).

What was wrong, and what it does now

D8 - every MySQL statement went through the prepared protocol, including MySQL's own CHECK TABLE

All 33 reads in mysql.ts called mysql2's execute - the binary PREPARED protocol - parameterless statements included. Three engines refuse whole statement classes there with This command is not supported in the prepared statement protocol yet, and one of the three is MySQL itself: CHECK TABLE - the exact text runMaintenance("check") builds - fails prepared on MySQL 26.7.0 and succeeds as text. So a maintenance action was unavailable on the engine this provider is named for, and this was never only a compatibility item.

One helper now owns every call site and picks by one fact: a statement carrying parameters keeps execute (binding is what keeps a value out of the SQL text); everything else takes the text protocol.

Why moving the user's own query path is safe, measured rather than assumed. One connection, one probe table, the same SELECT * both ways, 23 columns:

execute query
TINYINT(1), INT, YEAR, FLOAT, DOUBLE number identical
BIGINT 9007199254740993 9007199254740992 identical (both lose the same digit)
DECIMAL(20,4) string "12345678901234.5678" identical
DATE / DATETIME / TIMESTAMP Date identical
BLOB, BIT(1), BIT(8) Buffer identical
JSON, ENUM, SET, NULLs object / string / null identical

Every value identical by typeof and by JSON.stringify; every FieldPacket identical in columnType, flags, characterSet, columnLength, decimals, so columnTypes names the same types; a non-result-set statement answers the same ResultSetHeader, which is what buildQueryResult reads.

What it recovers, in the browser on the built app. SingleStore 9.1.1: the header badge reads Online instead of Slow, the fleet card reads 152ms / 4 conn, and Monitoring renders MySQL 5.7.32, an uptime, Connections 7/100000, Tables 2 / 2 indexes where the whole dashboard used to fail. Storage renders too - all zeros, which is this engine's own zeroed information_schema.TABLES and not a failure of ours. Optimize and Check recover, measured through the provider. The routing is visible from the server's side in the app's own session list: SHOW STATUS LIKE 'Uptime' shows state Query while parameterised reads show Execute.

Two of D8's own claims were refuted by re-measuring rather than carried forward. EXPLAIN FORMAT=JSON is ER_PARSE_ERROR on SingleStore on both protocols - its grammar is EXPLAIN JSON, so the Explain panel is a statement problem wearing a protocol problem's message (recorded as X14). And StarRocks health does not recover: it fails on a missing information_schema.PROCESSLIST, over a parameterised read this change does not touch.

D17 - an unmeasured connection count read 0, and the model was told the same

DatabaseOverview.activeConnections was a required number, so a provider that cannot measure it answered 0 and the card drew "0 connections" for a server with sessions open. ScyllaDB has no system_views keyspace to count from; a Cassandra role denied that grant has answered a fabricated 0 since the provider shipped.

Both halves are optional now - HealthInfo.activeConnections too, because that is the field the agent's curated health reading forwards to the LLM, and telling a model "0 connections" about a server it cannot measure is the same fabrication one layer down. The provider omits the key when the read degraded; the card says not published; the trend chart drops the sample rather than plotting a zero; the fleet card drops its connection chip; the agent sees null. maxConnections is deliberately unchanged - there 0 already MEANS "no limit published", a different fact.

Browser-verified, both arms in one pass:

Connections card Fleet card
ScyllaDB 2026.2.4 N/A · not published 170ms · N/A (no conn chip)
Apache Cassandra 5.0.9 1 · no limit published 238ms · N/A · 1 conn

X13 - every Cassandra CREATE TABLE the export wrote was rejected

CQL requires exactly one PRIMARY KEY and the DDL export wrote a column list and nothing else, so its Cassandra output could not run at all - the type names became right on 2026-08-24 and the file still needed a hand edit. It now appends PRIMARY KEY (<first column>) plus a comment saying that CQL requires a key, that a result set does not know the real one, and that the reader must confirm the choice is unique per row. A wrong-but-loud key beats a file that cannot parse; the identifier goes through the same quoteIdentifier as the column list and never into the comment prose, so a name carrying -- cannot break out of it.

Verified end to end: exported from the results grid in the browser, and that exact file replayed into live Cassandra 5.0.9 -

-- CQL requires exactly one PRIMARY KEY per table, and a result set carries no key of
-- its own. This export chose the first column as a placeholder: confirm it is unique
-- per row before running this statement.
CREATE TABLE table_name (
  "id" bigint, "name" varchar, "spend" double, "joined" timestamp, "active" boolean,
  PRIMARY KEY ("id")
);

DESCRIBE TABLE afterwards: id bigint PRIMARY KEY and the other four columns.

B57 - the agent read a binary cell as its wire JSON

renderRows stringified rows with a plain JSON.stringify, so a bytea/BLOB/blob value reached the model as {"type":"Buffer","data":[1,2,171]} while every other surface has shown \x0102ab since 2026-08-23. It now reads the same shared module the grid, the row sheet and the CSV read. Measured on a 160-byte value: 556 characters before, 322 after. The model gets the FULL hex, not the grid's 32-byte compact-cell preview - that cutoff exists for a one-line cell, and a model comparing two values needs the whole one.

Follow-ups recorded, not smuggled in

  • D22 - getMonitoringData composes its four core reads with Promise.all, so StarRocks' one missing table (information_schema.PROCESSLIST, the active-session read) discards the overview, the performance panel and the slow-query list that DID answer. Measured in the browser: the page renders nothing but the engine's sentence. Same shape as the health gate fixed on 2026-08-24 - an aggregate that fails whole rather than in parts.
  • X14 - SingleStore's Explain needs EXPLAIN JSON, which one strategy per format cannot express without engine sniffing the provider rules forbid; the shape that fits is a capability the connection declares.

Verification

Gate Result
bun run format clean, 924 files
bun run lint 0 errors
bun run typecheck clean
bun run knip clean
bun run test (run-core.sh + components) 336 core files + 33 groups, 0 fail
bun run coverage:check 100.00%, 41554/41554 lines
bun run build exit 0
bun run build:lib + attw exit 0, node16/bundler green
gitleaks, branch range 1 commits scanned, no leaks

Provider tri-sync held for mysql and cassandra: code, docs/providers/<id>.md and the integration test move together. docs/providers/README.md's StarRocks, SingleStore and ScyllaDB rows were rewritten from what was measured here, not from what was expected. Every citation of the four closed IDs across src/, tests/ and docs/ was rewritten to a date or a description, so nothing points at an entry that no longer exists.

One live-environment note for whoever reproduces this: SQL Server and SingleStore both need fs.aio-max-nr headroom, and a running ScyllaDB holds roughly 55k of this host's 65536 aio contexts.

…and four more

Five BACKLOG entries, each measured against a live server before the change and
re-measured in a browser on the built app after it.

D8 - every statement the MySQL provider issued went through mysql2's `execute`,
the binary PREPARED protocol, parameterless statements included. Three engines
refuse whole statement classes there with `This command is not supported in the
prepared statement protocol yet`, and one of the three is MySQL itself: `CHECK
TABLE` - the exact text `runMaintenance("check")` builds - fails prepared on
MySQL 26.7.0 and succeeds as text, so one maintenance action was unavailable on
the engine this provider is named for. One helper now owns all 33 call sites and
picks by one fact: a statement with parameters keeps `execute`, everything else
goes over the text protocol. Safe because the two protocols decode to the same
JS shapes - measured on one connection across 23 columns (TINYINT(1), INT,
BIGINT past 2^53, DECIMAL, FLOAT, DOUBLE, DATE, DATETIME, TIMESTAMP, TIME, YEAR,
CHAR, VARCHAR, TEXT, BLOB, BIT(1), BIT(8), JSON, ENUM, SET, NULLs): every value
identical by typeof and by JSON.stringify, every FieldPacket identical in
columnType, flags, characterSet, columnLength and decimals, and a non-result-set
statement answers the same ResultSetHeader.

What that recovers, verified in the browser: SingleStore 9.1.1's header badge
reads Online instead of Slow, its fleet card reads `152ms / 4 conn`, and its
Monitoring dashboard renders `MySQL 5.7.32`, an uptime, `Connections 7/100000`
and `Tables 2 / 2 indexes` where the whole panel used to fail. Optimize and
Check recover too, measured through the provider. StarRocks recovers at the
provider layer only, and the reason is a second defect this exposed (D22 below).

Two of D8's own claims were refuted by re-measuring rather than carried forward.
`EXPLAIN FORMAT=JSON` is `ER_PARSE_ERROR` on SingleStore on BOTH protocols -
its grammar is `EXPLAIN JSON`, so the Explain panel is a statement problem
wearing a protocol problem's message, recorded as X14. And StarRocks health does
not recover: it fails on a missing `information_schema.PROCESSLIST`, the
engine's own gap, over a parameterised read the change does not touch.

D17 - `DatabaseOverview.activeConnections` was a required number, so a provider
that cannot measure it answered 0 and the card drew "0 connections" for a server
with sessions open. Two real cases: ScyllaDB, whose count lives in the
`system_views` keyspace it does not have, and a Cassandra role denied the same
grant, which has answered a fabricated 0 since the provider shipped. Both halves
are now optional - `HealthInfo.activeConnections` too, because that is the field
the agent's curated health reading forwards to the model, and telling an LLM "0
connections" about a server it cannot measure is the same fabrication one layer
down. The Cassandra provider omits the key when the read degraded; the card says
"not published"; the trend chart drops the sample instead of plotting a zero; the
fleet card drops its connection chip; the agent sees `null`. `maxConnections` is
deliberately unchanged - there `0` already MEANS "no limit published".

Verified in the browser, both arms in one pass: ScyllaDB reads `N/A / not
published` while Apache Cassandra 5.0.9 reads `1 / no limit published`.

X13 - every Cassandra `CREATE TABLE` the DDL export wrote was rejected outright:
CQL requires exactly one PRIMARY KEY, and the export wrote a column list and
nothing else. It now appends `PRIMARY KEY (<first column>)` and a comment saying
that CQL requires a key, that a result set does not know the real one, and that
the reader must confirm the choice is unique per row. A wrong-but-loud key beats
a file that cannot parse, and the identifier goes through the same
`quoteIdentifier` as the column list, never into the comment prose. Verified by
exporting from the results grid in the browser and replaying that exact file into
live Cassandra 5.0.9: `id bigint PRIMARY KEY` and four columns.

B57 - `renderRows` put a binary cell into the model's prompt with a plain
`JSON.stringify`, so a bytea/BLOB/blob value reached the LLM as
`{"type":"Buffer","data":[1,2,171]}` where every other surface shows `\x0102ab`.
It now reads the same shared module the grid, the row sheet and the CSV read.
Measured on a 160-byte value: 556 characters before, 322 after. The model gets
the full hex, not the grid's 32-byte compact-cell preview - that cutoff exists
for a one-line cell, and a model comparing two values needs the whole one.

Two further defects found while closing these, recorded rather than smuggled in:
D22 - `getMonitoringData` composes its four core reads with `Promise.all`, so
StarRocks' one missing table discards the overview, performance and slow-query
panels that did answer; and X14, the SingleStore Explain grammar above.

All six gates green on the final tree, plus `build:lib` + `attw`, with coverage
at 100.00% (41554/41554 lines).
Comment thread src/lib/db/providers/sql/mysql.ts Dismissed
Comment thread src/lib/db/providers/sql/mysql.ts Dismissed
@sonarqubecloud

Copy link
Copy Markdown

@cevheri

cevheri commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

CodeQL is red on this PR, and it is the dismissal anchor moving, not a new sink.

The two alerts are js/sql-injection (high) at src/lib/db/providers/sql/mysql.ts:105-106 — the two lines of the new runStatement helper, queryable.query(sql) and queryable.execute(sql, params).

The same rule, on the same file, at the line this PR replaced (mysql.ts:540, the editor's query path) is already dismissed as "won't fix — libredb-studio is sql editor", alongside about twenty more of the same rule across mysql.ts, postgres.ts, mssql.ts and mongodb.ts, all triaged under #100. Nothing about the taint changed: the SQL text was user-authored and executed before this PR and is user-authored and executed after it. What changed is the LOCATION — 33 call sites collapsed into one helper — so the existing dismissals no longer anchor and CodeQL reports the funnel as new. Two alerts to dismiss instead of the previous scattered set is the durable side of that.

Suggested dismissal reason, matching the existing entries' wording:

By design: Studio is a SQL IDE. This helper executes the user's own statement against their own configured connection, choosing mysql2's text protocol for a parameterless statement and the binary prepared protocol (with bound parameters) for a parameterised one. Triage: #100.

One genuine behavioural delta worth naming while dismissing, because it is not a taint change and is documented in docs/providers/mysql.md §3.4 rather than left implicit: a connection that opted into multipleStatements=true in its OWN pasted connection string can now run a ;-separated statement, which the prepared protocol used to refuse. That is the user's own SQL on their own connection, the app already has a dedicated multi-statement route, and CALL <proc>() already returns the same multi-result shape on both protocols — so nothing structurally new reaches buildQueryResult. It is also not an agent surface: the agent's SQL path requires a database-native read-only session, which MySQL does not offer, so agent runs on MySQL never reach this code.

CodeQL is not one of the three required checks; every required check plus the six local gates and 100% coverage are green.

@cevheri
cevheri merged commit 24dcc64 into main Aug 24, 2026
19 checks passed
@cevheri
cevheri deleted the chore/backlog-round7 branch August 24, 2026 11:38
cevheri added a commit that referenced this pull request Aug 24, 2026
…tuning

Brings the twelve commits `main` had that this branch did not, so the chart version
question can be settled here rather than left to whoever rebases.

Five files conflicted, and `src/lib/agent/tools.ts` was not one of them: main's #476
and this branch's changes to it merged cleanly.

CHART VERSION. `version: 0.1.48` — ours, because 0.1.47 is main's and is tagged.
`appVersion: "0.13.3"` — main's, and not a preference: the merge brings
`package.json` 0.13.3 and the required check holds the two equal. `images` had already
auto-merged to the 0.13.3 tag.

CHART CHANGELOG. Ours. ArtifactHub reads `artifacthub.io/changes` for the version
being published, and 0.1.47's entries are frozen in the chart that shipped them, so
0.1.48's list describes what 0.1.48 adds and nothing else.

`containsSecurityUpdates` stays FALSE and its note is rewritten rather than kept from
either side. Both sides' wording would have been wrong here: ours said "appVersion does
not move", which reads as false against a diff taken from this PR's base, and main's
described 0.13.3's transport fixes, which 0.1.47 already published with the flag set.
The note now says what 0.1.48 ADDS and points at 0.1.47 for the rest, which is true
whichever diff a reader is holding.

B-NUMBERS ARE UNCHANGED. B57-B63 stand: `main` has no B57 — its series also ends at
B56 — so the renumbering this merge was expected to force is not needed.

The operator's chart copy is regenerated with `bun run chart:bump` rather than resolved
by hand, and `chart:check` reports 0.1.48 / 0.13.3 in sync with package.json 0.13.3.
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.

2 participants