test(plugins): unblock TestResolveAndStage_NoInternalErrorsInHTTPErr (#1814)

Closes the second of two skipped tests in workspace_provision_test.go
that were blocked on interface refactors. The Broadcaster + CP
provisioner halves landed in earlier #1814 cycles; this is the
plugin-source-registry half.

Refactor:
  - Add handlers.pluginSources interface with the 3 methods handler
    code actually calls (Register, Resolve, Schemes)
  - Compile-time assertion `var _ pluginSources = (*plugins.Registry)(nil)`
    catches future method-signature drift at build time
  - PluginsHandler.sources narrowed from *plugins.Registry to the
    interface; production wiring (NewPluginsHandler, WithSourceResolver)
    still passes *plugins.Registry — satisfies the interface

Production fix (#1206 leak):
  - resolveAndStage's Fetch-failure path was interpolating err.Error()
    into the HTTP response body via `failed to fetch plugin from %s: %v`.
    Resolver errors routinely contain rate-limit text, github request
    IDs, raw HTTP body fragments, and (for local resolvers) file system
    paths — none has any business landing in a user's browser.
  - Body now carries just `failed to fetch plugin from <scheme>`; the
    status code already differentiates the failure shape (404 not
    found, 504 timeout, 502 generic). Full err detail stays in the
    server-side log line one statement above.

Test:
  - 6 sub-tests covering every error path inside resolveAndStage:
    empty source, invalid format, unknown scheme, local
    path-traversal, unpinned github (PLUGIN_ALLOW_UNPINNED unset),
    Fetch failure with a leaky synthetic error
  - The Fetch-failure case plants 5 realistic leak markers in the
    resolver's error string (rate limit text, x-github-request-id,
    auth_token, ghp_-prefixed token, /etc/passwd path); the assertion
    fails if ANY appears in the response body
  - Table-driven so a future error path added to resolveAndStage gets
    one new row, not a copy-paste of the assertion logic

Verification:
  - 6/6 sub-tests pass
  - Full workspace-server test suite passes (interface refactor is
    non-breaking; production caller paths unchanged)
  - go build ./... clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

workspace-server/internal/handlers/plugins.go

@@ -22,13 +22,38 @@ import (
 // workspace-scoped filtering (handler falls back to unfiltered list).
 type RuntimeLookup func(workspaceID string) (string, error)
 
+// pluginSources is the contract PluginsHandler uses to talk to the
+// plugin source registry. Extracted as an interface (#1814) so tests can
+// substitute a stub without standing up the real *plugins.Registry +
+// every concrete resolver. Production wires *plugins.Registry directly,
+// which satisfies this interface — see the compile-time assertion below.
+//
+// Method set is intentionally narrow — only what handler code calls.
+// Register is included because WithSourceResolver and NewPluginsHandler
+// both invoke it; a stub that doesn't need to record registrations can
+// implement it as a no-op.
+type pluginSources interface {
+	Register(resolver plugins.SourceResolver)
+	Resolve(source plugins.Source) (plugins.SourceResolver, error)
+	Schemes() []string
+}
+
+// Compile-time assertion: *plugins.Registry satisfies pluginSources.
+// Catches a future method-signature drift at build time instead of when
+// router wiring runs in main().
+var _ pluginSources = (*plugins.Registry)(nil)
+
 // PluginsHandler manages the plugin registry and per-workspace plugin installation.
 type PluginsHandler struct {
-	pluginsDir    string            // host path to plugins/ registry
-	docker        *client.Client    // Docker client for container operations
-	restartFunc   func(string)      // auto-restart workspace after install/uninstall
-	runtimeLookup RuntimeLookup     // workspace_id → runtime (optional)
-	sources       *plugins.Registry // pluggable install sources (local, github, clawhub, …)
+	pluginsDir    string         // host path to plugins/ registry
+	docker        *client.Client // Docker client for container operations
+	restartFunc   func(string)   // auto-restart workspace after install/uninstall
+	runtimeLookup RuntimeLookup  // workspace_id → runtime (optional)
+	// sources narrowed from `*plugins.Registry` to the pluginSources
+	// interface (#1814) so tests can substitute a stub. Production
+	// callers still pass *plugins.Registry, which satisfies the
+	// interface — see the compile-time assertion above.
+	sources pluginSources
 }
 
 // NewPluginsHandler constructs a PluginsHandler with the default source

workspace-server/internal/handlers/plugins_install_pipeline.go

@@ -191,8 +191,15 @@ func (h *PluginsHandler) resolveAndStage(ctx context.Context, req installRequest
 		} else if errors.Is(err, context.DeadlineExceeded) {
 			status = http.StatusGatewayTimeout
 		}
+		// F1086 / #1206: do NOT interpolate err into the response — a
+		// resolver failure (github API rate-limit text, raw HTTP body,
+		// file system path from a local-fs resolver) routinely contains
+		// internal detail that has no business landing in the user's
+		// browser. The status code already differentiates the failure
+		// shape (404 not found vs 504 timeout vs 502 generic) for the
+		// caller; full detail stays in the log line above.
 		return nil, newHTTPErr(status, gin.H{
-			"error":  fmt.Sprintf("failed to fetch plugin from %s: %v", source.Scheme, err),
+			"error":  fmt.Sprintf("failed to fetch plugin from %s", source.Scheme),
 			"source": source.Raw(),
 		})
 	}

workspace-server/internal/handlers/workspace_provision_test.go

@@ -3,6 +3,7 @@ package handlers
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
@@ -1202,31 +1203,129 @@ func (m *mockEnvMutator) Run(_ context.Context, _ string, _ map[string]string) e
 
 func (m *mockEnvMutator) Register(_ provisionhook.EnvMutator) {}
 
-// TestResolveAndStage_NoInternalErrorsInHTTPErr asserts that resolveAndStage
-// never puts err.Error() in HTTP error responses. Tests plugin source
-// parsing, resolver failures, and validation errors.
+// TestResolveAndStage_NoInternalErrorsInHTTPErr asserts that
+// resolveAndStage never puts internal error detail (resolver error
+// strings, file-system paths, upstream rate-limit text, auth tokens
+// echoed by a misbehaving upstream) into HTTP error response bodies.
+// Regression guard for #1206.
+//
+// Drives every error path inside resolveAndStage and asserts the
+// returned *httpErr's body carries none of the leak markers planted in
+// the stub's failing-Fetch error. Each path exercises the
+// corresponding `return nil, newHTTPErr(...)` site, so a future
+// regression that interpolates err into any of those bodies fails
+// here.
 func TestResolveAndStage_NoInternalErrorsInHTTPErr(t *testing.T) {
-	t.Skip("TODO: mockPluginsSources type mismatch with PluginsHandler.sources (*plugins.Registry). Needs resolver interface refactor — currently blocking package compile on main (2026-04-21).")
+	t.Setenv("PLUGIN_ALLOW_UNPINNED", "")
+	// Markers planted in the stub's failing-Fetch error. None of these
+	// is something a real plugin name, scheme, or schemes list would
+	// legitimately contain — so any appearance in the response body
+	// means err leaked through.
+	const leakyErrText = "rate limit exceeded x-github-request-id=ABC123 auth_token=ghp_INTERNAL_DETAIL /etc/passwd"
+	leakMarkers := []string{
+		"rate limit",
+		"x-github-request-id",
+		"auth_token",
+		"ghp_INTERNAL_DETAIL",
+		"/etc/passwd",
+	}
+
+	cases := []struct {
+		name       string
+		source     string
+		fetchErr   error // non-nil => path 6 (resolver Fetch failure)
+		wantStatus int
+	}{
+		{"empty source", "", nil, http.StatusBadRequest},
+		{"invalid source format", "not a valid uri", nil, http.StatusBadRequest},
+		{"unknown scheme", "weirdscheme://x", nil, http.StatusBadRequest},
+		{"local path-traversal", "local://../etc/passwd", nil, http.StatusBadRequest},
+		{"unpinned github source", "github://owner/repo", nil, http.StatusUnprocessableEntity},
+		// Path 6: resolver Fetch returns a leaky error. Pre-#1814 fix
+		// the body interpolated `%v` of err — every marker below would
+		// appear in the response. Post-fix the body is just the canned
+		// "failed to fetch plugin from <scheme>".
+		{"fetch failure with leaky error", "github://owner/repo#v1.0", fmt.Errorf("%s", leakyErrText), http.StatusBadGateway},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			sources := &mockPluginsSources{schemes: []string{"local", "github"}}
+			if tc.fetchErr != nil {
+				sources.failingResolver = &mockResolver{fetchErr: tc.fetchErr}
+			}
+			h := &PluginsHandler{sources: sources}
+
+			_, err := h.resolveAndStage(context.Background(), installRequest{Source: tc.source})
+			if err == nil {
+				t.Fatalf("expected error for source %q, got nil", tc.source)
+			}
+			httpE, ok := err.(*httpErr)
+			if !ok {
+				t.Fatalf("expected *httpErr for source %q, got %T", tc.source, err)
+			}
+			if httpE.Status != tc.wantStatus {
+				t.Errorf("status: source=%q got %d, want %d", tc.source, httpE.Status, tc.wantStatus)
+			}
+			// Body fields can be string, []string ("available_schemes"),
+			// or other types. Walk each, normalize to a string, and
+			// search for leak markers.
+			for k, v := range httpE.Body {
+				var serialized string
+				switch x := v.(type) {
+				case string:
+					serialized = x
+				case []string:
+					serialized = strings.Join(x, " ")
+				default:
+					continue
+				}
+				for _, mark := range leakMarkers {
+					if strings.Contains(serialized, mark) {
+						t.Errorf("source=%q field=%q leaked %q in value %q",
+							tc.source, k, mark, serialized)
+					}
+				}
+			}
+		})
+	}
 }
 
