fix: parameterize SQON-to-SQL filter values, closing a SQL injection

.dev/sessions/2026-08-20T152420.md

@@ -0,0 +1,6 @@
+Closed a SQL injection reachable through any SQON filter, plus a stored variant reachable through submitted data.
+
+- `packages/data-provider/src/utils/convertSqonToQuery.ts`: `processFilterOperator` spliced `fieldName`/`value` into `sql.raw()` text via an unescaping helper; a crafted value could alter the WHERE clause's boolean structure. Bound both through drizzle's `sql` template and `inArray()` instead, matching the `IN`-clause convention already used elsewhere in this codebase; removed the unescaping helper entirely rather than hardening it. Dropped a stale `TODO: SQL sanitization` comment this closes.
+- `packages/data-provider/src/repository/submittedRepository.ts`: `getSubmittedDataFiltered` had the same unescaped-splice pattern, more severe here since `dataValue` is read back out of a submitter's own previously-submitted record (foreign-key relationship resolution during compound-view reads and submission processing), so a malicious value submitted once would have poisoned every later query walking that record's relationships. Extracted the filter-building into `buildDataFieldFilter` and fixed the same way.
+- New tests: `test/unit/utils/convertSqonToQuery.spec.ts` and `test/unit/repository/submittedRepository.spec.ts` cover injection-shaped input (quotes, statement terminators, comment markers) on both fixes, a multi-element `in` array (the prior single-element-only coverage couldn't distinguish comma-expansion from an invalid single-array-parameter bind), and parameter-binding parity for ordinary input.
+- `.dev/tech-debt.md`: logged that issue #43 covers three sanitization items and only the SQON one is fixed here, and that SQON `fieldName` still has no allowlist against the active dictionary's schema (harmless now that it's parameterized, but worth closing for correctness).

.dev/tech-debt.md

@@ -51,6 +51,14 @@ context: `byCategoryIdAndOrganization` (`auditController.ts`) throws `NotFound('
 standalone: yes
 context: `dictionary_categories.id` is a plain auto-increment `serial`, used directly in URLs and responses, a predictable-identifier smell. The alias feature is a step away from this but doesn't remove or restrict the id. Not fixed: would mean exposing categories only by alias externally, or a non-sequential primary key, both out of scope here.
 
+### Issue #43 ("Sanitize JSON data") covers 3 items; only the SQON one is fixed
+standalone: yes
+context: #43 lists three items: SQON `fieldName`/`value` sanitization on the query endpoint, TSV-data sanitization before DB insert, and a general other-endpoints input-parameter audit. Only the first is fixed in code today, via parameterized queries in `convertSqonToQuery.ts` and, for the same unescaped-splice-into-`sql.raw()` pattern, `submittedRepository.ts`'s `getSubmittedDataFiltered` (that path is FK-relationship resolution during compound-view reads/submission processing, not literally "the query endpoint" the item names). TSV sanitization and the broader endpoint audit remain untouched and unscoped. Fix: two further, separate efforts — (1) sanitize TSV data before inserting into the database, (2) audit other endpoints' input parameters (path params, query params, etc.) for the same class of issue. #43 should stay open until both land.
+
+### SQON `fieldName` has no allowlist against the dictionary schema
+standalone: yes
+context: `dataGetByQueryRequestSchema`'s `sqonSchema` only checks that a SQON parses structurally (`zod.custom` around `parseSQON`); sqon-builder's own `fieldName: zod.ZodString` accepts any non-empty string. Nothing compares it against the category's active dictionary schema, so a caller can query on any arbitrary key — harmless (an unmapped key just returns no matches; SQL injection via `fieldName` is closed separately, via parameterization in `convertSqonToQuery.ts`), but worth closing for correctness and to catch typo'd/renamed field names at request time. Fix: validate `fieldName` (and any nested path segments) against the active dictionary's actual field names before building the query, in `convertSqonToQuery.ts` or the controller layer above it; reject with a 400 rather than silently returning zero rows for an unknown field.
+
 ---
 
 ## Resolved

packages/data-provider/src/repository/submittedRepository.ts

@@ -17,6 +17,15 @@ import { BaseDependencies } from '../config/config.js';
 import { ServiceUnavailable } from '../utils/errors.js';
 import { AUDIT_ACTION, BooleanTrueObject, PaginationOptions, SubmittedDataResponse } from '../utils/types.js';
 
+/**
+ * Builds a filter comparing a JSONB data field against a value, binding both as query
+ * parameters rather than splicing them into the SQL text.
+ */
+export const buildDataFieldFilter = (dataField: string, dataValue: string | undefined): SQL<unknown> => {
+	const jsonbField = sql`${sql.raw(submittedData.data.name)} ->> ${dataField}`;
+	return inArray(jsonbField, [String(dataValue)]);
+};
+
 const repository = (dependencies: BaseDependencies) => {
 	const LOG_MODULE = 'SUBMITTEDDATA_REPOSITORY';
 	const { db, logger, features } = dependencies;
@@ -88,9 +97,6 @@ const repository = (dependencies: BaseDependencies) => {
 		return await (tx || db).insert(auditSubmittedData).values(newAudit);
 	};
 
-	// Column name on the database used to build JSONB query
-	const jsonbColumnName = submittedData.data.name;
-
 	const paginatedColumns: BooleanTrueObject = {
 		entityName: true,
 		data: true,
@@ -524,7 +530,7 @@ const repository = (dependencies: BaseDependencies) => {
 		): Promise<SubmittedData[]> => {
 			const sqlDataFilter = filterData.map((filter) => {
 				return and(
-					sql.raw(`${jsonbColumnName} ->> '${filter.dataField}' IN ('${filter.dataValue}')`),
+					buildDataFieldFilter(filter.dataField, filter.dataValue),
 					eq(submittedData.entityName, filter.entityName),
 				);
 			});

packages/data-provider/src/utils/convertSqonToQuery.ts

@@ -1,9 +1,8 @@
-import { and, not, or, SQL, sql } from 'drizzle-orm';
+import { and, inArray, not, or, SQL, sql } from 'drizzle-orm';
 import * as _ from 'lodash-es';
 import { ZodError } from 'zod';
 
 import SQONBuilder, {
-	ArrayFilterValue,
 	CombinationKeys,
 	CombinationOperator,
 	FilterOperator,
@@ -27,44 +26,23 @@ const isGreaterThanFilter = (operator: Operator): operator is GreaterThanFilter
 const isLesserThanFilter = (operator: Operator): operator is LesserThanFilter =>
 	LesserThanFilter.safeParse(operator).success;
 
-// Map the array and format each element based on its type
-const formatForSQL = (value: ArrayFilterValue) => {
-	if (Array.isArray(value)) {
-		// Handle array of strings or numbers
-		return value
-			.map((element) => {
-				if (typeof element === 'string') {
-					return `'${element}'`; // Surround strings with single quotes
-				} else if (typeof element === 'number') {
-					return element.toString(); // Numbers don't need quotes
-				} else {
-					throw new BadRequest(`Invalid SQON format. Unsupported data type: ${typeof element}`);
-				}
-			})
-			.join(', ');
-	} else if (typeof value === 'string') {
-		// Handle single string
-		return value;
-	} else if (typeof value === 'number') {
-		// Handle single number
-		return value;
-	}
-
-	throw new BadRequest(`Invalid SQON. Unsupported data type: ${typeof value}`);
-};
-
 const processFilterOperator = (operator: FilterOperator): SQL<unknown> => {
 	const { fieldName, value } = operator.content;
+	const jsonbField = sql`${sql.raw(jsonbColumnName)} ->> ${fieldName}`;
 
 	if (isArrayFilter(operator)) {
-		// op is in
-		return sql.raw(`${jsonbColumnName} ->> '${formatForSQL(fieldName)}' IN (${formatForSQL(value)})`);
+		// op is in; inArray matches the IN-clause convention already used elsewhere in this
+		// codebase (activeSubmissionRepository.ts, submittedRepository.ts) instead of a
+		// hand-rolled sql template, and rejects an empty array up front rather than emitting
+		// invalid `IN ()` SQL.
+		const values = (Array.isArray(value) ? value : [value]).map(String);
+		return inArray(jsonbField, values);
 	} else if (isGreaterThanFilter(operator)) {
 		// is an scalar filter op is gt
-		return sql.raw(`${jsonbColumnName} ->> '${formatForSQL(fieldName)}' > '${formatForSQL(value)}'`);
+		return sql`${jsonbField} > ${String(value)}`;
 	} else if (isLesserThanFilter(operator)) {
 		// is an scalar filter op is lt
-		return sql.raw(`${jsonbColumnName} ->> '${formatForSQL(fieldName)}' < '${formatForSQL(value)}'`);
+		return sql`${jsonbField} < ${String(value)}`;
 	}
 
 	throw new BadRequest(`Invalid SQON format. Unsupported SQON filter operator`);
@@ -155,8 +133,6 @@ export const parseSQON = (input: unknown): SQON | undefined => {
 		// Given any input, attempt to parse it as a SQON.
 		// An error will be thrown if the provided input is invalid.
 		return SQONBuilder.default.from(input);
-
-		// TODO: SQL sanitization (https://github.com/overture-stack/lyric/issues/43)
 	} catch (error: unknown) {
 		if (isZodError(error)) {
 			throw new BadRequest('Invalid SQON format', error.issues);

packages/data-provider/test/unit/repository/submittedRepository.spec.ts

@@ -0,0 +1,45 @@
+import { expect } from 'chai';
+import { PgDialect } from 'drizzle-orm/pg-core';
+import { describe, it } from 'mocha';
+
+import { buildDataFieldFilter } from '../../../src/repository/submittedRepository.js';
+
+const dialect = new PgDialect();
+
+describe('submittedRepository', () => {
+	describe('buildDataFieldFilter', () => {
+		it('should build a parameterized filter comparing a JSONB field against a value', () => {
+			const result = buildDataFieldFilter('submitter_donor_id', 'DO-01');
+			const { sql, params } = dialect.sqlToQuery(result);
+
+			expect(sql).to.eql('data ->> $1 in ($2)');
+			expect(params).to.eql(['submitter_donor_id', 'DO-01']);
+		});
+
+		it('should bind a dataField containing SQL metacharacters as a parameter, not as SQL text', () => {
+			// Regression coverage for a stored SQL injection: dataField/dataValue here can originate
+			// from a submitter's own submitted data (see searchDataRelations.ts, viewMode.ts), so they
+			// must never be spliced into the query text.
+			const result = buildDataFieldFilter(`x' OR '1'='1`, 'a');
+			const { sql, params } = dialect.sqlToQuery(result);
+
+			expect(sql).to.eql('data ->> $1 in ($2)');
+			expect(params).to.eql([`x' OR '1'='1`, 'a']);
+		});
+
+		it('should bind a dataValue containing a statement terminator and comment marker as a parameter', () => {
+			const result = buildDataFieldFilter('submitter_donor_id', `DO-01'; DROP TABLE submitted_data; --`);
+			const { sql, params } = dialect.sqlToQuery(result);
+
+			expect(sql).to.eql('data ->> $1 in ($2)');
+			expect(params).to.eql(['submitter_donor_id', `DO-01'; DROP TABLE submitted_data; --`]);
+		});
+
+		it('should coerce an undefined dataValue to the string "undefined", matching prior behaviour', () => {
+			const result = buildDataFieldFilter('submitter_donor_id', undefined);
+			const { params } = dialect.sqlToQuery(result);
+
+			expect(params).to.eql(['submitter_donor_id', 'undefined']);
+		});
+	});
+});

