sql(mysql): validate response sequence ids to prevent cross-query result delivery - #34048
sql(mysql): validate response sequence ids to prevent cross-query result delivery#34048robobun wants to merge 4 commits into
Conversation
…ult delivery process_packets tracked the incoming sequence id but never validated it, and the connection's expected sequence id was never reset when a new command (seq 0) was written. Residual bytes buffered after a completed command's terminator were routed to the next queued query, which resolved with rows the server never produced for it. Validate header.sequence_id against the expected value in the Connected state and fail the connection (ERR_MYSQL_PACKETS_OUT_OF_ORDER) on mismatch, matching libmysql's CR_NET_PACKETS_OUT_OF_ORDER. Reset the expected id to 1 at every ready-for-next-command transition (auth OK, result-set terminator, ERR packet, prepared-statement completion).
|
Updated 11:05 AM PT - Jul 12th, 2026
❌ @robobun, your commit b0d8643 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34048That installs a local version of the PR into your bun-34048 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not linking #32005: that one is client-side write ordering ( This PR addresses the server-triggered case: residual/late bytes carrying continuation sequence ids after a command's terminator. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesMySQL protocol handling now validates packet sequence IDs, resets sequence state between command exchanges, maps packet-order failures to a JavaScript error code, and adds TCP-level regression tests for desynchronized, correctly sequenced, and sequence-wrapping queued queries. MySQL sequence validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sql_jsc/mysql/MySQLConnection.rs`:
- Around line 592-601: Extend the MySQL connection test coverage around the
terminal paths at the cached failure, prepared completion, prepared error, and
multi-result completion branches to queue a following command and inject stale
or mismatched packet sequences. Include the 255-to-0 command-transition
wraparound case validated by the sequence check in the connection packet
handling logic, and assert each scenario returns the expected packet-order error
without misrouting the queued command.
In `@test/js/sql/sql-mysql-sequence-desync.test.ts`:
- Around line 43-49: Condense the later narrative comment block describing
concurrent queries, the max:1 pool, and ghost result packets to no more than
three lines, preserving only the protocol invariant that sequence-id validation
must prevent unsolicited trailing packets from being routed to query B. Leave
the accepted opening fault-injection banner unchanged and apply the same cleanup
to the additional block around the referenced location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4226ec85-5b2b-40da-8f11-ec6186dbdf89
📒 Files selected for processing (4)
src/sql/mysql/protocol/AnyMySQLError.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/protocol/any_mysql_error_jsc.rstest/js/sql/sql-mysql-sequence-desync.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes the MySQL wire-protocol state machine — a strict sequence-id check plus six distributed sequence_id = 1 reset points across auth-OK, result-set OK/ERR, prepared-statement done/ERR, and cached-failure paths. A missed or misplaced reset would reject every well-formed response on that connection, so this warrants a human look.
Checked: the check is gated on status == Connected, so the auth phase (which continues the handshake sequence) is exempt; all command writes go through writer.start(0) so expecting seq 1 on the first response is correct.
Checked: is_last_result == false (SERVER_MORE_RESULTS_EXISTS) skips the reset so multi-result sequences continue uninterrupted.
Checked: legacy-EOF prepared-statement completion — the intermediate EOF calls check_if_prepared_statement_is_done but only resets when both counts are satisfied; the u8 wrap uses wrapping_add on both sides.
Extended reasoning...
Overview
Adds strict validation of the MySQL packet sequence_id in process_packets (only when status == Connected), a new PacketsOutOfOrder error variant with a JS error-code mapping, and self.sequence_id = 1 resets at every ready-for-next-command transition in MySQLConnection.rs. A new fault-injection test file drives a mock TCP server through four scenarios (ghost after OK, ghost after ERR, well-formed baseline, 300-row 255→0 wrap).
Security risks
The bug being fixed is itself a data-integrity/security issue (server-injected trailing bytes delivered to the wrong query). The fix tightens validation and fails closed. No new attack surface is introduced; the only regression risk is over-rejection of legitimate traffic.
Level of scrutiny
High. This is the core packet-dispatch loop of a production database driver. The correctness of the fix depends on every command-terminal path resetting sequence_id before the next response arrives — six separate sites across simple queries, prepared statements (both CLIENT_DEPRECATE_EOF and legacy-EOF modes), error packets, and cached-failure replay. If any real-server flow reaches Connected with a response whose first packet isn't seq 1 (or whose terminal path wasn't reset), every subsequent query on that connection fails. The mock-server tests cover COM_QUERY paths well; the prepared-statement and legacy-EOF resets rely on the container-gated sql-mysql.helpers.test.ts / sql-mysql.transactions.test.ts suites, which the author reports pass against MariaDB 11.8.
Other factors
I traced the reset placement against each terminal branch and against the write side (writer.start(0) in MySQLQuery.rs / MySQLRequest.rs / Query.rs) and it looks consistent. The check correctly excludes the auth phase, and wrapping_add(1) on a u8 matches the wire's 255→0 wrap. CodeRabbit's two threads are resolved. Still, the interaction surface (re-entrant JS in on_query_result/on_error_packet, legacy vs deprecate-EOF, multi-result continuation) is subtle enough that a maintainer familiar with the MySQL driver should confirm no terminal path was missed.
There was a problem hiding this comment.
I didn't find any bugs, but this adds a validation gate to the MySQL packet-processing loop and six sequence_id = 1 resets across command-transition sites — a missed reset would spuriously fail legitimate traffic with ERR_MYSQL_PACKETS_OUT_OF_ORDER, so it's worth a human pass on completeness.
What was reviewed:
- All command-phase writes go through
writer.start(0)(MySQLRequest.rs, MySQLQuery.rs, Query.rs), so expecting seq 1 after each reset is sound. - Validation is gated on
ConnectionState::Connected, so auth/TLS-handshake sequence progressions are untouched. - Multi-result:
is_last_result=falseskips the reset, soSERVER_MORE_RESULTS_EXISTScontinues the sequence; 255→0 wrap is handled bywrapping_addand covered by the 300-row test.
Extended reasoning...
Overview
This PR adds MySQL protocol sequence-id validation to prevent residual bytes from one command exchange being routed to the next queued query. It touches:
src/sql/mysql/protocol/AnyMySQLError.rs— newPacketsOutOfOrdervariant (1 line)src/sql_jsc/mysql/protocol/any_mysql_error_jsc.rs— maps the variant toERR_MYSQL_PACKETS_OUT_OF_ORDER(1 line)src/sql_jsc/mysql/MySQLConnection.rs— a validation check in theprocess_packetsloop plussequence_id = 1resets at six ready-for-next-command transitions (auth OK, cached-failure replay, prepared-statement done, prepared-statement ERR, result-set terminator whenis_last_result, result-set ERR)test/js/sql/sql-mysql-sequence-desync.test.ts— 280-line new file with a mock TCP MySQL server, four tests (two desync fault-injection cases, a sequential baseline, and a 255→0 wrap baseline)
Security risks
The bug being fixed is itself a data-integrity/security issue: a hostile or desynced server/proxy could cause query B to resolve with rows meant for (or forged after) query A. The fix fails closed — the connection is torn down on mismatch, matching libmysql's CR_NET_PACKETS_OUT_OF_ORDER. No new attack surface is introduced; the risk is the inverse — an incomplete set of reset points would cause false-positive rejections on legitimate traffic.
Level of scrutiny
This is a protocol state-machine change in the MySQL driver's packet-processing hot path. The correctness of the fix depends on the six reset points covering every path where the connection becomes ready for the next seq-0 command. I traced the command-write sites (MySQLRequest.rs:17,30, MySQLQuery.rs:266, Query.rs:32,97) and confirmed they all use writer.start(0), and the validation is correctly scoped to ConnectionState::Connected so auth-phase packets (which use self.sequence_id for continuation writes at lines 854/866/1097) are exempt. But enumerating that no transition was missed — e.g. any future or less-common command path — is exactly the kind of completeness audit a human maintainer of this subsystem should sign off on.
Other factors
- The PR description reports end-to-end verification against real MariaDB 11.8 (concurrent simple queries, prepared queries, 1000-row wrap, multi-statement
SERVER_MORE_RESULTS_EXISTS, error-then-success) and clean runs ofsql-mysql.helpers.test.tsandsql-mysql.transactions.test.ts. - The gate evidence shows the new tests fail on main (B resolves with
[{g:"GHOST"}]) and pass on the PR build in both debug-ASAN and release. - CodeRabbit's two comments (narrative-comment length, 255→0 wrap coverage) were addressed in 647a62c and both threads are resolved.
- No CODEOWNERS entry covers
src/sql/orsrc/sql_jsc/. - The bug-hunting system found no issues.
|
CI status: the new The red lanes are unrelated to this diff (which touches only
The failure set differs between the two runs, and none of the tests above exercise the MySQL wire protocol. Ready for review. |
1 participant
Problem
Two concurrent simple queries A and B on a
max: 1pool (B queues client-side). The server answers A with its result set plus an unsolicited trailing result set in the same TCP segment, carrying continuation sequence ids (5..8). A real answer to B would restart at seq 1 after B's COM_QUERY (seq 0).process_packetstracked the incoming sequence id (self.sequence_id = header.sequence_id.wrapping_add(1)) but never validated it, and the connection's expected sequence id was never reset when a new command was written. A's terminator advances the queue, B's COM_QUERY is sent, and the trailing ghost bytes in the same read buffer are routed to B. B resolves with rows the server never produced for it, with no error raised:This is server-triggerable (one hostile write, or any protocol-corrupting proxy / server-side desync) on an otherwise idle default connection. The same shape applies to residual bytes after an ERR packet, after an OK, after a completed prepared-statement response.
Fix
src/sql_jsc/mysql/MySQLConnection.rs:process_packets, whenstatus == Connected, reject the connection withPacketsOutOfOrderifheader.sequence_id != self.sequence_id. libmysql fails withCR_NET_PACKETS_OUT_OF_ORDERhere; node-mysql2 warns on the same mismatch.self.sequence_id = 1at every ready-for-next-command transition (auth OK, result-set last terminator, result-set ERR, prepared-statement done, prepared-statement ERR, cached-failure replay), since every command packet is written at seq 0 viawriter.start(0).New error variant
PacketsOutOfOrdersurfaces asERR_MYSQL_PACKETS_OUT_OF_ORDER.Verification
test/js/sql/sql-mysql-sequence-desync.test.ts(mock server via the sharedwire-frames.tsbuilders, no Docker):ERR_MYSQL_PACKETS_OUT_OF_ORDER(on main, B resolves with[{g:"GHOST"}]).ERR_MYSQL_PACKETS_OUT_OF_ORDER(on main, B resolves with[{g:"GHOST"}]).Against a real MariaDB 11.8 on the fixed debug (ASAN) build: concurrent simple queries, prepared queries (COM_STMT_PREPARE then COM_STMT_EXECUTE), a 1000-row result set (sequence id wraps), multi-statement
SELECT 1; SELECT 2(SERVER_MORE_RESULTS_EXISTS, sequence continues across the sub-results), and error-then-success all pass.sql-mysql.helpers.test.ts14/14,sql-mysql.transactions.test.ts12/12 (minus two error-message wording diffs that also fail on main against MariaDB).[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file