aminghadersohi commented on code in PR #40684:
URL: https://github.com/apache/superset/pull/40684#discussion_r3342927792
##########
superset/mcp_service/chart/validation/pipeline.py:
##########
@@ -149,8 +149,10 @@ def validate_request_with_warnings(
ChartErrorBuilder,
)
- # SECURITY FIX: Sanitize validation error to prevent information
disclosure
- sanitized_reason = _sanitize_validation_error(e)
+ # SECURITY FIX: Sanitize validation error to prevent information
disclosure.
+ # logger.exception above already logged the original error; pass
+ # log_original=False so the sanitizer does not log it a second
time at INFO.
+ sanitized_reason = _sanitize_validation_error(e,
log_original=False)
Review Comment:
PRAISE — Multi-line comment explains the `log_original=False` rationale
precisely: `logger.exception` above already captured the full traceback so the
sanitizer must not re-log at INFO. Comment and fix together make the invariant
self-documenting.
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -564,7 +564,7 @@ async def load_access_token(self, token: str) ->
AccessToken | None: # noqa: C9
"JWT authentication succeeded: client_id='%s', scopes=%s, "
"auth_method='bearer_jwt'",
_sanitize_for_log(client_id),
- _sanitize_for_log(sorted(scopes)),
+ _sanitize_for_log(" ".join(sorted(scopes))),
Review Comment:
PRAISE — Formatting scopes as `"read write"` matches OAuth 2.0 RFC 6749 §3.3
scope syntax (space-delimited tokens) and is more human-readable than `['read',
'write']` in logs.
NIT — When `scopes` is empty, `" ".join(sorted(scopes))` yields `""`, so the
log reads `scopes=, auth_method='bearer_jwt'`. Slightly less self-documenting
than the former `scopes=[]`. Consider `" ".join(sorted(scopes)) or "(none)"`
for the empty case.
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -325,7 +325,7 @@ def _auth_error_handler(conn: HTTPConnection, exc:
AuthenticationError) -> Respo
logger.warning(
"JWT authentication failed: %s (source_ip=%s, path=%s, user_agent=%s)",
- exc,
+ _sanitize_for_log(exc),
Review Comment:
PRAISE — `_sanitize_for_log(exc)` wrapping adds defense-in-depth. Even
though `DetailedBearerAuthBackend` only raises with one of the 8 generic reason
strings today, upstream fastmcp could produce richer `AuthenticationError`
messages in a future version. Sanitizing unconditionally is the right posture.
--
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]
