fix(lang): patch FAB's LocaleView to redirect to previous page#31692
fix(lang): patch FAB's LocaleView to redirect to previous page#31692mistercrunch merged 10 commits intoapache:masterfrom
Conversation
to redirect to the request referrer instead of relying on the session page_history. This change is needed because most of Superset's page views are handled by the frontend, and as such, not stored in the session page_history.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Non-descriptive locale error message ▹ view | |
|
Unvalidated redirect from Referer header ▹ view | ✅ |
 |
Insecure Open Redirect ▹ view | ✅ |
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
pomegranited
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Installing the pre-commit hook should resolve at least some of the CI issues. / Alternatively it is possible to run pre-commit by running pre-commit manually: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #31692 +/- ##
===========================================
+ Coverage 0 83.26% +83.26%
===========================================
Files 0 552 +552
Lines 0 39892 +39892
===========================================
+ Hits 0 33218 +33218
- Misses 0 6674 +6674
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Because we need to be able to change the locale even when logged out, this view cannot be protected.
|
Hi @rusackas :) Is there anything more I can do here to get this PR approved? |
|
I was hoping someone more FABtastic than me would come along to review this. If not @dpgaspar, perhaps @mistercrunch can take a look. |
|
Hi @jpchev , would you like to review this PR for me? Might help get it merged if there's some community testing/support :) |
yes, I've tested the changes and can confirm I am not redirected to another page after switching language |
dpgaspar
left a comment
dpgaspar
left a comment
There was a problem hiding this comment.
Looks good, would be great if you could add a couple of tests
|
hi @dpgaspar , is there anything else you need from me before this can be merged? |
|
@mistercrunch Can you help me get this PR merged? |
superset/initialization/__init__.py
Outdated
| return redirect("/superset/welcome/") | ||
|
|
||
| @staticmethod | ||
| def is_safe_url(target: str) -> bool: |
There was a problem hiding this comment.
might belong in superset/utils/urls.py would be surprised if we don't already have a similar method there, grepping for urlparse or next= could point to a similar method.
There was a problem hiding this comment.
Thank you @mistercrunch ! I didn't find a similar method anywhere, so 4bb4916 creates superset.utils.urls.is_safe_redirect_url(source_url, target_url).
Does this look ok now?
superset/utils/urls.py
Outdated
| return parsed_url.scheme == "https" | ||
|
|
||
|
|
||
| def is_safe_redirect_url(source_url: str, target_url: str) -> bool: |
There was a problem hiding this comment.
sorry for the multi-phase review, but GPT recommends further checking here, and since this is security for XSS I'm thinking let's do it...
def is_safe_redirect_url(source_url: str, target_url: str) -> bool:
if not target_url:
return False
joined = urljoin(source_url, target_url) # resolves relative URLs
parsed_source = urlparse(source_url)
parsed_target = urlparse(joined)
return (
parsed_source.scheme == parsed_target.scheme and
parsed_source.hostname == parsed_target.hostname
)
GPT says: This handles edge cases like user-supplied target_url values starting with // (which browsers interpret as external redirects) or other relative path tricks. Using urljoin() ensures we're validating the fully resolved URL against the expected scheme and host. Safer for open internet exposure.
There was a problem hiding this comment.
Easy done, thank you for this security fix @mistercrunch ! Applied with 08010c9
However, should we to use the joined target_url when doing the actual redirect?
There was a problem hiding this comment.
Ok, I've found what we needed in FAB: get_safe_redirect. My only worry is it's not exposed as an explicit part of the API, and so might change out from under us. But it's used in several places in the FAB code base, so it's a calculated risk.
|
@pomegranited just approved CI and it looks like this is mergeable pending feedback/action on the last comment from @mistercrunch. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
I was getting itchy over the merge button, so I'll block this awaiting feedback/implementation of the security-based code suggestion provided by @mistercrunch. CC @dpgaspar / @Antonio-RiveroMartnez for any other additional feedback or suggestions on that approach.
|
We just need to be super careful around the Couldn't find the pattern in the codebase after a quick search, maybe FAB exposes something we piggy-back on? @dpgaspar |
|
Still would love for @dpgaspar to chime in, maybe FAB has builtin functions for that already |
|
Hi @rusackas I found a FAB method to do what we need, so I think this resolves @mistercrunch 's concern? |
Nice! yes |
dpgaspar
left a comment
dpgaspar
left a comment
There was a problem hiding this comment.
Nice, not a perfect solution but an improvement
|
Hi @rusackas , I think we're good to merge now? |
|
Hi @rusackas , I think I've addressed everything here, and we got an approval from our FAB expert.. can this be merged? |
mistercrunch
dismissed
rusackas’s stale review
Requested changes have been addressed
|
hitting a weird GH bug I've never seen before -> Will try again a little later... |
|
Yay!! Thank you @mistercrunch @rusackas @dpgaspar for your help here! |
michael-s-molina
added
the
v5.0
mistercrunch
added
🍒 5.0.0
6 participants
SUMMARY
Overrides FAB's LocaleView to redirect to the request's referrer instead of relying on the session page history as suggested here.
This isn't a perfect solution, but it keeps the change very close to the actual issue.
Note that this view is marked out in the tests as an "unsecured" view, because we need to be able to change the current locale even when logged out (so that the login page can be presented in the user's preferred language).
Rejected alternatives
update_redirect()whenever a GET view is rendered (like this).This is more in line with how Flask AppBuilder works (and was recommended here).
However, this only works for pages rendered by Superset's backend, and most Superset pages are rendered by the frontend.
LOG_ACTIONS_SPA_NAVIGATIONevent is logged to the backend.This approach felt even hackier than the one posted here, and farther from the issue it addresses.
Also, Flask AppBuilder only provides the
update_redirect()method which uses the currentrequest.url, so we'd need to add another method that could use a differenturl.Fixes: #12768
Part of: #30334
Fixes: openedx/openedx-aspects#299
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Untitled.video.-.Made.with.Clipchamp.12.mp4
After:
Language.redirect.-.Made.with.Clipchamp.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION