fix(selfhost): strip a query-string password even with an encoded key name - #2519
Merged
Merged
Conversation
… name libpq percent-decodes query KEY NAMES before matching them against connection keywords, not just values -- so `pass%77ord=secret` (%77 = 'w') is just as much a `password` parameter as the literal spelling, and libpq will authenticate with it. A literal string match against "&password=" (the previous approach) never saw it: the raw text is "pass%77ord=", not "password=", so it sailed through untouched into $PG_SANITIZED_URL, still a real credential in pg_dump's argv. Replaced the literal-match loop with one that walks each '&'-separated query pair individually, decodes ONLY the key half of each, and compares that against "password" -- rebuilding the query from every pair whose decoded key isn't a match, in original order, with values left percent-encoded exactly as given (they're not being re-parsed, just handed to libpq, which decodes them itself). A repeated key is handled the same way as before: each match overwrites the stored password, so the last occurrence wins. Verified against the exact encoded-key case from the review, alongside every prior regression case (still passing). Reverting just this change reproduces the exact leak.
JSONbored
added a commit
that referenced
this pull request
Jul 2, 2026
… 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.
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 07:19:15 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 3 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2519 +/- ##
==========================================
+ Coverage 95.94% 95.95% +0.01%
==========================================
Files 226 226
Lines 25361 25425 +64
Branches 9229 9244 +15
==========================================
+ Hits 24333 24397 +64
Misses 417 417
Partials 611 611 🚀 New features to boost your workflow:
|
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.
Summary
pass%77ord=secret(%77=w) is just as much apasswordparameter as the literal spelling, and libpq will authenticate with it. The previous fix's literal string match against&password=never saw this: the raw text ispass%77ord=, notpassword=, so it survived untouched into$PG_SANITIZED_URL, still a real credential inpg_dump's argv.&-separated query pair individually, decodes only the key half of each, and compares that againstpassword— rebuilding the query from every pair whose decoded key isn't a match, in original order, with values left percent-encoded exactly as given (they're not being re-parsed, just handed tolibpq, which decodes them itself). A repeated key is handled the same way as before: each match overwrites the stored password, so the last occurrence wins.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): 321 passed / 2 skipped, 6092 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=moderatepass%77ord=secret) from the review, plus every prior regression case from fix(selfhost): hide Postgres backup credentials #2459 (duplicate keys, userinfo password, query-string-only host,+in password, fake@/:in a query value, percent-encoded values, the substring-only negative case) — all still passing via a standalone shell harness before wiring into the vitest suite.If any required check was skipped, explain why:
Safety
Notes
scripts/verify-backup.shin fix(selfhost): stop leaking Postgres credentials via verify-backup.sh argv #2512, which shares this logic.