fix(import): sanitize row errors — never leak raw SQL, friendly constraint messages (#3566)

.changeset/import-sanitize-row-errors.md

@@ -0,0 +1,25 @@
+---
+"@objectstack/rest": patch
+"@objectstack/plugin-auth": patch
+---
+
+fix(import): sanitize row errors — never leak raw SQL, map constraint failures to human wording (#3566)
+
+A failing import row surfaced the driver's raw error verbatim. When a write hit
+a DB constraint (e.g. `sys_user.phone_number` is `unique`), the query builder
+embeds the entire failing statement in `err.message`, and `toFailedResult`
+handed that straight back — so the importer saw ``insert into `sys_user`
+(...) values (...) - UNIQUE constraint failed: sys_user.phone_number``. That is
+both unreadable and an information disclosure of the schema.
+
+- `sanitizeRowError()` (import-runner) maps the common constraint failures —
+  SQLite / MySQL / Postgres `UNIQUE` and `NOT NULL` — to human wording
+  ("A record with this `<column>` already exists.", "`<column>` is required.")
+  and, as a backstop, never lets a message that still reads as a SQL statement
+  reach the client (it salvages the driver's trailing reason, or falls back to
+  a generic message). Already-friendly messages (e.g. better-auth's "User
+  already exists") pass through unchanged. Applies to every import path.
+- `isLikelyEmail` now rejects non-ASCII addresses, so an address like
+  `x@柴仟.com` fails the import **dry-run** pre-check instead of passing client
+  and dry-run validation only to be rejected by better-auth's strict ASCII
+  validator at real-import time.

packages/plugins/plugin-auth/src/admin-user-endpoints.test.ts

@@ -83,6 +83,16 @@ describe('isLikelyEmail (linear-time, no regex backtracking)', () => {
     expect(isLikelyEmail('has space@b.co')).toBe(false);
   });
 
+  it('rejects non-ASCII addresses (framework#3566)', async () => {
+    const { isLikelyEmail } = await import('./admin-user-endpoints.js');
+    // Chinese domain — passes the structural checks but better-auth's strict
+    // ASCII validator would reject it, so reject it here (and in the dry-run).
+    expect(isLikelyEmail('735431496@柴仟.com')).toBe(false);
+    // Full-width / invisible characters anywhere in the address.
+    expect(isLikelyEmail('ａｂｃ@b.com')).toBe(false);
+    expect(isLikelyEmail('a@b​.com')).toBe(false);
+  });
+
   it('is fast on the CodeQL adversarial inputs', async () => {
     const { isLikelyEmail } = await import('./admin-user-endpoints.js');
     const attack = '!@'.repeat(100_000);

packages/plugins/plugin-auth/src/admin-user-endpoints.ts

@@ -187,6 +187,12 @@ function badRequest(message: string): EndpointResult {
  */
 export function isLikelyEmail(value: string): boolean {
   if (value.length === 0 || value.length > 254 || /\s/.test(value)) return false;
+  // Reject non-ASCII up front so obviously-invalid addresses (e.g. a Chinese
+  // domain like `x@柴仟.com`) fail this pre-filter — and therefore the import
+  // dry-run — instead of passing here only to be rejected later by
+  // better-auth's strict ASCII validator at real-import time (framework#3566).
+  // A single linear char-class test; no backtracking (see the ReDoS note above).
+  if (/[^\x00-\x7f]/.test(value)) return false;
   const at = value.indexOf('@');
   if (at <= 0 || at !== value.lastIndexOf('@') || at === value.length - 1) return false;
   const domain = value.slice(at + 1);

packages/rest/src/import-runner-error-sanitize.test.ts

@@ -0,0 +1,66 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * sanitizeRowError() — driver/query-builder errors embed the whole failing SQL
+ * statement in `err.message`; it must never reach the importer verbatim
+ * (framework#3566). Common constraint failures map to human wording.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { sanitizeRowError } from './import-runner';
+
+describe('sanitizeRowError', () => {
+  it('maps a sqlite UNIQUE violation to a friendly, SQL-free message', () => {
+    const raw =
+      "insert into `sys_user` (`email`, `name`, `phone_number`) values " +
+      "('a@b.com', 'zhoujunyi', '13800000000') - UNIQUE constraint failed: sys_user.phone_number";
+    const out = sanitizeRowError(raw);
+    expect(out).toBe('A record with this phone_number already exists.');
+    expect(out).not.toMatch(/insert into/i);
+  });
+
+  it('maps a mysql duplicate entry', () => {
+    const raw =
+      "insert into `sys_user` ... - ER_DUP_ENTRY: Duplicate entry 'a@b.com' for key 'sys_user.email'";
+    expect(sanitizeRowError(raw)).toBe('A record with this email already exists.');
+  });
+
+  it('maps a postgres unique violation', () => {
+    const raw =
+      'insert into "sys_user" ... - duplicate key value violates unique constraint "sys_user_email_key" ' +
+      'Detail: Key (email)=(a@b.com) already exists.';
+    expect(sanitizeRowError(raw)).toBe('A record with this email already exists.');
+  });
+
+  it('maps a NOT NULL violation', () => {
+    const raw = 'insert into `sys_user` (...) values (...) - NOT NULL constraint failed: sys_user.name';
+    expect(sanitizeRowError(raw)).toBe('name is required.');
+  });
+
+  it('never leaks a raw SQL statement even for an unrecognized reason', () => {
+    const raw = 'insert into `sys_user` (`email`) values (?) - some cryptic driver failure';
+    const out = sanitizeRowError(raw);
+    expect(out).not.toMatch(/insert into/i);
+    // salvages the trailing non-SQL reason
+    expect(out).toBe('some cryptic driver failure');
+  });
+
+  it('falls back to a generic message when only SQL is present', () => {
+    const raw = 'insert into `sys_user` (`email`) values (?)';
+    expect(sanitizeRowError(raw)).toBe(
+      'The database rejected this row (a value may be invalid or already in use).',
+    );
+  });
+
+  it('passes already-friendly messages through unchanged', () => {
+    expect(sanitizeRowError('User already exists. Use another email.')).toBe(
+      'User already exists. Use another email.',
+    );
+  });
+
+  it('handles empty / non-string input', () => {
+    expect(sanitizeRowError('')).toBe('Row failed');
+    expect(sanitizeRowError(undefined)).toBe('Row failed');
+    expect(sanitizeRowError(null)).toBe('Row failed');
+  });
+});

packages/rest/src/import-runner.ts

@@ -144,9 +144,60 @@ function extractRecordId(rec: any): string | undefined {
   return id != null ? String(id) : undefined;
 }
 
+/** Does this text begin with a SQL statement? (leaked driver query builder output) */
+function looksLikeSql(text: string): boolean {
+  return /^\s*(insert|update|delete|select|with|replace)\s/i.test(text);
+}
+
+/** Strip a `table.column` (or quoted) constraint target down to a bare column name. */
+function bareColumn(raw: string): string {
+  const col = raw.trim().replace(/[`"']/g, '');
+  const dot = col.lastIndexOf('.');
+  return dot >= 0 ? col.slice(dot + 1) : col;
+}
+
+/**
+ * Turn a raw write error into a message safe to hand back to the importer.
+ *
+ * Driver / query-builder errors (knex et al.) embed the *entire* failing SQL
+ * statement in `err.message` — e.g. ``insert into `sys_user` (...) values
+ * (...) - UNIQUE constraint failed: sys_user.phone_number``. Surfacing that
+ * verbatim is both unreadable and an information disclosure of the schema
+ * (framework#3566). This maps the common constraint failures to human wording
+ * and, as a backstop, never lets a raw SQL statement escape to the client.
+ */
+export function sanitizeRowError(raw: unknown): string {
+  const msg = typeof raw === 'string' ? raw.trim() : '';
+  if (!msg) return 'Row failed';
+
+  // UNIQUE — surface the offending column (it maps to a user-facing import
+  // column, so naming it is helpful, not a schema leak).
+  const unique =
+    /unique constraint failed:\s*([^\s,)]+)/i.exec(msg) ??          // sqlite
+    /duplicate entry .* for key '([^']+)'/i.exec(msg) ??            // mysql
+    /duplicate key value violates unique constraint.*?[Kk]ey \(([^)]+)\)/is.exec(msg); // postgres
+  if (unique) return `A record with this ${bareColumn(unique[1])} already exists.`;
+
+  // NOT NULL — a required value is missing.
+  const notNull = /not null constraint failed:\s*([^\s,)]+)/i.exec(msg);
+  if (notNull) return `${bareColumn(notNull[1])} is required.`;
+
+  // Backstop: anything that still reads as a SQL statement must not reach the
+  // client. Prefer the driver's trailing reason (after `... - <reason>`) when
+  // it is itself not SQL; otherwise fall back to a generic message.
+  if (looksLikeSql(msg)) {
+    const sep = msg.lastIndexOf(' - ');
+    const reason = sep >= 0 ? msg.slice(sep + 3).trim() : '';
+    if (reason && !looksLikeSql(reason)) return reason.slice(0, 300);
+    return 'The database rejected this row (a value may be invalid or already in use).';
+  }
+
+  return msg.slice(0, 300);
+}
+
 function toFailedResult(rowNo: number, err: unknown): ImportRowResult {
   const code = (err as any)?.code ?? 'IMPORT_ROW_FAILED';
-  const message = typeof (err as any)?.message === 'string' ? (err as any).message.slice(0, 300) : 'Row failed';
+  const message = sanitizeRowError((err as any)?.message);
   return { row: rowNo, ok: false, action: 'failed', error: message, code };
 }