diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 29b72344..723b85b6 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -35,7 +35,13 @@ type WorkspaceHandler struct { // internal/events/broadcaster.go. broadcaster events.EventEmitter provisioner *provisioner.Provisioner - cpProv *provisioner.CPProvisioner + // cpProv narrowed from `*provisioner.CPProvisioner` to the + // provisioner.CPProvisionerAPI interface (#1814) so tests can + // substitute a stub without standing up the real CP HTTP client + + // auth chain. Production callers still pass *CPProvisioner via + // SetCPProvisioner, which satisfies the interface — see the + // compile-time assertion in internal/provisioner/cp_provisioner.go. + cpProv provisioner.CPProvisionerAPI platformURL string configsDir string // path to workspace-configs-templates/ (for reading templates) // envMutators runs registered EnvMutator plugins right before @@ -64,7 +70,10 @@ func NewWorkspaceHandler(b events.EventEmitter, p *provisioner.Provisioner, plat // SetCPProvisioner wires the control plane provisioner for SaaS tenants. // Auto-activated when MOLECULE_ORG_ID is set (no manual config needed). -func (h *WorkspaceHandler) SetCPProvisioner(cp *provisioner.CPProvisioner) { +// +// Parameter is the CPProvisionerAPI interface (#1814) — production passes +// the *CPProvisioner from NewCPProvisioner; tests pass a stub. +func (h *WorkspaceHandler) SetCPProvisioner(cp provisioner.CPProvisionerAPI) { h.cpProv = cp } diff --git a/workspace-server/internal/handlers/workspace_provision_test.go b/workspace-server/internal/handlers/workspace_provision_test.go index 8dd34a80..0a445967 100644 --- a/workspace-server/internal/handlers/workspace_provision_test.go +++ b/workspace-server/internal/handlers/workspace_provision_test.go @@ -1184,11 +1184,107 @@ func TestProvisionWorkspace_NoInternalErrorsInBroadcast(t *testing.T) { } } +// stubFailingCPProv implements provisioner.CPProvisionerAPI. Start +// always returns the canned-leaky error fed in by the test. Stop + +// GetConsoleOutput aren't reached on the provisionWorkspaceCP failure +// path so they panic on call — surfaces an unexpected production-code +// reach into them as a test failure rather than a silent passthrough. +type stubFailingCPProv struct { + startErr error +} + +func (s *stubFailingCPProv) Start(_ context.Context, _ provisioner.WorkspaceConfig) (string, error) { + return "", s.startErr +} + +func (s *stubFailingCPProv) Stop(_ context.Context, _ string) error { + panic("stubFailingCPProv.Stop not expected on the provisionWorkspaceCP failure path") +} + +func (s *stubFailingCPProv) GetConsoleOutput(_ context.Context, _ string) (string, error) { + panic("stubFailingCPProv.GetConsoleOutput not expected on the provisionWorkspaceCP failure path") +} + // TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast asserts that -// provisionWorkspaceCP never leaks err.Error() in WORKSPACE_PROVISION_FAILED -// broadcasts. Regression test for issue #1206. +// provisionWorkspaceCP never leaks err.Error() in +// WORKSPACE_PROVISION_FAILED broadcasts. Regression test for #1206. +// +// Drives the cpProv.Start failure path — the only path inside +// provisionWorkspaceCP that emits a broadcast. The stubbed Start +// returns an error string stuffed with concrete leak markers (machine +// type, AMI ID, VPC subnet, raw HTTP body fragment) — the kind of +// content the real CP provisioner has historically returned when +// AWS/CP misbehaves. A regression that interpolates err.Error() into +// the broadcast payload would surface every marker; the canned +// "provisioning failed" message must surface none of them. func TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast(t *testing.T) { - t.Skip("TODO: write the test body. Same status as TestProvisionWorkspace_NoInternalErrorsInBroadcast — interface blocker fixed (#1814), need to construct the provisionWorkspaceCP failure path + assert payload sanitization.") + mock := setupTestDB(t) + + // loadWorkspaceSecrets queries global_secrets and workspace_secrets + // in order. Empty result rows for both = no secrets to decrypt = + // the function returns ({}, "") = the decrypt-error early-return + // branch is bypassed so we reach cpProv.Start. + mock.ExpectQuery(`SELECT key, encrypted_value, encryption_version FROM global_secrets`). + WillReturnRows(sqlmock.NewRows([]string{"key", "encrypted_value", "encryption_version"})) + mock.ExpectQuery(`SELECT key, encrypted_value, encryption_version FROM workspace_secrets`). + WithArgs("ws-cp-1206"). + WillReturnRows(sqlmock.NewRows([]string{"key", "encrypted_value", "encryption_version"})) + // On cpProv.Start failure, provisionWorkspaceCP also marks the + // workspace failed. Match-anything on args so the test isn't + // coupled to the exact UPDATE column order. + mock.ExpectExec(`UPDATE workspaces SET status = 'failed'`). + WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + cap := &captureBroadcaster{} + // Synthetic leaky error — every fragment is the kind of detail + // past CP errors have actually surfaced. If a regression makes + // the broadcast carry err.Error() verbatim, every marker below + // will appear in the captured payload and the assert loop catches + // it. (Same redaction-pin pattern as the sibling + // TestProvisionWorkspace_NoInternalErrorsInBroadcast — see the + // comment there for why we don't use containsUnsafeString.) + leakyErr := fmt.Errorf( + "CP API rejected provision: machine_type=t3.large ami=ami-0abcd1234efgh5678 " + + "vpc=vpc-deadbeef subnet=subnet-cafef00d body=\"{\\\"error\\\":\\\"InvalidSubnet.Conflict\\\"}\"", + ) + + handler := NewWorkspaceHandler(cap, nil, "http://localhost:8080", t.TempDir()) + handler.SetCPProvisioner(&stubFailingCPProv{startErr: leakyErr}) + + handler.provisionWorkspaceCP("ws-cp-1206", "/nonexistent/template", nil, models.CreateWorkspacePayload{ + Name: "ws-cp-1206", + Tier: 1, + Runtime: "claude-code", + }) + + if cap.lastData == nil { + t.Fatal("expected RecordAndBroadcast to capture data on cpProv.Start failure; got nothing") + } + if got := cap.lastData["error"]; got != "provisioning failed" { + t.Errorf("broadcast carried unexpected error message %q — should be the safe canned string", got) + } + for _, v := range cap.lastData { + s, ok := v.(string) + if !ok { + continue + } + for _, leakMarker := range []string{ + "t3.large", // machine type + "ami-0abcd1234efgh5678", // AMI id + "vpc-deadbeef", // VPC id + "subnet-cafef00d", // subnet id + "InvalidSubnet.Conflict", // raw upstream HTTP body + "CP API rejected", // raw error string head + } { + if strings.Contains(s, leakMarker) { + t.Errorf("broadcast leaked %q in payload value %q", leakMarker, s) + } + } + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } } // mockEnvMutator is a provisionhook.Registry stub that always returns a fixed error. diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index e9cfc8c9..fd7e640d 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -15,6 +15,28 @@ import ( "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" ) +// CPProvisionerAPI is the contract WorkspaceHandler uses to talk to the +// control-plane provisioner. Extracted as an interface (#1814) so handler +// tests can substitute a mock without standing up the real CP HTTP client +// + auth chain. Production wires *CPProvisioner directly via +// NewCPProvisioner — see the compile-time assertion below. +// +// Method set is intentionally narrow — only the methods that +// WorkspaceHandler actually calls. Adding a new handler call site that +// reaches into CPProvisioner means widening this interface explicitly, +// which surfaces the dependency in code review. +type CPProvisionerAPI interface { + Start(ctx context.Context, cfg WorkspaceConfig) (string, error) + Stop(ctx context.Context, workspaceID string) error + GetConsoleOutput(ctx context.Context, workspaceID string) (string, error) +} + +// Compile-time assertion: *CPProvisioner satisfies CPProvisionerAPI. +// Catches a future method-signature drift at build time instead of at +// the SetCPProvisioner call site (which would be a runtime "interface +// not implemented" only when the SaaS path is exercised). +var _ CPProvisionerAPI = (*CPProvisioner)(nil) + // CPProvisioner provisions workspace agents by calling the control plane's // workspace provision API. The control plane creates EC2 instances with // Docker + the workspace runtime installed at boot from PyPI.