sha174n commented on code in PR #45452:
URL: https://github.com/apache/superset/pull/45452#discussion_r4362280555
##########
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:
Moved `GUEST_TOKEN_IDENTITY_ENFORCE` into `superset/config.py` next to
`GUEST_TOKEN_REVOCATION_ENABLED`, read via `current_app.config`, with tests on
`with_config` / `patch.dict(app.config)` and `UPDATING.md` updated, in
f8f214aefb.
##########
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:
Added `GuestTokenIdentityShapeError`, which `enforce_guest_token_identity`
always re-raises, plus a request-flow test showing a bare-string `user` claim
now returns None under the warn default, in f8f214aefb.
##########
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:
Warn-only violations now increment `security.GuestTokenIdentityViolation`
(via a helper shared with `_log_audit_event`) and the log line carries the
token's resource ids, in f8f214aefb.
##########
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:
Removed the `UserSchema` hook; mint-time validation lives only in
`create_guest_access_token`, in f8f214aefb.
--
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]
