codeant-ai-for-open-source[bot] commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2759112584
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts:
##########
@@ -16,15 +16,20 @@
* specific language governing permissions and limitations
* under the License.
*/
-export default function transformProps(chartProps) {
+import { ChartProps } from '@superset-ui/core';
+
+export default function transformProps(chartProps: ChartProps) {
const { height, width, formData, queriesData } = chartProps;
- const { horizonColorScale, seriesHeight } = formData;
+ const {
+ horizon_color_scale: horizonColorScale,
+ series_height: seriesHeight,
+ } = formData;
return {
- colorScale: horizonColorScale,
+ colorScale: horizonColorScale as string | undefined,
Review Comment:
**Suggestion:** When `horizon_color_scale` is missing from `formData`,
`colorScale` is explicitly set to `undefined`, which overrides the React
component's `defaultProps` and causes the chart to lose its intended default
value instead of falling back to `'series'`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Horizon charts use wrong color scale by default.
- ⚠️ Visual regressions for saved charts missing the field.
- ⚠️ Dashboard appearance inconsistent across users.
```
</details>
```suggestion
colorScale: (horizonColorScale as string | undefined) ?? 'series',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load a dashboard or chart that uses the horizon chart plugin so the
plugin transform
runs. The transform implementation is in
superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts
(return block
around line 29).
2. Construct chartProps with formData that does NOT include the
horizon_color_scale key
(e.g., formData = { /* no horizon_color_scale */ , series_height: 30 } ).
This is a
realistic scenario because many existing saved charts omit optional form
fields.
3. Call transformProps(chartProps) (the function exported from the file
above). The
function destructures horizon_color_scale into horizonColorScale and then
executes the
return at line 29, producing the property colorScale: horizonColorScale as
string |
undefined.
4. Because horizon_color_scale is absent, horizonColorScale is undefined and
the code sets
colorScale explicitly to undefined (the cast does not change the value).
That explicit
undefined is then passed downstream into the chart component props (the
plugin's viz
component receives the returned props), which overrides any React
defaultProps or internal
fallback logic and prevents the chart from using the intended default
'series' color
scale.
5. Observe the visual change: the horizon chart no longer uses the expected
default color
scale (saved/default behavior), causing mismatched appearance for charts
that relied on
the implicit default. This reproduces deterministically whenever formData
has no
horizon_color_scale.
Note: The reproduction traces directly to transformProps at
superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts
(return
object, line 29) and requires no other code paths.
```
</details>
<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-horizon/src/transformProps.ts
**Line:** 29:29
**Comment:**
*Logic Error: When `horizon_color_scale` is missing from `formData`,
`colorScale` is explicitly set to `undefined`, which overrides the React
component's `defaultProps` and causes the chart to lose its intended default
value instead of falling back to `'series'`.
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.
```
</details>
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -213,8 +234,16 @@ function CountryMap(element, props) {
if (map) {
drawMap(map);
} else {
- const url = countries[country];
- d3.json(url, (error, mapData) => {
+ const url = (countries as Record<string, string>)[country];
+ if (!url) {
+ const countryName =
+ countryOptions.find(x => x[0] === country)?.[1] || country;
+ d3.select(element).html(
+ `<div class="alert alert-danger">No map data available for
${countryName}</div>`,
+ );
Review Comment:
**Suggestion:** The error message for missing country map data directly
interpolates the country name into an HTML string via `.html(...)` without
escaping, so if the `country` parameter is user-controlled an attacker could
inject arbitrary HTML/JavaScript and trigger an XSS when this path is hit.
[security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard country-map alert can execute attacker HTML.
- ⚠️ Visualizations using CountryMap may show injected content.
- ⚠️ Admin/editor-supplied chart configuration affected.
```
</details>
```suggestion
const containerSelection = d3.select(element);
containerSelection.selectAll('*').remove();
containerSelection
.append('div')
.attr('class', 'alert alert-danger')
.text(`No map data available for ${countryName}`);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Locate the CountryMap component at
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.ts
around lines
235-246 (the else branch handling absent cached map). The code at 238-245
writes HTML via
d3.select(element).html(...).
2. Render a chart that uses this CountryMap (any Superset chart/dashboard
that selects
this legacy country-map plugin). When the CountryMap code path executes the
"no url"
branch (lines 238-245), it computes countryName then calls
d3.select(element).html(`<div
...>${countryName}</div>`).
3. Provide a country identifier containing malicious markup (for example a
country key
that is not found in countryOptions so the raw country value is used). When
CountryMap
runs the branch in CountryMap.ts:238-245 the unescaped value is interpolated
and inserted
as HTML, executing script if present.
4. Observe script execution in the browser (XSS) when the malicious country
value is used
and the "no map data" branch is triggered. The exact vulnerable lines are
the template
interpolation in CountryMap.ts lines 241-242 where countryName is placed
into .html(...).
Note: The final file shows direct .html(template) insertion (CountryMap.ts
lines
~238-245). Replacing .html(...) with element.append().text(...) (as
proposed) avoids
injecting raw HTML and mitigates XSS.
```
</details>
<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:** 241:243
**Comment:**
*Security: The error message for missing country map data directly
interpolates the country name into an HTML string via `.html(...)` without
escaping, so if the `country` parameter is user-controlled an attacker could
inject arbitrary HTML/JavaScript and trigger an XSS when this path is hit.
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.
```
</details>
##########
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.ts:
##########
@@ -95,11 +97,20 @@ function Calendar(element, props) {
calContainer.text(`${METRIC_TEXT}: ${verboseMap[metric] || metric}`);
}
const timestamps = metricsData[metric];
- const extents = d3Extent(Object.keys(timestamps), key => timestamps[key]);
- const step = (extents[1] - extents[0]) / (steps - 1);
- const colorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(extents);
+ const rawExtents = d3Extent(
+ Object.keys(timestamps),
+ key => timestamps[key],
+ );
+ // Guard against undefined extents (empty data)
+ const extents: [number, number] =
+ rawExtents[0] != null && rawExtents[1] != null
+ ? [rawExtents[0], rawExtents[1]]
+ : [0, 1];
+ const step = (extents[1] - extents[0]) / (steps - 1) || 0;
+ const colorScheme = getSequentialSchemeRegistry().get(linearColorScheme);
+ const colorScale = colorScheme
+ ? colorScheme.createLinearScale(extents)
+ : (v: number) => '#ccc'; // fallback if scheme not found
const legend = d3Range(steps).map(i => extents[0] + step * i);
Review Comment:
**Suggestion:** When the configured number of color steps is 1 or less, the
current step calculation divides by zero so `step` becomes `Infinity`, which
then produces `NaN` legend values and can result in invalid colors or runtime
issues in the heatmap rendering; you should guard against `steps <= 1` by
computing a finite step and a minimal legend array in that case. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Calendar chart legend can render invalid colors.
- ❌ CalHeatMap initialization may throw runtime errors.
- ⚠️ Visual regression for calendar chart consumers.
- ⚠️ Affects chart rendering in dashboards using this plugin.
```
</details>
```suggestion
const step =
steps > 1 ? (extents[1] - extents[0]) / (steps - 1) : 0;
const colorScheme = getSequentialSchemeRegistry().get(linearColorScheme);
const colorScale = colorScheme
? colorScheme.createLinearScale(extents)
: (v: number) => '#ccc'; // fallback if scheme not found
const legend =
steps > 1
? d3Range(steps).map(i => extents[0] + step * i)
: [extents[0]];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the Calendar chart with props where steps is set to 1 (e.g., a
chart
configuration using the calendar plugin). The Calendar constructor is
defined in
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.ts and
the loop that
computes legend values starts at lines 97-116 (see lines 100-116 where
extents/step/legend
are computed).
2. Execution enters the Object.keys(timestamps) iteration at
Calendar.ts:97-99 and
computes rawExtents at Calendar.ts:100-103 ("const rawExtents =
d3Extent(...)").
3. The current code computes step as (extents[1] - extents[0]) / (steps - 1)
at
Calendar.ts:106-110. With steps === 1 this becomes a division by zero
leading step to be
Infinity; the fallback "|| 0" does not catch Infinity, so step remains
Infinity.
4. d3Range(steps) at Calendar.ts:114 becomes d3Range(1) producing [0], then
legend
calculation at Calendar.ts:114 ("extents[0] + step * i") yields NaN
(extents[0] + Infinity
* 0 may be NaN depending on step), and legendColors computed at
Calendar.ts:116
("legend.map(x => colorScale(x))") receives invalid values causing
colorScale or
downstream CalHeatMap.init to be given invalid legend/colour inputs, which
can produce
rendering errors or runtime exceptions when the heatmap initializes.
5. Observe broken legend rendering or console errors when CalHeatMap.init is
called at
Calendar.ts:118-... with legend/legendColors containing NaN or invalid color
values.
Note: this reproduction is based on concrete code locations in
superset-frontend/plugins/legacy-plugin-chart-calendar/src/Calendar.ts
(lines referenced
above). The suggested guard (steps > 1) avoids division by zero and provides
a
single-value legend when steps === 1.
```
</details>
<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-calendar/src/Calendar.ts
**Line:** 109:115
**Comment:**
*Logic Error: When the configured number of color steps is 1 or less,
the current step calculation divides by zero so `step` becomes `Infinity`,
which then produces `NaN` legend values and can result in invalid colors or
runtime issues in the heatmap rendering; you should guard against `steps <= 1`
by computing a finite step and a minimal legend array in that case.
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.
```
</details>
--
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]
