From 5fe2c534d6580b52eb29a032e21770aad398e19d Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 21:41:03 -0700 Subject: [PATCH 1/6] fix(selfhost): hide Postgres backup credentials --- scripts/backup.sh | 87 +++++++++++++++++++++++- test/unit/selfhost-backup-script.test.ts | 29 ++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 3e80211830..046a652be2 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -10,8 +10,92 @@ RETAIN=${BACKUP_RETAIN:-7} DB=${DATABASE_PATH:-/data/gittensory.sqlite} PG_DB="${GITTENSORY_BACKUP_SOURCE_DATABASE_URL:-${DATABASE_URL:-}}" OUT=${BACKUP_OUT_DIR:-/backups} +PGPASSFILE_CREATED="" +cleanup() { + if [ -n "$PGPASSFILE_CREATED" ]; then + rm -f "$PGPASSFILE_CREATED" + fi +} +trap cleanup EXIT HUP INT TERM mkdir -p "$OUT/postgres" "$OUT/sqlite" "$OUT/qdrant" +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 if (c == "+") { + out = out " "; + } else { + out = out c; + } + } + printf "%s", out; + }' +} + +pgpass_escape() { + printf '%s' "$1" | sed 's/\\/\\\\/g; s/:/\\:/g' +} + +prepare_pg_env() { + pg_url_no_scheme=${PG_DB#postgres://} + pg_url_no_scheme=${pg_url_no_scheme#postgresql://} + pg_without_query=${pg_url_no_scheme%%\?*} + + pg_auth="" + pg_host_path=$pg_without_query + case "$pg_without_query" in + *@*) + pg_auth=${pg_without_query%%@*} + pg_host_path=${pg_without_query#*@} + ;; + esac + + PGUSER_VALUE="" + PGPASSWORD_VALUE="" + if [ -n "$pg_auth" ]; then + PGUSER_VALUE=$(url_decode "${pg_auth%%:*}") + if [ "$pg_auth" != "${pg_auth#*:}" ]; then + PGPASSWORD_VALUE=$(url_decode "${pg_auth#*:}") + fi + fi + + pg_host_port=${pg_host_path%%/*} + PGDATABASE_VALUE=$(url_decode "${pg_host_path#*/}") + if [ "$PGDATABASE_VALUE" = "$pg_host_path" ] || [ -z "$PGDATABASE_VALUE" ]; then + PGDATABASE_VALUE=postgres + fi + + PGHOST_VALUE=${pg_host_port%%:*} + PGPORT_VALUE=${pg_host_port#*:} + if [ "$PGPORT_VALUE" = "$pg_host_port" ]; then + PGPORT_VALUE=5432 + fi + + export PGHOST="$PGHOST_VALUE" PGPORT="$PGPORT_VALUE" PGDATABASE="$PGDATABASE_VALUE" + if [ -n "$PGUSER_VALUE" ]; then + export PGUSER="$PGUSER_VALUE" + fi + + if [ -n "$PGPASSWORD_VALUE" ]; then + PGPASSFILE_CREATED=$(mktemp "${TMPDIR:-/tmp}/gittensory-pgpass.XXXXXX") + chmod 600 "$PGPASSFILE_CREATED" + printf '%s:%s:%s:%s:%s\n' \ + "$(pgpass_escape "$PGHOST_VALUE")" \ + "$(pgpass_escape "$PGPORT_VALUE")" \ + "$(pgpass_escape "$PGDATABASE_VALUE")" \ + "$(pgpass_escape "${PGUSER_VALUE:-*}")" \ + "$(pgpass_escape "$PGPASSWORD_VALUE")" > "$PGPASSFILE_CREATED" + export PGPASSFILE="$PGPASSFILE_CREATED" + fi +} + # 1) Active app database. Prefer Postgres when DATABASE_URL is set; otherwise keep the SQLite online backup path. case "$PG_DB" in postgres://*|postgresql://*) @@ -19,7 +103,8 @@ case "$PG_DB" in echo "[backup] pg_dump not found; cannot back up Postgres database" >&2 exit 1 fi - pg_dump -Fc -f "$OUT/postgres/gittensory-$TS.dump" "$PG_DB" + prepare_pg_env + pg_dump -Fc -f "$OUT/postgres/gittensory-$TS.dump" echo "[backup] postgres -> $OUT/postgres/gittensory-$TS.dump" ;; *) diff --git a/test/unit/selfhost-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index c9cfd60328..297d67bd30 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -28,6 +28,7 @@ function fakePgDump(root: string): string { join(bin, "pg_dump"), `#!/bin/sh out='' +original_args="$*" while [ "$#" -gt 0 ]; do case "$1" in -f) @@ -43,6 +44,12 @@ if [ -z "$out" ]; then echo 'missing -f output' >&2 exit 2 fi +if [ -n "\${PG_DUMP_ARGS_FILE:-}" ]; then + printf '%s\\n' "$original_args" > "$PG_DUMP_ARGS_FILE" +fi +if [ -n "\${PG_DUMP_ENV_FILE:-}" ]; then + printf '%s\\n' "$PGHOST|$PGPORT|$PGDATABASE|\${PGUSER:-}|\${PGPASSFILE:-}" > "$PG_DUMP_ENV_FILE" +fi printf 'postgres dump\\n' > "$out" `, ); @@ -91,6 +98,8 @@ describe("self-host backup script", () => { const output = runBackup(root, { DATABASE_URL: "postgres://gittensory:pw@postgres:5432/gittensory", + PG_DUMP_ARGS_FILE: join(root, "pg-dump.args"), + PG_DUMP_ENV_FILE: join(root, "pg-dump.env"), DATABASE_PATH: staleSqlite, PATH: `${pgBin}:${process.env.PATH ?? ""}`, }); @@ -102,6 +111,26 @@ describe("self-host backup script", () => { expect(readdirSync(join(root, "backups", "sqlite"))).toEqual([]); }); + it("does not pass Postgres credentials in pg_dump arguments", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + const envFile = join(root, "pg-dump.env"); + + runBackup(root, { + DATABASE_URL: "postgresql://app_user:SuperSecret123%21@db.example:6543/gittensory", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + PG_DUMP_ENV_FILE: envFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + const pgEnv = execFileSync("cat", [envFile], { encoding: "utf8" }).trim(); + expect(args).not.toContain("SuperSecret123"); + expect(args).not.toContain("postgresql://"); + expect(pgEnv).toMatch(/^db\.example\|6543\|gittensory\|app_user\|\/tmp\/gittensory-pgpass\./); + }); + it("keeps the SQLite online backup path when no Postgres URL is configured", () => { const root = tmpRoot(); const sqliteBin = fakeSqlite(root); From 71b01b603a7afb86e0ec744d2d1e39529ad3f3b0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 22:41:18 -0700 Subject: [PATCH 2/6] fix(selfhost): stop dropping the query string from the Postgres backup 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 (`*:*:*:*:`) 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. --- scripts/backup.sh | 77 +++++++++++------------- test/unit/selfhost-backup-script.test.ts | 59 ++++++++++++++++-- 2 files changed, 89 insertions(+), 47 deletions(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 046a652be2..886d87fd9b 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -19,6 +19,10 @@ cleanup() { trap cleanup EXIT HUP INT TERM mkdir -p "$OUT/postgres" "$OUT/sqlite" "$OUT/qdrant" +# 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); } @@ -29,8 +33,6 @@ url_decode() { 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 if (c == "+") { - out = out " "; } else { out = out c; } @@ -43,55 +45,44 @@ pgpass_escape() { printf '%s' "$1" | sed 's/\\/\\\\/g; s/:/\\:/g' } +# Strips only the password from a postgres(ql):// URI and hands pg_dump the rest untouched (host, port, +# dbname, and the FULL query string) as its connection argument, instead of re-parsing those pieces +# ourselves. libpq's own URI parser already handles every form it needs to (query-string-only host, as in +# `postgresql:///db?host=/var/run/postgresql`, IPv6 literals, multi-host strings, sslmode, etc.) -- an +# earlier version of this function extracted host/port/dbname manually and discarded the query string +# entirely, silently breaking any URL that relied on it. Sets $PG_SANITIZED_URL (the password-free URI to +# pass to pg_dump) and, if a password was present, a $PGPASSFILE_CREATED wildcard passfile (host/port/ +# dbname/user are wildcarded: this file exists only to supply the ONE password for the single connection +# this script makes, and is deleted immediately after via the `cleanup` trap, so there's no scoped value +# in re-deriving the exact host/port/dbname libpq will resolve -- which the query string can override +# anyway -- just to match them precisely). prepare_pg_env() { - pg_url_no_scheme=${PG_DB#postgres://} - pg_url_no_scheme=${pg_url_no_scheme#postgresql://} - pg_without_query=${pg_url_no_scheme%%\?*} + pg_rest=${PG_DB#postgres://} + pg_rest=${pg_rest#postgresql://} - pg_auth="" - pg_host_path=$pg_without_query - case "$pg_without_query" in + PG_SANITIZED_URL="postgresql://$pg_rest" + PGPASSWORD_VALUE="" + case "$pg_rest" in *@*) - pg_auth=${pg_without_query%%@*} - pg_host_path=${pg_without_query#*@} + pg_userinfo=${pg_rest%%@*} + pg_after_at=${pg_rest#*@} + case "$pg_userinfo" in + *:*) + pg_user_part=${pg_userinfo%%:*} + PGPASSWORD_VALUE=$(url_decode "${pg_userinfo#*:}") + PG_SANITIZED_URL="postgresql://${pg_user_part}@${pg_after_at}" + ;; + *) + PG_SANITIZED_URL="postgresql://${pg_userinfo}@${pg_after_at}" + ;; + esac ;; esac - PGUSER_VALUE="" - PGPASSWORD_VALUE="" - if [ -n "$pg_auth" ]; then - PGUSER_VALUE=$(url_decode "${pg_auth%%:*}") - if [ "$pg_auth" != "${pg_auth#*:}" ]; then - PGPASSWORD_VALUE=$(url_decode "${pg_auth#*:}") - fi - fi - - pg_host_port=${pg_host_path%%/*} - PGDATABASE_VALUE=$(url_decode "${pg_host_path#*/}") - if [ "$PGDATABASE_VALUE" = "$pg_host_path" ] || [ -z "$PGDATABASE_VALUE" ]; then - PGDATABASE_VALUE=postgres - fi - - PGHOST_VALUE=${pg_host_port%%:*} - PGPORT_VALUE=${pg_host_port#*:} - if [ "$PGPORT_VALUE" = "$pg_host_port" ]; then - PGPORT_VALUE=5432 - fi - - export PGHOST="$PGHOST_VALUE" PGPORT="$PGPORT_VALUE" PGDATABASE="$PGDATABASE_VALUE" - if [ -n "$PGUSER_VALUE" ]; then - export PGUSER="$PGUSER_VALUE" - fi - if [ -n "$PGPASSWORD_VALUE" ]; then PGPASSFILE_CREATED=$(mktemp "${TMPDIR:-/tmp}/gittensory-pgpass.XXXXXX") chmod 600 "$PGPASSFILE_CREATED" - printf '%s:%s:%s:%s:%s\n' \ - "$(pgpass_escape "$PGHOST_VALUE")" \ - "$(pgpass_escape "$PGPORT_VALUE")" \ - "$(pgpass_escape "$PGDATABASE_VALUE")" \ - "$(pgpass_escape "${PGUSER_VALUE:-*}")" \ - "$(pgpass_escape "$PGPASSWORD_VALUE")" > "$PGPASSFILE_CREATED" + printf '*:*:*:*:%s\n' "$(pgpass_escape "$PGPASSWORD_VALUE")" > "$PGPASSFILE_CREATED" export PGPASSFILE="$PGPASSFILE_CREATED" fi } @@ -104,7 +95,7 @@ case "$PG_DB" in exit 1 fi prepare_pg_env - pg_dump -Fc -f "$OUT/postgres/gittensory-$TS.dump" + pg_dump -Fc -f "$OUT/postgres/gittensory-$TS.dump" "$PG_SANITIZED_URL" echo "[backup] postgres -> $OUT/postgres/gittensory-$TS.dump" ;; *) diff --git a/test/unit/selfhost-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index 297d67bd30..1e464b3119 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -48,7 +48,12 @@ if [ -n "\${PG_DUMP_ARGS_FILE:-}" ]; then printf '%s\\n' "$original_args" > "$PG_DUMP_ARGS_FILE" fi if [ -n "\${PG_DUMP_ENV_FILE:-}" ]; then - printf '%s\\n' "$PGHOST|$PGPORT|$PGDATABASE|\${PGUSER:-}|\${PGPASSFILE:-}" > "$PG_DUMP_ENV_FILE" + passfile_path="\${PGPASSFILE:-}" + passfile_content="" + if [ -n "$passfile_path" ] && [ -f "$passfile_path" ]; then + passfile_content="$(cat "$passfile_path")" + fi + printf '%s|%s\\n' "$passfile_path" "$passfile_content" > "$PG_DUMP_ENV_FILE" fi printf 'postgres dump\\n' > "$out" `, @@ -111,7 +116,7 @@ describe("self-host backup script", () => { expect(readdirSync(join(root, "backups", "sqlite"))).toEqual([]); }); - it("does not pass Postgres credentials in pg_dump arguments", () => { + it("does not pass the Postgres password in pg_dump arguments, but keeps host/port/dbname/user reachable via a sanitized URL", () => { const root = tmpRoot(); const pgBin = fakePgDump(root); const argsFile = join(root, "pg-dump.args"); @@ -126,9 +131,55 @@ describe("self-host backup script", () => { const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); const pgEnv = execFileSync("cat", [envFile], { encoding: "utf8" }).trim(); + const [passfilePath, passfileContent] = pgEnv.split("|"); + + // The password (percent-encoded or decoded) must never appear on argv. expect(args).not.toContain("SuperSecret123"); - expect(args).not.toContain("postgresql://"); - expect(pgEnv).toMatch(/^db\.example\|6543\|gittensory\|app_user\|\/tmp\/gittensory-pgpass\./); + expect(args).not.toContain("app_user:"); + // Host/port/dbname/user are pg_dump's connection info, not secrets -- libpq resolves them from this + // sanitized (password-free) URL exactly as it would have from the original. + expect(args).toContain("postgresql://app_user@db.example:6543/gittensory"); + // The password reaches pg_dump out-of-band via a 600-permission PGPASSFILE, url-decoded. Match on the + // basename only, not a hardcoded /tmp/ prefix: mktemp resolves under $TMPDIR, which macOS sets to a + // per-user private directory rather than /tmp (the CI runner's Linux environment does default to /tmp, + // but the assertion shouldn't assume that). + expect(passfilePath).toMatch(/\/gittensory-pgpass\.[^/]+$/); + expect(passfileContent).toBe("*:*:*:*:SuperSecret123!"); + }); + + it("preserves query-string-only connection info that a host/port/dbname split would otherwise drop", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + + // No authority host at all -- the actual connection target is supplied entirely via the query string + // (a valid, real-world libpq URI form for connecting over a Unix socket at a non-default path). + runBackup(root, { + DATABASE_URL: "postgresql:///gittensory?host=/var/run/postgresql", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + expect(args).toContain("postgresql:///gittensory?host=/var/run/postgresql"); + }); + + it("keeps a literal '+' in the password as '+', not a decoded space", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const envFile = join(root, "pg-dump.env"); + + // '+' means "space" only in application/x-www-form-urlencoded query values, not in a URI's userinfo + // component, where it's an ordinary allowed character -- decoding it as a space would corrupt any + // password containing one. + runBackup(root, { + DATABASE_URL: "postgres://user:pass+word@host/db", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ENV_FILE: envFile, + }); + + const [, passfileContent] = execFileSync("cat", [envFile], { encoding: "utf8" }).trim().split("|"); + expect(passfileContent).toBe("*:*:*:*:pass+word"); }); it("keeps the SQLite online backup path when no Postgres URL is configured", () => { From 8638acb4a3c34310e79def1f22e794f4a7918308 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:02:52 -0700 Subject: [PATCH 3/6] fix(selfhost): reject a newline/CR in the decoded backup password 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. --- scripts/backup.sh | 16 +++++++++++++++ test/unit/selfhost-backup-script.test.ts | 25 ++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 886d87fd9b..3ba7d92ea0 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -80,6 +80,22 @@ prepare_pg_env() { esac if [ -n "$PGPASSWORD_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. + # + # NOTE: "$(printf '\n')" as a case pattern would NOT work 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 "$PGPASSWORD_VALUE" in + *"$pg_nl"*|*"$pg_cr"*) + echo "[backup] refusing to write PGPASSFILE: decoded Postgres password contains a newline or carriage return" >&2 + exit 1 + ;; + esac PGPASSFILE_CREATED=$(mktemp "${TMPDIR:-/tmp}/gittensory-pgpass.XXXXXX") chmod 600 "$PGPASSFILE_CREATED" printf '*:*:*:*:%s\n' "$(pgpass_escape "$PGPASSWORD_VALUE")" > "$PGPASSFILE_CREATED" diff --git a/test/unit/selfhost-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index 1e464b3119..d16dc84c48 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -50,10 +50,15 @@ fi if [ -n "\${PG_DUMP_ENV_FILE:-}" ]; then passfile_path="\${PGPASSFILE:-}" passfile_content="" + passfile_mode_ok="no" if [ -n "$passfile_path" ] && [ -f "$passfile_path" ]; then passfile_content="$(cat "$passfile_path")" + # find -perm is portable across BSD (macOS) and GNU (Linux) find, unlike \`stat\`'s incompatible flags. + case "$(find "$passfile_path" -perm 600 2>/dev/null)" in + "$passfile_path") passfile_mode_ok="yes" ;; + esac fi - printf '%s|%s\\n' "$passfile_path" "$passfile_content" > "$PG_DUMP_ENV_FILE" + printf '%s|%s|%s\\n' "$passfile_path" "$passfile_content" "$passfile_mode_ok" > "$PG_DUMP_ENV_FILE" fi printf 'postgres dump\\n' > "$out" `, @@ -131,7 +136,7 @@ describe("self-host backup script", () => { const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); const pgEnv = execFileSync("cat", [envFile], { encoding: "utf8" }).trim(); - const [passfilePath, passfileContent] = pgEnv.split("|"); + const [passfilePath, passfileContent, passfileModeOk] = pgEnv.split("|"); // The password (percent-encoded or decoded) must never appear on argv. expect(args).not.toContain("SuperSecret123"); @@ -145,6 +150,7 @@ describe("self-host backup script", () => { // but the assertion shouldn't assume that). expect(passfilePath).toMatch(/\/gittensory-pgpass\.[^/]+$/); expect(passfileContent).toBe("*:*:*:*:SuperSecret123!"); + expect(passfileModeOk).toBe("yes"); }); it("preserves query-string-only connection info that a host/port/dbname split would otherwise drop", () => { @@ -182,6 +188,21 @@ describe("self-host backup script", () => { expect(passfileContent).toBe("*:*:*:*:pass+word"); }); + it("refuses to write a PGPASSFILE when the decoded password contains a newline", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + + // pgpass_escape only escapes ':' and '\' -- a raw newline (here percent-encoded as %0A) would still + // split the entry across lines and corrupt the single-line pgpass format, so this must fail loudly + // rather than silently write a malformed passfile. + expect(() => + runBackup(root, { + DATABASE_URL: "postgres://user:bad%0Apassword@host/db", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + }), + ).toThrow(/backup\.sh/); + }); + it("keeps the SQLite online backup path when no Postgres URL is configured", () => { const root = tmpRoot(); const sqliteBin = fakeSqlite(root); From 3f0cb28321150142228bbb3a533b06a39bae3d1e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:16:00 -0700 Subject: [PATCH 4/6] fix(selfhost): stop scanning the whole URL for userinfo, only the authority 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. --- scripts/backup.sh | 26 ++++++++++---- test/unit/selfhost-backup-script.test.ts | 44 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 3ba7d92ea0..6b9b82de47 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -60,20 +60,34 @@ prepare_pg_env() { pg_rest=${PG_DB#postgres://} pg_rest=${pg_rest#postgresql://} - PG_SANITIZED_URL="postgresql://$pg_rest" + # Userinfo (user:password@) can ONLY appear in the authority component -- everything before the first + # '/', '?', or '#' -- never later in the URI. Find that boundary FIRST and never look for '@'/':' past + # it, or a literal '@'/':' inside a query-string value (e.g. ?application_name=a:b@worker) gets + # misread as credentials, corrupting an otherwise-untouched query string. POSIX parameter expansion has + # no single "find the first of several delimiters" primitive: compute the substring before each + # candidate delimiter and keep whichever is shortest, since the delimiter that occurs earliest produces + # the shortest "before" substring. + 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_SANITIZED_URL="postgresql://$pg_authority$pg_suffix" PGPASSWORD_VALUE="" - case "$pg_rest" in + case "$pg_authority" in *@*) - pg_userinfo=${pg_rest%%@*} - pg_after_at=${pg_rest#*@} + pg_userinfo=${pg_authority%%@*} + pg_after_at=${pg_authority#*@} case "$pg_userinfo" in *:*) pg_user_part=${pg_userinfo%%:*} PGPASSWORD_VALUE=$(url_decode "${pg_userinfo#*:}") - PG_SANITIZED_URL="postgresql://${pg_user_part}@${pg_after_at}" + PG_SANITIZED_URL="postgresql://${pg_user_part}@${pg_after_at}$pg_suffix" ;; *) - PG_SANITIZED_URL="postgresql://${pg_userinfo}@${pg_after_at}" + PG_SANITIZED_URL="postgresql://${pg_userinfo}@${pg_after_at}$pg_suffix" ;; esac ;; diff --git a/test/unit/selfhost-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index d16dc84c48..de1de4ce01 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -170,6 +170,50 @@ describe("self-host backup script", () => { expect(args).toContain("postgresql:///gittensory?host=/var/run/postgresql"); }); + it("does not mistake a literal '@'/':' inside the query string for userinfo", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + const envFile = join(root, "pg-dump.env"); + + // No userinfo here at all -- the '@' and ':' both belong to a query parameter VALUE. Userinfo can + // only appear in the authority (before the first '/', '?', or '#'); scanning the whole remaining + // string for the first '@' would wrongly treat "gittensory?application_name=a:b" as userinfo and + // strip ":b" out as a fake password, corrupting the URL passed to pg_dump. + const url = "postgresql://db.example/gittensory?application_name=a:b@worker"; + runBackup(root, { + DATABASE_URL: url, + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + PG_DUMP_ENV_FILE: envFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + const [passfilePath] = execFileSync("cat", [envFile], { encoding: "utf8" }).trim().split("|"); + expect(args).toContain(url); + expect(passfilePath).toBe(""); + }); + + it("extracts a real password even when the query string separately contains '@'/':'", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + const envFile = join(root, "pg-dump.env"); + + runBackup(root, { + DATABASE_URL: "postgres://user:realpass@host/db?application_name=a:b@worker", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + PG_DUMP_ENV_FILE: envFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + const [, passfileContent] = execFileSync("cat", [envFile], { encoding: "utf8" }).trim().split("|"); + expect(args).not.toContain("realpass"); + expect(args).toContain("postgresql://user@host/db?application_name=a:b@worker"); + expect(passfileContent).toBe("*:*:*:*:realpass"); + }); + it("keeps a literal '+' in the password as '+', not a decoded space", () => { const root = tmpRoot(); const pgBin = fakePgDump(root); From 99c223473dbe4656731e461763244bd7abbfb4fb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:33:43 -0700 Subject: [PATCH 5/6] fix(selfhost): strip a password supplied via the libpq query-string form 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. --- scripts/backup.sh | 56 +++++++++++++++++++++--- test/unit/selfhost-backup-script.test.ts | 40 +++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 6b9b82de47..4677dddf32 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -45,8 +45,10 @@ pgpass_escape() { printf '%s' "$1" | sed 's/\\/\\\\/g; s/:/\\:/g' } -# Strips only the password from a postgres(ql):// URI and hands pg_dump the rest untouched (host, port, -# dbname, and the FULL query string) as its connection argument, instead of re-parsing those pieces +# 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 pg_dump everything else untouched (host, port, dbname, +# and every other query parameter) as its connection argument, instead of re-parsing those pieces # ourselves. libpq's own URI parser already handles every form it needs to (query-string-only host, as in # `postgresql:///db?host=/var/run/postgresql`, IPv6 literals, multi-host strings, sslmode, etc.) -- an # earlier version of this function extracted host/port/dbname manually and discarded the query string @@ -74,8 +76,8 @@ prepare_pg_env() { if [ ${#pg_before_frag} -lt ${#pg_authority} ]; then pg_authority=$pg_before_frag; fi pg_suffix=${pg_rest#"$pg_authority"} - PG_SANITIZED_URL="postgresql://$pg_authority$pg_suffix" PGPASSWORD_VALUE="" + pg_sanitized_authority=$pg_authority case "$pg_authority" in *@*) pg_userinfo=${pg_authority%%@*} @@ -84,15 +86,59 @@ prepare_pg_env() { *:*) pg_user_part=${pg_userinfo%%:*} PGPASSWORD_VALUE=$(url_decode "${pg_userinfo#*:}") - PG_SANITIZED_URL="postgresql://${pg_user_part}@${pg_after_at}$pg_suffix" + pg_sanitized_authority="${pg_user_part}@${pg_after_at}" ;; *) - PG_SANITIZED_URL="postgresql://${pg_userinfo}@${pg_after_at}$pg_suffix" + pg_sanitized_authority="${pg_userinfo}@${pg_after_at}" ;; esac ;; esac + # A libpq query string can carry `password=...` as an alternative to userinfo -- split $pg_suffix into + # its path / query / fragment components (in that order; each optional) and, if the query component has + # a `password` key, extract and remove it, leaving every other parameter (and their order) untouched. + 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=} + PGPASSWORD_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" + if [ -n "$PGPASSWORD_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 diff --git a/test/unit/selfhost-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index de1de4ce01..04d9b85e5c 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -170,6 +170,46 @@ describe("self-host backup script", () => { expect(args).toContain("postgresql:///gittensory?host=/var/run/postgresql"); }); + it("strips a password supplied via the libpq query-string form, not just userinfo", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + const envFile = join(root, "pg-dump.env"); + + // postgresql://user@host/db?password=... is an equally valid, if less common, way to supply a libpq + // password -- entirely independent of the userinfo form this function already strips. Includes other + // query parameters on both sides of `password` to prove they survive, in order, untouched. + runBackup(root, { + DATABASE_URL: "postgresql://app_user@db.example:6543/gittensory?sslmode=require&password=SuperSecret123%21&application_name=app", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + PG_DUMP_ENV_FILE: envFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + const [, passfileContent] = execFileSync("cat", [envFile], { encoding: "utf8" }).trim().split("|"); + expect(args).not.toContain("SuperSecret123"); + expect(args).not.toContain("password="); + expect(args).toContain("postgresql://app_user@db.example:6543/gittensory?sslmode=require&application_name=app"); + expect(passfileContent).toBe("*:*:*:*:SuperSecret123!"); + }); + + it("does not mistake a query value merely containing the substring 'password' for the password key", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + + const url = "postgresql://host/db?application_name=has_password_in_name&other=1"; + runBackup(root, { + DATABASE_URL: url, + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + expect(args).toContain(url); + }); + it("does not mistake a literal '@'/':' inside the query string for userinfo", () => { const root = tmpRoot(); const pgBin = fakePgDump(root); From 3aac594d53ca28e51a25df74cb2ab9e6e0e626af Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:48:18 -0700 Subject: [PATCH 6/6] fix(selfhost): strip every occurrence of a repeated query-string password 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. --- scripts/backup.sh | 25 +++++++++++++----------- test/unit/selfhost-backup-script.test.ts | 21 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/scripts/backup.sh b/scripts/backup.sh index 4677dddf32..568f40dc22 100644 --- a/scripts/backup.sh +++ b/scripts/backup.sh @@ -121,18 +121,21 @@ prepare_pg_env() { ;; 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 PGPASSWORD_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=} - PGPASSWORD_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=} + PGPASSWORD_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-backup-script.test.ts b/test/unit/selfhost-backup-script.test.ts index 04d9b85e5c..84fbd109cd 100644 --- a/test/unit/selfhost-backup-script.test.ts +++ b/test/unit/selfhost-backup-script.test.ts @@ -194,6 +194,27 @@ describe("self-host backup script", () => { expect(passfileContent).toBe("*:*:*:*:SuperSecret123!"); }); + it("strips EVERY occurrence of a repeated query-string password, not just the first", () => { + const root = tmpRoot(); + const pgBin = fakePgDump(root); + const argsFile = join(root, "pg-dump.args"); + + // 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. + runBackup(root, { + DATABASE_URL: "postgresql://u@h/db?password=oneSecret&sslmode=require&password=twoSecret", + PATH: `${pgBin}:${process.env.PATH ?? ""}`, + PG_DUMP_ARGS_FILE: argsFile, + }); + + const args = execFileSync("cat", [argsFile], { encoding: "utf8" }); + expect(args).not.toContain("oneSecret"); + expect(args).not.toContain("twoSecret"); + expect(args).not.toContain("password="); + expect(args).toContain("postgresql://u@h/db?sslmode=require"); + }); + it("does not mistake a query value merely containing the substring 'password' for the password key", () => { const root = tmpRoot(); const pgBin = fakePgDump(root);