From c6ace41d7b80d29f57c4dda17e606eb3e5b29f72 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:34:40 -0700 Subject: [PATCH 1/4] 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. --- scripts/verify-backup.sh | 153 +++++++++++++++++- .../selfhost-verify-backup-script.test.ts | 103 +++++++++++- 2 files changed, 247 insertions(+), 9 deletions(-) diff --git a/scripts/verify-backup.sh b/scripts/verify-backup.sh index 4c4b85d705..c8aae43e92 100644 --- a/scripts/verify-backup.sh +++ b/scripts/verify-backup.sh @@ -12,6 +12,150 @@ set -eu OUT=${BACKUP_OUT_DIR:-/backups} PG_DB="${GITTENSORY_BACKUP_SOURCE_DATABASE_URL:-${DATABASE_URL:-}}" TARGET="${1:-}" +PG_PASSFILES="" +cleanup() { + for pg_pf in $PG_PASSFILES; do + rm -f "$pg_pf" + done +} +trap cleanup EXIT HUP INT TERM + +# Percent-decodes a URI userinfo component (RFC 3986). Deliberately does NOT treat '+' as a space -- that +# convention is specific to application/x-www-form-urlencoded query values, not URI userinfo, where '+' is +# an ordinary sub-delims character allowed unencoded; the only caller of this function decodes a password +# extracted from the userinfo section, and a literal '+' there must stay a '+', not become a space. +url_decode() { + printf '%s' "$1" | awk ' + BEGIN { for (i = 0; i < 256; i++) hex[sprintf("%02X", i)] = sprintf("%c", i); } + { + out = ""; + for (i = 1; i <= length($0); i++) { + c = substr($0, i, 1); + if (c == "%" && i + 2 <= length($0)) { + h = toupper(substr($0, i + 1, 2)); + if (h in hex) { out = out hex[h]; i += 2; } else { out = out c; } + } else { + out = out c; + } + } + printf "%s", out; + }' +} + +pgpass_escape() { + printf '%s' "$1" | sed 's/\\/\\\\/g; s/:/\\:/g' +} + +# Strips the password from a postgres(ql):// URI -- from EITHER the userinfo (user:password@host) or a +# `password=` libpq query-string parameter (postgresql://user@host/db?password=secret is equally valid +# and equally a leak if left in place) -- and hands back everything else untouched (host, port, dbname, +# and every other query parameter), instead of re-parsing those pieces ourselves -- the same approach +# backup.sh uses, see that file for the full rationale. Userinfo detection is restricted to the authority +# component (before the first '/', '?', or '#'), never the whole remaining string, so a literal '@'/':' +# inside a query-string value (e.g. ?application_name=a:b@worker) is never mistaken for credentials. +# Unlike backup.sh, this script may need to connect to TWO different URLs in the same run (the live source +# and a scratch database), so this takes the URL as an argument and is safe to call repeatedly: it always +# unsets PGPASSFILE first, so a previous call's password can never leak into a connection for a URL that +# doesn't have one of its own. Sets $PG_SANITIZED_URL; exports PGPASSFILE (tracked in $PG_PASSFILES for +# cleanup) if the given URL had a password. +pg_connect_arg() { + pg_rest=${1#postgres://} + pg_rest=${pg_rest#postgresql://} + + pg_authority=${pg_rest%%/*} + pg_before_query=${pg_rest%%\?*} + pg_before_frag=${pg_rest%%#*} + if [ ${#pg_before_query} -lt ${#pg_authority} ]; then pg_authority=$pg_before_query; fi + if [ ${#pg_before_frag} -lt ${#pg_authority} ]; then pg_authority=$pg_before_frag; fi + pg_suffix=${pg_rest#"$pg_authority"} + + pg_password_value="" + pg_sanitized_authority=$pg_authority + case "$pg_authority" in + *@*) + pg_userinfo=${pg_authority%%@*} + pg_after_at=${pg_authority#*@} + case "$pg_userinfo" in + *:*) + pg_user_part=${pg_userinfo%%:*} + pg_password_value=$(url_decode "${pg_userinfo#*:}") + pg_sanitized_authority="${pg_user_part}@${pg_after_at}" + ;; + *) + pg_sanitized_authority="${pg_userinfo}@${pg_after_at}" + ;; + esac + ;; + esac + + pg_path=$pg_suffix + pg_query="" + pg_frag="" + case "$pg_suffix" in + *\?*) + pg_path=${pg_suffix%%\?*} + pg_after_q=${pg_suffix#*\?} + case "$pg_after_q" in + *#*) + pg_query=${pg_after_q%%#*} + pg_frag="#${pg_after_q#*#}" + ;; + *) + pg_query=$pg_after_q + ;; + esac + ;; + *#*) + pg_path=${pg_suffix%%#*} + pg_frag="#${pg_suffix#*#}" + ;; + esac + + pg_query_wrapped="&$pg_query&" + case "$pg_query_wrapped" in + *"&password="*"&"*) + pg_before_pw=${pg_query_wrapped%%&password=*} + pg_from_pw=${pg_query_wrapped#*&password=} + pg_password_value=$(url_decode "${pg_from_pw%%&*}") + pg_after_pw=${pg_from_pw#*&} + pg_query_wrapped="${pg_before_pw}&${pg_after_pw}" + pg_query=${pg_query_wrapped#&} + pg_query=${pg_query%&} + ;; + esac + + pg_suffix=$pg_path + if [ -n "$pg_query" ]; then pg_suffix="$pg_suffix?$pg_query"; fi + pg_suffix="$pg_suffix$pg_frag" + PG_SANITIZED_URL="postgresql://$pg_sanitized_authority$pg_suffix" + + unset PGPASSFILE + if [ -n "$pg_password_value" ]; then + # pgpass is a single-line-per-entry format; pgpass_escape only handles 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 + # rather than silently write a malformed passfile. "$(printf '\n')" would NOT work as a case pattern + # here -- command substitution strips ALL trailing newlines, so it evaluates to an empty string and + # the pattern would match everything; build a variable holding exactly one newline/CR by stripping a + # trailing marker byte instead. + pg_nl=$(printf '\nx'); pg_nl=${pg_nl%x} + pg_cr=$(printf '\rx'); pg_cr=${pg_cr%x} + case "$pg_password_value" in + *"$pg_nl"*|*"$pg_cr"*) + echo "[verify] refusing to use a decoded Postgres password containing a newline or carriage return" >&2 + exit 1 + ;; + esac + # Host/port/dbname/user are wildcarded: each passfile is single-purpose, deleted at the end of this + # run via the `cleanup` trap, so there's no value in re-deriving the exact host/port/dbname libpq will + # resolve -- which the query string can override anyway -- just to match them precisely. + pg_passfile=$(mktemp "${TMPDIR:-/tmp}/gittensory-pgpass.XXXXXX") + chmod 600 "$pg_passfile" + printf '*:*:*:*:%s\n' "$(pgpass_escape "$pg_password_value")" > "$pg_passfile" + PG_PASSFILES="$PG_PASSFILES $pg_passfile" + export PGPASSFILE="$pg_passfile" + fi +} verify_postgres() { dump="$1" @@ -65,7 +209,8 @@ verify_postgres() { # PUBLIC has EXECUTE on pg_control_system() by default. Any failure to fingerprint EITHER side aborts (fail # closed) rather than assuming the databases differ. db_identity() { - psql "$1" -X -q -t -A -v ON_ERROR_STOP=1 \ + pg_connect_arg "$1" + psql "$PG_SANITIZED_URL" -X -q -t -A -v ON_ERROR_STOP=1 \ -c "SELECT current_database() || '@' || (SELECT system_identifier FROM pg_control_system())::text" \ 2>/dev/null } @@ -88,11 +233,13 @@ verify_postgres() { ;; esac echo "[verify] restoring $dump into the scratch database…" - if ! pg_restore --clean --if-exists --no-owner --no-privileges --dbname "$scratch" "$dump" >/dev/null 2>&1; then + pg_connect_arg "$scratch" + if ! pg_restore --clean --if-exists --no-owner --no-privileges --dbname "$PG_SANITIZED_URL" "$dump" >/dev/null 2>&1; then echo "[verify] scratch restore failed for $dump" >&2 return 1 fi - tables="$(psql "$scratch" -X -q -t -A -v ON_ERROR_STOP=1 -c "SELECT count(*) FROM information_schema.tables WHERE table_schema = 'public'")" || { + pg_connect_arg "$scratch" + tables="$(psql "$PG_SANITIZED_URL" -X -q -t -A -v ON_ERROR_STOP=1 -c "SELECT count(*) FROM information_schema.tables WHERE table_schema = 'public'")" || { echo "[verify] scratch sanity query failed" >&2 return 1 } diff --git a/test/unit/selfhost-verify-backup-script.test.ts b/test/unit/selfhost-verify-backup-script.test.ts index 325555e1a5..d0b212bc03 100644 --- a/test/unit/selfhost-verify-backup-script.test.ts +++ b/test/unit/selfhost-verify-backup-script.test.ts @@ -20,17 +20,24 @@ afterEach(() => { // A well-formed dump contains GOODDUMP; anything else makes the fake pg_restore fail as if the archive were // truncated. `--list` prints a header (`;`-prefixed) plus two TOC entry lines; a restore just exits 0. const PG_RESTORE = `#!/bin/sh +if [ -n "\${PG_CAPTURE_FILE:-}" ]; then + printf 'pg_restore %s | PGPASSFILE=%s\\n' "$*" "\${PGPASSFILE:-}" >> "$PG_CAPTURE_FILE" +fi mode=list dump= +dbname= while [ "$#" -gt 0 ]; do case "$1" in --list) mode=list; shift ;; - --dbname) mode=restore; shift 2 ;; + --dbname) mode=restore; dbname="$2"; shift 2 ;; --clean|--if-exists|--no-owner|--no-privileges) shift ;; -*) shift ;; *) dump="$1"; shift ;; esac done +if [ "$mode" = restore ] && [ -n "\${PG_CAPTURE_DBNAME_FILE:-}" ]; then + printf '%s\\n' "$dbname" >> "$PG_CAPTURE_DBNAME_FILE" +fi if ! grep -q GOODDUMP "$dump" 2>/dev/null; then echo "pg_restore: error: could not read from input file: end of file" >&2 exit 1 @@ -55,6 +62,9 @@ function fakePsql(identities: Record, tableCount = "3"): string .join("\n"); return `#!/bin/sh url="$1" +if [ -n "\${PG_CAPTURE_FILE:-}" ]; then + printf 'psql %s | PGPASSFILE=%s\\n' "$url" "\${PGPASSFILE:-}" >> "$PG_CAPTURE_FILE" +fi shift sql="" while [ "$#" -gt 0 ]; do @@ -77,6 +87,37 @@ esac `; } +// Mirrors scripts/verify-backup.sh's pg_connect_arg: strips the password from a postgres(ql):// URI -- +// from EITHER the userinfo (restricted to the authority component, before the first '/', '?', or '#', so +// a literal '@'/':' in the query string is never mistaken for credentials) OR a `password=` query-string +// parameter -- and normalizes the scheme to postgresql://. The real script never invokes psql/pg_restore +// with the raw (possibly password-bearing) URL, so fakePsql's identity map must be keyed on this +// sanitized form to prove that property -- a test that could still pass with the raw URL as the key would +// not actually verify the credential never reaches argv. +function sanitizedUrl(url: string): string { + const rest = url.replace(/^postgres:\/\//, "").replace(/^postgresql:\/\//, ""); + const boundary = rest.search(/[/?#]/); + const boundaryIdx = boundary === -1 ? rest.length : boundary; + const authority = rest.slice(0, boundaryIdx); + const suffix = rest.slice(boundaryIdx); + const atIdx = authority.indexOf("@"); + const sanitizedAuthority = (() => { + if (atIdx === -1) return authority; + const userinfo = authority.slice(0, atIdx); + const afterAt = authority.slice(atIdx + 1); + const colonIdx = userinfo.indexOf(":"); + const user = colonIdx === -1 ? userinfo : userinfo.slice(0, colonIdx); + return `${user}@${afterAt}`; + })(); + + const queryMatch = suffix.match(/^([^?#]*)(?:\?([^#]*))?(#.*)?$/); + const [, path = "", query = "", frag = ""] = queryMatch ?? ["", "", "", ""]; + const params = query.length > 0 ? query.split("&").filter((kv) => !kv.startsWith("password=")) : []; + const cleanedSuffix = path + (params.length > 0 ? `?${params.join("&")}` : "") + frag; + + return `postgresql://${sanitizedAuthority}${cleanedSuffix}`; +} + function fakeBin(root: string, bins: Record): string { const bin = join(root, "bin"); mkdirSync(bin, { recursive: true }); @@ -194,7 +235,7 @@ describe("self-host verify-backup script", () => { VERIFY_RESTORE_SCRATCH: "1", GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: live, }, - { pg_restore: PG_RESTORE, psql: fakePsql({ [live]: "same-cluster@10.0.0.5:5432/live" }) }, + { pg_restore: PG_RESTORE, psql: fakePsql({ [sanitizedUrl(live)]: "same-cluster@10.0.0.5:5432/live" }) }, ); expect(r.status).toBe(1); @@ -223,8 +264,8 @@ describe("self-host verify-backup script", () => { // Both URLs resolve to the identical real connection identity, exactly as they would in production // if they point at the same Postgres server/database despite the different spelling. psql: fakePsql({ - [live]: "gittensory@10.0.0.5:5432", - [scratch]: "gittensory@10.0.0.5:5432", + [sanitizedUrl(live)]: "gittensory@10.0.0.5:5432", + [sanitizedUrl(scratch)]: "gittensory@10.0.0.5:5432", }), }, ); @@ -268,7 +309,7 @@ describe("self-host verify-backup script", () => { GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: scratch, }, // Scratch resolves fine, but the live URL has no mapping — its identity query fails. - { pg_restore: PG_RESTORE, psql: fakePsql({ [scratch]: "gittensory@10.0.0.9:5432/scratch" }) }, + { pg_restore: PG_RESTORE, psql: fakePsql({ [sanitizedUrl(scratch)]: "gittensory@10.0.0.9:5432/scratch" }) }, ); expect(r.status).toBe(1); @@ -291,7 +332,10 @@ describe("self-host verify-backup script", () => { }, { pg_restore: PG_RESTORE, - psql: fakePsql({ [live]: "gittensory@10.0.0.5:5432/live", [scratch]: "gittensory@10.0.0.5:5432/scratch" }, "42"), + psql: fakePsql( + { [sanitizedUrl(live)]: "gittensory@10.0.0.5:5432/live", [sanitizedUrl(scratch)]: "gittensory@10.0.0.5:5432/scratch" }, + "42", + ), }, ); @@ -299,6 +343,53 @@ describe("self-host verify-backup script", () => { expect(r.out).toContain("scratch restore OK: 42 tables"); }); + it("never passes a password to psql/pg_restore argv across the full scratch-restore flow, and never leaks one URL's password onto a different URL's connection", () => { + const root = tmpRoot(); + writePgDump(root, "gittensory-a.dump", true); + // live has NO password; scratch supplies its password via the libpq query-string form (not userinfo) + // -- both must be handled, and the live connection (checked between two scratch connections) must + // never see a PGPASSFILE left over from scratch's. + const live = "postgres://gittensory@postgres/gittensory"; + const scratch = "postgresql://gittensory@postgres/scratch?password=SuperSecret123%21"; + const captureFile = join(root, "pg-capture.log"); + + const r = runVerify( + root, + [], + { + GITTENSORY_BACKUP_SOURCE_DATABASE_URL: live, + VERIFY_RESTORE_SCRATCH: "1", + GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: scratch, + PG_CAPTURE_FILE: captureFile, + }, + { + pg_restore: PG_RESTORE, + psql: fakePsql( + { [sanitizedUrl(scratch)]: "gittensory@10.0.0.5:5432/scratch", [sanitizedUrl(live)]: "gittensory@10.0.0.5:5432/live" }, + "7", + ), + }, + ); + + expect(r.status).toBe(0); + expect(r.out).toContain("scratch restore OK: 7 tables"); + const capture = execFileSync("cat", [captureFile], { encoding: "utf8" }); + const lines = capture.trim().split("\n"); + // 5 connections in order: pg_restore --list (structural validation, no URL involved yet), + // db_identity(scratch), db_identity(live), pg_restore --dbname scratch, psql scratch (sanity query). + expect(lines).toHaveLength(5); + expect(capture).not.toContain("SuperSecret123"); + expect(capture).not.toContain("password="); + // The scratch connections (indices 1, 3, 4) must show a real PGPASSFILE... + expect(lines[1]).toContain("PGPASSFILE=/"); + expect(lines[3]).toContain("PGPASSFILE=/"); + expect(lines[4]).toContain("PGPASSFILE=/"); + // ...but the live connection sandwiched between two scratch ones (index 2) must NOT inherit scratch's + // PGPASSFILE, since live's own URL has no password at all. + expect(lines[2]).toContain("PGPASSFILE="); + expect(lines[2]).not.toMatch(/PGPASSFILE=\/./); + }); + it("verifies an explicit dump path argument", () => { const root = tmpRoot(); const target = writePgDump(root, "chosen.dump", true); From fddba9430302633727f971d8e4fdd1bf9e5a5b9c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:52:24 -0700 Subject: [PATCH 2/4] 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. --- scripts/verify-backup.sh | 25 ++++--- .../selfhost-verify-backup-script.test.ts | 67 +++++++++++++++++++ 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/scripts/verify-backup.sh b/scripts/verify-backup.sh index c8aae43e92..ff05cbfd05 100644 --- a/scripts/verify-backup.sh +++ b/scripts/verify-backup.sh @@ -111,18 +111,21 @@ pg_connect_arg() { ;; esac + # A malformed (but not rejected by libpq's own parser) URL could repeat `password=` -- loop until none + # remain rather than stripping only the first, so a leftover second occurrence can never survive into + # $PG_SANITIZED_URL. Each iteration overwrites pg_password_value, so the LAST occurrence wins; which one + # libpq itself would actually authenticate with is unspecified for a duplicate key, but every occurrence + # is a credential either way and none may reach argv. pg_query_wrapped="&$pg_query&" - case "$pg_query_wrapped" in - *"&password="*"&"*) - pg_before_pw=${pg_query_wrapped%%&password=*} - pg_from_pw=${pg_query_wrapped#*&password=} - pg_password_value=$(url_decode "${pg_from_pw%%&*}") - pg_after_pw=${pg_from_pw#*&} - pg_query_wrapped="${pg_before_pw}&${pg_after_pw}" - pg_query=${pg_query_wrapped#&} - pg_query=${pg_query%&} - ;; - esac + while case "$pg_query_wrapped" in *"&password="*"&"*) true ;; *) false ;; esac; do + pg_before_pw=${pg_query_wrapped%%&password=*} + pg_from_pw=${pg_query_wrapped#*&password=} + pg_password_value=$(url_decode "${pg_from_pw%%&*}") + pg_after_pw=${pg_from_pw#*&} + pg_query_wrapped="${pg_before_pw}&${pg_after_pw}" + done + pg_query=${pg_query_wrapped#&} + pg_query=${pg_query%&} pg_suffix=$pg_path if [ -n "$pg_query" ]; then pg_suffix="$pg_suffix?$pg_query"; fi diff --git a/test/unit/selfhost-verify-backup-script.test.ts b/test/unit/selfhost-verify-backup-script.test.ts index d0b212bc03..8a31926e64 100644 --- a/test/unit/selfhost-verify-backup-script.test.ts +++ b/test/unit/selfhost-verify-backup-script.test.ts @@ -390,6 +390,73 @@ describe("self-host verify-backup script", () => { expect(lines[2]).not.toMatch(/PGPASSFILE=\/./); }); + it("strips EVERY occurrence of a repeated query-string password, not just the first", () => { + const root = tmpRoot(); + writePgDump(root, "gittensory-a.dump", true); + const captureFile = join(root, "pg-capture.log"); + // A malformed URL repeating `password=` isn't rejected by libpq's own parser -- stripping only the + // first occurrence would leave a second one sitting in argv, still a leaked credential regardless of + // which one libpq itself would actually authenticate with. + const scratch = "postgresql://u@h/scratch?password=oneSecret&sslmode=require&password=twoSecret"; + + const r = runVerify( + root, + [], + { + GITTENSORY_BACKUP_SOURCE_DATABASE_URL: "postgres://u:p@h/live", + VERIFY_RESTORE_SCRATCH: "1", + GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: scratch, + PG_CAPTURE_FILE: captureFile, + }, + // No identity entries: db_identity(scratch) fails before ever reaching db_identity(live) or the + // actual restore -- the point of this test is what reached argv along the way, not the happy path. + { pg_restore: PG_RESTORE, psql: fakePsql({}) }, + ); + + expect(r.status).toBe(1); + expect(r.out).toContain("could not connect to the scratch database"); + const capture = execFileSync("cat", [captureFile], { encoding: "utf8" }); + expect(capture).not.toContain("oneSecret"); + expect(capture).not.toContain("twoSecret"); + expect(capture).not.toContain("password="); + expect(capture).toContain("postgresql://u@h/scratch?sslmode=require"); + }); + + it("proves the userinfo-password form through the same full scratch-restore flow as the query-string form", () => { + const root = tmpRoot(); + writePgDump(root, "gittensory-a.dump", true); + const captureFile = join(root, "pg-capture.log"); + // Mirrors the query-string-password test above, but with the password in userinfo instead -- both + // forms must be proven through the identical multi-connection flow (identity checks, the actual + // restore, and the sanity query), not just in isolation. + const live = "postgresql://gittensory@postgres/gittensory"; + const scratch = "postgres://gittensory:SuperSecret123%21@postgres/scratch"; + + const r = runVerify( + root, + [], + { + GITTENSORY_BACKUP_SOURCE_DATABASE_URL: live, + VERIFY_RESTORE_SCRATCH: "1", + GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: scratch, + PG_CAPTURE_FILE: captureFile, + }, + { + pg_restore: PG_RESTORE, + psql: fakePsql( + { [sanitizedUrl(scratch)]: "gittensory@10.0.0.5:5432/scratch", [sanitizedUrl(live)]: "gittensory@10.0.0.5:5432/live" }, + "9", + ), + }, + ); + + expect(r.status).toBe(0); + expect(r.out).toContain("scratch restore OK: 9 tables"); + const capture = execFileSync("cat", [captureFile], { encoding: "utf8" }); + expect(capture).not.toContain("SuperSecret123"); + expect(capture.trim().split("\n")).toHaveLength(5); + }); + it("verifies an explicit dump path argument", () => { const root = tmpRoot(); const target = writePgDump(root, "chosen.dump", true); From 7aa5f797481883ea0a5b94bcc5d8a18bb9dd0e17 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 00:06:37 -0700 Subject: [PATCH 3/4] 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. --- scripts/verify-backup.sh | 40 ++++++++++++------- .../selfhost-verify-backup-script.test.ts | 30 ++++++++++++++ 2 files changed, 56 insertions(+), 14 deletions(-) diff --git a/scripts/verify-backup.sh b/scripts/verify-backup.sh index ff05cbfd05..938fee44e2 100644 --- a/scripts/verify-backup.sh +++ b/scripts/verify-backup.sh @@ -111,21 +111,33 @@ pg_connect_arg() { ;; esac - # A malformed (but not rejected by libpq's own parser) URL could repeat `password=` -- loop until none - # remain rather than stripping only the first, so a leftover second occurrence can never survive into - # $PG_SANITIZED_URL. Each iteration overwrites pg_password_value, so the LAST occurrence wins; which one - # libpq itself would actually authenticate with is unspecified for a duplicate key, but every occurrence - # is a credential either way and none may reach argv. - pg_query_wrapped="&$pg_query&" - while case "$pg_query_wrapped" in *"&password="*"&"*) true ;; *) false ;; esac; do - pg_before_pw=${pg_query_wrapped%%&password=*} - pg_from_pw=${pg_query_wrapped#*&password=} - pg_password_value=$(url_decode "${pg_from_pw%%&*}") - pg_after_pw=${pg_from_pw#*&} - pg_query_wrapped="${pg_before_pw}&${pg_after_pw}" + # libpq percent-decodes query KEY NAMES before matching them against connection keywords, so + # `pass%77ord=secret` (%77 = 'w') is just as much a password as a literal `password=secret` -- a literal + # string match against "&password=" (an earlier version of this loop) would miss it entirely, leaving a + # real credential in $PG_SANITIZED_URL. Walk each '&'-separated pair individually (a trailing '&' is + # appended so the last real pair is terminated the same as every other), decode ONLY the key half of + # each to compare it against "password", and rebuild the query from every pair whose decoded key isn't + # "password" -- in original order, values left percent-encoded exactly as given (they're not being + # re-parsed, just passed through to libpq, which decodes them itself). A malformed (but not rejected by + # libpq's own parser) URL repeating the key is handled naturally: each match overwrites + # pg_password_value, so the LAST occurrence wins -- which one libpq itself would authenticate with is + # unspecified for a duplicate key, but every occurrence is a credential either way, so none may reach argv. + pg_remaining="$pg_query&" + pg_query="" + while [ -n "$pg_remaining" ]; do + pg_pair=${pg_remaining%%&*} + pg_remaining=${pg_remaining#*&} + if [ -z "$pg_pair" ]; then continue; fi + case "$pg_pair" in + *=*) pg_key_raw=${pg_pair%%=*}; pg_val_raw=${pg_pair#*=} ;; + *) pg_key_raw=$pg_pair; pg_val_raw="" ;; + esac + if [ "$(url_decode "$pg_key_raw")" = "password" ]; then + pg_password_value=$(url_decode "$pg_val_raw") + else + if [ -n "$pg_query" ]; then pg_query="$pg_query&$pg_pair"; else pg_query=$pg_pair; fi + fi done - pg_query=${pg_query_wrapped#&} - pg_query=${pg_query%&} pg_suffix=$pg_path if [ -n "$pg_query" ]; then pg_suffix="$pg_suffix?$pg_query"; fi diff --git a/test/unit/selfhost-verify-backup-script.test.ts b/test/unit/selfhost-verify-backup-script.test.ts index 8a31926e64..8cc6e60d22 100644 --- a/test/unit/selfhost-verify-backup-script.test.ts +++ b/test/unit/selfhost-verify-backup-script.test.ts @@ -422,6 +422,36 @@ describe("self-host verify-backup script", () => { expect(capture).toContain("postgresql://u@h/scratch?sslmode=require"); }); + it("strips a query-string password even when its KEY NAME is percent-encoded", () => { + const root = tmpRoot(); + writePgDump(root, "gittensory-a.dump", true); + const captureFile = join(root, "pg-capture.log"); + // libpq percent-decodes query KEY NAMES before matching them against connection keywords, so + // pass%77ord (%77 = 'w') is just as much `password` as the literal spelling -- a literal string match + // against "password=" would miss it entirely, leaving a real credential in argv. + const scratch = "postgresql://u@h/scratch?sslmode=require&pass%77ord=SuperSecret123%21&application_name=app"; + + const r = runVerify( + root, + [], + { + GITTENSORY_BACKUP_SOURCE_DATABASE_URL: "postgres://u:p@h/live", + VERIFY_RESTORE_SCRATCH: "1", + GITTENSORY_VERIFY_SCRATCH_DATABASE_URL: scratch, + PG_CAPTURE_FILE: captureFile, + }, + { pg_restore: PG_RESTORE, psql: fakePsql({}) }, + ); + + expect(r.status).toBe(1); + expect(r.out).toContain("could not connect to the scratch database"); + const capture = execFileSync("cat", [captureFile], { encoding: "utf8" }); + expect(capture).not.toContain("SuperSecret123"); + expect(capture).not.toContain("pass%77ord"); + expect(capture).not.toContain("password="); + expect(capture).toContain("postgresql://u@h/scratch?sslmode=require&application_name=app"); + }); + it("proves the userinfo-password form through the same full scratch-restore flow as the query-string form", () => { const root = tmpRoot(); writePgDump(root, "gittensory-a.dump", true); From 2013ee29c1799c5506d5a2bcfa31254a01191903 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 00:36:09 -0700 Subject: [PATCH 4/4] 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. --- scripts/verify-backup.sh | 20 ++++++++++++---- .../selfhost-verify-backup-script.test.ts | 23 +++++++++++++++++-- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/scripts/verify-backup.sh b/scripts/verify-backup.sh index 938fee44e2..14110cf58e 100644 --- a/scripts/verify-backup.sh +++ b/scripts/verify-backup.sh @@ -59,6 +59,10 @@ pgpass_escape() { # doesn't have one of its own. Sets $PG_SANITIZED_URL; exports PGPASSFILE (tracked in $PG_PASSFILES for # cleanup) if the given URL had a password. pg_connect_arg() { + # Cleared up front, not just when this URL turns out to have no password: any helper command invoked + # below (e.g. url_decode) would otherwise inherit a still-exported PGPASSFILE left over from a PREVIOUS + # call for a different URL, for the whole duration of this function's parsing work. + unset PGPASSFILE pg_rest=${1#postgres://} pg_rest=${pg_rest#postgresql://} @@ -144,7 +148,6 @@ pg_connect_arg() { pg_suffix="$pg_suffix$pg_frag" PG_SANITIZED_URL="postgresql://$pg_sanitized_authority$pg_suffix" - unset PGPASSFILE if [ -n "$pg_password_value" ]; then # pgpass is a single-line-per-entry format; pgpass_escape only handles the two characters (':' and # '\') that format itself treats specially. A decoded password containing a raw newline or carriage @@ -223,20 +226,27 @@ verify_postgres() { # the same cluster as distinct (a legitimate, common scratch-DB setup). No special privilege is required: # PUBLIC has EXECUTE on pg_control_system() by default. Any failure to fingerprint EITHER side aborts (fail # closed) rather than assuming the databases differ. + # Takes an ALREADY-sanitized URL, not the raw one -- pg_connect_arg must be called by the caller in the + # PARENT shell before invoking this via command substitution ($(db_identity ...)), never from inside + # this function's own body. Command substitution always forks a subshell, and pg_connect_arg's + # PG_PASSFILES-tracking side effect would be silently lost when that subshell exits (subshells get a + # copy of the parent's variables; changes never propagate back out), orphaning a real, + # credential-bearing 600-permission temp file on disk with no owner left to clean it up. db_identity() { - pg_connect_arg "$1" - psql "$PG_SANITIZED_URL" -X -q -t -A -v ON_ERROR_STOP=1 \ + psql "$1" -X -q -t -A -v ON_ERROR_STOP=1 \ -c "SELECT current_database() || '@' || (SELECT system_identifier FROM pg_control_system())::text" \ 2>/dev/null } - scratch_identity="$(db_identity "$scratch")" || scratch_identity="" + pg_connect_arg "$scratch" + scratch_identity="$(db_identity "$PG_SANITIZED_URL")" || scratch_identity="" if [ -z "$scratch_identity" ]; then echo "[verify] could not connect to the scratch database to verify its identity; refusing to proceed" >&2 return 1 fi case "$PG_DB" in postgres://* | postgresql://*) - live_identity="$(db_identity "$PG_DB")" || live_identity="" + pg_connect_arg "$PG_DB" + live_identity="$(db_identity "$PG_SANITIZED_URL")" || live_identity="" if [ -z "$live_identity" ]; then echo "[verify] could not connect to the live backup source to verify its identity; refusing to proceed" >&2 return 1 diff --git a/test/unit/selfhost-verify-backup-script.test.ts b/test/unit/selfhost-verify-backup-script.test.ts index 8cc6e60d22..3a537a461c 100644 --- a/test/unit/selfhost-verify-backup-script.test.ts +++ b/test/unit/selfhost-verify-backup-script.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { gzipSync } from "node:zlib"; @@ -112,7 +112,17 @@ function sanitizedUrl(url: string): string { const queryMatch = suffix.match(/^([^?#]*)(?:\?([^#]*))?(#.*)?$/); const [, path = "", query = "", frag = ""] = queryMatch ?? ["", "", "", ""]; - const params = query.length > 0 ? query.split("&").filter((kv) => !kv.startsWith("password=")) : []; + // Mirrors the shell's url_decode of the KEY half only -- libpq percent-decodes query key names before + // matching them against connection keywords, so `pass%77ord=` is a password key too, not just a literal + // `password=`. An invalid percent-sequence is left as-is, same as the shell's awk decoder does. + const decodeKey = (raw: string): string => { + try { + return decodeURIComponent(raw); + } catch { + return raw; + } + }; + const params = query.length > 0 ? query.split("&").filter((kv) => decodeKey(kv.split("=")[0] ?? "") !== "password") : []; const cleanedSuffix = path + (params.length > 0 ? `?${params.join("&")}` : "") + frag; return `postgresql://${sanitizedAuthority}${cleanedSuffix}`; @@ -388,6 +398,15 @@ describe("self-host verify-backup script", () => { // PGPASSFILE, since live's own URL has no password at all. expect(lines[2]).toContain("PGPASSFILE="); expect(lines[2]).not.toMatch(/PGPASSFILE=\/./); + // Every PGPASSFILE created during the run -- including the two from db_identity()'s command + // substitutions ($(db_identity ...) forks a subshell, so pg_connect_arg must be called by the caller + // in the PARENT shell for its PG_PASSFILES bookkeeping to survive) -- must be gone once the script has + // exited and its cleanup trap has run, not merely have kept the password out of argv. + for (const line of [lines[1] ?? "", lines[3] ?? "", lines[4] ?? ""]) { + const passfile = line.split("PGPASSFILE=")[1]; + expect(passfile).toMatch(/^\/.+/); + expect(existsSync(passfile ?? "")).toBe(false); + } }); it("strips EVERY occurrence of a repeated query-string password, not just the first", () => {