Skip to content

Commit efd789d

Browse files
committed
Sanitize / parameterize queries better
1 parent 11a366d commit efd789d

File tree

3 files changed

+278
-3
lines changed

3 files changed

+278
-3
lines changed

dev-packages/node-integration-tests/suites/tracing/postgresjs/test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,29 @@ describe('postgresjs auto instrumentation', () => {
117117
timestamp: expect.any(Number),
118118
trace_id: expect.any(String),
119119
}),
120+
// Parameterized query test - verifies that tagged template queries with interpolations
121+
// are properly reconstructed with $1, $2 placeholders then sanitized to ?
122+
expect.objectContaining({
123+
data: expect.objectContaining({
124+
'db.namespace': 'test_db',
125+
'db.system.name': 'postgres',
126+
'db.operation.name': 'SELECT',
127+
'db.query.text': `SELECT * FROM "User" WHERE "email" = ? AND "name" = ?`,
128+
'sentry.op': 'db',
129+
'sentry.origin': 'auto.db.postgresjs',
130+
'server.address': 'localhost',
131+
'server.port': 5444,
132+
}),
133+
description: `SELECT * FROM "User" WHERE "email" = ? AND "name" = ?`,
134+
op: 'db',
135+
status: 'ok',
136+
origin: 'auto.db.postgresjs',
137+
parent_span_id: expect.any(String),
138+
span_id: expect.any(String),
139+
start_timestamp: expect.any(Number),
140+
timestamp: expect.any(Number),
141+
trace_id: expect.any(String),
142+
}),
120143
expect.objectContaining({
121144
data: expect.objectContaining({
122145
'db.namespace': 'test_db',
@@ -312,6 +335,29 @@ describe('postgresjs auto instrumentation', () => {
312335
timestamp: expect.any(Number),
313336
trace_id: expect.any(String),
314337
}),
338+
// Parameterized query test - verifies that tagged template queries with interpolations
339+
// are properly reconstructed with $1, $2 placeholders then sanitized to ?
340+
expect.objectContaining({
341+
data: expect.objectContaining({
342+
'db.namespace': 'test_db',
343+
'db.system.name': 'postgres',
344+
'db.operation.name': 'SELECT',
345+
'db.query.text': `SELECT * FROM "User" WHERE "email" = ? AND "name" = ?`,
346+
'sentry.op': 'db',
347+
'sentry.origin': 'auto.db.postgresjs',
348+
'server.address': 'localhost',
349+
'server.port': 5444,
350+
}),
351+
description: `SELECT * FROM "User" WHERE "email" = ? AND "name" = ?`,
352+
op: 'db',
353+
status: 'ok',
354+
origin: 'auto.db.postgresjs',
355+
parent_span_id: expect.any(String),
356+
span_id: expect.any(String),
357+
start_timestamp: expect.any(Number),
358+
timestamp: expect.any(Number),
359+
trace_id: expect.any(String),
360+
}),
315361
expect.objectContaining({
316362
data: expect.objectContaining({
317363
'db.namespace': 'test_db',

packages/node/src/integrations/tracing/postgresjs.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,8 @@ export class PostgresJsInstrumentation extends InstrumentationBase<PostgresJsIns
388388
return originalHandle.apply(this, args);
389389
}
390390

391-
const sanitizedSqlQuery = self._sanitizeSqlQuery(query.strings?.[0]);
391+
const fullQuery = self._reconstructQuery(query.strings);
392+
const sanitizedSqlQuery = self._sanitizeSqlQuery(fullQuery);
392393

393394
return startSpanManual(
394395
{
@@ -494,6 +495,24 @@ export class PostgresJsInstrumentation extends InstrumentationBase<PostgresJsIns
494495
return hasParentSpan || !config.requireParentSpan;
495496
}
496497

498+
/**
499+
* Reconstructs the full SQL query from template strings with PostgreSQL placeholders.
500+
*
501+
* For sql`SELECT * FROM users WHERE id = ${123} AND name = ${'foo'}`:
502+
* strings = ["SELECT * FROM users WHERE id = ", " AND name = ", ""]
503+
* returns: "SELECT * FROM users WHERE id = $1 AND name = $2"
504+
*/
505+
private _reconstructQuery(strings: string[] | undefined): string | undefined {
506+
if (!strings?.length) {
507+
return undefined;
508+
}
509+
if (strings.length === 1) {
510+
return strings[0] || undefined;
511+
}
512+
// Join template parts with PostgreSQL placeholders ($1, $2, etc.)
513+
return strings.reduce((acc, str, i) => (i === 0 ? str : `${acc}$${i}${str}`), '');
514+
}
515+
497516
/**
498517
* Sanitize SQL query as per the OTEL semantic conventions
499518
* https://opentelemetry.io/docs/specs/semconv/database/database-spans/#sanitization-of-dbquerytext
@@ -558,7 +577,8 @@ export class PostgresJsInstrumentation extends InstrumentationBase<PostgresJsIns
558577
return originalHandle.apply(this, args);
559578
}
560579

561-
const sanitizedSqlQuery = self._sanitizeSqlQuery(this.strings?.[0]);
580+
const fullQuery = self._reconstructQuery(this.strings);
581+
const sanitizedSqlQuery = self._sanitizeSqlQuery(fullQuery);
562582

563583
return startSpanManual(
564584
{

packages/node/test/integrations/tracing/postgresjs.test.ts

Lines changed: 210 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,217 @@ import { describe, expect, it } from 'vitest';
22
import { PostgresJsInstrumentation } from '../../../src/integrations/tracing/postgresjs';
33

44
describe('PostgresJs', () => {
5+
const instrumentation = new PostgresJsInstrumentation({ requireParentSpan: true });
6+
7+
describe('_reconstructQuery', () => {
8+
const reconstruct = (strings: string[] | undefined) =>
9+
(
10+
instrumentation as unknown as { _reconstructQuery: (s: string[] | undefined) => string | undefined }
11+
)._reconstructQuery(strings);
12+
13+
describe('undefined/null/empty input handling', () => {
14+
it('returns undefined for undefined input', () => {
15+
expect(reconstruct(undefined)).toBeUndefined();
16+
});
17+
18+
it('returns undefined for null input', () => {
19+
expect(reconstruct(null as unknown as undefined)).toBeUndefined();
20+
});
21+
22+
it('returns undefined for empty array', () => {
23+
expect(reconstruct([])).toBeUndefined();
24+
});
25+
26+
it('returns undefined for array with single empty string', () => {
27+
expect(reconstruct([''])).toBeUndefined();
28+
});
29+
30+
it('returns undefined for whitespace-only single element', () => {
31+
// Whitespace-only strings are truthy, so they should be returned
32+
expect(reconstruct([' '])).toBe(' ');
33+
});
34+
});
35+
36+
describe('single-element array (non-parameterized queries)', () => {
37+
it('returns the string as-is for a single-element array', () => {
38+
expect(reconstruct(['SELECT * FROM users'])).toBe('SELECT * FROM users');
39+
});
40+
41+
it('handles sql.unsafe() style queries', () => {
42+
expect(reconstruct(['SELECT * FROM users WHERE id = $1'])).toBe('SELECT * FROM users WHERE id = $1');
43+
});
44+
45+
it('handles complex single-element queries', () => {
46+
expect(reconstruct(['INSERT INTO users (email, name) VALUES ($1, $2)'])).toBe(
47+
'INSERT INTO users (email, name) VALUES ($1, $2)',
48+
);
49+
});
50+
});
51+
52+
describe('multi-element array (parameterized queries)', () => {
53+
it('reconstructs query with single parameter', () => {
54+
// sql`SELECT * FROM users WHERE id = ${123}`
55+
// strings = ["SELECT * FROM users WHERE id = ", ""]
56+
expect(reconstruct(['SELECT * FROM users WHERE id = ', ''])).toBe('SELECT * FROM users WHERE id = $1');
57+
});
58+
59+
it('reconstructs query with two parameters', () => {
60+
// sql`SELECT * FROM users WHERE id = ${123} AND name = ${'foo'}`
61+
// strings = ["SELECT * FROM users WHERE id = ", " AND name = ", ""]
62+
expect(reconstruct(['SELECT * FROM users WHERE id = ', ' AND name = ', ''])).toBe(
63+
'SELECT * FROM users WHERE id = $1 AND name = $2',
64+
);
65+
});
66+
67+
it('reconstructs query with three parameters', () => {
68+
// sql`INSERT INTO users (id, name, email) VALUES (${1}, ${'John'}, ${'john@example.com'})`
69+
expect(reconstruct(['INSERT INTO users (id, name, email) VALUES (', ', ', ', ', ')'])).toBe(
70+
'INSERT INTO users (id, name, email) VALUES ($1, $2, $3)',
71+
);
72+
});
73+
74+
it('reconstructs query with parameter at the beginning', () => {
75+
// sql`${tableName} WHERE id = ${123}`
76+
// strings = ["", " WHERE id = ", ""]
77+
expect(reconstruct(['', ' WHERE id = ', ''])).toBe('$1 WHERE id = $2');
78+
});
79+
80+
it('reconstructs complex query with multiple parameters', () => {
81+
// sql`SELECT * FROM ${table} WHERE id = ${id} AND status IN (${s1}, ${s2}) ORDER BY ${col}`
82+
expect(reconstruct(['SELECT * FROM ', ' WHERE id = ', ' AND status IN (', ', ', ') ORDER BY ', ''])).toBe(
83+
'SELECT * FROM $1 WHERE id = $2 AND status IN ($3, $4) ORDER BY $5',
84+
);
85+
});
86+
});
87+
88+
describe('edge cases', () => {
89+
it('handles whitespace-only strings in array', () => {
90+
expect(reconstruct(['SELECT * FROM users WHERE id = ', ' ', ''])).toBe(
91+
'SELECT * FROM users WHERE id = $1 $2',
92+
);
93+
});
94+
95+
it('handles query ending without trailing empty string', () => {
96+
// Some edge cases might not have trailing empty string
97+
expect(reconstruct(['SELECT * FROM users WHERE id = ', ' LIMIT 10'])).toBe(
98+
'SELECT * FROM users WHERE id = $1 LIMIT 10',
99+
);
100+
});
101+
102+
it('handles many parameters (10+)', () => {
103+
// sql`INSERT INTO t VALUES (${a}, ${b}, ${c}, ${d}, ${e}, ${f}, ${g}, ${h}, ${i}, ${j})`
104+
// 10 params need 11 string parts: prefix + 9 separators + suffix
105+
const strings = ['INSERT INTO t VALUES (', ', ', ', ', ', ', ', ', ', ', ', ', ', ', ', ', ', ', ')'];
106+
expect(reconstruct(strings)).toBe('INSERT INTO t VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)');
107+
});
108+
109+
it('handles newlines in template strings', () => {
110+
// sql`SELECT *\nFROM users\nWHERE id = ${123}`
111+
expect(reconstruct(['SELECT *\nFROM users\nWHERE id = ', ''])).toBe('SELECT *\nFROM users\nWHERE id = $1');
112+
});
113+
114+
it('handles unicode characters', () => {
115+
expect(reconstruct(['SELECT * FROM users WHERE name = ', ' AND emoji = ', ''])).toBe(
116+
'SELECT * FROM users WHERE name = $1 AND emoji = $2',
117+
);
118+
});
119+
120+
it('handles quotes in template strings', () => {
121+
// sql`SELECT * FROM "User" WHERE "email" = ${email}`
122+
expect(reconstruct(['SELECT * FROM "User" WHERE "email" = ', ''])).toBe(
123+
'SELECT * FROM "User" WHERE "email" = $1',
124+
);
125+
});
126+
127+
it('handles consecutive parameters', () => {
128+
// sql`SELECT ${a}${b}${c}`
129+
expect(reconstruct(['SELECT ', '', '', ''])).toBe('SELECT $1$2$3');
130+
});
131+
132+
it('handles parameter only query', () => {
133+
// sql`${rawSql}` - just a single parameter
134+
expect(reconstruct(['', ''])).toBe('$1');
135+
});
136+
});
137+
138+
describe('integration with _sanitizeSqlQuery', () => {
139+
const sanitize = (query: string | undefined) =>
140+
(instrumentation as unknown as { _sanitizeSqlQuery: (q: string | undefined) => string })._sanitizeSqlQuery(
141+
query,
142+
);
143+
144+
it('reconstructed query gets properly sanitized', () => {
145+
// Full flow: reconstruct then sanitize
146+
const strings = ['SELECT * FROM users WHERE id = ', ' AND name = ', ''];
147+
const reconstructed = reconstruct(strings);
148+
const sanitized = sanitize(reconstructed);
149+
expect(sanitized).toBe('SELECT * FROM users WHERE id = ? AND name = ?');
150+
});
151+
152+
it('handles complex parameterized query end-to-end', () => {
153+
const strings = ['SELECT * FROM users WHERE id = ', ' AND status IN (', ', ', ', ', ')'];
154+
const reconstructed = reconstruct(strings);
155+
const sanitized = sanitize(reconstructed);
156+
expect(sanitized).toBe('SELECT * FROM users WHERE id = ? AND status IN (?)');
157+
});
158+
159+
it('handles undefined strings array gracefully in full flow', () => {
160+
const reconstructed = reconstruct(undefined);
161+
const sanitized = sanitize(reconstructed);
162+
expect(sanitized).toBe('Unknown SQL Query');
163+
});
164+
165+
it('handles INSERT with parameterized values', () => {
166+
// sql`INSERT INTO users (email, name) VALUES (${email}, ${name})`
167+
const strings = ['INSERT INTO users (email, name) VALUES (', ', ', ')'];
168+
const reconstructed = reconstruct(strings);
169+
const sanitized = sanitize(reconstructed);
170+
expect(sanitized).toBe('INSERT INTO users (email, name) VALUES (?, ?)');
171+
});
172+
173+
it('handles UPDATE with parameterized values', () => {
174+
// sql`UPDATE users SET name = ${name} WHERE id = ${id}`
175+
const strings = ['UPDATE users SET name = ', ' WHERE id = ', ''];
176+
const reconstructed = reconstruct(strings);
177+
const sanitized = sanitize(reconstructed);
178+
expect(sanitized).toBe('UPDATE users SET name = ? WHERE id = ?');
179+
});
180+
181+
it('handles DELETE with parameterized values', () => {
182+
// sql`DELETE FROM users WHERE id = ${id}`
183+
const strings = ['DELETE FROM users WHERE id = ', ''];
184+
const reconstructed = reconstruct(strings);
185+
const sanitized = sanitize(reconstructed);
186+
expect(sanitized).toBe('DELETE FROM users WHERE id = ?');
187+
});
188+
189+
it('handles query with newlines that get normalized', () => {
190+
// sql`SELECT *\n FROM users\n WHERE id = ${id}`
191+
const strings = ['SELECT *\n FROM users\n WHERE id = ', ''];
192+
const reconstructed = reconstruct(strings);
193+
const sanitized = sanitize(reconstructed);
194+
expect(sanitized).toBe('SELECT * FROM users WHERE id = ?');
195+
});
196+
197+
it('handles query with trailing semicolon', () => {
198+
// sql`SELECT * FROM users WHERE id = ${id};`
199+
const strings = ['SELECT * FROM users WHERE id = ', ';'];
200+
const reconstructed = reconstruct(strings);
201+
const sanitized = sanitize(reconstructed);
202+
expect(sanitized).toBe('SELECT * FROM users WHERE id = ?');
203+
});
204+
205+
it('handles real-world postgres.js query pattern', () => {
206+
// Actual pattern from postgres.js: sql`SELECT * FROM "User" WHERE "email" = ${email} AND "name" = ${name}`
207+
const strings = ['SELECT * FROM "User" WHERE "email" = ', ' AND "name" = ', ''];
208+
const reconstructed = reconstruct(strings);
209+
const sanitized = sanitize(reconstructed);
210+
expect(sanitized).toBe('SELECT * FROM "User" WHERE "email" = ? AND "name" = ?');
211+
});
212+
});
213+
});
214+
5215
describe('_sanitizeSqlQuery', () => {
6-
const instrumentation = new PostgresJsInstrumentation({ requireParentSpan: true });
7216
const sanitize = (query: string | undefined) =>
8217
(instrumentation as unknown as { _sanitizeSqlQuery: (q: string | undefined) => string })._sanitizeSqlQuery(query);
9218

0 commit comments

Comments
 (0)