fix(security): enforce token_teams narrowing across all Layer 2 RBAC paths by bogdanmariusc10 · Pull Request #3932 · IBM/mcp-context-forge

bogdanmariusc10 · 2026-03-30T15:00:46Z

🔗 Related Issue

📝 Summary

This PR fixes pre-existing security gaps where token_teams (Layer 1 scoping) was either not forwarded or ignored in several RBAC code paths, allowing narrowed or public-only tokens to satisfy permission checks using roles from teams outside their scope.

Key Fixes:

Gap 2: Fixed admin bypass suppression for token_teams=[] by changing from separate if to elif in check_permission() (lines 126-136)
Gap 6: Fixed critical bug where base_condition was unconditionally appended even for token_teams=[], now properly excludes ALL team roles for public-only tokens (lines 528-550)

Already Implemented (Verified):

Gap 1: Explicit team_id validation (lines 502-508)
Gap 3: check_admin_permission accepts token_teams (lines 389-410, rbac.py 784-793)
Gap 4: Token minting forwards token_teams (tokens.py line 96)
Gap 5: Admin UI menu forwards token_teams (admin.py line 510)

Design Decision: Implemented strict isolation where token_teams=[] means public-only access at both Layer 1 (visibility) and Layer 2 (RBAC), providing defense-in-depth security.

🏷️ Type of Change

🧪 Verification

Check	Command	Status
Lint suite	`make lint`	Passed ✅
Unit tests	`make test`	Passed ✅
Coverage ≥ 80%	`make coverage`	Passed ✅

✅ Checklist

Code formatted (make black isort pre-commit)
Tests added/updated for changes
Documentation updated (if applicable)
No secrets or credentials committed

📓 Notes

Test Coverage Added

Added 9 comprehensive regression tests in test_permission_service_token_narrowing.py:

Explicit team_id validation (2 tests):
- test_explicit_team_id_rejects_out_of_scope_team - Verifies rejection when team_id not in token_teams
- test_explicit_team_id_allows_in_scope_team - Verifies access when team_id is in token_teams
Admin bypass behavior (3 tests):
- test_admin_bypass_suppressed_for_public_only_non_admin_perm - Verifies bypass suppression for token_teams=[]
- test_admin_bypass_works_for_narrowed_token - Verifies bypass works for narrowed tokens
- test_admin_bypass_works_for_unnarrowed_token - Verifies bypass works for un-narrowed tokens
Admin permission checks (2 tests):
- test_check_admin_permission_respects_token_teams - Verifies token_teams=[] filters out team roles
- test_check_admin_permission_allows_with_proper_scope - Verifies proper scoping allows access
Team role filtering (2 tests):
- test_public_only_token_excludes_all_team_roles - Verifies token_teams=[] excludes ALL team roles
- test_narrowed_token_includes_specified_teams - Verifies narrowed tokens filter to specified teams

Security Impact

These fixes prevent privilege escalation scenarios where:

A session narrowed to team-A could pass Layer 2 checks for team-B operations
A public-only token (token_teams=[]) could benefit from team-scoped roles or admin bypass
Token minting could derive permissions from out-of-scope teams

Files Modified

mcpgateway/services/permission_service.py - Core RBAC logic fixes
mcpgateway/admin.py - Admin UI menu token scope forwarding (verified)
mcpgateway/middleware/rbac.py - RBAC middleware token scope forwarding (verified)
mcpgateway/routers/tokens.py - Token minting token scope forwarding (verified)
tests/unit/mcpgateway/services/test_permission_service_token_narrowing.py - Comprehensive test coverage
.secrets.baseline - Updated baseline for secret scanning

…paths Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

