Redact MongoDB URI credentials before logging them

[ademidoff]

Fixes #1345.

MongoDB connection URIs carrying credentials were written to the logs verbatim. buildURI (main.go:300) injects the monitoring user's password into the URI, and a URI supplied via --mongodb.uri/MONGODB_URI may already embed one, so several log statements disclosed user:password in cleartext.

What changed

A new internal/redact package holds the two helpers. The leaking call sites straddle package main and package exporter, so neither could host a shared helper; internal/util is MongoDB-command code and pulls in the driver, which made it a poor home for a pure string function.

• redact.MongoURI(uri) — (*url.URL).Redacted() on the happy path, keeping the username and substituting xxxxx.
• redact.Error(err) — scrubs credentials out of an error's message (see below).

Six call sites now redact:

Location	Level
main.go:143 — "Connection URI"	Debug
main.go:259 — --split-cluster parse failure	Fatal
exporter/seedlist.go:31 — URI parse failure	Fatal
exporter/seedlist.go:36 — SRV lookup failure	Error
exporter/seedlist.go:41 — no SRV records	Error
exporter/server.go:181 — buildServerMap parse failure	Error

Only the first is Debug-level; the other five fire at the default log level, on error paths reachable through ordinary operational failures such as a DNS hiccup or a malformed URI.

The error argument leaked too

Three of those sites log because url.Parse failed. url.Parse returns a *url.Error, whose Error() quotes the offending URL verbatim — so the err argument alone disclosed the full password, and redacting only the URI argument would have fixed nothing there. Hence redact.Error.

This is not hypothetical. The URI PMM generates with an escaped socket path is exactly what makes url.Parse fail:

before: address=mongodb://monitor:SuperSecret123@%2F... error="parse \"mongodb://monitor:SuperSecret123@%2F...\": invalid URL escape \"%2F\""
after:  address=mongodb://monitor:xxxxx@%2F...         error="parse \"mongodb://monitor:xxxxx@%2F...\": invalid URL escape \"%2F\""

Where parsing failed there is no *url.URL to redact, so MongoURI falls back to a regexp that rewrites the userinfo of the raw string. The URI is kept in the message rather than dropped, because with multiple targets configured it is the only way to tell which one is malformed.

Tests

internal/redact/redact_test.go is table-driven over credentialed, uncredentialed, percent-encoded, SRV, multi-host and unparseable inputs. The assertion that matters is that the password substring does not survive. TestErrorHidesURIQuotedByParseError first asserts that the raw url.Parse error really does contain the password, then that the redacted form does not — so the test would catch a future Go release that changed this behaviour.

Also verified manually against the built binary for three scenarios (credentials via --mongodb.user/--mongodb.password, credentials embedded in --mongodb.uri, and the socket URI), grepping the logs for the password in each.

Not changed

buildURI and buildURIManually are untouched — injecting the credentials is intended; only logging them was wrong.

Two spots were examined and left alone: exporter/pbm_collector.go:88 passes the URI to the third-party PBM SDK and logs only err, and exporter/exporter.go:426 wraps a driver validation error whose messages quote individual option keys rather than the whole URI. Both carry residual third-party risk outside this repo's control.

Pre-existing lint findings in the touched files are left in place to keep the diff surgical. golangci-lint run --new-from-rev=origin/main reports 0 issues, which matches what CI's reviewdog gate (filter_mode: added) evaluates.

Conflict with #1336

PR #1336 adds an unexported redactMongoURI to package main and redacts main.go:143 as incidental cleanup. Whichever lands second needs a small rebase there — drop the local helper in favour of internal/redact, or re-apply that one line. No other overlap.

[Copilot]

Pull request overview

Adds shared MongoDB URI credential redaction and applies it to logging paths.

Changes:

• Introduces URI and error-message redaction helpers.
• Redacts six affected log sites.
• Adds comprehensive redaction tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
main.go	Redacts connection and parse-failure logs.
internal/redact/redact.go	Implements shared redaction helpers.
internal/redact/redact_test.go	Tests URI and error redaction cases.
exporter/server.go	Redacts invalid server addresses.
exporter/seedlist.go	Redacts SRV-related URI logs.

Suppressed comments (1)

internal/redact/redact.go:58

• Error does not fully scrub every *url.Error: parsing splits at ?/# before constructing the error, so the regex never sees the trailing @. The added TestErrorLeavesTruncatedURIAlone confirms that a password prefix such as pa is still emitted by the three default-level parse-failure logs. This contradicts the helper's safety contract and the issue requirement; for *url.Error, log only the underlying parse diagnostic (or pass the original URI into the redactor) and update the known-gap test.

	return credentialsRE.ReplaceAllString(err.Error(), replacement)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[JiriCtvrtka]

Looks good.

Two places in exporter/pbm_collector.go I think are worth another look. Both are outside this diff, so I couldn't leave inline comments.

