database_observability: don't log query text if redaction is enabled #4599
Conversation
cristiangreco
force-pushed
the
cristian/dbo11y-pg-dont-log-sample-if-unneeded
branch
2 times, most recently
from
864a087 to
2e79b14
Compare
![semgrep-code-grafana[bot]](https://avatars.githubusercontent.com/u/7195757?s=60&v=4){kind=small}
| queryTextField = queryTextClause | ||
| } | ||
|
|
||
| query := fmt.Sprintf(selectPgStatActivity, queryTextField) |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
String-formatted SQL query detected. This could lead to SQL injection if the string is not sanitized properly. Audit this call to ensure the SQL is not manipulable by external data.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by string-formatted-query.
We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.
You can view more details about this finding in the Semgrep AppSec Platform.
cristiangreco
force-pushed
the
cristian/dbo11y-pg-dont-log-sample-if-unneeded
branch
4 times, most recently
from
292ccf4 to
beca1f6
Compare
Do not log sql text in postgres collector when `disable_query_redaction` is false (the default). Additionally, don't log it if it is set to true for op=wait_event entries (not really needed). Clarify that the unredacted sql text _might_ include parameters.
cristiangreco
force-pushed
the
cristian/dbo11y-pg-dont-log-sample-if-unneeded
branch
from
beca1f6 to
ab400d5
Compare
| queryText = redact(queryText) | ||
| } | ||
| return fmt.Sprintf( | ||
| `datname="%s" pid="%d" leader_pid="%s" user="%s" backend_type="%s" state="%s" xid="%d" xmin="%d" wait_time="%s" wait_event_type="%s" wait_event="%s" wait_event_name="%s" blocked_by_pids="%v" queryid="%d" query="%s" engine="postgres"`, |
There was a problem hiding this comment.
Removed query text from op=WAIT_EVENT
|
💻 Deploy preview deleted (database_observability: don't log query text if redaction is enabled). |
| require.Equal(t, `level="info" datname="testdb" pid="301" leader_pid="" user="testuser" app="testapp" client="127.0.0.1:5432" backend_type="client backend" state="active" xid="0" xmin="0" xact_time="2m0s" query_time="0s" queryid="555" cpu_time="0s" query="UPDATE users SET status = 'active'"`, entries[0].Line) | ||
| require.Equal(t, model.LabelSet{"op": OP_WAIT_EVENT}, entries[1].Labels) | ||
| require.Contains(t, entries[1].Line, `wait_time="10s"`) | ||
| require.Equal(t, `level="info" datname="testdb" pid="301" leader_pid="" user="testuser" backend_type="client backend" state="active" xid="0" xmin="0" wait_time="10s" wait_event_type="Lock" wait_event="relation" wait_event_name="Lock:relation" blocked_by_pids="[103 104]" queryid="555"`, entries[1].Line) |
There was a problem hiding this comment.
Extended validation to the entire message.
cristiangreco
requested review from
a team and
clayton-cornell
as code owners
| } | ||
|
|
||
| labels := fmt.Sprintf( | ||
| `datname="%s" pid="%d" leader_pid="%s" user="%s" app="%s" client="%s" backend_type="%s" state="%s" xid="%d" xmin="%d" xact_time="%s" query_time="%s" queryid="%d" query="%s" engine="postgres"`, |
There was a problem hiding this comment.
Are we removing the engine=postgres on purpose?
There was a problem hiding this comment.
Yes this was on purpose. Can't find the ticket anymore, but at some point we stopped using it on the frontend.
internal/component/database_observability/mysql/collector/query_samples.go
Outdated
Show resolved
Hide resolved
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
Co-authored-by: MattNolf <86960799+matthewnolf@users.noreply.github.com>
cristiangreco
deleted the
cristian/dbo11y-pg-dont-log-sample-if-unneeded
branch
4 participants
PR Description
Do not log sql text in postgres collector when
disable_query_redactionis false (the default). Additionally, don't log it if it is set to true forop=wait_evententries (not really needed).Clarify that the unredacted sql text might (as opposed to will) include parameters.
Which issue(s) this PR fixes
Relates to https://github.com/grafana/grafana-dbo11y-app/issues/1775
Notes to the Reviewer
PR Checklist