Skip to content

feat: log unsafe queries to Slack for better monitoring #1651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Artuomka merged 1 commit into from
Mar 6, 2026
Merged

feat: log unsafe queries to Slack for better monitoring #1651

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions backend/src/entities/visualizations/panel/utils/check-query-is-safe.util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { BadRequestException } from '@nestjs/common';
import { ConnectionTypesEnum } from '@rocketadmin/shared-code/dist/src/shared/enums/connection-types-enum.js';
import { slackPostMessage } from '../../../../helpers/index.js';
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Importing slackPostMessage into this low-level query-safety utility introduces a network/IO side effect and a dependency on the Slack helper into code that otherwise acts like a pure validator. This makes the function harder to reuse/test and also prevents callers from attaching context (e.g., userId/connectionId) or choosing different reporting behavior. Consider keeping validateQuerySafety side-effect free and moving the Slack notification to the use-cases/controllers (or injecting an optional reporter callback).

Suggested change
import { slackPostMessage } from '../../../../helpers/index.js';

Copilot uses AI. Check for mistakes.

const FORBIDDEN_SQL_KEYWORDS = [
'INSERT',
Expand Down Expand Up @@ -243,6 +244,7 @@ export function validateQuerySafety(query: string, connectionType: ConnectionTyp
const result = checker(query);

if (!result.isSafe) {
slackPostMessage(`Unsafe query: ${query}\nReason: ${result.reason}\nConnection Type: ${connectionType}`);
throw new BadRequestException(`Unsafe query: ${result.reason}. Only read-only queries are allowed.`);
Comment on lines +247 to 248
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

This posts the full raw query text to Slack. Since queries can contain sensitive information (table/column names, tenant identifiers, literal values, etc.) and are user-controlled, this can cause unintended data exposure and may violate privacy/compliance expectations. Prefer logging a redacted/truncated form (e.g., remove string/numeric literals, cap length) and/or a hash + metadata, and consider a dedicated Slack channel or feature-flagging this behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +247 to 248
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Posting a Slack message for every rejected query can be very noisy and can be abused to generate high Slack volume (and outbound HTTP traffic) by repeatedly submitting unsafe queries. Consider adding throttling/sampling, limiting to certain request sources (e.g., AI-generated queries only), or guarding with an environment flag so it can be enabled for monitoring without impacting normal operation.

Copilot uses AI. Check for mistakes.
}
}
Expand Down
Loading