Skip to content

Commit a589de3

Browse files
fix(security): enforce management-plane isolation for API tokens, fix team scoping, and fix blocked usage logging (#3414)
* fix: block API tokens from token management, fix team_id inheritance, and fix blocked/revoked usage logging Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * update test coverage Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * test(security): add differential coverage for 401/403 middleware paths and list_team_tokens Add 4 tests covering uncovered edge-case paths in the token usage middleware's 401/403 handler (no Bearer header, non-API-token JWT, malformed token) and the missing list_team_tokens API-token-blocked test, achieving 100% differential coverage on new code. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * style: apply linter formatting to tokens.py Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(security): enforce token revocation by propagating 401 from auth middleware Three root causes allowed revoked API tokens to bypass rejection: 1. AuthContextMiddleware caught ALL exceptions from get_current_user() (including HTTPException 401 for revoked tokens) and silently continued as anonymous, letting the request reach public endpoints. Fix: Re-raise 401/403 HTTPExceptions as hard deny responses instead of swallowing them. 2. The fallback revocation check in auth.py silently swallowed _check_token_revoked_sync errors (e.g. missing table, DB unreachable) and allowed the token through. Fix: Fail-secure — reject the token when the revocation check itself fails. 3. Cache invalidation on revoke used asyncio.create_task (fire-and-forget), creating a race window where the next request could arrive before the invalidation task ran. Fix: Await invalidate_revocation() directly in both revoke_token() and admin_revoke_token(). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(security): cross-worker revocation via Redis revoke key check In a multi-worker deployment (gunicorn behind nginx), revoking a token only updated the local worker's _revoked_jtis set. Other workers served stale L1 cache entries with is_token_revoked=False until the 30-second TTL expired. Fix: In get_auth_context(), check the Redis revocation marker key (mcpgw:auth:revoke:{jti}) BEFORE the L1 in-memory cache. When found, promote the JTI to the local _revoked_jtis set and evict stale L1 entries, so subsequent requests on this worker skip the Redis call. One Redis EXISTS per request with a JTI — sub-millisecond overhead. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(security): validate JTI before logging, preserve browser redirect, scope blocked to 4xx Address three review findings from Codex: 1. Forged JWT audit-log poisoning: the 401/403 rejected-token logging path decoded unverified JWTs and wrote jti/sub claims directly to token_usage_logs. An attacker could craft a JWT with fake identity claims to pollute audit data. Fix: verify the JTI exists in the email_api_tokens table before logging. 2. Browser redirect broken: the hard-deny JSONResponse for 401/403 in AuthContextMiddleware also applied to browser/HTMX requests with stale cookies, returning raw JSON instead of letting the RBAC layer redirect to /admin/login. Fix: detect browser requests (Accept: text/html or HX-Request) and let them pass through without user context. 3. 5xx folded into blocked metrics: status_code >= 400 included backend errors (5xx) in the blocked count, skewing security denial analytics. Fix: scope to 400 <= status_code < 500. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * style: fix import ordering in token_usage_middleware Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(security): achieve 100% diff coverage for auth middleware, cache, and usage logging Add 11 tests covering all uncovered lines from diff-cover: - auth_middleware.py: HTTPException 401/403 hard deny for API requests, browser/HTMX passthrough for redirect, failure logging with DB errors, DB close errors, and non-401/403 HTTPException anonymous fallthrough - auth_cache.py: Redis revocation marker detection with L1 eviction and local set promotion, Redis error fallthrough to L1/L2 - token_usage_middleware.py: forged JWT with unknown JTI skipped, DB error during JTI verification skipped Coverage: auth_middleware.py 100%, auth_cache.py 100%, token_usage_middleware.py 100% Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(security): use DB-stored email for usage logs, align browser detection, add security headers Address three findings from final review: 1. Forged user attribution: the rejected-token logging path used the unverified JWT sub/email claim for usage logs. An attacker knowing a valid JTI could forge a JWT with an arbitrary email to poison another user's blocked-request stats. Fix: query EmailApiToken for both id and user_email by JTI, use the DB-stored owner email. 2. Referer-based browser detection gap: AuthContextMiddleware only checked Accept and HX-Request headers for browser detection, but rbac.py also treats Referer containing /admin as browser traffic. Admin UI fetch requests with Accept: */* and an /admin Referer got a JSON 401 instead of passing through for redirect. Fix: include /admin Referer check to match RBAC behavior. 3. Missing security headers on JSON 401/403: responses returned directly from AuthContextMiddleware bypassed SecurityHeadersMiddleware. Fix: add X-Content-Type-Options: nosniff and Referrer-Policy headers to the JSONResponse. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(auth): only hard-deny revocation/disabled 401s, not all auth failures The auth middleware was hard-denying ALL 401/403 HTTPExceptions, which broke registration scripts and other callers using minimal JWT claims (no user/teams/token_use). These previously fell through to route-level auth handlers. Narrow the hard-deny to only security-critical rejections: "Token has been revoked", "Account disabled", and "Token validation failed". All other 401/403s continue as anonymous for route-level handling. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * embeeded auth Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * pylint Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Shoumi <shoumimukherjee@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
1 parent c6c2ad0 commit a589de3

File tree

12 files changed

+1056
-201
lines changed

12 files changed

+1056
-201
lines changed

docker-compose-embedded.yml

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ services:
7373
- AUTH_REQUIRED=true
7474
- EMAIL_AUTH_ENABLED=true
7575
- MCP_CLIENT_AUTH_ENABLED=true
76-
- JWT_SECRET_KEY=jwt-secret-key
76+
- JWT_SECRET_KEY=${JWT_SECRET_KEY:-my-test-key}
7777
- PLATFORM_ADMIN_EMAIL=admin@example.com
7878
- PLATFORM_ADMIN_PASSWORD=Changeme123!
7979
- PLATFORM_ADMIN_FULL_NAME=Platform Administrator
@@ -149,7 +149,7 @@ services:
149149
register_benchmark:
150150
image: ghcr.io/ibm/mcp-context-forge:1ba8130f7fb82e6f393435be8d064879f234ace1
151151
environment:
152-
- JWT_SECRET_KEY=jwt-secret-key
152+
- JWT_SECRET_KEY=${JWT_SECRET_KEY:-my-test-key}
153153
- BENCHMARK_SERVER_COUNT=${BENCHMARK_SERVER_COUNT:-10}
154154
- BENCHMARK_START_PORT=${BENCHMARK_START_PORT:-9000}
155155
command:
@@ -209,11 +209,3 @@ services:
209209
print(f'Registration complete: {registered}/{server_count} benchmark servers')
210210
"
211211
212-
# NOTE: register_fast_time uses hardcoded secret in its command script.
213-
# It will fail with 401 when JWT_SECRET_KEY differs from base compose.
214-
# This is non-critical — benchmark servers provide the bulk test data.
215-
# To also register fast_time, manually run:
216-
# curl -X POST http://localhost:8080/gateways \
217-
# -H "Authorization: Bearer $(python3 -m mcpgateway.utils.create_jwt_token --username admin@example.com --exp 10080 --secret jwt-secret-key --algo HS256)" \
218-
# -H "Content-Type: application/json" \
219-
# -d '{"name":"fast_time","url":"http://fast_time_server:8080/http","transport":"STREAMABLEHTTP"}'

mcpgateway/auth.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1286,8 +1286,15 @@ async def _set_auth_method_from_payload(payload: dict) -> None:
12861286
except HTTPException:
12871287
raise
12881288
except Exception as revoke_check_error:
1289-
# Log the error but don't fail authentication for admin tokens
1290-
logger.warning(f"Token revocation check failed for JTI {jti}: {revoke_check_error}")
1289+
# Fail-secure: if the revocation check itself errors, reject the token.
1290+
# Allowing through on error would let revoked tokens bypass enforcement
1291+
# when the DB is unreachable or the table is missing.
1292+
logger.warning(f"Token revocation check failed for JTI {jti} — denying access (fail-secure): {revoke_check_error}")
1293+
raise HTTPException(
1294+
status_code=status.HTTP_401_UNAUTHORIZED,
1295+
detail="Token validation failed",
1296+
headers={"WWW-Authenticate": "Bearer"},
1297+
)
12911298

12921299
# Resolve teams based on token_use
12931300
token_use = payload.get("token_use")

mcpgateway/cache/auth_cache.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,26 @@ async def get_auth_context(
266266
self._hit_count += 1
267267
return CachedAuthContext(is_token_revoked=True)
268268

269+
# Cross-worker revocation check: when a token is revoked on another worker,
270+
# the revoking worker writes a Redis revocation marker. Check it BEFORE the
271+
# L1 in-memory cache so that stale L1 entries cannot bypass revocation.
272+
if jti:
273+
redis = await self._get_redis_client()
274+
if redis:
275+
try:
276+
revoke_key = self._get_redis_key("revoke", jti)
277+
if await redis.exists(revoke_key):
278+
# Promote to local set so subsequent requests skip the Redis call
279+
with self._lock:
280+
self._revoked_jtis.add(jti)
281+
# Evict any stale L1 context entries for this JTI
282+
for k in [k for k in self._context_cache if k.endswith(f":{jti}")]:
283+
self._context_cache.pop(k, None)
284+
self._hit_count += 1
285+
return CachedAuthContext(is_token_revoked=True)
286+
except Exception as exc:
287+
logger.debug(f"AuthCache: Redis revocation check failed for {jti[:8]}: {exc}")
288+
269289
cache_key = f"{email}:{jti or 'no-jti'}"
270290

271291
# Check L1 in-memory cache first (no network I/O)

mcpgateway/middleware/auth_middleware.py

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,11 @@
2020
from typing import Callable
2121

2222
# Third-Party
23+
from fastapi import HTTPException
2324
from fastapi.security import HTTPAuthorizationCredentials
2425
from starlette.middleware.base import BaseHTTPMiddleware
2526
from starlette.requests import Request
26-
from starlette.responses import Response
27+
from starlette.responses import JSONResponse, Response
2728

2829
# First-Party
2930
from mcpgateway.auth import get_current_user
@@ -35,6 +36,12 @@
3536
logger = logging.getLogger(__name__)
3637
security_logger = get_security_logger()
3738

39+
# HTTPException detail strings that indicate security-critical rejections
40+
# (revoked tokens, disabled accounts, fail-secure validation errors).
41+
# Only these trigger a hard JSON deny in the auth middleware; all other
42+
# 401/403s fall through to route-level auth for backwards compatibility.
43+
_HARD_DENY_DETAILS = frozenset({"Token has been revoked", "Account disabled", "Token validation failed"})
44+
3845

3946
def _should_log_auth_success() -> bool:
4047
"""Check if successful authentication should be logged based on settings.
@@ -144,8 +151,60 @@ async def dispatch(self, request: Request, call_next: Callable) -> Response:
144151
except Exception as close_error:
145152
logger.debug(f"Failed to close database session: {close_error}")
146153

154+
except HTTPException as e:
155+
if e.status_code in (401, 403) and e.detail in _HARD_DENY_DETAILS:
156+
logger.info(f"✗ Auth rejected ({e.status_code}): {e.detail}")
157+
158+
if log_failure:
159+
db = SessionLocal()
160+
try:
161+
security_logger.log_authentication_attempt(
162+
user_id="unknown",
163+
user_email=None,
164+
auth_method="bearer_token",
165+
success=False,
166+
client_ip=request.client.host if request.client else "unknown",
167+
user_agent=request.headers.get("user-agent"),
168+
failure_reason=str(e.detail),
169+
db=db,
170+
)
171+
db.commit()
172+
except Exception as log_error:
173+
logger.debug(f"Failed to log auth failure: {log_error}")
174+
finally:
175+
try:
176+
db.close()
177+
except Exception as close_error:
178+
logger.debug(f"Failed to close database session: {close_error}")
179+
180+
# Browser/admin requests with stale cookies: let the request continue
181+
# without user context so the RBAC layer can redirect to /admin/login.
182+
# API requests: return a hard JSON 401/403 deny.
183+
# Detection must match rbac.py's is_browser_request logic (Accept,
184+
# HX-Request, and Referer: /admin) to avoid breaking admin UI flows.
185+
accept_header = request.headers.get("accept", "")
186+
is_htmx = request.headers.get("hx-request") == "true"
187+
referer = request.headers.get("referer", "")
188+
is_browser = "text/html" in accept_header or is_htmx or "/admin" in referer
189+
if is_browser:
190+
logger.debug("Browser request with rejected auth — continuing without user for redirect")
191+
return await call_next(request)
192+
193+
# Include essential security headers since this response bypasses
194+
# SecurityHeadersMiddleware (it returns before call_next).
195+
resp_headers = dict(e.headers) if e.headers else {}
196+
resp_headers.setdefault("X-Content-Type-Options", "nosniff")
197+
resp_headers.setdefault("Referrer-Policy", "strict-origin-when-cross-origin")
198+
return JSONResponse(
199+
status_code=e.status_code,
200+
content={"detail": e.detail},
201+
headers=resp_headers,
202+
)
203+
204+
# Non-security HTTP errors (e.g. 500 from a downstream service) — continue as anonymous
205+
logger.info(f"✗ Auth context extraction failed (continuing as anonymous): {e}")
147206
except Exception as e:
148-
# Silently fail - let route handlers enforce auth if needed
207+
# Non-HTTP errors (network, decode, etc.) — continue as anonymous
149208
logger.info(f"✗ Auth context extraction failed (continuing as anonymous): {e}")
150209

151210
# Log failed authentication attempt (based on logging level)

mcpgateway/middleware/token_usage_middleware.py

Lines changed: 94 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
# Standard
2222
import logging
2323
import time
24+
from typing import Optional
2425

2526
# Third-Party
27+
import jwt as _jwt
2628
from starlette.datastructures import Headers
2729
from starlette.requests import Request
2830
from starlette.types import ASGIApp, Receive, Scope, Send
@@ -108,62 +110,120 @@ async def send_wrapper(message: dict) -> None:
108110
# Calculate response time
109111
response_time_ms = round((time.time() - start_time) * 1000)
110112

111-
# Only log if this was an API token request
113+
# Log API token usage — covers both successful requests and auth-rejected attempts.
114+
# Every request that uses (or tries to use) an API token is recorded,
115+
# including blocked calls with revoked/expired tokens, so that usage stats are accurate.
112116
state = scope.get("state", {})
113117
auth_method = state.get("auth_method") if state else None
114118

115-
if auth_method != "api_token":
116-
return
119+
jti: Optional[str] = None
120+
user_email: Optional[str] = None
121+
blocked: bool = False
122+
block_reason: Optional[str] = None
123+
124+
if auth_method == "api_token":
125+
# --- Successfully authenticated API token request ---
126+
jti = state.get("jti") if state else None
127+
user = state.get("user") if state else None
128+
user_email = getattr(user, "email", None) if user else None
129+
if not user_email:
130+
user_email = state.get("user_email") if state else None
131+
132+
# If we don't have JTI or email, try to decode the token from the header
133+
if not jti or not user_email:
134+
try:
135+
headers = Headers(scope=scope)
136+
auth_header = headers.get("authorization")
137+
if not auth_header or not auth_header.startswith("Bearer "):
138+
return
139+
token = auth_header.replace("Bearer ", "")
140+
request = Request(scope, receive)
141+
try:
142+
payload = await verify_jwt_token_cached(token, request)
143+
jti = jti or payload.get("jti")
144+
user_email = user_email or payload.get("sub") or payload.get("email")
145+
except Exception as decode_error:
146+
logger.debug(f"Failed to decode token for usage logging: {decode_error}")
147+
return
148+
except Exception as e:
149+
logger.debug(f"Error extracting token information: {e}")
150+
return
117151

118-
# Extract token information from scope state
119-
jti = state.get("jti") if state else None
120-
user = state.get("user") if state else None
121-
user_email = getattr(user, "email", None) if user else None
122-
if not user_email:
123-
user_email = state.get("user_email") if state else None
152+
if not jti or not user_email:
153+
logger.debug("Missing JTI or user_email for token usage logging")
154+
return
124155

125-
# If we don't have JTI or email, try to decode the token
126-
if not jti or not user_email:
156+
# Bug 3a fix: reflect the actual outcome — 4xx responses mark the attempt
157+
# as blocked (e.g. RBAC denied, rate-limited, or server-scoping violation).
158+
# 5xx errors are backend failures, not security denials, so exclude them.
159+
blocked = 400 <= status_code < 500
160+
if blocked:
161+
block_reason = f"http_{status_code}"
162+
163+
elif status_code in (401, 403):
164+
# --- Auth-rejected request: check if the Bearer token was an API token ---
165+
# When a revoked or expired API token is used, auth middleware rejects the
166+
# request before setting auth_method="api_token", so the path above is
167+
# never reached. We detect the attempt here by decoding the JWT payload
168+
# without re-verifying it (the token identity is valid even if rejected).
127169
try:
128-
# Get token from Authorization header
129170
headers = Headers(scope=scope)
130171
auth_header = headers.get("authorization")
131172
if not auth_header or not auth_header.startswith("Bearer "):
132173
return
174+
raw_token = auth_header[7:] # strip "Bearer "
133175

134-
token = auth_header.replace("Bearer ", "")
176+
# Decode without signature/expiry check — for identification only, not auth.
177+
unverified = _jwt.decode(raw_token, options={"verify_signature": False})
178+
user_info = unverified.get("user", {})
179+
if user_info.get("auth_provider") != "api_token":
180+
return # Not an API token — nothing to log
135181

136-
# Decode token to get JTI and user email
137-
# Note: We need to create a minimal Request-like object
138-
request = Request(scope, receive)
139-
try:
140-
payload = await verify_jwt_token_cached(token, request)
141-
jti = jti or payload.get("jti")
142-
user_email = user_email or payload.get("sub") or payload.get("email")
143-
except Exception as decode_error:
144-
logger.debug(f"Failed to decode token for usage logging: {decode_error}")
182+
jti = unverified.get("jti")
183+
user_email = unverified.get("sub") or unverified.get("email")
184+
if not jti or not user_email:
145185
return
186+
187+
# Verify JTI belongs to a real API token before logging.
188+
# Without this check, an attacker can craft a JWT with fake
189+
# jti/sub and auth_provider=api_token to pollute usage logs.
190+
# Verify JTI belongs to a real API token and use the DB-stored
191+
# owner email instead of the unverified JWT claim. Without this,
192+
# an attacker who knows a valid JTI could forge a JWT with an
193+
# arbitrary sub/email to poison another user's usage stats.
194+
try:
195+
# Third-Party
196+
from sqlalchemy import select # pylint: disable=import-outside-toplevel
197+
198+
# First-Party
199+
from mcpgateway.db import EmailApiToken # pylint: disable=import-outside-toplevel
200+
201+
with fresh_db_session() as verify_db:
202+
token_row = verify_db.execute(select(EmailApiToken.id, EmailApiToken.user_email).where(EmailApiToken.jti == jti)).first()
203+
if token_row is None:
204+
return # JTI not in DB — forged token, skip logging
205+
# Use the DB-stored owner, not the unverified JWT claim
206+
user_email = token_row.user_email
207+
except Exception:
208+
return # DB error — skip logging rather than log unverified data
209+
210+
blocked = True
211+
block_reason = "revoked_or_expired" if status_code == 401 else f"http_{status_code}"
146212
except Exception as e:
147-
logger.debug(f"Error extracting token information: {e}")
213+
logger.debug(f"Failed to extract API token identity from rejected request: {e}")
148214
return
215+
else:
216+
return # Not an API token request — nothing to log
149217

150-
if not jti or not user_email:
151-
logger.debug("Missing JTI or user_email for token usage logging")
152-
return
153-
154-
# Log token usage
218+
# Shared logging path for both authenticated and blocked API token requests
155219
try:
156220
with fresh_db_session() as db:
157221
token_service = TokenCatalogService(db)
158-
# Get client IP
159222
client = scope.get("client")
160223
ip_address = client[0] if client else None
161-
162-
# Get user agent
163224
headers = Headers(scope=scope)
164225
user_agent = headers.get("user-agent")
165226

166-
# Log usage
167227
await token_service.log_token_usage(
168228
jti=jti,
169229
user_email=user_email,
@@ -173,8 +233,8 @@ async def send_wrapper(message: dict) -> None:
173233
user_agent=user_agent,
174234
status_code=status_code,
175235
response_time_ms=response_time_ms,
176-
blocked=False,
177-
block_reason=None,
236+
blocked=blocked,
237+
block_reason=block_reason,
178238
)
179239
except Exception as e:
180240
logger.debug(f"Failed to log token usage: {e}")

0 commit comments

Comments
 (0)