gabotorresruiz commented on code in PR #45452:
URL: https://github.com/apache/superset/pull/45452#discussion_r4328097492
##########
superset/security/guest_token.py:
##########
@@ -138,6 +139,44 @@ class GuestTokenUser(TypedDict, total=False):
attributes: Optional[dict[str, Any]]
+# Upper bound for the identity strings carried in a guest token's ``user``
+# claim, a generous bound for a display identity.
+GUEST_TOKEN_IDENTITY_MAX_LENGTH = 255
+GUEST_TOKEN_IDENTITY_FIELDS = ("username", "first_name", "last_name")
Review Comment:
Question on the field list rather than the code. `attributes` is the other
half of the `user` claim, it is typed `Optional[dict[str, Any]]`, declared with
`fields.Raw` values and no bound, and it reaches the templated SQL surface
through `get_guest_user_attribute` (`superset/jinja_context.py:400`).
Deliberately out of scope, or should it get a bound here too?
Related, and this is where I might be misreading the docstring:
`current_username` already escapes through the dialect before interpolation
(`superset/jinja_context.py:257`), so the templated query context looks covered
at the sink for these three fields. Is the real driver here the log and header
surface rather than templating?
##########
superset/security/manager.py:
##########
@@ -6165,6 +6167,7 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
token = self.parse_jwt_guest_token(raw_token)
if token.get("user") is None:
raise ValueError("Guest token does not contain a user claim")
+ validate_guest_token_identity(token["user"])
Review Comment:
This is the only line in the diff that changes behaviour for tokens that
already exist, and I think refusing is right for a value that ends up in a
cache key, but nothing pins it.
I checked it on this branch: a token minted before this change carrying a
256 character `username`, or a `username` with a trailing newline, no longer
resolves to a guest. `get_guest_user_from_request` returns `None` and the
request runs as the anonymous principal instead. Deleting this line makes no
test fail, in `tests/unit_tests/security/` or in
`tests/integration_tests/security/`, so the new `api_tests.py` case only covers
the request schema path. `test_revoked_token_rejected_through_request_flow` in
`tests/unit_tests/security/guest_token_revocation_test.py` is a ready made
template: mint a token, hand it to `get_guest_user_from_request` through a fake
request, assert `None`.
Worth an `UPDATING.md` note under `## Next` too. A host application whose
directory legitimately hands you a long display name, or a `username` with a
tab in it, has working embedded dashboards today and loses them after this
lands, with only a `logger.warning("Invalid guest token")` to go on.
##########
superset/security/guest_token.py:
##########
@@ -138,6 +139,44 @@ class GuestTokenUser(TypedDict, total=False):
attributes: Optional[dict[str, Any]]
+# Upper bound for the identity strings carried in a guest token's ``user``
+# claim, a generous bound for a display identity.
+GUEST_TOKEN_IDENTITY_MAX_LENGTH = 255
+GUEST_TOKEN_IDENTITY_FIELDS = ("username", "first_name", "last_name")
+
+
+def validate_guest_token_identity(user: Optional[GuestTokenUser]) -> None:
+ """Validate the identity strings carried in a guest token's ``user`` claim.
+
+ ``username``/``first_name``/``last_name`` are supplied by the host
+ application that mints the token and later surface in logging, request
+ context, and templated query contexts. Constrain them to bounded,
+ single-line strings where they enter the system, consistent with how other
+ request-supplied strings are validated: a display identity has no reason to
+ carry control characters (e.g. an embedded newline) or an unbounded value.
+
+ A missing ``user`` claim is not rejected here; that check belongs to the
+ point where the token is consumed.
+
+ :param user: the ``user`` claim from a guest token
+ :raises ValueError: if a provided identity field is not a valid string
+ """
+ if user is None:
+ return
+ if not isinstance(user, dict):
+ raise ValueError("Guest token user must be an object")
+ for field in GUEST_TOKEN_IDENTITY_FIELDS:
+ value = user.get(field)
+ if value is None:
+ continue
+ if not isinstance(value, str):
+ raise ValueError(f"Guest token {field} must be a string")
+ if len(value) > GUEST_TOKEN_IDENTITY_MAX_LENGTH:
+ raise ValueError(f"Guest token {field} exceeds the maximum length")
+ if CONTROL_CHARS_RE.search(value):
Review Comment:
Just a small NIT on the "single-line" wording: `U+2028` and `U+2029` get
through this, and `str.splitlines()` treats both as line breaks. Everything
else `splitlines` splits on is already inside the class, so it is only those
two.
```suggestion
if CONTROL_CHARS_RE.search(value) or any(c in value for c in
"\u2028\u2029"):
raise ValueError(f"Guest token {field} contains invalid
characters")
```
Equally happy if you would rather leave the check alone and say "control
characters" in the docstring instead, since those two are separators rather
than control characters.
##########
superset/utils/file.py:
##########
@@ -22,7 +22,7 @@
# All C0 (U+0000–U+001F) and C1 (U+007F–U+009F) control characters.
# Stripping every control char (including tab, LF, CR) keeps titles safe for
# SMTP headers, Content-Disposition filenames, and headless-browser
document.title.
-_CONTROL_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]")
+CONTROL_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]")
Review Comment:
NIT: now that this is shared with the guest token validator, the comment
above it describes only half its callers. Someone loosening it for export
filenames would be loosening an auth path check with no hint of that here.
Worth naming the second caller.
##########
superset/security/manager.py:
##########
@@ -6109,6 +6110,7 @@ def create_guest_access_token(
rls: list[GuestTokenRlsRule],
datasets: Optional[list[int]] = None,
) -> bytes:
+ validate_guest_token_identity(user)
Review Comment:
`UserSchema.validate_identity` already runs this exact check on the request
body, so by the time we reach here the API payload is validated, and removing
this line makes no test fail either. Fine as insurance for non-HTTP callers,
but the two paths answer differently: `api.py` catches `ValidationError` and
not `ValueError`, so if this guard ever fires ahead of the schema the caller
gets a 500 rather than a 400. Would it be cleaner to raise `ValidationError`
here, or to add `ValueError` to the handler at `superset/security/api.py:304`?
--
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]
