sadpandajoe commented on code in PR #44365:
URL: https://github.com/apache/superset/pull/44365#discussion_r4201626269
##########
superset/security/api.py:
##########
@@ -279,6 +295,195 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ # Request data is deliberately excluded from the event log. A resolver may
+ # read the caller's proof of identity -- an OIDC id token, an internal
+ # service credential -- from the body or query string, and
+ # ``collect_request_payload`` would otherwise persist it verbatim into
+ # ``logs.json``, including on rejection. The one-time token's TTL bounds
+ # nothing about that upstream credential's lifetime.
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.login_token",
+ include_request_data=False,
+ )
+ @safe
+ @statsd_metrics
+ @transaction()
+ def login_token(self) -> Response:
+ """Mint a one-time login token for iframe embedding.
+ ---
+ post:
+ summary: Mint a one-time login token
+ description: >-
+ Exchanges a caller-supplied proof of identity for an opaque,
single-use
+ token that GET on this same path trades for a session cookie.
Intended
+ to be called server-to-server by a trusted parent application so
the
+ underlying credential never reaches the browser. The
+ LOGIN_TOKEN_IDENTITY_RESOLVER hook decides what counts as proof.
+ responses:
+ 200:
+ description: The minted token and its expiry
+ content:
+ application/json:
+ schema: LoginTokenResponseSchema
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ # 404 rather than 403: with the feature off there is nothing here
to
+ # be forbidden from, and this keeps the surface closed by default.
+ return self.response_404()
+
+ if (userinfo := login_token_utils.resolve_identity(request)) is None:
+ return self.response_401()
+
+ token, expires_on = login_token_utils.mint(userinfo)
+ logger.info(
+ "One-time login token minted for '%s' from %s",
+ userinfo.get("username") or userinfo.get("email"),
+ request.remote_addr,
+ )
+ return self.response(
+ 200,
+ access_token=token,
+ expires_at=int(expires_on.timestamp()),
+ )
+
+ @expose("/login-token/", methods=("GET",))
+ # Request data is excluded from the event log for the same reason as on
mint:
+ # the token travels in the query string, and ``collect_request_payload``
+ # would otherwise copy it into ``logs.json``. A row written while the token
+ # is still live -- for instance by a request that fails before the burn
+ # commits -- would be a redeemable credential at rest. The action name is
+ # left as the default, so existing log queries are unaffected.
+ @event_logger.log_this_with_context(include_request_data=False)
+ @statsd_metrics
+ @safe
+ @transaction()
+ def login_with_token(self) -> Response:
+ """Consume a one-time login token and establish a session.
+ ---
+ get:
+ summary: Consume a one-time login token
+ description: >-
+ Reached by navigating an iframe to this URL. Exchanges the token
for a
+ standard session cookie and redirects to `next`, so the frame
holds an
+ ordinary Superset session with the user's own roles and row-level
+ security. The token is deleted on use.
+ parameters:
+ - in: query
+ name: token
+ required: true
+ schema:
+ type: string
+ description: The opaque token returned by POST on this path
+ - in: query
+ name: next
+ required: false
+ schema:
+ type: string
+ description: >-
+ Site-relative path to redirect to, e.g. `/dashboard/1/`. Must
begin
+ with a single `/`; absolute URLs and protocol-relative values are
+ rejected and fall back to `/`.
+ responses:
+ 302:
+ description: Session established; redirect to `next`
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ return self.response_404()
+
+ token = request.args.get("token", "")
+ # A single failure mode for unknown, malformed, expired and
already-spent
+ # tokens, so the response cannot be used to probe which one it was.
+ if not token or (userinfo := login_token_utils.consume(token)) is None:
+ return self.response_401()
+
+ # Refuse to replace a different user's session. A token is a bearer
+ # credential that is not bound to a browser, so without this an
attacker
+ # who mints for their own identity and gets a signed-in victim's frame
to
+ # navigate here would silently swap the victim into the attacker's
+ # account (login CSRF). The token is already burned, so a refused one
+ # cannot be retried, and the check runs before provisioning so a
refused
+ # exchange never registers or updates the attacker's account. The
+ # identifier is selected the way ``auth_user_oauth`` selects it. A user
+ # re-establishing their own session -- a reloaded frame -- is
unaffected.
+ #
+ # This only narrows the residual risk: a victim with no Superset
session,
+ # the usual case for an embed, still has nothing here to compare
against.
+ resolved_username = userinfo.get("username") or userinfo.get("email")
+ if (
+ getattr(current_user, "is_authenticated", False)
+ and getattr(current_user, "username", None) != resolved_username
Review Comment:
The new different-user guard compares `current_user.username` to the
resolver's identifier byte for byte, but `auth_user_oauth` resolves the account
through `find_user`, which is case-insensitive by default
(`AUTH_USERNAME_CI=True`). With an existing user `Alice` and a resolver that
returns `alice`, the first redemption logs in as `Alice`; redeeming another
token when the iframe reloads lands here, returns 401, and the token is already
burned, so reloads of a legitimate embed fail. The comment at
`login_token.py:226` also describes `find_user` as an exact match, which isn't
the default. Should this resolve the account the way `auth_user_oauth` does
(for example compare `find_user(...)` ids against `current_user`) instead of
comparing raw strings?
##########
superset/security/api.py:
##########
@@ -268,6 +284,154 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ # Request data is deliberately excluded from the event log. A resolver may
+ # read the caller's proof of identity -- an OIDC id token, an internal
+ # service credential -- from the body or query string, and
+ # ``collect_request_payload`` would otherwise persist it verbatim into
+ # ``logs.json``, including on rejection. The one-time token's TTL bounds
+ # nothing about that upstream credential's lifetime.
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.login_token",
+ include_request_data=False,
+ )
+ @safe
+ @statsd_metrics
+ @transaction()
+ def login_token(self) -> Response:
+ """Mint a one-time login token for iframe embedding.
+ ---
+ post:
+ summary: Mint a one-time login token
+ description: >-
+ Exchanges a caller-supplied proof of identity for an opaque,
single-use
+ token that GET on this same path trades for a session cookie.
Intended
+ to be called server-to-server by a trusted parent application so
the
+ underlying credential never reaches the browser. The
+ LOGIN_TOKEN_IDENTITY_RESOLVER hook decides what counts as proof.
+ responses:
+ 200:
+ description: The minted token and its expiry
+ content:
+ application/json:
+ schema: LoginTokenResponseSchema
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ # 404 rather than 403: with the feature off there is nothing here
to
+ # be forbidden from, and this keeps the surface closed by default.
+ return self.response_404()
+
+ if (userinfo := login_token_utils.resolve_identity(request)) is None:
+ return self.response_401()
+
+ token, expires_on = login_token_utils.mint(userinfo)
+ logger.info(
+ "One-time login token minted for '%s' from %s",
+ userinfo.get("username") or userinfo.get("email"),
+ request.remote_addr,
+ )
+ return self.response(
+ 200,
+ access_token=token,
+ expires_at=int(expires_on.timestamp()),
+ )
+
+ @expose("/login-token/", methods=("GET",))
+ @event_logger.log_this
+ @statsd_metrics
+ @safe
+ @transaction()
+ def login_with_token(self) -> Response:
+ """Consume a one-time login token and establish a session.
+ ---
+ get:
+ summary: Consume a one-time login token
+ description: >-
+ Reached by navigating an iframe to this URL. Exchanges the token
for a
+ standard session cookie and redirects to `next`, so the frame
holds an
+ ordinary Superset session with the user's own roles and row-level
+ security. The token is deleted on use.
+ parameters:
+ - in: query
+ name: token
+ required: true
+ schema:
+ type: string
+ description: The opaque token returned by POST on this path
+ - in: query
+ name: next
+ required: false
+ schema:
+ type: string
+ description: >-
+ Site-relative path to redirect to, e.g. `/dashboard/1/`. Must
begin
+ with a single `/`; absolute URLs and protocol-relative values are
+ rejected and fall back to `/`.
+ responses:
+ 302:
+ description: Session established; redirect to `next`
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ return self.response_404()
+
+ token = request.args.get("token", "")
+ # A single failure mode for unknown, malformed, expired and
already-spent
+ # tokens, so the response cannot be used to probe which one it was.
+ if not token or (userinfo := login_token_utils.consume(token)) is None:
+ return self.response_401()
+
+ # ``consume`` has already committed the burn, so nothing here can make
a
+ # spent token redeemable again. Provisioning failures are still caught
+ # and reported as a denial rather than allowed to escape: an exception
+ # would otherwise surface as a 500, which is indistinguishable from an
+ # outage to the parent application and invites a retry with a token
that
+ # no longer exists.
+ try:
+ user = self.appbuilder.sm.auth_user_oauth(userinfo)
+ except Exception: # pylint: disable=broad-except
+ logger.exception("Provisioning failed for a one-time login token")
+ user = None
+
+ if user is None:
+ # Provisioning declined the identity: the user is deactivated, or
+ # AUTH_USER_REGISTRATION is off and they have no account yet.
+ logger.warning(
+ "One-time login token resolved an identity that could not be "
+ "provisioned: '%s'",
+ userinfo.get("username") or userinfo.get("email"),
+ )
+ return self.response_401()
+
+ login_user(user)
+ logger.info("Session established from a one-time login token for
'%s'", user)
+
+ # Only ever redirect to a value that has passed the relative-path
check.
+ # Assigning into a separate variable inside the guarded branch — rather
+ # than reassigning the request-derived one — keeps the sanitizer on the
+ # path to the redirect, which taint analysis can follow.
+ requested_next = request.args.get("next") or "/"
+ safe_next_url = "/"
+ if login_token_utils.is_safe_next_path(requested_next):
+ safe_next_url = requested_next
Review Comment:
Trailing CR/LF still gets through: `is_safe_next_path` strips before it
looks for control characters (`login_token.py:124-125`), so
`next=%2Fdashboard%2F1%2F%0D%0A` decodes to `/dashboard/1/\r\n`, the trimmed
copy passes, and `redirect()` here still receives the original value. Werkzeug
refuses a newline in `Location`, so the handler returns 500 after the token is
burned and the session is established, and a retry gets 401. The CR/LF cases in
`login_token_api_tests.py:699` and `login_token_test.py:393` all put characters
after the newline, so they don't hit it. Could the control-character check run
on `url` before `strip()`, with a trailing-newline case added to both tests?
--
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]
