Skip to content

Commit 9c24b4e

Browse files
fix(cli): path-escape server names in security subcommands (#1753)
# Pull Request ## Description `cmd/mcpproxy/security_cmd.go` builds every per-server request as `"/api/v1/servers/"+serverName+"/…"`. The daemon decodes a `%2F`-encoded `{id}` on `/api/v1/servers/{id}/*` (MCP-1118), but only if the client sends one. With an official-registry id like `io.github.owner/repo`, the raw slash splits the name across path segments and the request lands on another route. **Trigger.** `mcpproxy security scan|status|report|approve|reject|integrity io.github.owner/repo`. Ten call sites: `/scan`, `/scan/status` (x3), `/scan/files`, `/scan/report` (x2), `/security/approve`, `/security/reject`, `/integrity`. All ten now go through one `serverAPIPath(name, suffix)` helper using `url.PathEscape`, so a new subcommand doesn't add an eleventh raw concatenation. `security_cmd.go` isn't touched by any open PR. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass `cmd/mcpproxy/security_cmd_path_escape_test.go`: - `TestSecurityCmd_EscapesSlashServerName` runs `fetchFailedScannerInfo`, `printScanSummary` and `printScanDryRunPlan` against an `httptest` daemon and checks every per-server `EscapedPath()` carries `io.github.owner%2Frepo`. - `TestServerAPIPath` covers the helper, including the unchanged plain-name case. ``` go vet ./cmd/mcpproxy # clean go test ./cmd/mcpproxy -count=1 # ok # with the original call sites restored (helper kept so the test compiles): go test ./cmd/mcpproxy -run TestSecurityCmd_EscapesSlashServerName -count=1 # FAIL: per-server request path "/api/v1/servers/io.github.owner/repo/scan/status" ... gofmt -l <changed files> # clean ``` Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent e9181a1 commit 9c24b4e

2 files changed

Lines changed: 85 additions & 10 deletions

File tree

‎cmd/mcpproxy/security_cmd.go‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"io"
88
"net/http"
9+
"net/url"
910
"os"
1011
"path/filepath"
1112
"regexp"
@@ -643,7 +644,7 @@ func runSecurityScan(_ *cobra.Command, args []string) error {
643644
ctx, cancel := context.WithTimeout(context.Background(), hardTimeout)
644645
defer cancel()
645646

646-
resp, err := client.DoRaw(ctx, http.MethodPost, "/api/v1/servers/"+serverName+"/scan", body)
647+
resp, err := client.DoRaw(ctx, http.MethodPost, serverAPIPath(serverName, "/scan"), body)
647648
if err != nil {
648649
return fmt.Errorf("failed to start scan: %w", err)
649650
}
@@ -717,7 +718,7 @@ func runSecurityScan(_ *cobra.Command, args []string) error {
717718
case <-ticker.C:
718719
}
719720

720-
statusResp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/status", nil)
721+
statusResp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/status"), nil)
721722
if err != nil {
722723
return fmt.Errorf("failed to check scan status: %w", err)
723724
}
@@ -905,7 +906,7 @@ func printScanDryRunPlan(client *cliclient.Client, serverName, scannerFlag strin
905906
// 3. Best-effort: fetch the server's last scan context for source info.
906907
// This may 404 if the server has never been scanned — that's fine.
907908
var scanContext map[string]interface{}
908-
filesResp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/files", nil)
909+
filesResp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/files"), nil)
909910
if err == nil {
910911
filesRaw, _ := io.ReadAll(filesResp.Body)
911912
filesResp.Body.Close()
@@ -1389,7 +1390,7 @@ func runSecurityStatus(_ *cobra.Command, args []string) error {
13891390
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
13901391
defer cancel()
13911392

1392-
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/status", nil)
1393+
resp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/status"), nil)
13931394
if err != nil {
13941395
return fmt.Errorf("failed to get scan status: %w", err)
13951396
}
@@ -1453,7 +1454,7 @@ func runSecurityReport(_ *cobra.Command, args []string) error {
14531454
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
14541455
defer cancel()
14551456

1456-
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/report", nil)
1457+
resp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/report"), nil)
14571458
if err != nil {
14581459
return fmt.Errorf("failed to get scan report: %w", err)
14591460
}
@@ -1522,7 +1523,7 @@ type failedScannerInfo struct {
15221523
// complete in the most recent scan job for `serverName`. Returns nil on any
15231524
// error so the caller can render the report without per-scanner names.
15241525
func fetchFailedScannerInfo(client *cliclient.Client, ctx context.Context, serverName string) []failedScannerInfo {
1525-
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/status", nil)
1526+
resp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/status"), nil)
15261527
if err != nil {
15271528
return nil
15281529
}
@@ -1581,7 +1582,7 @@ func runSecurityApprove(_ *cobra.Command, args []string) error {
15811582
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
15821583
defer cancel()
15831584

1584-
resp, err := client.DoRaw(ctx, http.MethodPost, "/api/v1/servers/"+serverName+"/security/approve", body)
1585+
resp, err := client.DoRaw(ctx, http.MethodPost, serverAPIPath(serverName, "/security/approve"), body)
15851586
if err != nil {
15861587
return fmt.Errorf("failed to approve server: %w", err)
15871588
}
@@ -1628,7 +1629,7 @@ func runSecurityReject(_ *cobra.Command, args []string) error {
16281629
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
16291630
defer cancel()
16301631

1631-
resp, err := client.DoRaw(ctx, http.MethodPost, "/api/v1/servers/"+serverName+"/security/reject", nil)
1632+
resp, err := client.DoRaw(ctx, http.MethodPost, serverAPIPath(serverName, "/security/reject"), nil)
16321633
if err != nil {
16331634
return fmt.Errorf("failed to reject server: %w", err)
16341635
}
@@ -1741,7 +1742,7 @@ func runSecurityIntegrity(_ *cobra.Command, args []string) error {
17411742
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
17421743
defer cancel()
17431744

1744-
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/integrity", nil)
1745+
resp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/integrity"), nil)
17451746
if err != nil {
17461747
return fmt.Errorf("failed to check integrity: %w", err)
17471748
}
@@ -1815,7 +1816,7 @@ func runSecurityIntegrity(_ *cobra.Command, args []string) error {
18151816

18161817
// printScanSummary fetches and prints a compact summary after a scan completes.
18171818
func printScanSummary(client *cliclient.Client, ctx context.Context, serverName string) error {
1818-
resp, err := client.DoRaw(ctx, http.MethodGet, "/api/v1/servers/"+serverName+"/scan/report", nil)
1819+
resp, err := client.DoRaw(ctx, http.MethodGet, serverAPIPath(serverName, "/scan/report"), nil)
18191820
if err != nil {
18201821
fmt.Println("Scan completed. Use 'mcpproxy security report " + serverName + "' to view results.")
18211822
return nil
@@ -2973,3 +2974,10 @@ func formatTimestamp(ts string) string {
29732974
}
29742975
return ts
29752976
}
2977+
2978+
// serverAPIPath returns the REST path of a per-server sub-resource. The name is
2979+
// path-escaped: official-registry ids are namespace/name, and the daemon only
2980+
// decodes a %2F-encoded {id} (MCP-1118).
2981+
func serverAPIPath(serverName, suffix string) string {
2982+
return "/api/v1/servers/" + url.PathEscape(serverName) + suffix
2983+
}
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package main
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"net/http/httptest"
7+
"strings"
8+
"sync"
9+
"testing"
10+
11+
"go.uber.org/zap"
12+
13+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient"
14+
)
15+
16+
// `mcpproxy security status|report|scan io.github.owner/repo` must address the
17+
// server as one %2F-encoded path segment; an unescaped slash splits it and the
18+
// request lands on another route.
19+
func TestSecurityCmd_EscapesSlashServerName(t *testing.T) {
20+
const name = "io.github.owner/repo"
21+
22+
var (
23+
mu sync.Mutex
24+
paths []string
25+
)
26+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
27+
mu.Lock()
28+
paths = append(paths, r.URL.EscapedPath())
29+
mu.Unlock()
30+
w.Header().Set("Content-Type", "application/json")
31+
_, _ = w.Write([]byte(`{"success":true,"data":{}}`))
32+
}))
33+
defer ts.Close()
34+
35+
client := cliclient.NewClient(ts.URL, zap.NewNop().Sugar())
36+
ctx := context.Background()
37+
38+
_ = fetchFailedScannerInfo(client, ctx, name)
39+
_ = printScanSummary(client, ctx, name)
40+
_ = printScanDryRunPlan(client, name, "")
41+
42+
mu.Lock()
43+
defer mu.Unlock()
44+
var perServer []string
45+
for _, p := range paths {
46+
if strings.HasPrefix(p, "/api/v1/servers/") {
47+
perServer = append(perServer, p)
48+
}
49+
}
50+
if len(perServer) < 3 {
51+
t.Fatalf("expected per-server requests, got %v", paths)
52+
}
53+
for _, p := range perServer {
54+
if !strings.HasPrefix(p, "/api/v1/servers/io.github.owner%2Frepo/") {
55+
t.Errorf("per-server request path %q does not carry the escaped server name", p)
56+
}
57+
}
58+
}
59+
60+
func TestServerAPIPath(t *testing.T) {
61+
if got, want := serverAPIPath("io.github.owner/repo", "/scan"), "/api/v1/servers/io.github.owner%2Frepo/scan"; got != want {
62+
t.Errorf("serverAPIPath = %q, want %q", got, want)
63+
}
64+
if got, want := serverAPIPath("github", "/integrity"), "/api/v1/servers/github/integrity"; got != want {
65+
t.Errorf("serverAPIPath = %q, want %q", got, want)
66+
}
67+
}

0 commit comments

Comments
 (0)