fix(selfhost): hide Postgres backup credentials - #2459
Merged
JSONbored merged 6 commits intoJul 2, 2026
Conversation
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 06:39:48 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 7 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.
|
loopover-orb
Bot
added
gittensor
gittensor:bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2459 +/- ##
=======================================
Coverage 95.82% 95.82%
=======================================
Files 224 224
Lines 24975 24975
Branches 9076 9076
=======================================
Hits 23933 23933
Misses 428 428
Partials 614 614 🚀 New features to boost your workflow:
|
This was referenced Jul 2, 2026
…p URL prepare_pg_env manually re-parsed the Postgres URL into PGHOST/PGPORT/ PGDATABASE, discarding the query string entirely before ever looking at it. Any DATABASE_URL that relies on it -- e.g. `postgresql:///gittensory?host= /var/run/postgresql` (connecting over a Unix socket at a non-default path, the authority left empty on purpose) -- silently stopped connecting the way the original `pg_dump "$PG_DB"` call did. Re-implementing libpq's own URI parsing in shell (host/port/dbname, every query parameter, precedence rules between them) is exactly the kind of thing that's easy to get wrong piecemeal, as this already showed. Instead: strip ONLY the password from the URI's userinfo section and hand pg_dump the rest untouched -- host, port, dbname, and the full query string, byte for byte -- as its connection argument, so libpq's own parser resolves it exactly as it always has. The password never touches pg_dump's argv/`ps` output; it reaches pg_dump out-of-band via a PGPASSFILE. That file wildcards host/port/dbname/ user (`*:*:*:*:<password>`) rather than trying to match them exactly: it's a 600-permission temp file scoped to the one connection this script makes and deleted immediately after (see the existing `cleanup` trap), so there's no value in re-deriving the exact host/port/dbname libpq will resolve just to match them -- the query string can override those anyway. Also fixed url_decode: it decoded '+' as a space, which is only correct for application/x-www-form-urlencoded query values, not a URI's userinfo component, where '+' is an ordinary allowed character. A password containing a literal '+' would have been silently corrupted to a space. Found via the same review pass that caught the query-string regression. Updated the existing "does not pass credentials" test: its `not.toContain( "postgresql://")` assertion no longer holds (a password-free connection string on argv isn't a credential leak -- that assertion conflated "the scheme string appears" with "a secret appears," which is what pushed the original fix toward manual re-parsing in the first place). Replaced it with assertions on the actual security property (no password substring on argv) plus a check that the sanitized URL correctly reaches pg_dump. Added two new regressions: a query-string-only connection URL (the exact form from the review) round-trips into pg_dump's argv unchanged, and a password containing a literal '+' survives as '+', not a space.
pgpass is a single-line-per-entry format, and pgpass_escape only escapes the
two characters (':' and '\') that format itself treats specially. A decoded
password containing a raw newline or carriage return would still split the
entry across lines, corrupting the field layout -- refuse outright (exit 1)
rather than silently write a malformed PGPASSFILE. Found via the same review
pass that caught the query-string regression, since a newline is a valid
(if unusual) percent-encoded byte a URI userinfo password can carry.
First attempt at the check was itself broken: `case "$X" in *"$(printf
'\n')"*)` looks reasonable but command substitution strips ALL trailing
newlines, so `$(printf '\n')` evaluates to an empty string and the pattern
matches literally every string -- caught immediately by the existing
regression tests (the query-string and '+'-password tests both failed
against every normal password once this shipped, before this commit ever
reached CI). Fixed with the standard `$(printf '\nx'); ${VAR%x}` idiom:
appending a marker byte means there's nothing trailing for the command
substitution to strip, then the marker itself is stripped by parameter
expansion, leaving exactly one newline/CR character to match against.
Also added a test asserting the PGPASSFILE is actually created with 0600
permissions (the existing test only asserted the path pattern, not the mode,
despite the PR's own description depending on it) and a regression for the
newline-rejection behavior itself.
This was referenced Jul 2, 2026
…hority prepare_pg_env searched for the first '@' across the ENTIRE remaining URL string, including the query string. Userinfo (user:password@) can only appear in the authority component -- everything before the first '/', '?', or '#' -- never later in the URI. A URL with no real credentials at all but a literal '@'/':' inside a query VALUE, e.g. `postgresql://db.example/gittensory?application_name=a:b@worker`, got misread: everything before that '@' was wrongly treated as userinfo, ':b' was stripped out as a fake password, and the corrupted URL `postgresql://db.example/gittensory?application_name=a@worker` (missing the ":b") was what actually reached pg_dump. Fixed by finding the authority boundary FIRST -- the substring before whichever of '/', '?', '#' occurs earliest (computed via three %% removals, keeping the shortest result, since the earliest delimiter produces the shortest "before" substring) -- and restricting the '@'/':' userinfo search to within that authority substring only. Everything from the boundary onward (path, query, fragment) is carried through untouched and reattached verbatim, so it can never be mistaken for credentials regardless of what it contains. Verified against the exact URL from the review (confirmed it round-trips unchanged with no passfile created), every prior regression case (still passing), and a new combined case: a REAL password alongside a query string that separately contains its own '@'/':' -- both are now handled correctly in the same URL. Reverting just this fix reproduces the exact corrupted output the reviewer predicted, confirming the new tests actually catch it.
This was referenced Jul 2, 2026
…orm too libpq connection URIs support supplying `password` as a query-string parameter, not just via userinfo -- `postgresql://user@host/db?password=secret` is an equally valid, if less common, connection string. The previous fix only ever stripped a userinfo password and passed the entire query string through untouched, so this form still leaked the password verbatim into pg_dump's argv/`ps` output. Split the URI's suffix (everything from the authority boundary onward) into its path / query / fragment components and, if the query component has a `password` key, extract it (url-decoded, same as a userinfo password) and remove it, leaving every other parameter -- and their order -- untouched. Whichever form (userinfo or query-string) actually carried a password now reaches the same PGPASSFILE mechanism; pg_dump's argv never sees either. Verified against the exact URL form from the review, a password positioned first/middle/last among other query parameters, a password immediately before a URI fragment, a percent-encoded password value, and the negative case (a parameter whose VALUE merely contains the substring "password", e.g. `application_name=has_password_in_name`, must not be misidentified as the key) -- alongside every prior regression case, all still passing. Reverting just this change reproduces the exact leak the review flagged.
28 tasks
…word The prior fix stripped only the first `password=` occurrence in the query string. A malformed URL repeating the key -- e.g. `postgresql://u@h/db?password=one&sslmode=require&password=two` -- isn't rejected by libpq's own parser, so the second occurrence survived untouched into $PG_SANITIZED_URL, still leaking a credential into pg_dump's argv regardless of which one libpq would actually authenticate with. Loop the extraction until no `password=` remains in the query string instead of stripping once. Each iteration overwrites PGPASSWORD_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. Verified against the exact duplicate-key URL from the review, a 3-occurrence case, and every prior regression case (still passing). Reverting just this change reproduces the exact residual leak the review flagged.
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
…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.
26 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… argv (#2512) * fix(selfhost): stop leaking Postgres credentials via verify-backup.sh 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. * fix(selfhost): strip every occurrence of a repeated query-string password 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. * fix(selfhost): strip a query-string password even with an encoded key 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. * fix(selfhost): call pg_connect_arg in the parent shell for identity checks 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.
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
1 participant
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.
Motivation
pg_dump, exposing credentials via process argv; this change prevents that leak while keeping backup functionality.Description
scripts/backup.shand export libpq connection pieces (PGHOST,PGPORT,PGDATABASE,PGUSER) instead of passing the full URL onpg_dumpargv.PGPASSFILE, populate it with an escaped entry, exportPGPASSFILE, and register atrapto securely remove the file on exit.pg_dump ... "$PG_DB"invocation withprepare_pg_env+pg_dump -Fc -f ...sopg_dumpruns with env-based credentials only.test/unit/selfhost-backup-script.test.tsthat fakespg_dump, captures its argv/env, and asserts that neither the Postgres URL nor the password appear in process arguments while existing Postgres and SQLite backup behavior remains unchanged.scripts/backup.shandtest/unit/selfhost-backup-script.test.ts.Testing
sh -n scripts/backup.shreturned successfully with no syntax errors.npx vitest run test/unit/selfhost-backup-script.test.tsand passed (3 tests), including the new regression that verifies credentials are not passed onpg_dumpargv.git diff --checkpassed locally.npm run test:cicould not complete in this environment due to network/actionlint setup issues (WASM fallback reported the repo's custom self-hosted runner label), andnpm audit --audit-level=moderatereturned403 Forbiddenfrom the npm audit endpoint.Codex Task