fix(server-utils): Sanitize SQL Server bracketed identifiers

packages/server-utils/src/integrations/knex.ts

@@ -33,7 +33,7 @@ import { DB } from '@sentry/conventions/op';
 import { DEBUG_BUILD } from '../debug-build';
 import { CHANNELS } from '../orchestrion/channels';
 import { bindTracingChannelToSpan } from '../tracing-channel';
-import { sanitizeSqlQueryWithSummary } from '../utils/sql';
+import { sanitizeSqlQueryWithSummary, toSqlDialect } from '../utils/sql';
 
 // NOTE: this uses the same name as the OTel integration by design. `@sentry/node`'s `knexIntegration`
 // picks this subscriber over the vendored OTel path when orchestrion injection is active.
@@ -174,7 +174,7 @@ function subscribeQuery(): void {
         connection?.filename || connection?.database || extractDatabaseFromConnectionString(connectionString);
       const dbSystem = mapSystem(client?.driverName);
 
-      const dialect = client?.driverName === 'mysql' || client?.driverName === 'mysql2' ? 'mysql' : undefined;
+      const dialect = toSqlDialect(client?.driverName);
       const { queryText: dbStatement, querySummary } = sanitizeSqlQueryWithSummary(
         query?.sql ? truncate(query.sql, MAX_QUERY_LENGTH) : undefined,
         dialect,

packages/server-utils/src/integrations/prisma/tracing-helper.ts

@@ -35,7 +35,7 @@ import {
   SENTRY_KIND,
   SENTRY_OP,
 } from '@sentry/conventions/attributes';
-import { getSqlQuerySummary, sanitizeSqlQuery, type SqlDialect } from '../../utils/sql';
+import { getSqlQuerySummary, sanitizeSqlQuery, type SqlDialect, toSqlDialect } from '../../utils/sql';
 
 // Reading `process.env` can throw in runtimes that gate env access (e.g. Deno without `--allow-env`)
 // and `process` may be absent altogether (edge runtimes), so this degrades to `false` in those cases.
@@ -128,10 +128,10 @@ function buildSpanAttributes(name: string, attributes: Record<string, unknown> |
  * a string literal rather than a quoted identifier, so sanitizing it as standard SQL leaves the value
  * in place — and a literal containing `FROM`/`JOIN` then reads as a table name in the summary.
  */
-function getSqlDialect(attributes: SpanAttributes): SqlDialect | undefined {
+function getSqlDialect(attributes: SpanAttributes): SqlDialect {
   // oxlint-disable-next-line typescript/no-deprecated
   const system = attributes[DB_SYSTEM_NAME] ?? attributes[DB_SYSTEM];
-  return system === 'mysql' || system === 'mariadb' ? 'mysql' : undefined;
+  return toSqlDialect(system);
 }
 
 /**

packages/server-utils/src/integrations/tedious.ts

@@ -132,7 +132,7 @@ function subscribeQuery(channelName: string, operation: string): void {
 
     const databaseName = connection[currentDatabaseSymbol];
     const sql = extractSql(request);
-    const queryText = sql ? sanitizeSqlQuery(sql) : undefined;
+    const queryText = sql ? sanitizeSqlQuery(sql, 'mssql') : undefined;
     const querySummary = queryText && operation !== 'callProcedure' ? getSqlQuerySummary(queryText) : undefined;
 
     const attributes: SpanAttributes = {

packages/server-utils/src/utils/sql.ts

@@ -149,23 +149,62 @@ function truncate(summary: string): string {
 }
 
 let integerLiteralRE: RegExp | undefined;
+let mssqlIntegerLiteralRE: RegExp | undefined;
+
+/**
+ * The lookbehind keeps `$n` and `?n` parameter placeholders out of the match. T-SQL has neither,
+ * and `$1000` there is a money literal, so `$` only guards a placeholder outside `mssql`.
+ *
+ * Lazy init: constructing a lookbehind at module scope evaluates it on import and crashes
+ * Safari <16.4 browser bundles that reach this file via the core barrel.
+ */
+function getIntegerLiteralRE(dialect: SqlDialect): RegExp {
+  if (dialect === 'mssql') {
+    if (!mssqlIntegerLiteralRE) {
+      mssqlIntegerLiteralRE = new RegExp('(?<!\\?)-?\\b\\d+\\b', 'g');
+    }
+    return mssqlIntegerLiteralRE;
+  }
+  if (!integerLiteralRE) {
+    integerLiteralRE = new RegExp('(?<![$?])-?\\b\\d+\\b', 'g');
+  }
+  return integerLiteralRE;
+}
 
 /**
  * SQL dialect variants that matter for finding the end of a string literal:
- * - `standard` (PostgreSQL, SQLite, SQL Server): `"` quotes identifiers, `''` is the only
- *   in-string escape, and PostgreSQL's `$$…$$` dollar quoting opens a literal.
+ * - `standard` (PostgreSQL, SQLite): `"` quotes identifiers, `''` is the only in-string escape,
+ *   and PostgreSQL's `$$…$$` dollar quoting opens a literal.
  * - `mysql`: `"` quotes a string literal unless `ANSI_QUOTES` is set, and `\` escapes the next
  *   character unless `NO_BACKSLASH_ESCAPES` is set. Both default to off, and mysql/mysql2 escape
  *   inlined values with backslashes, so this is the mode their statements arrive in.
+ * - `mssql`: `[...]` quotes an identifier, so a `'` inside one is part of the name. `"` quotes
+ *   one too, unless the connection sets `QUOTED_IDENTIFIER OFF`, which tedious leaves on.
+ */
+export type SqlDialect = 'standard' | 'mysql' | 'mssql';
+
+/**
+ * Maps a driver or `db.system.name` value to the dialect its statements are written in. Callers
+ * report different spellings for one engine: knex uses the driver name, Prisma the provider name,
+ * and OTel the semantic-convention name. An engine we do not know about is lexed as `standard`.
  */
-export type SqlDialect = 'standard' | 'mysql';
+export function toSqlDialect(system: unknown): SqlDialect {
+  if (system === 'mysql' || system === 'mysql2' || system === 'mariadb') {
+    return 'mysql';
+  }
+  if (system === 'mssql' || system === 'sqlserver' || system === 'microsoft.sql_server') {
+    return 'mssql';
+  }
+  return 'standard';
+}
 
 // Sticky, so the scanner can test one position without slicing the query on every `$`.
 const DOLLAR_QUOTE_RE = /\$(?:[A-Za-z_]\w*)?\$/y;
 
 /**
- * Returns the index just past the run's closing `delimiter`, or the end of the query if the run is
- * never closed — an unterminated literal must swallow the remainder rather than let it through.
+ * Returns the index just past the run's closing `delimiter`, or -1 if the run never closes. The
+ * caller cannot derive that from the index alone: a query ending in `[a]]` consumed a doubled
+ * delimiter, not a closing one.
  *
  * A doubled delimiter (`''`) escapes itself in every dialect; backslash escapes are dialect- and
  * context-dependent, so the caller decides.
@@ -182,7 +221,7 @@ function findQuotedRunEnd(sql: string, start: number, delimiter: string, backsla
       i++;
     }
   }
-  return sql.length;
+  return -1;
 }
 
 /**
@@ -219,8 +258,9 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string {
 
     // In a dollar-quoted body (`$$body$$`, `$tag$body$tag$`) nothing has syntax meaning. Read the
     // previous character from the query, not from `out`, where a dropped comment would leave `$$`
-    // looking like part of an identifier. MySQL has no dollar quoting and allows `$` in names.
-    if (!isMysql && char === '$' && !isIdentifierChar(sql[i - 1])) {
+    // looking like part of an identifier. Of the `standard` engines only PostgreSQL has dollar
+    // quoting. SQLite `$name` parameters do not match the tag pattern, apart from the rare `$a$b`.
+    if (dialect === 'standard' && char === '$' && !isIdentifierChar(sql[i - 1])) {
       const tag = matchDollarQuoteTag(sql, i);
       if (tag) {
         const bodyEnd = sql.indexOf(tag, i + tag.length);
@@ -230,20 +270,23 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string {
       }
     }
 
-    // Quoted identifiers: backticks in MySQL, double quotes everywhere else
-    if (char === '`' || (char === '"' && !isMysql)) {
-      const runEnd = findQuotedRunEnd(sql, i, char, false);
-      out += sql.slice(i, runEnd);
-      i = runEnd;
+    const identifierCloser = getIdentifierCloser(char, dialect);
+    if (identifierCloser) {
+      const runEnd = findQuotedRunEnd(sql, i, identifierCloser, false);
+      // A run that never closes is not an identifier, so it collapses instead of being copied out.
+      // Copying would carry every literal in the rest of the statement through unlexed.
+      out += runEnd === -1 ? '?' : sql.slice(i, runEnd);
+      i = runEnd === -1 ? sql.length : runEnd;
       continue;
     }
 
     if (char === "'" || (char === '"' && isMysql)) {
       // A prefix like `X'1A'`, `B'01'`, `N'…'` or PostgreSQL's `E'a\nb'` is part of the literal, so it has
       // to collapse into the same `?` instead of being left behind as a bare identifier.
-      const prefix = char === "'" ? getLiteralPrefix(out, isMysql) : undefined;
+      const prefix = char === "'" ? getLiteralPrefix(out, dialect) : undefined;
       out = prefix ? out.slice(0, -1) : out;
-      i = findQuotedRunEnd(sql, i, char, isMysql || prefix === 'E');
+      const runEnd = findQuotedRunEnd(sql, i, char, isMysql || prefix === 'E');
+      i = runEnd === -1 ? sql.length : runEnd;
       out += '?';
       continue;
     }
@@ -255,6 +298,21 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string {
   return out;
 }
 
+/**
+ * Returns the character that closes an identifier opened by `char`, or undefined if `char` opens
+ * none. SQL Server brackets close on `]`, and `]]` escapes a `]` inside the name, which is the same
+ * doubling rule `findQuotedRunEnd` applies to quotes.
+ */
+function getIdentifierCloser(char: string, dialect: SqlDialect): string | undefined {
+  if (char === '`') {
+    return '`';
+  }
+  if (char === '"' && dialect !== 'mysql') {
+    return '"';
+  }
+  return char === '[' && dialect === 'mssql' ? ']' : undefined;
+}
+
 /** Whether `char` can appear inside an identifier. `undefined` (start of query) counts as a break. */
 function isIdentifierChar(char: string | undefined): boolean {
   return char !== undefined && /[\w$]/.test(char);
@@ -271,7 +329,7 @@ function matchDollarQuoteTag(sql: string, start: number): string | undefined {
  * hex/binary literals, `N` for a national-character literal (SQL Server, MySQL), or `E` for a
  * PostgreSQL escape string (which honors backslash escapes).
  */
-function getLiteralPrefix(out: string, isMysql: boolean): 'X' | 'B' | 'N' | 'E' | undefined {
+function getLiteralPrefix(out: string, dialect: SqlDialect): 'X' | 'B' | 'N' | 'E' | undefined {
   // A prefix only counts when it stands alone — the `X` in `MAX'...'` belongs to the identifier
   if (isIdentifierChar(out.slice(-2, -1))) {
     return undefined;
@@ -281,7 +339,7 @@ function getLiteralPrefix(out: string, isMysql: boolean): 'X' | 'B' | 'N' | 'E'
   if (prefix === 'X' || prefix === 'B' || prefix === 'N') {
     return prefix;
   }
-  return prefix === 'E' && !isMysql ? 'E' : undefined;
+  return prefix === 'E' && dialect === 'standard' ? 'E' : undefined;
 }
 
 /**
@@ -300,13 +358,6 @@ export function sanitizeSqlQuery(sqlQuery: string | undefined, dialect: SqlDiale
     return 'Unknown SQL Query';
   }
 
-  // Lazy init: constructing this at module scope would evaluate the lookbehind
-  // on import and crash Safari <16.4 browser bundles that reach this file via
-  // the core barrel. Building it on first call keeps the cost off the import path.
-  if (!integerLiteralRE) {
-    integerLiteralRE = new RegExp('(?<![$?])-?\\b\\d+\\b', 'g');
-  }
-
   return (
     // Strip comments and string literals first: everything below is a regex that cannot tell
     // whether it is looking at SQL syntax or at a user-supplied value.
@@ -323,7 +374,7 @@ export function sanitizeSqlQuery(sqlQuery: string | undefined, dialect: SqlDiale
       .replace(/-?\b\d+\.?\d*[eE][+-]?\d+\b/g, '?') // Scientific notation
       .replace(/-?\b\d+\.\d+\b/g, '?') // Decimals
       .replace(/-?\.\d+\b/g, '?') // Decimals starting with dot
-      .replace(integerLiteralRE, '?') // Integers (NOT $n placeholders)
+      .replace(getIntegerLiteralRE(dialect), '?') // Integers (NOT parameter placeholders)
       // Collapse IN clauses for cardinality (both ? and $n variants)
       .replace(/\bIN\b\s*\(\s*\?(?:\s*,\s*\?)*\s*\)/gi, 'IN (?)')
       .replace(/\bIN\b\s*\(\s*\$\d+(?:\s*,\s*\$\d+)*\s*\)/gi, 'IN ($?)')

packages/server-utils/test/utils/sql.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest';
-import { getSqlQuerySummary, sanitizeSqlQuery, sanitizeSqlQueryWithSummary } from '../../src/utils/sql';
+import { getSqlQuerySummary, sanitizeSqlQuery, sanitizeSqlQueryWithSummary, toSqlDialect } from '../../src/utils/sql';
 
 describe('getSqlQuerySummary', () => {
   it.each([undefined, ''])('returns undefined for %j', input => {
@@ -679,6 +679,26 @@ describe('sanitizeSqlQuery', () => {
     });
   });
 
+  describe("dialect: 'mssql'", () => {
+    it.each([
+      // `[...]` quotes an identifier, so a `'` inside one belongs to the name
+      ["SELECT * FROM [dbo].[user's] WHERE email = 'jane@example.com'", "SELECT * FROM [dbo].[user's] WHERE email = ?"],
+      // `]]` escapes a `]` inside the name
+      ['SELECT [a]]b] FROM [t] WHERE c = 1', 'SELECT [a]]b] FROM [t] WHERE c = ?'],
+      ['SELECT * FROM [dbo].[Customer Orders] WHERE id = @P1', 'SELECT * FROM [dbo].[Customer Orders] WHERE id = @P1'],
+      ["INSERT INTO [dbo].[users] ([name]) VALUES (N'Jane')", 'INSERT INTO [dbo].[users] ([name]) VALUES (?)'],
+      // T-SQL has no `E'...'` escape strings, so the `E` stays an identifier
+      ["SELECT * FROM t WHERE a = E'x'", 'SELECT * FROM t WHERE a = E?'],
+      // ... and no dollar quoting, where `$` is an ordinary identifier character
+      ['SELECT * FROM t WHERE a = $$x$$', 'SELECT * FROM t WHERE a = $$x$$'],
+      // `$n` is a money literal here, not the placeholder it is in PostgreSQL
+      ['SELECT * FROM t WHERE price = $1000', 'SELECT * FROM t WHERE price = $?'],
+      ['SELECT * FROM t WHERE price = $10.50', 'SELECT * FROM t WHERE price = $?'],
+    ])('sanitizes %p', (input, expected) => {
+      expect(sanitizeSqlQuery(input, 'mssql')).toBe(expected);
+    });
+  });
+
   describe('unterminated literals swallow the rest of the statement', () => {
     it.each([
       ["SELECT * FROM t WHERE a = 'jane@example.com AND b = 2", 'standard' as const],
@@ -689,6 +709,25 @@ describe('sanitizeSqlQuery', () => {
     ])('drops the unterminated value in %p (%s)', (input, dialect) => {
       expect(sanitizeSqlQuery(input, dialect)).toBe('SELECT * FROM t WHERE a = ?');
     });
+
+    // An unterminated identifier quote is the same hazard: the rest of the statement is not a
+    // name, so copying it through would carry the literals in it out unlexed.
+    it.each([
+      ["SELECT * FROM [users WHERE email = 'jane@example.com'", 'mssql' as const],
+      ["SELECT * FROM \"users WHERE email = 'jane@example.com'", 'standard' as const],
+      ["SELECT * FROM `users WHERE email = 'jane@example.com'", 'mysql' as const],
+    ])('drops the unterminated identifier in %p (%s)', (input, dialect) => {
+      expect(sanitizeSqlQuery(input, dialect)).toBe('SELECT * FROM ?');
+    });
+
+    // The run ends on a doubled closer, which escapes the character rather than closing the name
+    it.each([
+      ["SELECT * FROM [t WHERE email = 'jane@example.com' AND x = [a]]", 'mssql' as const],
+      ['SELECT * FROM "t WHERE email = \'jane@example.com\' AND x = a""', 'standard' as const],
+      ["SELECT * FROM `t WHERE email = 'jane@example.com' AND x = a``", 'mysql' as const],
+    ])('drops an identifier left open by an escaped closer in %p (%s)', (input, dialect) => {
+      expect(sanitizeSqlQuery(input, dialect)).toBe('SELECT * FROM ?');
+    });
   });
 
   describe('representative statements per driver', () => {
@@ -768,7 +807,7 @@ describe('sanitizeSqlQuery', () => {
         'INSERT INTO users (name, email) VALUES (?, ?)',
       ],
     ])('sanitizes SQL Server statement %p', (input, expected) => {
-      expect(sanitizeSqlQuery(input)).toBe(expected);
+      expect(sanitizeSqlQuery(input, 'mssql')).toBe(expected);
     });
   });
 
@@ -787,6 +826,7 @@ describe('sanitizeSqlQuery', () => {
       ['standard' as const, 'INSERT INTO t (c) VALUES ($tag$select from s3cret-token$tag$)', 's3cret-token'],
       ['standard' as const, "SELECT * FROM users WHERE name = N'from ACME'", 'ACME'],
       ['standard' as const, String.raw`UPDATE t SET a = E'x\'y from Z' WHERE id = 5`, 'from Z'],
+      ['mssql' as const, "SELECT * FROM [dbo].[users] WHERE note = 'from bob@secret.com'", 'bob@secret.com'],
     ])('strips the value out of %s statement %p', (dialect, input, value) => {
       const sanitized = sanitizeSqlQuery(input, dialect);
       expect(sanitized).not.toContain(value);
@@ -795,6 +835,44 @@ describe('sanitizeSqlQuery', () => {
   });
 });
 
+describe('sanitizeSqlQueryWithSummary', () => {
+  it('returns the sanitized statement and its summary', () => {
+    expect(sanitizeSqlQueryWithSummary("SELECT * FROM users WHERE email = 'jane@example.com'")).toEqual({
+      queryText: 'SELECT * FROM users WHERE email = ?',
+      querySummary: 'SELECT users',
+    });
+  });
+
+  it('passes the dialect through to the sanitizer', () => {
+    expect(sanitizeSqlQueryWithSummary('SELECT * FROM users WHERE email = "jane@example.com"', 'mysql')).toEqual({
+      queryText: 'SELECT * FROM users WHERE email = ?',
+      querySummary: 'SELECT users',
+    });
+  });
+
+  it('returns undefined for both when there is no statement', () => {
+    expect(sanitizeSqlQueryWithSummary(undefined)).toEqual({ queryText: undefined, querySummary: undefined });
+    expect(sanitizeSqlQueryWithSummary('')).toEqual({ queryText: undefined, querySummary: undefined });
+  });
+});
+
+describe('toSqlDialect', () => {
+  it.each([
+    ['mysql', 'mysql'],
+    ['mysql2', 'mysql'],
+    ['mariadb', 'mysql'],
+    ['mssql', 'mssql'],
+    ['sqlserver', 'mssql'],
+    ['microsoft.sql_server', 'mssql'],
+  ])('maps %j to %j', (system, expected) => {
+    expect(toSqlDialect(system)).toBe(expected);
+  });
+
+  it.each(['postgresql', 'sqlite', 'oracle', '', undefined])('falls back to standard for %j', system => {
+    expect(toSqlDialect(system)).toBe('standard');
+  });
+});
+
 describe('sanitizeSqlQueryWithSummary', () => {
   it('returns the sanitized statement and its summary', () => {
     expect(sanitizeSqlQueryWithSummary("SELECT * FROM users WHERE email = 'jane@example.com'")).toEqual({
@@ -815,6 +893,13 @@ describe('sanitizeSqlQueryWithSummary', () => {
     });
   });
 
+  it('passes the mssql dialect through, so a quote inside a bracketed name stays part of the name', () => {
+    expect(sanitizeSqlQueryWithSummary("SELECT * FROM [users] WHERE [email] = 'jane@example.com'", 'mssql')).toEqual({
+      queryText: 'SELECT * FROM [users] WHERE [email] = ?',
+      querySummary: 'SELECT [users]',
+    });
+  });
+
   it('derives the summary from the sanitized statement, not the raw one', () => {
     expect(sanitizeSqlQueryWithSummary("SELECT * FROM users WHERE bio = 'from secret_table'")).toEqual({
       queryText: 'SELECT * FROM users WHERE bio = ?',