fix(auth): preserve login redirects across tabs

[aminghadersohi]

SUMMARY

When several tabs reach /login/?next=... after a session expires, completing login in one tab authenticates the shared browser session. Refreshing another tab then takes the login view's already-authenticated branch, which previously discarded next and sent the user to the index.

This adds login_redirect=1 while the request is known to be anonymous. An already-authenticated login request honors next only when that flag is present. Unmarked authenticated login requests still go to the index, preserving the existing protection against redirect loops when an authenticated user is sent to login after a permission denial.

The flag grants no authorization and is intentionally not a security token. Destinations still pass Flask-AppBuilder's safe-redirect validation; backslash-containing destinations are rejected so browser and server URL parsing cannot disagree.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; this changes server-side redirect behavior without changing the UI.

TESTING INSTRUCTIONS

Automated:

PYTHONPATH="$PWD/superset-core/src:$PWD" pytest -q tests/unit_tests/views/test_auth.py
uvx pre-commit run --files superset/views/auth.py tests/unit_tests/views/test_auth.py

The regression suite covers:

• flagging a safe next while anonymous;
• restoring the original dashboard after another tab authenticates;
• retaining the index fallback for unflagged or invalidly flagged authenticated requests;
• external, scheme-relative, script-scheme, control-character, triple-slash, and backslash redirect forms;
• repeated query parameters and SCRIPT_NAME/application-root deployments.

Manual:

1. Configure a short PERMANENT_SESSION_LIFETIME.
2. Log in and open multiple dashboard tabs.
3. Let the session expire and allow each tab to reach the login page.
4. Complete login in one tab.
5. Refresh the remaining tabs and verify each returns to its original dashboard.
6. As an authenticated user, request a page without permission and verify the login redirect does not loop back to the denied page.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.24%. Comparing base (30402b4) to head (1e5cc17).
⚠️ Report is 39 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #43606      +/-   ##
==========================================
+ Coverage   79.21%   79.24%   +0.02%     
==========================================
  Files        2879     2880       +1     
  Lines      166040   166322     +282     
  Branches    38405    38471      +66     
==========================================
+ Hits       131536   131808     +272     
- Misses      32025    32027       +2     
- Partials     2479     2487       +8     

