fix(scanner): resolve source for runtime-added Docker-isolated stdio servers (MCP-2123 Defect A)

internal/dockernaming/naming.go

@@ -0,0 +1,64 @@
+// Package dockernaming centralises the rule for turning an MCPProxy server name
+// into the sanitized token used inside a Docker container name
+// (mcpproxy-<sanitized>-<suffix>).
+//
+// It exists so the component that NAMES a container at launch
+// (internal/upstream/core) and the component that LOOKS UP a running container
+// by name (internal/security/scanner) share one source of truth. They used to
+// carry independent sanitizers that disagreed on '.': the launcher preserved it
+// (Docker allows '.') while the scanner mapped it to '-'. For official-registry
+// servers — whose names are a dotted namespace plus a slash, e.g.
+// "com.pulsemcp/google-flights" — that mismatch meant the scanner's
+// `docker ps --filter name=mcpproxy-<sanitized>-` prefix never matched the real
+// container, so source extraction silently fell through to "No Source Available"
+// (MCP-2123). Keeping the rule in one leaf package makes that drift impossible.
+package dockernaming
+
+import (
+	"regexp"
+	"strings"
+)
+
+var (
+	invalidContainerChars = regexp.MustCompile(`[^a-zA-Z0-9_.-]+`)
+	leadingAlphanumeric   = regexp.MustCompile(`^[a-zA-Z0-9]`)
+)
+
+// maxSanitizedLen bounds the sanitized token so the full container name
+// (mcpproxy- prefix + token + - + 4-char suffix) stays well under Docker's
+// 253-char limit.
+const maxSanitizedLen = 200
+
+// SanitizeServerName converts a server name into a valid Docker container-name
+// token. Docker container names may contain [a-zA-Z0-9][a-zA-Z0-9_.-]*, so this
+// preserves letters, digits, '_', '.', and '-' and replaces any other run of
+// characters with a single '-'. It then collapses consecutive hyphens, ensures
+// the result starts with an alphanumeric character, trims trailing '-'/'.', and
+// truncates to a safe length.
+func SanitizeServerName(name string) string {
+	sanitized := invalidContainerChars.ReplaceAllString(name, "-")
+
+	for strings.Contains(sanitized, "--") {
+		sanitized = strings.ReplaceAll(sanitized, "--", "-")
+	}
+
+	if sanitized != "" && !leadingAlphanumeric.MatchString(sanitized) {
+		sanitized = "server-" + sanitized
+		for strings.Contains(sanitized, "--") {
+			sanitized = strings.ReplaceAll(sanitized, "--", "-")
+		}
+	}
+
+	sanitized = strings.TrimRight(sanitized, "-.")
+
+	if sanitized == "" {
+		sanitized = "server"
+	}
+
+	if len(sanitized) > maxSanitizedLen {
+		sanitized = sanitized[:maxSanitizedLen]
+		sanitized = strings.TrimRight(sanitized, "-.")
+	}
+
+	return sanitized
+}

internal/dockernaming/naming_test.go

@@ -0,0 +1,43 @@
+package dockernaming
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestSanitizeServerName(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		// Regression cases for MCP-2123: official-registry servers carry a
+		// dotted namespace AND a slash (namespace/name). The dot MUST be
+		// preserved (Docker allows it) while the slash becomes a hyphen, so the
+		// scanner's container-name prefix matches what the launcher actually
+		// named the container (mcpproxy-com.pulsemcp-google-flights-<suffix>).
+		{name: "official registry namespaced", input: "com.pulsemcp/google-flights", expected: "com.pulsemcp-google-flights"},
+		{name: "github official namespaced", input: "io.github.owner/repo", expected: "io.github.owner-repo"},
+
+		// Parity with the launcher's existing sanitizer
+		// (internal/upstream/core sanitizeServerNameForContainer).
+		{name: "simple name", input: "github-server", expected: "github-server"},
+		{name: "name with spaces", input: "my server name", expected: "my-server-name"},
+		{name: "name with special characters", input: "server@#$%^&*()", expected: "server"},
+		{name: "name starting with invalid character", input: "-invalid-start", expected: "server-invalid-start"},
+		{name: "name with dots", input: "server.name.test", expected: "server.name.test"},
+		{name: "name with underscores", input: "server_name_test", expected: "server_name_test"},
+		{name: "empty name", input: "", expected: "server"},
+		{name: "name with multiple consecutive special chars", input: "server!!!@@@name", expected: "server-name"},
+		{name: "name ending with hyphens and dots", input: "server-name-..", expected: "server-name"},
+		{name: "very long name", input: strings.Repeat("a", 250), expected: strings.Repeat("a", 200)},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := SanitizeServerName(tt.input); got != tt.expected {
+				t.Errorf("SanitizeServerName(%q) = %q, want %q", tt.input, got, tt.expected)
+			}
+		})
+	}
+}

