fix(security): enforce management-plane isolation for API tokens, fix team scoping, and fix blocked usage logging#3414
Conversation
079406b to
d1c5d5f
Compare
|
Thanks @shoummu1 — critical security fixes for #3291. The token chaining prevention, team inheritance fix, and audit logging improvements are all important. Well-documented root cause analysis. Make sure deny-path regression tests cover unauthenticated and wrong-team scenarios per project security invariants. |
… and fix blocked/revoked usage logging Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
…s 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>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
2e0a159 to
748fbac
Compare
crivetimihai
left a comment
There was a problem hiding this comment.
Review Summary
Rebased onto main (clean, no conflicts), reviewed all changes, verified all three bugs, and added differential test coverage.
Bug Verification
All three bugs confirmed against main:
- Token Chaining (Privilege Escalation) —
_require_authenticated_session()only blockedNoneand"anonymous", allowing API tokens to manage other tokens. Fix correctly addsapi_tokendenial. - Team Auto-Inheritance —
mainhad no team auto-inheritance; tokens created withoutteam_idgot public-only scope silently. Fix adds auto-inheritance usingtoken_teams(correct key perrbac.py), only for single-team non-admin users. - Blocked Usage Logging —
mainhardcodedblocked=Falseand exited early for revoked tokens. Fix correctly derives blocked status from HTTP status code and adds a 401/403 handler for revoked/expired token logging.
Security
jwt.decode(verify_signature=False)usage is appropriate — identification-only, no auth decisions, wrapped in try/except._require_authenticated_sessionchanges are strictly more restrictive (safe direction).- Auth method values (
api_token,jwt,anonymous) consistent withauth.pyandrbac.py. - No new attack surfaces.
Test Coverage
token_usage_middleware.py: 100% coverage (added 3 edge-case tests for 401/403 paths)tokens.py: 98% (4 uncovered lines are pre-existing, not from this PR)- Added missing
list_team_tokensAPI-token-blocked test - 86 tests pass for changed files; 13,390 unit tests pass full suite
Commits Added During Review
test(security): add differential coverage for 401/403 middleware paths and list_team_tokens— 4 new testsstyle: apply linter formatting to tokens.py— linter auto-format
crivetimihai
left a comment
There was a problem hiding this comment.
Review Summary
Rebased onto main (clean, no conflicts), reviewed all changes, verified all three bugs, and added differential test coverage.
Bug Verification
All three bugs confirmed against main:
- Token Chaining (Privilege Escalation) — _require_authenticated_session() only blocked None and anonymous, allowing API tokens to manage other tokens. Fix correctly adds api_token denial.
- Team Auto-Inheritance — main had no team auto-inheritance; tokens created without team_id got public-only scope silently. Fix adds auto-inheritance using token_teams (correct key per rbac.py), only for single-team non-admin users.
- Blocked Usage Logging — main hardcoded blocked=False and exited early for revoked tokens. Fix correctly derives blocked status from HTTP status code and adds a 401/403 handler for revoked/expired token logging.
Security
- jwt.decode(verify_signature=False) usage is appropriate — identification-only, no auth decisions, wrapped in try/except.
- _require_authenticated_session changes are strictly more restrictive (safe direction).
- Auth method values (api_token, jwt, anonymous) consistent with auth.py and rbac.py.
- No new attack surfaces.
Test Coverage
- token_usage_middleware.py: 100% coverage (added 3 edge-case tests for 401/403 paths)
- tokens.py: 98% (4 uncovered lines are pre-existing, not from this PR)
- Added missing list_team_tokens API-token-blocked test
- 86 tests pass for changed files; 13,390 unit tests pass full suite
Commits Added During Review
- test(security): add differential coverage for 401/403 middleware paths and list_team_tokens — 4 new tests
- style: apply linter formatting to tokens.py — linter auto-format
…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>
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>
…, 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>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Review Commits AddedAfter initial review and approval, discovered and fixed additional security issues during live testing on a 3-worker deployment behind nginx. Commits added during review
Verified on live 3-worker deployment (localhost:8080)
|
…, 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>
…ction, 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>
…ures 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>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
… 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>
… 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> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Bug-fix PR
Summary
Fixes three security bugs in the API token lifecycle: token management endpoints could be accessed using API tokens (privilege escalation), team membership was not auto-inherited when creating tokens (broken scoping), and blocked/revoked token usage was never logged (invisible audit trail).
Related Issue
Closes: #3291
Root Cause
Bug 1 — Token Chaining (Privilege Escalation)
_require_authenticated_session()only blocked anonymous (unauthenticated) requests. It did not checkauth_method, so an API token bearer could callPOST /tokens,GET /tokens,DELETE /tokens/{id}, etc. — allowing an API token to create or revoke other tokens.Bug 2 — Broken Team Inheritance
create_token()attempted to auto-populateteam_idfrom the caller's team membership usingcurrent_user.get("teams"). However,get_current_user_with_permissions()(inrbac.py) injects teams into the user dict under the keytoken_teams— notteams. The lookup always returnedNone, so non-admin users who omittedteam_idalways created globally-scoped tokens regardless of their team membership.Bug 3a —
blockedFlag Always Falsetoken_usage_middleware.pyhardcodedblocked=Falsewhen logging authenticated API token requests, regardless of the actual HTTP response status code.Bug 3b — Revoked/Expired Token Attempts Not Logged
When a revoked or expired API token was rejected (HTTP 401/403), the middleware exited early because
request.state.auth_methodis never set for failed auth. These attempts were silently dropped with no audit trail.Fix Description
mcpgateway/routers/tokens.pyBug 1: Added an explicit
auth_method == "api_token"check inside_require_authenticated_session(), which is called at the top of all 11 token management endpoints:Bug 2: Replaced the broken
.get("teams")lookup with.get("token_teams")and introduced aneffective_team_idvariable with fallback logic for single-team non-admin users. Admin tokens remain globally scoped (team_id=null).mcpgateway/middleware/token_usage_middleware.pyBug 3a: Replaced
blocked=Falsehardcode withblocked = status_code >= 400.Bug 3b: Added a new
elif status_code in (401, 403)branch that usesjwt.decode(..., options={"verify_signature": False})to identify whether the rejected bearer token was an API token, then logs it withblocked=Trueand the appropriateblock_reason("revoked_or_expired"for 401,"http_403"for 403).Verification
curl -H "Authorization: Bearer <api_token>" POST /tokensteam_id; check responseteam_idauto-set to user's teamGET /tokens/{id}/statsblocked_requests > 0total_requests >= 3,blocked_requests >= 3Unit tests:
pytest tests/unit/mcpgateway/routers/test_tokens.py tests/unit/mcpgateway/middleware/test_token_usage_middleware.py→ 100% passFull suite:
pytest tests/unit/→ 694 passed, 15 skipped, 1 pre-existing unrelated failure intest_email_auth_service_admin_role_sync(unrelated to changed files).📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)