Skip to content

Commit ca567c8

Browse files
committed
fix(mssql): compose the shared WHERE guard and close the semicolon-less batch gap
The local validateWhereClause re-derived an older copy of the shared patterns and scanned raw text, so it missed a bare 1=1 and false-positived on prose in a quoted value. Delegate to validateSqlWhereClause, which masks string literals first, and keep only the SQL Server surfaces it has no reason to know about. T-SQL needs no statement terminator, so every semicolon-anchored stacked-query check reads straight past `id = 1 DROP TABLE dbo.users`. Screen for a bare statement-introducing keyword to close that; word boundaries leave ordinary column names like updated_at and deleted_at untouched. Export maskSqlStringLiterals so the dialect layer masks the same way the shared guard does rather than carrying a weaker single-quote-only copy.
1 parent a60715a commit ca567c8

2 files changed

Lines changed: 65 additions & 62 deletions

File tree

apps/sim/app/api/tools/mssql/utils.ts

Lines changed: 64 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import net from 'node:net'
22
import sql from 'mssql'
3-
import { validateDatabaseHost } from '@/lib/core/security/input-validation.server'
3+
import {
4+
maskSqlStringLiterals,
5+
validateDatabaseHost,
6+
validateSqlWhereClause,
7+
} from '@/lib/core/security/input-validation.server'
48

59
export interface MSSQLConnectionConfig {
610
host: string
@@ -128,19 +132,6 @@ export async function executeQuery(
128132
}
129133
}
130134

