fix(server-utils): Always sanitize inline literals out of db.query.text - #24089
Open
s1gr1d wants to merge 6 commits into
Open
fix(server-utils): Always sanitize inline literals out of db.query.text#24089s1gr1d wants to merge 6 commits into
db.query.text#24089s1gr1d wants to merge 6 commits into
Conversation
…erals Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
size-limit report 📦
|
…ery-data # Conflicts: # packages/cloudflare/src/instrumentations/worker/instrumentD1.ts # packages/core/src/server.ts # packages/nuxt/src/runtime/utils/instrumentDatabase.ts # packages/server-utils/src/integrations/knex.ts # packages/server-utils/src/integrations/mysql.ts # packages/server-utils/src/integrations/mysql2/index.ts # packages/server-utils/src/integrations/postgres.ts # packages/server-utils/src/integrations/tedious.ts
s1gr1d
marked this pull request as ready for review
September 7, 2026 08:41
s1gr1d
requested review from
isaacs,
mydea and
nicohrubec
and removed request for
a team
September 7, 2026 08:41
Per OTel, query text may only be collected by default when literals are sanitized, and our docs say `databaseQueryData` does not control `db.query.text`. Sanitize unconditionally at every site that attached raw SQL and drop the `filterCollectedDbQueryText` gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dataCollection.databaseQueryData to inline SQL literalsdb.query.text
3 tasks
Lms24
approved these changes
Sep 8, 2026
Member
There was a problem hiding this comment.
Thanks for fixing this!
One observation from this PR, but totally out of scope for it: Looks like we have very few tests that check proper SQL queries, e.g. with filter conditions. Queries like SELECT 1 + 1 are not super representative here. I'm fine with them being sanitized as well although it's IMHO not as critically necessary as literals in filter clauses or insertion values WHERE col = ?.
That said, some of the tedious tests now show that literals in filter conditions are also sanitized, so all good!
Again, nothing to change for this PR but rather something we should tackle sometime to have more representative tests.
…ery-data # Conflicts: # dev-packages/e2e-tests/test-applications/astro-6-cf-workers/tests/db.test.ts # dev-packages/e2e-tests/test-applications/astro-7/tests/db.test.ts # dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts # dev-packages/e2e-tests/test-applications/node-express-v5/tests/mysql.test.ts
Comment on lines
134
to
+136
| function createStartSpanOptions(query: string, type: D1QueryType): StartSpanOptions { | ||
| const querySummary = query ? getSqlQuerySummary(sanitizeSqlQuery(query)) : undefined; | ||
| const queryText = sanitizeSqlQuery(query); | ||
| const querySummary = query ? getSqlQuerySummary(queryText) : undefined; |
Contributor
There was a problem hiding this comment.
Bug: The D1 instrumentation sets db.query.text to 'Unknown SQL Query' for empty queries, unlike other integrations which omit the attribute, due to an unguarded call to sanitizeSqlQuery.
Severity: LOW
Suggested Fix
Add a guard before calling sanitizeSqlQuery to align with other database integrations. Change the line to const queryText = query ? sanitizeSqlQuery(query) : undefined;. This will ensure that for empty or falsy queries, the db.query.text attribute is omitted from the span.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cloudflare/src/instrumentations/worker/instrumentD1.ts#L134-L136
Potential issue: In the D1 instrumentation, `sanitizeSqlQuery(query)` is called without
first checking if the `query` is a falsy value, such as an empty string. The
`sanitizeSqlQuery` function returns the string `'Unknown SQL Query'` for falsy inputs.
Consequently, if `D1PreparedStatement.prepare("")` is ever called, the resulting span
will have a `db.query.text` attribute set to `'Unknown SQL Query'`. This behavior is
inconsistent with other database integrations in the codebase, which use a guard to
ensure the attribute is omitted for empty or undefined queries. While this scenario is
unlikely in normal usage, it represents a deviation from the established pattern.
Also affects:
packages/cloudflare/src/instrumentations/worker/instrumentD1.ts:126~126
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
2 participants
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.
mysql, mysql2, pg, tedious, knex, the Nuxt DB helper and Cloudflare D1 wrote the raw statement to
db.query.text, so inline literals likeWHERE email = 'jane@example.com'shipped as span data. postgres-js and the mysql2 channel subscriber already sanitized; the rest did not.Per OTel, query text may be collected by default only when literals are sanitized to
?, and parameterized text passes through untouched ($nand?placeholders survive the sanitizer). Our own docs saydatabaseQueryDatadoes not controldb.query.text, so this is not gated on an option. Each site now sanitizes once and reuses the result for the attribute, the summary, and the non-streamed span name, which is less work than before (the summary path already sanitized). The D1 breadcrumb message gets the same treatment.An earlier revision of this PR gated the raw text on
databaseQueryDatainstead; that read the option as covering query text, which the docs explicitly carve out. Relay-side scrubbing for this attribute is being enabled separately in getsentry/sentry-conventions#625.This resolves the two
TODO: (fix)notes in the tedious integration test.Fixes #24084