Skip to content

Commit 581f2d4

Browse files
committed
fix(sea): input validation — empty-string metadata coercion + bind-time param guards
Three SEA-adapter input-validation fixes from the jira-candidate triage, all in the node layer (no kernel change), all parity-preserving vs the Thrift backend: - Metadata empty-string args (jira #2026-05-22-sea-rejects-empty-string-args): the kernel's Identifier/LikePattern reject "" with InvalidArgument while Thrift treats "" as "unspecified" (match-all/default). getSchemas/getTables/ getColumns/getFunctions now coerce "" -> undefined before the napi call (emptyToUndefined), restoring Thrift parity. Live: getSchemas(catalog="") now returns rows instead of throwing ParameterError. - Array/object param values (jira #2026-05-25-thrift-array-ordinal-hangs): an array bound as a parameter stringified to "1,2,3" and the operation never returned to FINISHED (DoS). Reject array/object values at bind time (assertBindableValue) on both positional and named paths; Date/Int64/ scalars/DBSQLParameter are allowed. - Ordinal arity (jira #2026-05-25-thrift-ordinal-excess-silent): excess ordinal params were silently dropped server-side (data-correctness footgun). Validate positionalParams.length === '?' marker count, with a quote/comment- aware scanner mirroring the kernel's count_parameter_markers. 214 sea unit tests pass; all three fixes verified live against a warehouse. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent 3f1d903 commit 581f2d4

5 files changed

Lines changed: 254 additions & 18 deletions

File tree

lib/sea/SeaInputValidation.ts

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
// Copyright (c) 2026 Databricks, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
import Int64 from 'node-int64';
16+
import { DBSQLParameter, DBSQLParameterValue } from '../DBSQLParameter';
17+
import ParameterError from '../errors/ParameterError';
18+
19+
/**
20+
* Coerce an empty-string metadata argument to `undefined`.
21+
*
22+
* The kernel's `Identifier` / `LikePattern` reject empty strings with
23+
* `InvalidArgument`, whereas the Thrift backend forwards `""` to the server
24+
* which treats it as "unspecified" (match-all / session default). To keep the
25+
* SEA metadata surface behaviourally identical to Thrift, the SEA adapter
26+
* maps `""` → `undefined` before crossing the napi boundary so the kernel
27+
* sees "argument omitted" rather than "empty identifier".
28+
*/
29+
export function emptyToUndefined(value: string | undefined | null): string | undefined {
30+
return value == null || value === '' ? undefined : value;
31+
}
32+
33+
/**
34+
* Walk a SQL string counting `?` parameter markers, ignoring markers inside
35+
* string literals (`'...'`, `"..."`), backtick-quoted identifiers, and
36+
* comments (`-- ...`, `/* ... *​/`). Mirrors the kernel's
37+
* `statement::params::count_parameter_markers` state machine so the JS-side
38+
* arity check matches what the kernel binds.
39+
*/
40+
export function countParameterMarkers(sql: string): number {
41+
let count = 0;
42+
let i = 0;
43+
const n = sql.length;
44+
type State = 'normal' | 'single' | 'double' | 'backtick' | 'line' | 'block';
45+
let state: State = 'normal';
46+
while (i < n) {
47+
const c = sql[i];
48+
const next = i + 1 < n ? sql[i + 1] : '';
49+
switch (state) {
50+
case 'normal':
51+
if (c === '?') {
52+
count += 1;
53+
} else if (c === "'") {
54+
state = 'single';
55+
} else if (c === '"') {
56+
state = 'double';
57+
} else if (c === '`') {
58+
state = 'backtick';
59+
} else if (c === '-' && next === '-') {
60+
state = 'line';
61+
i += 1;
62+
} else if (c === '/' && next === '*') {
63+
state = 'block';
64+
i += 1;
65+
}
66+
break;
67+
case 'single':
68+
if (c === "'" && next === "'") i += 1; // escaped ''
69+
else if (c === "'") state = 'normal';
70+
break;
71+
case 'double':
72+
if (c === '"' && next === '"') i += 1; // escaped ""
73+
else if (c === '"') state = 'normal';
74+
break;
75+
case 'backtick':
76+
if (c === '`') state = 'normal';
77+
break;
78+
case 'line':
79+
if (c === '\n') state = 'normal';
80+
break;
81+
case 'block':
82+
if (c === '*' && next === '/') {
83+
state = 'normal';
84+
i += 1;
85+
}
86+
break;
87+
}
88+
i += 1;
89+
}
90+
return count;
91+
}
92+
93+
/**
94+
* Reject a parameter value that cannot be bound as a scalar. Arrays and plain
95+
* objects stringify to garbage (e.g. `[1,2,3]` → `"1,2,3"`) that the server
96+
* fails to coerce — on the Thrift path the operation never returns to
97+
* FINISHED (a DoS hazard), and on SEA it surfaces an opaque server error. We
98+
* fail fast at bind time instead, mirroring the kernel's compound-type
99+
* rejection. `DBSQLParameter`, `Int64`, `Date`, and JS primitives are allowed.
100+
*/
101+
export function assertBindableValue(value: DBSQLParameter | DBSQLParameterValue, label: string): void {
102+
if (value instanceof DBSQLParameter) return;
103+
if (value === null || value === undefined) return;
104+
if (Array.isArray(value)) {
105+
throw new ParameterError(
106+
`${label} is an array; compound types (ARRAY/MAP/STRUCT) are not bindable as a parameter value`,
107+
);
108+
}
109+
if (typeof value === 'object' && !(value instanceof Date) && !(value instanceof Int64)) {
110+
throw new ParameterError(
111+
`${label} is an object; only scalar values (string/number/bigint/boolean), Date, and Int64 are bindable`,
112+
);
113+
}
114+
}

lib/sea/SeaPositionalParams.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import { DBSQLParameter, DBSQLParameterValue } from '../DBSQLParameter';
1616
import { SeaNativeTypedValueInput, SeaNativeNamedTypedValueInput } from './SeaNativeLoader';
17+
import { assertBindableValue } from './SeaInputValidation';
1718

1819
/**
1920
* Derive `(precision,scale)` from a decimal value string for the SEA
@@ -73,7 +74,10 @@ export function buildSeaPositionalParams(
7374
if (ordinalParameters === undefined || ordinalParameters.length === 0) {
7475
return undefined;
7576
}
76-
return ordinalParameters.map(toTypedValueInput);
77+
return ordinalParameters.map((value, i) => {
78+
assertBindableValue(value, `ordinalParameters[${i}]`);
79+
return toTypedValueInput(value);
80+
});
7781
}
7882

7983
/**
@@ -88,8 +92,8 @@ export function buildSeaNamedParams(
8892
if (namedParameters === undefined || Object.keys(namedParameters).length === 0) {
8993
return undefined;
9094
}
91-
return Object.keys(namedParameters).map((name) => ({
92-
name,
93-
...toTypedValueInput(namedParameters[name]),
94-
}));
95+
return Object.keys(namedParameters).map((name) => {
96+
assertBindableValue(namedParameters[name], `namedParameters[${name}]`);
97+
return { name, ...toTypedValueInput(namedParameters[name]) };
98+
});
9599
}

lib/sea/SeaSessionBackend.ts

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import SeaTableTypeFilter from './SeaTableTypeFilter';
3838
import { seaServerInfoValue } from './SeaServerInfo';
3939
import { buildSeaPositionalParams, buildSeaNamedParams } from './SeaPositionalParams';
4040
import ParameterError from '../errors/ParameterError';
41+
import { emptyToUndefined, countParameterMarkers } from './SeaInputValidation';
4142

4243
export interface SeaSessionBackendOptions {
4344
/** The opaque napi `Connection` handle returned by `openSession`. */
@@ -141,6 +142,18 @@ export default class SeaSessionBackend implements ISessionBackend {
141142
if (positionalParams !== undefined && namedParams !== undefined) {
142143
throw new ParameterError('Driver does not support both ordinal and named parameters.');
143144
}
145+
// Arity check: positional params must match the `?` marker count, or the
146+
// server silently binds the prefix and drops the rest (data-correctness
147+
// footgun). Markers inside string literals / comments are not counted.
148+
if (positionalParams !== undefined) {
149+
const markerCount = countParameterMarkers(statement);
150+
if (positionalParams.length !== markerCount) {
151+
throw new ParameterError(
152+
`ordinalParameters length ${positionalParams.length} does not match the ` +
153+
`${markerCount} '?' placeholder(s) in the SQL`,
154+
);
155+
}
156+
}
144157

145158
const nativeOptions: SeaNativeExecuteOptions = {};
146159
if (positionalParams !== undefined) {
@@ -198,8 +211,8 @@ export default class SeaSessionBackend implements ISessionBackend {
198211
let nativeStatement;
199212
try {
200213
nativeStatement = await this.connection.listSchemas(
201-
request.catalogName,
202-
request.schemaName,
214+
emptyToUndefined(request.catalogName),
215+
emptyToUndefined(request.schemaName),
203216
);
204217
} catch (err) {
205218
throw decodeNapiKernelError(err);
@@ -212,9 +225,9 @@ export default class SeaSessionBackend implements ISessionBackend {
212225
let nativeStatement;
213226
try {
214227
nativeStatement = await this.connection.listTables(
215-
request.catalogName,
216-
request.schemaName,
217-
request.tableName,
228+
emptyToUndefined(request.catalogName),
229+
emptyToUndefined(request.schemaName),
230+
emptyToUndefined(request.tableName),
218231
request.tableTypes,
219232
);
220233
} catch (err) {
@@ -245,10 +258,10 @@ export default class SeaSessionBackend implements ISessionBackend {
245258
let nativeStatement;
246259
try {
247260
nativeStatement = await this.connection.listColumns(
248-
request.catalogName,
249-
request.schemaName,
250-
request.tableName,
251-
request.columnName,
261+
emptyToUndefined(request.catalogName),
262+
emptyToUndefined(request.schemaName),
263+
emptyToUndefined(request.tableName),
264+
emptyToUndefined(request.columnName),
252265
);
253266
} catch (err) {
254267
throw decodeNapiKernelError(err);
@@ -261,9 +274,9 @@ export default class SeaSessionBackend implements ISessionBackend {
261274
let nativeStatement;
262275
try {
263276
nativeStatement = await this.connection.listFunctions(
264-
request.catalogName,
265-
request.schemaName,
266-
request.functionName,
277+
emptyToUndefined(request.catalogName),
278+
emptyToUndefined(request.schemaName),
279+
emptyToUndefined(request.functionName),
267280
);
268281
} catch (err) {
269282
throw decodeNapiKernelError(err);

tests/unit/sea/execution.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ class FakeNativeConnection implements SeaNativeConnection {
8282
// Metadata stubs — return a fresh statement so callers can test wrapping.
8383
public async listCatalogs() { return new FakeNativeStatement(); }
8484

85-
public async listSchemas(_catalog: string | undefined, _schemaPattern: string | undefined) {
85+
public lastListSchemasArgs?: [string | undefined | null, string | undefined | null];
86+
87+
public async listSchemas(catalog: string | undefined | null, schemaPattern: string | undefined | null) {
88+
this.lastListSchemasArgs = [catalog, schemaPattern];
8689
return new FakeNativeStatement();
8790
}
8891

@@ -397,6 +400,37 @@ describe('SeaSessionBackend', () => {
397400
expect((thrown as Error).message).to.match(/both ordinal and named/);
398401
});
399402

403+
it('executeStatement rejects an array-shaped ordinal parameter (DoS guard)', async () => {
404+
const session = makeSession(new FakeNativeConnection());
405+
let thrown: unknown;
406+
try {
407+
await session.executeStatement('SELECT ?', { ordinalParameters: [[1, 2, 3]] as never });
408+
} catch (err) {
409+
thrown = err;
410+
}
411+
expect(thrown).to.be.instanceOf(Error);
412+
expect((thrown as Error).message).to.match(/array/);
413+
});
414+
415+
it('executeStatement rejects an ordinal-parameter count mismatch', async () => {
416+
const session = makeSession(new FakeNativeConnection());
417+
let thrown: unknown;
418+
try {
419+
await session.executeStatement('SELECT ? AS only', { ordinalParameters: [1, 2] });
420+
} catch (err) {
421+
thrown = err;
422+
}
423+
expect(thrown).to.be.instanceOf(Error);
424+
expect((thrown as Error).message).to.match(/does not match/);
425+
});
426+
427+
it('getSchemas coerces empty-string args to undefined (Thrift-parity for the kernel)', async () => {
428+
const connection = new FakeNativeConnection();
429+
const session = makeSession(connection);
430+
await session.getSchemas({ catalogName: '', schemaName: '%' });
431+
expect(connection.lastListSchemasArgs).to.deep.equal([undefined, '%']);
432+
});
433+
400434
it('executeStatement uses the no-options fast path when nothing is bound', async () => {
401435
const connection = new FakeNativeConnection();
402436
const session = makeSession(connection);
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Copyright (c) 2026 Databricks, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
import { expect } from 'chai';
16+
import Int64 from 'node-int64';
17+
import {
18+
emptyToUndefined,
19+
countParameterMarkers,
20+
assertBindableValue,
21+
} from '../../../lib/sea/SeaInputValidation';
22+
import { DBSQLParameter, DBSQLParameterType } from '../../../lib/DBSQLParameter';
23+
import ParameterError from '../../../lib/errors/ParameterError';
24+
25+
describe('SeaInputValidation.emptyToUndefined', () => {
26+
it('maps empty string and null/undefined to undefined; passes real values', () => {
27+
expect(emptyToUndefined('')).to.equal(undefined);
28+
expect(emptyToUndefined(null)).to.equal(undefined);
29+
expect(emptyToUndefined(undefined)).to.equal(undefined);
30+
expect(emptyToUndefined('samples')).to.equal('samples');
31+
expect(emptyToUndefined('%')).to.equal('%');
32+
});
33+
});
34+
35+
describe('SeaInputValidation.countParameterMarkers', () => {
36+
it('counts bare markers', () => {
37+
expect(countParameterMarkers('SELECT ?')).to.equal(1);
38+
expect(countParameterMarkers('SELECT * FROM t WHERE a = ? AND b = ?')).to.equal(2);
39+
expect(countParameterMarkers('SELECT 1')).to.equal(0);
40+
});
41+
42+
it('ignores markers inside string literals, identifiers, and comments', () => {
43+
expect(countParameterMarkers("SELECT '?' AS q")).to.equal(0);
44+
expect(countParameterMarkers('SELECT "?" AS q')).to.equal(0);
45+
expect(countParameterMarkers('SELECT `a?b` FROM t')).to.equal(0);
46+
expect(countParameterMarkers('SELECT 1 -- ? in a line comment\n, ?')).to.equal(1);
47+
expect(countParameterMarkers('SELECT /* ? in block */ ?')).to.equal(1);
48+
expect(countParameterMarkers("SELECT 'it''s ?' , ?")).to.equal(1); // escaped quote
49+
});
50+
});
51+
52+
describe('SeaInputValidation.assertBindableValue', () => {
53+
it('accepts scalars, Date, Int64, bigint, null, and DBSQLParameter', () => {
54+
expect(() => assertBindableValue(42, 'p')).to.not.throw();
55+
expect(() => assertBindableValue('x', 'p')).to.not.throw();
56+
expect(() => assertBindableValue(true, 'p')).to.not.throw();
57+
expect(() => assertBindableValue(BigInt(10), 'p')).to.not.throw();
58+
expect(() => assertBindableValue(null, 'p')).to.not.throw();
59+
expect(() => assertBindableValue(new Date(), 'p')).to.not.throw();
60+
expect(() => assertBindableValue(new Int64(5), 'p')).to.not.throw();
61+
expect(() => assertBindableValue(new DBSQLParameter({ type: DBSQLParameterType.INTEGER, value: 1 }), 'p')).to.not.throw();
62+
});
63+
64+
it('rejects arrays (compound types)', () => {
65+
expect(() => assertBindableValue([1, 2, 3] as never, 'ordinalParameters[0]')).to.throw(ParameterError, /array/);
66+
});
67+
68+
it('rejects plain objects', () => {
69+
expect(() => assertBindableValue({ a: 1 } as never, 'p')).to.throw(ParameterError, /object/);
70+
});
71+
});

0 commit comments

Comments
 (0)