fix: redirect to login page on manual admin/logout call in the browser

[marekdano]

Closes: #3555

🐛 Bug-fix PR

📌 Summary

Fixed admin logout behavior to redirect browser users to the login page while maintaining OIDC front-channel logout compliance.

Root causes

User Experience: Users manually navigating to /admin/logout in browser are now automatically redirected to login page

🔁 Reproduction Steps

1. Log in to the app
2. Manually enter admin/logout into browser
3. Page will clear and show 'Logged out' message

💡 Fix Description

mcpgateway/admin.py

• Added browser detection in _admin_logout() using the Accept header
• GET requests with Accept: text/html (browser navigation) now redirect to /admin/login (303)
• GET requests without Accept: text/html (OIDC front-channel callbacks) return 200 OK per OIDC spec
• POST requests continue to redirect to /admin/login after clearing cookies

tests/unit/mcpgateway/test_admin_module.py

• Enhanced test_admin_logout_paths() to cover all three logout scenarios:
• POST request → 303 redirect to login
• GET with Accept: text/html (browser) → 303 redirect to login
• GET without Accept: text/html (OIDC) → 200 OK

🧪 Verification

Check	Command	Status
Lint suite	make lint	✅
Unit tests	make test	✅
Coverage ≥ 80 %	make coverage	✅
Manual regression no longer fails	steps / screenshots

📐 MCP Compliance (if relevant)

• Matches current MCP spec
• No breaking change to MCP clients

✅ Checklist

• Code formatted (make black isort pre-commit)
• No secrets/credentials committed

[gcgoncalves]

🍭

[msureshkumar88]

PR Review Report — PR #3564: Redirect manually accessed admin/logout to admin/login page

Author: @marekdano

Branch: 3555-redirect-to-login-from-manual-logout → main
By BOB

---

Summary

This PR addresses issue #3555 by implementing logic to redirect browser users who manually navigate to /admin/logout to the login page, while preserving OIDC front-channel logout functionality. The implementation uses the Accept header to distinguish between browser navigation (which should redirect) and OIDC IdP callbacks (which should return 200 OK per spec).

Changes: 2 files modified (1 source file, 1 test file)
Risk Level: Low-Medium (authentication flow changes require careful validation)

---

Linked Issues — Completion Status

Issue	Title	Status	Notes
#3555	[BUG][UI]: Redirect manually accessed admin/logout to admin/login page	✅ Done	Core requirement met: browser navigation to /admin/logout now redirects to login

Overall Issue Coverage: Primary requirement addressed. However, see security findings below regarding cookie clearing for OIDC callbacks.

---

Security Findings

Overview

No critical security vulnerabilities identified. One medium-severity functional issue found related to OIDC front-channel logout cookie handling. The implementation correctly distinguishes between browser requests and OIDC callbacks, but may not fully comply with OIDC front-channel logout specification regarding session clearing.

Findings Summary

#	File	Line	Severity	Description
1	mcpgateway/admin.py	4276	Medium	OIDC front-channel logout may not clear cookies properly

Suggestions

1. Finding 1 — Move cookie clearing logic (lines 4289-4299) outside the if/else block so it executes for all logout requests, including OIDC front-channel callbacks. According to the OIDC front-channel logout spec, the application should clear the session even for IdP-initiated logouts. Effort: Low (simple code restructuring)

Invariant Compliance

All ContextForge security invariants satisfied. The implementation maintains proper authentication flow and doesn't introduce privilege escalation risks.

Deny-Path Coverage

No security-sensitive changes requiring deny-path regression tests identified.

---

General PR Quality

Check	Status	Notes
Tests added/updated	✅	Comprehensive test coverage added for all three logout scenarios
No hardcoded secrets	✅	No credentials in code
No debug code	✅	Clean implementation
Docs/comments updated	⚠️	Docstring needs update to reflect new GET behavior
Migration present (if needed)	N/A	No database changes
Coding standards met	✅	Follows project conventions

---

Files Changed

File	Status	Summary
mcpgateway/admin.py	modified	Added Accept header detection to distinguish browser navigation from OIDC callbacks
tests/unit/mcpgateway/test_admin_module.py	modified	Added tests for browser GET, OIDC GET, and POST logout scenarios

---

Code Review Findings

5 issues identified (1 medium, 4 low severity) - All added to Bob Findings panel:

1. Medium: OIDC front-channel logout may not clear cookies properly (admin.py:4276)
2. Low: Browser detection inconsistent with existing patterns (admin.py:4269)
3. Low: Docstring not updated to reflect new behavior (admin.py:4117)
4. Low: Comment could reference OIDC spec (admin.py:4276)
5. Low: Test coverage missing edge cases (test_admin_module.py:482)

---

Overall Verdict

Dimension	Rating
Issue Completion	🟢 Complete
Security Risk	🟡 Medium
PR Quality	🟡 Needs Work

Recommendation: ⚠️ Approve with fixes

