fix(datasets): respect application root in database management link#36986
Conversation
The 'Manage your databases here' link in the AddDataset LeftPanel was hardcoded as /databaseview/list, which didn't respect the configured HTTP prefix in subdirectory deployments. This fix uses ensureAppRoot() to properly prefix the URL with the application root, ensuring the link works correctly in all deployment configurations.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #380d7cActionable 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 |
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| <span> | ||
| {t('Manage your databases')}{' '} | ||
| <Typography.Link href="/databaseview/list"> | ||
| <Typography.Link href={ensureAppRoot('/databaseview/list')}> |
There was a problem hiding this comment.
Suggestion: A malicious or misconfigured ensureAppRoot could return a non-relative or unsafe URL (for example a javascript: or external URL). Constrain the value to a safe internal path (e.g. ensure it starts with '/') and fallback if not, to prevent link injection/XSS-like issues. [security]
Severity Level: Critical 🚨
| <Typography.Link href={ensureAppRoot('/databaseview/list')}> | |
| <Typography.Link href={((): string => { const p = ensureAppRoot('/databaseview/list'); return p && p.startsWith('/') ? encodeURI(p) : '/databaseview/list'; })()}> |
Why it matters? ⭐
Valid security concern: untrusted or misconfigured ensureAppRoot could return an external or unsafe URL (javascript: scheme etc). The proposed guard (verify it starts with '/' and fallback) prevents link injection and is a reasonable defensive measure. It's a legitimate fix when the origin of ensureAppRoot's value isn't strictly controlled.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx
**Line:** 195:195
**Comment:**
*Security: A malicious or misconfigured `ensureAppRoot` could return a non-relative or unsafe URL (for example a javascript: or external URL). Constrain the value to a safe internal path (e.g. ensure it starts with '/') and fallback if not, to prevent link injection/XSS-like issues.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
I have checked, and a lot of other existing links in the project use the very same pattern as above, not the much more complicated (but more secure) version you suggested. Can you check what the convention in the existing code base is?
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@martyngigg any thoughts on this one? |
sadpandajoe
added
the
v6.0
4 participants
SUMMARY
The 'Manage your databases here' link in the AddDataset LeftPanel was hardcoded as
/databaseview/list, which didn't respect the configured HTTP prefix in subdirectory deployments (e.g., whenAPPLICATION_ROOT
is set to a path like/superset`).This fix uses
ensureAppRoot()to properly prefix the URL with the application root, ensuring the link works correctly in all deployment configurations.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Link behavior fix only
TESTING INSTRUCTIONS
APPLICATION_ROOT = "/superset"in your config/superset/databaseview/listinstead of just/databaseview/listAPPLICATION_ROOTconfigured)ADDITIONAL INFORMATION