From 4a644ab205ccf2e7b47149e91c315f953f95b653 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 8 Jun 2026 19:18:27 +0200 Subject: [PATCH 1/2] fix(sql-utils): reject blank and non-integer rowids in validateRowId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateRowId only did Number()+isFinite, so '' / ' ' coerced to 0 and '123.45' / '1e3' were accepted — silently turning malformed input into plausible-but-wrong rowids on WHERE rowid = ? paths. Now require a canonical integer string form and Number.isSafeInteger (rejects blank/whitespace, fractional, scientific-notation, NaN/Infinity, and >2^53 magnitudes). Also addresses two test-quality findings from the post-batch Codex review: - workerFactory_browser.test.ts: drop stale 'name' arg from the local FakeEndpoint.exportDatabase signature (leftover from #452's arity change) - databaseModel.test.ts: replace the tautological serializeDatabase assertion (asserted a value it set itself) with call-count + zero-arity checks Tests flipped to assert rejection for blank/whitespace/fractional/scientific rowids. tsc -p tsconfig.json clean; 428 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/sql-utils.ts | 21 ++++++++++++++++++--- tests/unit/databaseModel.test.ts | 13 +++++++++---- tests/unit/sql-utils.test.ts | 20 +++++++++++++------- tests/unit/workerFactory_browser.test.ts | 2 +- 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/core/sql-utils.ts b/src/core/sql-utils.ts index 4f10d92..bbb3ae4 100644 --- a/src/core/sql-utils.ts +++ b/src/core/sql-utils.ts @@ -101,15 +101,30 @@ 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: + * - 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 { + // 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..c57299d 100644 --- a/tests/unit/sql-utils.test.ts +++ b/tests/unit/sql-utils.test.ts @@ -144,15 +144,21 @@ 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 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; From aa90fa8cb733a7de9dd792d2a66e5e39669f7043 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 8 Jun 2026 19:24:17 +0200 Subject: [PATCH 2/2] fix(sql-utils): reject non-string/number/bigint rowids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Gemini's review on #456: the prior guard still let null/boolean/ array slip through — Number(null)/Number(false)/Number([]) are 0, Number(true) is 1, Number([5]) is 5, all passing isSafeInteger. Add an explicit typeof guard (string/number/bigint only) before coercion, plus tests asserting null/undefined/boolean/array/object are rejected. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/sql-utils.ts | 11 +++++++++++ tests/unit/sql-utils.test.ts | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/core/sql-utils.ts b/src/core/sql-utils.ts index bbb3ae4..ef97460 100644 --- a/src/core/sql-utils.ts +++ b/src/core/sql-utils.ts @@ -106,6 +106,8 @@ export function escapeLikePattern(pattern: string, escapeChar: string = '\\'): s * * 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) @@ -115,6 +117,15 @@ export function escapeLikePattern(pattern: string, escapeChar: string = '\\'): s * @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. diff --git a/tests/unit/sql-utils.test.ts b/tests/unit/sql-utils.test.ts index c57299d..aa5cfc6 100644 --- a/tests/unit/sql-utils.test.ts +++ b/tests/unit/sql-utils.test.ts @@ -161,6 +161,25 @@ describe('SQL Utils', () => { 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', () => { assert.throws(() => validateRowId('abc'), /Invalid rowid: abc/); assert.throws(() => validateRowId('12a'), /Invalid rowid: 12a/);