From e25b8a508eae30e3190890096095cad2ef2f97f5 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Sun, 26 Apr 2026 22:31:30 -0700 Subject: [PATCH] test(provisioning): pin no-internal-errors-in-broadcast for global-secret decrypt path (#1814) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [Molecule-Platform-Evolvement-Manager] ## What this fixes Closes one of the three skipped tests in workspace_provision_test.go that #1814's interface refactor enabled but never had a body written: `TestProvisionWorkspace_NoInternalErrorsInBroadcast`. The interface blocker (`captureBroadcaster` couldn't substitute for `*events.Broadcaster`) was already fixed when `events.EventEmitter` was extracted; this PR ships the test body that the prior refactor made possible. The test was effectively unverified regression cover for issue #1206 (internal error leak in WORKSPACE_PROVISION_FAILED broadcasts) until now. ## What the test pins Drives the **earliest** failure path in `provisionWorkspace` — the global-secrets decrypt failure — so the setup needs only: - one `global_secrets` mock row (with `encryption_version=99` to force `crypto.DecryptVersioned` to error with a string that includes the literal version number) - one `UPDATE workspaces SET status = 'failed'` expectation - a `captureBroadcaster` (already in the test file) injected via `NewWorkspaceHandler` Asserts the captured `WORKSPACE_PROVISION_FAILED` payload: 1. carries the safe canned `"failed to decrypt global secret"` only 2. does NOT contain `"version=99"`, `"platform upgrade required"`, or the global_secret row's `key` value (`FAKE_KEY`) — the three leak markers a regression that interpolates `err.Error()` into the broadcast would surface ## Why not use containsUnsafeString The test file already has a `containsUnsafeString` helper with `"secret"` and `"token"` in its prohibition list. Those substrings match the legitimate redacted message (`"failed to decrypt global secret"`) — appropriate in user-facing copy, NOT a leak. Using the broad helper would either fail the test against the source's own correct message OR require loosening the helper for everyone else. Per-test explicit leak markers keep the assertion precise without weakening shared infrastructure. ## What's still skipped (out of scope for this PR) - `TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast` — same shape but blocked on a different refactor: `provisionWorkspaceCP` routes through `*provisioner.CPProvisioner` (concrete pointer, no interface), so the test would need either an interface extraction or a real CPProvisioner with a mocked HTTP server. Larger scope; deferred. - `TestResolveAndStage_NoInternalErrorsInHTTPErr` — different blocker (`mockPluginsSources` vs `*plugins.Registry` type mismatch). Needs a SourceResolver-side interface refactor. Both still carry their `t.Skip` notes documenting the remaining work. ## Test plan - [x] New test passes - [x] Full handlers package suite still green (`go test ./internal/handlers/`) - [x] No changes to production code — pure test addition 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../handlers/workspace_provision_test.go | 72 ++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/workspace-server/internal/handlers/workspace_provision_test.go b/workspace-server/internal/handlers/workspace_provision_test.go index 5297fd15..8dd34a80 100644 --- a/workspace-server/internal/handlers/workspace_provision_test.go +++ b/workspace-server/internal/handlers/workspace_provision_test.go @@ -1111,9 +1111,77 @@ func containsUnsafeString(v interface{}) bool { // TestProvisionWorkspace_NoInternalErrorsInBroadcast asserts that provisionWorkspace // never leaks internal error details in WORKSPACE_PROVISION_FAILED broadcasts. -// Regression test for issue #1206. +// Regression test for issue #1206 — drives the global-secrets decrypt-fail +// branch (the earliest failure path in provisionWorkspace) and asserts the +// captured broadcast payload contains the safe canned message ONLY, with +// none of the raw decrypt-error wording leaking through. +// +// Why drive the decrypt-fail path specifically: +// - It runs BEFORE workspace_secrets, env-mutator, provisioner config build, +// and the actual provisioner.Provision call — so the test setup needs +// only one mock query (global_secrets) and one UPDATE expectation. +// - The decrypted error string returned by crypto.DecryptVersioned for a +// bogus encryption_version contains the literal version number; if a +// refactor regresses the redaction (e.g. someone passes err.Error() +// verbatim into the broadcast payload), this test catches it without +// having to stand up the full provisioner stack. func TestProvisionWorkspace_NoInternalErrorsInBroadcast(t *testing.T) { - t.Skip("TODO: write the test body. The interface blocker (#1814) is fixed — captureBroadcaster now satisfies events.EventEmitter and can be passed to NewWorkspaceHandler. The remaining work is constructing the provisionWorkspace failure path + asserting captured payload doesn't contain unsafeErrorStrings.") + mock := setupTestDB(t) + + // Mock global_secrets returns ONE row with encryption_version=99. + // crypto.DecryptVersioned errors on unknown version with a string + // that includes "version=99" — concrete-but-safe payload to verify + // the broadcast only carries the canned message. + mock.ExpectQuery(`SELECT key, encrypted_value, encryption_version FROM global_secrets`). + WillReturnRows(sqlmock.NewRows([]string{"key", "encrypted_value", "encryption_version"}). + AddRow("FAKE_KEY", []byte("any-bytes"), 99)) + // On decrypt failure provisionWorkspace also marks the workspace as + // failed via UPDATE workspaces. Match-anything on the args so the + // test isn't coupled to the exact UPDATE column order. + mock.ExpectExec(`UPDATE workspaces SET status = 'failed'`). + WithArgs(sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + cap := &captureBroadcaster{} + handler := NewWorkspaceHandler(cap, nil, "http://localhost:8080", t.TempDir()) + + handler.provisionWorkspace("ws-1206", "/nonexistent/template", nil, models.CreateWorkspacePayload{ + Name: "ws-1206", + Tier: 1, + }) + + if cap.lastData == nil { + t.Fatal("expected RecordAndBroadcast to capture data on decrypt failure; got nothing") + } + if got := cap.lastData["error"]; got != "failed to decrypt global secret" { + t.Errorf("broadcast carried unexpected error message %q — should be the safe canned string", got) + } + // containsUnsafeString is intentionally NOT used here: its + // "secret" / "token" entries match the legitimate redacted + // messages (e.g. "failed to decrypt global secret" itself) — those + // strings are appropriate in user-facing copy. The actual leak + // vector for THIS code path is the raw DecryptVersioned error + // string ("version=99", "platform upgrade required"); pin each + // of those explicitly so a future regression that interpolates + // err.Error() into the payload fails this test. + for _, v := range cap.lastData { + s, ok := v.(string) + if !ok { + continue + } + for _, leakMarker := range []string{ + "version=99", // raw DecryptVersioned error head + "platform upgrade required", // raw DecryptVersioned error tail + "FAKE_KEY", // global_secrets row's key column + } { + 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) + } } // TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast asserts that