Fix: Web UI delete server functionality and add comprehensive tests#97
Merged
Conversation
This commit fixes the HTTP 500 error when deleting servers from the Web UI and adds comprehensive test coverage. ## Changes ### Frontend (Web UI) - Fix API call to use correct operation name (`remove` instead of `delete`) - `frontend/src/services/api.ts`: Changed operation from 'delete' to 'remove' - Add alphabetical server sorting to match tray menu behavior - `frontend/src/stores/servers.ts`: Sort servers by name using localeCompare - Implement delete button with confirmation modal - `frontend/src/components/ServerCard.vue`: Added Delete button and modal dialog - `frontend/src/stores/servers.ts`: Added deleteServer() function with optimistic updates ### Backend (Tests) - Add unit tests for server deletion validation logic - `internal/server/mcp_test.go`: TestHandleRemoveUpstream with 6 test cases - Tests cover: success cases, error handling, security checks, permissions - Add E2E test for complete deletion flow - `internal/server/mcp_test.go`: TestE2E_DeleteServerFlow with 5-step verification - Tests: add server → verify exists → delete → verify removed → error on re-delete ## Bug Fix **Root Cause**: Frontend was calling upstream_servers tool with operation="delete" but the backend MCP handler expects operation="remove" (defined in mcp.go:38 as operationRemove). **Impact**: All delete operations from Web UI returned HTTP 500 errors. **Solution**: Updated frontend to use correct operation name "remove". ## Testing All tests pass: - ✅ TestHandleRemoveUpstream (6 test cases) - ✅ TestE2E_DeleteServerFlow (complete deletion flow) - ✅ Verified with Playwright browser automation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
CI Test ResultsSummary✅ All tests related to this PR passed successfully The CI shows one failing test, but it's unrelated to these changes: Test Results:
Unrelated Failure:
This test:
Verification:The logs show my new test completed successfully: All changes in this PR (delete functionality, sorting, unit tests, E2E tests) are working correctly. |
Dumbris
added a commit
that referenced
this pull request
Jun 22, 2026
…1076) (#745) * feat(registries): harden registry fetches against SSRF (CWE-918, MCP-1076) Registry server-list fetches use a user-supplied URL (`registry add-source`), which CodeQL flags as go/request-forgery (#97/#98): a malicious or typo'd registry source could point the daemon at internal/metadata endpoints (169.254.169.254, RFC1918 ranges). Add an SSRF guard centered on a single `isBlockedIP` predicate (loopback, RFC1918, RFC6598 CGNAT, link-local incl. the cloud-metadata endpoint, IPv6 unique-local, unspecified, multicast; fails closed), applied at three layers: - dial time: a net.Dialer Control hook on the shared registry HTTP client rejects the actual resolved IP before connect — covers hostnames that resolve into blocked ranges and closes the DNS-rebinding TOCTOU window a parse-time check alone cannot. - fetch pre-flight: validateRegistryURL rejects a literal-IP host (defense in depth + an explicit barrier at the http.NewRequest sink). - add-source/edit-source: literal-IP fail-fast (no DNS, stays pure/offline) so a bad source is refused up front with the stable invalid_registry_url code. Opt out with a new top-level `allow_private_registry_fetch` config flag (default false) for operators who run a trusted registry mirror on an internal/private address. TDD: table tests for every blocked/allowed range (incl. CGNAT and RFC1918 boundaries), an end-to-end loopback dial block, add-source literal-IP rejection, and the two registry-add E2E daemon tests opt in via the new flag. * test(server): opt loopback registry fetch tests past the SSRF guard The in-process server tests that fetch a registry served on a loopback httptest server (consistency_official, mcp_add_from_registry's startTestRegistry helper) now hit the new SSRF dial guard, because the registries-package test bypass does not apply to the server package's test binary. Set AllowPrivateRegistryFetch on their SetRegistriesFromConfig configs — exactly how an operator opts in for a trusted internal mirror — so the fetch is permitted while the production guard stays active for the add-source rejection test. Fixes the linux/arm64 Build Binaries leg (TestCrossSurfaceConsistency_*, TestHandleUpstreamServers_AddFromRegistry_*). * fix(registries): close SSRF proxy-bypass with app-layer host resolution CodexReviewer (round 2) found a real gap: the registry client clones http.DefaultTransport, which keeps Proxy: ProxyFromEnvironment. With HTTP_PROXY/HTTPS_PROXY set, the dialer connects to the PROXY as the first hop, so registryDialControl only ever validates the proxy's IP — never the real target. A hostname resolving to loopback/private/link-local (incl. 169.254.169.254) could still be fetched through a proxy. Add an application-layer guard (guardRegistryTargetHost) that resolves the target host and rejects the fetch if ANY resolved IP is in a blocked range. It runs before the request in registryGet, independent of the transport/proxy, so it holds in the proxy case; the dial-time Control stays as defense-in-depth for the direct path and DNS-rebind TOCTOU. Fail-open on resolver error (a flaky resolver must not break every fetch); literal IPs already covered pre-flight; relaxed by the allow_private_registry_fetch opt-in. Regression coverage: a public-looking hostname resolving to 169.254.169.254 is refused with HTTP(S)_PROXY set; plus direct predicate tests (mixed public+private resolution blocked, all-public allowed, resolver-error fail-open, public literal allowed). Resolver is injectable via a package var for deterministic, network-free tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the HTTP 500 error when deleting servers from the Web UI and adds comprehensive test coverage for the delete functionality.
Problem
When users tried to delete a server using the Web UI delete button, they encountered HTTP 500 errors. The root cause was a mismatch between the frontend API call and the backend MCP handler:
upstream_serverstool withoperation: "delete"operation: "remove"(defined ininternal/server/mcp.go:38)Changes
Frontend (Web UI)
Fixed API call (frontend/src/services/api.ts)
'delete'to'remove'Added alphabetical server sorting (frontend/src/stores/servers.ts)
Implemented delete functionality
deleteServer()function in stores (frontend/src/stores/servers.ts)Backend (Tests)
Unit tests (internal/server/mcp_test.go)
TestHandleRemoveUpstreamwith 6 test cases covering:E2E test (internal/server/mcp_test.go)
TestE2E_DeleteServerFlowwith complete deletion flow:operation: "remove"Testing
All tests pass successfully:
=== RUN TestHandleRemoveUpstream --- PASS: TestHandleRemoveUpstream (0.00s) --- PASS: TestHandleRemoveUpstream/successful_removal_of_existing_server --- PASS: TestHandleRemoveUpstream/fail_to_remove_non-existent_server --- PASS: TestHandleRemoveUpstream/fail_to_remove_in_read-only_mode --- PASS: TestHandleRemoveUpstream/fail_to_remove_when_management_disabled --- PASS: TestHandleRemoveUpstream/fail_to_remove_when_not_allowed --- PASS: TestHandleRemoveUpstream/successfully_remove_quarantined_server === RUN TestE2E_DeleteServerFlow --- PASS: TestE2E_DeleteServerFlow (12.92s)Also verified with Playwright browser automation:
Screenshots
Before: Clicking delete resulted in HTTP 500 error
After: Delete button works with confirmation modal and success toast
Checklist
make build)Related Issues
Fixes the HTTP 500 error reported by user when attempting to delete servers from Web UI.