131-
/**
132-
* Strips single-quoted string literals so keyword screening reads only code.
133-
*
134-
* T-SQL escapes a quote by doubling it, so `'it''s'` is one literal rather than
135-
* two — matching that form is what keeps the scanner from resynchronizing on
136-
* the wrong quote and exposing the rest of the statement as if it were a
137-
* literal.
138-
* @see https://learn.microsoft.com/en-us/sql/t-sql/data-types/constants-transact-sql
139-
*/
140-
function stripStringLiterals(query: string): string {
141-
return query.replace(/'(?:[^']|'')*'/g, "''")
142-
}
143-
144135
/**
145136
* Restricts the Query operation to statements that only read.
146137
*
@@ -152,8 +143,9 @@ function stripStringLiterals(query: string): string {
152143
* A leading `WITH` is admitted because a CTE is the normal way to write a
153144
* non-trivial SELECT, but T-SQL also allows `WITH x AS (...) DELETE FROM x`, so
154145
* the body is screened for mutating keywords rather than trusting the leading
155-
* token alone. Screening runs over the statement with string literals removed,
156-
* which keeps ordinary prose in a WHERE clause from tripping it.
146+
* token alone. Screening runs over the statement with string literals masked by
147+
* the shared {@link maskSqlStringLiterals}, which keeps ordinary prose in a
148+
* WHERE clause from tripping it.
157149
*
158150
* The screen is lexical, not a parser, so it can still reject a legitimate
159151
* query that uses a keyword as a bare identifier. It fails closed on purpose,
@@ -172,7 +164,7 @@ export function validateReadOnlyQuery(query: string): { isValid: boolean; error?
172164

173165
const mutating =
174166
/\b(insert|update|delete|merge|drop|create|alter|truncate|grant|revoke|exec|execute|into)\b/i.exec(
175-
stripStringLiterals(trimmedQuery)
167+
maskSqlStringLiterals(trimmedQuery)
176168
)
177169
if (mutating) {
178170
return {
@@ -235,52 +227,63 @@ export function buildDeleteQuery(table: string, where: string) {
235227
}
236228

237229
/**
238-
* Validates a WHERE clause to prevent SQL injection attacks
239-
* @param where - The WHERE clause string to validate
240-
* @throws {Error} If the WHERE clause contains potentially dangerous patterns
230+
* T-SQL-specific WHERE screening layered on top of the shared guard.
231+
*
232+
* The first pattern is the one that does not generalize: **T-SQL does not
233+
* require a statement terminator**, so `id = 1 DROP TABLE dbo.users` is a valid
234+
* two-statement batch and every semicolon-anchored stacked-query check — the
235+
* shared guard's included — reads straight past it. Screening for a bare
236+
* statement-introducing keyword is what closes that. Word boundaries keep
237+
* ordinary column names (`updated_at`, `deleted_at`, `created_by`) matching
238+
* nothing; a column whose name *is* a bare keyword must be reached through the
239+
* Execute Raw SQL operation instead.
240+
*
241+
* The rest are SQL Server surfaces the shared guard has no reason to know
242+
* about: the `OPEN*` rowset functions, `BULK INSERT`, `WAITFOR` timing probes,
243+
* catalog and legacy compatibility views (`master..sysobjects`), and the
244+
* extended/OLE-automation procedures.
245+
* @see https://learn.microsoft.com/en-us/sql/t-sql/language-elements/transact-sql-syntax-conventions-transact-sql
246+
* @see https://learn.microsoft.com/en-us/sql/relational-databases/system-catalog-views/catalog-views-transact-sql
247+
*/
248+
const MSSQL_WHERE_PATTERNS: readonly RegExp[] = [
249+
/\b(?:drop|create|alter|truncate|grant|revoke|insert|update|delete|merge|exec|execute|backup|restore|shutdown|reconfigure)\b/i,
250+
/\bopenrowset\s*\(/i,
251+
/\bopendatasource\s*\(/i,
252+
/\bopenquery\s*\(/i,
253+
/\bopenxml\s*\(/i,
254+
/\bbulk\s+insert\b/i,
255+
/\bwaitfor\s+(?:delay|time)\b/i,
256+
/information_schema/i,
257+
/\bsys\./i,
258+
/\.\.\s*sys\w*/i,
259+
/\bsys(?:objects|columns|databases|users|indexes|comments)\b/i,
260+
/\bxp_\w+/i,
261+
/\bsp_(?:executesql|oacreate|oamethod|oagetproperty|configure|addextendedproc)\b/i,
262+
]
263+
264+
/**
265+
* Rejects WHERE clauses containing injection or always-true tautology patterns
266+
* so a user-supplied condition cannot broaden an update or delete to every row.
267+
*
268+
* Delegates the shared checks to {@link validateSqlWhereClause} — which masks
269+
* string literals before scanning, so prose inside a quoted value cannot trip a
270+
* structural pattern — then applies the T-SQL-specific screening above.
271+
*
272+
* As the shared guard's own documentation states, this is defense-in-depth
273+
* rather than a security boundary: the caller supplies their own database
274+
* credentials and can run equivalent SQL through the Execute Raw SQL operation.
275+
* It stops the easy ways an injected condition escalates, nothing more.
276+
* @throws {Error} If the WHERE clause matches any screened pattern
241277
*/
242278
function validateWhereClause(where: string): void {
243-
const dangerousPatterns = [
244-
// DDL and DML injection via stacked queries
245-
/;\s*(drop|delete|insert|update|create|alter|grant|revoke|truncate)/i,
246-
// Union-based injection
247-
/union\s+(all\s+)?select/i,
248-
// File and external data operations
249-
/\bopenrowset\s*\(/i,
250-
/\bopendatasource\s*\(/i,
251-
/\bopenquery\s*\(/i,
252-
/\bopenxml\s*\(/i,
253-
/\bbulk\s+insert\b/i,
254-
// Comment-based injection (can truncate query)
255-
/--/,
256-
/\/\*/,
257-
/\*\//,
258-
// Tautologies - always true/false conditions using backreferences
259-
/\bor\s+(['"]?)(\w+)\1\s*=\s*\1\2\1/i,
260-
/\bor\s+true\b/i,
261-
/\bor\s+false\b/i,
262-
/\band\s+(['"]?)(\w+)\1\s*=\s*\1\2\1/i,
263-
/\band\s+true\b/i,
264-
/\band\s+false\b/i,
265-
// Time-based blind injection
266-
/\bwaitfor\s+(delay|time)\b/i,
267-
// Stacked queries (any statement after semicolon)
268-
/;\s*\w+/,
269-
// Information schema / system catalog queries, including the legacy
270-
// compatibility views reachable as `master..sysobjects`
271-
/information_schema/i,
272-
/\bsys\./i,
273-
/\.\.\s*sys\w*/i,
274-
/\bsys(objects|columns|databases|users|indexes|comments)\b/i,
275-
// Extended and OLE-automation stored procedures
276-
/\bxp_\w+/i,
277-
/\bsp_(executesql|oacreate|oamethod|oagetproperty|configure|addextendedproc)\b/i,
278-
]
279-
280-
for (const pattern of dangerousPatterns) {
281-
if (pattern.test(where)) {
282-
throw new Error('WHERE clause contains potentially dangerous operation')
283-
}
279+
const shared = validateSqlWhereClause(where, 'WHERE clause')
280+
if (!shared.isValid) {
281+
throw new Error(shared.error)
282+
}
283+
284+
const masked = maskSqlStringLiterals(where)
285+
if (MSSQL_WHERE_PATTERNS.some((pattern) => pattern.test(masked))) {
286+
throw new Error('WHERE clause contains potentially dangerous operation')
284287
}
285288
}
286289

apps/sim/lib/core/security/input-validation.server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ const SQL_WHERE_RAW_PATTERNS: readonly RegExp[] = [
284284
* scans do not treat data inside quotes as SQL. Comments are intentionally left
285285
* intact so comment-injection sequences are still detected.
286286
*/
287-
function maskSqlStringLiterals(sql: string): string {
287+
export function maskSqlStringLiterals(sql: string): string {
288288
let out = ''
289289
let i = 0
290290
while (i < sql.length) {

0 commit comments

Comments
 (0)