geido commented on code in PR #45273:
URL: https://github.com/apache/superset/pull/45273#discussion_r4288445592
##########
superset/charts/data/api.py:
##########
@@ -563,18 +564,11 @@ def _process_data(query_data: Any) -> Any:
# their guest token has been granted "can view query on
Dashboard",
# mirroring the permission the frontend uses to expose the
# "View query" action. Stacktraces and driver errors stay
redacted
- # regardless, as those leak details of the deployment itself.
- can_view_query = security_manager.can_access(
- "can_view_query", "Dashboard"
- )
- for query in queries:
- if not can_view_query:
- query.pop("query", None)
- query.pop("stacktrace", None)
- if query.get("error"):
- query["error"] = sanitize_error_message(
- query["error"], required=sanitize_required
- )
+ # regardless, as those leak details of the deployment itself.
The
+ # same helper redacts the ``/datasource/samples`` (Drill to
+ # detail) response so the two guest-facing endpoints cannot
+ # diverge.
+ redact_sensitive_query_fields(queries,
required=sanitize_required)
Review Comment:
Follow-up: once the guard was gone, nothing passed `required` into the
helper, so Codecov flagged `if required is None` as an unreachable branch.
c5bab97 dropped the parameter; the helper now [resolves the decision
itself](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/utils/error_sanitization.py#L194)
and [passes
`required=True`](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/utils/error_sanitization.py#L206)
into the error sanitization, so the [call
site](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/charts/data/api.py#L560)
stays a bare call. Behavior is unchanged: the existing chart-data tests pass
as-is, including
[`test_send_chart_response_does_not_half_redact_guest_query_error`](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/tests/unit_tests/charts/test_chart_data_api.py#L665)
(still a single `is_guest_user` look
up) and
[`test_send_chart_response_keeps_guest_query_when_permitted`](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/tests/unit_tests/charts/test_chart_data_api.py#L571).
##########
tests/unit_tests/views/datasource/views_test.py:
##########
@@ -888,6 +888,36 @@ def test_samples_proceeds_for_supported_datasource_type(
view.json_response.assert_called_once_with({"result": {"rows": []}})
+@patch("superset.views.datasource.views.redact_sensitive_query_fields")
Review Comment:
One more tightening since (a82a73d): the real-helper test now [captures what
`json_response`
serializes](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/tests/unit_tests/views/datasource/views_test.py#L939)
and asserts on that, instead of on the `rv` dict the helper mutates. Checked
in a scratch copy with the redaction moved after `json_response`: the previous
assertion still passed, the new one fails.
##########
superset/views/datasource/views.py:
##########
@@ -444,6 +445,12 @@ def samples(self) -> FlaskResponse:
datasource=dataset,
dashboard_id=dashboard_id,
)
+ # Drill to detail is available to embedded guests (see the guest branch
+ # above), and the samples payload carries the generated SQL under
+ # ``query``. Apply the same guest redaction ``/api/v1/chart/data`` has
+ # applied since CVE-2025-55673 so the SQL is not disclosed to a guest
+ # whose role lacks "can view query on Dashboard".
+ redact_sensitive_query_fields([rv])
Review Comment:
Adding the specifics: every failure exit in `get_samples` raises
`DatasetSamplesFailedError` instead of returning the payload
([count](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/views/datasource/utils.py#L185),
[samples](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/views/datasource/utils.py#L216)),
so a failed payload's `query`/`stacktrace` are never serialized. The view sits
under
[`@handle_api_exception`](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/views/datasource/views.py#L387),
whose [`SupersetException`
branch](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/views/error_handling.py#L154)
goes through `json_error_response`, which [swaps the message for
guests](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/superset/views/error_handling.py#L92).
That's the part that
matters, since a SQLAlchemy driver error can quote the full statement as
`[SQL: ...]`.
[`test_samples_failure_returns_generic_error_to_guests`](https://github.com/apache/superset/blob/3f5800786a70d041958c1e0fea10ad6b8fee92d4/tests/unit_tests/views/datasource/views_test.py#L1001)
feeds it such an error and gets a 422 with only the generic message, and an
end-to-end run with a real guest token and a failing samples query returned the
same `{"error": "An error occurred while fetching the data."}`. Those tests pin
existing behavior rather than change it; the guest failure path was already
safe.
--
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]
