codeant-ai-for-open-source[bot] commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3384772256
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before",
None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
Review Comment:
**Suggestion:** Revocation can be bypassed for valid guest tokens that still
use dashboard numeric IDs in `resources`: this check always treats
`resource.id` as an embedded UUID and never resolves dashboard IDs to their
embedded config before reading the revocation cutoff. Since resource validation
and access checks still accept dashboard IDs, revoked dashboards can continue
to be accessed with those tokens. Resolve dashboard IDs to their related
embedded row (or check both forms) before comparing against the revocation
timestamp. [security]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Revoked embedded dashboards stay accessible via dashboard-ID tokens.
- โ Revocation logic inconsistent with existing access checks behavior.
- โ ๏ธ Legacy clients bypass new revocation without any error.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Issue a guest token via the REST API `POST /api/v1/security/guest_token/`
implemented
in `superset/security/api.py:149-207`, passing a payload where `resources =
[{"type":
"dashboard", "id": "<dashboard_id>"}]` and `<dashboard_id>` is the numeric
dashboard ID
(or slug) rather than the embedded UUID. `ResourceSchema` at
`superset/security/api.py:62-65` accepts `id` as a string, and
`validate_guest_token_resources` at `superset/security/manager.py:3607-24`
resolves this
ID using `Dashboard.get(str(resource["id"]))`, so the token is accepted and
created with a
dashboard-based resource identifier.
2. Configure embedding for that dashboard using
`EmbeddedDashboardDAO.upsert` as in
`superset/daos/dashboard.py:610-21` and the embedded dashboard API at
`superset/dashboards/api.py:2103-2107`, then obtain the embedded UUID for
use with
revocation. Call
`SupersetSecurityManager.revoke_guest_token_access(embedded_uuid)` at
`superset/security/manager.py:68-83`, which looks up the `EmbeddedDashboard`
row by UUID
via `EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))` and sets
`embedded.guest_token_revoked_before`.
3. Later, send an embedded HTTP request carrying the previously issued guest
token in the
header or form field that `get_guest_user_from_request` reads
(`superset/security/manager.py:3652-66`). The Flask-Login request loader at
`superset/security/manager.py:660-665` calls `get_guest_user_from_request`,
which
successfully decodes the token and then invokes
`_is_guest_token_revoked(token)` at
`superset/security/manager.py:48-66`.
4. Inside `_is_guest_token_revoked`, the code at
`superset/security/manager.py:59-65`
iterates over resources and, for dashboard resources, calls
`EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))`. Because
`resource["id"]` is
the dashboard ID (not the embedded UUID), `EmbeddedDashboardDAO.find_by_id`
(configured
with `id_column_name = "uuid"` at `superset/daos/dashboard.py:605-608`)
returns `None`, so
`revoked_before = getattr(embedded, "guest_token_revoked_before", None)`
yields `None`.
The comparison `issued_at < revoked_before` at line 64 is never true,
`_is_guest_token_revoked` returns `False`, and `get_guest_user_from_request`
returns a
`GuestUser` even though revocation was configured. Downstream,
`has_guest_access` at
`superset/security/manager.py:3764-21` still grants access because it
explicitly supports
dashboard-ID tokens by checking `str(resource["id"]) == str(dashboard.id)`
before checking
the embedded UUID, so the revoked embedded dashboard remains accessible with
legacy
dashboard-ID tokens.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=04f76c0c183c4571922eb8354406ac7b&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=04f76c0c183c4571922eb8354406ac7b&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:** 3698:3704
**Comment:**
*Security: Revocation can be bypassed for valid guest tokens that still
use dashboard numeric IDs in `resources`: this check always treats
`resource.id` as an embedded UUID and never resolves dashboard IDs to their
embedded config before reading the revocation cutoff. Since resource validation
and access checks still accept dashboard IDs, revoked dashboards can continue
to be accessed with those tokens. Resolve dashboard IDs to their related
embedded row (or check both forms) before comparing against the revocation
timestamp.
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%2F40676&comment_hash=f929231633a68e847b1a4035f26948e3a33f2bf69ca3e100fffbabc61707d092&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=f929231633a68e847b1a4035f26948e3a33f2bf69ca3e100fffbabc61707d092&reaction=dislike'>๐</a>
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
Review Comment:
**Suggestion:** Returning `False` when `iat` is missing means tokens without
an issued-at claim are always treated as not revoked, even when a dashboard has
an active revocation cutoff. That weakens the revocation control and allows
bypass for any token lacking `iat`. Reject missing `iat` (or treat it as
revoked once a cutoff exists) to enforce revocation consistently. [security]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Guest tokens without iat ignore configured revocation cutoffs.
- โ Administrators cannot revoke such externally minted tokens.
- โ ๏ธ Behavior conflicts with stated revocation security objectives.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Construct a JWT for embedded access using the configured secret and
algorithm (used by
`SupersetSecurityManager.parse_jwt_guest_token` at
`superset/security/manager.py:91-101`).
The payload includes: `"type": "guest"`, a valid `"user"` object, a
`"resources"` list
with at least one dashboard resource (e.g. `{"type": "dashboard", "id":
"<embedded_uuid_or_dashboard_id>"}`), `"rls_rules": []`, and a valid `"exp"`
claim, but
intentionally omits the `"iat"` claim.
2. Send this JWT to Superset as part of an embedded request, in the header
or form field
read by `SupersetSecurityManager.get_guest_user_from_request`
(`superset/security/manager.py:62-64`). `parse_jwt_guest_token` decodes the
token without
requiring `"iat"` (it only enforces standard JWT validity and audience), and
`get_guest_user_from_request` at `superset/security/manager.py:69-37`
verifies the
presence of `"user"`, `"resources"`, `"rls_rules"`, and `"type" == "guest"`,
but does not
require `"iat"`, so the token proceeds to revocation checking.
3. `get_guest_user_from_request` calls `_is_guest_token_revoked(token)` at
`superset/security/manager.py:48-66`. Inside this function, line 52 reads
`issued_at =
token.get("iat")`, which is `None` for this crafted token, and line 53-54
immediately
returns `False` when `if not issued_at:` is true. No `EmbeddedDashboardDAO`
lookup or
comparison to `guest_token_revoked_before` is performed, regardless of
whether revocation
has been configured for the embedded dashboard.
4. Because `_is_guest_token_revoked` returns `False`,
`get_guest_user_from_request`
returns a `GuestUser` (line 46) and downstream access checks like
`has_guest_access` at
`superset/security/manager.py:3764-21` can still authorize access to the
embedded
dashboard when the resource IDs match. The unit test
`test_guest_token_without_iat_is_not_revoked` at
`tests/unit_tests/security/test_guest_token_revocation.py:62-64` currently
codifies this
behavior, but it also means that any guest token lacking `"iat"` is
effectively exempt
from revocation, weakening the control's effectiveness.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f9513b5cb5540239aad45b815c3e9c3&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=0f9513b5cb5540239aad45b815c3e9c3&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:** 3691:3693
**Comment:**
*Security: Returning `False` when `iat` is missing means tokens without
an issued-at claim are always treated as not revoked, even when a dashboard has
an active revocation cutoff. That weakens the revocation control and allows
bypass for any token lacking `iat`. Reject missing `iat` (or treat it as
revoked once a cutoff exists) to enforce revocation consistently.
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%2F40676&comment_hash=af11721cf35836e92321f3106684ccb7d67403bd80c00936be211aa9b26a7bd8&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=af11721cf35836e92321f3106684ccb7d67403bd80c00936be211aa9b26a7bd8&reaction=dislike'>๐</a>
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before",
None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ embedded.guest_token_revoked_before = (
+ before if before is not None else
int(self._get_current_epoch_time())
Review Comment:
**Suggestion:** The revocation cutoff is truncated to an integer second,
while issued-at values are created from `time.time()` and can contain
fractional seconds. This creates a same-second gap where tokens issued just
before revocation are not invalidated because their `iat` is greater than the
floored cutoff. Store/use consistent precision (or round cutoff up) so all
tokens issued before revocation are reliably rejected. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Recently revoked guest tokens can remain valid briefly.
- โ ๏ธ Revocation semantics differ from intuitive "revoke immediately".
- โ ๏ธ Difficult to reason about exact revocation cutoff behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Issue a guest token using
`SupersetSecurityManager.create_guest_access_token` at
`superset/security/manager.py:25-51`, either directly (as in
`tests/integration_tests/security_tests.py:11-17`) or via the REST API `POST
/api/v1/security/guest_token/` (`superset/security/api.py:149-207`).
`create_guest_access_token` calls `_get_current_epoch_time()`
(`superset/security/manager.py:1-3`), which returns `time.time()` as a
float, and stores
this exact float in the JWT `iat` claim (`"iat": now` at line 44).
2. Shortly after issuing the tokenโwithin the same wall-clock secondโinvoke
`SupersetSecurityManager.revoke_guest_token_access(embedded_uuid)` at
`superset/security/manager.py:68-83` for the corresponding embedded
dashboard UUID. This
method sets `embedded.guest_token_revoked_before = before if before is not
None else
int(self._get_current_epoch_time())` (lines 81-82), truncating the
revocation cutoff to an
integer number of seconds via `int(...)`.
3. On a subsequent HTTP request where the client presents the previously
issued token,
Flask-Login calls `SupersetSecurityManager.get_guest_user_from_request` as a
request
loader (`superset/security/manager.py:660-665` and 3652-46). This decodes
the token, then
calls `_is_guest_token_revoked(token)`
(`superset/security/manager.py:48-66`), which reads
`issued_at = token.get("iat")` (float) and, for the relevant embedded
dashboard resource,
reads `revoked_before = embedded.guest_token_revoked_before` (integer floor
of the
revocation time).
4. Because the comparison in `_is_guest_token_revoked` is `if revoked_before
is not None
and issued_at < revoked_before:` (lines 63-65), any tokens issued in the
interval
`[floor(revocation_time), revocation_time)` have `issued_at >=
revoked_before` even though
they were created before the actual revocation moment. For such tokens
`_is_guest_token_revoked` returns `False`, `get_guest_user_from_request`
accepts the
token, and the user retains access despite revocation having been requested.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37e112c31f2b493ea22d36ad54bbd2e3&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=37e112c31f2b493ea22d36ad54bbd2e3&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:** 3720:3721
**Comment:**
*Logic Error: The revocation cutoff is truncated to an integer second,
while issued-at values are created from `time.time()` and can contain
fractional seconds. This creates a same-second gap where tokens issued just
before revocation are not invalidated because their `iat` is greater than the
floored cutoff. Store/use consistent precision (or round cutoff up) so all
tokens issued before revocation are reliably rejected.
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%2F40676&comment_hash=8a73aae98e47a891263a02c2134acfb6dc9bb26f4283abb01db20261821a46ea&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=8a73aae98e47a891263a02c2134acfb6dc9bb26f4283abb01db20261821a46ea&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]
