codeant-ai-for-open-source[bot] commented on code in PR #22604: URL: https://github.com/apache/superset/pull/22604#discussion_r3485734783
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/getUrlLinks.ts: ########## @@ -0,0 +1,42 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import memoizeOne from 'memoize-one'; +import { DataRecord } from '@superset-ui/core'; +import { UrlLinkConfig } from '../types'; + +const parseLinkSchema = (schema: string) => schema.match(/(?<=\$\{).*?(?=\})/g); + +const genLinkHref = (values: DataRecord, schema: string) => { + let result = schema; + parseLinkSchema(schema)?.forEach(name => { + const val = values[name]?.toString() ?? ''; + result = result.replace(`$\{${name}}`, val); Review Comment: **Suggestion:** The generated link string is returned without any URL sanitization or scheme allowlisting, so a configured schema like `javascript:...` (or data/vbscript payloads) can be emitted and later rendered as a clickable link. Sanitize the final URL (or enforce an allowed protocol list such as http/https/mailto/tel) before returning it. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Table chart links can execute javascript: payloads. - ⚠️ Drill-enabled dashboards expose clickable unsafe link targets. - ⚠️ Multi-tenant editors can inject stored XSS links. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a table visualization using the Table chart plugin (`superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:13-29`), which exposes the `url_link` control of type `UrlLinkControl`. 2. In Explore, open the Url Link control UI, which uses `UrlLinkControl` and `UrlLinkPopoverContent` (`superset-frontend/src/explore/components/controls/UrlLinkControl/UrlLinkControl.tsx:96-141`, `UrlLinkPopoverContent.tsx:64-88`), and create a link config with `linkSchema` set to a dangerous scheme such as `javascript:alert(document.domain)`. 3. When the chart is run, `transformProps` reads `formData.url_link` into `urlLink` (`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:67-87`) and builds `columnUrlLinks` via `getTextFromValues(urlLink)` (`transformProps.ts:248-250`), which in turn calls `genLinkHref(values, linkSchema!)` (`getUrlLinks.ts:35-40`) and returns the raw `linkSchema` string for each row because `genLinkHref` does no scheme validation (`getUrlLinks.ts:25-31`). 4. `TableChart` renders these URLs as clickable anchors in each matching column (`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:861-883`), using `<a target="_blank" href={urlLinkHref} rel="noreferrer">`, so the `javascript:` URL is sent directly to the browser; when a user clicks the link, the injected JavaScript executes in the context of the Superset UI. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f1741fe683ed47ed87085be2dadb1ab4&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=f1741fe683ed47ed87085be2dadb1ab4&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/packages/superset-ui-chart-controls/src/utils/getUrlLinks.ts **Line:** 28:29 **Comment:** *Security: The generated link string is returned without any URL sanitization or scheme allowlisting, so a configured schema like `javascript:...` (or data/vbscript payloads) can be emitted and later rendered as a clickable link. Sanitize the final URL (or enforce an allowed protocol list such as http/https/mailto/tel) before returning it. 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%2F22604&comment_hash=611419fa5e6f3abbcc1911f82d20c06f3e9ce29738b41b133100d13185104678&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F22604&comment_hash=611419fa5e6f3abbcc1911f82d20c06f3e9ce29738b41b133100d13185104678&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-table/src/transformProps.ts: ########## @@ -693,6 +695,28 @@ const transformProps = ( : ''; const [metrics, percentMetrics, columns] = processColumns(chartProps); + + const columnUrlLinks = getTextFromValues(urlLink) ?? []; + + // append url link columns to columns + if (columnUrlLinks.length) { + const existingColumnKeys = new Set(columns.map(col => col.key)); + columnUrlLinks.forEach(({ column }) => { + if (!existingColumnKeys.has(column)) { + existingColumnKeys.add(column); + columns.push({ + key: column, + label: column, + dataType: GenericDataType.String, + isNumeric: false, + isMetric: false, + isPercentMetric: false, + config: {}, + }); + } + }); + } Review Comment: **Suggestion:** This appends URL-link columns into the main `columns` metadata even when they are not query result columns, which makes downstream table logic treat them as real datasource fields (for filtering/drill-to-detail). That can emit filters for non-existent columns and break backend drill/filter flows. Keep virtual link columns separate from query columns (or explicitly exclude them from filter/drill column sets). [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Drill-to-detail from table fails for url-link charts. - ❌ Context menu cross-filters emit filters on invalid columns. - ⚠️ Users see backend errors when exploring table links. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a Table chart and use the Url Link control (`superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:13-29`) to add a link with a new `columnName` such as `Details link` and a `linkSchema` like `https://example.com/issues/${issueId}` via `UrlLinkPopoverContent` (`UrlLinkPopoverContent.tsx:64-88`), so `rawFormData.url_link` contains an entry whose `columnName` does not match any existing query column. 2. When the visualization runs, `transformProps` computes `[metrics, percentMetrics, columns] = processColumns(chartProps)` and then builds `columnUrlLinks = getTextFromValues(urlLink) ?? []` (`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:248-250`), and appends a new `DataColumnMeta` entry to `columns` for any `column` not already present (`transformProps.ts:252-259`), with `key` and `label` set to the configured `columnName`. 3. `TableChart` receives this augmented `columnsMeta` and uses it both for rendering and for constructing drill/cross-filter metadata: `filteredColumnsMeta` is derived from `columnsMeta` (`TableChart.tsx:204-225`), and the context menu handler iterates all non-metric columns, reading `dataRecordValue = value[col.key]` from the row and pushing filters into `drillToDetailFilters` (`TableChart.tsx:247-291`). 4. For the virtual link column `Details link`, `row.original['Details link']` is always `undefined` because it is not a real query result field, so the context menu logic adds an `IS NULL` filter on a non-existent column (`col: 'Details link'`) into the drill-to-detail and cross-filter payloads (`TableChart.tsx:247-291`, `TableChart.tsx:293-314`), causing backend queries issued from drill/cross-filter interactions on that table to reference invalid columns and fail. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e64fae5ca1b049e08de55b6c0c520b69&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=e64fae5ca1b049e08de55b6c0c520b69&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/plugins/plugin-chart-table/src/transformProps.ts **Line:** 702:718 **Comment:** *Api Mismatch: This appends URL-link columns into the main `columns` metadata even when they are not query result columns, which makes downstream table logic treat them as real datasource fields (for filtering/drill-to-detail). That can emit filters for non-existent columns and break backend drill/filter flows. Keep virtual link columns separate from query columns (or explicitly exclude them from filter/drill column sets). 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%2F22604&comment_hash=962ed2ad71fc8c084f7a750b8065a45873d3148e97987f8d08e54e1aeac71ea0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F22604&comment_hash=962ed2ad71fc8c084f7a750b8065a45873d3148e97987f8d08e54e1aeac71ea0&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-chart-controls/src/utils/getUrlLinks.ts: ########## @@ -0,0 +1,42 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import memoizeOne from 'memoize-one'; +import { DataRecord } from '@superset-ui/core'; +import { UrlLinkConfig } from '../types'; + +const parseLinkSchema = (schema: string) => schema.match(/(?<=\$\{).*?(?=\})/g); + +const genLinkHref = (values: DataRecord, schema: string) => { + let result = schema; + parseLinkSchema(schema)?.forEach(name => { + const val = values[name]?.toString() ?? ''; + result = result.replace(`$\{${name}}`, val); + }); + return result; +}; + +export const getTextFromValues = memoizeOne( + (columnConfig: UrlLinkConfig[] | undefined) => + columnConfig?.map(({ columnName, linkText, linkSchema }) => ({ + column: columnName!, + linkText: linkText!, + getTextFromValues: (_val: number, values: DataRecord) => + genLinkHref(values, linkSchema!), Review Comment: **Suggestion:** `UrlLinkConfig` fields are optional, but this mapping force-unwraps them with non-null assertions; if a persisted config is incomplete or malformed, `linkSchema` can be `undefined` and `genLinkHref` will crash at runtime. Filter invalid entries (or default safely) before creating link handlers. [null pointer] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Malformed url_link configs crash table chart rendering. - ⚠️ Imported dashboards with bad config become unusable. - ⚠️ Debugging broken slices is harder for end users. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Persist a table chart whose `rawFormData.url_link` contains an entry missing `linkSchema` (or other fields) even though `UrlLinkConfig` marks them optional (`superset-frontend/packages/superset-ui-chart-controls/src/types.ts:173-177`), for example `{ "columnName": "Issue link", "linkText": "Open" }` without `linkSchema`. 2. When the chart is loaded, `transformProps` merges `rawFormData` and reads `url_link: urlLink` from `formData` (`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:61-87`), then calls `const columnUrlLinks = getTextFromValues(urlLink) ?? []` (`transformProps.ts:248-250`). 3. Inside `getTextFromValues`, the code force-unwraps the optional fields with non-null assertions (`getUrlLinks.ts:36-40`), so for the malformed entry `linkSchema!` is `undefined` and `genLinkHref(values, linkSchema!)` is invoked with `schema` effectively `undefined`. 4. The next time `TableChart` renders a cell for that configured column and calls `matchedUrlLink.getTextFromValues(value as number, row.original)` (`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:861-873`), `genLinkHref` executes `schema.match(...)` on `undefined` (`getUrlLinks.ts:23-27`), throwing a runtime TypeError and breaking the table visualization. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87e5cae7efc947c6ae93c2cebd04ab6c&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=87e5cae7efc947c6ae93c2cebd04ab6c&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/packages/superset-ui-chart-controls/src/utils/getUrlLinks.ts **Line:** 36:40 **Comment:** *Null Pointer: `UrlLinkConfig` fields are optional, but this mapping force-unwraps them with non-null assertions; if a persisted config is incomplete or malformed, `linkSchema` can be `undefined` and `genLinkHref` will crash at runtime. Filter invalid entries (or default safely) before creating link handlers. 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%2F22604&comment_hash=f0eb1ec8376c037d161277b07324f2aaef52dd0d7e83721ebdc81a08fc703043&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F22604&comment_hash=f0eb1ec8376c037d161277b07324f2aaef52dd0d7e83721ebdc81a08fc703043&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]
