fix(sql-editor): critical data-migrate correctness (keys, identity, auth) - #190
Merged
Conversation
…uth) - Encode composite migrate keys as JSON pairs so values containing |/= cannot collide and UPDATE/DELETE the wrong row - Strip identity columns on INSERT when Include identity is off (buildPeekInsert only skipped empty IDs) - Tighten trigger-column heuristic so bare created/updated/modified are not silently omitted - Block migrate when result grids have duplicate keys - Classify /data-migrate/execute SQL and require verb to match op (block DDL/GRANT and op/SQL mismatch) Co-authored-by: huy.phan9 <huyplb@users.noreply.github.com>
Number(bigint) silently rounds past MAX_SAFE_INTEGER, so Peek/result UPDATE/DELETE WHERE clauses could target the wrong key. Safe integers still coerce to number for introspection and small PKs. Co-authored-by: huy.phan9 <huyplb@users.noreply.github.com>
Data migrate classified only currently loaded grid rows, so paging one side (or hasNext) made Delete remove destination keys that still exist later in the source. Require both grids on page 1 with no remaining pages. Also stop defaulting keys to the first column / free checkboxes — use PK or unique index columns only, matching Peek editability. Co-authored-by: huy.phan9 <huyplb@users.noreply.github.com>
Mirror data-migrate hardening so a client cannot claim insert while sending UPDATE/DELETE under the grid-write permission path. Co-authored-by: huy.phan9 <huyplb@users.noreply.github.com>
INSERT ... ON CONFLICT DO UPDATE / ON DUPLICATE KEY UPDATE / OR REPLACE were treated as insert-only and skipped confirmation. WITH AS MATERIALIZED and quoted CTE names also made walkWithCtes fail open so DELETE/UPDATE ran without Safe Mode. Fail closed on unscannable WITH; parse MATERIALIZED and quoted CTE names. Co-authored-by: huy.phan9 <huyplb@users.noreply.github.com>
Contributor
Author
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_765d56d7-458c-49fd-a10f-447bd6a42729) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Deep bug hunt on the data-migrate feature from #188 /
8a1bd55found several high-confidence critical correctness issues. This PR fixes them.Bugs fixed
a=x|b=y, so values containing|/=could alias distinct keys. Keys are now JSON-encoded pairs.buildPeekInsertonly skips empty identity values; migrate now strips identity columns when the checkbox is off so the destination generates IDs./data-migrate/execute— trusted clientop+ raw SQL without classification. Now requires DML-only SQL,statementVerbmatchingop, and category permissions (blocks DDL/GRANT and op/SQL mismatch).created/updated/modifiedwere treated as audit columns (default Skip trigger cols) and omitted from INSERT/UPDATE. Heuristic now requires an explicit time/actor suffix.Checked (no critical issue / residual)
sql.id(no value injection in the happy path).connectionIdfor execute.IDENTITY_INSERTis still not wrapped when Include identity is on (engine error, not silent wrong success).Test plan
vitestforresultRowDiff,dataMigratePlans,triggerManagedColumns,data-migrate-execute,rowDmlapps/webtsc --noEmit|do not merge as one updateNote
High Risk
Changes row-level migrate matching, paging gates, and server-side SQL classification on data-migrate and datagrid execute—areas where bugs can delete or update wrong rows or bypass permissions.
Overview
Hardens side-by-side data migrate so row matching and apply semantics match what the UI promises, and closes permission gaps on SQL execute paths.
Migrate matching & apply: Composite keys use JSON-encoded wire values so
|/=in data cannot alias distinct rows. Duplicate keys in either grid are counted and block apply. Migrate runs only when both grids are on page 1 with no further pages (hasNext/truncated), avoiding deletes of destination rows that still exist off-page. Keys come from schema PK/unique columns present in the SELECT (read-only badges)—no manual key checkboxes or “first column” fallback.INSERT identity: With Include identity off, identity columns are stripped before plan build so source IDs are not preserved (Peek insert only skipped empty identity values).
API:
/sql/executerequiresdatagridActionto matchstatementVerbon each statement./data-migrate/executeclassifies eachop.sql(DML-only, category permissions), enforces verb ↔opmatch, and caps statement length.Related: Trigger-column heuristic no longer treats bare
created/updated/modifiedas audit fields. DuckDB bigint PKs beyondMAX_SAFE_INTEGERstringify instead of rounding. Safe mode / write detection treats upserts and richerWITHCTE shapes; user guide documents migrate and safe-mode rules.Reviewed by Cursor Bugbot for commit 90fa251. Bugbot is set up for automated code reviews on this repo. Configure here.