- PermissionChecker.has_admin_permission: forward token_teams to check_admin_permission (middleware/rbac.py) - has_admin_permission on PermissionService: add token_teams param, suppress admin bypass for public-only tokens (permission_service.py) - AdminAuthMiddleware: forward token_teams to has_admin_permission (main.py) - get_my_permissions endpoint: forward token_teams to get_user_permissions (routers/rbac.py) - _get_user_roles: reject explicit team_id when token_teams=[] (public-only tokens must not access team-specific roles) - Fix stale test asserting token_teams=[] retains team perms (contradicts Option A strict isolation) - Fix stale test asserting team_id ignores token_teams (team_id is now validated against token_teams) - Add regression tests for all new paths Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Update test assertions in test_permission_service.py and test_main_extended.py to include the new token_teams parameter added to has_admin_permission(). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Address all 4 findings from the Codex code review: Finding 1 (HIGH) — _get_caller_permissions admin bypass: - Only treat admin as unrestricted when token_teams is None. Narrowed/public-only admin sessions now derive permissions through the token-aware path (routers/tokens.py). - create_token auto-inheritance now applies to narrowed admins too. Finding 2 (HIGH) — scope_id IS NULL team roles on public-only: - _get_user_roles with team_id=None, include_all_teams=False now excludes scope='team', scope_id=NULL roles when token_teams=[] (permission_service.py). Finding 3 (HIGH) — list_all_tokens/admin_revoke_token admin guard: - Both endpoints now require un-narrowed admin (token_teams is None). Narrowed and public-only admin sessions are rejected with 403 (routers/tokens.py). Finding 4 (LOW) — cache key collision None vs []: - Cache keys now use __public__ sentinel for token_teams=[] and include token_teams in team_id cache keys (permission_service.py). Add 12 regression tests covering all fixed paths. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai · 2026-03-30T19:13:09Z

Review Changes (3 fixup commits)

Rebased cleanly on main. After thorough code review and Code cross-check, I found and fixed the following issues:

Commit `9dc90e9` — Close remaining token_teams gaps in RBAC paths

The original PR missed several call sites that still didn't forward token_teams:

PermissionChecker.has_admin_permission (rbac.py) — was NOT forwarding token_teams to check_admin_permission (both db_session and fresh_db paths).
has_admin_permission on PermissionService (permission_service.py) — didn't accept token_teams at all. Added the parameter, public-only bypass suppression, and forwarding to get_user_permissions.
AdminAuthMiddleware (main.py) — was NOT forwarding token_teams to has_admin_permission. Defense-in-depth fix (early-return already exists for token_teams=[]).
get_my_permissions endpoint (routers/rbac.py) — user's own permission query wasn't narrowed by token_teams.
_get_user_roles team_id validation bug — original condition len(token_teams) > 0 and team_id not in token_teams didn't reject token_teams=[] with explicit team_id. Fixed to len(token_teams) == 0 or team_id not in token_teams.
Fixed two stale tests from PR feat(rbac): enforce session-token team narrowing in Layer 2 permission checks #3919 that contradicted Option A strict isolation.
Added 9 regression tests covering all fixed paths.

Commit `bed00a4` — Update has_admin_permission test assertions

Updated assert_awaited_once_with calls in test_permission_service.py (2 tests) and test_main_extended.py (9 tests) to include the new token_teams parameter.

Commit `a7e248d` — Close remaining gaps from Code review

Addressed all 4 findings from an independent Code code review:

Finding 1 (HIGH) — _get_caller_permissions() returned ["*"] for any admin regardless of token_teams. Narrowed admin sessions could create unrestricted tokens. Fixed: only treat admin as unrestricted when token_teams is None. Also fixed create_token auto-inheritance to apply to narrowed admins.
Finding 2 (HIGH) — _get_user_roles with team_id=None, include_all_teams=False unconditionally included scope='team', scope_id IS NULL roles even for token_teams=[]. Public-only tokens could satisfy admin checks through "all-teams" role assignments. Fixed: excluded when token_teams=[].
Finding 3 (HIGH) — list_all_tokens() and admin_revoke_token() checked raw is_admin without token_teams. Narrowed/public-only admin sessions had full token oversight access. Fixed: require un-narrowed admin (token_teams is None).
Finding 4 (LOW) — Cache key collision between token_teams=None and token_teams=[] (both produced user:__anyteam__). Fixed: added __public__ sentinel and encoded token_teams in all cache key variants.