The PR successfully addresses the reported issue (#3555) by redirecting browser users from /admin/logout to /admin/login. The implementation is sound and includes good test coverage. However, there's a medium-severity issue where OIDC front-channel logout callbacks may not properly clear session cookies, which could leave stale sessions. Additionally, the browser detection logic should be aligned with existing patterns in auth_middleware.py and rbac.py to handle HTMX requests and admin referer checks consistently.

Recommended Actions:

1. Restructure cookie clearing to execute for all logout types (including OIDC callbacks)
2. Align browser detection with existing middleware patterns
3. Update docstring to document the three logout scenarios
4. Add edge case tests for Accept header variations

[msureshkumar88]

✅ PR #3564 Review Update — All Recommended Changes Implemented

Reviewed by: @msureshkumar88
Status: All 4 recommended actions have been successfully implemented

---

Changes Verification Summary

✅ 1. Cookie Clearing for All Logout Types (Medium Priority)

Status: IMPLEMENTED

The cookie clearing logic (lines 4297-4308) is now outside the if/else block, ensuring that session cookies are cleared for all logout scenarios including:

• POST requests (user-initiated)
• GET requests with browser headers (manual navigation)
• GET requests without browser headers (OIDC front-channel callbacks)

This fixes the medium-severity issue where OIDC front-channel logout callbacks were not properly clearing session cookies.

✅ 2. Browser Detection Aligned with Middleware Patterns

Status: IMPLEMENTED

The browser detection logic (lines 4269-4275) now matches the patterns used in auth_middleware.py:188 and rbac.py:264:

accept_header = request.headers.get("accept", "")
is_htmx = request.headers.get("hx-request") == "true"
referer = request.headers.get("referer", "")
is_browser_request = "text/html" in accept_header or is_htmx or "/admin" in referer

This ensures consistent behavior across the codebase for detecting browser vs API requests.

✅ 3. Docstring Updated for Three Logout Scenarios

Status: IMPLEMENTED

The function docstring (lines 4117-4125) now clearly documents all three logout scenarios:

• POST: User-initiated logout from UI
• GET with browser headers: Browser navigation to /admin/logout
• GET without browser headers: OIDC front-channel logout callback

The docstring also includes a reference to the OpenID Connect Front-Channel Logout 1.0 specification.

✅ 4. Edge Case Test Coverage Added

Status: IMPLEMENTED

Comprehensive test coverage has been added (lines 499-523) for edge cases:

• ✅ HX-Request header detection (HTMX requests)
• ✅ Admin referer detection
• ✅ Wildcard Accept header (*/*)
• ✅ Multiple Accept types (text/html,application/xhtml+xml)

---

Additional Improvements Implemented

Beyond the recommended changes, the following enhancements were also made:

1. OIDC Spec Reference Added (lines 4281-4284)

   • Added explicit reference to OpenID Connect Front-Channel Logout 1.0 spec
   • Includes URL: https://openid.net/specs/openid-connect-frontchannel-1_0.html

2. Improved Code Comments (lines 4269-4271)

   • Added comment explaining the need for consistency with auth_middleware.py and rbac.py
   • Clarified the distinction between browser navigation and OIDC callbacks

---

Final Verdict

Dimension	Previous Rating	Current Rating
Issue Completion	🟢 Complete	🟢 Complete
Security Risk	🟡 Medium	🟢 Low
PR Quality	🟡 Needs Work	🟢 Good

Updated Recommendation: ✅ APPROVE

All identified issues have been resolved:

• ✅ Cookie clearing now works for all logout types (including OIDC callbacks)
• ✅ Browser detection is consistent with existing middleware patterns
• ✅ Documentation is complete and accurate
• ✅ Comprehensive test coverage for edge cases

The PR is now ready for merge. The implementation correctly addresses issue #3555 while maintaining OIDC front-channel logout compliance and ensuring consistent behavior across the authentication system.

[crivetimihai]

Rebased onto main (clean, no conflicts) and reviewed.

Design & Logic — Sound. The fix correctly distinguishes three logout scenarios:

1. Browser GET (Accept: text/html / HX-Request / admin Referer) → redirect to login (303)
2. OIDC front-channel GET (no browser headers) → 200 OK per OpenID Connect Front-Channel Logout 1.0 spec
3. POST (UI button) → redirect to login (or Keycloak RP-initiated logout)

Consistency — The browser detection pattern ("text/html" in accept_header or is_htmx or "/admin" in referer) exactly matches the established pattern in middleware/auth_middleware.py and middleware/rbac.py.

Security — No issues. Redirect target is hardcoded ({root_path}/admin/login), cookies are always cleared regardless of path, no open redirect or header injection vectors, */* accept header correctly falls through to OIDC path.

Test coverage — All new code paths are covered (6 test cases: POST redirect, browser GET redirect, OIDC GET 200, HTMX GET redirect, referer GET redirect, wildcard accept GET 200). All 59 admin tests pass.

Docs — SSO tutorial update accurately describes the three scenarios.

LGTM.