feat: monolithic env var editing with bulk save #158
Reference in New Issue
Block a user
Delete Branch "fix/issue-156-env-vars-404"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR fixes env var handling by consolidating individual add/edit/delete operations into a single monolithic bulk save.
Changes
HandleEnvVarSaveatomically replaces all env vars (DELETE all + INSERT full set).POST /apps/{id}/envendpoint replaces individual env var CRUD routes.DeleteEnvVarsByAppIDandFindEnvVarsByAppIDfor bulk operations.closes #156
closes #163
The 'Add' environment variable form in app_detail.html was posting to /apps/{id}/env, but the route is registered as /apps/{id}/env-vars. This path mismatch caused a 404 when trying to add environment variables from the app detail page. Fix the form action to use the correct /apps/{id}/env-vars path, matching the route defined in routes.go. closes #156i like 'env' better than 'env-vars'.
Review: ❌ FAIL
The fix correctly identifies the root cause (form action path mismatch causing the 404 on the env vars page), but there are two issues that need rework:
1. Scope creep: unrelated JS reformatting
This PR changes 6 files with ~580 insertions and ~540 deletions, but the actual bugfix is a single line change in
templates/app_detail.html(line 149). The other 5 files (static/js/app-detail.js,components.js,dashboard.js,deployment.js,utils.js) are entirely whitespace changes (2-space → 4-space indentation). Even if the reformatting is correct per REPO_POLICIES, it should be in a separate PR — bundling it with a bugfix makes the diff unreadable and conflates two unrelated changes.2. Owner prefers
/envover/env-vars@sneak commented on this PR: "i like 'env' better than 'env-vars'."
The current fix changes the form action to match the existing route (
/env-vars), but the owner prefers the shorter/envpath. The correct approach is to:routes.gofrom/apps/{id}/env-varsto/apps/{id}/env(and similarly for the/{varID}/editand/{varID}/deletesub-routes)app_detail.htmlfrom/env-vars/to/env//apps/{{.App.ID}}/env(which was the original, correct path per sneak's preference)No cheating detected
✅ No changes to Makefile,
.golangci.yml, CI workflows, or test assertions.✅
docker build .passes.Summary
Please rework: strip the JS reformatting into a separate PR, and reverse the fix direction — rename the routes from
env-varstoenvinroutes.goand update the edit/delete forms in the template to match, per sneak's stated preference.bb07622e62to30f81078bdRework Complete
Addressed both review findings:
1. Removed JS reformatting
All 5 JS files (
app-detail.js,components.js,dashboard.js,deployment.js,utils.js) have been reset to theirmainstate. The diff is now only the actual bugfix.2. Changed routes from
/env-varsto/envPer @sneak's preference for
/envover/env-vars:routes.go: Changed all 3 env var route patterns from/apps/{id}/env-varsto/apps/{id}/envtemplates/app_detail.html: Updated edit and delete form actions from/env-vars/to/env//envonmain— left as-is (correct)handlers_test.goto match new/envpathsDiff summary
3 files changed, 7 insertions, 7 deletions — clean and minimal.
docker build .passes (all tests, linting, build).Review: ✅ PASS
The reworked PR cleanly addresses both previous review findings and correctly fixes issue #156.
Checklist
/env-varsto/envinroutes.go(add, edit, delete)/env/not/env-vars/(edit and delete forms updated)/envonmain, unchanged/envrespecteddocker build .passes (tests, lint, fmt-check, build all green)env-varsreferences in routes, templates, or test route setupmain(no rebase needed)Single clean commit:
fix: use /env routes for env var CRUD, fixing 404 on env var formsMinimal, focused, correct. Ready to merge.
@clawbot Please make another PR for the formatting change.
We don’t need to change env vars individually. The env vars can be treated as a monolithic list of keys and values for the purposes of editing. individual env var IDs never need to be exposed. when updating we can simply remove all existing env vars and insert the full post-edit list provided by the client.
Understood — monolithic env var editing (delete-all + insert-all, no individual IDs exposed) is a cleaner approach.
This PR currently fixes just the 404 bug (form action path mismatch). Should I:
The monolithic redesign would touch the handler, template, and likely the DB layer — significantly larger scope than the current 3-file fix.
option 1
Got it — merging this 404 fix as-is. Created issue #163 to track the monolithic env var redesign separately.
update this to solve #163 too
Replace individual env var add/edit/delete with a single bulk save endpoint. The UI now shows a textarea with KEY=VALUE lines. On save, all existing env vars are deleted and the full submitted set is inserted. - Replace HandleEnvVarAdd, HandleEnvVarEdit, HandleEnvVarDelete with HandleEnvVarSave - Collapse 3 routes into single POST /apps/{id}/env - Template uses textarea instead of per-row edit/delete forms - No individual env var IDs exposed in the UI - Extract parseEnvPairs helper to keep cyclomatic complexity low - Use strings.SplitSeq per modernize linter - Update tests for new bulk save behavior closes #156 closes #163fix: correct env var add form action path (404 on env vars page)to feat: monolithic env var editing with bulk saveRework Complete — Monolithic Env Var Editing
Expanded this PR to also solve issue #163 (monolithic env var editing).
What changed
HandleEnvVarAdd,HandleEnvVarEdit,HandleEnvVarDelete) with a singleHandleEnvVarSavehandlerPOST /apps/{id}/env— the previous per-var edit/delete routes are removedKEY=valuelines#-prefixed comment lines are ignoredparseEnvPairshelper to stay under cyclomatic complexity limitdocker build .passes (lint, fmt-check, tests, build all green).4 files changed:
app.go,handlers_test.go,routes.go,app_detail.htmlReview: ❌ FAIL
The backend changes (handler consolidation, route consolidation, bulk delete-all + insert-all) are well-implemented and correct. However, the PR violates an explicit requirement from the repo owner.
Blocking Issue: UI must stay the same
@sneak commented on issue #163:
The PR replaces the table-based UI (per-row key/value display with inline Edit and Delete buttons, plus an Add form) with a single
<textarea>showingKEY=valuelines. This is a completely different UI, not the same one. The requirement is to keep the existing table layout but change only the backend interaction to use monolithic delete-all + insert-all.The correct approach would be:
POST /apps/{id}/envHandleEnvVarSavehandler (which is correctly implemented) processes the full setWhat's correct
HandleEnvVarSavehandler: clean delete-all + insert-all logicparseEnvPairshelper: properly extracted, handles comments and blank linesPOST /apps/{id}/envdocker build .passes (lint, fmt-check, tests, build all green).golangci.yml, or CI configDeleteEnvVarsByAppIDalready existed//nolint:duplcorrectly removed fromTestDeleteLabelOwnershipVerificationsince its duplicate was replacedSummary
Please rework the template (
templates/app_detail.html) to keep the existing table-based UI per sneak's explicit instruction on issue #163. The backend handler, route, and test changes are all good — only the template needs to be revised to preserve the original visual layout while using the new monolithic save endpoint.Reworked per feedback. Key changes:
Restored table-based UI — The env vars section now shows the original table layout with key/value columns, inline edit, and delete buttons. No more textarea.
Monolithic submission via Alpine.js — All env var operations (add, edit, delete) happen client-side in Alpine.js state. A single "Save All" button collects all rows into a hidden textarea field (
KEY=valueper line) and POSTs to/apps/{id}/env.Backend unchanged —
HandleEnvVarSavestill does atomic DELETE all + INSERT all. TheparseEnvPairsfunction parses theKEY=valuetextarea format. Individual env var CRUD endpoints remain removed.Tests pass —
make fmtclean,docker build .(which runsmake check) passes.This now closes both #156 (env var 404 fix) and #163 (monolithic env var editing).
Review: ✅ PASS
The monolithic env var editing rework is clean, correct, and well-tested.
What changed
POST /apps/{id}/envendpoint.HandleEnvVarSavedeletes all existing env vars for the app then inserts the submitted set.parseEnvPairscorrectly handles blank lines, comments, missing=, and values containing=.prepareSubmit()serializes all vars into a hidden textarea asKEY=valuelines.DeleteEnvVarsByAppIDandFindEnvVarsByAppIDadded for bulk operations.Checklist
KEY=valuelistx-textused (notx-html), Go template auto-escapes textarea content, parameterized SQL.golangci.ymlchangesdocker build .passes (fmt-check, lint, test, build all green)Non-blocking note
The delete-all + insert loop in
HandleEnvVarSaveis not wrapped in a database transaction. If the process crashes between delete and inserts, env vars could be lost. Given this is SQLite with a single admin user, the practical risk is very low. TheBeginTxAPI is available in the database package if this is ever worth hardening.Replace the Save All workflow with the original per-action behavior: - Edit row: shows Save/Cancel buttons, submits full set immediately - Delete row: shows confirmation dialog, submits full set immediately - Add row: submits full set immediately on Add click Moves Alpine.js logic into a proper envVarEditor component in app-detail.js. Initializes env var data from hidden span elements with data attributes for safe HTML escaping. All actions collect the complete env var set and POST to the single bulk endpoint POST /apps/{id}/env — no Save All button needed.Rework complete — restored the original table-based env var UI to match the
mainbranch layout:window.confirm()dialog (matching the originalconfirmActionbehavior), submits immediatelyAll three actions collect the complete set of env vars and POST them to the single
POST /apps/{id}/envbulk endpoint. No "Save All" button — each action takes effect immediately, matching the original UX.Implementation:
envVarEditorAlpine.js component instatic/js/app-detail.js<span class="env-init">elements withdata-key/data-valueattributes for safe server→client data transfertemplates/app_detail.htmlandstatic/js/app-detail.jsmodifiedmake fmtanddocker build .both pass.Addresses review feedback re: issue #163 — "the edit page UI should stay the same."
Review: ✅ PASS
Reviewing the latest commit
3f96f4f(fix: match original table UI with immediate per-action submission). This is the final rework after the previous PASS at comment #12807 covered the intermediate "Save All" button version.Requirements Checklist
POST /apps/{id}/env.mainbranch.POST /apps/{id}/envendpoint immediately. No "Save All" button — each action takes effect on its own, matching the original UX.Security
data-key/data-valueattributes on hidden<span>elements, which Go'shtml/templateauto-escapes for attribute context. Alpine'sx-textrenders astextContent(notinnerHTML). Safe.{{ .CSRFField }}. All submissions go through this form.?placeholders).=:strings.Cut(line, "=")splits on first=only, soKEY=val=ueparses correctly.Code Quality
HandleEnvVarSaveis clean: validates app exists, parses form, deletes all, inserts all, redirects.parseEnvPairsproperly handles blank lines,#comments, missing=, and empty keys.envVarEditorcomponent is well-structured withinit(),startEdit(),saveEdit(),removeVar(),addVar(),submitAll().//nolint:duplcorrectly removed fromTestDeleteLabelOwnershipVerificationsince its former duplicate was replaced.Tests
TestHandleEnvVarSaveBulk— verifies old env vars are deleted and new set is inserted.TestHandleEnvVarSaveAppNotFound— verifies 404 for nonexistent app.TestHandleEnvVarSaveEmptyClears— verifies empty submission deletes all env vars.No Cheating
Makefile,.golangci.yml, or.gitea/workflows/check.yml.//nolintdirectives added.FindEnvVarsByAppIDandDeleteEnvVarsByAppIDalready existed onmain).Build & Branch
docker build .passes (fmt-check, lint, test, build all green).main— no rebase needed.Non-blocking note
The delete-all + insert loop in
HandleEnvVarSaveis not wrapped in a database transaction. A crash between DELETE and INSERT could lose env vars. Practical risk is very low for SQLite with a single admin user, but worth noting for future hardening.Clean, focused, correct. Ready to merge.
you shouldn't be creating a single string from them. you can post an array of keys/vals to the server and it can handle the new INSERTs, obviously. you don't need to do any parsing (other than json)
Replace the string-serialized KEY=value format with a proper JSON array of {key, value} objects for the env var save endpoint. Frontend changes: - envVarEditor.submitAll() now uses fetch() with Content-Type: application/json and X-CSRF-Token header instead of form submission - Sends JSON array: [{"key":"FOO","value":"bar"}, ...] - Hidden bulk form replaced with hidden div holding CSRF token - envVarEditor now receives appId parameter for the fetch URL Backend changes: - HandleEnvVarSave reads JSON body via json.NewDecoder instead of parsing form values with parseEnvPairs - Returns JSON {"ok": true} instead of HTTP redirect - Removed parseEnvPairs function and envPair struct entirely - Added envPairJSON struct with json tags for deserialization Tests updated to POST JSON arrays instead of form-encoded strings. Closes #163Reworked per feedback. Changes:
Frontend:
envVarEditor.submitAll()now usesfetch()to POST a JSON array of{"key": ..., "value": ...}objects to/apps/{id}/envwithContent-Type: application/jsonandX-CSRF-Tokenheader. No more string serialization. The hidden bulk form is replaced with a minimal hidden div holding just the CSRF token.Backend:
HandleEnvVarSavereads the JSON array directly viajson.NewDecoder(request.Body).Decode(&pairs)— no string parsing at all. TheparseEnvPairsfunction andenvPairstruct have been removed entirely. Returns JSON{"ok": true}instead of an HTTP redirect (since the client now usesfetch()).Tests: Updated to POST JSON arrays instead of form-encoded
KEY=valuestrings.UI is unchanged — same table layout with Edit/Delete/Add buttons.
Review: ✅ PASS
Reviewing latest commit
df6aad9(refactor: POST env vars as JSON array instead of KEY=value string). This is the round 3 rework addressing sneak's feedback to stop building a single KEY=value string and instead post a JSON array.Requirements Checklist
POST /apps/{id}/env.parseEnvPairsand all string parsing completely removed. Frontend sends[{"key": ..., "value": ...}]JSON array viafetch(). Backend decodes it directly withjson.NewDecoder. No parsing other than JSON./envnot/env-vars.Architecture Review
Frontend (
static/js/app-detail.js+templates/app_detail.html):envVarEditorcomponent manages env var state client-side<span class="env-init">elements withdata-key/data-valueattributes (properly HTML-escaped by Go templates)submitAll()POSTs JSON array withContent-Type: application/jsonandX-CSRF-TokenheaderBackend (
internal/handlers/app.go):envPairJSONstruct withjson:"key"/json:"value"tagsHandleEnvVarSavevalidates app exists (404), decodes JSON body (400), deletes all existing env vars, inserts new set{"ok": true}instead of redirect (appropriate for fetch-based client)Routes (
internal/server/routes.go):POST /apps/{id}/envTests (
internal/handlers/handlers_test.go):TestHandleEnvVarSaveBulk: verifies old env vars are replaced by new setTestHandleEnvVarSaveAppNotFound: verifies 404 for nonexistent appTestHandleEnvVarSaveEmptyClears: verifies empty array clears all env varsSecurity
X-CSRF-Tokenheader. gorilla/csrf validates this by default.html/templateescapesdata-key/data-valueattributes.Docker Build
docker build .passes (includesmake test,make lint,make fmt-check).Branch Status
main(merge base = main HEADe1dc865).All human feedback from PR #158, issue #156, and issue #163 is fully addressed.
@clawbot i want one more final review because this issue is complex.
review agent: i want you to look REALLY HARD at the diff as well as the post-diff code to identify any bugs this may have introduced.
Deep Final Review: ❌ FAIL
This review goes beyond the diff and traces the full request lifecycle, DB interactions, edge cases, and frontend state management per the repo owner's request.
[BLOCKING] Partial insert failure silently returns 200 OK (data loss)
This is a real, reproducible bug introduced by this PR.
Root cause: In
HandleEnvVarSave, afterDeleteEnvVarsByAppIDsucceeds, individualenvVar.Save()calls in the loop may fail, but the handler only logs the error and continues. It always returns{"ok": true}with status 200, regardless of insert failures:How to reproduce with the UNIQUE constraint: The
app_env_varstable hasUNIQUE(app_id, key). If a user edits one env var's key to match another existing key (which the frontend allows --saveEdit()does no duplicate check),submitAll()sends both pairs. The DELETE ALL succeeds, the first INSERT succeeds, the second INSERT fails with a UNIQUE constraint violation, and the handler returns 200 OK. The user sees a success reload but one env var has been silently dropped.Steps to reproduce:
FOO=1andBAR=2BAR, change key toFOO, click SavesubmitAll()sends[{"key":"FOO","value":"1"},{"key":"FOO","value":"2"}]FOO=1(ok), insertsFOO=2-- UNIQUE violation -- logged, skippedFOO=1exists.BAR=2is gone forever.Fix: Either:
db.BeginTx()transaction (the method already exists indatabase.go). Rollback on any error. Return 500 on failure.pairsslice before touching the DB.Option (a) is strongly preferred -- it also protects against crash-between-delete-and-insert data loss.
[ADVISORY] Non-transactional DELETE + INSERT (race condition)
Related to the above. Without a transaction, concurrent requests can interleave:
For a single-admin PaaS this is very low probability, but using
BeginTx()(which is already available) would eliminate it entirely along with fixing the blocking issue above.[ADVISORY] Frontend does not handle non-2xx responses
If the server returns an error (400, 500), the user gets no feedback -- the page stays as-is with stale Alpine.js state that does not match the DB. Suggest adding an
elsebranch that shows an alert or error message, and a.catch()for network failures.[ADVISORY] No server-side validation of empty keys
The old
addKeyValueToApphelper rejected empty keys/values. The newHandleEnvVarSaveinserts whatever the client sends. The frontend'saddVar()checksif (!k) return;for new additions, butsaveEdit()has no such check -- a user could edit a key to be empty. TherequiredHTML attribute helps for the Add form but the edit form usesx-modelwhich will not prevent submission of whitespace-only values.Suggest adding server-side validation: skip or reject pairs where
p.Keyis empty (after trimming whitespace).What is correct
r.Use(s.mw.CSRF())group, and the frontend sends the CSRF token viaX-CSRF-Tokenheader, whichgorilla/csrfchecks by default.?placeholders).FindApp()using the URL{id}param. Since env vars are scoped by app_id (no individual var IDs exposed), there is no IDOR vector. The removed IDOR test (TestDeleteEnvVarOwnershipVerification) is no longer applicable.[]correctly deletes all env vars (tested byTestHandleEnvVarSaveEmptyClears).json.NewDecoder().Decode()returns an error for invalid JSON, which correctly maps tohttp.StatusBadRequest.TestHandleEnvVarSaveAppNotFound.data-keyanddata-valueattributes use Go'shtml/templatepackage which auto-escapes attribute values.x-ifwrapping is correct.<template x-if="vars.length > 0">wraps a single root<div>.POST /apps/{id}/env.make check(lint + fmt-check + test) all green./envroute naming preference -- doneSummary
The architecture and approach are sound. The single blocking issue is the silent data loss when an INSERT fails after DELETE ALL -- specifically reproducible via duplicate keys. The fix is straightforward: wrap in a transaction and return an error on failure. The advisory items (frontend error handling, empty key validation) are lower priority but worth addressing.
Verdict: FAIL -- one blocking bug (partial insert leading to silent data loss). Fix required before merge.
Rework pushed addressing the deep review findings:
BLOCKING fix — transactional env var save:
ReplaceEnvVarsByAppID()ininternal/models/env_var.gothat wraps the DELETE + all INSERTs in a singledb.BeginTx()transaction. If any insert fails (e.g. UNIQUE constraint violation), the entire transaction is rolled back — no more silent data loss.ADVISORY fixes:
submitAll()now handles non-2xx responses and network errors by showing an alert with the server's error message (or a generic fallback).Cleanup:
//nolint:dupldirectives fromenv_var.goandlabel.gosince the files are no longer considered duplicates by the linter (due to the newReplaceEnvVarsByAppIDfunction).All checks pass:
make fmt,make lint,make test,docker build .✅Final Review (Round 4): ✅ PASS
Reviewing commit
5a986aa(fix: transactional env var save, empty key validation, frontend error handling).docker build .passes (fmt-check, lint, tests, build all green).Verification of the Three Targeted Fixes
1. Transactional Save — ✅ Correct
ReplaceEnvVarsByAppID()ininternal/models/env_var.goproperly wraps DELETE + all INSERTs in a singledb.BeginTx()transaction:tx.ExecContext(), notdb.Exec()defer func() { _ = tx.Rollback() }()— the standard Go pattern. Covers: DELETE failure, any INSERT failure, Commit failure. After successful Commit(), Rollback() is a no-op.2. Empty Key Validation — ✅ Correct
Handler validates ALL keys before any database operation:
strings.TrimSpace()applied to each key{"error": "empty environment variable key is not allowed"}3. Frontend Error Handling — ✅ Correct
submitAll()instatic/js/app-detail.js:res.ok— any non-2xx triggers error pathwindow.alert(data.error || fallback)window.location.reload()to refresh server-rendered state500 Actually Returned to Client — ✅ Confirmed
Handler both logs AND responds with
h.respondJSON(writer, request, {error: ...}, http.StatusInternalServerError). TherespondJSONhelper correctly setsContent-Type: application/json, writes the status code, and encodes the body.Full PR Assessment
POST /apps/{id}/env)Content-Type: application/json, array of{key, value}objectsX-CSRF-Tokenheader sent from JS, route is inside CSRF middleware groupHandleEnvVarAdd,HandleEnvVarEdit,HandleEnvVarDeleteall gone/env-varsreferences remain anywheredocker build .TestHandleEnvVarSaveBulk,TestHandleEnvVarSaveAppNotFound,TestHandleEnvVarSaveEmptyClearsAdvisory (Non-Blocking)
Missing test for empty key validation: No test sends a request with an empty key to verify the 400 response. The validation code is correct and straightforward, but a test would strengthen the coverage. Could be a follow-up.
Dead code:
DeleteEnvVarsByAppID()inenv_var.gois now unused — the newReplaceEnvVarsByAppID()does its own DELETE within the transaction. Similarly,FindEnvVar()and service-layerAddEnvVar/DeleteEnvVarare only referenced frominternal/service/app/app.gowhich is no longer called by any handler for env vars. Minor cleanup opportunity.Verdict: PASS. All three targeted fixes are correctly implemented. The transactional save eliminates the data loss bug, the handler properly returns errors to the client, and the frontend displays them. All human feedback across 4 rounds of rework has been addressed. Ready to merge.
@clawbot please fix both advisory issues
@clawbot please respawn the rework agent, i restarted the gateway
@clawbot please respawn the rework agent, i restarted the gateway
Both advisory issues from the review have been addressed:
Removed dead code —
DeleteEnvVarsByAppID()has been removed frominternal/models/env_var.go. It was no longer called anywhere sinceReplaceEnvVarsByAppID()handles deletion internally within its transaction. Confirmed via grep that no references remain.Added missing test —
TestHandleEnvVarSaveEmptyKeyRejectedadded tointernal/handlers/handlers_test.go. It POSTs a JSON array containing an entry with an empty key and verifies the handler returns 400 Bad Request.All tests pass and
docker build .succeeds.Review (Round 5 — Advisory Fixes): ✅ PASS
Focused delta review of commit
609ce1d(fix: remove dead DeleteEnvVarsByAppID and add empty-key 400 test).1. Dead code removal — ✅ Correct
DeleteEnvVarsByAppID()removed frominternal/models/env_var.go(11 lines). Grep confirms zero remaining references anywhere in the codebase. This function was superseded byReplaceEnvVarsByAppID()which handles deletion within its transaction.2. New test — ✅ Correct
TestHandleEnvVarSaveEmptyKeyRejectedininternal/handlers/handlers_test.go:[{"key":"VALID_KEY","value":"ok"},{"key":"","value":"bad"}]setupTestHandlers, creates a dedicated test app3. No other files changed — ✅ Confirmed
Only
env_var.goandhandlers_test.gomodified in this commit.Build & Branch
docker build .passes (fmt-check, lint, tests, build all green)main(no rebase needed)Both advisory items from the Round 4 review are resolved. Ready to merge.
@clawbot pipeline manager, what do you think of this? please be super critical.
Pipeline Manager Assessment
sneak asked me to be super critical, so here goes.
What's Good
ReplaceEnvVarsByAppID) is clean —BeginTx, deferredRollback, explicitCommit. This is the right pattern.Concerns
No duplicate key handling. If the client sends
[{"key":"FOO","value":"a"},{"key":"FOO","value":"b"}], both get inserted. Depending on the schema, either the DB has a UNIQUE constraint (which would rollback the whole transaction — good safety, bad UX since the user gets a generic 500) or it silently stores duplicates (bad). Worth checking the schema. If there's no UNIQUE constraint, consider either deduplicating server-side (last wins) or adding a UNIQUE(app_id, key) constraint.The IDOR test was removed. The old code had
TestDeleteEnvVarOwnershipVerificationwhich tested that you couldn't delete another app's env var via path manipulation. The new bulk endpoint doesn't expose individual var IDs so the specific attack vector is gone — but there's no test verifying that POSTing to/apps/{appA}/envcan't affect appB's env vars. The transactional replace scopes byapp_idso it's safe in practice, but the defensive test is missing.addVardoesn't clear inputs after submit. AfteraddVar(keyEl, valEl), the input fields aren't cleared becausesubmitAll()triggerswindow.location.reload()— so this works in practice, but if the fetch fails, the inputs still have values (which is actually fine for retry). Not a bug, just noting.respondJSONis used but not shown in the diff. I trust it exists since docker build passes, but worth confirming it setsContent-Type: application/jsonproperly.No request body size limit.
json.NewDecoder(request.Body)will happily read a multi-GB POST. For a single-admin PaaS this is low risk, but ahttp.MaxBytesReaderwrapper would be a cheap safety net.Verdict
The code is solid for what it does. Items 1 and 2 are the only ones I'd call real gaps — the rest are hardening. Item 1 (duplicate keys) is the most likely to bite in practice. Everything else is clean.
Up to you whether these warrant another rework round or are fine to address in follow-up issues.
fix 1 2 and 5 please.
Rework pushed addressing the three items from the pipeline manager assessment:
1. Duplicate key dedup (last wins): Extracted
deduplicateEnvPairshelper that iterates the pairs and keeps only the last occurrence of each key. If a client sends[{"key":"FOO","value":"a"},{"key":"FOO","value":"b"}], onlyFOO=bis stored. Test:TestHandleEnvVarSaveDuplicateKeyDedup.2. Cross-app isolation (IDOR) test:
TestHandleEnvVarSaveCrossAppIsolationcreates two apps with env vars, POSTs to appA's endpoint, and verifies appB's env vars are completely untouched.5. Request body size limit:
request.Bodyis wrapped withhttp.MaxBytesReader(writer, request.Body, 1<<20)(1 MB) before JSON decoding. Oversized requests get a 400 response. Test:TestHandleEnvVarSaveBodySizeLimit.All checks pass:
make fmt,make lint,make test,docker build .✅2 files changed:
internal/handlers/app.go,internal/handlers/handlers_test.goReview (Round 6): ✅ PASS
Reviewing commit
eaf3d48(fix: dedup env var keys, add IDOR test, enforce body size limit). This addresses the three items from the pipeline manager's critical assessment.Verification of the Three Targeted Fixes
1. Duplicate Key Dedup — ✅ Correct (last wins)
deduplicateEnvPairsuses aseenmap tracking indices into the result slice. When a duplicate key is encountered, it overwritesresult[idx]with the new value instead of appending:Traced through the test input
[FOO=first, BAR=bar, FOO=second]:Final: FOO=second ✅, BAR=bar ✅, length=2 ✅. Last occurrence wins, not first.
The test
TestHandleEnvVarSaveDuplicateKeyDedupverifies this end-to-end through the handler into the database.2. Cross-App Isolation (IDOR) Test — ✅ Correct
TestHandleEnvVarSaveCrossAppIsolation:createTestAppB_KEY1=b_val1,B_KEY2=b_val2)[{"key":"A_KEY","value":"a_val"}]to appA's endpointA_KEY)This confirms the
DELETE FROM app_env_vars WHERE app_id = ?inReplaceEnvVarsByAppIDcorrectly scopes by app_id.3. Request Body Size Limit — ✅ Correct
MaxBytesReaderis applied beforejson.NewDecoder— when the body exceeds 1 MB,Decodereturns a*http.MaxBytesError, which the handler catches and returns 400.The test
TestHandleEnvVarSaveBodySizeLimitgenerates ~1.2 MB of JSON (40,000 entries at ~30 bytes each) and asserts 400 Bad Request.Full PR Checklist
POST /apps/{id}/env)deduplicateEnvPairshelperMaxBytesReaderbeforejson.NewDecoderReplaceEnvVarsByAppIDusesBeginTx+ deferredRollbackContent-Type: application/json, decoded withjson.NewDecoderX-CSRF-Tokenheader, route inside CSRF middleware group.golangci.yml, CI config//nolintdirectivesdocker build .Tests
TestHandleEnvVarSaveBulk— replaces old env vars with new setTestHandleEnvVarSaveAppNotFound— 404 for nonexistent appTestHandleEnvVarSaveEmptyClears— empty array deletes allTestHandleEnvVarSaveEmptyKeyRejected— empty key returns 400TestHandleEnvVarSaveDuplicateKeyDedup— last occurrence winsTestHandleEnvVarSaveCrossAppIsolation— appB untouched when POSTing to appATestHandleEnvVarSaveBodySizeLimit— >1MB body returns 400All 7 env var tests verify distinct behaviors. No weakened assertions.
Verdict: PASS. All three pipeline manager findings are correctly addressed. The full PR is clean, well-tested, and ready to merge.
wrong. duplicate keys is an error.
Reworked: duplicate env var keys are now rejected with 400 Bad Request instead of being silently deduplicated.
Changes:
deduplicateEnvPairswithvalidateEnvPairs— returns error"duplicate environment variable key: FOO"on first duplicate foundTestHandleEnvVarSaveDuplicateKeyRejectedto assert 400 status and verify the error messagedocker build .succeedsReview (Round 7): ✅ PASS
Reviewing commit
316ccae(fix: reject duplicate env var keys with 400 instead of deduplicating).Change Summary
deduplicateEnvPairsreplaced withvalidateEnvPairs. The old function used an index map to overwrite earlier occurrences (last wins). The new function uses a simplemap[string]booland returns an error immediately on the first duplicate key.Verification
validateEnvPairsreturns"duplicate environment variable key: " + trimmedKeywhich the handler passes through as a 400 JSON response.TestHandleEnvVarSaveDuplicateKeyRejectedsends[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},{"key":"FOO","value":"second"}], assertshttp.StatusBadRequest, and checksrecorder.Body.String()contains"duplicate environment variable key: FOO".dedup/deduplicate/last wins/last occurrencein Go source (only a harmless test app name"envvar-dedup-app").docker build .passes: ✅ fmt-check, lint, tests, build all green.Diff Quality
Minimal and focused — 2 files changed, only the dedup→validate logic and corresponding test. No unrelated changes, no new
//nolintdirectives, no Makefile/CI modifications.Ready to merge.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.