Skip to content

fix(sql-editor): critical data-migrate correctness (keys, identity, auth) - #190

Merged
huyplb merged 6 commits into
mainfrom
cursor/critical-bug-management-9f41
Aug 7, 2026
Merged

fix(sql-editor): critical data-migrate correctness (keys, identity, auth)#190
huyplb merged 6 commits into
mainfrom
cursor/critical-bug-management-9f41

Conversation

@cursor

@cursor cursor Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Deep bug hunt on the data-migrate feature from #188 / 8a1bd55 found several high-confidence critical correctness issues. This PR fixes them.

Bugs fixed

  1. Wrong-row UPDATE/DELETE via key collision — composite keys were joined as a=x|b=y, so values containing |/= could alias distinct keys. Keys are now JSON-encoded pairs.
  2. Include identity off still preserved source IDsbuildPeekInsert only skips empty identity values; migrate now strips identity columns when the checkbox is off so the destination generates IDs.
  3. Auth gap on /data-migrate/execute — trusted client op + raw SQL without classification. Now requires DML-only SQL, statementVerb matching op, and category permissions (blocks DDL/GRANT and op/SQL mismatch).
  4. Trigger heuristic silent data loss — bare created / updated / modified were treated as audit columns (default Skip trigger cols) and omitted from INSERT/UPDATE. Heuristic now requires an explicit time/actor suffix.
  5. Duplicate keys — first row won silently; migrate now counts duplicates and refuses to apply.

Checked (no critical issue / residual)

  • Generated DML uses bound params + sql.id (no value injection in the happy path).
  • Source→dest connection wiring uses dest connectionId for execute.
  • Transaction / continue-on-error status marking looks correct; rollback path covered by unit tests.
  • Residual: SQL Server IDENTITY_INSERT is still not wrapped when Include identity is on (engine error, not silent wrong success).

Test plan

  • vitest for resultRowDiff, dataMigratePlans, triggerManagedColumns, data-migrate-execute, rowDml
  • apps/web tsc --noEmit
  • Manual: side-by-side migrate with Include identity off → INSERT omits id
  • Manual: composite string keys containing | do not merge as one update
Open in Web View Automation 

Note

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/execute requires datagridAction to match statementVerb on each statement. /data-migrate/execute classifies each op.sql (DML-only, category permissions), enforces verb ↔ op match, and caps statement length.

Related: Trigger-column heuristic no longer treats bare created/updated/modified as audit fields. DuckDB bigint PKs beyond MAX_SAFE_INTEGER stringify instead of rounding. Safe mode / write detection treats upserts and richer WITH CTE 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.

cursoragent and others added 6 commits August 7, 2026 11:06
…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>
@huyplb
huyplb marked this pull request as ready for review August 7, 2026 17:15
@huyplb
huyplb merged commit f1d142e into main Aug 7, 2026
6 of 7 checks passed
@huyplb
huyplb deleted the cursor/critical-bug-management-9f41 branch August 7, 2026 17:15
@cursor

cursor Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Bugbot couldn't run - usage limit reached

Bugbot 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)

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