From a360b641574f6d61d94878e6a9fdfcd4c9a33986 Mon Sep 17 00:00:00 2001 From: "molecule-ai[bot]" <276602405+molecule-ai[bot]@users.noreply.github.com> Date: Fri, 17 Apr 2026 00:46:17 +0000 Subject: [PATCH] fix(platform): persist secrets envelope from POST /workspaces payload (#568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CreateWorkspacePayload` was missing a `Secrets` field, so any `secrets: { KEY: value }` included in a POST /workspaces body was silently dropped by ShouldBindJSON. Changes: - Add `Secrets map[string]string` field to `CreateWorkspacePayload` - Wrap workspace INSERT in a DB transaction; iterate over secrets, encrypt each value via `crypto.Encrypt`, and upsert into `workspace_secrets` within the same tx — rollback both on any failure - Add `mock.ExpectBegin()`/`mock.ExpectCommit()`/`mock.ExpectRollback()` to all existing Create tests that were missing transaction expectations - Add 3 new tests: WithSecrets_Persists, SecretPersistFails_RollsBack, EmptySecrets_OK Closes #545 Co-authored-by: Molecule AI Backend Engineer Co-authored-by: Claude Sonnet 4.6 --- .../handlers/handlers_additional_test.go | 4 + platform/internal/handlers/handlers_test.go | 6 + platform/internal/handlers/workspace.go | 47 ++++++- platform/internal/handlers/workspace_test.go | 118 +++++++++++++++++- platform/internal/models/workspace.go | 4 + 5 files changed, 176 insertions(+), 3 deletions(-) diff --git a/platform/internal/handlers/handlers_additional_test.go b/platform/internal/handlers/handlers_additional_test.go index edc6513e..1ca55547 100644 --- a/platform/internal/handlers/handlers_additional_test.go +++ b/platform/internal/handlers/handlers_additional_test.go @@ -28,9 +28,11 @@ func TestWorkspaceCreate_WithParentID(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) parentID := "parent-ws-123" + mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). WithArgs(sqlmock.AnyArg(), "Child Agent", nil, 1, "langgraph", sqlmock.AnyArg(), &parentID, nil, "none"). WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). WithArgs(sqlmock.AnyArg(), float64(0), float64(0)). WillReturnResult(sqlmock.NewResult(0, 1)) @@ -61,9 +63,11 @@ func TestWorkspaceCreate_ExplicitClaudeCodeRuntime(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). WithArgs(sqlmock.AnyArg(), "CC Agent", nil, 2, "claude-code", sqlmock.AnyArg(), (*string)(nil), nil, "none"). WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). WithArgs(sqlmock.AnyArg(), float64(10), float64(20)). WillReturnResult(sqlmock.NewResult(0, 1)) diff --git a/platform/internal/handlers/handlers_test.go b/platform/internal/handlers/handlers_test.go index d6cfca9f..c8dae41e 100644 --- a/platform/internal/handlers/handlers_test.go +++ b/platform/internal/handlers/handlers_test.go @@ -248,11 +248,17 @@ func TestWorkspaceCreate(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", "/tmp/configs") + // Expect transaction begin for atomic workspace+secrets creation + mock.ExpectBegin() + // Expect workspace INSERT (uuid is dynamic, use AnyArg for id, runtime, awareness_namespace) mock.ExpectExec("INSERT INTO workspaces"). WithArgs(sqlmock.AnyArg(), "Test Agent", nil, 1, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none"). WillReturnResult(sqlmock.NewResult(0, 1)) + // Expect transaction commit (no secrets in this payload) + mock.ExpectCommit() + // Expect canvas_layouts INSERT mock.ExpectExec("INSERT INTO canvas_layouts"). WithArgs(sqlmock.AnyArg(), float64(100), float64(200)). diff --git a/platform/internal/handlers/workspace.go b/platform/internal/handlers/workspace.go index f003317d..99609482 100644 --- a/platform/internal/handlers/workspace.go +++ b/platform/internal/handlers/workspace.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/crypto" "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" "github.com/Molecule-AI/molecule-monorepo/platform/internal/events" "github.com/Molecule-AI/molecule-monorepo/platform/internal/middleware" @@ -129,17 +130,59 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { return } - // Insert workspace with runtime persisted in DB - _, err := db.DB.ExecContext(ctx, ` + // Begin a transaction so the workspace row and any initial secrets are + // committed atomically. A secret-encrypt or DB error rolls back the + // workspace insert so we never leave a workspace row with missing secrets. + tx, txErr := db.DB.BeginTx(ctx, nil) + if txErr != nil { + log.Printf("Create workspace: begin tx error: %v", txErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create workspace"}) + return + } + + // Insert workspace with runtime persisted in DB (inside transaction) + _, err := tx.ExecContext(ctx, ` INSERT INTO workspaces (id, name, role, tier, runtime, awareness_namespace, status, parent_id, workspace_dir, workspace_access) VALUES ($1, $2, $3, $4, $5, $6, 'provisioning', $7, $8, $9) `, id, payload.Name, role, payload.Tier, payload.Runtime, awarenessNamespace, payload.ParentID, workspaceDir, workspaceAccess) if err != nil { + tx.Rollback() //nolint:errcheck log.Printf("Create workspace error: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create workspace"}) return } + // Persist initial secrets from the create payload (inside same transaction). + // nil/empty map is a no-op. Any failure rolls back the workspace insert + // so we never have a workspace row without its intended secrets. + for k, v := range payload.Secrets { + encrypted, encErr := crypto.Encrypt([]byte(v)) + if encErr != nil { + tx.Rollback() //nolint:errcheck + log.Printf("Create workspace %s: failed to encrypt secret %q: %v", id, k, encErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to encrypt secret: " + k}) + return + } + version := crypto.CurrentEncryptionVersion() + if _, dbErr := tx.ExecContext(ctx, ` + INSERT INTO workspace_secrets (workspace_id, key, encrypted_value, encryption_version) + VALUES ($1, $2, $3, $4) + ON CONFLICT (workspace_id, key) DO UPDATE + SET encrypted_value = $3, encryption_version = $4, updated_at = now() + `, id, k, encrypted, version); dbErr != nil { + tx.Rollback() //nolint:errcheck + log.Printf("Create workspace %s: failed to persist secret %q: %v", id, k, dbErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to save secret: " + k}) + return + } + } + + if commitErr := tx.Commit(); commitErr != nil { + log.Printf("Create workspace %s: transaction commit failed: %v", id, commitErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create workspace"}) + return + } + // Insert canvas layout — non-fatal: workspace can be dragged into position later if _, err := db.DB.ExecContext(ctx, ` INSERT INTO canvas_layouts (workspace_id, x, y) VALUES ($1, $2, $3) diff --git a/platform/internal/handlers/workspace_test.go b/platform/internal/handlers/workspace_test.go index 924621f9..e36665d0 100644 --- a/platform/internal/handlers/workspace_test.go +++ b/platform/internal/handlers/workspace_test.go @@ -146,10 +146,12 @@ func TestWorkspaceCreate_DBInsertError(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) - // Workspace INSERT fails + // Transaction begins, workspace INSERT fails, transaction is rolled back. + mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). WithArgs(sqlmock.AnyArg(), "Failing Agent", nil, 1, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none"). WillReturnError(sql.ErrConnDone) + mock.ExpectRollback() w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -175,10 +177,13 @@ func TestWorkspaceCreate_DefaultsApplied(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + // Transaction wraps the workspace INSERT (no secrets in this request). + mock.ExpectBegin() // Expect workspace INSERT with defaulted tier=1, runtime="langgraph" mock.ExpectExec("INSERT INTO workspaces"). WithArgs(sqlmock.AnyArg(), "Default Agent", nil, 1, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none"). WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() // Expect canvas_layouts INSERT (x=0, y=0 — defaults) mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -215,6 +220,117 @@ func TestWorkspaceCreate_DefaultsApplied(t *testing.T) { } } +// TestWorkspaceCreate_WithSecrets_Persists asserts that secrets in the create +// payload are written to workspace_secrets inside the same transaction as the +// workspace row, and that the handler returns 201. +func TestWorkspaceCreate_WithSecrets_Persists(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + // External workspace: simplest code path — no provisioner goroutine. + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs(sqlmock.AnyArg(), "Hermes Agent", nil, 1, "hermes", sqlmock.AnyArg(), (*string)(nil), nil, "none"). + WillReturnResult(sqlmock.NewResult(0, 1)) + // Secret inserted inside the same transaction. + mock.ExpectExec("INSERT INTO workspace_secrets"). + WithArgs(sqlmock.AnyArg(), "HERMES_API_KEY", sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() + + // canvas_layouts (non-fatal, outside tx) + mock.ExpectExec("INSERT INTO canvas_layouts"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"name":"Hermes Agent","runtime":"hermes","external":true,"secrets":{"HERMES_API_KEY":"sk-test-123"}}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusCreated { + t.Errorf("expected status 201, got %d: %s", w.Code, w.Body.String()) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestWorkspaceCreate_SecretPersistFails_RollsBack asserts that a DB error +// while persisting a secret causes the entire transaction to roll back and +// the handler to return 500. The workspace row must NOT be committed. +func TestWorkspaceCreate_SecretPersistFails_RollsBack(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO workspace_secrets"). + WillReturnError(sql.ErrConnDone) // DB failure while writing secret + mock.ExpectRollback() // workspace insert must be rolled back + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"name":"Rollback Agent","secrets":{"OPENAI_API_KEY":"sk-fail"}}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusInternalServerError { + t.Errorf("expected status 500, got %d: %s", w.Code, w.Body.String()) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestWorkspaceCreate_EmptySecrets_OK asserts that an empty secrets map (or +// no secrets key at all) creates the workspace normally without touching +// workspace_secrets. +func TestWorkspaceCreate_EmptySecrets_OK(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WillReturnResult(sqlmock.NewResult(0, 1)) + // No ExpectExec for workspace_secrets — empty map must be a no-op. + mock.ExpectCommit() + mock.ExpectExec("INSERT INTO canvas_layouts"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"name":"No Secrets Agent","external":true,"secrets":{}}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusCreated { + t.Errorf("expected status 201, got %d: %s", w.Code, w.Body.String()) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // ==================== GET /workspaces (List) ==================== func TestWorkspaceList_Empty(t *testing.T) { diff --git a/platform/internal/models/workspace.go b/platform/internal/models/workspace.go index 7a688fc8..4bf9ed9a 100644 --- a/platform/internal/models/workspace.go +++ b/platform/internal/models/workspace.go @@ -63,6 +63,10 @@ type CreateWorkspacePayload struct { WorkspaceDir string `json:"workspace_dir"` // host path to mount as /workspace (empty = isolated volume) WorkspaceAccess string `json:"workspace_access"` // "none" (default), "read_only", or "read_write" — see #65 ParentID *string `json:"parent_id"` + // Secrets is an optional map of key→plaintext-value pairs to persist as + // workspace secrets at creation time. Stored encrypted (same path as + // POST /workspaces/:id/secrets). Nil/empty map is a no-op. + Secrets map[string]string `json:"secrets"` Canvas struct { X float64 `json:"x"` Y float64 `json:"y"`