fix(sql-utils): harden validateRowId + post-review test fixes

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;

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', {

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', () => {

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<unknown[]>;
-  exportDatabase(name: string): Promise<Uint8Array>;
+  exportDatabase(): Promise<Uint8Array>;
   applyModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise<void>;
   undoModification?(mod: ModificationEntry): Promise<void>;
   redoModification?(mod: ModificationEntry): Promise<void>;