fix: the guest token creation endpoint at superset/s... in api.py#40026
fix: the guest token creation endpoint at superset/s... in api.py#40026orbisai0security wants to merge 2 commits into
Conversation
Automated security fix generated by Orbis Security AI
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| for resource in body["resources"]: | ||
| if resource["type"] == GuestTokenResourceType.DASHBOARD.value: | ||
| from superset.models.embedded_dashboard import EmbeddedDashboard # noqa: PLC0415 | ||
| embedded = ( | ||
| db.session.query(EmbeddedDashboard) | ||
| .filter_by(uuid=resource["id"]) | ||
| .one_or_none() | ||
| ) | ||
| if embedded and not self.appbuilder.sm.can_access_dashboard( | ||
| embedded.dashboard | ||
| ): | ||
| raise ForbiddenError() |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The authorization check in the guest token endpoint only enforces can_access_dashboard when the resource ID resolves to an EmbeddedDashboard UUID; if a dashboard resource is specified by legacy dashboard ID, embedded is None and no access check is performed. Since validate_guest_token_resources still accepts dashboard IDs and has_guest_access still honors them, a user with can_grant_guest_token can mint guest tokens for dashboards they cannot access by using dashboard IDs.
Suggestion: For each dashboard resource, resolve a concrete Dashboard object using the same dual-path logic as validate_guest_token_resources (dashboard ID or embedded UUID), and always enforce can_access_dashboard on that resolved dashboard; add an integration test ensuring that a caller without access submitting a dashboard ID receives 403.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/security/api.py
**Line:** 175:186
**Comment:**
*CRITICAL: The authorization check in the guest token endpoint only enforces `can_access_dashboard` when the resource ID resolves to an `EmbeddedDashboard` UUID; if a dashboard resource is specified by legacy dashboard ID, `embedded` is `None` and no access check is performed. Since `validate_guest_token_resources` still accepts dashboard IDs and `has_guest_access` still honors them, a user with `can_grant_guest_token` can mint guest tokens for dashboards they cannot access by using dashboard IDs.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
@orbisai0security can you address the code review comments?
There was a problem hiding this comment.
Yes — the review comment is correct.
Why this is a real issue
The new check only validates access when the resource maps to an EmbeddedDashboard UUID.
If the request uses a legacy dashboard ID, embedded is None, so the code skips the authorization check entirely.
That means a user who can call guest_token could still mint a guest token for a dashboard they shouldn’t be able to access, which defeats the intent of the fix.
Minimal fix
Apply the access check to the resolved dashboard in both cases:
- legacy dashboard ID
- embedded dashboard UUID
For example, resolve the dashboard using the same logic already used by validate_guest_token_resources, then always call can_access_dashboard(...) on the resolved dashboard.
for resource in body["resources"]:
if resource["type"] == GuestTokenResourceType.DASHBOARD.value:
# resolve dashboard by legacy ID or embedded UUID
dashboard = ...
if not self.appbuilder.sm.can_access_dashboard(dashboard):
raise ForbiddenError()Recommendation
I’d keep the change localized in guest_token() and add an integration test that verifies:
- a caller without dashboard access gets
403 - this works for both dashboard ID and embedded UUID inputs
So the fix is necessary, and the current patch should be adjusted rather than merged as-is.
|
The flagged issue is correct: the authorization check only applies when the resource ID is an EmbeddedDashboard UUID, bypassing checks for legacy dashboard IDs. This allows users with can_grant_guest_token to mint tokens for inaccessible dashboards via IDs. To resolve, modify the loop to resolve the Dashboard object using dual-path logic: try as UUID for EmbeddedDashboard, else as ID for Dashboard, then always check can_access_dashboard. Add an integration test for 403 on unauthorized dashboard ID access. superset/security/api.py |
![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 #b2d947
Actionable Suggestions - 1
-
superset/security/api.py - 1
- N+1 Query in Access Validation · Line 174-186
Review Details
-
Files reviewed - 1 · Commit Range:
99c0f4c..99c0f4c- superset/security/api.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
| # Verify requesting user has access to each specified resource | ||
| for resource in body["resources"]: | ||
| if resource["type"] == GuestTokenResourceType.DASHBOARD.value: | ||
| from superset.models.embedded_dashboard import EmbeddedDashboard # noqa: PLC0415 | ||
| embedded = ( | ||
| db.session.query(EmbeddedDashboard) | ||
| .filter_by(uuid=resource["id"]) | ||
| .one_or_none() | ||
| ) | ||
| if embedded and not self.appbuilder.sm.can_access_dashboard( | ||
| embedded.dashboard | ||
| ): | ||
| raise ForbiddenError() |
There was a problem hiding this comment.
The added access validation loop performs a separate database query for each dashboard resource, potentially causing N+1 query performance issues when multiple dashboards are specified in guest token requests. Batching the queries would improve efficiency on plausible execution paths.
Code suggestion
Check the AI-generated fix before applying
| # Verify requesting user has access to each specified resource | |
| for resource in body["resources"]: | |
| if resource["type"] == GuestTokenResourceType.DASHBOARD.value: | |
| from superset.models.embedded_dashboard import EmbeddedDashboard # noqa: PLC0415 | |
| embedded = ( | |
| db.session.query(EmbeddedDashboard) | |
| .filter_by(uuid=resource["id"]) | |
| .one_or_none() | |
| ) | |
| if embedded and not self.appbuilder.sm.can_access_dashboard( | |
| embedded.dashboard | |
| ): | |
| raise ForbiddenError() | |
| # Verify requesting user has access to each specified resource | |
| dashboard_uuids = [resource["id"] for resource in body["resources"] if resource["type"] == GuestTokenResourceType.DASHBOARD.value] | |
| if dashboard_uuids: | |
| from superset.models.embedded_dashboard import EmbeddedDashboard # noqa: PLC0415 | |
| embedded_dashboards = db.session.query(EmbeddedDashboard).filter(EmbeddedDashboard.uuid.in_(dashboard_uuids)).all() | |
| for embedded in embedded_dashboards: | |
| if not self.appbuilder.sm.can_access_dashboard(embedded.dashboard): | |
| raise ForbiddenError() |
Code Review Run #b2d947
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
✅ Changes Applied I've updated the code based on your feedback: The fix addresses two code review concerns:
Files modified:
The changes have been pushed to this PR branch. Please review! |
Code Review Agent Run #48d5afActionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40026 +/- ##
==========================================
- Coverage 63.83% 63.83% -0.01%
==========================================
Files 2589 2589
Lines 137815 137831 +16
Branches 31921 31928 +7
==========================================
+ Hits 87973 87980 +7
- Misses 48325 48330 +5
- Partials 1517 1521 +4
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:
|
2 participants
Summary
Fix critical severity security issue in
superset/security/api.py.Vulnerability
V-004superset/security/api.py:172Description: The guest token creation endpoint at superset/security/api.py:172 loads the request body via guest_token_create_schema.load() but does not verify that the requesting user has explicit access to every resource (dashboard or dataset ID) specified in the payload. Any authenticated user with permission to call this endpoint can craft a request specifying restricted resource IDs they do not own, causing the server to issue a valid guest token granting access to those restricted resources.
Changes
superset/security/api.pyVerification
Automated security fix by OrbisAI Security