test(provisioning): pin no-internal-errors-in-broadcast for global-secret decrypt path (#1814)

[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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-26 22:31:30 -07:00
parent 751b6aa2d9
commit e25b8a508e

View File

@ -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