Added 12 regression tests covering all 4 findings.

Test Results

884 passed, 9 skipped (pre-existing flaky plugin manager tests). All pre-commit hooks pass.

crivetimihai

Approved after thorough review. All Layer 2 RBAC paths now correctly enforce token_teams narrowing. Three fixup commits address gaps found during review and code review cross-check. 884 tests passing.

bogdanmariusc10 added this to the Release 1.0.0 milestone Mar 30, 2026

bogdanmariusc10 requested review from crivetimihai, kevalmahajan and madhav165 as code owners March 30, 2026 15:00

bogdanmariusc10 added security Improves security rbac Role-based Access Control labels Mar 30, 2026

bogdanmariusc10 linked an issue Mar 30, 2026 that may be closed by this pull request

[BUG][SECURITY]: token_teams narrowing not enforced consistently across Layer 2 permission paths #3924

Closed

2 tasks

bogdanmariusc10 added the MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe label Mar 30, 2026

bogdanmariusc10 force-pushed the 3924-bugsecurity-token_teams-narrowing-not-enforced-consistently-across-layer-2-permission-paths branch from d70ff28 to 5867861 Compare March 30, 2026 15:07

fix(security): enforce token_teams narrowing across all Layer 2 RBAC …

768e35e

…paths Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

bogdanmariusc10 force-pushed the 3924-bugsecurity-token_teams-narrowing-not-enforced-consistently-across-layer-2-permission-paths branch from 5867861 to 768e35e Compare March 30, 2026 15:21

crivetimihai added 3 commits March 30, 2026 16:53

fix(tests): update has_admin_permission assertions for token_teams param

bed00a4

Update test assertions in test_permission_service.py and test_main_extended.py to include the new token_teams parameter added to has_admin_permission(). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai approved these changes Mar 30, 2026

View reviewed changes

crivetimihai requested a review from brian-hussey March 30, 2026 19:30

crivetimihai added the release-fix Critical bugfix required for the release label Mar 30, 2026

crivetimihai requested review from cafalchio and dawid-nowak March 30, 2026 19:30

crivetimihai self-assigned this Mar 30, 2026

crivetimihai merged commit 49a1a48 into main Mar 30, 2026
28 checks passed

crivetimihai deleted the 3924-bugsecurity-token_teams-narrowing-not-enforced-consistently-across-layer-2-permission-paths branch March 30, 2026 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): enforce token_teams narrowing across all Layer 2 RBAC paths#3932

fix(security): enforce token_teams narrowing across all Layer 2 RBAC paths#3932
crivetimihai merged 4 commits intomainfrom
3924-bugsecurity-token_teams-narrowing-not-enforced-consistently-across-layer-2-permission-paths

bogdanmariusc10 commented Mar 30, 2026

Uh oh!

crivetimihai commented Mar 30, 2026 •

edited

Loading

Uh oh!

crivetimihai left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bogdanmariusc10 commented Mar 30, 2026

🔗 Related Issue

📝 Summary

🏷️ Type of Change

🧪 Verification

✅ Checklist

📓 Notes

Test Coverage Added

Security Impact

Files Modified

Uh oh!

crivetimihai commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Changes (3 fixup commits)

Commit 9dc90e9 — Close remaining token_teams gaps in RBAC paths

Commit bed00a4 — Update has_admin_permission test assertions

Commit a7e248d — Close remaining gaps from Code review

Test Results

Uh oh!

crivetimihai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

crivetimihai commented Mar 30, 2026 •

edited

Loading

Commit `9dc90e9` — Close remaining token_teams gaps in RBAC paths

Commit `bed00a4` — Update has_admin_permission test assertions

Commit `a7e248d` — Close remaining gaps from Code review

crivetimihai left a comment •

edited

Loading