michael-s-molina commented on PR #42864: URL: https://github.com/apache/superset/pull/42864#issuecomment-5217119335
Nice fix for the general case, but the two lookup tables aren't complete yet — I found several other real controls that map into `metrics`/`columns` via `buildQuery` but aren't listed, so those chart types will still hit the same false-403 whenever the chart has no cached `query_context`: **Missing from `STORED_METRIC_PARAMS`:** - `tooltip_metrics` (Gantt) - `timeseries_limit_metric_b` (MixedTimeseries, Query B) - `point_radius_fixed` (deck.gl Scatter/Polygon, when set to a metric) **Missing from `STORED_COLUMN_PARAMS`:** - `groupby_b` (MixedTimeseries, Query B) - `source_category` / `target_category` (Graph/network) - `dimension`, `line_column`, `js_columns` (deck.gl) - `geom_column` (Cartodiagram) **Structural gaps** that adding a name alone won't fix, since the value isn't a flat string/list: - deck.gl `spatial`/`start_spatial`/`end_spatial` — nested lat/lon/geohash column configs (decomposed today only via `getSpatialColumns()` on the frontend) - deck.gl `tooltip_contents` — list of tooltip-config objects wrapping column refs - Cartodiagram's `selected_chart` — a JSON-encoded embedded chart definition with its own `metrics`/`groupby` nested inside a string; the flat key scan never looks inside it These all fail closed (false 403 for a legit guest), not open, so no new security exposure — but the fix doesn't cover "every stored control" yet as claimed in the description. -- 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]
