Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,12 @@ describe('mysql auto instrumentation', () => {
type: 'string',
value: 'SELECT 1 + 1 AS solution',
},
'db.query.summary': {
type: 'string',
value: 'SELECT',
},
},
name: 'SELECT 1 + 1 AS solution',
name: 'SELECT',
...COMMON_SPAN_PROPS,
},
{
Expand All @@ -274,8 +278,12 @@ describe('mysql auto instrumentation', () => {
type: 'string',
value: 'SELECT NOW()',
},
'db.query.summary': {
type: 'string',
value: 'SELECT',
},
},
name: 'SELECT NOW()',
name: 'SELECT',
...COMMON_SPAN_PROPS,
},
]);
Expand Down
20 changes: 18 additions & 2 deletions packages/server-utils/src/integrations/mysql.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as diagnosticsChannel from 'node:diagnostics_channel';
import {
DB_NAMESPACE,
DB_QUERY_SUMMARY,
DB_QUERY_TEXT,
DB_SYSTEM_NAME,
DB_USER,
Expand All @@ -10,10 +11,14 @@ import {
} from '@sentry/conventions/attributes';
import type { IntegrationFn, Scope } from '@sentry/core';
import {
_INTERNAL_getSqlQuerySummary,
_INTERNAL_sanitizeSqlQuery,
isObjectLike,
bindScopeToEmitter,
defineIntegration,
getClient,
getCurrentScope,
hasSpanStreamingEnabled,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startInactiveSpan,
} from '@sentry/core';
Expand All @@ -30,6 +35,8 @@ const INTEGRATION_NAME = 'Mysql' as const;
// `@opentelemetry/instrumentation-mysql`'s default shape.
const ATTR_DB_CONNECTION_STRING = 'db.connection_string';

const DB_SYSTEM_NAME_VALUE_MYSQL = 'mysql' as const;

/**
* The shape orchestrion's transform attaches to the tracing-channel `context` object. Documented here
* rather than imported because orchestrion's runtime doesn't export it.
Expand Down Expand Up @@ -80,17 +87,26 @@ function instrumentMysql(): void {
// handler with the caller's context lost. `deferSpanEnd` replays this scope onto the emitter.
data._sentryCallerScope = getCurrentScope();

const querySummary = sql ? _INTERNAL_getSqlQuerySummary(_INTERNAL_sanitizeSqlQuery(sql)) : undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL literals can leak into summaries

Medium Severity

_INTERNAL_sanitizeSqlQuery only strips single-quoted literals, but MySQL treats double-quoted values as strings unless ANSI_QUOTES is on, and also allows backslash escapes. A user value containing from or join can therefore survive sanitization and land in db.query.summary and streamed span names, which this change presents as low-cardinality and leak-free.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 58cd60c. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in #23659


const client = getClient();
const name =
client && hasSpanStreamingEnabled(client)
? querySummary || database || DB_SYSTEM_NAME_VALUE_MYSQL
: (sql ?? 'mysql.query');

return startInactiveSpan({
name: sql ?? 'mysql.query',
name,
op: 'db',
attributes: {
[SENTRY_KIND]: 'client',
[DB_SYSTEM_NAME]: 'mysql',
[DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_MYSQL,
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.db.mysql',
[ATTR_DB_CONNECTION_STRING]: getJDBCString(host, portIsNumber ? portNumber : undefined, database),
...(database ? { [DB_NAMESPACE]: database } : {}),
...(user ? { [DB_USER]: user } : {}),
...(sql ? { [DB_QUERY_TEXT]: sql } : {}),
[DB_QUERY_SUMMARY]: querySummary,
[SERVER_ADDRESS]: host,
[SERVER_PORT]: portIsNumber ? portNumber : undefined,
},
Expand Down
18 changes: 16 additions & 2 deletions packages/server-utils/src/integrations/mysql2/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import * as diagnosticsChannel from 'node:diagnostics_channel';
import type { IntegrationFn, SpanAttributes } from '@sentry/core';
import {
_INTERNAL_getSqlQuerySummary,
_INTERNAL_sanitizeSqlQuery,
defineIntegration,
getClient,
hasSpanStreamingEnabled,
isObjectLike,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
Expand All @@ -16,6 +20,7 @@ import { mysql2ModuleNames } from '../../orchestrion/config/mysql2';
import { invokeOrchestrionInstrumentation } from '../../orchestrion/instrumentation';
import {
DB_NAMESPACE,
DB_QUERY_SUMMARY,
DB_QUERY_TEXT,
DB_SYSTEM_NAME,
DB_USER,
Expand Down Expand Up @@ -78,16 +83,25 @@ function subscribeQueryChannel(channelName: ChannelName): void {
diagnosticsChannel.tracingChannel<Mysql2QueryChannelContext>(channelName),
data => {
const statement = getQueryText(data.arguments);
const connectionAttributes = getConnectionAttributes(data.self?.config);
const querySummary = statement ? _INTERNAL_getSqlQuerySummary(_INTERNAL_sanitizeSqlQuery(statement)) : undefined;

const client = getClient();
const name =
client && hasSpanStreamingEnabled(client)
? querySummary || (connectionAttributes[DB_NAMESPACE] as string | undefined) || DB_SYSTEM_VALUE_MYSQL
: (statement ?? 'mysql2.query');

return startInactiveSpan({
name: statement ?? 'mysql2.query',
name,
attributes: {
[SENTRY_KIND]: 'client',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'db',
[DB_SYSTEM_NAME]: DB_SYSTEM_VALUE_MYSQL,
...getConnectionAttributes(data.self?.config),
[DB_QUERY_TEXT]: statement || undefined,
[DB_QUERY_SUMMARY]: querySummary,
...connectionAttributes,
},
});
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ import type { TracingChannel } from 'node:diagnostics_channel';
import {
DB_NAMESPACE,
DB_OPERATION_NAME,
DB_QUERY_SUMMARY,
DB_QUERY_TEXT,
DB_SYSTEM_NAME,
SERVER_ADDRESS,
SERVER_PORT,
} from '@sentry/conventions/attributes';
import {
_INTERNAL_getSqlQuerySummary,
_INTERNAL_sanitizeSqlQuery,
getClient,
hasSpanStreamingEnabled,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startInactiveSpan,
Expand Down Expand Up @@ -100,14 +104,22 @@ function setupQueryChannel(tracingChannel: MySQL2TracingChannelFactory, channelN
// literal before it leaves the process; `values` is never attached.
const queryText = data.query ? _INTERNAL_sanitizeSqlQuery(data.query) : undefined;
const operation = queryText?.match(SQL_OPERATION_RE)?.[1]?.toUpperCase();
const querySummary = _INTERNAL_getSqlQuerySummary(queryText);

const client = getClient();
const streamedName =
client && hasSpanStreamingEnabled(client)
? querySummary || data.database || DB_SYSTEM_NAME_VALUE_MYSQL
: queryText || 'mysql2.query';

return startInactiveSpan({
name: queryText || 'mysql2.query',
name: streamedName,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'db',
[DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_MYSQL,
[DB_QUERY_TEXT]: queryText,
[DB_QUERY_SUMMARY]: querySummary,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL escapes leak into summaries

Medium Severity

Summaries are built after _INTERNAL_sanitizeSqlQuery, which only treats doubled quotes ('') as escaped string delimiters. mysql2's query channel inlines values with MySQL backslash escapes (\'), so a literal that contains a quote plus FROM/JOIN is only partially stripped and can still land in db.query.summary and the streamed span name.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f7c6bc0. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be an issue but a broader one than this change. I wanna look into this separately.

[DB_OPERATION_NAME]: operation,
[DB_NAMESPACE]: data.database || undefined,
[SERVER_ADDRESS]: data.serverAddress,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@ class TestClient extends Client<any> {
}
}

function initTestClient(): void {
function initTestClient(traceLifecycle: 'static' | 'stream' = 'static'): void {
initAndBind(TestClient, {
dsn: 'https://username@domain/123',
integrations: [],
sendClientReports: false,
stackParser: () => [],
traceLifecycle,
tracesSampleRate: 1,
transport: () => createTransport({ recordDroppedEvent: () => undefined }, () => resolvedSyncPromise({})),
});
Expand Down Expand Up @@ -199,6 +200,8 @@ describe('subscribeMysql2DiagnosticChannels', () => {
expect(json.attributes['sentry.origin']).toBe('auto.db.mysql2.diagnostic_channel');
expect(json.attributes['db.system.name']).toBe('mysql');
expect(json.attributes['db.operation.name']).toBe('SELECT');
// reported regardless of trace lifecycle, even though only span streaming names the span after it
expect(json.attributes['db.query.summary']).toBe('SELECT maths');
expect(json.attributes['db.namespace']).toBe('test');
expect(json.attributes['server.address']).toBe('127.0.0.1');
expect(json.attributes['server.port']).toBe(3306);
Expand All @@ -222,6 +225,45 @@ describe('subscribeMysql2DiagnosticChannels', () => {
expect(json.name).toBe('SELECT * FROM users WHERE email = ? AND age = ?');
});

it('names the span after the query summary with span streaming enabled', async () => {
initTestClient('stream');

const { span } = await traceOperation(
MYSQL2_DC_CHANNEL_QUERY,
{ query: 'SELECT solution FROM maths' },
{ result: [] },
);

const json = spanToJSON(span!);
expect(json.name).toBe('SELECT maths');
expect(json.attributes['db.query.summary']).toBe('SELECT maths');
// the statement is still reported, just not as the name
expect(json.attributes['db.query.text']).toBe('SELECT solution FROM maths');
});

it('walks the name conventions when no query summary can be derived', async () => {
initTestClient('stream');

const { span } = await traceOperation(
MYSQL2_DC_CHANNEL_QUERY,
{ query: '', database: 'test', serverAddress: '127.0.0.1', serverPort: 3306 },
{ result: [] },
);

// no summary and no operation, so `{db.namespace}` is the first template that can be filled
const json = spanToJSON(span!);
expect(json.name).toBe('test');
expect(json.attributes['db.query.summary']).toBeUndefined();
});

it('falls back to the db system name when nothing else can be filled in', async () => {
initTestClient('stream');

const { span } = await traceOperation(MYSQL2_DC_CHANNEL_QUERY, { query: '' }, { result: [] });

expect(spanToJSON(span!).name).toBe('mysql');
});

it('does not attach raw values to the span', async () => {
const { span } = await traceOperation(
MYSQL2_DC_CHANNEL_QUERY,
Expand Down
Loading