Skip to content

Commit b1095d1

Browse files
bogdanmariusc10Bogdan-Marius-Catanusja8zyjits
authored
fix(auth): allow cookie auth for same-origin OAuth callback fetch requests (#4868)
* fix(auth): allow cookie auth for same-origin OAuth callback fetch requests The browser detection logic in the auth middleware only recognized /admin referrers as same-origin UI requests. Fetch calls from the OAuth callback page (/oauth/callback) use Accept: application/json, causing the middleware to treat them as external API requests and reject cookie-based authentication with a 401. Extended the same-origin referer check to also match /oauth/callback paths, so tool-fetching after a successful OAuth authorization flow is no longer incorrectly blocked. Fixes #4867 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix .secrets.baseline Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fixed the pre-commit Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> --------- Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> Co-authored-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> Co-authored-by: Jitesh Nair <jiteshnair@ibm.com>
1 parent b944cf0 commit b1095d1

7 files changed

Lines changed: 162 additions & 10 deletions

File tree

.secrets.baseline

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"files": "(?x)( package-lock\\.json$ |Cargo\\.lock$ |uv\\.lock$ |go\\.sum$ |mcpgateway/sri_hashes\\.json$ )|^.secrets.baseline$",
44
"lines": null
55
},
6-
"generated_at": "2026-06-04T10:56:44Z",
6+
"generated_at": "2026-06-04T16:06:29Z",
77
"plugins_used": [
88
{
99
"name": "AWSKeyDetector"

mcpgateway/admin.py

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4805,13 +4805,29 @@ def _build_keycloak_logout_url(root_path: str) -> Optional[str]:
48054805

48064806
# For GET requests, distinguish between browser navigation and OIDC front-channel logout
48074807
if request.method == "GET":
4808-
# Check if request is from a browser (Accept: text/html, HX-Request header, or admin referer)
4808+
# Check if request is from a browser (Accept: text/html, HX-Request header, or same-origin admin/oauth referer)
48094809
# Detection must match auth_middleware.py and rbac.py patterns to ensure consistent behavior
48104810
# Browser navigation should redirect to login, OIDC callbacks should return 200 OK
48114811
accept_header = request.headers.get("accept", "")
48124812
is_htmx = request.headers.get("hx-request") == "true"
48134813
referer = request.headers.get("referer", "")
4814-
is_browser_request = "text/html" in accept_header or is_htmx or "/admin" in referer
4814+
4815+
# Check if referer is from same origin (for admin UI and OAuth callback pages)
4816+
is_same_origin_referer = False
4817+
if referer:
4818+
try:
4819+
# Standard
4820+
from urllib.parse import urlparse
4821+
4822+
referer_parsed = urlparse(referer)
4823+
request_host = request.headers.get("host", "")
4824+
# Match if referer host matches request host and path contains /admin or /oauth/callback
4825+
if referer_parsed.netloc == request_host and ("/admin" in referer_parsed.path or "/oauth/callback" in referer_parsed.path):
4826+
is_same_origin_referer = True
4827+
except Exception:
4828+
pass # Invalid referer URL, treat as not same-origin
4829+
4830+
is_browser_request = "text/html" in accept_header or is_htmx or is_same_origin_referer
48154831

48164832
if is_browser_request:
48174833
# Browser navigation - redirect to login (cookies cleared below)

mcpgateway/middleware/auth_middleware.py

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,11 +271,27 @@ async def dispatch(self, request: Request, call_next: Callable) -> Response:
271271
# without user context so the RBAC layer can redirect to /admin/login.
272272
# API requests: return a hard JSON 401/403 deny.
273273
# Detection must match rbac.py's is_browser_request logic (Accept,
274-
# HX-Request, and Referer: /admin) to avoid breaking admin UI flows.
274+
# HX-Request, and same-origin Referer: /admin or /oauth/callback) to avoid breaking admin UI flows.
275275
accept_header = request.headers.get("accept", "")
276276
is_htmx = request.headers.get("hx-request") == "true"
277277
referer = request.headers.get("referer", "")
278-
is_browser = "text/html" in accept_header or is_htmx or "/admin" in referer
278+
279+
# Check if referer is from same origin (for admin UI and OAuth callback pages)
280+
is_same_origin_referer = False
281+
if referer:
282+
try:
283+
# Standard
284+
from urllib.parse import urlparse
285+
286+
referer_parsed = urlparse(referer)
287+
request_host = request.headers.get("host", "")
288+
# Match if referer host matches request host and path contains /admin or /oauth/callback
289+
if referer_parsed.netloc == request_host and ("/admin" in referer_parsed.path or "/oauth/callback" in referer_parsed.path):
290+
is_same_origin_referer = True
291+
except Exception:
292+
pass # Invalid referer URL, treat as not same-origin
293+
294+
is_browser = "text/html" in accept_header or is_htmx or is_same_origin_referer
279295
if is_browser:
280296
logger.debug("Browser request with rejected auth — continuing without user for redirect")
281297
return await call_next(request)

mcpgateway/middleware/rbac.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,11 +351,26 @@ def _set_trace_context_for_identity(*, email: Optional[str], is_admin: bool, aut
351351
accept_header = request.headers.get("accept", "")
352352
is_htmx = request.headers.get("hx-request") == "true"
353353
referer = request.headers.get("referer", "")
354-
is_admin_ui_request = "/admin" in referer
355-
is_browser_request = "text/html" in accept_header or is_htmx or is_admin_ui_request
354+
355+
# Check if referer is from same origin (for admin UI and OAuth callback pages)
356+
is_same_origin_referer = False
357+
if referer:
358+
try:
359+
# Standard
360+
from urllib.parse import urlparse
361+
362+
referer_parsed = urlparse(referer)
363+
request_host = request.headers.get("host", "")
364+
# Match if referer host matches request host and path contains /admin or /oauth/callback
365+
if referer_parsed.netloc == request_host and ("/admin" in referer_parsed.path or "/oauth/callback" in referer_parsed.path):
366+
is_same_origin_referer = True
367+
except Exception:
368+
pass # Invalid referer URL, treat as not same-origin
369+
370+
is_browser_request = "text/html" in accept_header or is_htmx or is_same_origin_referer
356371

357372
# SECURITY: Reject cookie-only authentication for API requests
358-
# Cookies should only be used for browser/HTML requests (including admin UI fetch calls)
373+
# Cookies should only be used for browser/HTML requests (including admin UI and OAuth callback fetch calls)
359374
if token_from_cookie and not is_browser_request:
360375
raise HTTPException(
361376
status_code=status.HTTP_401_UNAUTHORIZED,

tests/unit/mcpgateway/middleware/test_auth_middleware.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,59 @@ async def test_no_token_continues(monkeypatch):
4848
assert "user" not in request.state.__dict__
4949

5050

51+
@pytest.mark.asyncio
52+
async def test_invalid_referer_url_treated_as_api_request():
53+
"""Invalid referer URL that causes urlparse exception should be treated as not same-origin and trigger hard deny for cookie auth."""
54+
from fastapi import HTTPException
55+
56+
middleware = AuthContextMiddleware(app=AsyncMock())
57+
call_next = AsyncMock(return_value=Response("ok"))
58+
request = MagicMock(spec=Request)
59+
request.url.path = "/api/data"
60+
request.cookies = {"jwt_token": "invalid_token"}
61+
request.headers = {
62+
"accept": "application/json",
63+
"referer": "http://example.com/admin", # Valid URL but will be mocked to raise exception
64+
"host": "localhost:4444"
65+
}
66+
request.client = MagicMock()
67+
request.client.host = "127.0.0.1"
68+
69+
# Mock urlparse to raise an exception to test exception handling (lines 289-290)
70+
# Mock get_current_user to raise a hard-deny exception (simulating revoked token)
71+
with patch("urllib.parse.urlparse", side_effect=ValueError("Invalid URL")):
72+
with patch("mcpgateway.middleware.auth_middleware.get_current_user", side_effect=HTTPException(status_code=401, detail="Token has been revoked")):
73+
response = await middleware.dispatch(request, call_next)
74+
# Should return hard deny (401) because exception means not a browser request
75+
assert response.status_code == 401
76+
assert "Token has been revoked" in response.body.decode()
77+
78+
@pytest.mark.asyncio
79+
async def test_oauth_callback_referer_allows_browser_continuation():
80+
"""OAuth callback referer should be treated as same-origin and allow browser request to continue."""
81+
from fastapi import HTTPException
82+
83+
middleware = AuthContextMiddleware(app=AsyncMock())
84+
call_next = AsyncMock(return_value=Response("ok"))
85+
request = MagicMock(spec=Request)
86+
request.url.path = "/api/data"
87+
request.cookies = {"jwt_token": "invalid_token"}
88+
request.headers = {
89+
"accept": "application/json",
90+
"referer": "http://localhost:4444/oauth/callback?code=abc123",
91+
"host": "localhost:4444"
92+
}
93+
request.client = MagicMock()
94+
request.client.host = "127.0.0.1"
95+
96+
# Mock get_current_user to raise a hard-deny exception (simulating revoked token)
97+
with patch("mcpgateway.middleware.auth_middleware.get_current_user", side_effect=HTTPException(status_code=401, detail="Token has been revoked")):
98+
response = await middleware.dispatch(request, call_next)
99+
# Should continue to call_next (browser request) instead of returning hard deny
100+
call_next.assert_awaited_once_with(request)
101+
assert response.status_code == 200
102+
103+
51104
@pytest.mark.asyncio
52105
async def test_token_from_cookie(monkeypatch):
53106
"""Token extracted from cookie triggers authentication."""

tests/unit/mcpgateway/middleware/test_rbac.py

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ async def test_cookie_auth_allowed_with_admin_referer():
114114
"""/admin referer marks the request as a browser/UI request; cookie auth must be accepted."""
115115
mock_request = MagicMock(spec=Request)
116116
mock_request.cookies = {"jwt_token": "token123"}
117-
mock_request.headers = {"accept": "application/json", "referer": "http://localhost:4444/admin#gateways"}
117+
mock_request.headers = {"accept": "application/json", "referer": "http://localhost:4444/admin#gateways", "host": "localhost:4444"}
118118
mock_request.client = MagicMock()
119119
mock_request.client.host = "127.0.0.1"
120120
mock_request.state = MagicMock(auth_method="jwt", request_id="req-admin", token_teams=["team-1"])
@@ -141,6 +141,34 @@ async def test_cookie_auth_allowed_with_accept_text_html():
141141
assert result["email"] == "user@example.com"
142142

143143

144+
@pytest.mark.asyncio
145+
async def test_cookie_auth_allowed_with_oauth_callback_referer():
146+
"""OAuth callback referer with Accept: application/json must be treated as a browser request.
147+
148+
This test covers the scenario where the OAuth callback success page makes a fetch
149+
request to /oauth/fetch-tools with Accept: application/json. The referer header
150+
indicates it's from the OAuth callback page, so cookie authentication should be allowed.
151+
152+
Regression test for issue where OAuth tool fetching failed after PR #2680 added
153+
cookie authentication restrictions for API requests.
154+
"""
155+
mock_request = MagicMock(spec=Request)
156+
mock_request.cookies = {"jwt_token": "token123"}
157+
mock_request.headers = {
158+
"accept": "application/json",
159+
"referer": "http://localhost:4444/oauth/callback?code=abc&state=xyz",
160+
"host": "localhost:4444"
161+
}
162+
mock_request.client = MagicMock()
163+
mock_request.client.host = "127.0.0.1"
164+
mock_request.state = MagicMock(auth_method="jwt", request_id="req-oauth-fetch", token_teams=["team-1"])
165+
166+
mock_user = MagicMock(email="user@example.com", full_name="User", is_admin=False)
167+
with patch("mcpgateway.auth.validate_token_user", return_value=mock_user):
168+
result = await rbac.get_current_user_with_permissions(mock_request, credentials=None, jwt_token="token123")
169+
assert result["email"] == "user@example.com"
170+
171+
144172
@pytest.mark.asyncio
145173
async def test_cookie_auth_rejected_with_cross_origin_oauth_referer():
146174
"""Cross-origin /oauth/ referer without browser headers must NOT grant cookie auth."""
@@ -156,6 +184,30 @@ async def test_cookie_auth_rejected_with_cross_origin_oauth_referer():
156184
assert exc.value.status_code == status.HTTP_401_UNAUTHORIZED
157185
assert "Cookie authentication not allowed" in exc.value.detail
158186

187+
@pytest.mark.asyncio
188+
async def test_cookie_auth_rejected_with_invalid_referer_url():
189+
"""Invalid referer URL that causes urlparse exception should be treated as not same-origin and reject cookie auth."""
190+
from unittest.mock import patch
191+
192+
mock_request = MagicMock(spec=Request)
193+
mock_request.cookies = {"jwt_token": "token123"}
194+
mock_request.headers = {
195+
"accept": "application/json",
196+
"referer": "http://example.com/admin", # Valid URL but will be mocked to raise exception
197+
"host": "localhost:4444"
198+
}
199+
mock_request.client = MagicMock()
200+
mock_request.client.host = "127.0.0.1"
201+
mock_request.state = MagicMock(auth_method="jwt", request_id="req-invalid")
202+
203+
# Mock urlparse to raise an exception to test exception handling
204+
with patch("urllib.parse.urlparse", side_effect=ValueError("Invalid URL")):
205+
with pytest.raises(HTTPException) as exc:
206+
await rbac.get_current_user_with_permissions(mock_request, credentials=None, jwt_token=None)
207+
assert exc.value.status_code == status.HTTP_401_UNAUTHORIZED
208+
assert "Cookie authentication not allowed" in exc.value.detail
209+
210+
159211

160212
@pytest.mark.asyncio
161213
async def test_cookie_auth_rejected_with_unrelated_referer():

tests/unit/mcpgateway/test_admin_module.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ async def test_admin_logout_paths():
507507
# GET request with admin referer should redirect to login
508508
get_referer_request = _make_request(root_path="/root")
509509
get_referer_request.method = "GET"
510-
get_referer_request.headers = {"accept": "application/json", "referer": "http://localhost:4444/admin/users"}
510+
get_referer_request.headers = {"accept": "application/json", "referer": "http://localhost:4444/admin/users", "host": "localhost:4444"}
511511
response = await admin._admin_logout(get_referer_request)
512512
assert isinstance(response, RedirectResponse)
513513
assert response.status_code == 303

0 commit comments

Comments
 (0)