codeant-ai-for-open-source[bot] commented on code in PR #40699:
URL: https://github.com/apache/superset/pull/40699#discussion_r3345966337
##########
superset/security/manager.py:
##########
@@ -3500,6 +3502,41 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ def _guest_token_is_revoked(self, token: dict[str, Any]) -> bool:
+ """
+ Return True if any dashboard resource referenced by the token has been
+ revoked since the token was issued — i.e. the token's ``iat`` predates
+ that embedded dashboard's ``guest_token_revoked_before``.
+
+ Revocation is per embedded dashboard, so revoking one embed does not
+ affect guest tokens for other dashboards. Tokens with no ``iat`` (none
+ are issued without one) are treated as not revoked.
+ """
+ # pylint: disable=import-outside-toplevel
+ from datetime import timezone
+
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ iat = token.get("iat")
+ if iat is None:
+ return False
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
+ revoked_before = (
+ embedded.guest_token_revoked_before if embedded else None
+ )
+ if revoked_before is None:
+ continue
Review Comment:
**Suggestion:** Revocation lookup only checks
`EmbeddedDashboardDAO.find_by_id` using the token resource id as an embedded
UUID, but guest tokens are still allowed to carry legacy dashboard IDs. For
those tokens, lookup returns `None` and revocation is silently skipped, so
previously-issued dashboard-id tokens remain valid after revoke. Resolve
dashboard-id resources to their embedded config (or check both dashboard-id and
embedded-uuid paths) before deciding not revoked. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Per-dashboard revoke fails for dashboard-id guest tokens.
- ⚠️ Revoked embeds still accessible via legacy-id guest tokens.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Mint a guest token via the public API `POST
/api/v1/security/guest_token/` implemented
in `superset/security/api.py:144-163`. The request body is validated by
`GuestTokenCreateSchema` (`security/api.py:33-36`) and then passed to
`validate_guest_token_resources` (`security/api.py:42-45`).
2. In the request body, set `resources` to include a dashboard *ID* instead
of an embedded
UUID, for example: `{"type": "dashboard", "id": "<numeric_dashboard_id>"}`.
`SupersetSecurityManager.validate_guest_token_resources` at
`superset/security/manager.py:3428-3439` first calls
`Dashboard.get(str(resource["id"]))`;
since this id is a valid dashboard id, the check passes without requiring an
`EmbeddedDashboard` uuid.
3. Configure the same dashboard for embedding so it has an
`EmbeddedDashboard` row, and
then revoke its guest tokens via `POST
/api/v1/dashboard/<id_or_slug>/embedded/revoke`.
This hits `DashboardRestApi.revoke_embedded` in
`superset/dashboards/api.py:2144-2183`,
which calls `EmbeddedDashboardDAO.revoke_guest_tokens(dashboard)`
(`superset/daos/dashboard.py:23-32`), stamping
`embedded.guest_token_revoked_before` for
each embedded config.
4. Send the previously-minted token (whose resource id is the dashboard id,
not the
embedded uuid) in the guest token header to any embedded request, which is
processed by
`SupersetSecurityManager.get_guest_user_from_request`
(`superset/security/manager.py:21-54`). The method decodes the JWT and then
calls
`_guest_token_is_revoked` (`manager.py:56-89`), which looks up each
dashboard resource
only via `EmbeddedDashboardDAO.find_by_id(str(resource["id"]))`
(`manager.py:77`). For a
numeric dashboard id, this returns `None`, `revoked_before` is `None`, and
the loop
`continue`s (`manager.py:78-81`), so `_guest_token_is_revoked` returns
`False`. As a
result, the guest user is accepted even though
`EmbeddedDashboardDAO.revoke_guest_tokens`
marked the associated embedded configuration as revoked.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=df5cec8c328b422aa3410105fa6ee798&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=df5cec8c328b422aa3410105fa6ee798&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 3523:3531
**Comment:**
*Logic Error: Revocation lookup only checks
`EmbeddedDashboardDAO.find_by_id` using the token resource id as an embedded
UUID, but guest tokens are still allowed to carry legacy dashboard IDs. For
those tokens, lookup returns `None` and revocation is silently skipped, so
previously-issued dashboard-id tokens remain valid after revoke. Resolve
dashboard-id resources to their embedded config (or check both dashboard-id and
embedded-uuid paths) before deciding not revoked.
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.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40699&comment_hash=ffc30e2bdabdb4050c4c6caffb36b0d967e0539f8018c6731b8cc9bcc8e3ea0a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40699&comment_hash=ffc30e2bdabdb4050c4c6caffb36b0d967e0539f8018c6731b8cc9bcc8e3ea0a&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -3500,6 +3502,41 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ def _guest_token_is_revoked(self, token: dict[str, Any]) -> bool:
+ """
+ Return True if any dashboard resource referenced by the token has been
+ revoked since the token was issued — i.e. the token's ``iat`` predates
+ that embedded dashboard's ``guest_token_revoked_before``.
+
+ Revocation is per embedded dashboard, so revoking one embed does not
+ affect guest tokens for other dashboards. Tokens with no ``iat`` (none
+ are issued without one) are treated as not revoked.
+ """
+ # pylint: disable=import-outside-toplevel
+ from datetime import timezone
+
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ iat = token.get("iat")
+ if iat is None:
+ return False
Review Comment:
**Suggestion:** Tokens without an `iat` claim are treated as not revoked,
but `get_guest_user_from_request` does not require `iat`, so a validly-signed
token missing `iat` bypasses the new revocation control. Reject guest tokens
that lack `iat` during validation instead of returning `False` in revocation
checks. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Missing-iat guest tokens bypass per-dashboard revocation checks.
- ⚠️ Weakens guarantee that revoked embeds lose access.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a JWT manually using the same secret and algorithm as
configured in Superset
(`GUEST_TOKEN_JWT_SECRET` and `GUEST_TOKEN_JWT_ALGO`, read in
`SupersetSecurityManager.create_guest_access_token` at
`superset/security/manager.py:31-34` and `parse_jwt_guest_token` at
`manager.py:97-107`).
Omit the `iat` claim entirely, but include at least: `"user"`, `"resources"`,
`"rls_rules"`, `"aud"` matching `_get_guest_token_jwt_audience`
(`manager.py:1-6`),
`"type": "guest"`, and an `"exp"` if desired.
2. Ensure the token's `resources` reference an embedded dashboard that has
been configured
and later revoked via the per-dashboard revoke endpoint `POST
/api/v1/dashboard/<id_or_slug>/embedded/revoke`
(`superset/dashboards/api.py:2144-2183`),
which calls `EmbeddedDashboardDAO.revoke_guest_tokens` to set
`guest_token_revoked_before`
(`superset/daos/dashboard.py:23-32`).
3. Send this custom JWT as the guest token header in an HTTP request that
reaches
Superset. `SupersetSecurityManager.get_guest_user_from_request`
(`superset/security/manager.py:21-54`) reads the header, calls
`parse_jwt_guest_token`
(`manager.py:97-107`) to decode and validate the signature and audience, and
then performs
basic claim checks for `"user"`, `"resources"`, `"rls_rules"`, and `"type"`
(`manager.py:38-45). It does *not* require the presence of `"iat"`.
4. `get_guest_user_from_request` then calls `_guest_token_is_revoked`
(`manager.py:56-89`). Inside this method, `iat = token.get("iat")` is `None`
(`manager.py:71`), so the early check `if iat is None: return False`
(`manager.py:72-73`)
is hit, and revocation is not enforced. The function returns `False` (not
revoked), and
`get_guest_user_from_request` proceeds to return a `GuestUser`
(`manager.py:54, 91-95`),
allowing a signed guest token without `iat` to remain valid even after
revocation.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8ee4cf4cf5524d89997d98ef5ca8bb9e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8ee4cf4cf5524d89997d98ef5ca8bb9e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 3520:3522
**Comment:**
*Security: Tokens without an `iat` claim are treated as not revoked,
but `get_guest_user_from_request` does not require `iat`, so a validly-signed
token missing `iat` bypasses the new revocation control. Reject guest tokens
that lack `iat` during validation instead of returning `False` in revocation
checks.
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.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40699&comment_hash=c7ef7e251336302b73d62715e7caf59c87335e9eef0538eb83df7e33fa0ee2b9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40699&comment_hash=c7ef7e251336302b73d62715e7caf59c87335e9eef0538eb83df7e33fa0ee2b9&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
