rusackas opened a new pull request, #40646:
URL: https://github.com/apache/superset/pull/40646
### SUMMARY
Hardens authentication error handling and logging in the Superset MCP
service. The changes keep client-facing output generic while preserving full
diagnostics server-side, require token expiry, and make auth logging both more
useful and safer.
Changes:
1. **Generic auth error** — `get_user_from_request()` in `auth.py`
previously raised a `ValueError` whose message embedded server configuration
(`MCP_AUTH_ENABLED`, whether JWT keys / API key / `MCP_DEV_USERNAME` were set,
API-key prefix). The client-facing message is now generic ("Authentication
required. No valid credentials provided.") and the detailed diagnostics are
emitted via `logger.warning`/`logger.debug` server-side. The exception
type/flow is unchanged so callers still treat it as an auth failure.
2. **Log successful auth** — `DetailedJWTVerifier` now emits a structured
`logger.info` on the success path with safe metadata only (`client_id`,
`scopes`, auth method). Token contents and secrets are never logged.
3. **Require `exp` claim** — tokens without an `exp` claim are now rejected
(previously only expired tokens were rejected; a missing `exp` was accepted).
4. **Auth-failure log context** — the auth-failure handler now includes
request context (source IP, request path, user-agent) in its server-side
warning log, guarding for missing/partial connection fields.
5. **Log before sanitizing** — `_sanitize_validation_error` gains an
optional `log_original: bool = True` parameter that logs the original
(unsanitized) error server-side before returning the sanitized message. The
sanitized client output is unchanged.
6. **Sanitize claim values in debug logs** — attacker-controlled claim
values (alg, iss, aud, client_id) logged at debug now pass through a small
`_sanitize_for_log` helper that escapes `\n`/`\r`/`\t` to prevent log-line
injection.
Supporting updates: `middleware.py` and a few existing tests that depended
on the previous error string were updated to the new generic message.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend logging/error behavior).
### TESTING INSTRUCTIONS
- `python -m pytest tests/unit_tests/mcp_service/ -q` — full MCP service
suite (2132 passed locally).
- New/updated unit tests cover: the generic auth error message contains no
configuration details; a token missing `exp` is rejected; successful auth is
logged with safe metadata only; `_sanitize_for_log` escapes newlines;
`_sanitize_validation_error` logs the original error server-side without
changing the sanitized client output.
- `python -m ruff check` passes on all changed files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