packages/data-provider/test/unit/utils/convertSqonToQuery.spec.ts

@@ -1,34 +1,24 @@
 import { expect } from 'chai';
-import { SQL, SQLChunk } from 'drizzle-orm';
+import { PgDialect } from 'drizzle-orm/pg-core';
 import { describe, it } from 'mocha';
 
 import { SQON } from '@overture-stack/sqon-builder';
 
 import { convertSqonToQuery, parseSQON } from '../../../src/utils/convertSqonToQuery.js';
 
+const dialect = new PgDialect();
+
 /**
- * Function to facilitate test cases to extract array of SQL chunks from a `SQL` object
- * @param {SQL | undefined} obj
- * @param {string} key
- * @returns {SQLChunk[]}
+ * Renders a SQON to its final query text with parameters inlined, for readable test assertions.
+ * The production code path never inlines parameters; this is test-only.
  */
-function extractValues(obj: SQL | undefined, key: string): SQLChunk[] {
-	let values: SQLChunk[] = [];
-
-	function recurse(currentObj: SQL | undefined) {
-		if (Array.isArray(currentObj)) {
-			currentObj.forEach(recurse);
-		} else if (currentObj && typeof currentObj === 'object') {
-			const objectDescriptor = Object.getOwnPropertyDescriptor(currentObj, key);
-			if (objectDescriptor?.value) {
-				values = values.concat(objectDescriptor.value);
-			}
-			Object.values(currentObj).forEach(recurse);
-		}
+function toInlinedQuery(sqon: SQON | undefined): string {
+	const result = convertSqonToQuery(sqon);
+	if (!result) {
+		return '';
 	}
-
-	recurse(obj);
-	return values;
+	const { sql, params } = dialect.sqlToQuery(result);
+	return params.reduce((text: string, param) => text.replace(/\$\d+/, JSON.stringify(param)), sql);
 }
 
 describe('SQON utils', () => {
@@ -38,12 +28,9 @@ describe('SQON utils', () => {
 			content: { fieldName: 'date_of_birth', value: 197005 },
 		};
 
-		const greaterThanFilterChunk: SQLChunk[] = ["data ->> 'date_of_birth' > '197005'"];
-
 		it('should convert SQON with greater than filter into a database query', () => {
-			const result = convertSqonToQuery(sqonGreaterThanFilterParsed);
-			const extractedValues = extractValues(result, 'value');
-			expect(extractedValues).to.eql(greaterThanFilterChunk);
+			const result = toInlinedQuery(sqonGreaterThanFilterParsed);
+			expect(result).to.eql(`data ->> "date_of_birth" > "197005"`);
 		});
 	});
 
@@ -53,12 +40,28 @@ describe('SQON utils', () => {
 			content: { fieldName: 'date_of_birth', value: 197005 },
 		};
 
-		const lessThanFilterChunk: SQLChunk[] = ["data ->> 'date_of_birth' < '197005'"];
-
 		it('should convert SQON with less than filter into a database query', () => {
-			const result = convertSqonToQuery(sqonLessThanFilterParsed);
-			const extractedValues = extractValues(result, 'value');
-			expect(extractedValues).to.eql(lessThanFilterChunk);
+			const result = toInlinedQuery(sqonLessThanFilterParsed);
+			expect(result).to.eql(`data ->> "date_of_birth" < "197005"`);
+		});
+	});
+
+	describe('SQON with an "in" filter matching multiple values', () => {
+		// Regression coverage for a functional gap: every other "in" case in this file uses a
+		// single-element array, which cannot distinguish comma-expansion (one bound parameter per
+		// element, valid IN syntax) from binding the whole array as one parameter (invalid IN
+		// syntax in Postgres). Flagged by review; confirmed correct, this locks it in.
+		const sqonMultiValueInFilter: SQON = {
+			op: 'in',
+			content: { fieldName: 'player_id', value: ['NR-01', 'NR-02', 'NR-03'] },
+		};
+
+		it('binds each array element as its own parameter, not the array as a single parameter', () => {
+			const result = convertSqonToQuery(sqonMultiValueInFilter);
+			const { sql, params } = dialect.sqlToQuery(result!);
+
+			expect(sql).to.eql(`data ->> $1 in ($2, $3, $4)`);
+			expect(params).to.eql(['player_id', 'NR-01', 'NR-02', 'NR-03']);
 		});
 	});
 
@@ -81,17 +84,14 @@ describe('SQON utils', () => {
 			content: [{ op: 'in', content: { fieldName: 'player_id', value: ['NR-01'] } }],
 		};
 
-		const combinedNOTFilterChunks: SQLChunk[] = ['not ', "data ->> 'player_id' IN ('NR-01')", ''];
-
 		it('should convert a json text with NOT filter into a SQON format', () => {
 			const result = parseSQON(sqonCombinedNOTFilterRawInput);
 			expect(JSON.stringify(result)).to.eql(JSON.stringify(sqonCombinedNOTFilterParsed));
 		});
 
 		it('should convert SQON with NOT filter into a database query', () => {
-			const result = convertSqonToQuery(sqonCombinedNOTFilterParsed);
-			const extractedValues = extractValues(result, 'value');
-			expect(extractedValues).to.eql(combinedNOTFilterChunks);
+			const result = toInlinedQuery(sqonCombinedNOTFilterParsed);
+			expect(result).to.eql(`not data ->> "player_id" in ("NR-01")`);
 		});
 	});
 
@@ -124,23 +124,14 @@ describe('SQON utils', () => {
 			],
 		};
 
-		const combinedANDFilterChunks: SQLChunk[] = [
-			'(',
-			"data ->> 'player_id' IN ('NR-01')",
-			' and ',
-			"data ->> 'team_id' IN ('XYZ')",
-			')',
-		];
-
 		it('should convert a json text with AND filter into a SQON format', () => {
 			const result = parseSQON(sqonCombinedANDFilterRawInput);
 			expect(JSON.stringify(result)).to.eql(JSON.stringify(sqonCombinedANDFilterParsed));
 		});
 
 		it('should convert SQON with AND filter into a database query', () => {
-			const result = convertSqonToQuery(sqonCombinedANDFilterParsed);
-			const extractedValues = extractValues(result, 'value');
-			expect(extractedValues).to.eql(combinedANDFilterChunks);
+			const result = toInlinedQuery(sqonCombinedANDFilterParsed);
+			expect(result).to.eql(`(data ->> "player_id" in ("NR-01") and data ->> "team_id" in ("XYZ"))`);
 		});
 	});
 
