Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lang): patch FAB's LocaleView to redirect to previous page #31692

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(lang): patch FAB's LocaleView to redirect to previous page #31692

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75a64d0
fix: patch FAB's LocaleView
pomegranited Jan 3, 2025
dffbe1f
fix: ensure the referrer is safe before redirecting
pomegranited Jan 3, 2025
3e752db
fix: types and ruff format linting
pomegranited Jan 7, 2025
25814f9
test: adds patch_flask_locale to list of allowed unsecured views
pomegranited Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions superset/initialization/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@

import wtforms_json
from deprecation import deprecated
from flask import Flask, redirect
from flask import abort, Flask, redirect, request, session
from flask_appbuilder import expose, IndexView
from flask_babel import gettext as __
from flask_babel import gettext as __, refresh
from flask_compress import Compress
from flask_session import Session
from werkzeug.middleware.proxy_fix import ProxyFix
Expand Down Expand Up @@ -701,3 +701,21 @@ class SupersetIndexView(IndexView):
@expose("/")
def index(self) -> FlaskResponse:
return redirect("/superset/welcome/")

@expose("/lang/<string:locale>")
def patch_flask_locale(self, locale):
"""
Change user's locale and redirect back to the previous page.

Overrides FAB's babel.views.LocaleView so we can use the request
Referrer as the redirect target, in case our previous page was actually
served by the frontend (and thus not added to the session's page_history
stack).
"""
if locale not in self.appbuilder.bm.languages:
abort(404, description="Locale not supported.")
Comment on lines +727 to +728
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-descriptive locale error message category Error Handling

Tell me more
What is the issue?

The error message when aborting with 404 is too generic and doesn't provide context about which locale was attempted.

Why this matters

When debugging issues with locale changes, administrators won't know which specific locale value caused the failure without checking logs or request parameters.

Suggested change

abort(404, description=f"Locale '{locale}' is not supported. Supported locales: {list(self.appbuilder.bm.languages)}")

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this one up to the reviewers - see original LocaleView

session["locale"] = locale
refresh()
self.update_redirect()
redirect_to = request.headers.get("Referer") or self.get_redirect()
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
return redirect(redirect_to)

Check warning

Code scanning / CodeQL

URL redirection from remote source Medium

Untrusted URL redirection depends on a
user-provided value
Loading
.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this one up to the reviewers - see original LocaleView