Skip to content

fix(superset-ui-core): Include appRoot in endpoint of SupersetClientClass.postForm action - #34395

Merged
rusackas merged 1 commit into
apache:masterfrom
ISISNeutronMuon:fix-login-with-non-empty-approot
Aug 12, 2025
Merged

rusackas merged 1 commit into
apache:masterfrom
ISISNeutronMuon:fix-login-with-non-empty-approot

Conversation

@martyngigg

Copy link
Copy Markdown
Contributor

SUMMARY

SupersetClientClass.request uses SupersetClientClass.getUrl to construct the correct endpoint, including an optional appRoot if set.

SupersetClientClass.postForm used the url as passed to it and subsequently failed if appRoot is not empty.

With this change postForm also uses getUrl to ensure the appropriate endpoint is created.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

This can be verified using the docker compose development setup.

Set a non-empty root:

echo SUPERSET_APP_ROOT=/prefix >> docker/.env-local

Bring up the light docker compose config:

docker compose -f docker-compose-light.yml up -d

When the frontend has finished building go to http://localhost:9001/prefix and login with "admin:admin". Before these changes the successful login would redirect to http://localhost:9001/login rather than http://localhost:9001/prefix/login.

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

@dosubot dosubot Bot added the change:frontend Requires changing the frontend label Jul 30, 2025

@korbit-ai korbit-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Files scanned ​
File Path Reviewed
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts ✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

All other methods go via SupersetClientClass.request which
does this automatically.
await this.ensureAuth();
const hiddenForm = document.createElement('form');
hiddenForm.action = url;
hiddenForm.action = this.getUrl({ endpoint });

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML

[DOM text](1) is reinterpreted as HTML without escaping meta-characters.

@rusackas rusackas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM (and Claude and RooCode)

@rusackas
rusackas merged commit 34e10f5 into apache:master Aug 12, 2025
@martyngigg
martyngigg deleted the fix-login-with-non-empty-approot branch August 13, 2025 07:13
LisaHusband pushed a commit to LisaHusband/superset that referenced this pull request Aug 17, 2025
@dosubot dosubot Bot mentioned this pull request Aug 20, 2025
3 tasks done
@dosubot dosubot Bot mentioned this pull request Aug 28, 2025
3 tasks
@dosubot dosubot Bot mentioned this pull request Sep 5, 2025
3 tasks done
@github-actions github-actions Bot added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 6.0.0 First shipped in 6.0.0 labels Dec 18, 2025
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels change:frontend Requires changing the frontend packages size/M 🚢 6.0.0 First shipped in 6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants