rusackas commented on PR #40646: URL: https://github.com/apache/superset/pull/40646#issuecomment-4598927535
Thanks for the thorough review @aminghadersohi — all four addressed in 95fcf02: - **MEDIUM — dead branch (`jwt_verifier.py:587`):** ✅ Applied. `expires_at=int(exp) if exp else None` → `expires_at=int(exp)`; the new `exp is None` guard makes the fallback unreachable. - **MEDIUM — brittle `"Authentication required" in str(exc)`:** ✅ Applied. Added `MCPNoAuthSourceError(ValueError)`; `auth.py` and `middleware.py` now `isinstance`-check instead of matching the string. Subclassing `ValueError` keeps existing handlers/tests working and the fail-open vs fail-closed behavior identical (the two middleware tests now exercise the type distinction directly). - **NIT — duplicate escape logic:** ✅ Applied. Extracted `sanitize_for_log` into `utils/error_sanitization.py`, now used by both the JWT verifier and the validation-error sanitizer. - **NIT — test log side-effect:** ✅ Applied. `test_sanitize_redacts_table_name` now passes `log_original=False`. Affected unit tests (jwt_verifier, error_sanitization, auth, middleware) pass locally and pre-commit is clean. -- 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]
