Repository navigation
feat: accept server_id on tool preview - #7249
Conversation
msureshkumar88
left a comment
There was a problem hiding this comment.
Thank you for the PR. The code change is small, correct, and backend-only. It adds the missing server_id wiring that #6743 describes, and it needs no Alembic migration. The integration tests pass locally (31 passed with --with-integration).
No finding meets a blocking criterion. I request changes on the PR text, the docs, and the test coverage of the security-sensitive branches.
Required before merge
functionally-impacting | PR title and body | The title and body say this PR implements the endpoint, the ToolService preview logic, and preview_safe hook filtering. Those exist already (#5629). The diff does not touch tool_service.py. The body says the route requires tools.execute. It requires tools.preview. | Retitle to feat: accept server_id on tool preview (#6743). Describe the three real changes: the schema field, the route wiring, and the server visibility check.
functionally-impacting | PR body, "Closes" section | ## Closes followed by #6743 on the next line does not link the issue. GitHub reports no closing reference. | Write Closes #6743 on one line.
functionally-impacting | docs/docs/manage/api-usage.md (Preview a Tool Call) | The section does not document server_id, the new status codes, or an example. CHANGELOG.md has no entry. | Document the field and add an example. List 403 (token scope), 404 (unknown or hidden server), 404 (tool not attached), and 400 (ambiguous upstream name). Add a CHANGELOG line. State that an older gateway ignores server_id and previews globally.
functionally-impacting | tests/integration/test_tool_preview.py:363-411 | The server visibility deny paths mock ensure_server_access. No real-database test covers a nonexistent server, another user's private server, or a wrong-team server. These are the security-sensitive branches. | Add three real-database tests that expect the same generic 404. Follow TestToolPreviewWrongTeamIntegration.
functionally-impacting | tests/integration/test_tool_preview.py:336-387 | The 403 test patches extract_token_scopes_from_request. TokenScopingMiddleware._check_server_restriction default-denies /tools/preview for every server-scoped token, so a real server-scoped token returns 403 in the middleware and never reaches the new handler branch. The test describes behavior that real traffic cannot produce. | Add one middleware-level test that pins the 403 for a server-scoped token. State the limit in the docs. A server-scoped token cannot preview its own server today.
Should fix
suggestion | mcpgateway/main.py:5901-5905 | extract_token_scopes_from_request re-verifies the JWT and returns None on any exception. validate_server_access(None, ...) then returns True, so a failure grants access (CWE-755). /rpc reads the payload that auth already cached in request.state._jwt_verified_payload. This is the only route-level caller of the helper. | Read the cached payload. Move the "scope check, then ensure_server_access" sequence into one helper that /rpc and preview share. Deny when a token is present and its scopes cannot be read.
suggestion | mcpgateway/schemas.py:2175,2190-2207 | server_id has no length cap. A 100 000-character value passes validation, reaches a SQL bind parameter, and returns in the 403 and 404 detail strings (CWE-770). Server.id is String(36). The validator strips whitespace, but /rpc does not. The isinstance(v, str) test is dead code. | Use Annotated[str, StringConstraints(strip_whitespace=True, min_length=1, max_length=36)] and delete the validator.
suggestion | tests/integration/test_tool_preview.py:733-747 | The duplicate-name test asserts 400, then two 200 responses. It does not prove that each qualified name resolved to a different gateway. | Assert target.gatewayName equals preview-source-one and preview-source-two.
suggestion | tests/unit/mcpgateway/test_main.py:1515-1552 | No handler test covers server_id. Integration tests skip unless --with-integration is set, so make coverage diff-cover may miss the new main.py lines. | Add two handler-level cases for the 403 and 404 branches.
Optional
tests/unit/mcpgateway/test_tool_preview_schema.py: 168 lines for one field. Several tests restate Pydantic behavior. Collapse to a short parametrized test.tests/live_gateway/: AGENTS.md asks for a black-box test. Add one in this PR or a follow-up.- Squash the two
docs:follow-up commits. Restore the(#5629)reference removed from thepreview_tooldocstring. - Replace the repeated inline
db_mod.Tool(...)blocks with a factory helper.
Compatibility
The change is additive. Clients that already send server_id (ignored today because of extra="ignore") now get a scoped result, 403, 404, or 422. A new client on an older gateway gets a silent global preview. This is the divergence #6743 targets. Document the minimum gateway version.
Notes
.secrets.baselinechange is expected (line shift inschemas.py).- The server visibility check is necessary. The resolver checks tool access and tool-to-server membership, but not server visibility.
|
Updated:
Deferred as outside this PR’s scope:
Changelog updates happen at release time. All 80 focused tests pass. Broader suites pass separately; the combined run has three plugin-state isolation failures |
7f3ef7a to
3d2ffb7
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
The change delivers #6743 and stays additive. The optional server_id, the ensure_server_access preflight, and the real-DB deny-path tests (missing, private, wrong-team) are solid. The docs section is accurate. No Alembic or schema change is needed. Reviewed at 3d2ffb70b.
Two items must change before merge.
functionally-impacting | mcpgateway/main.py:5907 | `payload is None and (token or cached is not None)` returns 403 "Unable to read verified token scopes" for a valid request under proxy auth. | Fail closed only when `cached is not None and payload is None`; drop the `token or` clause.
functionally-impacting | tests/live_gateway/ | No black-box test against a running gateway, and no test compares preview with live `tools/call` for the same name and `server_id` (issue criterion 3). | Add one live-gateway test: two same-name tools on one server, qualified name resolves to the same tool in preview and in `/rpc tools/call`.
suggestion | mcpgateway/main.py:5902 | Third inline copy of the token-scope extraction (`main.py:508`, `11342`). The new copy fails closed; the others do not. | Extract one helper with a strict flag, or record a follow-up issue.
suggestion | tests/integration/test_tool_preview.py:544 | About eight near-identical `db_mod.Tool(...)` blocks. | Generalize `_make_team_tool` into one `_make_tool(...)` factory.
suggestion | tests/unit/mcpgateway/test_tool_preview_schema.py | `test_model_dump`, `test_json_serialization`, and similar tests exercise Pydantic, not this change. | Parametrize the `server_id` validation cases and drop the rest.
minor | mcpgateway/main.py:5871 | The docstring drops `(#5629)`. | Restore it or leave it unchanged.
minor | CHANGELOG.md | No entry. | Add one if the project lists API additions there.
minor | branch | Four commits behind `main`. | Rebase before merge.
Finding 1 detail. The failure needs MCP_CLIENT_AUTH_ENABLED=false, TRUST_PROXY_AUTH=true, and the dangerous-trust flag. The request carries X-Authenticated-User plus Authorization: Bearer <opaque IdP token>. Nothing sets _jwt_verified_payload on that path (rbac.py:277-285), so the same call succeeds without server_id and returns 403 with it. Add a unit test in test_tool_preview_handler.py for this case.
Why the scope block stays: TokenScopingMiddleware registers only when EMAIL_AUTH_ENABLED=true (main.py:3330). With it off, this block is the only server-scope guard. A server-scoped token for the wrong server gets 403, and the token's own server passes.
Two observations for context. The flaky wrong-team-/tools case failed once in my first full run and did not recur in later runs, so I cannot name a cause. The .secrets.baseline change is ignored.
|
Addressed both required items:
77 focused tests and the live test pass. Shared refactoring and optional cleanup remain deferred as outside this PR’s scope. |
msureshkumar88
left a comment
There was a problem hiding this comment.
Thank you for the follow-up. Both required items from the previous review are resolved. Reviewed at 47d4aff1f.
Resolved
mcpgateway/main.py:5905: the handler now fails closed only when a cached payload exists but is unreadable. A trusted-proxy request with an opaque bearer token now reachesensure_server_access. Every JWT path (verify_credentials_cached, external IdP, proxy auth) sets_jwt_verified_payload, so dropping thetoken orclause does not reopen a fail-open path.test_preview_handler_preserves_trusted_proxy_authpins this case through the realget_current_user_with_permissions.tests/live_gateway/e2e/test_tool_preview_parity.py: the test runs an isolated gateway with two real upstream echo servers. It proves the bare name returns 400, each qualified name resolves to the same source in preview and in/rpc tools/call, and preview runs zero upstream executions. This covers issue criterion 3.
Verification
- Handler, schema, middleware, and integration tests: 73 passed (
--with-integration). tests/unit/mcpgateway/test_main.py -k preview: 5 passed.- Live parity test: passed.
ruff checkon changed Python files: clean.
Before merge
minor | .secrets.baseline | The branch conflicts with main in this file only. | Rebase and take the main version, then regenerate with make detect-secrets-scan.
The deferred suggestions (shared scope-extraction helper, _make_tool factory, schema test trim) remain optional.
3340c0f
47d4aff to
3340c0f
Compare
3340c0f to
72a9288
Compare
57c602f
57c602f to
45d17b0
Compare
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
45d17b0 to
131cd63
Compare
Summary
server_idtoToolPreviewRequestso callers can preview a tool through a virtual-server scope.server_idto the existing preview service so only tools attached to that server are considered.server_idis omitted ornull.Note: This PR extends the existing tool preview endpoint. It does not add the endpoint, preview execution logic, or
preview_safehook filtering.Explanatory Diagrams
Tool Preview Before vs After This PR
Compatibility
server_id, or withserver_id: null, continue to preview globally.tools.previewpermission.403from token-scoping middleware on this REST route, including when the request names the token's own server. Changing that middleware behavior is outside this PR.Manual Verification
Verified with frontend main and backend PR #7249.
The run uses the existing SQLite
mcp.dband real backend APIs. Browser Preview and Live invocation are performed manually through the BFF.The test uses a temporary Python environment with the backend's lockfile dependencies. The steps support either that environment or an equivalent
.venv.Setup and test steps
Before starting
Use frontend main, the backend PR branch, and dependencies installed from their lockfiles. Use your existing admin account.
Commands default to the backend's
.venv. For another environment, exportPR7249_PYTHONwith its Python path in Terminals A and D.Replace the paths below. Keep ports
8000,5173,3000, and9010free. Run each service in its own terminal; keep Terminal D open for the helper commands.Use a fresh state directory for each run. Keep
PR7249_STATE_DIRset in Terminal D for every helper command.1. Start backend (Terminal A)
This uses your existing
.envandmcp.db. If startup rejects placeholder secrets, first run"$PR7249_PYTHON" -m mcpgateway.scripts.init_secrets --patch-env .env.2. Start frontend (Terminal B)
cd /path/to/contextforge-web-ui HUSKY=0 npm ci HUSKY=0 npm ci --prefix server npm run generate npm run dev -- --host 127.0.0.1 --port 5173 --strictPort3. Start BFF (Terminal C)
cd /path/to/contextforge-web-ui CONTEXTFORGE_URL=http://127.0.0.1:8000 COOKIE_SECURE=false HOST=127.0.0.1 PORT=3000 REDIS_URL=memory:// SSO_ENABLED=false PUBLIC_ORIGIN=http://localhost:3000 VITE_DEV_SERVER_URL=http://127.0.0.1:5173 npm run dev --prefix serverKeep Terminals A-C running. Open
http://localhost:3000and sign in.4. Create test fixtures (Terminal D)
Put the script from the next collapsible in the backend repo root as
pr7249-manual-test.py. Keep it untracked.Enter your admin password. For another account, append
--email your-admin@example.comtosetup.Expect one MCP echo source and two virtual servers:
pr7249-attached-...has echo;pr7249-empty-...does not. Upstream executions starts at0.5. Check API preview
Expect attached preview
200, unattached preview404, and execution count0 -> 0.6. Test browser Preview
Open Virtual Servers -> pr7249-attached-... -> View details -> Components -> Echo actions -> Test.
Enter
hello, leave Live invocation off, and click Preview. Expect Preview 200,validated: true, and the expected source. Check that the request snippet includes this server'sserver_id.Expect Upstream executions: 0.
7. Test Live invocation
Turn Live invocation on. Keep
message=helloand click Invoke tool once. Expect Live invoke 200, the attached server in Requested through, andhellofrom the same source as Preview.Expect Upstream executions: 1 and Message: hello.
8. Cleanup
Keep the backend running:
This removes this run's fixtures and stops its echo server. State and logs remain. Use a fresh state directory next time. Stop Terminals A-C with Ctrl-C when finished.
Supporting local helper script
Save this as
pr7249-manual-test.pyin the backend root. Keep it untracked.The helper creates fixtures and checks Preview through backend APIs. Perform browser Preview and Live invocation manually.
Cleanup checks whether gateway deletion already removed the tool before deleting it separately.
Observed Results
The user completed the browser steps and provided screenshots. The helper confirmed execution counts before and after Live invocation.
200; expected MCP source confirmed4040 -> 0message=hellovalidated: true; expected source and server ID in the request snippet0message=hellohellofrom the same source through the attached server1; recorded messagehello9010stopsRecorded fixture names:
The backend, frontend, and BFF remain running after fixture cleanup. Local test logs are retained.
This manual run uses one upstream tool. It does not repeat duplicate-name, access-denial, or server-scoped-token tests.
Closes #6743