mikebridge commented on code in PR #42534:
URL: https://github.com/apache/superset/pull/42534#discussion_r3898121372
##########
superset/views/datasource/views.py:
##########
@@ -258,6 +242,20 @@ def samples(self) -> FlaskResponse:
else:
dataset = None
+ # Refuse datasource types that don't model raw rows only after access
+ # validation. Running this gate first would disclose datasource
+ # capabilities to guest requests that should receive a 403 or 404.
+ # ``supports_samples`` defaults to True for datasource classes that
+ # don't explicitly opt out.
+ ds_class: type[DatasourceUnion] | None = DatasourceDAO.sources.get(
+ DatasourceType(params["datasource_type"]),
+ )
+ if ds_class is not None and not getattr(ds_class, "supports_samples",
True):
+ return json_error_response(
+ _("Samples are not available for this datasource type."),
+ status=400,
+ )
Review Comment:
Addressed at the current head (07eadf5fcc, originally in the 08-06
remediation). The ordering concern is closed for the case that matters: a guest
requesting any non-dataset datasource type now gets 404 before the DatasetDAO
lookup and before the capability gate — two tests pin 404-beats-400 for a
semantic view (with the dataset lookup asserted not to run) and the
drill-access 403. For authenticated requests, dataset types get per-object
`raise_for_access` in `_load_dataset_for_samples` before the gate; for
non-table types the 400 is a function of the request's `datasource_type` alone
— it references no object, so it discloses nothing about any semantic view the
caller can't access, and the gate's comment now states that contract explicitly.
_Reply generated by Claude (AI) on behalf of @mikebridge._
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:
##########
@@ -372,7 +372,11 @@ export default function DrillDetailPane({
type="error"
showIcon
message={t('Failed to load drill-to-detail rows')}
- description={responseError}
+ description={
+ <pre style={{ margin: 0, whiteSpace: 'pre-wrap' }}>
+ {responseError}
+ </pre>
+ }
Review Comment:
This PR doesn't change what is displayed — only how. On master this pane
already renders `responseError` verbatim in a bare `<pre>`; the change here
wraps the same text in an `Alert` with a stable headline (and the shared
`PreformattedErrorDescription`). The error text is the body of an API response
the client has already received, so the UI adds no exposure beyond what the
endpoint returns; making the samples endpoint's error messages less verbose is
server-side hygiene that belongs to a separate change, not to this restyle.
Filing that as its own follow-up is reasonable if the concern stands.
_Reply generated by Claude (AI) on behalf of @mikebridge._
--
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]
