codeant-ai-for-open-source[bot] commented on code in PR #40612:
URL: https://github.com/apache/superset/pull/40612#discussion_r3347415274
##########
superset-frontend/plugins/legacy-preset-chart-nvd3/test/utils.test.ts:
##########
@@ -125,6 +125,56 @@ describe('nvd3/utils', () => {
);
});
+ describe('generateMultiLineTooltipContent()', () => {
+ const identity = (value: any) => value;
+
+ test('renders the series key in the tooltip markup', () => {
+ const tooltip = generateMultiLineTooltipContent(
+ {
+ value: 'x-value',
+ series: [{ key: 'Region A', color: '#fff', value: 1 }],
+ },
+ identity,
+ [identity],
+ );
+ expect(tooltip).toContain('Region A');
+ });
+
+ test('strips a script payload from a malicious series key', () => {
+ const tooltip = generateMultiLineTooltipContent(
+ {
+ value: 'x-value',
+ series: [
+ {
+ key: '<img src=x onerror="alert(1)">',
+ color: '#fff',
+ value: 1,
+ },
+ ],
+ },
+ identity,
+ [identity],
+ );
+ // DOMPurify removes the event handler that would execute on render.
+ expect(tooltip).not.toContain('onerror');
+ expect(tooltip).not.toContain('alert(1)');
+ });
+
+ test('removes script tags injected via the series key', () => {
+ const tooltip = generateMultiLineTooltipContent(
+ {
+ value: 'x-value',
+ series: [
+ { key: '<script>alert(1)</script>', color: '#fff', value: 1 },
+ ],
+ },
+ identity,
+ [identity],
+ );
+ expect(tooltip).not.toContain('<script>');
+ });
Review Comment:
**Suggestion:** This test is a duplicate of the existing
`generateMultiLineTooltipContent() sanitises user input` `<script>`-stripping
test later in the same file, so it adds maintenance overhead without increasing
behavioral coverage. Consolidate these into one test block (or replace this one
with a different payload/assertion) to avoid redundant test logic and future
drift. [code quality]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Test suite includes duplicated XSS sanitization scenario.
- ⚠️ Future changes require updating two equivalent tests.
```
</details>
<details>
<summary><b>Steps of Reproduction âś… </b></summary>
```mdx
1. Open
`superset-frontend/plugins/legacy-preset-chart-nvd3/test/utils.test.ts` and
locate
the `describe('generateMultiLineTooltipContent()', () => { ... })` block
added in this PR
at lines 128–176; inside it, note the test starting at line 163:
`test('removes script
tags injected via the series key', () => { ... })` which calls
`generateMultiLineTooltipContent` with `series[0].key =
'<script>alert(1)</script>'` and
asserts `expect(tooltip).not.toContain('<script>');`.
2. In the same file, scroll down to the later block
`describe('generateMultiLineTooltipContent() sanitises user input', () => {
... })` at
lines 31–48 of the second chunk (overall file lines ~291–308); observe the
test
`test('strips <script> from a series key', () => { ... })` which also calls
`generateMultiLineTooltipContent` with `series[0].key =
'<script>alert(1)</script>'` and
asserts `expect(html).not.toMatch(/<script/i);`.
3. Compare the two tests' setups and expectations: both construct a
single-series payload
differing only in numeric vs string `value` and formatter functions, but in
each case the
behavior under test is identical—DOMPurify-driven removal of `<script>` from
the
`series.key`—with no additional branches or edge cases covered.
4. Run `npm run test -- plugins/legacy-preset-chart-nvd3/test/utils.test.ts`
from
`superset-frontend`; observe that both tests pass and exercise the same
sanitization
scenario for `'<script>alert(1)</script>'` on the same function,
demonstrating duplicated
behavioral coverage and introducing redundant test maintenance for future
changes to this
code path.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a9efb9a93a224f6e88f127c522c78a6d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a9efb9a93a224f6e88f127c522c78a6d&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/legacy-preset-chart-nvd3/test/utils.test.ts
**Line:** 163:175
**Comment:**
*Code Quality: This test is a duplicate of the existing
`generateMultiLineTooltipContent() sanitises user input` `<script>`-stripping
test later in the same file, so it adds maintenance overhead without increasing
behavioral coverage. Consolidate these into one test block (or replace this one
with a different payload/assertion) to avoid redundant test logic and future
drift.
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%2F40612&comment_hash=80e3c04c16d8bb7a0cf8413428de933515efae02702e7a37a22a7814de35524b&reaction=like'>đź‘Ť</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40612&comment_hash=80e3c04c16d8bb7a0cf8413428de933515efae02702e7a37a22a7814de35524b&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]
