codeant-ai-for-open-source[bot] commented on code in PR #41998:
URL: https://github.com/apache/superset/pull/41998#discussion_r3586060133
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -355,83 +310,134 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
+ const fetchSlackChannels = ({
searchString = '',
types = [],
exactMatch = false,
force = false,
+ page,
+ pageSize,
}: {
- searchString?: string | undefined;
+ searchString?: string;
types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
+ exactMatch?: boolean;
+ force?: boolean;
+ page?: number;
+ pageSize?: number;
} = {}): Promise<JsonResponse> => {
const queryString = rison.encode({
- searchString,
+ search_string: searchString,
types,
- exactMatch,
+ exact_match: exactMatch,
force,
+ ...(page !== undefined ? { page } : {}),
+ ...(pageSize !== undefined ? { page_size: pageSize } : {}),
});
const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
return SupersetClient.get({ endpoint });
Review Comment:
**Suggestion:** `searchString` is inserted into the URL using
`rison.encode`, not URL-safe encoding. Inputs containing characters like `#`,
`&`, or `?` can break/truncate the `q` query parameter before it reaches the
API, so server-side search behaves incorrectly. Use URI-safe Rison encoding for
query params (as done in other fetchers) before interpolating into the
endpoint. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SlackV2 channel search misbehaves for reserved characters.
- ⚠️ Large-workspace Slack recipient search becomes unreliable.
- ⚠️ Inconsistent Rison usage versus email recipient fetcher.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Alerts and Reports modal and add a Slack notification method so
that
NotificationMethod renders, as wired in AlertReportModal.tsx lines 2688-2727
where
<NotificationMethod /> is mapped over notificationSettings.
2. In NotificationMethod.tsx lines 253-267 and 679-12, select SlackV2 as the
delivery
method so the SlackV2 AsyncSelect is rendered and begin typing a search
string containing
URL-reserved characters such as `?`, `#`, or `&` in the recipients picker
(this value is
passed as the `search` argument into loadSlackChannels at lines 123-145).
3. Observe that loadSlackChannels at NotificationMethod.tsx lines 123-145
delegates to
fetchSlackChannels at lines 94-119, which builds `queryString` using
`rison.encode` at
line 109 with the raw `searchString` and interpolates it directly into the
endpoint
`/api/v1/report/slack_channels/?q=${queryString}` at line 117.
4. Because `rison.encode` does not URI-escape reserved characters and the
resulting string
is used verbatim in the query parameter, the browser issues a GET where the
`q` parameter
is truncated or split at the reserved characters, so the backend receives a
corrupted
Rison payload; server-side filtering for Slack channel search can misbehave
or fail for
such inputs, unlike fetchEmailRecipientOptions at lines 217-247 which uses
`rison.encode_uri` specifically to avoid this issue.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c52f6e8ec49f4909bd68652da15ffddb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c52f6e8ec49f4909bd68652da15ffddb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 328:337
**Comment:**
*Api Mismatch: `searchString` is inserted into the URL using
`rison.encode`, not URL-safe encoding. Inputs containing characters like `#`,
`&`, or `?` can break/truncate the `q` query parameter before it reaches the
API, so server-side search behaves incorrectly. Use URI-safe Rison encoding for
query params (as done in other fetchers) before interpolating into the endpoint.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=57c9480035bb0db01e032faaec38d0541591a24b72d9b0a699af80dd576c5843&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=57c9480035bb0db01e032faaec38d0541591a24b72d9b0a699af80dd576c5843&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -665,23 +671,26 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
) : (
// for SlackV2
<div className="input-container">
- <Select
+ <AsyncSelect
+ key={slackRefreshKey}
ariaLabel={t('Select channels')}
mode="multiple"
name="recipients"
value={slackRecipients}
- options={slackOptions}
+ options={loadSlackChannels}
onChange={onSlackRecipientsChange}
+ onError={() => setUseSlackV1(true)}
allowClear
Review Comment:
**Suggestion:** Setting `useSlackV1` in `onError` does not actually switch
the active picker when the current method is SlackV2, so fetch failures can
leave users stuck in the failing AsyncSelect path instead of falling back
automatically. Ensure the fallback also updates the selected notification
method (or render branch) so the UI truly moves to Slack V1 input on error.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SlackV2 picker does not auto-fallback on errors.
- ⚠️ Users stay on a broken SlackV2 input path.
- ⚠️ Manual reconfiguration required to reach Slack V1.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the SlackV2 feature flag is enabled so SlackV2 can be selected;
AlertReportModal.tsx lines 214-215 read allowedNotificationMethods, and
NotificationMethod.tsx lines 42-65 use
`isFeatureEnabled(FeatureFlag.AlertReportSlackV2)`
to include SlackV2 in methodOptions when `useSlackV1` is false.
2. Open the Alerts or Reports modal and configure a notification whose
NotificationSetting
has `method: NotificationMethodOption.SlackV2`, as exemplified by
mockSettingSlackV2 in
NotificationMethod.test.tsx lines 225-233; this causes NotificationMethod to
render the
SlackV2 AsyncSelect branch at lines 253-267 and 679-12.
3. Induce a failure in the SlackV2 channel fetch (for example by having
SupersetClient.get
reject inside loadSlackChannels at NotificationMethod.tsx lines 123-145) so
that the
AsyncSelect options loader throws; the AsyncSelect invokes its `onError`
handler defined
at line 683 as `() => setUseSlackV1(true)`, which flips `useSlackV1` to true
but leaves
`setting.method` as `NotificationMethodOption.SlackV2`.
4. After `useSlackV1` is true, methodOptions at NotificationMethod.tsx lines
42-65 filter
out SlackV2 and only include Slack, yet the current `method` prop still
equals `SlackV2`;
the Select’s `value` is computed as `methodOptions.find(option =>
option.value ===
method)` at lines 143-156 and becomes undefined, and the recipients branch
at lines
204-253 continues to render the SlackV2 AsyncSelect because it switches
solely on
`method`, not `useSlackV1`, leaving the user on the failing SlackV2 picker
instead of
automatically transitioning to the working SlackV1 textarea.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a4689e0ebfb0496098284ba6d488f558&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a4689e0ebfb0496098284ba6d488f558&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 683:683
**Comment:**
*Incomplete Implementation: Setting `useSlackV1` in `onError` does not
actually switch the active picker when the current method is SlackV2, so fetch
failures can leave users stuck in the failing AsyncSelect path instead of
falling back automatically. Ensure the fallback also updates the selected
notification method (or render branch) so the UI truly moves to Slack V1 input
on error.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=3934f32d70fed3b2d3d9171e4180eea9265639392fde73aab0f2f6e6942d38ea&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=3934f32d70fed3b2d3d9171e4180eea9265639392fde73aab0f2f6e6942d38ea&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -355,83 +310,134 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
+ const fetchSlackChannels = ({
searchString = '',
types = [],
exactMatch = false,
force = false,
+ page,
+ pageSize,
}: {
- searchString?: string | undefined;
+ searchString?: string;
types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
+ exactMatch?: boolean;
+ force?: boolean;
+ page?: number;
+ pageSize?: number;
} = {}): Promise<JsonResponse> => {
const queryString = rison.encode({
- searchString,
+ search_string: searchString,
types,
- exactMatch,
+ exact_match: exactMatch,
force,
+ ...(page !== undefined ? { page } : {}),
+ ...(pageSize !== undefined ? { page_size: pageSize } : {}),
});
const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
return SupersetClient.get({ endpoint });
};
- const updateSlackOptions = async ({
- force,
- }: {
- force?: boolean | undefined;
- } = {}) => {
- setIsSlackChannelsLoading(true);
- fetchSlackChannels({ types: ['public_channel', 'private_channel'], force })
- .then(({ json }) => {
- const { result } = json;
- const options: SlackOptionsType = mapChannelsToOptions(result);
-
- setSlackOptions(options);
-
- if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) {
- // for edit mode, map existing ids to names for display if slack v2
- // or names to ids if slack v1
- const [publicOptions, privateOptions] = options;
- if (
- method &&
- [
- NotificationMethodOption.SlackV2,
- NotificationMethodOption.Slack,
- ].includes(method)
- ) {
- setSlackRecipients(
- mapSlackValues({
- method,
- recipientValue,
- slackOptions: [
- ...publicOptions.options,
- ...privateOptions.options,
- ],
- }),
- );
- }
- }
- })
- .catch(() => {
- // Fallback to slack v1 if slack v2 is not compatible
- setUseSlackV1(true);
- })
- .finally(() => {
- setMethodOptionsLoading(false);
- setIsSlackChannelsLoading(false);
+ // Lazily fetch one page of channels as the user searches, so workspaces with
+ // tens of thousands of channels never load the whole list into the browser.
+ const loadSlackChannels = useCallback(
+ async (search: string, page: number, pageSize: number) => {
+ const force = forceRefreshSlackRef.current;
+ forceRefreshSlackRef.current = false;
+ const { json } = await fetchSlackChannels({
+ searchString: search,
+ types: ['public_channel', 'private_channel'],
+ force,
+ page,
+ pageSize,
});
+ const result = (json?.result ?? []) as SlackChannel[];
+ return {
+ data: result.map(channel => ({
+ label:
+ !channel.is_private && !channel.is_member
+ ? `${channel.name} ${t('(Bot not in channel)')}`
+ : channel.name,
+ value: channel.id,
+ })),
+ totalCount: json?.count ?? result.length,
+ };
+ },
+ // fetchSlackChannels only closes over stable refs/imports
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ [],
+ );
+
+ const onRefreshSlackChannels = () => {
+ forceRefreshSlackRef.current = true;
+ // Remount the AsyncSelect so its internal result cache is discarded and
the
+ // forced (cache-busting) fetch runs.
+ setSlackRefreshKey(key => key + 1);
};
+ // Resolve the channel ids saved on an existing SlackV2 report into
+ // human-readable labels. Bounded by exact_match so it never enumerates the
+ // whole workspace.
useEffect(() => {
+ let cancelled = false;
const slackEnabled = options?.some(
option =>
option === NotificationMethodOption.Slack ||
option === NotificationMethodOption.SlackV2,
);
- if (slackEnabled && !slackOptions[0]?.options.length) {
- updateSlackOptions();
+ const savedIds =
+ method === NotificationMethodOption.SlackV2 && recipientValue
+ ? recipientValue
+ .split(',')
+ .map(value => value.trim())
+ .filter(Boolean)
+ : [];
+
+ const resolveSavedRecipients = async () => {
+ // Show the saved ids immediately so the existing selection is always
+ // visible, then enrich with channel names once the lookup resolves.
+ if (savedIds.length && !cancelled) {
+ setSlackRecipients(savedIds.map(id => ({ label: id, value: id })));
+ }
+ try {
+ if (savedIds.length) {
+ const { json } = await fetchSlackChannels({
+ searchString: recipientValue,
+ types: ['public_channel', 'private_channel'],
+ exactMatch: true,
+ });
+ const result = (json?.result ?? []) as SlackChannel[];
+ const namesById = new Map(
+ result.map(channel => [channel.id, channel.name]),
+ );
+ if (!cancelled) {
+ setSlackRecipients(
+ savedIds.map(id => ({
+ label: namesById.get(id) ?? id,
+ value: id,
+ })),
+ );
+ }
+ }
+ } catch {
+ // Keep the raw ids as labels; the AsyncSelect onError handler drives
+ // the v1 fallback for the picker itself.
+ } finally {
+ if (!cancelled) {
+ setMethodOptionsLoading(false);
+ }
+ }
+ };
+
+ if (slackEnabled) {
+ resolveSavedRecipients();
+ } else {
+ setMethodOptionsLoading(false);
}
+
+ return () => {
+ cancelled = true;
+ };
+ // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Review Comment:
**Suggestion:** The saved-recipient resolution effect runs only once on
mount, but this component already supports `recipients` prop updates later.
When `setting.recipients` changes after mount, `slackRecipients` labels are not
re-resolved and can remain stale/incorrect. Re-run the resolution when relevant
inputs (`method`, `recipientValue`, `options`) change. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SlackV2 picker shows channels that are not persisted.
- ⚠️ Saved reports can lose SlackV2 recipients silently.
- ⚠️ Notification summary in backend diverges from UI state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In AlertReportModal.tsx lines 2688-2727, open the notification panel so
that
notificationSettings are mapped to <NotificationMethod /> instances, each
receiving a
NotificationSetting with `method`, `recipients`, and `options`.
2. Configure a notification with `method` set to
`NotificationMethodOption.SlackV2` and
select one or more channels in the SlackV2 AsyncSelect at
NotificationMethod.tsx lines
253-267 and 679-12; this populates `slackRecipients` via
onSlackRecipientsChange at lines
110-122 and persists a comma-separated channel id string into
`setting.recipients` via
updateNotificationSetting in AlertReportModal.tsx lines 834-859.
3. Change the delivery method to Slack (SlackV1) and then back to SlackV2
using the Select
at NotificationMethod.tsx lines 141-157, which calls onMethodChange at lines
72-92;
onMethodChange resets `recipientValue`, `ccValue`, and `bccValue` and
updates the parent
NotificationSetting (recipients cleared), but it does not clear
`slackRecipients`, and the
effect that resolves saved SlackV2 recipients at lines 161-222 is declared
with an empty
dependency array `[]` so it never re-runs when `method`, `recipientValue`,
or `options`
change.
4. After this toggle, the SlackV2 AsyncSelect still renders using the stale
`slackRecipients` array while the underlying `setting.recipients` string is
now empty;
when the user saves, AlertReportModal.tsx lines 65-82 and 115-133 build the
recipients
payload from `setting.recipients`, so no SlackV2 recipient is sent even
though the UI
shows selected channels, demonstrating stale label state caused by the
mount-only
resolution effect.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=712b11eedf654796823a796f88e7bce5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=712b11eedf654796823a796f88e7bce5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 380:441
**Comment:**
*Stale Reference: The saved-recipient resolution effect runs only once
on mount, but this component already supports `recipients` prop updates later.
When `setting.recipients` changes after mount, `slackRecipients` labels are not
re-resolved and can remain stale/incorrect. Re-run the resolution when relevant
inputs (`method`, `recipientValue`, `options`) change.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=c7cd3d04777461e5ec099fd8f3f8b0d70d7d8d14cf7195905fc16969d5932dba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=c7cd3d04777461e5ec099fd8f3f8b0d70d7d8d14cf7195905fc16969d5932dba&reaction=dislike'>👎</a>
##########
superset/reports/schemas.py:
##########
@@ -59,6 +59,9 @@
"items": {"type": "string", "enum": ["public_channel",
"private_channel"]},
},
"exact_match": {"type": "boolean"},
+ "force": {"type": "boolean"},
Review Comment:
**Suggestion:** Exposing `force` as an accepted query parameter allows any
caller of the Slack channels endpoint to bypass the cached channel list and
trigger a full Slack `conversations.list` crawl repeatedly. That reintroduces
the same rate-limit/timeout failure mode this change is trying to solve. Keep
`force` internal-only (not user-controllable from this API) or gate it behind a
stricter privileged path. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Slack channel fetch bypasses cache via force flag.
⚠️ Large workspaces hit Slack API rate limits.
⚠️ Slack recipient picker can freeze on repeated force.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with Alerts & Reports enabled and a Slack bot token
configured so Slack
channel listing is active (Slack client setup and channel enumeration are
implemented in
`superset/utils/slack.py:79-187`).
2. Call the Slack channels endpoint exposed by
`ReportScheduleRestApi.slack_channels` via
HTTP `GET /api/v1/report/slack_channels/` (route defined by
`@expose("/slack_channels/",
methods=("GET",))` at `superset/reports/api.py:628` and protected with
`@protect()` at
`superset/reports/api.py:63`).
3. Pass a `q` query parameter containing Rison/JSON with `"force": true`,
e.g.
`q={"search_string":"","force":true}`; the schema
`get_slack_channels_schema` at
`superset/reports/schemas.py:54-62` explicitly accepts a `force` boolean, and
`@parse_rison(get_slack_channels_schema)` at `superset/reports/api.py:64`
deserializes it
into `params["force"]` used at `superset/reports/api.py:685`.
4. Inside `slack_channels`, the code at `superset/reports/api.py:681-127`
calls
`get_channels_with_search(search_string=..., types=..., exact_match=...,
force=force)`;
`get_channels_with_search` in `superset/utils/slack.py:197-213` forwards
`force` into
`get_channels(force=force,
cache_timeout=app.config["SLACK_CACHE_TIMEOUT"])`, and
`get_channels`’ docstring at `superset/utils/slack.py:115-133` states that
`kwargs`
including `"force"` are forwarded to the memoized fetch `_get_channels`,
allowing the
caller to bypass the cached Slack conversations list and trigger a full
`conversations.list` crawl on every such request, reintroducing
rate-limit/timeout risks
for large workspaces.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=812c267830f34752b5ee894325b4ea8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=812c267830f34752b5ee894325b4ea8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 62:62
**Comment:**
*Cache: Exposing `force` as an accepted query parameter allows any
caller of the Slack channels endpoint to bypass the cached channel list and
trigger a full Slack `conversations.list` crawl repeatedly. That reintroduces
the same rate-limit/timeout failure mode this change is trying to solve. Keep
`force` internal-only (not user-controllable from this API) or gate it behind a
stricter privileged path.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=4104060c20347506f1986a1ba010e225a0ac83298a625c560f92208de3302d1f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=4104060c20347506f1986a1ba010e225a0ac83298a625c560f92208de3302d1f&reaction=dislike'>👎</a>
##########
superset/reports/schemas.py:
##########
@@ -59,6 +59,9 @@
"items": {"type": "string", "enum": ["public_channel",
"private_channel"]},
},
"exact_match": {"type": "boolean"},
+ "force": {"type": "boolean"},
+ "page": {"type": "integer", "minimum": 0},
+ "page_size": {"type": "integer", "minimum": 1},
Review Comment:
**Suggestion:** `page_size` is only bounded to be positive, but not capped,
so callers can request extremely large pages and effectively force the backend
to return the full filtered channel set in one response. This defeats the
pagination protection and can still cause large payloads and browser/backend
slowdowns. Add a strict maximum page size. [performance]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Clients can request entire Slack channel list in one response.
⚠️ Large responses slow browser Slack channel picker significantly.
⚠️ Backend spends memory serializing huge channel arrays.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset with Alerts & Reports enabled and SlackV2 configured; the
frontend Slack
recipient picker for Alerts & Reports uses the `slack_channels` API defined
in
`superset/reports/api.py:628-135` to enumerate channels.
2. From a client (browser devtools, API script, or custom integration),
issue `GET
/api/v1/report/slack_channels/` with query parameter `q` containing a large
`page_size`,
for example `q={"page":0,"page_size":100000}`; the OpenAPI schema
`get_slack_channels_schema` at `superset/reports/schemas.py:54-65` defines
`"page_size":
{"type": "integer", "minimum": 1}` without any maximum, so any positive
integer is
accepted.
3. In `ReportScheduleRestApi.slack_channels`
(`superset/reports/api.py:639-135`), `params
= kwargs.get("rison", {})` is read at line 681, then `page =
params.get("page")` and
`page_size = params.get("page_size")` at `superset/reports/api.py:683-684`;
the function
fetches the full filtered channel list via `get_channels_with_search(...)` at
`superset/reports/api.py:122-127`, which itself enumerates all cached
channels in
`superset/utils/slack.py:197-256` before any slicing.
4. Because `page_size` is unbounded, when the client requests `page_size`
larger than or
equal to the total channel count, the slicing logic `start = page *
page_size` and
`channels = channels[start : start + page_size]` at
`superset/reports/api.py:132-134`
returns nearly the entire `channels` list in a single response; this
recreates the large
JSON payloads and frontend rendering slowdowns that the pagination change is
intended to
prevent, especially on workspaces with tens of thousands of Slack channels.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=84a09ad519f944beae2d3ac93b53bfdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=84a09ad519f944beae2d3ac93b53bfdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 64:64
**Comment:**
*Performance: `page_size` is only bounded to be positive, but not
capped, so callers can request extremely large pages and effectively force the
backend to return the full filtered channel set in one response. This defeats
the pagination protection and can still cause large payloads and
browser/backend slowdowns. Add a strict maximum page size.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=e8f0724ea288e047a5526797c985ffd5efcee07df70d9e9e59d4128b91f3b3d3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41998&comment_hash=e8f0724ea288e047a5526797c985ffd5efcee07df70d9e9e59d4128b91f3b3d3&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
