feat: API endpoint is served under the /v1 prefix#4403
feat: API endpoint is served under the /v1 prefix#4403Lang-Akshay wants to merge 28 commits intomainfrom
/v1 prefix#4403Conversation
c8e7d17 to
77af9dd
Compare
cfeb057 to
d190e69
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Request Changes - PR #4403
Summary
This PR implements API versioning under /v1 prefix with excellent architecture and comprehensive test coverage. However, there are 1 CRITICAL and 2 MEDIUM priority issues that must be addressed before merging.
🚨 CRITICAL Issues (Must Fix)
1. Breaking Change Without Backward Compatibility
Location: mcpgateway/main.py:11544
Problem: This PR introduces an immediate breaking change for all external API consumers. Any client using unversioned endpoints (e.g., /tools, /servers, /gateways) will break immediately upon deployment.
Required Action: Implement a backward compatibility layer with deprecation warnings:
# In main.py after line 11544
# Mount v1_router at both /v1 (new) and root (deprecated)
app.include_router(v1_router) # /v1/* (new canonical path)
# Temporary compatibility layer - remove in v2.0.0
app.include_router(v1_router, prefix="") # /* (deprecated, for backward compatibility)
# Add deprecation warning middleware for unversioned paths
# Log warnings when clients use deprecated unversioned endpointsAdditional Requirements:
- Add
MIGRATION.mddocumenting the breaking change and migration path - Update
CHANGELOG.mdwith breaking change notice - Add deprecation timeline (e.g., "unversioned endpoints deprecated, will be removed in v2.0.0")
- Ensure all security middleware applies to both
/v1/*and/*paths
⚠️ MEDIUM Issues (Should Fix)
2. Duplicated Path Normalization Logic
Location: mcpgateway/middleware/token_scoping.py:243-250 and 345-352
Problem: Path normalization logic for stripping /v1 prefix is duplicated in two functions (_normalize_llm_api_prefix() and _normalize_path_for_matching()), creating maintenance burden and potential inconsistencies.
Required Action: Consolidate into a single shared utility function:
def _normalize_api_path(path: str) -> str:
"""Strip /v1 prefix for consistent pattern matching across middleware."""
if path.startswith("/v1/"):
return path[3:]
if path == "/v1":
return "/"
return path
# Then update both functions to use this shared utility3. Missing API Version Discovery Endpoint
Location: mcpgateway/api/v1/__init__.py:273
Problem: No endpoint exists for clients to programmatically discover the API version, making it difficult for clients to adapt to versioning changes.
Required Action: Add version discovery endpoint:
# In build_v1_router() after creating v1_router (line 73)
@v1_router.get("/version", tags=["Version"])
async def get_api_version():
"""Return current API version information."""
return {
"version": "v1",
"deprecated": False,
"deprecation_date": None,
"sunset_date": None
}📋 Additional Recommendations (Optional)
Test Coverage Gap
Add explicit test verifying unversioned routes remain unversioned:
def test_unversioned_routes_remain_unversioned():
"""Verify RFC well-known, OAuth, health remain at root."""
assert client.get("/.well-known/openid-configuration").status_code != 404
assert client.get("/oauth/authorize").status_code != 404
assert client.get("/health").status_code == 200Documentation
- Update OpenAPI schema to reflect
/v1prefix in all endpoint paths - Add API versioning policy to project documentation
✅ What's Working Well
- Excellent centralized router architecture
- Comprehensive test coverage (194 files updated)
- Proper security middleware updates
- Clean separation of versioned vs unversioned routes
- No security regressions identified
🎯 Merge Criteria
Cannot merge until:
- ✅ Backward compatibility layer implemented
- ✅ Migration documentation added (
MIGRATION.md,CHANGELOG.md) - ✅ Path normalization logic consolidated
- ✅ Version discovery endpoint added
Estimated effort: 2-4 hours to address all issues
Review Status: 🔴 REQUEST CHANGES
Risk Level: 🟡 MEDIUM (Technical: LOW, Business: MEDIUM due to breaking change)
msureshkumar88
left a comment
There was a problem hiding this comment.
As mentioned in the comments
cae920f to
027090f
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Code Review — PR #4403: feat: API endpoint is served under the /v1 prefix
Verdict: REQUEST CHANGES — 4 HIGH + 7 MEDIUM issues require resolution before merge.
What the PR Does
Centralizes all resource-management routes under a /v1 URL prefix via a new build_v1_router() factory in mcpgateway/api/v1/__init__.py. Protocol-level routes (MCP transport, OAuth, health, RFC well-known) correctly remain unversioned. The factory pattern is clean, documentation is thorough, and test coverage of the factory itself is good.
HIGH Severity — Request Changes
H1 — LLM Proxy Routing Collision
Files: main.py, api/v1/__init__.py
When LLM_API_PREFIX="/v1" (the default), the LLM proxy router is mounted on app at /v1 — the same prefix as the gateway's own v1_router. FastAPI silently registers both. Catch-all proxy routes can shadow gateway API endpoints. No runtime warning exists for this conflict. The MIGRATION.md acknowledges the tension but provides no guardrail.
Fix required: Add a startup CRITICAL log (or assertion) when llm_api_prefix.rstrip("/") == "/v1":
if settings.llmchat_enabled and settings.llm_api_prefix.rstrip("/") == "/v1":
logger.critical(
"LLM_API_PREFIX='/v1' conflicts with the gateway API prefix. "
"Set LLM_API_PREFIX to a different value (e.g. '/llm/v1') to avoid routing collisions."
)H2 — metrics_maintenance_router Double-Mounted
Files: main.py (direct on app) + api/v1/__init__.py (inside v1_router)
Both mounts use the same flag and log the identical string "Metrics maintenance router included". FastAPI silently registers duplicate routes, polluting OpenAPI docs and creating maintenance confusion.
Fix required: Mount only once. MIGRATION.md says it should stay at /api/metrics/** — remove it from v1_router.
H3 — Hard 404 Cutover With No Deprecation Path
All ~25 previously unversioned paths immediately return 404 post-upgrade. Zero-downtime upgrades are not possible. External clients, Kubernetes health scripts, and SDKs not in this repo will break silently.
Fix required: Either add temporary 301/308 redirect aliases for one release cycle, or explicitly document in MIGRATION.md that a coordinated cutover (e.g., blue-green deployment) is required and that zero-downtime upgrades are not supported.
H4 — Well-Known Router Double-Mounted, Violating RFC 8615
File: api/v1/__init__.py Group F
well_known_router is mounted at root (correct) and inside v1_router when mcpgateway_admin_api_enabled=True, creating routes at /v1/.well-known/**. RFC 8615 mandates well-known URIs are only served at /.well-known/. Serving them additionally under /v1/ is a spec violation.
Fix required: Extract only the admin status endpoint into a dedicated router rather than re-mounting the entire well_known_router inside v1.
MEDIUM Severity — Request Changes
M1 — _strip_v1 Closure Allocated on Every Admin Request (Performance)
File: main.py — AdminAuthMiddleware.dispatch()
async def dispatch(self, request, call_next):
def _strip_v1(p: str) -> str: # ← new closure object on EVERY request
return p[3:] if p.startswith("/v1") else pAlso, _strip_v1(p) is called for every item in EXEMPT_PATHS on every request, recomputing static values. Move to a class-level static method and pre-compute the stripped exempt paths as a class attribute.
M2 — EXEMPT_PATHS Inconsistency: /admin/static Not Versioned
File: main.py — AdminAuthMiddleware
EXEMPT_PATHS = [
"/v1/admin/login",
"/v1/admin/logout",
"/v1/admin/reset-password",
"/admin/static", # ← NOT versioned; all others have /v1/
]The _strip_v1 normalization makes this work at runtime, but intent is unclear and will confuse future maintainers. Add an explicit comment or make it consistent.
M3 — Service Layer Hardcodes /v1/ Path Segments (Separation of Concerns)
Files: email_auth_service.py, export_service.py
Password-reset email links and exported server endpoint URLs now hardcode /v1/:
return f"{app_domain}{root_path}/v1/admin/forgot-password"
"sse_endpoint": f"{root_path}/v1/servers/{server.id}/sse"These URLs are sent externally (emails, config exports). A future version bump requires hunting through service-layer business logic. Use settings.api_version_prefix (a new config field, default "/v1") to centralize this.
M4 — Open Redirect Risk in root_path-Based Location Headers (Security Hardening)
Files: middleware/rbac.py, routers/sso.py, admin.py
headers={"Location": f"{settings.app_root_path}/v1/admin/login"}
RedirectResponse(url=f"{root_path}/v1/admin/login?error={error_code}")If app_root_path is derived from X-Forwarded-Prefix without strict validation, an attacker can craft a request causing a redirect to an attacker-controlled URL (CWE-601). Validate app_root_path at startup with a strict allow-list regex (path-only, no scheme/host).
M5 — Plugin Server Prometheus Path Change Breaks External Scrapers (Observability)
File: plugins/framework/external/mcp/server/runtime.py
# Before
Route("/metrics/prometheus", metrics_endpoint)
# After
Route("/v1/metrics/prometheus", metrics_endpoint)The plugin server is a standalone Starlette app, entirely separate from the main gateway. Its internal routes should not inherit the gateway's API versioning prefix. Any Prometheus scrape_configs targeting /metrics/prometheus will immediately receive 404 post-upgrade. This looks like unintentional scope creep — the versioning prefix should not bleed into plugin server internals.
Fix required: Revert this change in runtime.py.
M6 — Legacy /admin Branch in AdminAuthMiddleware is Dead Code
File: main.py
is_admin_route = scope_path.startswith("/admin") or scope_path.startswith("/v1/admin")Admin routes are now only registered at /v1/admin/*. The startswith("/admin") branch protects paths that no longer exist. Dead code in security middleware is a maintenance liability. Remove it or add a dated comment for an explicit transition period.
M7 — scripts/update_test_paths.py is Dead After This PR
Complex regex-based one-time migration script with no unit tests. It has already been executed — the test files are migrated. Either delete it in this PR or add tests in tests/unit/scripts/ if it will be reused. Scripts like this have historically introduced silent bugs (over-matching paths).
Testing Gaps
| Gap | Severity |
|---|---|
No integration tests: real TestClient requests against /v1/* routes |
HIGH |
No test for LLM proxy collision when llm_api_prefix="/v1" |
HIGH |
| No regression tests asserting old unversioned paths now return 404 | MEDIUM |
_normalize_path_for_matching edge cases untested: bare /v1, /v1/v1/, /prefix/v1/tools |
MEDIUM |
| Plugin server Prometheus path change not verified by any E2E test | LOW |
Logging Gaps
| Location | Gap |
|---|---|
| Metrics maintenance double-mount | Both log identical string — indistinguishable in output |
api/v1/__init__.py Group F |
Admin router unavailable logged at ERROR; should be CRITICAL |
main.py LLM proxy |
No warning when llm_api_prefix conflicts with /v1 gateway prefix |
api/v1/__init__.py Group B |
tool_plugin_bindings import failure logged at ERROR but silently skipped — inconsistent severity |
Security Summary
| ID | Severity | Issue | CWE |
|---|---|---|---|
| H4 | HIGH | Well-known router at /v1/.well-known/** violates RFC 8615 |
CWE-706 |
| H1 | HIGH | LLM proxy route collision at /v1 default prefix |
CWE-610 |
| M4 | MEDIUM | Open redirect via unvalidated root_path in Location headers |
CWE-601 |
| M6 | MEDIUM | Dead admin middleware path match — operational confusion | CWE-670 |
| M5 | MEDIUM | Plugin server Prometheus path breaks external monitoring | — |
Alternative Implementations Worth Considering
-
Deprecation-period aliases — Mount routes at both
/v1/*(new) and/*(old, withDeprecation: true+Sunset: <date>response headers). Remove old aliases in the next release. Enables zero-downtime migration for existing clients. -
Sub-application mounting —
app.mount("/v1", v1_app)wherev1_appis a separate FastAPI instance. Each version gets its own middleware stack and/v1/docs. Clean isolation; future/v2is another sub-app. -
Settings-driven version prefix — Replace all hardcoded
/v1strings withsettings.api_version_prefix = "/v1". Service-layer URLs, factory prefix, cookie paths, and redirect URLs all read from one field. Future version bumps become a single config change, not a repo-wide grep-and-replace.
- Updated all instances of /v1/servers/{id}/mcp to /servers/{id}/mcp in test cases.
- Ensured consistency across tests for authentication and authorization checks.
- Adjusted related test assertions to reflect the new endpoint structure.
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
- Updated various service and router files to remove the /v1 prefix from API paths. - Adjusted tests to reflect the new endpoint structure without versioning. - Ensured that all references to admin routes are updated to include the new path format. - Modified the metrics endpoint to return data without the version prefix. - Cleaned up related tests to ensure they align with the new routing structure. Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…baseline refactor: reorder logging import in alembic env.py fix: add comment for first-party import in audit_trail_service.py cleanup: remove merge conflict markers in resource_service.py refactor: add first-party comment in context.py refactor: add blank line after import in streamablehttp_transport.py docs: enhance API endpoint organization documentation in plan.md chmod: update permissions for update_test_paths.py script Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…API paths to include versioning in tests Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…rage - Implement tests for legacy /admin/* paths to ensure they trigger admin auth checks. - Ensure /admin/login remains exempt from authentication. - Add tests for browser and HTMX request redirects when unauthenticated admin is enabled. - Introduce tests for token scoping and cancellation router functionalities. - Enhance audit trail service tests to handle identity extraction failures gracefully. - Add email auth service tests for forgot and reset password URL constructions. - Introduce comprehensive tests for the v1 API router, covering various feature flags and import errors. - Remove obsolete cancellation router tests to streamline the codebase. Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…aseline; add noqa comment for import in main.py Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…ioning Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…fix in tests Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
- Changed all admin-related fetch and form action URLs in observability templates to include the new versioning scheme (v1). - Updated paths in performance, prompts, resources, tools, and user management templates to reflect the new API structure. - Adjusted Playwright tests to align with the updated URL structure for admin routes. Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…ion in logout tests Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…files Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
- Added configuration options for enabling legacy API routes and setting a sunset date. - Implemented middleware to inject deprecation headers for legacy routes. - Updated main application to conditionally mount legacy routes based on configuration. - Created integration tests to verify behavior of legacy and v1 routes, including header presence and content parity. - Added unit tests for deprecation middleware and legacy router functionality. - Adjusted root path redirection to point to /v1/admin/ when UI is enabled. Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…nd validation scripts Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…ormat Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…ts, and utility functions Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…omments in framework files Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…regex Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
…n documentation and scripts Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
|
…alidations Signed-off-by: Lang-Akshay <akshay.shinde26@ibm.com>
msureshkumar88
left a comment
There was a problem hiding this comment.
Follow-up Review — PR #4403 (Final Sign-off)
All three remaining blockers have been verified in the current branch head (f19f6605b).
✅ All Blockers Resolved
B1 — well_known_router double-mount
include_router(well_known_router) is absent from _assemble_routers(). The RFC-violating /v1/.well-known/** routes no longer exist. The admin-status handler is correctly registered via a targeted add_api_route("/admin/well-known", get_well_known_status, ...) — only the relevant handler is versioned, not the entire well-known router. ✅
B2 — logger.critical for LLM_API_PREFIX='/v1' collision
Confirmed present at main.py:11696 (verified in prior round). ✅
B3 — Plugin server Prometheus path
runtime.py lines 349, 351, 421, 423 all register Route("/metrics/prometheus", ...). The stale /v1/metrics/prometheus entry is gone from the exempt-path set at line 181. Startup log at line 562 correctly announces /metrics/prometheus. ✅
B4 — Open redirect via app_root_path (CWE-601)
config.py lines 391–412 add validate_app_root_path as a Pydantic field_validator. Values containing :// or starting with // raise ValueError at startup. Misconfiguration is caught at boot rather than exploited at runtime. ✅
Merge Readiness
No blocking issues remain. The architecture is sound — the build_v1_router() / build_legacy_router() factory pattern via shared _assemble_routers() is clean, DeprecationHeadersMiddleware is correctly pure-ASGI, the parity test guards against _LEGACY_PREFIXES drift, and protocol-level routes (MCP, OAuth, health, well-known) are correctly unversioned.
The post-merge items from the prior round (deprecated=True on legacy routes, _normalize_path_for_matching edge case tests, CLAUDE.md/AGENTS.md path updates, settings.api_version_prefix centralization, hardcoded /v1/ in service layer) remain good follow-up issues but are not blocking.
Approving. Ready to merge once #3754 merge dependency is clear.
🎉 E2E Test Suite Results - PR #4403 API VersioningExecutive SummaryComprehensive end-to-end testing completed with 100% pass rate for all PR-related functionality. All critical blocking issues resolved, versioned endpoints working correctly, and backward compatibility maintained. 📊 Test ResultsTest Score: 43/44 tests passing (98% success rate)
✅ Critical Blocking Issues - RESOLVED
✅ Versioned Endpoints (/v1/*) - 100% SUCCESSAll versioned endpoints working correctly:
✅ Legacy Routes - 100% SUCCESSBackward compatibility maintained:
✅ Deprecation Headers - FULLY IMPLEMENTEDAll RFC 8594 compliant headers present:
✅ Authentication & AuthorizationUnauthenticated Access:
Authenticated Access (9 tests):
✅ OpenAPI Specification
✅ Path Normalization & Edge Cases
✅ Unversioned Endpoints StabilityInfrastructure endpoints remain unversioned (correct):
📁 Test DeliverablesCreated comprehensive test suite in repository:
🎯 Final Verdict✅ PR #4403 READY FOR MERGE All core API versioning functionality validated:
Success Rate: 100% for all PR-related functionality Tested on: Gateway running on localhost:8000 |
Summary
This PR introduces API versioning for ContextForge by serving all resource management and business-logic endpoints under a
/v1URL prefix. A newmcpgateway/api/v1/__init__.pymodule centralizes router assembly through abuild_v1_router()factory, keepingmain.pyclean and making future version additions (/v2, etc.) straightforward.Protocol-level routes (MCP transports, OAuth, well-known URIs, health probes) intentionally remain unversioned at the root — these follow external standards or must stay stable for infrastructure compatibility.
Changes
mcpgateway/api/v1/__init__.py— newbuild_v1_router()andbuild_legacy_router()factories;_assemble_routers()is the single source of truth for router assembly (feature-flag logic shared between both mounts)mcpgateway/api/__init__.py— new namespace packagemcpgateway/main.py— router registration refactored; versioned routers delegated tobuild_v1_router(); legacy shim conditionally mounted viabuild_legacy_router()whenLEGACY_API_ENABLED=true; unversioned routers mounted directly onappmcpgateway/admin.py— admin redirect paths updated to/v1/admin/*mcpgateway/middleware/deprecation.py— newDeprecationHeadersMiddlewarethat injectsDeprecation: true,Sunset, andLinkheaders on all legacy (unversioned) route responsesmcpgateway/middleware/— path references updated (path_filter.py,rbac.py,token_scoping.py)/v1prefixed pathsscripts/update_test_paths.py— utility script for migrating test path referencesEndpoint Classification
Versioned — served under
/v1/v1/protocol/**/v1/tools/**/v1/resources/**/v1/prompts/**/v1/gateways/**/v1/roots/**/v1/servers/**/v1/metrics/**/v1/tags/**/v1/export,/v1/import/v1/tools/plugin_bindings/**/v1/admin/**MCPGATEWAY_ADMIN_API_ENABLED/v1/admin/runtime/**MCPGATEWAY_ADMIN_API_ENABLED/v1/admin/llm/**MCPGATEWAY_LLMCHAT_ENABLED/v1/a2a/**MCPGATEWAY_A2A_ENABLED/v1/observability/**OBSERVABILITY_ENABLED/v1/reverse-proxy/**MCPGATEWAY_REVERSE_PROXY_ENABLED/v1/cancellation/**MCPGATEWAY_TOOL_CANCELLATION_ENABLED/v1/toolops/**TOOLOPS_ENABLED/v1/auth/**EMAIL_AUTH_ENABLED/v1/auth/email/**EMAIL_AUTH_ENABLED/v1/auth/sso/**EMAIL_AUTH_ENABLED+SSO_ENABLED/v1/teams/**EMAIL_AUTH_ENABLED/v1/tokens/**EMAIL_AUTH_ENABLED/v1/rbac/**EMAIL_AUTH_ENABLED/v1/llmchat/**MCPGATEWAY_LLMCHAT_ENABLED/v1/llm/**MCPGATEWAY_LLMCHAT_ENABLEDNot versioned — mounted at root
/health,/ready,/health/security/mcp/_internal/mcp/transport/oauth/**/.well-known/**/servers/{id}/.well-known/**/version/static/**//favicon.ico/api/logs/**{llm_api_prefix}(default/v1)LLM_API_PREFIX; cannot be nested inside the gateway's own/v1Breaking Change
All previously unversioned API paths (
/tools,/resources,/prompts,/gateways,/servers,/roots,/metrics,/tags,/a2a,/admin,/auth,/teams,/tokens,/rbac,/observability,/cancellation,/toolops,/reverse-proxy,/export,/import) are now canonically served under/v1. Clients should migrate base URLs to/v1.Backward compatibility is on by default. When
LEGACY_API_ENABLED=true(the default), all old unversioned paths remain functional alongside/v1— responses includeDeprecation: true,Sunset, andLinkheaders per RFC 8594 to signal the migration window. SetLEGACY_API_ENABLED=falseto drop the shim routes entirely once all clients have migrated.The MCP transports (
/mcp), OAuth endpoints (/oauth), health probes, and well-known URIs are unchanged.Legacy route configuration