feat(auth): add session refresh and validate endpoints - #6235
Conversation
|
LGTM! ✅ |
Lang-Akshay
left a comment
There was a problem hiding this comment.
Blocking Changes
| # | Area | File | Line | Blocking reason | Required change |
|---|---|---|---|---|---|
| 1 | Security | mcpgateway/routers/auth.py |
385-396, 479-507 | High / CWE-347: refresh and validation decode the request token with signature verification disabled. The security handoff found that get_current_user can involve a plugin/custom auth path, so the handler may trust forged token_use, session_start, scopes, and expiry claims. |
Use the exact payload returned by a verified JWT path, or reject refresh/validate for non-gateway-JWT auth methods. For cookie auth, verify the selected cookie token and bind its subject/JTI to the authenticated user before making session decisions. |
| 2 | Issue / Quality | mcpgateway/config.py, .env.example |
config.py:385-403, .env.example:1090-1105 |
Linked issue #6011 explicitly requires SESSION_IDLE_TIMEOUT with a 900-second default. The PR instead exposes the pre-existing TOKEN_IDLE_TIMEOUT (60-minute default) and does not add the requested setting. |
Add and surface SESSION_IDLE_TIMEOUT with the issue's required default/units, or obtain an explicit issue-level decision changing that acceptance criterion. |
| 3 | Issue / Verification | PR metadata | N/A | Issue #6011 requires make verify; the PR description marks full verification as pending CI, so this required acceptance criterion is unresolved in the reviewed state. |
Run and pass the required verification in CI before merge, and attach the result to the PR. |
|
Thanks for the thorough review @Lang-Akshay — the security catch in particular was a real gap. Here's where each finding stands: 1. Unverified token decode (CWE-347) — Fixed in a052f0b. Both endpoints now route the extracted token through 2. 3. Verification — |
a052f0b to
99fe794
Compare
Lang-Akshay
left a comment
There was a problem hiding this comment.
Blocking Changes
| # | Area | File | Line | Blocking reason | Required change |
|---|---|---|---|---|---|
| 1 | Quality / Issue | mcpgateway/routers/auth.py |
478-492, 511-531 | Both endpoints depend on get_current_user(), while the configured bearer dependency reads only the auth header. Cookie extraction in _extract_raw_token() is reached only after dependency authentication; a cookie-only request therefore fails before refresh/validate logic runs. This contradicts the PR's cookie-session requirement and makes the from_cookie cookie-reset branch unreachable in the normal route path. |
Make the route dependency authenticate the supported session cookie (preferably via the existing centralized verifier and user resolver), or use an auth dependency that accepts the cookie and preserves the same subject/revocation checks. Add an end-to-end cookie-only smoke path. |
|
Good catch again @Lang-Akshay — confirmed and fixed in 52d4052. |
127a3b4 to
26a4875
Compare
Lang-Akshay
left a comment
There was a problem hiding this comment.
Blocking Changes
| # | Area | File | Line | Blocking reason | Required change |
|---|---|---|---|---|---|
| 1 | Security | mcpgateway/routers/auth.py |
576–579 | Refresh generates new_jti but only records old_jti for audit; it never revokes the predecessor. The old bearer/cookie token remains valid and can be replayed to mint more tokens, defeating rotation and logout/revocation expectations (High, CWE-613). |
Atomically revoke/blocklist old_jti using its original expiry and a refresh reason before returning the new token. Make rotation single-use/compare-and-set so concurrent refreshes cannot both succeed; fail closed if predecessor revocation cannot be persisted. |
938669d to
5e806b2
Compare
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
5e806b2 to
879205e
Compare
🔗 Related Issue
Closes #6011
📝 Summary
Adds session-lifecycle endpoints so UI clients can implement silent refresh and idle detection now that
TOKEN_EXPIRYdefaults to 20 minutes:POST /auth/refresh— issues a new session JWT for a valid session. Preserves the old token's scope narrowing, rotates the CSRF token (CSRF tokens are HMAC-bound to the JWTjti, which changes on refresh), and re-sets thejwt_tokencookie when the request authenticated via cookie. Refuses API tokens and any token withouttoken_use="session"with 403 (fail-closed) — long-lived API tokens have their own expiry/revocation lifecycle. Refresh events are audit-logged viaAuditTrailService.GET /auth/validate— reportsvalid,expires_at/expires_in, the user profile,session_source(local | sso | api_token, derived from thetoken_use/auth_provider/sourceclaims), and aconfigblock of session-lifecycle values (normalized to seconds) so clients don't hardcode them.Both endpoints live in
mcpgateway/routers/auth.pyand are exposed at/v1/auth/*(canonical) plus the unversioned legacy shim, same as the sibling/auth/login//auth/logout. The issue's/api/auth/*paths don't exist in the codebase; the implemented paths match the actual v1/legacy mounts and the pre-existing manual test case (AUTH-004) and CSRF config references.Supporting changes:
SESSION_MAX_LIFETIME(default 480 min,0disables): absolute session-age cap enforced at refresh via asession_startclaim carried across refreshes — silent refresh cannot extend a session forever. NoSESSION_IDLE_TIMEOUTwas added: server-side idle enforcement already exists asTOKEN_IDLE_TIMEOUT(revocation-backed, inmcpgateway/auth.py);/auth/validatesurfaces it to clients instead of duplicating the knob.SESSION_WARNING_TIME,SESSION_REFRESH_BUFFER,SESSION_ACTIVITY_TRACKING: client-behavior hints surfaced via/auth/validate.SESSION_REFRESHtier inRateLimitMiddleware(pattern covers both/auth/refreshand/v1/auth/refresh), configured bySESSION_REFRESH_RATE_LIMIT(default 10/min). Reuses the existing Redis-backed, multi-dimensional (IP / user / team) infrastructure rather than adding a bespoke limiter./auth/refreshremoved from defaultCSRF_EXEMPT_PATHS: the entry predated the endpoint; a cookie-authenticated, CSRF-exempt refresh would let a cross-site request silently keep a victim's session alive. Bearer requests skip CSRF by design, so API clients are unaffected.create_access_token()accepts an optionalextra_claimsdict (reserved claims cannot be overridden) for thesession_startcarry-over.reason=token_refreshand its original expiry) before the new token is minted, with compare-and-set semantics so concurrent refreshes cannot both succeed; refresh fails closed (401) if the revocation cannot be persisted.📏 Reviewability
triage🏷️ Type of Change
🧪 Verification
ruff check,isort --check-only,pylint(9.96),bandit(0 issues),interrogate(100%)pytest tests/unit/mcpgateway/routers/ tests/unit/mcpgateway/middleware/test_rate_limit_middleware.py tests/unit/mcpgateway/middleware/test_csrf_middleware.py tests/unit/mcpgateway/test_config.py— 1586 passed, 5 skipped (pre-existing)mypy mcpgateway/routers/auth.py— 4 errors, identical to pre-change baseline (pre-existing debt, none introduced)make lint/make test/make coverageNew test coverage (13 tests in
test_auth.py, 1 intest_rate_limit_middleware.py), including the deny paths: API-token refresh refused (403), missing-token_usetoken refused (403), no session token (401), max-lifetime exceeded (401),session_startcarried over on refresh, SSO / external-IdP / api_tokensession_sourceclassification, cookie-vs-bearer cookie behavior, andSESSION_REFRESHtier matching on both mounts.✅ Checklist
make black isort pre-commit)📓 Notes (optional)
/auth/validatereturns 401 (consistent with the auth dependency) rather than{valid: false}; enforced durations follow the repo's minutes convention (TOKEN_EXPIRY-style) while client hints are in seconds —/auth/validatenormalizes everything to seconds.tests/unit/mcpgateway/middleware/test_rate_limit_middleware.pyincludes isort normalization of pre-existing import-heading debt (comment-only hunks) — the file was non-compliant before this change; touching it triggered the standard formatter cleanup.CSRF_EXEMPT_PATHSexplicitly in.envshould drop/auth/refreshfrom their list to pick up the secure default.