@@ -173,23 +164,14 @@ describe('SQON utils', () => {
 			],
 		};
 
-		const combinedORFilterChunks: SQLChunk[] = [
-			'(',
-			"data ->> 'player_id' IN ('NR-01')",
-			' or ',
-			"data ->> 'team_id' IN ('XYZ')",
-			')',
-		];
-
 		it('should convert a json text with OR filter into a SQON format', () => {
 			const result = parseSQON(sqonCombinedORFilterRawInput);
 			expect(JSON.stringify(result)).to.eql(JSON.stringify(sqonCombinedORFilterParsed));
 		});
 
 		it('should convert SQON with OR filter into a database query', () => {
-			const result = convertSqonToQuery(sqonCombinedORFilterParsed);
-			const extractedValues = extractValues(result, 'value');
-			expect(extractedValues).to.eql(combinedORFilterChunks);
+			const result = toInlinedQuery(sqonCombinedORFilterParsed);
+			expect(result).to.eql(`(data ->> "player_id" in ("NR-01") or data ->> "team_id" in ("XYZ"))`);
 		});
 	});
 
@@ -211,4 +193,55 @@ describe('SQON utils', () => {
 			expect(parseSQON.bind(sqonInvalidFilterRawInput)).to.throw('Invalid SQON format');
 		});
 	});