internal/httpapi/scan_jobid_decode_test.go

@@ -0,0 +1,23 @@
+package httpapi
+
+import "testing"
+
+// TestDecodePathParamScanJobID is the MCP-2123 regression guard for the
+// scan-report-by-job-id route. Scan job IDs are "scan-<serverName>-<ts>", and
+// official-registry server names contain '/', so the id reaches the handler
+// percent-encoded (chi routes on RawPath). handleGetScanReportByJobID must
+// percent-decode it before the exact-match job lookup, otherwise "View Full
+// Report" 404s for slash-named servers even after the SPA route resolves.
+func TestDecodePathParamScanJobID(t *testing.T) {
+	encoded := "scan-com.pulsemcp%2Fgoogle-flights-1781284446323229000"
+	want := "scan-com.pulsemcp/google-flights-1781284446323229000"
+	if got := decodePathParam(encoded); got != want {
+		t.Errorf("decodePathParam(%q) = %q, want %q", encoded, got, want)
+	}
+
+	// A non-encoded id must pass through unchanged.
+	plain := "scan-simple-server-123"
+	if got := decodePathParam(plain); got != plain {
+		t.Errorf("decodePathParam(%q) = %q, want unchanged", plain, got)
+	}
+}

internal/httpapi/security_scanner.go

