diff --git a/src/core/sql-utils.ts b/src/core/sql-utils.ts index 4f10d92..ef97460 100644 --- a/src/core/sql-utils.ts +++ b/src/core/sql-utils.ts @@ -101,15 +101,41 @@ export function escapeLikePattern(pattern: string, escapeChar: string = '\\'): s } /** - * Validate that a row ID is a finite number and convert it to a number. + * Validate that a row ID is a safe integer and convert it to a number. * Throws an error if the row ID is invalid. * + * A SQLite rowid is always an integer. We deliberately reject inputs that + * `Number()` would silently coerce into a plausible-but-wrong value: + * - non string/number/bigint types — Number(null)/Number(false)/Number([]) + * are all 0, Number(true) is 1, Number([5]) is 5 + * - blank/whitespace strings ('' / ' ' -> 0), which would target "row 0" + * - fractional ('123.45') and scientific-notation ('1e3') strings + * - NaN / Infinity, fractional numbers, and magnitudes beyond ±(2^53-1) + * that a JS number cannot represent without precision loss + * * @param rowId - The row ID to validate - * @returns The validated numeric row ID + * @returns The validated integer row ID */ export function validateRowId(rowId: RecordId): number { + // Only strings, numbers, and bigints are valid rowid inputs. RecordId is typed + // string | number, but this runs against untyped runtime values (RPC payloads, + // persisted history state), where null/boolean/array would otherwise sail + // through Number() as 0/1/element. Capturing typeof in a local keeps TS from + // narrowing rowId to `never` and flagging the (intentional) runtime guard. + const inputType = typeof rowId; + if (inputType !== 'string' && inputType !== 'number' && inputType !== 'bigint') { + throw new Error(`Invalid rowid: ${rowId}`); + } + // For string inputs, require a canonical integer form (optional sign + digits). + // This rejects '', ' ', '123.45' and '1e3' up front — none are valid rowids, + // even though Number() would happily turn them into 0 / 123.45 / 1000. + if (typeof rowId === 'string' && !/^[+-]?\d+$/.test(rowId.trim())) { + throw new Error(`Invalid rowid: ${rowId}`); + } const num = Number(rowId); - if (!Number.isFinite(num)) { + // Require a safe integer: rejects NaN, Infinity, fractional numbers, and + // values outside ±(2^53-1) that cannot be represented exactly as a JS number. + if (!Number.isSafeInteger(num)) { throw new Error(`Invalid rowid: ${rowId}`); } return num; diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index 14acf05..eb630b8 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -450,14 +450,18 @@ describe('DatabaseDocument save/saveAs fallback', () => { it('save: serializes and writes WASM database content for non-file URI', async () => { const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db'); - let serializedName: string | undefined; + let serializeCallCount = 0; + let serializeArgCount = -1; let writeFileCalled = false; const dbOps = { engineKind: Promise.resolve('wasm'), writeToFile: async () => { throw new Error('writeToFile should not be called for non-file URIs'); }, - serializeDatabase: async () => { - serializedName = "test.db"; + // Capture call count + arity so the test fails if save() regresses to passing a + // filename argument (the previous version asserted a value it set itself — tautological). + serializeDatabase: async (...args: unknown[]) => { + serializeCallCount++; + serializeArgCount = args.length; return new Uint8Array([4, 5, 6]); } }; @@ -482,7 +486,8 @@ describe('DatabaseDocument save/saveAs fallback', () => { try { await doc.save(); - assert.strictEqual(serializedName, 'test.db'); + assert.strictEqual(serializeCallCount, 1, 'serializeDatabase should be called exactly once'); + assert.strictEqual(serializeArgCount, 0, 'serializeDatabase should be called with no arguments'); assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called'); } finally { Object.defineProperty(mockVscode.workspace, 'fs', { diff --git a/tests/unit/sql-utils.test.ts b/tests/unit/sql-utils.test.ts index 6225a2c..aa5cfc6 100644 --- a/tests/unit/sql-utils.test.ts +++ b/tests/unit/sql-utils.test.ts @@ -144,15 +144,40 @@ describe('SQL Utils', () => { assert.strictEqual(validateRowId(-9007199254740991n), -9007199254740991); }); - it('should evaluate empty or whitespace strings as 0', () => { - assert.strictEqual(validateRowId(''), 0); - assert.strictEqual(validateRowId(' '), 0); + it('should reject empty or whitespace-only strings', () => { + // Number('') and Number(' ') coerce to 0; a rowid must never silently become "row 0". + assert.throws(() => validateRowId(''), /Invalid rowid:/); + assert.throws(() => validateRowId(' '), /Invalid rowid:/); }); - it('should handle float and scientific notation strings', () => { - assert.strictEqual(validateRowId('123.45'), 123.45); - assert.strictEqual(validateRowId('1e3'), 1000); - assert.strictEqual(validateRowId('-1e3'), -1000); + it('should reject fractional and scientific-notation strings', () => { + assert.throws(() => validateRowId('123.45'), /Invalid rowid: 123\.45/); + assert.throws(() => validateRowId('1e3'), /Invalid rowid: 1e3/); + assert.throws(() => validateRowId('-1e3'), /Invalid rowid: -1e3/); + }); + + it('should reject fractional numbers and unsafe-magnitude integers', () => { + assert.throws(() => validateRowId(123.45), /Invalid rowid: 123\.45/); + assert.throws(() => validateRowId(Number.MAX_SAFE_INTEGER + 1), /Invalid rowid:/); + }); + + it('should reject non-string/non-numeric types that Number() would coerce', () => { + // Number(null)/Number(false)/Number([]) are all 0, Number(true) is 1, Number([5]) is 5 — + // none are valid rowids. RecordId is string|number, but this guards untyped runtime values. + // @ts-ignore - exercising the runtime type guard for values outside RecordId + assert.throws(() => validateRowId(null), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId(undefined), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId(true), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId(false), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId([]), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId([5]), /Invalid rowid:/); + // @ts-ignore + assert.throws(() => validateRowId({}), /Invalid rowid:/); }); it('should throw an error for non-numeric strings', () => { diff --git a/tests/unit/workerFactory_browser.test.ts b/tests/unit/workerFactory_browser.test.ts index 5d5227a..c0f22d5 100644 --- a/tests/unit/workerFactory_browser.test.ts +++ b/tests/unit/workerFactory_browser.test.ts @@ -15,7 +15,7 @@ const workerFactorySource = fs.readFileSync(workerFactoryPath, 'utf8'); interface FakeEndpoint { initializeDatabase(filename: string, config: DatabaseInitConfig): Promise<{ isReadOnly: boolean }>; runQuery(sql: string, params?: CellValue[]): Promise; - exportDatabase(name: string): Promise; + exportDatabase(): Promise; applyModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise; undoModification?(mod: ModificationEntry): Promise; redoModification?(mod: ModificationEntry): Promise;