gabotorresruiz commented on code in PR #45452:
URL: https://github.com/apache/superset/pull/45452#discussion_r4361716125
##########
superset/security/api.py:
##########
@@ -90,6 +94,13 @@ class UserSchema(PermissiveSchema):
keys=fields.String(), values=fields.Raw(allow_none=True),
allow_none=True
)
+ @validates_schema
+ def validate_identity(self, data: dict[str, Any], **kwargs: Any) -> None:
+ try:
+ enforce_guest_token_identity(cast(GuestTokenUser, data))
Review Comment:
Do we still need this hook now that `create_guest_access_token` calls the
same thing? I neutralised lines 97 to 102 on this head and all 101 tests in
`tests/unit_tests/security` and `tests/unit_tests/mcp_service` still passed, as
did all 17 in `TestSecurityGuestTokenApi` and
`TestSecurityGuestTokenApiTokenValidator`,
`test_post_guest_token_invalid_identity` included, because the mint-time call
and the `except GuestTokenIdentityError` below already produce that 400. Its
one measurable effect at the shipping default is that a single mint request
logs the violation twice. Happy to be wrong if it is here for the OpenAPI spec
or for a caller that never reaches `create_guest_access_token`.
##########
superset/security/guest_token.py:
##########
@@ -138,6 +139,82 @@ 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")
+# C0/C1 control characters plus the Unicode line/paragraph separators, i.e.
+# everything ``str.splitlines()`` treats as a line break.
+_INVALID_IDENTITY_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f\u2028\u2029]")
+
+
+class GuestTokenIdentityError(ValueError):
+ """Raised when a guest token's ``user`` claim carries an invalid
identity."""
+
+
+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 log lines, cache
+ keys, and request context. 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 or line separators (e.g. an embedded newline) or an unbounded
+ value.
+
+ ``attributes`` is not covered: it is free-form, host-defined data whose
+ values are not display strings.
+
+ 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 GuestTokenIdentityError: if a provided identity field is not a
+ valid string
+ """
+ if user is None:
+ return
+ if not isinstance(user, dict):
+ raise GuestTokenIdentityError("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 GuestTokenIdentityError(f"Guest token {field} must be a
string")
+ if len(value) > GUEST_TOKEN_IDENTITY_MAX_LENGTH:
+ raise GuestTokenIdentityError(
+ f"Guest token {field} exceeds the maximum length"
+ )
+ if _INVALID_IDENTITY_CHARS_RE.search(value):
+ raise GuestTokenIdentityError(
+ f"Guest token {field} contains invalid characters"
+ )
+
+
+# Warn-first rollout flag. While False, enforce_guest_token_identity logs an
+# invalid identity instead of raising, so a host already sending a long or
+# multi-line value is not broken on deploy. Flip to True to enforce.
+GUEST_TOKEN_IDENTITY_ENFORCE = False
Review Comment:
This block worries me a bit, and it is the one note I would rather settle
before the flag day than after. Line 213 is the only read of
`GUEST_TOKEN_IDENTITY_ENFORCE` and it is a module global, so
`GUEST_TOKEN_IDENTITY_ENFORCE = True` in `superset_config.py` does nothing: I
set it in the app config on this branch and a 300 character `username` still
minted. That leaves flipping it as an edit to this file, so a deployment that
wants the hardening on today cannot have it, and `UPDATING.md` names the symbol
in a way that reads operator facing.
`GUEST_TOKEN_REVOCATION_ENABLED` is the same shape of guest token opt-in and
sits in `superset/config.py:3357`. `get_conf()` lives in `security/manager.py`
so importing it here would cycle, but it only returns `current_app.config`, and
all four call sites are already inside an app context (`_verify_guest_sync`
wraps its body in `self._app.app_context()`), so:
```python
if current_app.config["GUEST_TOKEN_IDENTITY_ENFORCE"]:
raise
```
with the default alongside `GUEST_TOKEN_REVOCATION_ENABLED` in
`superset/config.py`, and the tests swapping `patch(...)` for the `with_config`
helper `api_tests.py` already uses. Or is keeping the flag day a code change
deliberate, so that it goes through review rather than a config edit?
##########
superset/security/guest_token.py:
##########
@@ -138,6 +139,82 @@ 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")
+# C0/C1 control characters plus the Unicode line/paragraph separators, i.e.
+# everything ``str.splitlines()`` treats as a line break.
+_INVALID_IDENTITY_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f\u2028\u2029]")
+
+
+class GuestTokenIdentityError(ValueError):
+ """Raised when a guest token's ``user`` claim carries an invalid
identity."""
+
+
+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 log lines, cache
+ keys, and request context. 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 or line separators (e.g. an embedded newline) or an unbounded
+ value.
+
+ ``attributes`` is not covered: it is free-form, host-defined data whose
+ values are not display strings.
+
+ 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 GuestTokenIdentityError: if a provided identity field is not a
+ valid string
+ """
+ if user is None:
+ return
+ if not isinstance(user, dict):
+ raise GuestTokenIdentityError("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 GuestTokenIdentityError(f"Guest token {field} must be a
string")
+ if len(value) > GUEST_TOKEN_IDENTITY_MAX_LENGTH:
+ raise GuestTokenIdentityError(
+ f"Guest token {field} exceeds the maximum length"
+ )
+ if _INVALID_IDENTITY_CHARS_RE.search(value):
+ raise GuestTokenIdentityError(
+ f"Guest token {field} contains invalid characters"
+ )
+
+
+# Warn-first rollout flag. While False, enforce_guest_token_identity logs an
+# invalid identity instead of raising, so a host already sending a long or
+# multi-line value is not broken on deploy. Flip to True to enforce.
+GUEST_TOKEN_IDENTITY_ENFORCE = False
+
+
+def enforce_guest_token_identity(user: Optional[GuestTokenUser]) -> None:
+ """Validate a guest token identity, honoring the warn-first rollout flag.
+
+ While ``GUEST_TOKEN_IDENTITY_ENFORCE`` is False an invalid identity is
logged
+ and allowed through; once it is True the underlying
``GuestTokenIdentityError``
+ propagates and the caller rejects the token. Mint and consume paths call
this
+ rather than ``validate_guest_token_identity`` directly.
+ """
+ try:
+ validate_guest_token_identity(user)
+ except GuestTokenIdentityError as exc:
+ if GUEST_TOKEN_IDENTITY_ENFORCE:
Review Comment:
Following your point that `get_guest_user_from_token` runs outside the
request loader's `try`: the non-object branch of
`validate_guest_token_identity` is the one case where that bites under the warn
default. A `user` claim that is a bare string gets logged here, falls through,
and `GuestUser.__init__` calls `user.get("username")` on a `str`, which raises
out of `get_guest_user_from_request` at `manager.py:6238`, past the `except
Exception` that would have turned it into a 401. On this head the warn default
gives `AttributeError: 'str' object has no attribute 'get'`; with the flag on
it returns `None` cleanly. The same `AttributeError` is there at the merge
base, so it is not new, but your enforce path is what fixes it and warn-first
declines to.
It is not reachable through the mint API, since `UserSchema` already answers
`{'user': {'_schema': ['Invalid input type.']}}` for a non-object, so it takes
a Python caller or a token minted elsewhere with the shared secret. Still,
would it be worth letting that one branch raise regardless of the flag? It is a
crash guard rather than a display-length policy, so there is no existing
integration it could break:
```python
class GuestTokenIdentityShapeError(GuestTokenIdentityError):
"""The ``user`` claim is not an object at all; never warn-only."""
```
and let that subclass through. I might be missing a reason you want it
uniform across all shapes.
##########
superset/security/guest_token.py:
##########
@@ -138,6 +139,82 @@ 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")
+# C0/C1 control characters plus the Unicode line/paragraph separators, i.e.
+# everything ``str.splitlines()`` treats as a line break.
+_INVALID_IDENTITY_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f\u2028\u2029]")
+
+
+class GuestTokenIdentityError(ValueError):
+ """Raised when a guest token's ``user`` claim carries an invalid
identity."""
+
+
+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 log lines, cache
+ keys, and request context. 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 or line separators (e.g. an embedded newline) or an unbounded
+ value.
+
+ ``attributes`` is not covered: it is free-form, host-defined data whose
+ values are not display strings.
+
+ 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 GuestTokenIdentityError: if a provided identity field is not a
+ valid string
+ """
+ if user is None:
+ return
+ if not isinstance(user, dict):
+ raise GuestTokenIdentityError("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 GuestTokenIdentityError(f"Guest token {field} must be a
string")
+ if len(value) > GUEST_TOKEN_IDENTITY_MAX_LENGTH:
+ raise GuestTokenIdentityError(
+ f"Guest token {field} exceeds the maximum length"
+ )
+ if _INVALID_IDENTITY_CHARS_RE.search(value):
+ raise GuestTokenIdentityError(
+ f"Guest token {field} contains invalid characters"
+ )
+
+
+# Warn-first rollout flag. While False, enforce_guest_token_identity logs an
+# invalid identity instead of raising, so a host already sending a long or
+# multi-line value is not broken on deploy. Flip to True to enforce.
+GUEST_TOKEN_IDENTITY_ENFORCE = False
+
+
+def enforce_guest_token_identity(user: Optional[GuestTokenUser]) -> None:
+ """Validate a guest token identity, honoring the warn-first rollout flag.
+
+ While ``GUEST_TOKEN_IDENTITY_ENFORCE`` is False an invalid identity is
logged
+ and allowed through; once it is True the underlying
``GuestTokenIdentityError``
+ propagates and the caller rejects the token. Mint and consume paths call
this
+ rather than ``validate_guest_token_identity`` directly.
+ """
+ try:
+ validate_guest_token_identity(user)
+ except GuestTokenIdentityError as exc:
+ if GUEST_TOKEN_IDENTITY_ENFORCE:
+ raise
+ logger.warning("Guest token identity violation (warn-only): %s", exc)
Review Comment:
Good call building the logging before the enforcement, that is exactly the
order I was hoping for. One gap for whoever has to read it: the line carries
the field name but nothing identifying where the token came from, and on the
consume path it fires once per request for the token's whole life, so one bad
token over 5 requests gave me 5 identical warnings. An operator watching this
cannot tell one noisy host application from five.
`_log_audit_event` at `superset/security/manager.py:341` already does
`stats_logger_manager.instance.incr(f"security.{action}")` plus the event
logger, and is used for `UserLoggedIn` in the same class, so a countable
`security.GuestTokenIdentityViolation` is nearly free. Would an optional
argument carrying `token["resources"]` be enough to make it attributable, or is
the dashboard uuid too much to put in a log line?
--
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]
