fix(selfhost): stop leaking Postgres credentials via verify-backup.sh argv - #2512
Conversation
… argv scripts/backup.sh (a prior fix) had the same class of vulnerability this script still carried: db_identity(), the scratch pg_restore --dbname call, and the post-restore sanity psql call all passed a full postgres(ql):// URL -- potentially including a password, via userinfo OR the libpq `password=...` query-string form -- directly as a process argument, exposing it via `ps`/`/proc/PID/cmdline` to any other user on the same host. Ported the same sanitization approach from backup.sh (see that file for the full URI-parsing rationale): strip only the password -- from userinfo, restricted to the authority component so a literal '@'/':' in the query string is never mistaken for credentials, or from a `password=` query parameter -- and hand back everything else (host, port, dbname, every other query parameter) untouched as the connection argument, with the password supplied out-of-band via a temporary, 600-permission, wildcarded PGPASSFILE. Unlike backup.sh, this script may connect to TWO different URLs in the same run (the live source and a scratch database) via db_identity(), so the shared logic here (pg_connect_arg) takes the URL as an argument instead of reading a single script-global $PG_DB, tracks every passfile it creates in a list for cleanup (rather than backup.sh's single $PGPASSFILE_CREATED), and always unsets PGPASSFILE before checking the current URL for a password -- otherwise a PREVIOUS call's password could leak into a connection for a URL that doesn't have one of its own (e.g. a passwordless live source checked between two scratch-database connections). Not extracted into a shared sourced helper file: docker-compose.yml's `backup` service bind-mounts backup.sh and verify-backup.sh as individual files at the container root (./scripts/backup.sh:/backup.sh:ro, similarly for verify-backup.sh), not the whole scripts/ directory, so a shared file would need its own new mount entry kept in sync by hand -- more deployment coupling than the ~90 duplicated lines it would save. Updated every existing test that mocks psql by exact URL match: the real script now calls psql/pg_restore with a SANITIZED (password-free) URL, not the raw one, so fakePsql's identity map must be keyed on the sanitized form -- added a sanitizedUrl() test helper mirroring the shell logic exactly (a test that could still pass keyed on the raw URL would not actually verify the credential never reaches argv). Added a new test exercising the full scratch-restore flow (4 psql/pg_restore calls plus the initial structural pg_restore --list) with a password supplied via the query-string form on one URL and no password on the other, asserting: no password ever appears in any captured argv, and the passwordless URL's connection never inherits a PGPASSFILE left over from the other URL's.
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 07:35:45 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 5 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
…word The AI review on #2459 (scripts/backup.sh) found that the query-string password-stripping there only removed the FIRST `password=` occurrence, so a malformed URL repeating the key (not rejected by libpq's own parser) left a second occurrence sitting in argv, still a leaked credential. This script's pg_connect_arg has the identical query-string-password logic, so it carries the same gap -- ported the identical fix: loop the extraction until no `password=` remains rather than stripping once. Each iteration overwrites pg_password_value, so the LAST occurrence is what ends up in the PGPASSFILE; which one libpq itself would use for a duplicate key is unspecified, but every occurrence is a credential either way, so none may reach argv. Also added a full-scratch-restore-flow test for the userinfo-password form (user:password@host) -- the existing multi-connection flow test only proved the query-string form end to end, per a non-blocking nit from the same review round asking for both forms to be exercised through the complete flow, not just the isolated single-URL cases already covered. Verified against the duplicate-key case and every prior regression case (still passing). Reverting just the loop fix reproduces the exact residual leak.
… name The AI review on the sibling PR #2519 (scripts/backup.sh) found that the query-string password-stripping there only matched the LITERAL string "password=", so a percent-encoded key name like `pass%77ord=secret` (%77 decodes to 'w', and libpq percent-decodes query key names before matching them against connection keywords) still leaked a real credential into argv. This script's pg_connect_arg has the identical logic, so it carries the same gap -- ported the identical fix: walk each '&'-separated query pair individually, decode only the key half of each, compare the decoded key against "password", and rebuild the query from every pair whose decoded key isn't a match, in original order, with values left percent-encoded exactly as given. Added a matching regression test through the full scratch-restore flow. Verified against the exact encoded-key case and every prior regression case (still passing). Reverting just this change reproduces the exact leak.
…hecks db_identity() is invoked via command substitution ($(db_identity ...)), which forks a subshell -- calling pg_connect_arg from inside its body meant the PG_PASSFILES cleanup-list append never propagated back to the parent, orphaning a real, credential-bearing 600-permission passfile on disk for every identity check that needed one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2512 +/- ##
=======================================
Coverage 95.94% 95.95%
=======================================
Files 226 226
Lines 25369 25411 +42
Branches 9227 9242 +15
=======================================
+ Hits 24341 24383 +42
Misses 417 417
Partials 611 611 🚀 New features to boost your workflow:
|
Summary
scripts/backup.sh(PR fix(selfhost): hide Postgres backup credentials #2459) had the same class of vulnerability this script still carries:db_identity(), the scratchpg_restore --dbnamecall, and the post-restore sanitypsqlcall all passed a fullpostgres(ql)://URL — potentially including a password, via userinfo or the libpqpassword=...query-string form — directly as a process argument, exposing it viaps//proc/PID/cmdlineto any other user on the same host.backup.sh(see that file for the full URI-parsing rationale): strip only the password — from userinfo, restricted to the authority component so a literal@/:in the query string is never mistaken for credentials, or from apassword=query parameter — and hand back everything else (host, port, dbname, every other query parameter) untouched as the connection argument, with the password supplied out-of-band via a temporary, 600-permission, wildcardedPGPASSFILE.backup.sh, this script may connect to two different URLs in the same run (the live source and a scratch database) viadb_identity(), so the shared logic here (pg_connect_arg) takes the URL as an argument instead of reading a single script-global$PG_DB, tracks every passfile it creates in a list for cleanup, and always unsetsPGPASSFILEbefore checking the current URL for a password — otherwise a previous call's password could leak into a connection for a URL that doesn't have one of its own (e.g. a passwordless live source checked between two scratch-database connections).docker-compose.yml'sbackupservice bind-mountsbackup.shandverify-backup.shas individual files at the container root, not the wholescripts/directory, so a shared file would need its own new mount entry kept in sync by hand — more deployment coupling than the ~90 duplicated lines it would save.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.md.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coverage— full unsharded run green on Node 22.23.1 (matching CI's pinned.nvmrc): 320 passed / 2 skipped, 6067 tests passed, 0 failures.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:testnpm run ui:buildnpm audit --audit-level=moderatepsqlby exact URL match was updated: the real script now callspsql/pg_restorewith a SANITIZED (password-free) URL, not the raw one, sofakePsql's identity map is now keyed on the sanitized form via a newsanitizedUrl()test helper mirroring the shell logic exactly.psql/pg_restorecalls plus the initial structuralpg_restore --list) with a password supplied via the query-string form on one URL and no password on the other: asserts no password ever appears in any captured argv, and the passwordless URL's connection never inherits aPGPASSFILEleft over from the other URL's.backup.sh's own fix was tested against (userinfo password, query-string-only host,+in password, fake@/:in a query value, combined real-password-plus-fake-query-userinfo, query-stringpassword=in first/middle/last position and before a fragment, percent-encoded query password, the negative case where a value merely contains the substring "password") — confirmed via a standalone shell harness before wiring into the vitest suite.r.status !== 0failure against the sanitized-URL-keyed mocks, confirming the new tests actually exercise the behavior change rather than passing vacuously.If any required check was skipped, explain why:
Safety
Notes
scripts/backup.sh) for the sibling script that shares the same vulnerability class.