fix: dedup env var keys, add IDOR test, enforce body size limit
All checks were successful
Check / check (pull_request) Successful in 3m30s
All checks were successful
Check / check (pull_request) Successful in 3m30s
- Server-side duplicate key dedup (last wins) via deduplicateEnvPairs helper - Cross-app isolation test verifying env var save scopes by app_id - http.MaxBytesReader wraps request body with 1MB limit - Test for oversized body rejection (400)
This commit is contained in:
@@ -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)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// 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",
|
||||
"error": "invalid request body",
|
||||
}, http.StatusBadRequest)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
modelPairs = append(modelPairs, models.EnvVarPair{
|
||||
Key: trimmedKey,
|
||||
Value: p.Value,
|
||||
})
|
||||
modelPairs, validationErr := deduplicateEnvPairs(pairs)
|
||||
if validationErr != "" {
|
||||
h.respondJSON(writer, request, map[string]string{
|
||||
"error": validationErr,
|
||||
}, http.StatusBadRequest)
|
||||
|
||||
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{
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user