[IAmJSD]

Closes #557

#588 fixed most of the sites in this issue. What it left open was the "two encoders" question: scripts/entrypoint.sh percent-encoded the password with awk because the runtime image has no jq, and everything else used jq -sRr @uri. This PR picks a third option, so neither encoder is needed. Every shell site leaves the password out of the pgx5:// URL and passes it in PGPASSWORD. golang-migrate's pgx5 driver opens the URL through pgx, and pgx reads PGPASSWORD when the URL has no password. Go builders keep url.UserPassword (URL form) and quoteDSNValue (keyword/value form), and the keyword DSN now quotes every field, not just the password.

Per-site changes (re-verified on main at d7eb3b0)

Site	Before (main)	After
.github/workflows/ci.yml migrate step	jq -sRr @uri → pgx5://user:${ENCODED_PASSWORD}@…	PGPASSWORD="$DB_PASSWORD" migrate … pgx5://user@…
.github/workflows/database-migration.yml (AWS run + version, GCP, Azure: 4 steps)	jq -sRr @uri, an empty-encode check, and ::add-mask:: of the encoded form, then pgx5://cudly:${ENCODED_PASSWORD}@…	step env: PGPASSWORD: ${{ secrets.DB_PASSWORD_* }}, URL pgx5://cudly@…. No derived secret is left to mask.
docs/DEPLOYMENT.md (both migrate commands)	jq -sRr @uri + encoded URL	PGPASSWORD="$DB_PASSWORD" migrate … pgx5://cudly@…, and the note now explains why
scripts/entrypoint.sh	awk percent-encoder (a second encoder; it drops a newline)	PGPASSWORD="$DB_PASSWORD" migrate …, set only on the migrate command, not exported to the app
internal/database/config.go keyword DSN	only password quoted; host/user/dbname/sslmode raw, so e.g. DB_USER="x sslmode=disable" injected a keyword	every string field goes through quoteDSNValue (libpq rules: single-quote, escape \ and '). Plain values render the same as before
internal/testutil/postgres.go, migrations/migrate.go	already url.URL{User: url.UserPassword(…)}	unchanged

Side effects: the password no longer shows up in migrate's argv (ps / /proc/*/cmdline). The migrate version 2>&1 output that goes into $GITHUB_ENV also can't contain it, because the URL no longer has it.

Verification

All Go tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

• Keyword DSN unit tests (internal/database/config_dsn_quoting_test.go): hostile values @ : / %41 # ? space tab+newline ' \ trailing-\ = unicode empty, plus x sslmode=disable and a quoted-injection variant. Each goes through the password and through user/dbname, and pgconn.ParseConfig must return the exact value, with sslmode=require still in place (TLSConfig non-nil) and no stray runtime params. RedactedDSN never contains the password.

  • Fail-before: with main's config.go, 6 subtests fail (space, tab/newline, trailing backslash, keyword injection, quoted injection, mixed).
  • Mutation probes, each caught: dropping quoting on User (7 failures), on Database (7), on the password (10), dropping the ' escape, dropping the \ escape.

• Shell guard (TestShellMigrationURLs_PassPasswordViaPGPASSWORD, which replaces the jq/mask guards): it covers all 4 files. No pgx5:// userinfo may contain :, and no ENCODED_PASSWORD or @uri may remain. Workflows are parsed with yaml.v3, and every migrate -path line has to run either in a step whose env sets PGPASSWORD or with the PGPASSWORD="$DB_PASSWORD" prefix. The guard also refuses to pass if it finds no invocations at all.

  • Fail-before: all 4 files fail on main.
  • Mutation probes, each caught: removing the prefix in entrypoint.sh, removing it in ci.yml, putting :${DB_PASSWORD} back into a docs URL, renaming the GCP step env back to DB_PASSWORD.

• Integration test on testcontainers Postgres 16 (-tags integration, TestPGPasswordAndKeywordDSN_ConnectWithHostilePassword): sets the role password (scram) to 5 hostile values. For each one it authenticates through golang-migrate's pgx5 driver (password-less URL plus PGPASSWORD) and through pgx.Connect(cfg.DSN(pw)). A wrong PGPASSWORD is rejected, and that error doesn't echo the password.

• The real scripts/entrypoint.sh, run by hand in alpine:3.24.1 with a linux migrate built -tags pgx5 from go.mod and a Postgres 16 container, each case on a fresh database:

  password	main entrypoint	this PR
  @ : / %41 # ? =	ok	ok
  space + ' + "	ok	ok
  backslashes	ok	ok
  unicode	ok	ok
  = [ ] $ &	ok	ok
  contains a newline	fails (SASL auth: awk drops it)	ok
  wrong password (control)	—	fails as expected

  The password never appeared in container output.

• go build ./..., go vet ./..., and go vet -tags integration ./... pass. go test passes on ./internal/database/, ./internal/database/postgres/migrations/ and ./internal/mocks/; -tags integration passes on ./internal/database/ and ./internal/testutil/.

• golangci-lint v2.10.1 reports 0 issues on ./internal/database/..., with and without --build-tags integration. gocyclo -over 10 internal/database/config.go is clean.

• actionlint and zizmor 1.29.0 --offline --persona=pedantic --min-severity=medium are clean on both workflows. shellcheck scripts/entrypoint.sh and markdownlint-cli 0.47.0 docs/DEPLOYMENT.md are clean.

• git-secrets 1.3.0 in a fresh git init repo with an isolated HOME and the patterns from setup-git-secrets.sh finds nothing in the changed files; a positive control is caught. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Not verified: the edited database-migration.yml jobs against real AWS/GCP/Azure databases. Those need the real secrets and environment gates. The same pattern (password-less pgx5:// URL plus PGPASSWORD) is what CI's migrate step and the entrypoint run above exercise.

Out of scope

• docs/DEVELOPMENT.md local docker-compose URLs, which embed the fixed cudly_local_dev dev password.
• terraform/*/outputs.tf psql hints, which carry no password.
• known_issues/ has no entry for sec(ci): other raw database-password interpolations and two DSN encoders after #555 #557.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved database connection and migration reliability when passwords or other connection values contain special characters, whitespace, or Unicode.
  • Migration commands no longer include database passwords in connection URLs or command-line arguments.
• Documentation
  • Updated deployment and troubleshooting instructions to reflect the revised, more secure migration commands.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 978ee96f-d8b4-4914-ba2e-51c055418960

📥 Commits

Reviewing files that changed from the base of the PR and between 70a6457 and 74dc9b2.

📒 Files selected for processing: 8

• .github/workflows/ci.yml
• .github/workflows/database-migration.yml
• docs/DEPLOYMENT.md
• internal/database/config.go
• internal/database/config_dsn_quoting_test.go
• internal/database/postgres/migrations/migrate_dsn_encoding_test.go
• internal/database/postgres/migrations/migrate_pgpassword_integration_test.go
• scripts/entrypoint.sh

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Database DSN construction now quotes string values. Migration commands in CI, cloud workflows, scripts, and deployment instructions pass passwords through PGPASSWORD instead of embedding them in connection URLs. New tests cover hostile password values, DSN parsing and redaction, migration command configuration, and PostgreSQL authentication.

Changes: Database Password Handling

Layer / File(s)	Summary
Keyword DSN quoting internal/database/config.go, internal/database/config_dsn_quoting_test.go	Config.dsn now quotes host, user, password, database, and SSL mode values. Tests check hostile-value round trips, connection settings, and password redaction.
Migration password handling .github/workflows/ci.yml, .github/workflows/database-migration.yml, scripts/entrypoint.sh, docs/DEPLOYMENT.md, internal/database/postgres/migrations/migrate_dsn_encoding_test.go	Migration commands use PGPASSWORD and password-free pgx5 URLs. Tests inspect shell migration sources and workflow steps for this configuration.
PostgreSQL password integration test internal/database/postgres/migrations/migrate_pgpassword_integration_test.go	An integration test checks migration and direct pgx connections with hostile passwords. It also checks that an incorrect password fails without appearing in the error.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 74dc9

No actionable merge-blocking issue remains. The changed migration commands use a supported password mechanism; real cloud-account execution remains unverified.

Security architecture risk: 🔵 Low · up to 74dc9

The change reduces password exposure in command-line URLs and prevents configuration values from injecting connection settings. Database identities, cloud TLS settings and migration failure handling are preserved. Remaining uncertainty concerns deployment compatibility and interrupted or concurrent startup behavior, rather than an identified security regression.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The sensitive outcome remains schema mutation within each selected PostgreSQL database using its existing migration account. The cloud commands retain the cudly user, cudly database and sslmode=require; the credential-transport change does not itself broaden database authority.

Trust Boundaries and Controls

• observed — Cloud secrets remain scoped to migration or version-query steps. Container startup sets PGPASSWORD only for migrate, keeping the password out of its URL argument. This does not remove the preexisting DB_PASSWORD environment inherited by the application or create isolation from processes able to inspect credentials.

Resilience and Maintainability Implications

• observed — The new source guard checks password-free migration URLs and PGPASSWORD coverage across workflows, startup and documentation. This helps detect credential-handling drift, but it is a static check rather than evidence of interruption, recovery or concurrent-start behavior.

Hardening Proposals

• proposed — Validate the actual entrypoint and shipped CLI against failed authentication, dirty migration state, interruption, restart and simultaneous starts, checking both schema state and whether application startup remains blocked on migration failure.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #557 requires one documented encoding approach for every listed builder. This PR documents PGPASSWORD for shell migration sites, but the listed Go builders still use url.UserPassword and `qu…	Use one documented password-handling or encoding approach for every builder listed in #557, including the unchanged Go URL builders, and update the tests and deployment documentation to enforce that approach. If PGPASSWORD is the intended…

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes both primary changes: using PGPASSWORD for migration passwords and quoting keyword DSN values.
Out of Scope Changes check		The changed workflows, deployment documentation, entrypoint, DSN builder, and supporting tests all address the password-handling and DSN-quoting work in issue #557. The changes do not modify the state…
Docstring Coverage		Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (3 skipped: 3 …

Full details: Out of Scope Changes check

Explanation

The changed workflows, deployment documentation, entrypoint, DSN builder, and supporting tests all address the password-handling and DSN-quoting work in issue #557. The changes do not modify the stated out-of-scope Docker Compose URLs, Terraform hints, or known_issues/ content.

Full details: Docstring Coverage

Explanation

Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (3 skipped: 3 unsupported.)

Full details: Linked Issues check

Explanation

Issue #557 requires one documented encoding approach for every listed builder. This PR documents PGPASSWORD for shell migration sites, but the listed Go builders still use url.UserPassword and quoteDSNValue. The PR does quote and escape keyword DSN fields, updates DEPLOYMENT.md, and adds hostile-value tests. It does not meet the single-approach acceptance criterion.

Resolution

Use one documented password-handling or encoding approach for every builder listed in #557, including the unchanged Go URL builders, and update the tests and deployment documentation to enforce that approach. If PGPASSWORD is the intended alternative, update the issue acceptance criterion before claiming that it is complete.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review verdict: CLEAN at 74dc9b2

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

DSN quoting (internal/database/config.go:143-169): dsn() quotes host, user, password, dbname and sslmode. Port and connect_timeout are ints.

Input	Handling	Test
space, tab, newline	wrapped in '…'	"space", "tab and newline"
'	\'	"single quote", "quoted injection"
\	\\	"backslash", "trailing bslash"
empty	''	"empty"
@ : / % # ? =, unicode	left unquoted (correct for keyword DSNs)	own cases

The tests round-trip each value through pgconn.ParseConfig, for password, user and dbname.

Injection: the passwords x sslmode=disable and x' sslmode=disable ' keep TLS on (sslmode=require survives) and add no runtime params. An integration test also connects with a hostile password against scram.

The PGPASSWORD path:

• golang-migrate v4.19.1's pgx/v5 driver (pgx.go:139-150) goes through pgx.ParseConfig, which reads PGPASSWORD. The integration test proves it, including that a wrong PGPASSWORD fails.
• In scripts/entrypoint.sh:64-79 the migrate URL no longer carries the password. PGPASSWORD is a per-command prefix, not exported to the app, and there is no set -x.
• database-migration.yml and ci.yml:400-403 pass it as masked secret env.
• There is no double source, and no other DSN or URL builders exist outside tests.

Deploy impact (an ordinary code merge, no infra hold):

• database-migration.yml is workflow_dispatch only.
• entrypoint.sh runs on every container start. The env var names are unchanged and DATABASE_URL is unused. A non-empty password behaves the same as before, and an empty one is still skipped.

Note (non-blocking, already present before this PR): DB_USER, DB_NAME and DB_HOST are still interpolated raw into the migrate URL in the shell scripts. A follow-up could percent-encode them.

What was attacked and how:

• go vet ./internal/... is clean. go test -race -count=1 ./internal/database/... passes 3 packages.
• -tags integration -race -v on migrations against Docker Postgres: all PASS, 0 SKIP (PGPassword 5 subtests, BuildMigrateDSN).
• shellcheck and actionlint are clean.
• Fail-before: TestConfigDSN_HostileValuesRoundTrip fails on space, tabs, trailing backslash and both injection cases.
• Mutation probe: dropping the ' → \' escape fails the hostile-value, password-round-trip and quoting tests.

Gate:

• Labels are mirrored from sec(ci): other raw database-password interpolations and two DSN encoders after #555 #557.
• Needs the author to mark the draft ready, then green CI and CodeRabbit.

[cristim]

Gate record for 74dc9b2 (origin/main CLAUDE.md "Review gate"):

• Independent review (claude-cc-reviewer, non-author): CLEAN at 74dc9b2 (fix(sec): pass the migrate password via PGPASSWORD and quote every keyword DSN value #1033 (comment)). The verdict SHA equals the head.
• CI on 74dc9b2: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.
• Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(ci): other raw database-password interpolations and two DSN encoders after #555 #557.

Marking ready (owner-authorized) and merging at 74dc9b2 with --match-head-commit.

[cristim]

Gate record for 74dc9b2 (origin/main CLAUDE.md "Review gate"):

• Independent review (claude-cc-reviewer, non-author): CLEAN at 74dc9b2 (fix(sec): pass the migrate password via PGPASSWORD and quote every keyword DSN value #1033 (comment)). The verdict SHA equals the head.
• CI on 74dc9b2: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.
• Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(ci): other raw database-password interpolations and two DSN encoders after #555 #557.

Merging at 74dc9b2 with --match-head-commit.