fix: parameterize SQON-to-SQL filter values, closing a SQL injection - #219
Open
justincorrigible wants to merge 6 commits into
Open
fix: parameterize SQON-to-SQL filter values, closing a SQL injection#219justincorrigible wants to merge 6 commits into
justincorrigible wants to merge 6 commits into
Conversation
processFilterOperator spliced fieldName/value directly into sql.raw() text via an unescaping formatForSQL helper. Any SQON filter reaches this path (e.g. the organization-scoped query endpoint), so a crafted fieldName or value containing a quote could alter the WHERE clause's boolean structure, not just the intended predicate, since drizzle's outer and() wraps the whole clause group in one pair of parens without isolating each individual child condition. Bind fieldName and value through drizzle's sql template instead, which parameterizes any interpolated value rather than inlining it as text. Regression coverage added for a fieldName containing SQL metacharacters. Reported by the iMicroSeq Submission Service session.
…Filtered Same vulnerability class as fix/sqon-sql-injection's convertSqonToQuery.ts fix, found while auditing the codebase for other sql.raw() call sites with non-constant input: getSubmittedDataFiltered spliced dataField/dataValue directly into sql.raw() text with no escaping. This one is more severe than the SQON case: dataValue is not a query parameter a caller supplies at read time, it's a value read back out of a submitter's own previously-submitted record (searchDataRelations.ts, viewMode.ts, submissionProcessor.ts all resolve foreign-key relationships this way). A malicious value submitted once poisons every later query that walks that record's dictionary relationships, a stored/second-order injection rather than one confined to a single request. Extracted the filter-building into buildDataFieldFilter, which binds both values through drizzle's sql template instead of sql.raw(), and unit tested it directly (parity case, an injection-shaped dataField, and a statement-terminator/comment payload in dataValue) since the repository factory itself has no existing unit-test coverage to extend.
…e TODO The original regression test only covered a quote-injection fieldName on the "in" operator. Add: the same fieldName injection through the gt operator (proves the shared jsonbField construction is covered for scalar filters too, not just arrays), an injection-shaped element inside an "in" value array, and a combined statement-terminator/comment payload, the canonical "worst case" shape a reviewer would look for by name. Also drop parseSQON's "TODO: SQL sanitization" comment referencing lyric#43: that's what this fix (and the sibling fix in getSubmittedDataFiltered) addresses, so the TODO is now stale.
| // 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) |
Issue #43 covers three items and only one is fixed here; the other two (TSV sanitization, a broader endpoint-input audit) are separate, unstarted work. Also records the fieldName-allowlist gap noted in the PR description, so it isn't only living in a review comment that can get lost.
justincorrigible
force-pushed
the
fix/sqon-sql-injection
branch
from
August 20, 2026 17:45
2b45056 to
aab0c6c
Compare
Both fixes built the IN clause with a hand-rolled sql`${field} IN ${values}`
template instead of drizzle's own inArray() helper, which activeSubmissionRepository.ts
and submittedRepository.ts already use elsewhere for the same operation. Switched to
match, which also rejects an empty array up front instead of emitting invalid `IN ()`.
Every existing "in" test used a single-element array, which cannot distinguish
comma-expansion (one bound parameter per element, valid syntax) from binding the
whole array as one parameter (invalid IN syntax in Postgres). Verified directly
against drizzle's actual query output that comma-expansion is what happens, and
added a three-element regression test to lock it in. Flagged by review.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
processFilterOperatorspliced fieldName/value directly intosql.raw()text via an unescapingformatForSQLhelper.Any SQON filter reaches this path (e.g. the organization-scoped query endpoint), so a crafted
fieldNameor value containing a quote could alter the WHERE clause's boolean structure, not just the intended predicate, since drizzle's outerand()wraps the whole clause group in one pair of parens without isolating each individual child condition.Bind
fieldNameand value through drizzle's sql template instead, which parameterizes any interpolated value rather than inlining it as text. Regression coverage added for afieldNamecontaining SQL metacharacters.Changes cover #43
Note:
fieldNamestill has no allowlist against the dictionary's real field names, but it's harmless now that it's parameterized. worth addressing on a separate PR.Readiness Checklist
.env.schemafile and documented in the README