Only explain sampled Postgres statements that are a single SQL statement - #24839
Open
azhou-datadog wants to merge 6 commits into
Open
Only explain sampled Postgres statements that are a single SQL statement#24839azhou-datadog wants to merge 6 commits into
azhou-datadog wants to merge 6 commits into
Conversation
Sampled statement text is read out of pg_stat_activity and interpolated into a PREPARE that the agent executes as the monitoring user, using the simple query protocol. Anyone able to run a query on a monitored database could therefore plant statement separators in the text and have the agent run them with the monitoring role's privileges. Reject any sampled statement that is not unambiguously one top-level statement, before either explain path touches the database, and again at the PREPARE sink itself. Text that cannot be tokenized with certainty (unterminated literals or comments, a backslash in a plain string whose meaning depends on standard_conforming_strings) is refused rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
evalya-impact-summaryevalya impact analysis |
Contributor
🎉 All green!🧪 All tests passed 🚧 1 test that failed was ignored due to quarantine 🎯 Code Coverage (details) 🔗 Commit SHA: c21a78b | Docs | Datadog PR Page | Give us feedback! |
psycopg only leaves the simple query protocol, which executes every ;-separated statement in a string, when a query carries parameters, asks for binary results or runs in a pipeline. The pool's client-side cursors reject binary results, so a pipeline is the only route: wrap the PREPARE in one, and the server itself refuses a multi-command string. Guarded by a libpq 14+ capability check, since pipeline mode needs it. This puts the server rather than the single-statement scan in charge of the boundary. The scan stays: it reports a clean error code instead of a syntax error, and it is the only guard where libpq is older than 14. Also trims the comments and docstrings this branch added, and drops a stale example from the pre-existing datatype_mismatch comment: a multi-statement string sent through datadog.explain_statement raises InvalidCursorDefinition, not a datatype mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pipeline makes the server reject sampled text holding more than one statement, so the hand-written tokenizer was a second, weaker implementation of the same rule, and the only thing it still covered was a build whose libpq is too old for pipeline mode. Fail closed there instead: without pipeline support the query goes unexplained, with one warning at init, rather than falling back to the simple query protocol. Explaining a parameterized query is worth less than the guarantee that sampled text never runs. Also reverts the test_failed_explain_handling expectation. That statement has no $1, so it takes the datadog.explain_statement path, which rejects a multi-query plan on its own and still reports InvalidCursorDefinition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3719462adc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
test_execute_prepare_uses_a_pipeline asserted the pipeline branch while taking _can_use_pipeline from whatever libpq the machine links, so it would fail rather than skip on a build older than 14. Set it explicitly, as the fail-closed test already does. The integration test had the same dependence. Its injection assertion holds either way -- fail closed sends nothing -- so only the error-class assertion is now conditional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
eric-weaver
approved these changes
Aug 14, 2026
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
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.
What does this PR do?
The
PREPAREthat explains a parameterized query now goes out over the extended query protocol, where the server refuses a multi-command string outright instead of executing every statement in it.ClientCursorrefuses — or one inside a pipeline.latestlinks libpq 18.3, so that branch should never be reached in a shipped Agent.SELECT 1; CREATE TEMP TABLE …; --$1is refused withcannot insert multiple commands into a prepared statement, and the table never appears. That is an integration test; the suite is 773 passed, 4 skipped.Motivation
Text sampled from
pg_stat_activity.queryis interpolated intoPREPARE dd_<signature> AS <statement>and run as the monitoring user over the simple query protocol, which executes every;-separated statement. Any user who can query a monitored database can plant one and have the agent run it with the monitoring role's privileges. The feature is on by default.https://datadoghq.atlassian.net/browse/VULN-92306
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged