codeant-ai-for-open-source[bot] commented on code in PR #22604: URL: https://github.com/apache/superset/pull/22604#discussion_r3485734417
########## superset-frontend/src/explore/components/controls/UrlLinkControl/UrlLinkPopoverContent.test.tsx: ########## @@ -0,0 +1,35 @@ +/** + * 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 React from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; +import { UrlLinkPopoverContent } from './UrlLinkPopoverContent'; + +const renderComponent = (onChange = jest.fn()) => { + render(<UrlLinkPopoverContent onChange={onChange} columns={[]} />); +}; + +test('Should render', () => { + renderComponent(); + expect(screen.getByText('link column name')).toBeInTheDocument(); +}); + +test('Should render required fields', () => { + renderComponent(); + expect(screen.getAllByText('Required').length).toEqual(3); Review Comment: **Suggestion:** This assertion expects validation error text before any submit/validation event, but Ant Design form rules do not surface error messages until validation runs, so this test is incorrect and can fail nondeterministically. Trigger form submission first (or assert required markers/field constraints instead of error messages). [logic error] <details> <summary><b>Severity Level:</b> Critical ๐จ</summary> ```mdx - โ UrlLinkPopoverContent Jest test fails on every run. - โ ๏ธ CI for frontend PRs can be blocked by this test. - โ ๏ธ Misrepresents actual form behavior by asserting non-guaranteed errors. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Run the Jest test file `UrlLinkPopoverContent.test.tsx` using the command from the PR description, which targets `superset-frontend/src/explore/components/controls/UrlLinkControl/UrlLinkPopoverContent.test.tsx:27-35`. 2. The helper `renderComponent()` at `UrlLinkPopoverContent.test.tsx:23-25` renders the `UrlLinkPopoverContent` component without simulating any submit or change events, so no explicit validation is triggered. 3. `UrlLinkPopoverContent` (`superset-frontend/src/explore/components/controls/UrlLinkControl/UrlLinkPopoverContent.tsx:58-88`) renders an Ant Design `Form` with three `FormItem` fields using `rulesRequired = [{ required: true, message: t('Required') }]` (`UrlLinkPopoverContent.tsx:44`), but Ant Design only injects the error message text into the DOM after validation runs (e.g., on submit or `validateTrigger`), not immediately on mount. 4. The test assertion at `UrlLinkPopoverContent.test.tsx:34` calls `screen.getAllByText('Required')`, expecting three DOM nodes with the text `'Required'`, but since no validation has been triggered, there are zero such nodes and `getAllByText` throws, causing this test to fail reliably whenever the test suite is executed. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f6212d3acef84fb0a0f89226e49b7262&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=f6212d3acef84fb0a0f89226e49b7262&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/explore/components/controls/UrlLinkControl/UrlLinkPopoverContent.test.tsx **Line:** 34:34 **Comment:** *Logic Error: This assertion expects validation error text before any submit/validation event, but Ant Design form rules do not surface error messages until validation runs, so this test is incorrect and can fail nondeterministically. Trigger form submission first (or assert required markers/field constraints instead of error messages). 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=39b8a032be0e1a7913d28f479aa6a02cfa67d839bea93d2cb6a2f791bb40dad3&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F22604&comment_hash=39b8a032be0e1a7913d28f479aa6a02cfa67d839bea93d2cb6a2f791bb40dad3&reaction=dislike'>๐</a> ########## superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx: ########## @@ -1253,6 +1256,30 @@ export default function TableChart<D extends DataRecord = DataRecord>( // eslint-disable-next-line react/no-danger return <StyledCell {...cellProps} dangerouslySetInnerHTML={html} />; } + + let urlLinkHref; + let linkTextString; + if (hasColumnUrlLinks) { + const matchedUrlLink = columnUrlLinks?.find( + urlLink => urlLink.column === column.key, + ); + if (matchedUrlLink) { + urlLinkHref = matchedUrlLink.getTextFromValues( + value as number, + row.original, + ); + linkTextString = matchedUrlLink.linkText; + } + } + if (urlLinkHref) { + return ( + <StyledCell {...cellProps}> + <a target="_blank" href={urlLinkHref} rel="noreferrer"> + {linkTextString} + </a> Review Comment: **Suggestion:** The rendered link URL is inserted directly into `href` without sanitization, so a crafted schema like `javascript:...` can execute script when a user clicks the cell. Sanitize the generated URL before rendering (or reject non-http/https schemes) to prevent scriptable links. [security] <details> <summary><b>Severity Level:</b> Critical ๐จ</summary> ```mdx - โ Table chart links can execute arbitrary scripts on click. - โ ๏ธ Dashboard users exposed to config-driven XSS payloads. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Open or create a chart using the Table visualization plugin, registered as `TableChartPlugin` at `superset-frontend/plugins/plugin-chart-table/src/index.ts:69-81`. 2. In the chartโs control panel, configure the โUrl Linkโ control (defined at `superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:12-31`, rendered by `UrlLinkControl` in `superset-frontend/src/explore/components/controls/UrlLinkControl/UrlLinkControl.tsx:6-21`) to add a `UrlLinkConfig` entry stored in `formData.url_link` (`superset-frontend/plugins/plugin-chart-table/src/types.ts:28-47`), whose template resolves to a URL string such as `javascript:alert(1)`. 3. When the chart runs, `transformProps()` at `superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:12-29` extracts `url_link` as `urlLink` (`transformProps.ts:37-57`) and computes `columnUrlLinks = getTextFromValues(urlLink) ?? []` (`transformProps.ts:218-220`), passing `columnUrlLinks` into `TableChart` via the transformed props. 4. Inside `TableChart` (`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:374-412`), the Cell renderer computes `urlLinkHref = matchedUrlLink.getTextFromValues(value as number, row.original)` and `linkTextString = matchedUrlLink.linkText` (`TableChart.tsx:1260-1272`) and renders `<a target="_blank" href={urlLinkHref} rel="noreferrer">` (`TableChart.tsx:1277-1279`) without validating or restricting the URL scheme. 5. Load the chart and click the rendered link cell; the browser navigates to `urlLinkHref`, and if it is a `javascript:` or similar scriptable URL, the embedded script executes on click, demonstrating that untrusted link configuration can inject executable script into the userโs browsing context. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7e12bae19ca74de2a7c9289b2e687652&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=7e12bae19ca74de2a7c9289b2e687652&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/TableChart.tsx **Line:** 1277:1279 **Comment:** *Security: The rendered link URL is inserted directly into `href` without sanitization, so a crafted schema like `javascript:...` can execute script when a user clicks the cell. Sanitize the generated URL before rendering (or reject non-http/https schemes) to prevent scriptable links. 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=ecad681b1dd37223a0d9e3ed61a28791bf9c99f18a3590c7909182044a6b245d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F22604&comment_hash=ecad681b1dd37223a0d9e3ed61a28791bf9c99f18a3590c7909182044a6b245d&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]
