fix: Fix SupersetClient.postForm calls when SUPERSET_APP_ROOT defined#38069
fix: Fix SupersetClient.postForm calls when SUPERSET_APP_ROOT defined#38069martyngigg wants to merge 4 commits intoapache:masterfrom
Conversation
* Include app_root in login next= parameter * Fix login with OAUTH/LDAP when app_root is defined * Add tests for Login links on Register/Login pages
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| hiddenForm.action = this.getUrl({ | ||
| endpoint: postFormConfig.endpoint, | ||
| url: postFormConfig.url, | ||
| }); |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
|
The security alert flags a potential XSS vulnerability in the postForm method of SupersetClientClass.ts, where payload values are assigned to form input values without escaping HTML meta-characters. If these values are displayed as HTML on the target page after form submission, it could allow script injection. The diff shows changes to how the payload is handled, but the full method isn't visible. Sanitize or escape payload values before setting them to prevent reinterpreting text as HTML. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts
Outdated
Show resolved
Hide resolved
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #38069 +/- ##
===========================================
+ Coverage 0 66.43% +66.43%
===========================================
Files 0 668 +668
Lines 0 51376 +51376
Branches 0 5791 +5791
===========================================
+ Hits 0 34132 +34132
- Misses 0 15855 +15855
- Partials 0 1389 +1389
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #539b61
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts - 1
- Incorrect postForm parameter · Line 699-699
Review Details
-
Files reviewed - 16 · Commit Range:
8970767..8970767- superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
- superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
- superset-frontend/packages/superset-ui-core/src/connection/types.ts
- superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts
- superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts
- superset-frontend/src/SqlLab/components/ExploreCtasResultsButton/ExploreCtasResultsButton.test.tsx
- superset-frontend/src/components/Chart/chartAction.ts
- superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx
- superset-frontend/src/explore/components/controls/ViewQueryModalFooter.tsx
- superset-frontend/src/explore/exploreUtils/index.ts
- superset-frontend/src/pages/Login/Login.test.tsx
- superset-frontend/src/pages/Login/index.tsx
- superset-frontend/src/pages/Register/Register.test.tsx
- superset-frontend/src/pages/Register/index.tsx
- superset/views/utils.py
- tests/integration_tests/test_subdirectory_deployments.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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
AI Code Review powered by
superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts
Outdated
Show resolved
Hide resolved
Code Review Agent Run #b02e7dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #24b841Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:
This will install the hooks in your local repository. From now on, a series of checks will automatically run whenever you make a Git commit. To run pre-commit manually run the following:
|
|
@EnxDev Thanks. I had pre-commit enabled but it seems to pass when the CI checks fail it, which I find confusing. The failures that currently show up don't show up for me either. I'm not sure why. |
Code Review Agent Run #493a19Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
I think the pre-commit job on master is broken as merging in master here as caused the oxlint failures to show up. |
|
@martyngigg I haven't been working on that branch in awhile, feel free to drop or merge what you need into your branch to move things forward. |
sadpandajoe
requested review from
Vitor-Avila,
betodealmeida,
Copilot and
eschutho
There was a problem hiding this comment.
Pull request overview
This PR addresses incorrect URL construction when Superset is deployed under a non-root SUPERSET_APP_ROOT by updating how form POSTs and login redirects determine their effective URL, preventing “double app_root” prefixes and improving subdirectory deployment behavior.
Changes:
- Update
SupersetClient.postFormto accept either anendpointor a fullurl(resolved viagetUrl) and migrate call sites/tests. - Fix
redirect_to_loginso thenextparameter includesrequest.script_rootfor subdirectory deployments. - Update Login/Register/Explore flows and corresponding tests to respect app root in links and form submissions.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_tests/test_subdirectory_deployments.py | Adds coverage to ensure redirect_to_login() includes app root and preserves query strings. |
| superset/views/utils.py | Prefixes next targets with request.script_root to support subdirectory deployments. |
| superset-frontend/src/pages/Register/index.tsx | Uses ensureAppRoot for login link and updates postForm call to new config signature. |
| superset-frontend/src/pages/Register/Register.test.tsx | Adds assertions for login link href with/without app root. |
| superset-frontend/src/pages/Login/index.tsx | Ensures provider/register links respect app root and updates postForm usage. |
| superset-frontend/src/pages/Login/Login.test.tsx | Adds coverage for app-rooted register/oauth URLs and updated postForm signature. |
| superset-frontend/src/explore/exploreUtils/index.ts | Uses SupersetClient.getUrl for v1 export URL and updates postForm usage. |
| superset-frontend/src/explore/exploreUtils/exportChart.test.ts | Updates mocks to use SupersetClient.getUrl instead of legacy app-root logic. |
| superset-frontend/src/explore/components/controls/ViewQueryModalFooter.tsx | Migrates postForm call to config object signature. |
| superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx | Migrates postForm call to config object signature. |
| superset-frontend/src/components/Chart/chartAction.ts | Migrates SQL Lab redirect postForm call to config object signature. |
| superset-frontend/src/SqlLab/components/ExploreCtasResultsButton/ExploreCtasResultsButton.test.tsx | Updates expectation for new postForm({ url, payload }) call shape. |
| superset-frontend/packages/superset-ui-core/test/connection/SupersetClientClass.test.ts | Updates postForm tests to use new config signature. |
| superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts | Adds getUrl exposure test and updates configure-flow expectations. |
| superset-frontend/packages/superset-ui-core/src/connection/types.ts | Introduces PostFormConfig types to support endpoint or url inputs. |
| superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts | Refactors postForm to accept config object and resolve URLs via getUrl. |
| superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts | Exposes getUrl on the exported SupersetClient singleton. |
Comments suppressed due to low confidence (1)
superset-frontend/packages/superset-ui-core/test/connection/SupersetClient.test.ts:55
- The test for “throws before configure” doesn’t include the newly exposed
SupersetClient.getUrlmethod. To keep this test aligned with the public API surface (and to prevent regressions), add an assertion thatSupersetClient.getUrlthrows when called beforeconfigure().
test('throws if you call init, get, post, postForm, isAuthenticated, and reAuthenticate before configure', () => {
expect(SupersetClient.init).toThrow();
expect(SupersetClient.get).toThrow();
expect(SupersetClient.post).toThrow();
expect(SupersetClient.postForm).toThrow();
expect(SupersetClient.isAuthenticated).toThrow();
expect(SupersetClient.reAuthenticate).toThrow();
expect(SupersetClient.request).toThrow();
| async postForm(postFormConfig: PostFormConfig) { | ||
| if (postFormConfig.endpoint || postFormConfig.url) { | ||
| await this.ensureAuth(); | ||
| const hiddenForm = document.createElement('form'); | ||
| hiddenForm.action = this.getUrl({ endpoint }); | ||
| hiddenForm.action = this.getUrl({ | ||
| endpoint: postFormConfig.endpoint, | ||
| url: postFormConfig.url, | ||
| }); | ||
| hiddenForm.method = 'POST'; | ||
| hiddenForm.target = target; | ||
| hiddenForm.target = postFormConfig.target ?? '_blank'; | ||
| const payloadWithToken: Record<string, any> = { |
There was a problem hiding this comment.
SupersetClientClass.postForm now only accepts a config object, which is a breaking change for any external consumers still calling postForm(endpoint, payload, target). At runtime, passing a string will also silently no-op because postFormConfig.endpoint/url will be undefined on a string. Consider supporting the legacy positional signature (with a deprecation path) or throwing a clear error when the argument is not an object containing endpoint or url.
| SupersetClient.postForm({ | ||
| url, | ||
| payload: { | ||
| form_data: safeStringify(payload), | ||
| }, |
There was a problem hiding this comment.
url is typed as string | null, but it’s passed directly to SupersetClient.postForm({ url, ... }). Even though runtime control flow ensures url should be non-null here, TypeScript won’t necessarily narrow it across branches and this can break builds (and also allows onStartStreamingExport to receive a nullable URL type). Consider adding a single non-null guard after the legacy/v1 branch (or refactoring so url is a plain string by this point).
SUMMARY
The SupersetClient.postForm API only accepted an endpoint and when called with a url caused a doubling up of the app_root when it was defined. We have changed the postForm signature to accept either a url or endpoint that is passed to getUrl. Call sites and tests have been updated.
This PR required brings in changes from:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Set
SUPERSET_APP_ROOT=/analyticsindocker/.env-localand boot up the docker-compose-light setup.ADDITIONAL INFORMATION