[fix] neutralize COMPOSE_PROFILES leak, log swallowed persist error - #183
Merged
Conversation
Post-merge review of #180 found two issues in handleDeployProject: - An operator's own environment or a project's .env file can set COMPOSE_PROFILES, which docker compose falls back to whenever no --profile flag is on the command line (i.e. an empty profile selection) — activating more than what gets persisted as "last deployed profiles" and misbadging a running service "Not in active profile". ComposeUpFiles now sets COMPOSE_PROFILES="" on the subprocess so the explicit --profile flags are the sole source. - A failed SetLastDeployedProfiles write was completely swallowed. It still doesn't fail the deploy response, but now logs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix-forward from a post-merge review of #180 (Compose profiles: deployed-vs-selected state + badges), which surfaced two findings:
COMPOSE_PROFILEScan activate more than what gets persisted.handleDeployProjectpersists exactlybody.Profilesas "last deployed profiles". Butdocker compose upalso honorsCOMPOSE_PROFILESfrom the server process's own environment or a project's own.envfile (compose auto-loads.envfrom its working directory). Verified empirically against a realdocker compose up(Compose v5.4.0): once any--profileflag is passed, Compose uses it exclusively and ignoresCOMPOSE_PROFILESentirely — so the leak specifically happens when no profiles are selected (no--profileflag at all), where Compose falls back toCOMPOSE_PROFILESand can activate services the deploy request never asked for. Those services then get misbadged "Not in active profile" even though they're genuinely running — the exact bug [add] compose profiles: deployed-vs-selected state + badges #180 was built to fix, reintroduced by a side channel.Fix:
ComposeUpFilesnow always setsCOMPOSE_PROFILES=(empty) on the subprocess environment, so the explicit--profileflags (built from the caller's authoritativeprofilesargument) are the sole profile source. Confirmed empirically that a process-env override wins over a project's own.envfile too (shell/process env takes precedence over.envfor compose-level variables) — details and the full precedence order are written up indocs/gotchas.md.The persist error for "last deployed profiles" was completely swallowed. Kept the existing behavior (a storage hiccup must not turn a successful deploy into a reported failure), but the failure is no longer invisible — it's now logged server-side, matching the existing convention for other best-effort store writes in this codebase (e.g.
monitor: insert alert event: %v). Decided a server log is the right layer rather than also polluting theproject.deployaudit entry — that trail records user actions, and this is an infra hiccup unrelated to what the user did.Type of change
Checklist
go test -short ./...andgo vet ./...passgofmtgate is clean (gofmt -l $(git ls-files '*.go')after staging)cd web && npx tsc --noEmit) — N/A (noweb/srcchanges)web/dist— N/A (noweb/srcchanges)docs/and added aCHANGELOG.mdentry for user-facing changesNotes for reviewers
TestHandleDeployProject_EnvFileComposeProfilesDoesNotLeakIndrives a realdocker compose upagainst a fixture project whose.envsetsCOMPOSE_PROFILES=extra(a profile not in the deploy request) and asserts, viadocker pson the actual compose-labeled containers (not just the persisted DB value), that the extra service never started. Mutation-tested: reverting theCOMPOSE_PROFILES=override makes it fail with the extra service running.TestHandleDeployProject_PersistFailureIsLoggedNotSwallowedforcesSetLastDeployedProfilesto fail against a real sqlite write, by opening a second raw connection to the same db file and installing a trigger that aborts writes to just that one column on just that project's row (closing the whole store, asTestRBACFailsClosedOnStoreErrordoes elsewhere, would also break the handler's earlier reads before it ever reaches the write under test). Asserts the deploy response still reportsok:truewhile the failure shows up in the log. Mutation-tested: removing thelog.Printfcall makes it fail.go test ./internal/api/... ./internal/docker/... ./internal/store/...(no-short, real Docker daemon) in addition togo test -short ./...— both green.