[I] Malformed post_processing options return 500 instead of 400 for ValueError, KeyError and AttributeError [superset]

["SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>]

SEPURI-SAI-KRISHNA opened a new issue, #44952:
URL: https://github.com/apache/superset/issues/44952

   ### Bug description
   
   A malformed `post_processing` option returns **HTTP 500** instead of 400. Fuzzing the options of every operation found 39 distinct sites that reach `app.errorhandler(Exception)`; 38 of them are bad requests that should be 400, and 1 (a missing scipy) is correctly a 500.
   
   The intended contract is already pinned in the test suite — `tests/integration_tests/charts/data/api_tests.py::test_chart_data_invalid_post_processing` asserts `status_code == 400` for a bad `pivot` option. #44463 (merged 2026-10-02, `models/helpers.py`) and #44502 (merged 2026-09-21, `semantic_layers/models.py`) widened the guard at both `exec_post_processing` call sites so that raw pandas `TypeError` and `pandas.errors.DataError` become `QueryObjectValidationError`:
   
   ```python
   # superset/models/helpers.py:2684 and superset/semantic_layers/models.py:377
   except InvalidPostProcessingError as ex:
       raise QueryObjectValidationError(ex.message) from ex
   except (TypeError, pd.errors.DataError) as ex:
       raise QueryObjectValidationError(str(ex)) from ex
   ```
   
   **`ValueError`, `KeyError` and `AttributeError` escape the same way.** They are not caught here; `QueryContextProcessor.get_df_payload` catches only `QueryObjectValidationError`; `ChartDataRestApi.data` has no `@safe` decorator; so they reach `app.errorhandler(Exception)`, whose `json_error_response` defaults to `status=500`.
   
   ### Why these reach pandas at all
   
   `ChartDataPostProcessingOperationSchema.options` is a bare `fields.Dict` (`superset/charts/schemas.py:1123`) — no nested schema, so **no option value is validated before it reaches pandas**. The 11 `ChartData*OptionsSchema` classes (schemas.py:588-1054) are referenced only from `CHART_SCHEMAS`, which is OpenAPI output; they validate nothing. Eight operations have no options schema at all: `compare`, `cum`, `diff`, `flatten`, `histogram`, `rank`, `rename`, `resample`.
   
   ### Measured
   
   I fuzzed each operation's options one at a time away from a known-good baseline (910 calls over 19 operations), classifying by the oracle above. **39 distinct unhandled-exception sites**, spread over 14 of the 19 operations. The sweep mutates one option at a time and is not exhaustive over every possible value, so this is a lower bound rather than a complete census:
   
   | exception | distinct sites |
   |---|---|
   | `ValueError` | 21 |
   | `KeyError` | 10 |
   | `AttributeError` | 7 |
   | `ImportError` | 1 |
   
   `rolling` 9, `pivot` 5, `histogram` 5, `rank` 3, `diff` 3, `aggregate` 3, `sort` 2, `geohash_encode` 2, `boxplot` 2, and one each for `resample`, `geohash_decode`, `geodetic_parse`, `flatten`, `contribution`.
   
   Representative cases, each verified to pass `ChartDataQueryObjectSchema.load()` first and then raise:
   
   | `post_processing` options | raises | should be |
   |---|---|---|
   | `boxplot` `percentiles: [10, 200]` | `ValueError: Percentiles must be in the range [0, 100]` | 400 |
   | `rolling` `window: -1` | `ValueError: min_periods 0 must be <= window -1` | 400 |
   | `rolling` `center: "yes"` | `ValueError: center must be a boolean` | 400 |
   | `diff` `periods: 1.5` | `ValueError: periods must be an integer` | 400 |
   | `diff` `axis: "nope"` | `ValueError: No axis named nope` | 400 |
   | `sort` `ascending: null` | `ValueError: expected type bool` | 400 |
   | `pivot` `marginal_distributions: 1` | `ValueError: margins_name argument must be a string` | 400 |
   | `rank` `metric: "nope"` | `KeyError: 'nope'` | 400 |
   | `boxplot` `metrics: ["nope"]` | `KeyError: 'nope'` | 400 |
   | `aggregate` `aggregates: "x"` | `AttributeError: 'str' object has no attribute 'items'` | 400 |
   | `pivot` `aggregates: {"y": 0}` | `AttributeError: 'int' object has no attribute 'get'` | 400 |
   
   Note the `KeyError` family: a post-processing option naming a column that does not exist is a 500. The `invalid_columns` check in `get_df_payload` validates `query_obj.columns` and `metrics`, but not column names referenced from post-processing options.
   
   ### How to reproduce the bug
   
   1. `POST /api/v1/chart/data` with a query whose `post_processing` is
      `[{"operation": "rank", "options": {"metric": "does_not_exist"}}]`.
   2. The response is 500, not the 400 that `test_chart_data_invalid_post_processing` establishes as the contract.
   
   ### Suggested fix
   
   Handle `ValueError`, `KeyError` and `AttributeError` at both call sites, following the pattern #44463 established. That converts 38 of the 39 sites the fuzz found.
   
   `KeyError` wants its own arm rather than the shared `str(ex)` message: `str(KeyError("x"))` is `"'x'"`, so it would answer with `Error: 'does_not_exist'`. All ten `KeyError` sites name a column or MultiIndex level, so a specific message is accurate for each.
   
   Because `ValueError` and `AttributeError` are broad enough to also cover a genuine fault inside an operation, and `get_df_payload` records the message without logging a traceback, the handler should log the exception before re-raising so operators keep the stack trace.
   
   `ImportError` should be left out: the one case is `rolling` with a `win_type`, which needs scipy. A missing optional dependency is a deployment matter, not a bad request, so 500 is the right answer there.
   
   ### Not a security issue
   
   This is a robustness and API-contract bug, not a boundary violation under `SECURITY.md`: it requires a principal already entitled to run chart-data queries, grants no capability the role matrix withholds, and exposes no data. Filing it as a bug for that reason.
   
   ### Screenshots/recordings
   
   _No response_
   
   ### Superset version
   
   master / latest-dev
   
   ### Python version
   
   3.11
   
   ### Node version
   
   Not applicable
   
   ### Browser
   
   Not applicable
   
   ### Additional context
   
   The guard is duplicated verbatim in `superset/models/helpers.py` and `superset/semantic_layers/models.py`. That duplication is why the two were fixed eleven days apart (#44502 on 09-21, #44463 on 10-02); #44463's own description flags the second site as a follow-up. Consolidating it would prevent the next drift, but that is a separate change.
   
   ### Checklist
   
   - [x] I have searched Superset docs and Slack and didn't find a solution to my problem.
   - [x] I have searched the GitHub issue tracker and didn't find a similar bug report.
   - [x] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.
   


-- 
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: notifications-unsubscribe@superset.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [I] Malformed post_processing options return 500 instead of 400 for ValueError, KeyError and AttributeError [superset]

["SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>]

SEPURI-SAI-KRISHNA commented on issue #44952:
URL: https://github.com/apache/superset/issues/44952#issuecomment-5971977464

   Follow-up on the root cause, since #44953 only changes the status code and leaves the bad option reaching pandas.
   
   `options` stays an untyped `fields.Dict`. Of the 19 operations, 8 have no options schema at all (`compare`, `cum`, `diff`, `flatten`, `histogram`, `rank`, `rename`, `resample`), and the 11 that do exist are only listed in `CHART_SCHEMAS` for the OpenAPI spec, so they validate nothing.
   
   There is already a TODO for this at `superset/charts/schemas.py:2137`:
   
   > These should optimally be included in the QueryContext schema as an `anyOf` in ChartDataPostProcessingOperation.options, but since `anyOf` is not [supported] by Marshmallow<3, this is not currently possible.
   
   That blocker looks gone. The pin is `marshmallow==4.3.1`, `marshmallow-union` is already a dependency, and `Union` is already imported in the same file and used for `ChartDataDatasourceSchema.id`.
   
   Two ways to wire it:
   
   1. An untagged `Union` of the options schemas, which is what the TODO's `anyOf` describes.
   2. Dispatch on `operation` in a `@validates_schema` on the parent, so a bad option names the operation it belongs to. Better messages, which is the point of doing this at all.
   
   I would rather ask before writing it, because it is a breaking change: payloads that are malformed but tolerated today would start getting a 400. The part that needs a maintainer call is which of these you want:
   
   - reject outright
   - log a deprecation warning for one release, then enforce
   - enforce behind a feature flag
   
   Happy to implement whichever, including the 8 missing schemas and per-operation tests. I can open a separate issue for it if there is appetite.
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org