Repository navigation
fix(docker): treat an empty db password as missing in compose guards - #1881
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Failed to generate code suggestions for PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Auto-approved: Fixes the compose guard to reject empty passwords, making the intended safety check work. The change is bounded, clearly beneficial, and the implementation is straightforward.
Re-trigger cubic
The compose guards used `${POSTGRES_PASSWORD?err}`, which Docker Compose
fires only when the variable is absent. A present-but-empty value passes
straight through, so `POSTGRES_PASSWORD=` in .env would start Postgres
with an empty password and no warning. Verified against docker-compose
5.1.4: with `?`, unset errors and empty passes; with `:?`, both error.
Switch both interpolation sites to `:?` so the guard means what it looks
like it means, and document generating the value in the README setup
block. The password goes straight into a postgres:// URL, so it has to be
URL-safe, hence `openssl rand -hex 24`.
Context and the rejected alternatives are in
decisions/2026-07-26-postgres-password-env-guard.md.
f59f77f to
cf863d7
Compare
Leave POSTGRES_PASSWORD empty in .env.example so the compose guard can fire, and use the :? form so empty and unset both fail. README generation step lands separately in LucasSantana-Dev#1881.
LucasSantana-Dev
left a comment
LucasSantana-Dev
left a comment
There was a problem hiding this comment.
Review: changes-required (self-review pass)
The core ? → :? fix is correct and well-evidenced for docker-compose.yml, but the identical weak guard survives untouched in docker-compose.staging.yml and docker-compose.dev.yml, including the DIRECT_URL sites the decision doc says this covers.
P1 — fix incomplete across compose files: docker-compose.staging.yml:46,47,69 and docker-compose.dev.yml:9,65,66 keep ${POSTGRES_PASSWORD?...} (6 sites). Concrete failure: POSTGRES_PASSWORD= (present but empty) in the staging env, then scripts/deploy-staging.sh:24 runs Compose; the ? form errors only on unset, so the empty value passes and Postgres boots with an empty password. Unlike production, where deploy.sh:550-552 independently hard-fails, deploy-staging.sh has no password guard, so the compose guard is the only line of defense on that path. Fix: apply the same one-character ? → :? change to the 6 remaining sites.
Second angle on the same finding: decisions/2026-07-26-postgres-password-env-guard.md opens by enumerating "three places" in docker-compose.yml including DIRECT_URL ... # added by #1674 — but docker-compose.yml has no DIRECT_URL (grep: no match); those lines live in the staging/dev compose files (added by #1830). Intent-vs-implementation mismatch, not just scope.
P3: update the decision doc to say the guards were hardened in all three compose files. (The README echo ... >> .env duplicate-key trade is disclosed and justified — no action.)
What's good: the central ? vs :? claim is empirically verified against docker-compose 5.1.4 with a state table; the >> over sed -i GNU/BSD portability rationale and the URL-safe openssl rand -hex 24 justification are both correct and documented; the decision record follows repo convention with alternatives, consequences, and revisit triggers.
|
🤖 I have created a release *beep* *boop* --- <details><summary>2.38.0</summary> ## [2.38.0](v2.37.3...v2.38.0) (2026-07-27) ### Features * **bot:** add /ticket-setup for support category and agent role ([#1863](#1863)) ([3f4af39](3f4af39)) * **frontend:** per-action loading and connection gating on music controls ([#1866](#1866)) ([2dda60f](2dda60f)) * **frontend:** show stale progress when music SSE lags ([#1867](#1867)) ([4952e73](4952e73)) * **music:** surface recommendationReason in nowplaying and queue ([#1864](#1864)) ([960fd62](960fd62)) * **ops:** blue/green zero-downtime deploys — Phase 1 web tier ([#1786](#1786)) ([f5f7597](f5f7597)) ### Bug Fixes * **docker:** make compose stack boot from a fresh .env ([#1674](#1674)) ([babe0ef](babe0ef)) * **docker:** treat an empty db password as missing in compose guards ([#1881](#1881)) ([718c0ad](718c0ad)) * **frontend:** make landing page usable at mobile widths ([#1865](#1865)) ([6190350](6190350)) * **frontend:** stop hero grid columns overflowing on narrow viewports ([#1874](#1874)) ([ce5cea0](ce5cea0)) * **invite:** add /invite where cloudflare pages reads it ([#1895](#1895)) ([0528f66](0528f66)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
1 participant
What
docker-compose.ymlguarded the Postgres password with${POSTGRES_PASSWORD?POSTGRES_PASSWORD_required}. Docker Compose fires that form only when the variable is absent. A present-but-empty value passes straight through, soPOSTGRES_PASSWORD=in.envstarts Postgres with an empty password and no warning.Verified against
docker-compose5.1.4:.envstate${VAR?err}${VAR:?err}POSTGRES_PASSWORD=(empty)Both interpolation sites now use
:?, so the guard means what it looks like it means. Confirmed against the real compose file: empty value now fails withrequired variable POSTGRES_PASSWORD is missing a value: POSTGRES_PASSWORD_required, a real value renders the URL normally.Also documents generating the value in the README setup block. It is interpolated straight into a
postgres://URL, so it has to be URL-safe, henceopenssl rand -hex 24. Duplicate keys in.envresolve last-wins (verified), so>>is safe, and it avoids thesed -iGNU/BSD portability trap in a public README.Why now
Came out of reviewing #1674, which proposes an active 48-zero placeholder in
.env.example. That would permanently disable this guard for anyone following the documentedcp .env.example .envflow. The fix there is an empty placeholder, which only works once the guard actually rejects empty values, which is this PR.This is a pre-existing weakness, not something #1674 introduced.
Decision record
decisions/2026-07-26-postgres-password-env-guard.mdcovers the alternatives, including thescripts/setup-env.shoption and why it is the most likely thing to revisit.Not addressed here
Changing
POSTGRES_PASSWORDin.envafter first boot does not change the password, becausePGDATAis initialised once and the volume persists the original. Pre-existing; filed separately.Summary by cubic
Treats an empty
POSTGRES_PASSWORDas missing in Docker Compose so dev and staging stacks fail fast instead of starting Postgres with an empty password. Updates the README to generate a URL‑safe password and adds a decision record.Bug Fixes
${POSTGRES_PASSWORD:?POSTGRES_PASSWORD_required}indocker-compose.staging.ymlanddocker-compose.dev.ymlforDATABASE_URL,DIRECT_URL, and the Postgres service.Migration
POSTGRES_PASSWORDbeforedocker compose up, e.g.echo "POSTGRES_PASSWORD=$(openssl rand -hex 24)" >> .envWritten for commit eacd52b. Summary will update on new commits.