Skip to content

pgconn: redact a password containing '@' in malformed connection URIs - #2694

Open
dEeds83 wants to merge 1 commit into
jackc:masterfrom
dEeds83:redact-at-sign-in-password
Open

dEeds83 wants to merge 1 commit into
jackc:masterfrom
dEeds83:redact-at-sign-in-password

Conversation

@dEeds83

@dEeds83 dEeds83 commented Oct 21, 2026

Copy link
Copy Markdown

Refs #1271.

Problem

redactPW / redactURLPassword ends the userinfo at the first @, the same lookahead the parser uses. In a malformed
URI an unencoded password may itself contain @, and everything between that first @ and the next : (or the
terminating @) is then printed in clear text in ParseConfigError. Reproduced on master (d6d4b01) and v5.11.0:

password "S3cr3t@Part2:Part3/Part4#Part5"
  cannot parse `postgres://afs:xxxxx@Part2:xxxxxx@h1:5432,h2:5432/afs?sslmode=disable`: invalid port

password "x%zz@yyLeak"
  cannot parse `postgres://afs:xxxxx@yyLeak@h1:5432,h2:5432/afs?sslmode=disable`: failed to parse as URL (invalid percent-encoded token in password)

The existing best-effort heuristic (:[^@]+@) only masks segments that start with :, so Part2 and yyLeak survive.
We hit this in production when a database password containing @ was put into a connection URI without
percent-encoding: the error was logged and parts of the password reached the log pipeline.

A related case passes uriStructureUnambiguous: postgres://user:sec@ret@host/db?sslmode=bogus parses with host
ret@host, so the userinfo is treated as exact and ret is printed.

Change

  • When the userinfo has a password and the URI is not structurally unambiguous, the mask extends to the @ that
    most plausibly ends the userinfo (brokenUserinfoEnd): the last @ before the last ? — so an @ inside a query
    value such as application_name=me@example.com is not mistaken for it — or, when no @ follows before that ? (the
    ? then belongs to the password, as in user:pass?x@host), the last @ in the string. Over-masking a malformed host
    is preferred to printing part of a password, consistent with the existing "may over-mask" contract.
  • uriStructureUnambiguous returns false when the parsed host contains @, mirroring the existing non-numeric-port
    rule: no host name contains @, so it is a mislaid part of the password.

Valid URIs are unaffected (the new path only runs when uriStructureUnambiguous is false), and the existing
TestConfigError cases pass unchanged, including a@user:sec:ret@host (no password in the first userinfo, so the old
heuristic still applies there). Five table cases were added.

Not addressed here: parser error messages that quote a raw query key (missing key/value separator "=" in URI query parameter: "…") can still carry a fragment of an unencoded password that contains ?. That mirrors libpq's message and
the existing comment on uriDecode already notes the redaction is best-effort, so I left it alone.

Testing

go test ./... against PostgreSQL 18 (PGX_TEST_DATABASE set, hstore/ltree/uint64 set up per CONTRIBUTING.md)
passes, except TestTextCodecACLItem, which needs a postgres role my test cluster lacks and fails identically on
master. go vet ./pgconn/ and gofmt are clean.

AI disclosure

This change was written with Claude (Anthropic, Claude Opus 5.5) via Claude Code. The prompts were, in substance: find out
why a database password leaks into our logs when the connection string cannot be parsed, check whether pgx already has an
issue for it, and prepare a fix for pgx as a pull request.

🤖 Generated with Claude Code

redactURLPassword ended the userinfo at the first '@'. In a malformed URI
an unencoded password may itself contain '@', and the part between that
'@' and the next ':' or the terminating '@' was printed in clear text in
ParseConfigError ("postgres://afs:xxxxx@Part2:xxxxxx@host").

When the userinfo carries a password and the URI is not structurally
unambiguous, mask up to the '@' that most plausibly ends the userinfo:
the last '@' before the last '?', or the last '@' when the '?' belongs to
the password. uriStructureUnambiguous now also rejects a host containing
'@', mirroring the non-numeric port rule.

Refs jackc#1271.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jackc

jackc commented Oct 21, 2026

Copy link
Copy Markdown
Owner

As mentioned in the original issue:

redacting the error while preserving as much of the context as possible is a best-effort / impossible to be perfect task.

This PR solves one case, but breaks others (at least according to Astra)

I'm not sure it is worth trying to figure out all the ways it could fail and solve them. If we could do that we could have parsed the connection string successfully in the first place.


Astra's analysis:

The fallback can swallow the query delimiter (errors.go:405). For postgres://user:pw%zz@host/db?application_name=me@example.com&password=SECRET, it selects the query value’s @ as the userinfo terminator. The resulting error contains postgres://user:xxxxx@example.com&password=SECRET, exposing the entire query password. Preserve query-password masking when extending the userinfo mask.

Decoded hosts can legitimately contain @ (errors.go:378). Unix socket directories such as /tmp/pg@sock trigger the new ambiguity check. For postgres://host:5432/db?host=%2Ftmp%2Fpg%40sock&application_name=me@example.com&password=SECRET&connect_timeout=bogus, this activates the existing greedy heuristic and exposes password=SECRET. Check raw authority syntax rather than the decoded, query-overridden host.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants