Skip to content

Commit d610cba

Browse files
fix(scanner): sanitize slash-named servers in temp-dir patterns + decode scan-report jobId
Follow-up to the same MCP-2123 branch: once the sanitizer fix makes the container actually match, two more defects surface for official-registry slash-named servers (verified end-to-end by CEO on com.pulsemcp/google-flights): 1. os.MkdirTemp rejects a pattern containing a path separator, so the three extraction temp-dir patterns (mcpproxy-scan-%s-, -scan-full-%s-, -scan-tools-%s-) failed for '/'-containing names and source fell back to 'none'. Sanitize the name via dockernaming.SanitizeServerName first. 2. handleGetScanReportByJobID read the raw chi jobId param. Scan ids are scan-<serverName>-<ts> and contain '/', arriving percent-encoded, so the report endpoint 404'd. Decode via decodePathParam (same pattern the /servers subtree uses). Verified result: source_method=docker_extract, 23 files extracted, tools_exported=3, report endpoint HTTP 200. Adds regression tests: TestScanTempDirPatternHandlesSlashNames, TestDecodePathParamScanJobID. Related #MCP-2123 Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 parent 9ba555a commit d610cba

5 files changed

Lines changed: 80 additions & 5 deletions

File tree

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package httpapi
2+
3+
import "testing"
4+
5+
// TestDecodePathParamScanJobID is the MCP-2123 regression guard for the
6+
// scan-report-by-job-id route. Scan job IDs are "scan-<serverName>-<ts>", and
7+
// official-registry server names contain '/', so the id reaches the handler
8+
// percent-encoded (chi routes on RawPath). handleGetScanReportByJobID must
9+
// percent-decode it before the exact-match job lookup, otherwise "View Full
10+
// Report" 404s for slash-named servers even after the SPA route resolves.
11+
func TestDecodePathParamScanJobID(t *testing.T) {
12+
encoded := "scan-com.pulsemcp%2Fgoogle-flights-1781284446323229000"
13+
want := "scan-com.pulsemcp/google-flights-1781284446323229000"
14+
if got := decodePathParam(encoded); got != want {
15+
t.Errorf("decodePathParam(%q) = %q, want %q", encoded, got, want)
16+
}
17+
18+
// A non-encoded id must pass through unchanged.
19+
plain := "scan-simple-server-123"
20+
if got := decodePathParam(plain); got != plain {
21+
t.Errorf("decodePathParam(%q) = %q, want unchanged", plain, got)
22+
}
23+
}

internal/httpapi/security_scanner.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,12 @@ func (s *Server) handleGetScanReportByJobID(w http.ResponseWriter, r *http.Reque
663663
if !s.requireSecurity(w, r) {
664664
return
665665
}
666-
jobID := chi.URLParam(r, "jobId")
666+
// chi routes on RawPath, so the param arrives percent-encoded. Scan job IDs
667+
// embed the server name (scan-<serverName>-<ts>), and official-registry names
668+
// contain '/', so the slash reaches us as %2F and must be decoded before the
669+
// exact-match job lookup — otherwise the report page 404s for slash-named
670+
// servers even after the SPA route resolves (MCP-2123).
671+
jobID := decodePathParam(chi.URLParam(r, "jobId"))
667672
if jobID == "" {
668673
s.writeError(w, r, http.StatusBadRequest, "job ID is required")
669674
return

internal/security/scanner/service.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"sync/atomic"
1313
"time"
1414

15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/dockernaming"
1516
"go.uber.org/zap"
1617
)
1718

@@ -679,7 +680,7 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool,
679680
// If no source dir was resolved (no Docker container, no working_dir),
680681
// create a temp dir so Cisco scanner can at least scan tool definitions.
681682
if req.SourceDir == "" {
682-
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-tools-%s-", serverName))
683+
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-tools-%s-", dockernaming.SanitizeServerName(serverName)))
683684
if err == nil {
684685
req.SourceDir = tempDir
685686
// For HTTP/URL servers, preserve the "url" source method and path

internal/security/scanner/source_resolver.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,8 +351,11 @@ func (r *SourceResolver) findServerContainer(ctx context.Context, serverName str
351351
// hoisted into the same shared cache cannot leak into the scan.
352352
func (r *SourceResolver) extractFromContainer(ctx context.Context, containerID string, info ServerInfo) (string, func(), error) {
353353
serverName := info.Name
354-
// Create temp directory for extracted source
355-
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-%s-", serverName))
354+
// Create temp directory for extracted source. The server name can contain
355+
// path separators (official-registry names like "com.pulsemcp/google-flights"),
356+
// which os.MkdirTemp rejects in the pattern ("contains path separator") — so
357+
// sanitize it to the same token used for the container name (MCP-2123).
358+
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-%s-", dockernaming.SanitizeServerName(serverName)))
356359
if err != nil {
357360
return "", nil, fmt.Errorf("failed to create temp dir: %w", err)
358361
}
@@ -811,7 +814,8 @@ func (r *SourceResolver) ResolveFullSource(ctx context.Context, info ServerInfo)
811814
// false positives (e.g. flagging shutil.py or tempfile.py as "malicious").
812815
func (r *SourceResolver) extractFullFromContainer(ctx context.Context, containerID string, info ServerInfo) (string, func(), error) {
813816
serverName := info.Name
814-
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-full-%s-", serverName))
817+
// Sanitize: server names may contain '/' which os.MkdirTemp rejects (MCP-2123).
818+
tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-full-%s-", dockernaming.SanitizeServerName(serverName)))
815819
if err != nil {
816820
return "", nil, fmt.Errorf("failed to create temp dir: %w", err)
817821
}
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package scanner
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"testing"
7+
8+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/dockernaming"
9+
)
10+
11+
// TestScanTempDirPatternHandlesSlashNames is the MCP-2123 regression guard for
12+
// the extraction temp dirs. Official-registry server names contain '/'
13+
// (e.g. "com.pulsemcp/google-flights"). os.MkdirTemp rejects a pattern that
14+
// contains a path separator ("pattern contains path separator"), so the raw
15+
// interpolation used by extractFromContainer / extractFullFromContainer /
16+
// StartScan's tool-defs fallback failed and source resolution fell back to
17+
// "none". The fix sanitizes the name via dockernaming.SanitizeServerName before
18+
// building the pattern. This test proves the sanitized pattern is MkdirTemp-safe
19+
// while the raw one is not.
20+
func TestScanTempDirPatternHandlesSlashNames(t *testing.T) {
21+
const slashName = "com.pulsemcp/google-flights"
22+
23+
// Raw name reproduces the original failure — guards against anyone
24+
// reintroducing the un-sanitized interpolation.
25+
if _, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-%s-", slashName)); err == nil {
26+
t.Fatalf("expected os.MkdirTemp to reject raw slash-named pattern, but it succeeded")
27+
}
28+
29+
// Every pattern the scanner uses must succeed once sanitized.
30+
sanitized := dockernaming.SanitizeServerName(slashName)
31+
for _, prefix := range []string{
32+
"mcpproxy-scan-%s-",
33+
"mcpproxy-scan-full-%s-",
34+
"mcpproxy-scan-tools-%s-",
35+
} {
36+
dir, err := os.MkdirTemp("", fmt.Sprintf(prefix, sanitized))
37+
if err != nil {
38+
t.Fatalf("os.MkdirTemp with sanitized pattern %q failed: %v", prefix, err)
39+
}
40+
_ = os.RemoveAll(dir)
41+
}
42+
}

0 commit comments

Comments
 (0)