-// mockPluginsSources implements plugins.SourceResolver for testing.
+// mockPluginsSources is a stub pluginSources for testing — it satisfies
+// the interface (Register/Resolve/Schemes) but stores nothing of its
+// own except the scheme list to surface in error responses + an
+// optional failingResolver to drive the Fetch-failure path.
 type mockPluginsSources struct {
-	schemes []string
+	schemes         []string
+	failingResolver *mockResolver
 }
 
+// Register is a no-op — tests don't need to record registrations.
+func (m *mockPluginsSources) Register(_ plugins.SourceResolver) {}
+
 func (m *mockPluginsSources) Schemes() []string { return m.schemes }
 
 func (m *mockPluginsSources) Resolve(source plugins.Source) (plugins.SourceResolver, error) {
 	if source.Scheme == "github" {
+		if m.failingResolver != nil {
+			return m.failingResolver, nil
+		}
 		return &mockResolver{}, nil
 	}
 	return nil, fmt.Errorf("unsupported scheme %q", source.Scheme)
 }
 
-type mockResolver struct{}
+// mockResolver is a configurable plugins.SourceResolver: Fetch returns
+// (fetchName, fetchErr) verbatim. Default zero-value fetchErr=nil and
+// fetchName="" lets tests exercise the empty-name validation path; a
+// non-nil fetchErr exercises the Fetch-failure leak-redaction path.
+type mockResolver struct {
+	fetchName string
+	fetchErr  error
+}
 
 func (*mockResolver) Scheme() string { return "" }
 
-func (*mockResolver) Fetch(ctx context.Context, spec, destDir string) (string, error) {
-	return "", nil
+func (m *mockResolver) Fetch(_ context.Context, _, _ string) (string, error) {
+	return m.fetchName, m.fetchErr
 }