diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 7bbce06..9064951 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -903,50 +903,69 @@ func (h *Handlers) addKeyValueToApp( http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) } -// HandleEnvVarAdd handles adding an environment variable. -func (h *Handlers) HandleEnvVarAdd() http.HandlerFunc { - return func(writer http.ResponseWriter, request *http.Request) { - h.addKeyValueToApp( - writer, - request, - func(ctx context.Context, application *models.App, key, value string) error { - envVar := models.NewEnvVar(h.db) - envVar.AppID = application.ID - envVar.Key = key - envVar.Value = value - - return envVar.Save(ctx) - }, - ) - } +// envPairJSON represents a key-value pair in the JSON request body. +type envPairJSON struct { + Key string `json:"key"` + Value string `json:"value"` } -// HandleEnvVarDelete handles deleting an environment variable. -func (h *Handlers) HandleEnvVarDelete() http.HandlerFunc { +// HandleEnvVarSave handles bulk saving of all environment variables. +// It reads a JSON array of {key, value} objects from the request body, +// deletes all existing env vars for the app, and inserts the full +// submitted set atomically within a database transaction. +func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { appID := chi.URLParam(request, "id") - envVarIDStr := chi.URLParam(request, "varID") - envVarID, parseErr := strconv.ParseInt(envVarIDStr, 10, 64) - if parseErr != nil { + application, findErr := models.FindApp(request.Context(), h.db, appID) + if findErr != nil || application == nil { http.NotFound(writer, request) return } - envVar, findErr := models.FindEnvVar(request.Context(), h.db, envVarID) - if findErr != nil || envVar == nil || envVar.AppID != appID { - http.NotFound(writer, request) + var pairs []envPairJSON + + decodeErr := json.NewDecoder(request.Body).Decode(&pairs) + if decodeErr != nil { + http.Error(writer, "Bad Request", http.StatusBadRequest) return } - deleteErr := envVar.Delete(request.Context()) - if deleteErr != nil { - h.log.Error("failed to delete env var", "error", deleteErr) + // Validate: reject entries with empty keys + var modelPairs []models.EnvVarPair + + for _, p := range pairs { + trimmedKey := strings.TrimSpace(p.Key) + if trimmedKey == "" { + h.respondJSON(writer, request, map[string]string{ + "error": "empty environment variable key is not allowed", + }, http.StatusBadRequest) + + return + } + + modelPairs = append(modelPairs, models.EnvVarPair{ + Key: trimmedKey, + Value: p.Value, + }) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + // Atomically replace all env vars in a transaction + ctx := request.Context() + + replaceErr := models.ReplaceEnvVarsByAppID(ctx, h.db, application.ID, modelPairs) + if replaceErr != nil { + h.log.Error("failed to replace env vars", "error", replaceErr) + h.respondJSON(writer, request, map[string]string{ + "error": "failed to save environment variables", + }, http.StatusInternalServerError) + + return + } + + h.respondJSON(writer, request, map[string]bool{"ok": true}, http.StatusOK) } } @@ -1205,59 +1224,6 @@ func ValidateVolumePath(p string) error { return nil } -// HandleEnvVarEdit handles editing an existing environment variable. -func (h *Handlers) HandleEnvVarEdit() http.HandlerFunc { - return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - envVarIDStr := chi.URLParam(request, "varID") - - envVarID, parseErr := strconv.ParseInt(envVarIDStr, 10, 64) - if parseErr != nil { - http.NotFound(writer, request) - - return - } - - envVar, findErr := models.FindEnvVar(request.Context(), h.db, envVarID) - if findErr != nil || envVar == nil || envVar.AppID != appID { - http.NotFound(writer, request) - - return - } - - formErr := request.ParseForm() - if formErr != nil { - http.Error(writer, "Bad Request", http.StatusBadRequest) - - return - } - - key := request.FormValue("key") - value := request.FormValue("value") - - if key == "" || value == "" { - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) - - return - } - - envVar.Key = key - envVar.Value = value - - saveErr := envVar.Save(request.Context()) - if saveErr != nil { - h.log.Error("failed to update env var", "error", saveErr) - } - - http.Redirect( - writer, - request, - "/apps/"+appID+"?success=env-updated", - http.StatusSeeOther, - ) - } -} - // HandleLabelEdit handles editing an existing label. func (h *Handlers) HandleLabelEdit() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index c7ccef6..54a3631 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -560,45 +560,87 @@ func testOwnershipVerification(t *testing.T, cfg ownedResourceTestConfig) { cfg.verifyFn(t, testCtx, resourceID) } -// TestDeleteEnvVarOwnershipVerification tests that deleting an env var -// via another app's URL path returns 404 (IDOR prevention). -func TestDeleteEnvVarOwnershipVerification(t *testing.T) { //nolint:dupl // intentionally similar IDOR test pattern +// TestHandleEnvVarSaveBulk tests that HandleEnvVarSave replaces all env vars +// for an app with the submitted set (monolithic delete-all + insert-all). +func TestHandleEnvVarSaveBulk(t *testing.T) { t.Parallel() - testOwnershipVerification(t, ownedResourceTestConfig{ - appPrefix1: "envvar-owner-app", - appPrefix2: "envvar-other-app", - createFn: func(t *testing.T, tc *testContext, ownerApp *models.App) int64 { - t.Helper() + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "envvar-bulk-app") - envVar := models.NewEnvVar(tc.database) - envVar.AppID = ownerApp.ID - envVar.Key = "SECRET" - envVar.Value = "hunter2" - require.NoError(t, envVar.Save(context.Background())) + // Create some pre-existing env vars + for _, kv := range [][2]string{{"OLD_KEY", "old_value"}, {"REMOVE_ME", "gone"}} { + ev := models.NewEnvVar(testCtx.database) + ev.AppID = createdApp.ID + ev.Key = kv[0] + ev.Value = kv[1] + require.NoError(t, ev.Save(context.Background())) + } - return envVar.ID - }, - deletePath: func(appID string, resourceID int64) string { - return "/apps/" + appID + "/env/" + strconv.FormatInt(resourceID, 10) + "/delete" - }, - chiParams: func(appID string, resourceID int64) map[string]string { - return map[string]string{"id": appID, "varID": strconv.FormatInt(resourceID, 10)} - }, - handler: func(h *handlers.Handlers) http.HandlerFunc { return h.HandleEnvVarDelete() }, - verifyFn: func(t *testing.T, tc *testContext, resourceID int64) { - t.Helper() + // Submit a new set as a JSON array of key/value objects + body := `[{"key":"NEW_KEY","value":"new_value"},{"key":"ANOTHER","value":"42"}]` - found, findErr := models.FindEnvVar(context.Background(), tc.database, resourceID) - require.NoError(t, findErr) - assert.NotNil(t, found, "env var should still exist after IDOR attempt") - }, - }) + r := chi.NewRouter() + r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) + + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+createdApp.ID+"/env", + strings.NewReader(body), + ) + request.Header.Set("Content-Type", "application/json") + + recorder := httptest.NewRecorder() + r.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusOK, recorder.Code) + + // Verify old env vars are gone and new ones exist + envVars, err := models.FindEnvVarsByAppID( + context.Background(), testCtx.database, createdApp.ID, + ) + require.NoError(t, err) + assert.Len(t, envVars, 2) + + keys := make(map[string]string) + for _, ev := range envVars { + keys[ev.Key] = ev.Value + } + + assert.Equal(t, "new_value", keys["NEW_KEY"]) + assert.Equal(t, "42", keys["ANOTHER"]) + assert.Empty(t, keys["OLD_KEY"], "old env vars should be deleted") + assert.Empty(t, keys["REMOVE_ME"], "old env vars should be deleted") +} + +// TestHandleEnvVarSaveAppNotFound tests that HandleEnvVarSave returns 404 +// for a non-existent app. +func TestHandleEnvVarSaveAppNotFound(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + + body := `[{"key":"KEY","value":"value"}]` + + r := chi.NewRouter() + r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) + + request := httptest.NewRequest( + http.MethodPost, + "/apps/nonexistent-id/env", + strings.NewReader(body), + ) + request.Header.Set("Content-Type", "application/json") + + recorder := httptest.NewRecorder() + r.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusNotFound, recorder.Code) } // TestDeleteLabelOwnershipVerification tests that deleting a label // via another app's URL path returns 404 (IDOR prevention). -func TestDeleteLabelOwnershipVerification(t *testing.T) { //nolint:dupl // intentionally similar IDOR test pattern +func TestDeleteLabelOwnershipVerification(t *testing.T) { t.Parallel() testOwnershipVerification(t, ownedResourceTestConfig{ @@ -714,41 +756,43 @@ func TestDeletePortOwnershipVerification(t *testing.T) { assert.NotNil(t, found, "port should still exist after IDOR attempt") } -// TestHandleEnvVarDeleteUsesCorrectRouteParam verifies that HandleEnvVarDelete -// reads the "varID" chi URL parameter (matching the route definition {varID}), -// not a mismatched name like "envID". -func TestHandleEnvVarDeleteUsesCorrectRouteParam(t *testing.T) { +// TestHandleEnvVarSaveEmptyClears verifies that submitting an empty JSON +// array deletes all existing env vars for the app. +func TestHandleEnvVarSaveEmptyClears(t *testing.T) { t.Parallel() testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "envvar-clear-app") - createdApp := createTestApp(t, testCtx, "envdelete-param-app") + // Create a pre-existing env var + ev := models.NewEnvVar(testCtx.database) + ev.AppID = createdApp.ID + ev.Key = "DELETE_ME" + ev.Value = "gone" + require.NoError(t, ev.Save(context.Background())) - envVar := models.NewEnvVar(testCtx.database) - envVar.AppID = createdApp.ID - envVar.Key = "DELETE_ME" - envVar.Value = "gone" - require.NoError(t, envVar.Save(context.Background())) - - // Use chi router with the real route pattern to test param name + // Submit empty JSON array r := chi.NewRouter() - r.Post("/apps/{id}/env-vars/{varID}/delete", testCtx.handlers.HandleEnvVarDelete()) + r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) request := httptest.NewRequest( http.MethodPost, - "/apps/"+createdApp.ID+"/env-vars/"+strconv.FormatInt(envVar.ID, 10)+"/delete", - nil, + "/apps/"+createdApp.ID+"/env", + strings.NewReader("[]"), ) - recorder := httptest.NewRecorder() + request.Header.Set("Content-Type", "application/json") + recorder := httptest.NewRecorder() r.ServeHTTP(recorder, request) - assert.Equal(t, http.StatusSeeOther, recorder.Code) + assert.Equal(t, http.StatusOK, recorder.Code) - // Verify the env var was actually deleted - found, findErr := models.FindEnvVar(context.Background(), testCtx.database, envVar.ID) - require.NoError(t, findErr) - assert.Nil(t, found, "env var should be deleted when using correct route param") + // Verify all env vars are gone + envVars, err := models.FindEnvVarsByAppID( + context.Background(), testCtx.database, createdApp.ID, + ) + require.NoError(t, err) + assert.Empty(t, envVars, "all env vars should be deleted") } // TestHandleVolumeAddValidatesPaths verifies that HandleVolumeAdd validates diff --git a/internal/models/env_var.go b/internal/models/env_var.go index 6e5221c..e800cc8 100644 --- a/internal/models/env_var.go +++ b/internal/models/env_var.go @@ -1,4 +1,3 @@ -//nolint:dupl // Active Record pattern - similar structure to label.go is intentional package models import ( @@ -139,3 +138,49 @@ func DeleteEnvVarsByAppID( return err } + +// EnvVarPair is a key-value pair for bulk env var operations. +type EnvVarPair struct { + Key string + Value string +} + +// ReplaceEnvVarsByAppID atomically replaces all env vars for an app +// within a single database transaction. It deletes all existing env +// vars and inserts the provided pairs. If any operation fails, the +// entire transaction is rolled back. +func ReplaceEnvVarsByAppID( + ctx context.Context, + db *database.Database, + appID string, + pairs []EnvVarPair, +) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("beginning transaction: %w", err) + } + + defer func() { _ = tx.Rollback() }() + + _, err = tx.ExecContext(ctx, "DELETE FROM app_env_vars WHERE app_id = ?", appID) + if err != nil { + return fmt.Errorf("deleting env vars: %w", err) + } + + for _, p := range pairs { + _, err = tx.ExecContext(ctx, + "INSERT INTO app_env_vars (app_id, key, value) VALUES (?, ?, ?)", + appID, p.Key, p.Value, + ) + if err != nil { + return fmt.Errorf("inserting env var %q: %w", p.Key, err) + } + } + + err = tx.Commit() + if err != nil { + return fmt.Errorf("committing transaction: %w", err) + } + + return nil +} diff --git a/internal/models/label.go b/internal/models/label.go index e8aadcd..6910421 100644 --- a/internal/models/label.go +++ b/internal/models/label.go @@ -1,4 +1,3 @@ -//nolint:dupl // Active Record pattern - similar structure to env_var.go is intentional package models import ( diff --git a/internal/server/routes.go b/internal/server/routes.go index f02636d..ebddba9 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -82,10 +82,8 @@ func (s *Server) SetupRoutes() { r.Post("/apps/{id}/stop", s.handlers.HandleAppStop()) r.Post("/apps/{id}/start", s.handlers.HandleAppStart()) - // Environment variables - r.Post("/apps/{id}/env-vars", s.handlers.HandleEnvVarAdd()) - r.Post("/apps/{id}/env-vars/{varID}/edit", s.handlers.HandleEnvVarEdit()) - r.Post("/apps/{id}/env-vars/{varID}/delete", s.handlers.HandleEnvVarDelete()) + // Environment variables (monolithic bulk save) + r.Post("/apps/{id}/env", s.handlers.HandleEnvVarSave()) // Labels r.Post("/apps/{id}/labels", s.handlers.HandleLabelAdd()) diff --git a/static/js/app-detail.js b/static/js/app-detail.js index 9778ea8..eec57c5 100644 --- a/static/js/app-detail.js +++ b/static/js/app-detail.js @@ -6,6 +6,103 @@ */ document.addEventListener("alpine:init", () => { + // ============================================ + // Environment Variable Editor Component + // ============================================ + Alpine.data("envVarEditor", (appId) => ({ + vars: [], + editIdx: -1, + editKey: "", + editVal: "", + appId: appId, + + init() { + this.vars = Array.from(this.$el.querySelectorAll(".env-init")).map( + (span) => ({ + key: span.dataset.key, + value: span.dataset.value, + }), + ); + }, + + startEdit(i) { + this.editIdx = i; + this.editKey = this.vars[i].key; + this.editVal = this.vars[i].value; + }, + + saveEdit(i) { + this.vars[i] = { key: this.editKey, value: this.editVal }; + this.editIdx = -1; + this.submitAll(); + }, + + removeVar(i) { + if (!window.confirm("Delete this environment variable?")) { + return; + } + + this.vars.splice(i, 1); + this.submitAll(); + }, + + addVar(keyEl, valEl) { + const k = keyEl.value.trim(); + const v = valEl.value.trim(); + + if (!k) { + return; + } + + this.vars.push({ key: k, value: v }); + this.submitAll(); + }, + + submitAll() { + const csrfInput = this.$el.querySelector( + 'input[name="gorilla.csrf.Token"]', + ); + const csrfToken = csrfInput ? csrfInput.value : ""; + + fetch("/apps/" + this.appId + "/env", { + method: "POST", + headers: { + "Content-Type": "application/json", + "X-CSRF-Token": csrfToken, + }, + body: JSON.stringify( + this.vars.map((e) => ({ key: e.key, value: e.value })), + ), + }) + .then((res) => { + if (res.ok) { + window.location.reload(); + return; + } + res.json() + .then((data) => { + window.alert( + data.error || + "Failed to save environment variables.", + ); + }) + .catch(() => { + window.alert( + "Failed to save environment variables.", + ); + }); + }) + .catch(() => { + window.alert( + "Network error: could not save environment variables.", + ); + }); + }, + })); + + // ============================================ + // App Detail Page Component + // ============================================ Alpine.data("appDetail", (config) => ({ appId: config.appId, currentDeploymentId: config.initialDeploymentId, diff --git a/templates/app_detail.html b/templates/app_detail.html index 2d62de9..b80ad87 100644 --- a/templates/app_detail.html +++ b/templates/app_detail.html @@ -101,9 +101,10 @@ -
+

Environment Variables

- {{if .EnvVars}} + {{range .EnvVars}}{{end}} +