Copilot commented on code in PR #40660:
URL: https://github.com/apache/superset/pull/40660#discussion_r3338055577
##########
superset/views/core.py:
##########
@@ -864,12 +864,11 @@ def dashboard_permalink(
)
if url_params := state.get("urlParams"):
for param_key, param_val in url_params:
- if param_key == "native_filters":
- # native_filters doesnt need to be encoded here
- url = f"{url}&native_filters={param_val}"
- else:
- params = parse.urlencode([(param_key, param_val)])
- url = f"{url}&{params}"
+ # URL-encode every param value (including native_filters) so a
+ # value containing '&'/'#'/'=' cannot inject extra parameters
+ # into the redirect target. Flask decodes the value back on
read.
+ params = parse.urlencode([(param_key, param_val)])
+ url = f"{url}&{params}"
Review Comment:
The updated redirect logic is security-sensitive, but there isn’t a
regression test covering a stored `state.urlParams` entry for `native_filters`
that contains reserved characters (e.g. `&`, `#`, `=`) to ensure the `Location`
header percent-encodes the value and doesn’t allow query injection. There is
already an integration test for `dashboard_permalink` redirects in
`tests/integration_tests/core_tests.py`; extending it to include a mocked
permalink state with a `native_filters` value containing reserved characters
would help prevent future regressions.
--
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]
