codeant-ai-for-open-source[bot] commented on code in PR #35859:
URL: https://github.com/apache/superset/pull/35859#discussion_r3454821068
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -175,34 +239,134 @@ function CountryMap(element: HTMLElement, props:
CountryMapProps) {
.classed('popup-at-bottom', y > (svgHeight * 2) / 3);
};
- const mouseenter = function mouseenter(this: SVGPathElement, d: GeoFeature) {
+ const mouseenter = function mouseenter(
+ this: SVGPathElement,
+ d: GeoFeature,
+ ): void {
// Darken color
let c: string = colorFn(d);
- if (c !== 'none') {
+ if (c) {
c = d3.rgb(c).darker().toString();
}
d3.select(this).style('fill', c);
- // Display information popup
- const result = data.filter(
- region => region.country_id === d.properties.ISO,
- );
- hoverPopup.style('display', 'block').html(
- `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ?
formatter(result[0].metric) : ''}</div>`,
- );
+ // Display information popup
+ const result = data.filter(r => r.country_id === d?.properties?.ISO);
+ hoverPopup
+ .style('display', 'block')
+ .html(
+ `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ?
formatter(result[0].metric) : ''}</div>`,
+ );
Review Comment:
**Suggestion:** The tooltip HTML is built with raw region text and injected
via `.html(...)` without escaping, which allows script/markup injection if map
properties ever contain unsafe strings. Use escaped text (or text nodes) for
region names and formatted values before rendering tooltip content. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Malicious GeoJSON labels can execute script on hover.
- ⚠️ Country Map tooltips render unescaped region labels and metrics.
- ⚠️ Inconsistent escaping compared with other Country Map messages.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Country Map chart loads GeoJSON for the selected country via
`d3.json(url, (error,
mapData) => { ... })` in
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:47-58`,
where
`url` comes from the local `countries` mapping (`countries.ts:20-260`) and
each feature
carries `NAME_1`/`NAME_2` properties used as region labels.
2. When a user hovers a region, the `mouseenter` handler in
`CountryMap.ts:242-261`
filters `data` to find the metric for `d?.properties?.ISO` and then builds
the tooltip by
calling `hoverPopup.html(...)` with a template string at
`CountryMap.ts:255-259` that
interpolates `getNameOfRegion(d)` and `formatter(result[0].metric)`.
3. `getNameOfRegion` (`CountryMap.ts:220-230`) returns
`feature.properties.NAME_2` or
`NAME_1` verbatim with no escaping, and the metric formatter is produced in
`transformProps.ts:50-60` without any HTML sanitization. Although an
`escapeHtml` helper
exists and is used for error messages (`CountryMap.ts:33-40`,
`CountryMap.ts:40-52`), it
is not applied to these tooltip values.
4. If any GeoJSON feature's `NAME_1`/`NAME_2` (or formatted metric) contains
HTML or
script markup (for example, modifying one of the `*.geojson` files under
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/countries/`
to set `NAME_1`
to `<img src=x onerror=alert(1)>`), hovering that region will cause
`hoverPopup.html(...)`
to inject that markup directly into the DOM, resulting in script execution
within the
Superset UI.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2d265fe1c4049f5b88de3591942bbb9&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=d2d265fe1c4049f5b88de3591942bbb9&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-plugin-chart-country-map/src/CountryMap.ts
**Line:** 255:259
**Comment:**
*Security: The tooltip HTML is built with raw region text and injected
via `.html(...)` without escaping, which allows script/markup injection if map
properties ever contain unsafe strings. Use escaped text (or text nodes) for
region names and formatted values before rendering tooltip content.
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%2F35859&comment_hash=84ebe7441195db13e56f4c1c79d53dc85e6b5e0b96de41c73bc23b2ae2641f70&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=84ebe7441195db13e56f4c1c79d53dc85e6b5e0b96de41c73bc23b2ae2641f70&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -112,46 +146,76 @@ function CountryMap(element: HTMLElement, props:
CountryMapProps) {
.attr('width', width)
.attr('height', height)
.attr('preserveAspectRatio', 'xMidYMid meet');
+
+ // Only set grab cursor if not in edit mode
+ if (!isEditMode) {
+ svg.style('cursor', 'grab');
+ }
const backgroundRect = svg
.append('rect')
.attr('class', 'background')
.attr('width', width)
.attr('height', height);
const g = svg.append('g');
const mapLayer = g.append('g').classed('map-layer', true);
+ // Add hover popup for tooltip
const hoverPopup = div.append('div').attr('class', 'hover-popup');
- let centered: GeoFeature | null;
-
- const clicked = function clicked(d: GeoFeature) {
- const hasCenter = d && centered !== d;
- let x: number;
- let y: number;
- let k: number;
- const halfWidth = width / 2;
- const halfHeight = height / 2;
-
- if (hasCenter) {
- const centroid = path.centroid(d);
- [x, y] = centroid;
- k = 4;
- centered = d;
- } else {
- x = halfWidth;
- y = halfHeight;
- k = 1;
- centered = null;
- }
+ // Track mouse position to distinguish clicks from drags
+ let mousedownPos: { x: number; y: number } | null = null;
- g.transition()
- .duration(750)
- .attr(
- 'transform',
-
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
- );
+ // Cross-filter support
+ const getCrossFilterDataMask = (
+ source: GeoFeature,
+ ): { dataMask: DataMask; isCurrentValueSelected: boolean } | undefined => {
+ if (!entity) return undefined;
+
+ const selected = filterState?.selectedValues || [];
+ const iso = source?.properties?.ISO;
+ if (!iso) return undefined;
+
+ const isSelected = selected.includes(iso);
+ const values = isSelected ? [] : [iso];
Review Comment:
**Suggestion:** Multi-select cross-filtering is not actually implemented:
the selection logic always collapses to either one ISO code or none, so
Shift+Click cannot add/remove from an existing selection set. This will make
users lose prior selections on every click. Update the click/data-mask logic to
branch on the pointer event's Shift state and merge/toggle within
`filterState.selectedValues` instead of forcing a single-value array.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Country Map cross-filters collapse to a single region.
- ⚠️ Users lose previous region selection on each click.
- ⚠️ Dashboard filters less expressive than PR description promises.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard containing a Country Map chart (plugin registered in
`superset-frontend/plugins/legacy-plugin-chart-country-map/src/index.ts:30-66`),
configured with an `entity` column and cross-filtering enabled so
`emitCrossFilters`,
`filterState`, and `setDataMask` are passed via `transformProps`
(`transformProps.ts:21-32`, `transformProps.ts:62-79`).
2. In view mode, click any region on the Country Map. The SVG path click
handler in
`CountryMap.ts:400-405` calls `handleClick(feature)` defined at
`CountryMap.ts:352-367`,
which in turn calls `getCrossFilterDataMask(feature)`
(`CountryMap.ts:168-193`) and then
invokes `setDataMask(dataMask)`.
3. Inspect `getCrossFilterDataMask` in `CountryMap.ts:168-193`: it reads the
current
`filterState?.selectedValues || []` into `selected` (`CountryMap.ts:173`),
but then
computes `values` as either `[]` or `[iso]` (`CountryMap.ts:177-178`),
discarding any
other previously selected ISO codes. `filterState.selectedValues` in the
emitted
`dataMask` is therefore always `null` or a single-element array.
4. With one region already selected, Shift+Click a second region. Because
the click
handler in `CountryMap.ts:400-405` does not examine `d3.event` or any
`shiftKey` state,
and `handleClick` (`CountryMap.ts:352-367`) always replaces the selection
(`newSelection`
is `[]` or `[iso]` at `CountryMap.ts:364-366`), the second click clears the
first
selection instead of toggling it, so multi-select cross-filtering as
described in the PR
cannot work.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9cd3bcf77f0b44cfb94dd968184be65b&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=9cd3bcf77f0b44cfb94dd968184be65b&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-plugin-chart-country-map/src/CountryMap.ts
**Line:** 177:178
**Comment:**
*Incomplete Implementation: Multi-select cross-filtering is not
actually implemented: the selection logic always collapses to either one ISO
code or none, so Shift+Click cannot add/remove from an existing selection set.
This will make users lose prior selections on every click. Update the
click/data-mask logic to branch on the pointer event's Shift state and
merge/toggle within `filterState.selectedValues` instead of forcing a
single-value array.
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%2F35859&comment_hash=7e03a4bf6fb8edc71bc9a9136de2f738d634120a08ac773e5978c6d139abfd6d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35859&comment_hash=7e03a4bf6fb8edc71bc9a9136de2f738d634120a08ac773e5978c6d139abfd6d&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]
