rusackas commented on code in PR #40660:
URL: https://github.com/apache/superset/pull/40660#discussion_r3382290941
##########
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:
Good call — I went ahead and added this regression test rather than
deferring it. `test_dashboard_permalink_native_filters_encoded` in
`tests/integration_tests/core_tests.py` mocks a permalink state whose
`native_filters` urlParam contains reserved characters (`&`, `=`, `#`) and
asserts the redirect `Location` header percent-encodes the value
(`native_filters=value%26injected%3Devil%23frag`), that no injected
`injected=evil` param leaks in, and that the value round-trips back to the
original when decoded.
--
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]