@@ -663,7 +663,12 @@ func (s *Server) handleGetScanReportByJobID(w http.ResponseWriter, r *http.Reque
 	if !s.requireSecurity(w, r) {
 		return
 	}
-	jobID := chi.URLParam(r, "jobId")
+	// chi routes on RawPath, so the param arrives percent-encoded. Scan job IDs
+	// embed the server name (scan-<serverName>-<ts>), and official-registry names
+	// contain '/', so the slash reaches us as %2F and must be decoded before the
+	// exact-match job lookup — otherwise the report page 404s for slash-named
+	// servers even after the SPA route resolves (MCP-2123).
+	jobID := decodePathParam(chi.URLParam(r, "jobId"))
 	if jobID == "" {
 		s.writeError(w, r, http.StatusBadRequest, "job ID is required")
 		return

internal/security/scanner/service.go

@@ -679,7 +679,11 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool,
 		// If no source dir was resolved (no Docker container, no working_dir),
 		// create a temp dir so Cisco scanner can at least scan tool definitions.
 		if req.SourceDir == "" {
-			tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-tools-%s-", serverName))
+			// The temp-dir name is purely cosmetic — os.MkdirTemp's random suffix
+			// already guarantees uniqueness. Keep the pattern a constant so no
+			// user-controlled server name flows into the path (go/path-injection,
+			// MCP-2155) and slash-named servers are trivially safe (MCP-2123).
+			tempDir, err := os.MkdirTemp("", "mcpproxy-scan-tools-")
 			if err == nil {
 				req.SourceDir = tempDir
 				// For HTTP/URL servers, preserve the "url" source method and path

internal/security/scanner/source_resolver.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/dockernaming"
 	"github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap"
 	"go.uber.org/zap"
 )
@@ -307,12 +308,16 @@ func dirLooksLikeSource(dir string) bool {
 	return found
 }
 
-// findServerContainer finds the running Docker container for a server
-// MCPProxy names containers as: mcpproxy-<sanitized-server-name>-<suffix>
+// findServerContainer finds the running Docker container for a server.
+// MCPProxy names containers as: mcpproxy-<sanitized-server-name>-<suffix>.
+// The sanitization MUST match the one used to name the container at launch
+// (internal/upstream/core), hence the shared dockernaming package — official
+// registry names like "com.pulsemcp/google-flights" keep their dots and would
+// otherwise never match (MCP-2123).
 func (r *SourceResolver) findServerContainer(ctx context.Context, serverName string) (string, error) {
 	// Use docker ps with filter to find matching containers
 	cmd := r.dockerCmd(ctx, "ps",
-		"--filter", fmt.Sprintf("name=mcpproxy-%s-", sanitizeForDocker(serverName)),
+		"--filter", fmt.Sprintf("name=mcpproxy-%s-", dockernaming.SanitizeServerName(serverName)),
 		"--format", "{{.ID}}",
 		"--no-trunc",
 	)
@@ -346,8 +351,12 @@ func (r *SourceResolver) findServerContainer(ctx context.Context, serverName str
 // hoisted into the same shared cache cannot leak into the scan.
 func (r *SourceResolver) extractFromContainer(ctx context.Context, containerID string, info ServerInfo) (string, func(), error) {
 	serverName := info.Name
-	// Create temp directory for extracted source
-	tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-%s-", serverName))
+	// Create temp directory for extracted source. Keep the pattern a constant:
+	// os.MkdirTemp's random suffix already guarantees uniqueness, so embedding the
+	// (user-controlled) server name added nothing but a go/path-injection taint
+	// (MCP-2155) and a slash-rejection bug for official-registry names like
+	// "com.pulsemcp/google-flights" (MCP-2123). Dropping it fixes both.
+	tempDir, err := os.MkdirTemp("", "mcpproxy-scan-")
 	if err != nil {
 		return "", nil, fmt.Errorf("failed to create temp dir: %w", err)
 	}
@@ -806,7 +815,10 @@ func (r *SourceResolver) ResolveFullSource(ctx context.Context, info ServerInfo)
 // false positives (e.g. flagging shutil.py or tempfile.py as "malicious").
 func (r *SourceResolver) extractFullFromContainer(ctx context.Context, containerID string, info ServerInfo) (string, func(), error) {
 	serverName := info.Name
-	tempDir, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-full-%s-", serverName))
+	// Keep the pattern a constant — os.MkdirTemp's random suffix guarantees
+	// uniqueness; embedding the user-controlled server name only added a
+	// go/path-injection taint (MCP-2155) and a slash-rejection bug (MCP-2123).
+	tempDir, err := os.MkdirTemp("", "mcpproxy-scan-full-")
 	if err != nil {
 		return "", nil, fmt.Errorf("failed to create temp dir: %w", err)
 	}
@@ -1326,8 +1338,3 @@ func (r *SourceResolver) findGitCheckoutByRepo(checkoutsDir, repoName, gitURL st
 	}
 	return bestPath, nil
 }
-
-// sanitizeForDocker removes characters invalid in Docker container names
-func sanitizeForDocker(name string) string {
-	return strings.NewReplacer("/", "-", ":", "-", ".", "-", " ", "-").Replace(name)
-}

internal/security/scanner/source_resolver_test.go

@@ -671,27 +671,11 @@ func TestIsPackageRunnerCommand(t *testing.T) {
 	}
 }
 
-func TestSanitizeForDocker(t *testing.T) {
-	tests := []struct {
-		input string
-		want  string
-	}{
-		{"simple", "simple"},
-		{"my-server", "my-server"},
-		{"org/repo", "org-repo"},
-		{"host:port", "host-port"},
-		{"with.dots", "with-dots"},
-		{"with spaces", "with-spaces"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.input, func(t *testing.T) {
-			if got := sanitizeForDocker(tt.input); got != tt.want {
-				t.Errorf("sanitizeForDocker(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}
+// Container-name sanitization now lives in internal/dockernaming
+// (SanitizeServerName) so the scanner's container lookup and the launcher's
+// container naming share one rule. Its behavior — including the MCP-2123
+// regression case where dotted official-registry names must be preserved — is
+// covered by TestSanitizeServerName in that package.
 
 // TestDockerCmdResolvesBinaryViaShellwrap verifies that the SourceResolver
 // resolves the docker binary through shellwrap.ResolveDockerPath rather than

internal/security/scanner/tempdir_slashname_test.go

@@ -0,0 +1,49 @@
+package scanner
+
+import (
+	"fmt"
+	"os"
+	"strings"
+	"testing"
+)
+
+// TestScanTempDirPatternHandlesSlashNames is the MCP-2123 regression guard for
+// the extraction temp dirs. Official-registry server names contain '/'
+// (e.g. "com.pulsemcp/google-flights"). os.MkdirTemp rejects a pattern that
+// contains a path separator ("pattern contains path separator"), so the
+// original raw interpolation of the server name into the pattern
+// (extractFromContainer / extractFullFromContainer / StartScan's tool-defs
+// fallback) failed and source resolution fell back to "none".
+//
+// The fix (MCP-2155) drops the user-controlled server name from the pattern
+// entirely: the random suffix os.MkdirTemp appends already guarantees a unique
+// dir, so the name was purely cosmetic. Removing it kills the slash-rejection
+// bug AND the go/path-injection CodeQL alert at the source (no user-provided
+// value flows into the path expression). This test proves the three constant
+// patterns the scanner now uses are MkdirTemp-safe and carry no user data.
+func TestScanTempDirPatternHandlesSlashNames(t *testing.T) {
+	const slashName = "com.pulsemcp/google-flights"
+
+	// The original raw, name-interpolated pattern fails — guards against anyone
+	// reintroducing the un-sanitized interpolation that broke slash names.
+	if _, err := os.MkdirTemp("", fmt.Sprintf("mcpproxy-scan-%s-", slashName)); err == nil {
+		t.Fatalf("expected os.MkdirTemp to reject raw slash-named pattern, but it succeeded")
+	}
+
+	// Every constant pattern the scanner now uses must succeed regardless of the
+	// server name, and must not embed any user-controlled value.
+	for _, prefix := range []string{
+		"mcpproxy-scan-",
+		"mcpproxy-scan-full-",
+		"mcpproxy-scan-tools-",
+	} {
+		if strings.Contains(prefix, "%") {
+			t.Fatalf("pattern %q still interpolates a value; user data must not flow into the path", prefix)
+		}
+		dir, err := os.MkdirTemp("", prefix)
+		if err != nil {
+			t.Fatalf("os.MkdirTemp with constant pattern %q failed: %v", prefix, err)
+		}
+		_ = os.RemoveAll(dir)
+	}
+}

internal/server/security_scanner_provider_test.go

@@ -0,0 +1,72 @@
+package server
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// TestConfigServerInfoProvider_UsesLiveConfig is the MCP-2123 regression guard.
+//
+// configServerInfoProvider used to iterate a config snapshot captured at server
+// boot. Servers added at runtime (e.g. from the official registry, whose names
+// look like "com.pulsemcp/google-flights") were absent from that snapshot, so
+// GetServerInfo returned "not found". The scanner then ran with no ServerInfo:
+// source resolution stayed "none", no tools.json was exported, and Pass 2 was
+// skipped with "No server info available" — exactly the reported defect.
+//
+// The provider must resolve server info against the LIVE config.
+func TestConfigServerInfoProvider_UsesLiveConfig(t *testing.T) {
+	bootSnapshot := &config.Config{
+		Servers: []*config.ServerConfig{
+			{Name: "boot-server", Protocol: "stdio"},
+		},
+	}
+
+	// Simulates a server added at runtime after boot — present in the live
+	// config but not in the snapshot the provider was constructed with.
+	liveConfig := &config.Config{
+		Servers: []*config.ServerConfig{
+			{Name: "boot-server", Protocol: "stdio"},
+			{
+				Name:     "com.pulsemcp/google-flights",
+				Protocol: "stdio",
+				Command:  "npx",
+				Args:     []string{"-y", "@pulsemcp/google-flights"},
+			},
+		},
+	}
+
+	p := &configServerInfoProvider{
+		cfg:        bootSnapshot,
+		liveConfig: func() *config.Config { return liveConfig },
+	}
+
+	info, err := p.GetServerInfo("com.pulsemcp/google-flights")
+	require.NoError(t, err, "runtime-added server must be resolvable via live config")
+	require.NotNil(t, info)
+	assert.Equal(t, "com.pulsemcp/google-flights", info.Name)
+	assert.Equal(t, "stdio", info.Protocol)
+	assert.Equal(t, "npx", info.Command)
+}
+
+// TestConfigServerInfoProvider_FallsBackToSnapshot ensures the provider still
+// works when no live-config accessor is wired (defensive default).
+func TestConfigServerInfoProvider_FallsBackToSnapshot(t *testing.T) {
+	p := &configServerInfoProvider{
+		cfg: &config.Config{
+			Servers: []*config.ServerConfig{{Name: "only-server", Protocol: "stdio"}},
+		},
+	}
+
+	info, err := p.GetServerInfo("only-server")
+	require.NoError(t, err)
+	require.NotNil(t, info)
+	assert.Equal(t, "only-server", info.Name)
+
+	_, err = p.GetServerInfo("missing")
+	require.Error(t, err)
+}