+
+	describe('SQON filter values containing SQL metacharacters', () => {
+		// Regression coverage for a SQL injection: fieldName and value are user input and must
+		// always be bound as query parameters, never spliced into the generated SQL text.
+		const sqonWithInjectionAttempt: SQON = {
+			op: 'in',
+			content: { fieldName: `x' OR '1'='1`, value: ['a'] },
+		};
+
+		it('binds a fieldName containing a quote as a parameter rather than breaking out of the query', () => {
+			const result = convertSqonToQuery(sqonWithInjectionAttempt);
+			const { sql, params } = dialect.sqlToQuery(result!);
+
+			expect(sql).to.eql(`data ->> $1 in ($2)`);
+			expect(params).to.eql([`x' OR '1'='1`, 'a']);
+		});
+
+		it('renders the malicious fieldName as inert literal text, not as SQL syntax', () => {
+			const result = toInlinedQuery(sqonWithInjectionAttempt);
+			expect(result).to.eql(`data ->> "x' OR '1'='1" in ("a")`);
+		});
+
+		it('binds a fieldName containing a quote as a parameter for the gt operator too', () => {
+			// processFilterOperator builds the same jsonbField for in/gt/lt; cover a scalar operator
+			// as well since it shares the vulnerable construction, not just the array (in) branch.
+			const sqon: SQON = { op: 'gt', content: { fieldName: `x' OR '1'='1`, value: 197005 } };
+			const result = convertSqonToQuery(sqon);
+			const { sql, params } = dialect.sqlToQuery(result!);
+
+			expect(sql).to.eql(`data ->> $1 > $2`);
+			expect(params).to.eql([`x' OR '1'='1`, '197005']);
+		});
+
+		it('binds an "in" value array element containing SQL metacharacters as a parameter', () => {
+			const sqon: SQON = { op: 'in', content: { fieldName: 'player_id', value: [`NR-01' OR '1'='1`] } };
+			const result = convertSqonToQuery(sqon);
+			const { sql, params } = dialect.sqlToQuery(result!);
+
+			expect(sql).to.eql(`data ->> $1 in ($2)`);
+			expect(params).to.eql(['player_id', `NR-01' OR '1'='1`]);
+		});
+
+		it('binds a statement-terminator and comment-marker payload as an inert parameter', () => {
+			const sqon: SQON = {
+				op: 'in',
+				content: { fieldName: `x'; DROP TABLE submitted_data; --`, value: ['a'] },
+			};
+			const result = toInlinedQuery(sqon);
+			expect(result).to.eql(`data ->> "x'; DROP TABLE submitted_data; --" in ("a")`);
+		});
+	});
 });