bito-code-review[bot] commented on code in PR #39906:
URL: https://github.com/apache/superset/pull/39906#discussion_r3199793968
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/transformProps.ts:
##########
@@ -32,14 +32,25 @@ export default function transformProps(chartProps:
ChartProps) {
}
const records = getRecordsFromQuery(chartProps.queriesData);
+ const crossFilterCol = formData.cross_filter_column || undefined;
// Parse each record's geojson column value (replicates backend
DeckGeoJson.get_properties)
const features = records
.map((record: DataRecord) => {
const geojsonStr = record[geojsonCol];
if (geojsonStr == null) return null;
try {
- return JSON.parse(String(geojsonStr));
+ const feature = JSON.parse(String(geojsonStr));
+ // Surface cross_filter_column from the row onto feature.properties so
+ // that picking can emit a dimension filter even when the GeoJSON blob
+ // doesn't carry the column itself.
+ if (crossFilterCol && record[crossFilterCol] !== undefined) {
+ feature.properties = {
+ ...feature.properties,
+ [crossFilterCol]: record[crossFilterCol],
+ };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1321: Prototype Pollution in Feature Properties
Assignment</b></div>
<div id="fix">
To avoid prototype pollution, validate cross_filter_column before assignment
and exclude dangerous keys such as '__proto__', 'constructor', and 'prototype'
from being used as properties on feature.properties. (See also:
[CWE-1321](https://cwe.mitre.org/data/definitions/1321.html))
</div>
</div>
<small><i>Code Review Run #d9b4fb</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/Geojson/buildQuery.ts:
##########
@@ -61,6 +63,10 @@ export default function buildQuery(formData:
DeckGeoJsonFormData) {
const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns);
columns = withJsColumns as QueryFormColumn[];
+ if (cross_filter_column && !columns.includes(cross_filter_column)) {
+ columns.push(cross_filter_column);
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect duplicate detection</b></div>
<div id="fix">
The duplicate check for `cross_filter_column` fails when `columns` includes
QueryFormColumn objects, as `includes` uses strict equality and won't match a
string against an object with the same label. This could lead to duplicate
columns in the query, potentially causing incorrect results or performance
issues. Use a `some` check with string normalization instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if (cross_filter_column && !columns.some(col => (typeof col === 'string'
? col : col.label || col.sqlExpression || '') === cross_filter_column)) {
columns.push(cross_filter_column);
}
````
</div>
</details>
</div>
<small><i>Code Review Run #d9b4fb</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]
