fix: reject duplicate env var keys with 400 instead of deduplicating
All checks were successful
Check / check (pull_request) Successful in 3m18s
All checks were successful
Check / check (pull_request) Successful in 3m18s
Replace deduplicateEnvPairs with validateEnvPairs that returns a 400 Bad Request error when duplicate keys are submitted, instead of silently deduplicating (last wins). Ref: #158
This commit is contained in:
@@ -912,13 +912,12 @@ type envPairJSON struct {
|
|||||||
// envVarMaxBodyBytes is the maximum allowed request body size for env var saves (1 MB).
|
// envVarMaxBodyBytes is the maximum allowed request body size for env var saves (1 MB).
|
||||||
const envVarMaxBodyBytes = 1 << 20
|
const envVarMaxBodyBytes = 1 << 20
|
||||||
|
|
||||||
// deduplicateEnvPairs validates and deduplicates env var pairs.
|
// validateEnvPairs validates env var pairs.
|
||||||
// It rejects empty keys (returns a non-empty error string) and
|
// It rejects empty keys and duplicate keys (returns a non-empty error string).
|
||||||
// deduplicates by key, keeping the last occurrence.
|
func validateEnvPairs(pairs []envPairJSON) ([]models.EnvVarPair, string) {
|
||||||
func deduplicateEnvPairs(pairs []envPairJSON) ([]models.EnvVarPair, string) {
|
seen := make(map[string]bool, len(pairs))
|
||||||
seen := make(map[string]int, len(pairs))
|
|
||||||
|
|
||||||
var result []models.EnvVarPair
|
result := make([]models.EnvVarPair, 0, len(pairs))
|
||||||
|
|
||||||
for _, p := range pairs {
|
for _, p := range pairs {
|
||||||
trimmedKey := strings.TrimSpace(p.Key)
|
trimmedKey := strings.TrimSpace(p.Key)
|
||||||
@@ -926,12 +925,13 @@ func deduplicateEnvPairs(pairs []envPairJSON) ([]models.EnvVarPair, string) {
|
|||||||
return nil, "empty environment variable key is not allowed"
|
return nil, "empty environment variable key is not allowed"
|
||||||
}
|
}
|
||||||
|
|
||||||
if idx, exists := seen[trimmedKey]; exists {
|
if seen[trimmedKey] {
|
||||||
result[idx] = models.EnvVarPair{Key: trimmedKey, Value: p.Value}
|
return nil, "duplicate environment variable key: " + trimmedKey
|
||||||
} else {
|
|
||||||
seen[trimmedKey] = len(result)
|
|
||||||
result = append(result, models.EnvVarPair{Key: trimmedKey, Value: p.Value})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
seen[trimmedKey] = true
|
||||||
|
|
||||||
|
result = append(result, models.EnvVarPair{Key: trimmedKey, Value: p.Value})
|
||||||
}
|
}
|
||||||
|
|
||||||
return result, ""
|
return result, ""
|
||||||
@@ -941,7 +941,7 @@ func deduplicateEnvPairs(pairs []envPairJSON) ([]models.EnvVarPair, string) {
|
|||||||
// It reads a JSON array of {key, value} objects from the request body,
|
// 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
|
// deletes all existing env vars for the app, and inserts the full
|
||||||
// submitted set atomically within a database transaction.
|
// submitted set atomically within a database transaction.
|
||||||
// Duplicate keys are deduplicated server-side (last occurrence wins).
|
// Duplicate keys are rejected with a 400 Bad Request error.
|
||||||
func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
||||||
return func(writer http.ResponseWriter, request *http.Request) {
|
return func(writer http.ResponseWriter, request *http.Request) {
|
||||||
appID := chi.URLParam(request, "id")
|
appID := chi.URLParam(request, "id")
|
||||||
@@ -967,7 +967,7 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
modelPairs, validationErr := deduplicateEnvPairs(pairs)
|
modelPairs, validationErr := validateEnvPairs(pairs)
|
||||||
if validationErr != "" {
|
if validationErr != "" {
|
||||||
h.respondJSON(writer, request, map[string]string{
|
h.respondJSON(writer, request, map[string]string{
|
||||||
"error": validationErr,
|
"error": validationErr,
|
||||||
|
|||||||
@@ -664,15 +664,15 @@ func TestHandleEnvVarSaveEmptyKeyRejected(t *testing.T) {
|
|||||||
assert.Equal(t, http.StatusBadRequest, recorder.Code)
|
assert.Equal(t, http.StatusBadRequest, recorder.Code)
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestHandleEnvVarSaveDuplicateKeyDedup verifies that when the client
|
// TestHandleEnvVarSaveDuplicateKeyRejected verifies that when the client
|
||||||
// sends duplicate keys, the server deduplicates them (last wins).
|
// sends duplicate keys, the server rejects them with 400 Bad Request.
|
||||||
func TestHandleEnvVarSaveDuplicateKeyDedup(t *testing.T) {
|
func TestHandleEnvVarSaveDuplicateKeyRejected(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
testCtx := setupTestHandlers(t)
|
testCtx := setupTestHandlers(t)
|
||||||
createdApp := createTestApp(t, testCtx, "envvar-dedup-app")
|
createdApp := createTestApp(t, testCtx, "envvar-dedup-app")
|
||||||
|
|
||||||
// Send two entries with the same key — last should win
|
// Send two entries with the same key — should be rejected
|
||||||
body := `[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},{"key":"FOO","value":"second"}]`
|
body := `[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},{"key":"FOO","value":"second"}]`
|
||||||
|
|
||||||
r := chi.NewRouter()
|
r := chi.NewRouter()
|
||||||
@@ -688,21 +688,8 @@ func TestHandleEnvVarSaveDuplicateKeyDedup(t *testing.T) {
|
|||||||
recorder := httptest.NewRecorder()
|
recorder := httptest.NewRecorder()
|
||||||
r.ServeHTTP(recorder, request)
|
r.ServeHTTP(recorder, request)
|
||||||
|
|
||||||
assert.Equal(t, http.StatusOK, recorder.Code)
|
assert.Equal(t, http.StatusBadRequest, recorder.Code)
|
||||||
|
assert.Contains(t, recorder.Body.String(), "duplicate environment variable key: FOO")
|
||||||
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
|
// TestHandleEnvVarSaveCrossAppIsolation verifies that posting env vars
|
||||||
|
|||||||
Reference in New Issue
Block a user