exporter/pbm_collector.go:90 — logger.Warn("failed to create PBM client", "error", err.Error())
The Not changed section excludes this one because it "logs only err". But that's the case redact.Error was added for — per The error argument leaked too, redacting only the URI argument "would have fixed nothing there". Same shape here: the URI goes into sdk.NewClient, and pbm/connect.Connect wraps a *url.Error that renders it back out.

exporter/pbm_collector.go:139 — l.Error("failed to get cluster status", "error", err.Error())
This one isn't mentioned. cli.ClusterStatus(ctx, pbmClient, cli.RSConfGetter(p.mongoURI)) hands the URI to PBM, and at the pinned version (percona-backup-mongodb@v1.8.1-0.20251124214042-d06cab743541, sdk/cli/status.go:168-180):

curi, err := url.Parse(string(g))
if err != nil {
    return nil, errors.Wrapf(err, "parse mongo-uri '%s'", g)
}

g is the full URI. It fails on exactly the escaped-socket-path URI this PR uses as its motivating example, so the password appears twice in the line — once inside the *url.Error, once in PBM's own '%s'. Unlike :90, this fires on every scrape at Error level whenever --collector.pbm is on.

On "residual third-party risk outside this repo's control" — that's true of who builds the message, but redact.Error(err) scrubs it at the log site regardless of origin, which is what makes it work for the url.Parse cases already covered.

Both look like one-line changes:

logger.Warn("failed to create PBM client", "error", redact.Error(err))
l.Error("failed to get cluster status", "error", redact.Error(err))

Could be there's a reason these differ from the six you converted — happy to be corrected.

[ademidoff]

Thanks — you were right about pbm_collector.go, and the "logs only err" line in my Not changed section was a bad argument. Whether the message is safe depends on what the callee puts in it, not on which argument we pass. Both sites are converted in b8baabd. The two differ in what I could actually reproduce, so recording that here.

pbm_collector.go:139 — confirmed, exactly as you described

Calling cli.RSConfGetter(uri).Get(...) at the pinned version with the escaped-socket URI:

parse mongo-uri 'mongodb://monitor:SuperSecret123@%2Ftmp%2Fmongodb.sock/admin': parse "mongodb://monitor:SuperSecret123@%2Ftmp%2Fmongodb.sock/admin": invalid URL escape "%2F"

The password twice — once in PBM's own '%s', once in the wrapped *url.Error — at Error level, on every scrape with --collector.pbm. That is the worst of the sites this PR has touched, and it was not in the issue.

pbm_collector.go:90 — I could not reproduce a leak

Six malformed URIs through sdk.NewClient, all with credentials, none disclosed them:

URI defect	error
escaped socket path	ping: server selection error: ... Addr: /tmp/mongodb.sock ...
?ssl=notabool	invalid connection string option: error parsing uri: invalid value for "ssl": "notabool"
host:notaport	... port must be an integer: strconv.Atoi: parsing "notaport"
no host	... must have at least 1 host
raw space / # in password	ping: server selection error: ... dial tcp: lookup host: no such host
mongodb+srv://	... unescaped colon in password

That matches the source: Connect reaches errors.Wrap(err, "parse mongo-uri") at pbm/connect/connect.go:245 with no %s, and everything before it comes from the driver's connstring parser, which names the offending option rather than echoing the URI.

Converted it anyway. Not because I found a leak, but because "we handed this function a credential-bearing URI, so we redact what it hands back" is an invariant that survives a dependency bump, whereas "I read PBM 1.8.1's error strings and they looked fine" has to be re-audited every time. It is one line.

What redact.Error does and does not fix there

It removes the full password from both copies:

red = parse mongo-uri 'mongodb://monitor:xxxxx@%2Ftmp%2Fmongodb.sock/admin": invalid URL escape "%2F"

It does not close the truncation gap in the wrapped *url.Error. For a password containing ? or #, url.Parse cuts the URI at that byte, so the inner quote reads parse "mongodb://monitor:pa" with no @ left to anchor on and the prefix pa survives. Nothing in the pattern can tell monitor:pa from host:27017.

So the two groups are handled differently, deliberately:

• Where we build the message (the three url.Parse sites in main.go, exporter/seedlist.go, exporter/server.go) the parse error is now dropped entirely and only the redacted URI is logged. That closes the gap rather than narrowing it, and costs little — the redacted URI already says which target failed. This also resolves the Copilot comment on this PR, which flagged the same thing.
• Where PBM builds it, dropping the error would throw away the only diagnostic, so redact.Error stays and the prefix residue remains a known limit. TestErrorLeavesTruncatedURIAlone pins it, and TestErrorRedactsURIWrappedByThirdParty covers PBM's double-quoted shape.

That is also what keeps redact.Error in the tree: dropping the error at the three parse sites left it with no callers, and these two sites are now its reason to exist.

golangci-lint run --new-from-rev=origin/main is clean; internal/redact, exporter/dsn_fix and root tests pass. make check and go test ./... still fail the same way they do on main — 347 pre-existing lint findings, and the ./exporter / internal/tu suites needing make test-cluster.