aminghadersohi commented on code in PR #43786:
URL: https://github.com/apache/superset/pull/43786#discussion_r4048973491
##########
superset/security/session_invalidation.py:
##########
@@ -50,6 +50,10 @@
#: Session key holding the epoch-seconds timestamp of when the session logged
in.
SESSION_LOGIN_AT_KEY = "_login_at"
+# Health checks are deliberately independent of authentication and the metadata
+# database, so they must not resolve ``current_user`` or perform session
checks.
+_HEALTH_CHECK_PATHS = frozenset({"/health", "/healthcheck", "/ping"})
Review Comment:
Adopted in `7a17771`, and thanks for the pointer — I re-verified the
ordering rather than take it on faith. `RequestContext.push()` calls
`match_request()` at the end of its body, which sets `request.url_rule`, and
that happens when the context is pushed in `wsgi_app()`, well before
`full_dispatch_request()` reaches `preprocess_request()`. Confirmed empirically
too: a `before_request` hook on a blueprint registered exactly like `health`
sees `request.endpoint == "health.health"` for `/health`, `/healthcheck` and
`/ping`, and `None` for an unrouted target. So the path check was buying
nothing that the endpoint check does not, and the endpoint form covers a fourth
route for free.
Took your test as written, driven off `app.url_map.iter_rules()` on the real
app fixture, including the `current_user.__bool__.assert_not_called()`
assertion — that is the part the old test only implied. `has_request_context()`
is kept, since the other tests in the file call `enforce_session_validity()`
with no request context. `tests/unit_tests/security/` is at 346 passed, and the
new test fails on `current.__bool__.assert_not_called()` if the early return is
deleted.
I also corrected the description. You are right that the "probe availability
independent of authentication metadata" claim was too broad: an unauthenticated
probe already costs nothing on `master` and the hook fails open with a dead
metadata database. The body now states the narrower, real delta — 3 queries
versus 0 on a probe carrying a session cookie, plus the fail-open warning going
away.
--
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]
