diff --git a/.changeset/import-sanitize-row-errors.md b/.changeset/import-sanitize-row-errors.md new file mode 100644 index 0000000000..6911b070c6 --- /dev/null +++ b/.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 `` already exists.", "`` 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. diff --git a/packages/plugins/plugin-auth/src/admin-user-endpoints.test.ts b/packages/plugins/plugin-auth/src/admin-user-endpoints.test.ts index ead4d919f1..6534c4f0a8 100644 --- a/packages/plugins/plugin-auth/src/admin-user-endpoints.test.ts +++ b/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); diff --git a/packages/plugins/plugin-auth/src/admin-user-endpoints.ts b/packages/plugins/plugin-auth/src/admin-user-endpoints.ts index cd32589b47..ab2bdd4a5e 100644 --- a/packages/plugins/plugin-auth/src/admin-user-endpoints.ts +++ b/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); diff --git a/packages/rest/src/import-runner-error-sanitize.test.ts b/packages/rest/src/import-runner-error-sanitize.test.ts new file mode 100644 index 0000000000..3925b5835c --- /dev/null +++ b/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'); + }); +}); diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 608a38b237..9157fa73a3 100644 --- a/packages/rest/src/import-runner.ts +++ b/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 `... - `) 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 }; }