Repository navigation
fix: percent-encode userinfo in database URL construction - #363
Conversation
Watson-Branch: #362
`build_postgres_candidates` emitted `postgres://user@/db?host=/path` for the unix-socket candidate. The WHATWG URL rules that sqlx's `Url::parse` implements reject an empty host following a userinfo component, so `PgConnectOptions::from_str` returned `Configuration(EmptyHost)` and the candidate never connected — for every username, credential-independent. Splice the same inert `localhost` placeholder the MySQL socket branch already uses. sqlx reads `host=` after the authority and stores it as the socket, so the parsed options carry socket `/tmp/.s.PGSQL.5432` with host `localhost`. The existing coverage matched `?host=…` in the URL *string*, which an unparseable URL satisfies just as well; the new test hands the candidate to the real parser. Verified by mutation: reverting the authority reddens the new test and leaves the old one green. Refs #362
`userinfo` spliced the `.env` username and password into a `driver://…` URL raw. sqlx routes every connection string through `Url::parse`, which ends the authority at the first `/`, `?` or `#` regardless of intent, so a credential holding one escaped its own slot two ways: - `p/ss` produced `mysql://user:p/ss@host:3306/db`, whose authority is `user:p` — the connection failed with *invalid port number*, no useful diagnostic. - Where the run before the delimiter was all digits and fit a `u16`, the authority parsed instead. Every shape from the issue resolved to host `sail`, the digits as the port, the default `root` user, and no password at all: the connection silently went elsewhere, and a redactor that locates the credential span by parsing the URL is told there is nothing to mask. Base64 passwords mix digits and `/` routinely, so this is an ordinary shape. `encode_userinfo_component` passes through exactly RFC 3986's `unreserved` and `sub-delims` sets and percent-encodes every other byte, so credentials made of legal characters produce byte-identical URLs to before. `:` is encoded in the username too, where the grammar permits it, because `userinfo` overloads `:` as its own separator. `%` encodes to `%25`, so `sec%3Dret` round-trips literally rather than decoding to `sec=ret`. All four call sites — both MySQL and both Postgres branches — route through the one function; `DB_URL` is passed through verbatim and never re-encoded. Every new test settles its claim through `MySqlConnectOptions` / `PgConnectOptions::from_str` rather than string comparison. Neither type exposes a password accessor, so the password is pinned differentially against `to_url_lossy()` of the same options with the expected password set. Mutation-verified: reverting the encoding, encoding only the password, admitting `%`, `:` or `/` into the pass-through set, over-encoding a legal sub-delim, and dropping the empty-password shape each redden at least one test, as does substituting a wrong expected password in the helpers. Fixes #362
There was a problem hiding this comment.
✅ Approved
Review Summary
userinfo now routes both components through encode_userinfo_component, closing the class where a .env credential holding a delimiter escaped its own slot. All seven acceptance criteria are met. CI green on all seven checks; 111 database::tests pass, including the 14 new ones.
The encode set is exactly right. database.rs:479-490 passes through precisely RFC 3986 unreserved + sub-delims and nothing else. : is correctly excluded from pass-through in both components — the AC's reasoning holds, since userinfo overloads : as its own separator, so an unencoded one in a username mis-splits exactly as an unencoded / mis-splits the authority. Iteration is over raw.bytes(), so non-ASCII UTF-8 encodes per byte, and byte as char is safe because the pass-through arm only ever matches single ASCII bytes.
The invariant class is swept, not spot-fixed. This is the part I checked hardest, because it is this repo's most-repeated defect — the guard landing at one call site and not the sibling beside it (#294 path_within_root, #348 rounds 1 and 2 on masking, #353 escalated at round 4). I enumerated it independently rather than trusting the summary: six ConnCandidate construction sites, four route through userinfo (:2271, :2290, :2333, :2346), two are the AC-excluded DB_URL passthroughs. The only other credential use in the tree is AuthMethod::sql_server (:2582), which takes fields structurally and never parses a URL; sqlite: carries no credentials. There is no fifth splice. The class is closed.
The tests are honest. The differential password assertion deserved scrutiny — if to_url_lossy() redacted the password, both sides would compare equal for any value and twenty tests would prove nothing. It doesn't: build_url() does utf8_percent_encode(self.password, NON_ALPHANUMERIC) and embeds the real password, and that encoding is injective, so two distinct passwords cannot collide. Neither options type exposes a password getter, so the technique is necessary rather than gratuitous. Every parse test checks real fields; nothing asserts bare is_ok(); the call-site tests build real ConnCandidates through the production functions instead of re-implementing URL assembly. The mutation table in the PR description is corroborated by the tests actually present.
Noted divergence — adjudicated, not waived
Commit 99ce9ac splices a localhost placeholder into the Postgres unix-socket authority, which no AC bullet names literally. This is AC-mandated, not scope creep. Criterion 7(d) requires a test per production call site — both branches of build_postgres_candidates — confirming the credential and the trailing host= value parse back correctly. That branch emitted postgres://user@/db?host=…, which WHATWG rules reject as EmptyHost for every username, credential-independent. The criterion was unsatisfiable without the fix. Shipping it as a separate reviewable commit was the right call, and host= is read after the authority so the placeholder is inert.
📋 Non-blocking follow-ups
- The
DB_URLpassthrough (config.url) still reachesmask_url_credentialsunencoded, so a user-typed raw@in that value can leave its password tail visible atdatabase.rs:1191— Noted, not tracked. Three reasons it stays a note: the AC explicitly requiresconfig.urlbe passed through verbatim, so it is excluded by contract; the behavior is documented as a deliberate best-effort tradeoff atcompletion_display.rs:105-109; and it is already tracked by #355 with PR #358 in flight against it. Opening an anchor here would duplicate active work, and #355 isIn Progress— expanding it now would move the goalposts mid-build. config.databaseand the socket path are still spliced raw. Correctly flagged rather than folded in: neither is a credential, and encoding them would move URLs for every existing user.
Ready for @mikebronner to merge.
Summary
Implements #362.
database::userinfospliced the.envusername and password into adriver://…connection URL raw. sqlx parses every connection string withUrl::parse, which ends the authority at the first/,?or#whether or not that character was meant as a delimiter, so a credential holding one escaped its own slot.Both failure modes were reproduced against sqlx 0.9 before the fix:
p/ssErr(Configuration(InvalidPort))— connection fails, no useful diagnostic12/34Ok(host "sail", port 12, user "root", no password)1234/56,012345/aG9zdG5hbWU=,12?34,1234?56,12#34,1234#56The second row is the dangerous one: nothing errors. The connection goes to a host and port assembled out of the password, the username falls back to
root, and any redactor that locates the credential span by parsing the URL is told there is no password to mask.openssl rand -base64output mixes digits and/routinely, so this is an ordinary password shape.Changes
encode_userinfo_component(new,database.rs) — percent-encodes one userinfo component. Passes through exactly RFC 3986unreserved(ALPHA DIGIT - . _ ~) andsub-delims(! $ & ' ( ) * + , ; =); every other byte becomes%XX, including:,/,?,#,[,],@,%, space, control bytes, and each byte of a non-ASCII UTF-8 sequence. No new dependency: the crate set needed is not anypercent-encodingpreset (NON_ALPHANUMERICwould encode the sub-delims and move URLs that work today), so a custom set was required either way.userinfonow routes both components through it. The username is encoded symmetrically with the password,:included —userinfooverloads:as its own separator, so an unencoded one there mis-splits exactly as an unencoded/mis-splits the authority..env, and%self-encodes.99ce9ac) — see below.All four call sites that build a URL through
userinfo—build_mysql_candidates' socket and TCP branches,build_postgres_candidates' socket and TCP branches — route through the one function; there is no fifth credential splice in the tree (sqlite:carries no credentials, and SQL Server goes throughtiberius::AuthMethod::sql_server, which takes the fields structurally).DB_URLis passed through verbatim and never re-encoded.The extra commit: the Postgres socket candidate never connected
Writing the AC's per-call-site test surfaced a defect independent of this one.
build_postgres_candidatesemittedpostgres://user@/db?host=/path, and the WHATWG URL rules sqlx'sUrl::parseimplements reject an empty host that follows a userinfo component:That candidate could never connect, for any username, credential-independent. The existing coverage matched
?host=in the URL string, which an unparseable URL satisfies just as well. The AC requires this branch to parse back correctly, so the fix ships here: splice the same inertlocalhostplaceholder the MySQL socket branch already uses. It is committed separately (99ce9ac) so it can be read on its own.Deliberately out of scope:
config.databaseand the socket path are still spliced raw. Neither is a credential, both are a different class from the one this issue names, and encoding them would move URLs for every user. Flagging rather than folding in.Acceptance Criteria
database::userinfo()percent-encodes every byte outsideunreserved/sub-delims, identically for username and password,:in the username included./,?,#or@now parses viaMySqlConnectOptions/PgConnectOptions::from_str, yielding the intended username and password — no parse error, no misread host/port.userinfo_leaves_legal_characters_byte_for_bytepins both the component and the assembled URL). The empty-password no-trailing-colon behavior is preserved.%always encodes to%25;sec%3Dretround-trips to the literalsec%3Dret, neversec=ret.DB_URLpassthrough is untouched, both drivers (db_url_passthrough_is_never_re_encoded)./,?,#,@in a password; (b) all three issue digit-run shapes plus the?and#equivalents; (c) reserved characters in the username; (d) one test per production call site building a realConnCandidate, asserting the credential and the trailingsocket=/host=value both parse back.Test Plan
cargo test— 3,491 pass, 0 fail.cargo clippy --all-targets— no warnings.cargo fmt --check— clean.to_url_lossy()of the parsed options againstto_url_lossy()of the same options (a clone, so every other field is identical) with the expected password set. Equality holds exactly when the parsed password is the expected one.userinfoto the raw splice%through the pass-through setuserinfo_encodes_percent_so_hex_pairs_do_not_decode:through/through+userinfo_leaves_legal_characters_byte_for_bytepostgres_socket_candidate_round_trips_credentials_and_socketFixes #362