Skip to content

Commit 4a644ab

Browse files
zknprclaude
andcommitted
fix(sql-utils): reject blank and non-integer rowids in validateRowId
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) <noreply@anthropic.com>
1 parent 3eb7512 commit 4a644ab

4 files changed

Lines changed: 41 additions & 15 deletions

File tree

src/core/sql-utils.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,30 @@ export function escapeLikePattern(pattern: string, escapeChar: string = '\\'): s
101101
}
102102

103103
/**
104-
* Validate that a row ID is a finite number and convert it to a number.
104+
* Validate that a row ID is a safe integer and convert it to a number.
105105
* Throws an error if the row ID is invalid.
106106
*
107+
* A SQLite rowid is always an integer. We deliberately reject inputs that
108+
* `Number()` would silently coerce into a plausible-but-wrong value:
109+
* - blank/whitespace strings ('' / ' ' -> 0), which would target "row 0"
110+
* - fractional ('123.45') and scientific-notation ('1e3') strings
111+
* - NaN / Infinity, fractional numbers, and magnitudes beyond ±(2^53-1)
112+
* that a JS number cannot represent without precision loss
113+
*
107114
* @param rowId - The row ID to validate
108-
* @returns The validated numeric row ID
115+
* @returns The validated integer row ID
109116
*/
110117
export function validateRowId(rowId: RecordId): number {
118+
// For string inputs, require a canonical integer form (optional sign + digits).
119+
// This rejects '', ' ', '123.45' and '1e3' up front — none are valid rowids,
120+
// even though Number() would happily turn them into 0 / 123.45 / 1000.
121+
if (typeof rowId === 'string' && !/^[+-]?\d+$/.test(rowId.trim())) {
122+
throw new Error(`Invalid rowid: ${rowId}`);
123+
}
111124
const num = Number(rowId);
112-
if (!Number.isFinite(num)) {
125+
// Require a safe integer: rejects NaN, Infinity, fractional numbers, and
126+
// values outside ±(2^53-1) that cannot be represented exactly as a JS number.
127+
if (!Number.isSafeInteger(num)) {
113128
throw new Error(`Invalid rowid: ${rowId}`);
114129
}
115130
return num;

tests/unit/databaseModel.test.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -450,14 +450,18 @@ describe('DatabaseDocument save/saveAs fallback', () => {
450450

451451
it('save: serializes and writes WASM database content for non-file URI', async () => {
452452
const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db');
453-
let serializedName: string | undefined;
453+
let serializeCallCount = 0;
454+
let serializeArgCount = -1;
454455
let writeFileCalled = false;
455456

456457
const dbOps = {
457458
engineKind: Promise.resolve('wasm'),
458459
writeToFile: async () => { throw new Error('writeToFile should not be called for non-file URIs'); },
459-
serializeDatabase: async () => {
460-
serializedName = "test.db";
460+
// Capture call count + arity so the test fails if save() regresses to passing a
461+
// filename argument (the previous version asserted a value it set itself — tautological).
462+
serializeDatabase: async (...args: unknown[]) => {
463+
serializeCallCount++;
464+
serializeArgCount = args.length;
461465
return new Uint8Array([4, 5, 6]);
462466
}
463467
};
@@ -482,7 +486,8 @@ describe('DatabaseDocument save/saveAs fallback', () => {
482486
try {
483487
await doc.save();
484488

485-
assert.strictEqual(serializedName, 'test.db');
489+
assert.strictEqual(serializeCallCount, 1, 'serializeDatabase should be called exactly once');
490+
assert.strictEqual(serializeArgCount, 0, 'serializeDatabase should be called with no arguments');
486491
assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called');
487492
} finally {
488493
Object.defineProperty(mockVscode.workspace, 'fs', {

tests/unit/sql-utils.test.ts

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -144,15 +144,21 @@ describe('SQL Utils', () => {
144144
assert.strictEqual(validateRowId(-9007199254740991n), -9007199254740991);
145145
});
146146

147-
it('should evaluate empty or whitespace strings as 0', () => {
148-
assert.strictEqual(validateRowId(''), 0);
149-
assert.strictEqual(validateRowId(' '), 0);
147+
it('should reject empty or whitespace-only strings', () => {
148+
// Number('') and Number(' ') coerce to 0; a rowid must never silently become "row 0".
149+
assert.throws(() => validateRowId(''), /Invalid rowid:/);
150+
assert.throws(() => validateRowId(' '), /Invalid rowid:/);
150151
});
151152

152-
it('should handle float and scientific notation strings', () => {
153-
assert.strictEqual(validateRowId('123.45'), 123.45);
154-
assert.strictEqual(validateRowId('1e3'), 1000);
155-
assert.strictEqual(validateRowId('-1e3'), -1000);
153+
it('should reject fractional and scientific-notation strings', () => {
154+
assert.throws(() => validateRowId('123.45'), /Invalid rowid: 123\.45/);
155+
assert.throws(() => validateRowId('1e3'), /Invalid rowid: 1e3/);
156+
assert.throws(() => validateRowId('-1e3'), /Invalid rowid: -1e3/);
157+
});
158+
159+
it('should reject fractional numbers and unsafe-magnitude integers', () => {
160+
assert.throws(() => validateRowId(123.45), /Invalid rowid: 123\.45/);
161+
assert.throws(() => validateRowId(Number.MAX_SAFE_INTEGER + 1), /Invalid rowid:/);
156162
});
157163

158164
it('should throw an error for non-numeric strings', () => {

tests/unit/workerFactory_browser.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ const workerFactorySource = fs.readFileSync(workerFactoryPath, 'utf8');
1515
interface FakeEndpoint {
1616
initializeDatabase(filename: string, config: DatabaseInitConfig): Promise<{ isReadOnly: boolean }>;
1717
runQuery(sql: string, params?: CellValue[]): Promise<unknown[]>;
18-
exportDatabase(name: string): Promise<Uint8Array>;
18+
exportDatabase(): Promise<Uint8Array>;
1919
applyModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise<void>;
2020
undoModification?(mod: ModificationEntry): Promise<void>;
2121
redoModification?(mod: ModificationEntry): Promise<void>;

0 commit comments

Comments
 (0)