fix(app-root): prevent double-prefixed logout link under SUPERSET_APP_ROOT - #42848
Closed
TamayiM-WHO wants to merge 1 commit into
Closed
fix(app-root): prevent double-prefixed logout link under SUPERSET_APP_ROOT#42848TamayiM-WHO wants to merge 1 commit into
TamayiM-WHO wants to merge 1 commit into
Conversation
…_ROOT `navbarRight.user_logout_url` is produced by Flask-AppBuilder's `get_url_for_logout`, which resolves through Flask's `url_for` and is therefore already SCRIPT_NAME-aware. Wrapping it in `ensureAppRoot()` prefixed the application root a second time, so under a sub-path deployment the Logout menu item pointed at `/app-root/app-root/logout/` and returned 404. Unlike `user_info_url`, which `menu_data` emits as the relative literal `/user_info/` and therefore does need prefixing, the logout URL arrives already rooted. Dropping the wrapper leaves it single-prefixed. This cherry-picks the `RightMenu.tsx` hunk of apache#38033 onto the 6.2 release branch, together with the regression test added in apache#39925. `master` also carries an idempotence guard inside `ensureAppRoot()`; that is deliberately not backported here to keep the release-branch change minimal. Refs apache#40576 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Member
|
Closing this as this isn't how we would introduce changes into official release branches. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Backport of the
RightMenu.tsxhunk of #38033 to the6.2release branch, plus the regression test added in #39925.navbarRight.user_logout_urlcomes from Flask-AppBuilder'sget_url_for_logout, which resolves through Flask'surl_forand is therefore alreadySCRIPT_NAME-aware. On6.2it is still wrapped inensureAppRoot(), which prefixes the application root a second time, so the Logout menu item points outside the mount point and 404s under a sub-path deployment.This is the asymmetry that makes the bug easy to miss:
menu_dataemitsuser_info_urlas the relative literal/user_info/(so it does need prefixing), whileuser_logout_urlarrives already rooted. Both were being wrapped identically, so exactly one of them doubled.This was reported as #40576 and closed as fixed on
master, but the fix never reached a release branch. Evaluating the shippedensureAppRootbody on each ref against the value the backend actually emits (APPLICATION_ROOT=/app-superset):6.1.0 already shipped this, and
6.2would ship it again without this backport.masteradditionally carries an idempotence guard insideensureAppRoot()itself. That is deliberately not backported here — the one-lineRightMenu.tsxchange is sufficient and is the smaller risk on a release branch.Happy to open the equivalent PR against
6.1if maintainers want it there too.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (
APPLICATION_ROOT=/app-superset), Settings → Logout:After:
TESTING INSTRUCTIONS
Automated — this PR ports the regression test #39925 added on
master, where it passes. It asserts the Logouthrefstays single-prefixed when the backend supplies an already-rooted URL, so it fails against6.2's currentensureAppRoot()wrapping:Manual:
SUPERSET_APP_ROOT=/app-superset(orAPPLICATION_ROOT) behind a proxy that routes only/app-superset/*.href./app-superset/logout/, not/app-superset/app-superset/logout/, and clicking it should log you out rather than 404.ADDITIONAL INFORMATION
app_roottwice. #40576 (fixed onmasterby fix(superset_app_root): when used with oauth #38033; this backports it to6.2)