bito-code-review[bot] commented on code in PR #40100:
URL: https://github.com/apache/superset/pull/40100#discussion_r3245399804
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +126,102 @@ function processPolygonData(
const excludeKeys = new Set([line_column, ...(js_columns || [])]);
- return records
- .map(record => {
- let feature: PolygonFeature = {
- extraProps: {},
- metrics: {},
- };
-
- feature = addJsColumnsToExtraProps(feature, record, js_columns);
- const updatedFeature = addPropertiesToFeature(
- feature as unknown as Record<string, unknown>,
- record,
- excludeKeys,
- );
- feature = updatedFeature as unknown as PolygonFeature;
-
- const rawPolygonData = record[line_column];
- if (!rawPolygonData) {
- return null;
- }
+ return records.flatMap(record => {
+ let feature: PolygonFeature = {
+ extraProps: {},
+ metrics: {},
+ };
+
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ const updatedFeature = addPropertiesToFeature(
+ feature as unknown as Record<string, unknown>,
+ record,
+ excludeKeys,
+ );
+ feature = updatedFeature as unknown as PolygonFeature;
+
+ const rawPolygonData = record[line_column];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1321: Object injection via dynamic property
access</b></div>
<div id="fix">
Dynamic property access `record[line_column]` on line 143 is flagged as a
potential object injection vulnerability
([CWE-1321](https://cwe.mitre.org/data/definitions/1321.html)). While
`line_column` is a form data parameter, validate or sanitize it to prevent
prototype pollution attacks. Similar issues exist on lines 198, 199, 205, 206,
and 211.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- const rawPolygonData = record[line_column];
+ // Validate line_column is a safe property key
+ if (typeof line_column !== 'string' ||
!line_column.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
+ const rawPolygonData = record[line_column];
if (!rawPolygonData) {
return [];
}
@@ -198,6 +211,10 @@
} else if (elevationLabel && record[elevationLabel] != null) {
+ // Validate elevationLabel is a safe property key
+ if (typeof elevationLabel !== 'string' ||
!elevationLabel.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
const elevationValue = parseMetricValue(record[elevationLabel]);
@@ -205,6 +218,10 @@
if (metricLabel && record[metricLabel] != null) {
+ // Validate metricLabel is a safe property key
+ if (typeof metricLabel !== 'string' ||
!metricLabel.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
const metricValue = record[metricLabel];
```
</div>
</details>
</div>
<small><i>Code Review Run #343f21</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +126,102 @@ function processPolygonData(
const excludeKeys = new Set([line_column, ...(js_columns || [])]);
- return records
- .map(record => {
- let feature: PolygonFeature = {
- extraProps: {},
- metrics: {},
- };
-
- feature = addJsColumnsToExtraProps(feature, record, js_columns);
- const updatedFeature = addPropertiesToFeature(
- feature as unknown as Record<string, unknown>,
- record,
- excludeKeys,
- );
- feature = updatedFeature as unknown as PolygonFeature;
-
- const rawPolygonData = record[line_column];
- if (!rawPolygonData) {
- return null;
- }
+ return records.flatMap(record => {
+ let feature: PolygonFeature = {
+ extraProps: {},
+ metrics: {},
+ };
+
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ const updatedFeature = addPropertiesToFeature(
+ feature as unknown as Record<string, unknown>,
+ record,
+ excludeKeys,
+ );
+ feature = updatedFeature as unknown as PolygonFeature;
+
+ const rawPolygonData = record[line_column];
+ if (!rawPolygonData) {
+ return [];
+ }
- try {
- let polygonCoords: number[][];
-
- switch (line_type) {
- case 'json': {
- const parsed =
- typeof rawPolygonData === 'string'
- ? JSON.parse(rawPolygonData)
- : rawPolygonData;
-
- if (parsed.coordinates) {
- polygonCoords = parsed.coordinates[0] || parsed.coordinates;
- } else if (parsed.geometry?.coordinates) {
- // Non-standard format with nested geometry
- polygonCoords =
- parsed.geometry.coordinates[0] || parsed.geometry.coordinates;
- } else if (Array.isArray(parsed)) {
- polygonCoords = parsed;
- } else {
- return null;
- }
- break;
+ try {
+ let polygonCoordParts: PolygonCoordinates[];
+
+ switch (line_type) {
+ case 'json': {
+ const parsed =
+ typeof rawPolygonData === 'string'
+ ? JSON.parse(rawPolygonData)
+ : rawPolygonData;
+ const parsedPolygonCoordParts = getPolygonCoordinateParts(parsed);
+
+ if (!parsedPolygonCoordParts) {
+ return [];
}
- case 'geohash':
- polygonCoords = [];
- const decoded = decode_bbox(String(rawPolygonData));
- if (decoded) {
- polygonCoords.push([decoded[1], decoded[0]]); // SW (minLon,
minLat)
- polygonCoords.push([decoded[1], decoded[2]]); // NW (minLon,
maxLat)
- polygonCoords.push([decoded[3], decoded[2]]); // NE (maxLon,
maxLat)
- polygonCoords.push([decoded[3], decoded[0]]); // SE (maxLon,
minLat)
- polygonCoords.push([decoded[1], decoded[0]]); // SW (close
polygon)
- }
- break;
- case 'zipcode':
- default: {
- polygonCoords = Array.isArray(rawPolygonData) ? rawPolygonData :
[];
- break;
+
+ polygonCoordParts = parsedPolygonCoordParts;
+ break;
+ }
+ case 'geohash': {
+ const polygonCoords: PolygonCoordinates = [];
+ const decoded = decode_bbox(String(rawPolygonData));
+ if (decoded) {
+ polygonCoords.push([decoded[1], decoded[0]]); // SW (minLon,
minLat)
+ polygonCoords.push([decoded[1], decoded[2]]); // NW (minLon,
maxLat)
+ polygonCoords.push([decoded[3], decoded[2]]); // NE (maxLon,
maxLat)
+ polygonCoords.push([decoded[3], decoded[0]]); // SE (maxLon,
minLat)
+ polygonCoords.push([decoded[1], decoded[0]]); // SW (close polygon)
}
+ polygonCoordParts = [polygonCoords];
+ break;
}
-
- if (reverse_long_lat && polygonCoords.length > 0) {
- polygonCoords = polygonCoords.map(coord => [coord[1], coord[0]]);
+ case 'zipcode':
+ default: {
+ polygonCoordParts = [
+ Array.isArray(rawPolygonData)
+ ? (rawPolygonData as PolygonCoordinates)
+ : [],
+ ];
+ break;
}
+ }
- feature.polygon = polygonCoords;
+ if (reverse_long_lat) {
+ polygonCoordParts = polygonCoordParts.map(polygonCoords =>
+ polygonCoords.map(coord => [coord[1], coord[0]]),
+ );
+ }
- if (fixedElevationValue !== undefined) {
- feature.elevation = fixedElevationValue;
- } else if (elevationLabel && record[elevationLabel] != null) {
- const elevationValue = parseMetricValue(record[elevationLabel]);
- if (elevationValue !== undefined) {
- feature.elevation = elevationValue;
- }
+ if (fixedElevationValue !== undefined) {
+ feature.elevation = fixedElevationValue;
+ } else if (elevationLabel && record[elevationLabel] != null) {
+ const elevationValue = parseMetricValue(record[elevationLabel]);
+ if (elevationValue !== undefined) {
+ feature.elevation = elevationValue;
}
+ }
- if (metricLabel && record[metricLabel] != null) {
- const metricValue = record[metricLabel];
- if (
- typeof metricValue === 'string' ||
- typeof metricValue === 'number'
- ) {
- feature.metrics![metricLabel] = metricValue;
- }
+ if (metricLabel && record[metricLabel] != null) {
+ const metricValue = record[metricLabel];
+ if (
+ typeof metricValue === 'string' ||
+ typeof metricValue === 'number'
+ ) {
+ feature.metrics![metricLabel] = metricValue;
}
- } catch {
- return null;
}
- return feature;
- })
- .filter((feature): feature is PolygonFeature => feature !== null);
+ return polygonCoordParts.map(polygonCoords => ({
+ ...feature,
+ extraProps: { ...feature.extraProps },
+ metrics: { ...feature.metrics },
+ polygon: polygonCoords,
+ }));
+ } catch {
+ return [];
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broad Exception Handling</b></div>
<div id="fix">
The catch block uses a broad exception handler that catches all exceptions,
potentially masking unexpected errors that indicate bugs. Per BITO.md adaptive
rule [10033], replace with specific exception types (e.g., SyntaxError for
JSON.parse, TypeError for array operations) and rethrow others to allow proper
debugging.
</div>
</div>
<small><i>Code Review Run #343f21</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
