diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 9064951..b10de59 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -909,10 +909,39 @@ type envPairJSON struct { Value string `json:"value"` } +// envVarMaxBodyBytes is the maximum allowed request body size for env var saves (1 MB). +const envVarMaxBodyBytes = 1 << 20 + +// deduplicateEnvPairs validates and deduplicates env var pairs. +// It rejects empty keys (returns a non-empty error string) and +// deduplicates by key, keeping the last occurrence. +func deduplicateEnvPairs(pairs []envPairJSON) ([]models.EnvVarPair, string) { + seen := make(map[string]int, len(pairs)) + + var result []models.EnvVarPair + + for _, p := range pairs { + trimmedKey := strings.TrimSpace(p.Key) + if trimmedKey == "" { + return nil, "empty environment variable key is not allowed" + } + + if idx, exists := seen[trimmedKey]; exists { + result[idx] = models.EnvVarPair{Key: trimmedKey, Value: p.Value} + } else { + seen[trimmedKey] = len(result) + result = append(result, models.EnvVarPair{Key: trimmedKey, Value: p.Value}) + } + } + + return result, "" +} + // 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. +// Duplicate keys are deduplicated server-side (last occurrence wins). func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { appID := chi.URLParam(request, "id") @@ -924,38 +953,32 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { return } + // Limit request body size to prevent abuse + request.Body = http.MaxBytesReader(writer, request.Body, envVarMaxBodyBytes) + var pairs []envPairJSON decodeErr := json.NewDecoder(request.Body).Decode(&pairs) if decodeErr != nil { - http.Error(writer, "Bad Request", http.StatusBadRequest) + h.respondJSON(writer, request, map[string]string{ + "error": "invalid request body", + }, http.StatusBadRequest) return } - // Validate: reject entries with empty keys - var modelPairs []models.EnvVarPair + modelPairs, validationErr := deduplicateEnvPairs(pairs) + if validationErr != "" { + h.respondJSON(writer, request, map[string]string{ + "error": validationErr, + }, http.StatusBadRequest) - 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, - }) + return } - // Atomically replace all env vars in a transaction - ctx := request.Context() - - replaceErr := models.ReplaceEnvVarsByAppID(ctx, h.db, application.ID, modelPairs) + replaceErr := models.ReplaceEnvVarsByAppID( + request.Context(), 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{ diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index a7636f0..178d200 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -664,6 +664,148 @@ func TestHandleEnvVarSaveEmptyKeyRejected(t *testing.T) { assert.Equal(t, http.StatusBadRequest, recorder.Code) } +// TestHandleEnvVarSaveDuplicateKeyDedup verifies that when the client +// sends duplicate keys, the server deduplicates them (last wins). +func TestHandleEnvVarSaveDuplicateKeyDedup(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "envvar-dedup-app") + + // Send two entries with the same key — last should win + body := `[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},{"key":"FOO","value":"second"}]` + + 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) + + envVars, err := models.FindEnvVarsByAppID( + context.Background(), testCtx.database, createdApp.ID, + ) + require.NoError(t, err) + assert.Len(t, envVars, 2, "duplicate key should be deduplicated") + + keys := make(map[string]string) + for _, ev := range envVars { + keys[ev.Key] = ev.Value + } + + assert.Equal(t, "second", keys["FOO"], "last occurrence should win") + assert.Equal(t, "bar", keys["BAR"]) +} + +// TestHandleEnvVarSaveCrossAppIsolation verifies that posting env vars +// to appA's endpoint does not affect appB's env vars (IDOR prevention). +func TestHandleEnvVarSaveCrossAppIsolation(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + appA := createTestApp(t, testCtx, "envvar-iso-appA") + appB := createTestApp(t, testCtx, "envvar-iso-appB") + + // Give appB some env vars + for _, kv := range [][2]string{{"B_KEY1", "b_val1"}, {"B_KEY2", "b_val2"}} { + ev := models.NewEnvVar(testCtx.database) + ev.AppID = appB.ID + ev.Key = kv[0] + ev.Value = kv[1] + require.NoError(t, ev.Save(context.Background())) + } + + // POST new env vars to appA's endpoint + body := `[{"key":"A_KEY","value":"a_val"}]` + + r := chi.NewRouter() + r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) + + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+appA.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 appA has exactly what we sent + appAVars, err := models.FindEnvVarsByAppID( + context.Background(), testCtx.database, appA.ID, + ) + require.NoError(t, err) + assert.Len(t, appAVars, 1) + assert.Equal(t, "A_KEY", appAVars[0].Key) + + // Verify appB's env vars are completely untouched + appBVars, err := models.FindEnvVarsByAppID( + context.Background(), testCtx.database, appB.ID, + ) + require.NoError(t, err) + assert.Len(t, appBVars, 2, "appB env vars must not be affected") + + bKeys := make(map[string]string) + for _, ev := range appBVars { + bKeys[ev.Key] = ev.Value + } + + assert.Equal(t, "b_val1", bKeys["B_KEY1"]) + assert.Equal(t, "b_val2", bKeys["B_KEY2"]) +} + +// TestHandleEnvVarSaveBodySizeLimit verifies that a request body +// exceeding the 1 MB limit is rejected. +func TestHandleEnvVarSaveBodySizeLimit(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "envvar-sizelimit-app") + + // Build a JSON body that exceeds 1 MB + // Each entry is ~30 bytes; 40000 entries ≈ 1.2 MB + var sb strings.Builder + + sb.WriteString("[") + + for i := range 40000 { + if i > 0 { + sb.WriteString(",") + } + + sb.WriteString(`{"key":"K` + strconv.Itoa(i) + `","value":"val"}`) + } + + sb.WriteString("]") + + r := chi.NewRouter() + r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) + + request := httptest.NewRequest( + http.MethodPost, + "/apps/"+createdApp.ID+"/env", + strings.NewReader(sb.String()), + ) + request.Header.Set("Content-Type", "application/json") + + recorder := httptest.NewRecorder() + r.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusBadRequest, recorder.Code, + "oversized body should be rejected with 400") +} + // TestDeleteLabelOwnershipVerification tests that deleting a label // via another app's URL path returns 404 (IDOR prevention). func TestDeleteLabelOwnershipVerification(t *testing.T) {