Flag	Coverage Δ
hive	37.90% <69.56%> (-0.04%)	⬇️
mysql	57.57% <69.56%> (-0.10%)	⬇️
postgres	57.60% <69.56%> (-0.11%)	⬇️
presto	39.80% <69.56%> (-0.04%)	⬇️
python	83.78% <100.00%> (+0.03%)	⬆️
sqlite	57.29% <69.56%> (-0.10%)	⬇️
unit	74.21% <100.00%> (+0.24%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #6f75cb

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 7c737f0..7c737f0

  • superset/views/auth.py
  • tests/unit_tests/views/test_auth.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[Vitor-Avila]

hey @aminghadersohi, thanks for working on this! I'm just curious if we really need the marker? It isn't bound to a session or user, so an attacker can mint one with an unauthenticated curl /login/?next=/anything and hand the result to a logged-in user. But get_safe_redirect already keeps next same-host, so they could just send /anything directly anyway (there's nothing extra being opened up by an unmarked next). I understand we're solving a redirect loop, but signing feels like overkill for that when a plain ?login_redirect=1 would distinguish the two entry points just as well.

Also, the marker is minted when the login page loads, so other tabs that are stale for a longer time could be impacted.

[aminghadersohi]

Good point — I agree that signing did not provide meaningful provenance here because the anonymous endpoint would mint a token for any safe same-host destination, and the expiry worked against the stale-tab case. I simplified this to a plain login_redirect=1 entry-point flag, retained get_safe_redirect() and the backslash validation, and kept the index fallback for unflagged authenticated requests. I also updated the tests and PR description. Thanks for calling this out!

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	1e5cc17
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a96028898cabc0008977e97
😎 Deploy Preview	https://deploy-preview-43606--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bito-code-review]

Code Review Agent Run #212781

Actionable Suggestions - 0

Additional Suggestions - 2

• superset/views/auth.py - 1

  • CWE-601: Redirect Marker Forgeable · Line 61-61
    The signed marker (URLSafeTimedSerializer + compare_digest) was replaced with a plain constant, so `_marked_next_url()` now trusts any `login_redirect=1`. An attacker can forge `/login/?next=&login_redirect=1` and redirect an authenticated user to an arbitrary same-host destination — the exact forgery the signed marker previously blocked. External hosts are still blocked by FAB's `get_safe_redirect`, but the defense-in-depth is gone. ([CWE-601](https://cwe.mitre.org/data/definitions/601.html))
• tests/unit_tests/views/test_auth.py - 1

  • Lost security test coverage · Line 137-175
    Removing `test_authenticated_login_rejects_mismatched_next_url` drops the only coverage for the security path where a marker must not redirect to an unverified destination. With the plain-flag design, `_marked_next_url()` returns `_safe_next_url()` whenever the marker equals `LOGIN_REDIRECT_MARKER_VALUE`, so a valid marker + unsafe `next` must still fall back to the index. Add a test for that case.

Review Details

• Files reviewed - 2 · Commit Range: 7c737f0..1e5cc17

  • superset/views/auth.py
  • tests/unit_tests/views/test_auth.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[Vitor-Avila]

thanks, @aminghadersohi 🙏 I think this looks better.

Just one thought here: as far as I can tell there are 3 problems here and none of them are really in Superset:

1. FAB's login views drop next when you're already authenticated
2. FAB's has_access sends authenticated-but-unauthorized users to /login/?next=<denied url> (decorators.py#L176-L180). It treats "you need to log in" and "you're not allowed in" as the same thing.
3. is_safe_redirect_url accepts /\evil.com

If 2 was fixed upstream so authenticated users get a 403 instead of a login redirect, then /login/?next=X would only ever mean "not logged in", we could honor next unconditionally, and the flag + helpers would no longer be needed.

On 3, the SPA posts credentials to /login/?next=... (index.tsx#L99-L103), which FAB's AuthDBView.login handles with a bare get_safe_redirect, so next=/\evil.com still sends the user offsite right after they submit the form.

It might be better if we:

1. Open FAB PRs for 3 (clear-cut security fix) and 2 + 1 together (behavior change, needs a bit more discussion);
2. Then bump FAB here, and this whole thing collapses to roughly:

if g.user is not None and g.user.is_authenticated:
    return redirect(self._safe_next_url() or self.appbuilder.get_url_for_index)

What do you think? Curious if @dpgaspar has an thoughts here as well.

[aminghadersohi]

Thanks, Vitor — I confirmed the three behaviors upstream. I split the work as suggested:

• The authenticated has_access / login next behavior change is in draft FAB PR dpgaspar/Flask-AppBuilder#2479.
• I reported the backslash redirect issue privately to the FAB maintainers in accordance with FAB’s security policy; I’ll link the public fix once coordinated disclosure permits it.

Both are linked to Shortcut story 97504. I’m leaving this Superset PR open as a draft until the upstream fixes are accepted and there is a release we can bump to; at that point this can be replaced by the much smaller dependency-bump/Superset change you outlined.

[aminghadersohi]

Thanks again, @Vitor-Avila, for digging into this and identifying that the underlying problems belong in Flask-AppBuilder. Your suggested direction is cleaner than carrying a Superset-specific marker and redirect workaround.

I’ve split the work upstream:

• FAB behavior change: dpgaspar/Flask-AppBuilder#2479 makes authenticated authorization failures return 403 and lets already-authenticated login requests honor a safe next.
• The backslash redirect-validation issue has been reported privately to the FAB maintainers in accordance with their security policy.

I’m closing this PR without merging because those upstream fixes supersede its approach. Once the FAB changes are released, we can update the FAB dependency in Superset and add the minimal Superset-side change/regression coverage needed for the original multi-tab scenario.

Thank you for steering this toward the right layer 🙏