Copilot commented on code in PR #40624:
URL: https://github.com/apache/superset/pull/40624#discussion_r3337855750
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts:
##########
@@ -379,7 +379,16 @@ function simpleFilterToWhereClause(
}
if (type === FILTER_OPERATORS.IN_RANGE && filterTo !== undefined) {
- return `${columnName} ${SQL_OPERATORS.BETWEEN} ${value} AND ${filterTo}`;
+ // BETWEEN bounds are interpolated directly into the WHERE clause, so only
+ // accept finite numeric bounds (date ranges are handled separately above).
+ // Numeric strings from serialized filter state are coerced; anything that
+ // isn't a finite number is dropped rather than concatenated as raw SQL.
+ const from = Number(value);
+ const to = Number(filterTo);
+ if (!Number.isFinite(from) || !Number.isFinite(to)) {
+ return '';
+ }
+ return `${columnName} ${SQL_OPERATORS.BETWEEN} ${from} AND ${to}`;
}
Review Comment:
`filterTo` is typed as `FilterValue` (can be `null` or an empty string). The
current guard only checks `filterTo !== undefined`, so `null`/`''` will be
coerced by `Number()` to `0` and can incorrectly generate `BETWEEN ... AND 0`
instead of dropping the condition. Explicitly reject `null`/empty-string (and
non string/number types) for both bounds before coercion.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts:
##########
@@ -771,6 +771,57 @@ describe('agGridFilterConverter', () => {
// Should reject column names longer than 255 characters
expect(result.simpleFilters).toHaveLength(0);
});
+
+ test('should drop inRange bounds that are not numeric', () => {
+ const filterModel: AgGridFilterModel = {
+ age: {
+ filterType: 'number',
+ operator: 'AND',
+ condition1: {
+ filterType: 'number',
+ type: 'inRange',
+ filter: '0 AND 1=1--',
+ filterTo: '100',
+ },
+ condition2: {
+ filterType: 'number',
+ type: 'greaterThan',
+ filter: 5,
+ },
+ } as AgGridCompoundFilter,
+ };
+
+ const result = convertAgGridFiltersToSQL(filterModel);
+
+ // The malicious range condition is dropped, so its payload never reaches
+ // the WHERE clause; the sibling numeric condition is unaffected.
+ expect(result.complexWhere ?? '').not.toContain('1=1');
+ expect(result.complexWhere ?? '').not.toContain('BETWEEN');
+ });
Review Comment:
The new test verifies the injected payload is absent, but it doesn't assert
that the sibling valid condition is preserved. As written, this would also pass
if the whole compound filter were dropped, so it may not catch regressions in
filter composition.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts:
##########
@@ -771,6 +771,57 @@ describe('agGridFilterConverter', () => {
// Should reject column names longer than 255 characters
expect(result.simpleFilters).toHaveLength(0);
});
+
+ test('should drop inRange bounds that are not numeric', () => {
+ const filterModel: AgGridFilterModel = {
+ age: {
+ filterType: 'number',
+ operator: 'AND',
+ condition1: {
+ filterType: 'number',
+ type: 'inRange',
+ filter: '0 AND 1=1--',
+ filterTo: '100',
+ },
+ condition2: {
+ filterType: 'number',
+ type: 'greaterThan',
+ filter: 5,
+ },
+ } as AgGridCompoundFilter,
+ };
+
+ const result = convertAgGridFiltersToSQL(filterModel);
+
+ // The malicious range condition is dropped, so its payload never reaches
+ // the WHERE clause; the sibling numeric condition is unaffected.
+ expect(result.complexWhere ?? '').not.toContain('1=1');
+ expect(result.complexWhere ?? '').not.toContain('BETWEEN');
+ });
+
+ test('should keep numeric inRange bounds (including numeric strings)', ()
=> {
+ const filterModel: AgGridFilterModel = {
+ age: {
+ filterType: 'number',
+ operator: 'AND',
+ condition1: {
+ filterType: 'number',
+ type: 'inRange',
+ filter: '18',
+ filterTo: 65,
+ },
+ condition2: {
+ filterType: 'number',
+ type: 'lessThan',
+ filter: 100,
+ },
+ } as AgGridCompoundFilter,
+ };
+
+ const result = convertAgGridFiltersToSQL(filterModel);
+
+ expect(result.complexWhere).toContain('BETWEEN 18 AND 65');
+ });
Review Comment:
This assertion only checks that the BETWEEN fragment exists; it won't fail
if the upper bound is wrong or if the second condition is omitted. Asserting
the full compound WHERE clause makes the test more robust and directly
validates the numeric coercion behavior.
--
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]
