Repository navigation
Conversation
The ``enforce_session_validity`` ``before_request`` hook resolved ``current_user`` on every request, including anonymous page loads, static assets and token-authenticated API calls. That is wasteful — ``current_user`` is a LocalProxy whose resolution can query the metadata database — and it is fragile: deployments are free to install a Flask-Login ``request_loader``, and a loader that *raises* for credential-less requests (a common way to trigger a custom login redirect) instead of returning the anonymous user turns every anonymous request into a warning with a full traceback. Upstream assumed ``current_user`` always resolves to an anonymous user, which only holds for cookie sessions. The hook can only ever invalidate a *session* login: the per-user epoch is compared against ``_login_at``, stamped into the session at login. So return before touching ``current_user`` when the request carries no session login, and log a JWT-based loader finding no token at debug rather than warning — an unauthenticated request is not a failure of this check. A "remember me" cookie still counts as a session login: Flask-Login restores it while resolving ``current_user``, after this hook runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #8a61c7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44528 +/- ##
==========================================
- Coverage 66.22% 66.19% -0.03%
==========================================
Files 2949 2949
Lines 177381 177384 +3
Branches 41107 41102 -5
==========================================
- Hits 117465 117414 -51
- Misses 57413 57464 +51
- Partials 2503 2506 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Good catch on the anonymous-request hot path and the remember-cookie edge case, the early-return logic there checks out.
One thing before this merges though: except JWTExtendedException is wider than the "no usable token" case this is meant to quiet down. That base class also covers RevokedTokenError, JWTDecodeError (a tampered/malformed token), CSRFError, UserClaimsVerificationError, and WrongTokenError, all of which are more interesting than ordinary anonymous traffic and arguably still belong at warning. Since the PR's own tests and the manual repro only ever exercise NoAuthorizationError, narrowing the except to that (plus UserLookupError if you want to cover the "no role" case too, same as #44213) would quiet the real noise without losing visibility into revoked/tampered tokens hitting this path.
Separately, the one failing check (check_pot_drift) is unrelated to this change, it's flagging a string from something else merged to master since this branch was cut. A rebase should clear it.
This branch has not been deployed
2 participants
SUMMARY
SupersetAppInitializerregistersenforce_session_validityas an unconditionalbefore_requesthook, and the hook resolvescurrent_useron every request —anonymous page loads, static assets, token-authenticated API calls included.
Two problems with that:
It is wasteful.
current_useris aLocalProxywhose resolution can hitthe metadata database. Nothing about an anonymous request needs it: this hook
can only ever invalidate a session login, because the per-user epoch is
compared against
_login_at, which is stamped into the session at login time.A request with no session login has nothing for the hook to invalidate.
It assumes
current_usernever raises. That holds for cookie sessions,where an unauthenticated request resolves to the anonymous user. It does not
hold for deployments that install a Flask-Login
request_loader— raisingfrom the loader is a common way to trigger a custom login redirect. Under such
a deployment every anonymous request produces a
logger.warningwith a fulltraceback from the fail-open handler. In one deployment that is hundreds of MB
of log noise per day, all of it describing ordinary unauthenticated traffic.
The fix is two small changes to
enforce_session_validity:current_user, when the request carries nosession login. A "remember me" cookie still counts as one, since Flask-Login
restores that login while resolving
current_user— i.e. after this hook runs— so the cookie's presence keeps the check enabled.
except: a JWT-based loader that finds no usable token raisesJWTExtendedException, which now logs atdebuginstead ofwarning. Anunauthenticated request is not a failure of this check. Any other error keeps
the existing fail-open
warningwithexc_info.Behaviour for the case the mechanism exists to serve is unchanged: a browser
session for a disabled user carries
_user_id, so it is still checked and stillforced out on its next request.
This follows the same reasoning as the existing health-probe early return
(#43786), generalized from one endpoint to "no session login".
TESTING INSTRUCTIONS
New unit tests cover:
current_user(asserts__bool__is notcalled, mirroring the existing health-probe test);
NoAuthorizationErroris logged atdebug, notwarning, and the request is allowed.Manual: install a Flask-Login
request_loaderthat raisesNoAuthorizationErrorfor credential-less requests, hit any anonymous route,and confirm the logs stay clean; then log in, disable the account, and confirm
the next request is still forced out.
ADDITIONAL INFORMATION