rusackas commented on code in PR #40624:
URL: https://github.com/apache/superset/pull/40624#discussion_r3342428225
##########
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:
Good catch — `filterTo` is typed as `FilterValue`, so `null`/`''` would slip
past the `!== undefined` guard and `Number()` coerces both to `0`, producing a
misleading `BETWEEN x AND 0`. Fixed in bade51a42a by explicitly rejecting
non-coercible (null/empty, non string/number) 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:
Agreed — the absence assertions alone would also pass if the whole compound
filter were dropped. Tightened in bade51a42a to
`expect(result.complexWhere).toBe('age > 5')`, directly asserting the sibling
condition survives unchanged.
##########
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:
Right — as written it would pass even if filter composition dropped
everything. Fixed in bade51a42a with `expect(result.complexWhere).toBe('age >
5')`, so the test now verifies the valid sibling condition is preserved.
--
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]
