diff --git a/workspace-server/internal/handlers/platform_agent.go b/workspace-server/internal/handlers/platform_agent.go index 0cb6d249..386cf266 100644 --- a/workspace-server/internal/handlers/platform_agent.go +++ b/workspace-server/internal/handlers/platform_agent.go @@ -360,7 +360,10 @@ func (h *WorkspaceHandler) ensureConciergeModel(ctx context.Context, workspaceID // boot). Re-asserting on EVERY provision would silently revert the customer's // pick — exactly the platform-overriding-customer violation the SSOT // directive forbids. - if existing := readStoredModelSecret(ctx, workspaceID); existing != "" { + if existing, readErr := readStoredModelSecret(ctx, workspaceID); readErr != nil { + log.Printf("Provisioner: concierge %s MODEL read failed (failing closed, NOT seeding): %v", workspaceID, readErr) + return + } else if existing != "" { return // explicit model already set; never overwrite the customer's choice } @@ -392,23 +395,39 @@ func (h *WorkspaceHandler) ensureConciergeModel(ctx context.Context, workspaceID } } -// readStoredModelSecret returns the decrypted MODEL workspace_secret, or "" when -// none is stored (or on any read/decrypt error — treated as "unset" so a -// transient miss re-seeds rather than wedges). Used by ensureConciergeModel to -// decide seed-vs-respect. -func readStoredModelSecret(ctx context.Context, workspaceID string) string { +// readStoredModelSecret returns the decrypted MODEL workspace_secret. +// The second return value distinguishes the three observed states so the caller +// can fail closed rather than fail open on a transient error: +// +// - (value, nil): a secret is stored and decrypted successfully → caller +// respects the existing model and skips the seed. +// - ("", nil): no row exists for this workspace/key → caller may re-seed +// safely (this is the fresh-boot / cleared-secret case). +// - ("", error): the row exists (or the read otherwise succeeded) but the +// decryption failed → caller MUST NOT treat this as "unset" and MUST NOT +// fall back to seeding the platform default. Returning "" without the +// error used to silently overwrite the customer's model pick if the +// secret store later recovered: the seed path would re-fire on the next +// provision and the customer's choice would be lost without any error +// surfaced. Sibling of core#3162 (which closed the same fail-open shape +// on readStoredProviderSecret). +func readStoredModelSecret(ctx context.Context, workspaceID string) (string, error) { var stored []byte var version int - if err := db.DB.QueryRowContext(ctx, + err := db.DB.QueryRowContext(ctx, `SELECT encrypted_value, encryption_version FROM workspace_secrets WHERE workspace_id = $1 AND key = 'MODEL'`, - workspaceID).Scan(&stored, &version); err != nil { - return "" - } - dec, err := crypto.DecryptVersioned(stored, version) + workspaceID).Scan(&stored, &version) if err != nil { - return "" + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", fmt.Errorf("readStoredModelSecret scan: %w", err) } - return string(dec) + dec, derr := crypto.DecryptVersioned(stored, version) + if derr != nil { + return "", fmt.Errorf("readStoredModelSecret decrypt: %w", derr) + } + return string(dec), nil } // conciergeProvider is the provider-registry NAME the concierge's declared diff --git a/workspace-server/internal/handlers/platform_agent_test.go b/workspace-server/internal/handlers/platform_agent_test.go index f0379bde..55e1cd81 100644 --- a/workspace-server/internal/handlers/platform_agent_test.go +++ b/workspace-server/internal/handlers/platform_agent_test.go @@ -1140,3 +1140,94 @@ func TestEnsureConciergeProvider_FailsClosedOnReadError(t *testing.T) { } }) } + +// TestEnsureConciergeModel_FailsClosedOnReadError is the sibling regression +// gate for the MODEL half of the fail-open shape (paired with +// TestEnsureConciergeProvider_FailsClosedOnReadError). Same shape as core#3162: +// a transient decrypt/read error on an existing MODEL row used to be collapsed +// into "" and treated as "unset", which would silently overwrite the customer's +// model pick if the secret store later recovered (the seed path would re-fire +// on the next provision and the customer's choice would be lost without any +// error surfaced). The fix returns the error; ensureConciergeModel MUST fail +// closed (return without seeding) so the next provision re-tries rather than +// losing the customer's pick. +func TestEnsureConciergeModel_FailsClosedOnReadError(t *testing.T) { + h := &WorkspaceHandler{} + const modelSelQuery = `SELECT encrypted_value, encryption_version FROM workspace_secrets WHERE workspace_id = \$1 AND key = 'MODEL'` + const secretInsert = `INSERT INTO workspace_secrets` + + t.Run("decrypt error on existing MODEL row fails closed (does NOT seed default)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-decrypt-fail"). + WillReturnRows(sqlmock.NewRows([]string{"encrypted_value", "encryption_version"}). + AddRow([]byte("corrupt-ciphertext-that-cannot-be-decrypted"), 0)) + // NO ExpectExec: the fail-closed path MUST NOT persist MODEL. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-decrypt-fail", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("transient decrypt error seeded MODEL=%q — would silently overwrite the customer's pick", env["MODEL"]) + } + if _, seeded := env["MOLECULE_MODEL"]; seeded { + t.Errorf("transient decrypt error seeded MOLECULE_MODEL=%q", env["MOLECULE_MODEL"]) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations (a MODEL INSERT means the fail-closed path leaked): %v", err) + } + }) + + t.Run("db scan error (non-ErrNoRows) on MODEL fails closed", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-scan-fail"). + WillReturnError(fmt.Errorf("connection refused")) + // NO ExpectExec: fail closed. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-scan-fail", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("DB scan error seeded MODEL=%q", env["MODEL"]) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + }) + + t.Run("sql.ErrNoRows (genuine unset MODEL) proceeds to seed (no regression)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-fresh-unset"). + WillReturnError(sql.ErrNoRows) + mock.ExpectExec(secretInsert). + WithArgs("ws-model-fresh-unset", sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-fresh-unset", env) + + if env["MODEL"] != conciergeDeclaredModel { + t.Errorf("genuine unset did not seed MODEL=%q; got %q (env=%v) — regression on the seed path", conciergeDeclaredModel, env["MODEL"], env) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations (the seed path did not persist): %v", err) + } + }) + + t.Run("existing stored MODEL is respected on successful read (no regression)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-existing"). + WillReturnRows(sqlmock.NewRows([]string{"encrypted_value", "encryption_version"}). + AddRow([]byte("anthropic:claude-opus-4-8"), 0)) + // NO ExpectExec: existing pick wins → no seed. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-existing", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("existing MODEL pick wrongly overwrote with default (env=%v)", env) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + }) +}