sadpandajoe commented on code in PR #39925:
URL: https://github.com/apache/superset/pull/39925#discussion_r3293333470
##########
superset/views/core.py:
##########
@@ -448,7 +423,18 @@
key: str | None = None,
) -> FlaskResponse:
if request.method == "GET":
- return redirect(Superset.get_redirect_url())
+ # Share the form_data → form_data_key cache-and-redirect contract
+ # with `ExploreView.root` via the lifted helper. Helper returns
+ # ``None`` for: empty/non-dict form_data, missing datasource,
+ # malformed datasource (AF-2), invalid `DatasourceType` (AF-2),
+ # cache-write failure (loop guard), and "already at target" loop
+ # guard. Falling through to `super().render_app_template()` kills
+ # the typed-entry `/explore/<dst>/<int:dsid>/` GET loop that the
+ # `isinstance(dict)` gate alone missed.
+ redirect_url = get_explore_redirect_url()
+ if redirect_url:
+ return redirect(redirect_url)
Review Comment:
Addressed in 3922c7fc1e: `get_explore_redirect_url` now builds the target
via `url_for("ExploreView.root", **query)` instead of the prior
`f"{url_for(...)}?{urlencode(...)}"` form. The endpoint is hard-coded — the
only user-controlled input is the query payload, which is round-tripped through
`parse_qs` and passed as kwargs to Flask's URL builder. CodeQL's data-flow
model recognises `url_for` as a sanitiser barrier, but the string-concat with
`urlencode` left the path unresolved as a tainted-string output.
Belt-and-suspenders: `parse_qs` lists are flattened to scalars before the
splat (`url_for` expects scalars, and the endpoint params here — `slice_id`,
`dataset_id`, `form_data_key`, … — are single-valued in practice), and
`loads_request_json` now coerces non-object JSON payloads to `{}` so a scalar
`form_data=42` cannot crash the downstream `.update()`. Resolving.
##########
superset/views/explore.py:
##########
@@ -33,11 +35,26 @@
@permission_name("read")
@event_logger.log_this
def root(self) -> FlaskResponse:
+ # After `Superset.route_base = ""`, both `Superset.explore` and this
+ # view register at `/explore/`; this view wins. Preserve the legacy
+ # form_data → form_data_key cache-and-redirect contract via the
+ # shared `get_explore_redirect_url` helper (lives in `views/utils.py`
+ # so both callers share the same guards: AF-2 malformed datasource,
+ # AF-3 non-numeric slice_id, cache-write failure, loop guard). The
+ # helper returns ``None`` for any case that would otherwise loop;
+ # we render the SPA in that branch.
+ if redirect_url := get_explore_redirect_url():
+ return redirect(redirect_url)
Review Comment:
Addressed in 3922c7fc1e: `get_explore_redirect_url` now builds the target
via `url_for("ExploreView.root", **query)` instead of the prior
`f"{url_for(...)}?{urlencode(...)}"` form. The endpoint is hard-coded — the
only user-controlled input is the query payload, which is round-tripped through
`parse_qs` and passed as kwargs to Flask's URL builder. CodeQL's data-flow
model recognises `url_for` as a sanitiser barrier, but the string-concat with
`urlencode` left the path unresolved as a tainted-string output.
Belt-and-suspenders: `parse_qs` lists are flattened to scalars before the
splat (`url_for` expects scalars, and the endpoint params here — `slice_id`,
`dataset_id`, `form_data_key`, … — are single-valued in practice), and
`loads_request_json` now coerces non-object JSON payloads to `{}` so a scalar
`form_data=42` cannot crash the downstream `.update()`. Resolving.
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:
##########
@@ -145,7 +146,7 @@
<span>
{t('Add an annotation layer')}{' '}
<a
- href="/annotationlayer/list"
+ href={ensureAppRoot('/annotationlayer/list')}
Review Comment:
Addressed in 3922c7fc1e: migrated this site from `<a
href={ensureAppRoot('/annotationlayer/list')}>` to `<AppLink
href="/annotationlayer/list">`. `AppLink` (in `src/utils/navigationUtils.ts`)
routes the href through `assertSafeNavigationUrl(ensureAppRoot(...))`, which is
the CodeQL-recognised sanitiser barrier — the prior through-function
`ensureAppRoot` call wasn't propagated as a sanitiser in CodeQL's default model.
The path literal here is static, so the alert was a false positive on the
data-flow shape rather than a real injection. Resolving.
--
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]
