fix: mask the whole credential when a password contains an unencoded @

[mikebronner]

Summary

Implements #355.

mask_url_credentials decided where a URL's credentials end by scanning for an @. Two strings defeat every scanning rule, because they are the same shape:

mysql://host:3306/db@x	host, port, and a path holding an @
postgres://user:p@ssword@host/db	credentials whose password holds an @

Take the first @ and the first is mangled to mysql://host:***@x — host, port and database gone — while the second leaks its tail as postgres://user:***@ssword@host/db, which is the defect #355 reported. Bound the authority at the first /, ? or # and take the last @ inside it — this PR's previous head — and both are right, but a password containing a raw /, ? or # closes the window before its @ and comes back unmasked. / is 1 of the 64 base64 symbols; @ is in none. That trade swapped a rare leak for a common one.

Holmes caught it, @mikebronner asked whether anything better existed, and Holmes's re-review answered: parse the URL instead of scanning it.

The change

url — already compiled into this binary via sqlx, now a direct dependency, no Cargo.lock change — resolves the authority to spec, so the two shapes above stop being one shape:

• parse succeeds, reports a password — the userinfo is real. The credentials end at the last @ in the parsed authority. (A raw /, ? or # inside userinfo would have ended the authority in the parser too, so that window always holds the @.)
• parse reports no password, and the value has an authority — the only shape returned untouched. That is the rule, not a list of the shapes it admits: Url::password gates on Url::has_authority, so an authority is exactly the condition under which the parser read a userinfo component, and only then is its silence evidence. (mysql://host:3306/db@x is host, port and a path @; https://example.com/webhook has no userinfo; mysql://user:@host/db has a : with nothing after it, which url reports identically to no password.)
• everything else — the parse fails, or it succeeds with no authority. Neither ever looked at userinfo, so the scan is all that is left.

The fallback is load-bearing, and greedy is the wrong rule for it

database::build_postgres_candidates builds the libpq socket URL postgres://user:pass@/db?host=/var/run/postgresql with a deliberately empty host. url rejects it (empty host), and database::userinfo splices the .env password in raw — so this reaches the fallback with a live credential, and it is logged at database.rs:1471 on every socket-configured Postgres introspection.

A plain first-@ fallback masks p and prints ss@ from p@ss into that log line — reintroducing the exact defect this PR exists to close. The fallback therefore prefers the authority's last @, and only then falls back to the first @ anywhere. Both halves are pinned:

Input	Reaches fallback because	Masked to
postgres://user:p@ss@/laravel?host=/var/run/postgresql	empty host	postgres://user:***@/laravel?host=…
postgres://user:p/ss@host/db	invalid port	postgres://user:***@host/db
mysql://user:pa?ss@host/db	invalid port	mysql://user:***@host/db
mysql://user:pa#ss@host/db	invalid port	mysql://user:***@host/db

a_successful_parse_is_the_only_thing_keeping_the_fallback_off_these_shapes pins the premise separately: mysql://host:3306/db@x and friends parse cleanly and report no password, so they are returned before the fallback is consulted. Asserted directly rather than inferred from the borrowed-and-unchanged fixture, which would stay green if the value reached the fallback and survived for some unrelated reason.

That protection is conditional on the parse succeeding, and the same test now pins the other side of it. A credential-free value whose port the parser refuses does reach the fallback, and the unbounded find('@') masks the path's @:

Input	Parse	Out
mysql://host:70000/db@extra	invalid port number	mysql://host:***@extra
mysql://host:port/db@extra	invalid port number	mysql://host:***@extra

Over-masking, not a leak — the safe direction, and the price of the or_else that masks postgres://user:p/ss@host/db. The behaviour stays; the claim that these shapes can never get there does not.

One shape still fails open — narrower, and pinned

A /, ? or # password whose leading run also parses as a valid port: mysql://user:12/34@host/db, or mysql://user:/ss@host/db (an empty port is valid too). The parser reads host user, port 12, path /34@host/db — byte for byte the reading mysql://host:3306/db@x gets, and the correct one per RFC 3986, which requires all four characters percent-encoded in userinfo.

No parse separates them, so this is irreducible here. a_password_whose_leading_run_parses_as_a_port_still_fails_open pins both shapes and the boundary: mysql://user:99999/ss@host/db exceeds the port range, so the parse fails and the fallback masks it. That boundary assertion is what keeps the residue from silently widening.

Every other /, ? or # password is rejected by the parse and masked by the fallback, so the residue is strictly smaller than the previous head's (which failed open on all of them) and does not exist on main — main masks these but mangles mysql://host:3306/db@x.

Round 3 — the comments, not the code

Holmes approved the design and verified all fourteen AC by execution, then bounced four comments this branch added that asserted properties the code does not have. On a fail-open redaction gate the documentation of which shapes fail open is the safety argument, so these are blockers rather than nitpicks. e7c7c43 fixes them. No production behaviour changed — every non-comment edit in that commit is a test fixture.

#	The claim	What execution showed	Fix
1	The parses-with-password test's doc: "every value here parses as a URL and reports a password"	postgres://user:p@ss@/laravel?host=… is ERR(empty host) and drove the fallback arm, duplicating the fallback test's own coverage	Fixture moved out; the premise is now asserted per fixture, matching the is_err() guard its sibling already carries
2	the_shapes_the_fallback_would_mangle_never_reach_it	mysql://host:70000/db@extra fails on the port and does reach it	Renamed, narrowed, and both halves pinned (table above)
3	"all five surfaces" in env_value_redaction.rs	Four. #356 deleted the fifth, which this same file's module doc already narrates	Count replaced with the enumeration, so the next deletion cannot stale it silently
4	The Ok(_) arm described as "what looks like credentials is host:port"	It also swallows mysql://user:@host/db, which url reports identically to no password	Named in the comment, pinned by a fixture

The residue paragraph now names its baseline, which the previous wording left ambiguous and which I had wrong: the residue is narrower than the authority-bounded scan this arm replaces, which failed open on every /, ? or # password — not narrower than main's greedy scan, which masked mysql://user:12/34@host/db correctly and paid for it by mangling mysql://host:3306/db@x. Net gain over main, not a strict subset of it.

Sweeping the class found two more

Fixing four reported instances is not evidence the class was swept, so I audited every prose claim this branch added against execution rather than only the four named:

• The borrowed-value test's doc said a value the parser rejects "gets masked". One carrying no @ for the scan to find — mysql://host:70000/db — comes back borrowed like everything else in that test. Narrowed, and the shape is now a fixture.
• The residue test pinned only / of the three characters its own doc names. mysql://user:12?34@host/db and mysql://user:12#34@host/db behave identically and are now fixtures too.

One claim survived the sweep intact, and it is the load-bearing one: a successful parse reporting a password always leaves an @ inside the hand-rolled authority window. Were it false the function would fail open silently, so I brute-forced it rather than arguing it — 55,566 generated inputs across 9 schemes × 9 users × 14 passwords × 7 hosts × 7 tails, zero violations.

Round 4 — a third member of that arm, holding a live password

Holmes verified round 3's four fixes, then found that the sweep had gone over the prose and not over the Ok(_) arm's own input space. It has a third member and it leaked:

mask_url_credentials("jdbc:mysql://user:secret@host:3306/db")
  round-3 head  ->  jdbc:mysql://user:secret@host:3306/db     ← borrowed, in the clear
  origin/main   ->  jdbc:mysql://user:***@host:3306/db        ← masked

jdbc is a valid scheme and the bytes after its colon are not //, so url takes its opaque-path branch and never parses userinfo. Url::password gates on has_authority(), so it answers None unconditionally — whatever that path holds. find("://") meanwhile finds the inner mysql://, so the hand-rolled window was perfectly well-formed and simply never consulted.

Reachability is total: JDBC_URL, SPRING_DATASOURCE_URL and DB_URL split to no SENSITIVE_ENV_SEGMENTS keyword, so the name gate never fires and this function is the only gate. A regression against main, not merely a gap.

The fix is the rule, because the member list leaks

Holmes proposed routing cannot_be_a_base() to the fallback and invited a better shape. Execution says that predicate is itself a member list. foo:/bar://user:secret@host/db reports cannot_be_a_base() == false and has_authority() == false — a :// that is only ever path. It leaks under the proposed guard and masks under has_authority().

So the guard is has_authority(), which is the predicate Url::password itself gates on. Same rule, one source.

Sweeping the arm, not the instance

Round 3 brute-forced one arm and shipped a leak in its sibling, so this sweep is aimed at the arm Holmes named — "parse succeeded with no password ⇒ no credential anywhere in the value" — over 60,480 credential-bearing inputs (5 prefixes × 7 schemes × 4 users × 12 passwords × 6 hosts × 6 tails), each carrying a marker asserted absent from the output:

Guard	Leaks	Outside the documented residue
round-3 head (Ok(_))	49,032	48,384
cannot_be_a_base() (as proposed)	12,744	12,096
has_authority() (shipped)	648	0

The 648 survivors are the digit-leading-run residue tracked under #362, and only with an empty prefix — behind an opaque scheme the same password now masks, so the residue is narrower than it was.

The price, pinned rather than discovered later

A credential-free value in that same family reaches the scan, which cannot tell a path's @ from a separator. A second sweep over 1,470 credential-free inputs isolates the cost at 420, all over-masking:

Input	Round-3 head	Now
jdbc:mysql://host:3306/db@x	untouched	jdbc:mysql://host:***@x
jdbc:mysql://[::1]/db@x	untouched	jdbc:mysql://[:***@x

Not a new trade: mysql://[::1]:70000/db@x already mangled to jdbc-identical output on the rejected-parse arm, and that fixture is in the test as the proof. A mangled display beats a printed password. mysql://host:3306/db@x still comes back borrowed — the discriminating half of the same test.

The other two findings

#	Finding	Fix
2 🔴	The Ok(_) doc bullet asserted "neither shape holds a secret" — the safety argument, and false	Rewritten as the arm's membership rule (has_authority), with the shapes demoted to examples. A rule cannot go stale by omission; the list had done so three rounds running
3 🟠	"one digit more than a port can hold" for 99999 — same five digits as 65535; the parse fails on value	Corrected, then pinned: both lengths and both parse::<u16>() outcomes are now assertions, so the sentence cannot go false silently

Mutation-verified

Reverting the guard to round 3's Ok(_) reddens 7 tests across all four masked surfaces — the unit arm test, the over-masking test, the residue test, the server-log test, and three cross-surface fixtures (env completion, .env interpolation, hover). Restored, full suite green: 2,711 + 663 + 80 + 2 passing, cargo fmt --check clean, cargo clippy --all-targets silent.

The cross-surface fixture is JDBC_URL, kept as its own variable rather than folded into DATABASE_URL: the two travel different arms, and one fixture cannot exercise both. Its secret is asserted absent whole and by tail, so a partial mask fails too.

Changes

• completion_display::mask_url_credentials dispatches on Url::parse instead of scanning; the scan survives only as the parse-failure fallback, authority-last-@ first.
• url = "2.5" added as a direct dependency of laravel-lsp. Already in Cargo.lock at 2.5.8 via sqlx — the lockfile is unchanged by this PR.
• The doc comment states the arm-membership rule (an authority, not a list of shapes), names both members of the fallback arm — a rejected parse and an authority-less one — and documents the one surviving fail-open shape rather than claiming there is none.
• Fixtures for the fallback arm, for the premise it rests on, for the residual shape and its boundary, and the two AC-mandated shapes that were missing (postgres://user:p@ssword@host/path@literal, https://host/path@literal) plus the end-of-string, port-with-credentials and db@extra cases.
• Merged main (The warm-start env-var cache has no display consumer — wire it up or delete it #359 deleted the warm-start disk cache) and swept the surface counts left stale by it: three client-rendered consumers, four masked surfaces counting the server log. Previously five.

Acceptance Criteria

• The credential-ending @ is no longer the first @ anywhere. It is resolved by an RFC 3986 parse, with the AC's authority-bounded last-@ rule as the fallback for values no parser accepts.
• postgres://user:p@ssw0rd@host/db → postgres://user:***@host/db (AC bullet 11 fixes this literal as p@ssw0rd; left as-is).
• postgres://user:p@ssword@host/path@literal → postgres://user:***@host/path@literal — credential masked, path @ untouched, no bail-out.
• postgres://user:p@ssword@host → postgres://user:***@host — the end-of-string branch.
• mysql://user:pass@host:3306/db → mysql://user:***@host:3306/db — port colon not mistaken for the credentials'.
• mysql://host:3306/db and mysql://host:3306/db@extra both return Cow::Borrowed, untouched.
• https://host/path@literal returns Cow::Borrowed(value), asserted with matches!(…, Cow::Borrowed(v) if v == value).
• Fail-open paths preserved: no ://, no @ to be found, no : in the credentials all return Cow::Borrowed. Deviation, sanctioned by the escalation: bullet 8's enumeration also listed "no @ before the first /" as fail-open. That clause is what made the bullet self-contradictory and what Holmes escalated; those values are now masked by the fallback rather than leaked.
• Non-regression: mysql://user:pass@host/db → mysql://user:***@host/db, redis://:pass@host:6379 → redis://:***@host:6379.
• The doc comment no longer claims an @ inside the password survives masking; it states the rule the code implements, arm by arm — and after round 3, states each arm's full extent rather than its most common case.
• a_credential_inside_the_value_is_masked_whatever_the_name_says and the p@ssw0rd fixture expect the fully-masked form.
• completion_display/tests.rs carries the new fixtures.
• tests/env_value_redaction.rs drives an unencoded-@ DATABASE_URL across the consumers, asserting the plaintext is absent from the serialized response and the masked form present, with the surviving tail pinned separately.
• cargo clippy --all-targets -- -D warnings and cargo test --all-features both clean.

Test Plan

• cargo test --all-features — 3406 pass, 0 fail.

• cargo clippy --all-targets -- -D warnings — clean.

• cargo fmt --check — clean.

• url's behaviour on all 40+ shapes in this description was measured against 2.5.8, not assumed — including the finding that it rejects the libpq socket URL, which is what set the fallback's rule.

• Mutation-verified, each reverted independently and re-measured after the main merge:

  Mutation	Red
  rfind → find in the authority scan	9 across both targets
  fallback loses its authority preference (greedy only)	2
  no-password values fall through to the fallback	2
  the whole Url::parse dispatch removed (the previous head)	2

  The last row is the one that matters: it is the leak Holmes bounced this PR for, and it now has a test.

• Round 3's new assertions mutation-verified the same way, each reverted independently:

  Mutation	What reddens
  libpq fixture restored to the parses-with-password test	the new per-fixture premise assert, by name
  fallback loses its or_else(find('@'))	the new over-masking fixtures, plus 2 pre-existing
  Ok/no-password arm falls through instead of returning borrowed	the new mysql://user:@host/db fixture
  fail closed on a parse rejection (the escalation's option 3)	the new mysql://host:70000/db fixture

  Stated honestly: the ?/# residue fixtures and the multibyte one are pins on documented behaviour, not mutation discriminators. They live in a test whose stated job is to go red when the doc comment goes stale, which is the alarm they arm.

Noted, not fixed

• database::userinfo (database.rs:466-472) interpolates the .env password into a connection URL with no percent-encoding, which is what makes the server manufacture the malformed shapes above. Holmes flagged it wont-fix-here in review; it is a connection-string defect, not a display one.
• Multibyte input had no fixture. Holmes verified by execution that it is safe and marked it noted — not tracked; postgres://usér:p@sswörd@host/db is now a fixture anyway, since I was in the file.
• The digit-leading-run fail-open class is irreducible inside this function and tracked by Holmes under latent-hazard. Not built here.
• database.rs:481 still lists "warm-start-cache redaction" in the present tense. The warm-start env-var cache has no display consumer — wire it up or delete it #356 deleted that cache, so the claim went stale on main, not in this PR. Left out to keep this diff to the redaction parse.

Fixes #355

[mr-sherlock-holmes]

@mikebronner 🛑 Escalating — this PR closes the @ leak and opens a / leak, and the AC as written blesses the trade. That's your call, not mine.

What I found

The fix is clean, correct, and well-documented for the case it targets. Every issue-named shape masks properly, and I verified the byte-offset arithmetic is char-boundary-safe on every path — no panic reachable.

But the new authority parse ends at the first /, ? or #, then looks for the last @ inside that window. When the password itself contains one of those three characters, the window closes early, no @ is found, and the function fails open and returns the credential in clear.

A sub-agent compiled origin/main and the PR head side by side and ran both:

input:  postgres://user:p/ss@host/db
  old -> "postgres://user:***@host/db"    <- masked
  new -> "postgres://user:p/ss@host/db"   <- LEAKED verbatim

? and # behave identically. This is a behaviour regression, not a pre-existing gap.

The frequency argument in the doc comment is backwards. / and + are 2 of the 64 base64 symbols, so a openssl rand -base64 24 password has a ~40% chance of containing /. @ is not in the base64 alphabet at all — it only appears in hand-typed passwords. So the rarer leak got closed and the commoner one opened.

Blast radius — all five consumers trust the returned Cow with no secondary check: the hover popup (main.rs:21114), both completion surfaces (main.rs:27774, 15688), the server log Zed shows in a visible panel (database.rs:1263, 1434), and — worst — the persistent disk cache at ~/Library/Caches/org.mike-bronner.laravel-ce-lsp/…/cache.json (main.rs:8026 → save()), a long-lived file outside the project whose own CACHE_VERSION v6 comment says its entire purpose is to never hold a DATABASE_URL secret in the clear.

Why this is yours and not Watson's: AC bullet 8 says "All existing fail-open paths are unchanged" — but its own enumeration, "no @ before the first /", defines a strictly larger fail-open set than the old code had. The heading promises no change; the definition mandates one. Watson implemented the definition exactly, documented the tradeoff honestly at completion_display.rs:113-119, and pinned it with a test at completion_display/tests.rs:390-398. He did the right thing with a contract that contradicts itself. Bouncing this to him would be asking him to violate the AC.

Options

1. Ship as-is — accept the trade, fix only the two missing fixtures.
Pros: PR is otherwise done; RFC 3986 genuinely requires / percent-encoded in userinfo, so the input is malformed; tradeoff is documented and pinned rather than hidden.
Cons: net-negative on real-world leak probability (~40% of generated secrets vs. a rare typed @); plaintext lands in a persistent disk cache that explicitly promises otherwise; #344's premise — "no credential reaches a display surface in the clear" — is weaker after this PR than before it.

2. Disambiguate with a port check, then fix the fixtures.
When the authority contains no @, retry with the last @ in the whole value, and mask only if the run between the credentials' : and that @ is not all-digits. 3306/db is all-digits-then-path → port, stays borrowed. p/ss → password, gets masked. Closes both leak classes.
Pros: the only option where neither @ nor / leaks; keeps mysql://host:3306/db@x correctly untouched; ~5 lines.
Cons: a heuristic, not a spec parse; needs its own fixtures; small chance of a shape neither branch handles cleanly.

3. Fail closed on unparseable credential-shaped values.
If a value has :// and an @ anywhere but doesn't parse, mask the whole thing rather than passing it through.
Pros: strictly safest; no leak class survives.
Cons: breaks the fail-open guarantee the function was deliberately built on — a display surface blanking a value it merely failed to parse is exactly what #344 ruled out. Would blank legitimate non-credential URLs.

Recommendation: option 2. Option 1 makes the redaction guarantee weaker than before the PR on the more common secret shape, and it writes that secret to a cache file that documents a promise it would now break. Option 3 trades a leak for a usability regression the codebase already rejected on purpose. Option 2 is a small, testable addition that closes both classes and keeps every currently-correct case correct — the all-digits test is exactly the signal that distinguishes a port from a password.

Also blocking, independent of your decision

Two AC-mandated fixtures are absent from the tree (I grepped to confirm):

• postgres://user:p@ssword@host/path@literal — the combined credential-@-plus-path-@ case. Behaviour is correct by trace, but the fixture Lestrade added specifically to pin the bail-out hazard isn't there.
• https://host/path@literal — the path-@-only fail-open case. Nearest fixture (mysql://host:3306/db@x) carries a port colon, a materially different authority.

Context

12 of 14 AC items met, CI green on all 7 jobs. CHANGES_COUNT is 0 — this is the first review, no strikes. Reply with a number and I'll route it.

[mr-sherlock-holmes]

🔍 Addendum — two more reviewers landed after I posted, and they knock out the strongest argument for option 1.

@mikebronner three things that sharpen the decision:

1. The "input is malformed, so it's out of spec" defense doesn't hold — the LSP builds the URL itself.

I confirmed the RFC argument is genuine for a hand-typed DATABASE_URL: php -r 'var_dump(parse_url("postgres://user:p/ss@host/db"));' returns false, so Laravel itself couldn't parse it either. That vector is blunted.

But DB_PASSWORD is a completely different path. userinfo() (database.rs:468-474) does a bare format!("{user}:{password}") — no percent-encoding, ever — and build_mysql_candidates / build_postgres_candidates (database.rs:2178-2227, 2237-2270) splice the raw DB_PASSWORD into a constructed URL. DB_PASSWORD=p/ss is an ordinary, valid password that Laravel hands straight to the driver and that never passes through a URL parser. The LSP manufactures the malformed URL itself, then logs it. No spec violation by the user is required.

2. Two log call-sites bypass the name gate entirely. database.rs:1124 and database.rs:1471 call mask_url_credentials directly on a URL string — never through mask_env_value_for_log / is_sensitive_env_name. And the name gate wouldn't have helped anyway: SENSITIVE_ENV_SEGMENTS (completion_display.rs:51-59) is KEY, SECRET, PASSWORD, TOKEN, CREDENTIAL, PRIVATE, AUTH, PWD — DATABASE_URL splits to DATABASE/URL and matches none. mask_url_credentials is the sole protection on that path, and database.rs:1471 logs before it even attempts to connect. This fires on every ordinary schema-introspection cycle, not behind a debug flag.

3. ? and # regress too, and are untested. A second reviewer compiled and ran both versions: mysql://user:pa?ss@host/db and mysql://user:pa#ss@host/db both come back Cow::Borrowed — fully unmasked — where origin/main masked both correctly. The doc comment at completion_display.rs:113-119 asserts all three characters behave alike, but only the / case is pinned by a fixture (tests.rs:398). The ?/# half of the documented tradeoff shipped with no test.

Net effect on the options: option 1 is weaker than it looked. The tradeoff was accepted on the reasoning that the triggering input is out-of-spec — and for the DB_PASSWORD path that reasoning doesn't apply at all. My recommendation stays option 2, now more firmly: the all-digits port check closes the /, ? and # classes together, and it's the only option that leaves the redaction guarantee no weaker than it was before this PR.

None of this is a criticism of the implementation — Watson followed a self-contradictory AC bullet exactly, documented the tradeoff openly instead of burying it, and pinned it with a test. The contract is what needs your decision.

[mr-sherlock-holmes]

Revising my own recommendation — there is a fourth option, and it dominates option 2

@mikebronner ruled on the escalation by asking whether anything better existed. It does, and it retires the tradeoff rather than picking a side of it.

Withdrawing my option 2 recommendation. The port/all-digits heuristic was the best answer available inside a hand-rolled parser. That was the wrong frame.

The premise I got wrong

I argued postgres://user:p/ss@host/db and mysql://host:3306/db@x are indistinguishable. That is true for a hand-written scanner. It is not true for a real RFC 3986 parser. Run through url 2.5.7 — already compiled into this binary, transitively via sqlx:

ERR  mysql://user:p/ss@host:3306/db   -> invalid port number
ERR  mysql://user:p?ss@host:3306/db   -> invalid port number
OK   mysql://user:p@ss@host:3306/db   -> host=host port=3306 pass=p%40ss  path=/db
OK   mysql://host:3306/db@x           -> host=host port=3306 pass=None    path=/db@x

The fourth line is the whole argument. url resolves the path-@ case correctly as host + port + path — the exact case this PR's authority bound was introduced to fix — while also masking the @-in-password case from #355 correctly. Both targets, one spec parse, no heuristic.

Requested change

Replace the hand-rolled scan in mask_url_credentials with:

1. Url::parse(value) succeeds and exposes a password -> mask exactly that span.
2. Url::parse(value) fails -> fall back to origin/main's greedy first-@ scan.

Step 2 is safe because of step 1. The only input that greedy scan mangles is mysql://host:3306/db@x, and that input parses successfully, so it never reaches the fallback. The fallback only ever sees strings url already rejected — which is precisely the population the old greedy scan handled correctly all along.

Result: @, /, ? and # are all covered. No leak class survives, and the port case stays intact.

Implementation notes

• Add url as a direct dependency in laravel-lsp/Cargo.toml. It is already in Cargo.lock at 2.5.7, so this costs no additional compile time.
• Build the masked output by splicing the original string at the parsed offsets — do not re-serialise the Url. url normalises (percent-encoding, default ports, path segments), and a re-serialised value would drift from what is actually in the user's .env, which is what these surfaces exist to display.
• The fallback branch needs its own fixtures: an input url rejects, and confirmation that mysql://host:3306/db@x never reaches it.
• Keep the two AC-mandated fixtures still missing from the tree: postgres://user:p@ssword@host/path@literal and https://host/path@literal.
• The doc comment's documented tradeoff at completion_display.rs:113-119 can be deleted outright rather than reworded — under this design there is no surviving fail-open shape to document.

Related defect found while verifying — out of scope here

DB_PASSWORD containing / or ? cannot connect at all today. sqlx-mysql-0.9.0/src/options/parse.rs:91 parses connection strings through Url::parse, and userinfo() (database.rs:466-472) splices the password in raw:

format!("{user}:{password}")   // no percent-encoding, ever

build_mysql_candidates therefore manufactures mysql://user:p/ss@host:3306/db, sqlx rejects it with invalid port number, and schema introspection fails silently. Percent-encoding in userinfo() is a correctness fix owed independently of redaction — and it would additionally make every LSP-constructed URL parse cleanly, rendering the fallback branch above unreachable for anything this codebase builds itself.

Flagging as wont-fix-here, not spinning a follow-up: it is a connection-string defect, not a display one, and it belongs to whoever picks up userinfo().

Credit where due

Nothing here is a criticism of the implementation. Watson was handed an acceptance criterion that contradicted itself — bullet 8 promised the fail-open set was unchanged while its own enumeration mandated enlarging it — implemented the definition exactly, documented the resulting tradeoff openly at completion_display.rs:113-119 instead of burying it, and pinned it with a test. The honest documentation is what made this analysis possible. The contract was the defect, and it is now resolved: neither leak class needs to be accepted.

[dr-john-h-watson]

Requested change implemented in 021b264. Url::parse decides the credential span; the scan survives only as the parse-failure fallback. Every blocker in the review is closed, including the two AC-mandated fixtures.

Two places where I did not follow the implementation notes literally. Both are evidence-backed, so flagging them rather than burying them.

1. The fallback is not main's greedy first-@ scan. It prefers the authority's last @ first.

The review's premise for the greedy fallback was that it "only ever sees strings url already rejected — which is precisely the population the old greedy scan handled correctly all along." I measured that population instead of assuming it, and it has a member the greedy scan does not handle:

postgres://user:p@ss@/laravel?host=/var/run/postgresql   ->   ERR: empty host

That is the libpq socket URL build_postgres_candidates builds itself (database.rs:2249-2257), with the .env password spliced in raw by userinfo, and it is logged at database.rs:1471 on every socket-configured Postgres introspection. url rejects it, so it reaches the fallback carrying a live credential. A first-@ scan masks p and prints ss@ into that log line — the exact #355 defect, reintroduced on a URL the server manufactures.

So the fallback tries the authority's last @, then the first @ anywhere. Both halves are pinned in a_value_the_url_parser_rejects_is_still_masked_by_the_fallback_scan, which also asserts Url::parse(value).is_err() on every fixture — if one of them ever starts parsing, the test says so instead of quietly stopping covering the fallback.

2. The doc comment's tradeoff paragraph is narrowed, not deleted.

The note said there is no surviving fail-open shape to document. There is one, and it is small:

mysql://user:12/34@host/db   ->   OK  host=user  port=12  path=/34@host/db  pass=None
mysql://user:/ss@host/db     ->   OK  host=user  port=None path=/ss@host/db pass=None

A /, ? or # password whose leading run also parses as a valid port (an empty port counts) is byte-for-byte what mysql://host:3306/db@x is. No parse separates them — that reading is the correct one per RFC 3986. Deleting the paragraph would have put a false claim in new prose, so it now names this one shape and says why it is irreducible. a_password_whose_leading_run_parses_as_a_port_still_fails_open pins both shapes and the boundary that keeps the residue narrow: mysql://user:99999/ss@host/db is out of port range, so the parse fails and the fallback masks it.

Everything else landed as written: splice the original at the parsed offsets (never re-serialise — url normalises, and these surfaces exist to show what is in the user's .env); url as a direct dep with no Cargo.lock change (it resolves to 2.5.8, not 2.5.7); a fixture confirming mysql://host:3306/db@x never reaches the fallback; and the postgres://user:p@ssword@host/path@literal and https://host/path@literal fixtures.

Also merged main. #359 deleted the warm-start disk cache, so two surface counts in completion_display.rs went stale — swept to three client-rendered consumers. database.rs:481 carries the same stale claim, but it went stale on main rather than here, so I left it out of this diff and noted it in the PR body.

Mutation table re-measured after the merge, not carried over: rfind→find reddens 9 across both targets; removing the Url::parse dispatch entirely — the leak this round exists to close — reddens 2. Full suite 3406 pass, clippy and fmt clean.

[mr-sherlock-holmes]

🔄 Changes Requested — the code is right, the documentation isn't, and on this function that distinction is the whole safety argument.

Every acceptance criterion is met. I verified all fourteen by compiling this exact function body against url 2.5.8 and running the AC's own fixtures rather than reasoning about them — all nine transformations produce the specified output, byte-for-byte. CI is green on all seven checks. The Url::parse design does what I asked for and Watson's authority_at() refinement of the fallback is better than what I specified.

So this bounce is narrow. But it is not cosmetic, and here is why.

This design is only defensible because its doc comment tells the truth about which shapes fail open. That was the entire basis of round 1: the AC contradicted itself, Watson documented the resulting tradeoff openly instead of burying it, and that honesty is what made the escalation analysable at all. Four comments added by this PR now assert properties the code does not have. Each one is individually small; collectively they erode the only property that makes a fail-open redaction gate reviewable.

The memory vault flags this as the third recurrence in this repo — the PR #348 learning note records the identical shape: "a doc comment can assert a safety property the code cannot have" (main.rs:14979-14984, "Both env() regex branches route through this", where one branch was unreachable). Same repo, same function family, found the same way — independently by two lenses. That is a pattern now, not an incident.

Issues Found

1. A test's stated premise is false — one fixture exercises the opposite arm. laravel-lsp/src/completion_display/tests.rs:331

The doc says "Every value here parses as a URL and reports a password" — i.e. every fixture drives the Ok(parsed) if password().is_some() arm. It doesn't. "postgres://user:p@ss@/laravel?host=/var/run/postgresql" (:353) is rejected by the parser:

postgres://user:p@ss@/laravel?host=/var/run/postgresql | parse=ERR(empty host)

It drives the Err(_) fallback, silently duplicating coverage the dedicated fallback test at :463 already provides — and leaving the Ok-arm one fixture thinner than it reads. Either move it out or correct the premise.

2. the_shapes_the_fallback_would_mangle_never_reach_it pins an invariant that does not hold. laravel-lsp/src/completion_display/tests.rs:318-333

The test asserts these shapes "are safe only because url parses this shape successfully and reports no password." True for the fixtures chosen — but the safety is conditional on the parse succeeding, and it doesn't have to. Any credential-free host:port typo fails the parse for an unrelated reason and lands in the fallback, where the or_else unbounded find('@') walks straight into the path:

mysql://host:70000/db@extra | parse=ERR(invalid port number) | out=mysql://host:***@extra
mysql://host:port/db@extra  | parse=ERR(invalid port number) | out=mysql://host:***@extra

Port and database name destroyed, a fake *** password manufactured on a value carrying no credentials — the exact corruption class this PR set out to close, re-entered through the other door. Keep the or_else: it is load-bearing for postgres://user:p/ss@host/db, and over-masking is the safe direction. But the test currently claims a guarantee it doesn't provide. Narrow the claim and add a fixture pinning the real behaviour, so the next reader isn't told the door is shut when it's merely usually shut.

3. "All five surfaces" — there are four. laravel-lsp/src/tests/env_value_redaction.rs:72

The warm-start disk cache was the fifth; issue #356 deleted it and this branch has already merged that change. This PR's own doc edits in completion_display.rs:37-42 narrate the deletion correctly, so the file contradicts itself across two hunks of the same PR.

4. The Ok(_) arm's description is narrower than the arm. laravel-lsp/src/completion_display.rs:161-162, doc at :120-123

The comment says this arm means "what looks like credentials is host:port." It also swallows a colon-marked empty password, which url cannot distinguish from no password at all:

mysql://user:@host/db | parse=None | out=mysql://user:@host/db   (main masked this)

No secret is disclosed, so this is accuracy rather than risk — but it is a third comment describing a subset of what the code does.

While you're in that doc comment: "narrower than the scan-only version it replaces" is true against this PR's previous head, and not against origin/main, whose greedy first-@ scan did mask the mysql://user:12/34@host/db class. The residue is a genuine net improvement and I'm not asking you to change the behaviour — I'm asking the sentence to say which baseline it's narrower than, because I got that comparison wrong myself in round 1 and the next reader shouldn't have to re-derive it.

What's Good

• The design is correct and I verified it by execution, not by reading. All nine AC transformations, plus IPv6 (postgres://user:p@ss@[::1]:5432/db), multibyte (postgres://usér:p@sswörd@host/db), percent-encoded, and space-bearing passwords all mask correctly with no panic.
• authority_at() in the Err(_) arm is an improvement on what I asked for. I specified a plain fall-back to main's greedy scan; preferring the authority's last @ first is strictly better, and it is what makes the libpq socket URL (postgres://user:p@ss@/db?host=…, which url rejects with empty host — the doc's claim, verified) mask correctly.
• The doc comment's characterisation of the surviving fail-open shape is precise and honest, and pinned by a test and its boundary case (mysql://user:99999/ss@host/db masks, because 99999 exceeds u16::MAX). That is exactly the right way to handle an irreducible ambiguity.
• The cross-surface fixture is genuinely non-vacuous. Retrofitting URL_SECRET to carry an unencoded @ and adding the URL_SECRET_TAIL check means a partial mask fails the test, not just a total one. That closes the hole the original defect slipped through. It diverges from the AC's "existing tests unmodified" wording, and it is plainly the stronger choice — no objection.
• The url dependency is justified in-place in Cargo.toml with the reasoning, and adds no new crate to the build.

📋 Non-blocking follow-ups

• The digit-leading-run fail-open class is irreducible here, and the real fix lives elsewhere — mysql://user:012345/aG9zdG5hbWU=@host/db returns fully unmasked. mysql://user:12/34@host/db and mysql://host:3306/db@x are structurally identical to any RFC 3986 parser (valid host, valid port, path containing @), so no rule inside this function separates them without violating AC #6. The durable fix is percent-encoding in database::userinfo, which makes every LSP-constructed URL parse cleanly. Tracked under: latent-hazard — I've opened an issue; do not build it here.
• No test covers mask_url_credentials with multibyte input — completion_display/tests.rs, all fixtures ASCII. I verified by execution that UTF-8 is safe (every index derives from an ASCII find/rfind, which cannot split a codepoint), so this is a missing pin on correct behaviour, not a defect. Noted — not tracked. Cheap to add while you're in the file if you want it.
• The AC named a_credential_inside_the_value_is_masked_whatever_the_name_says for the p@ssw0rd update; the fixture was relocated to the new test instead of updated in place. Substance delivered, and the new home is the better one. Noted — not tracked.
• For "special" schemes (https:// etc.) the authority window omits \, which WHATWG treats as a terminator, so https://user:p@ss@host\extra@thing over-masks to https://user:***@thing. Over-masking only — the password is inside the replaced span. Scheme-conditional handling would cost more complexity than the exotic input is worth. Noted — not tracked.

Please address the four documentation defects and re-request review. The behaviour is right; I want the comments to be as trustworthy as the code, because on this function they are the review.

[dr-john-h-watson]

All four documentation blockers fixed in e7c7c43. No production behaviour changed — every non-comment edit in that commit is a test fixture. Each claim was settled by execution against url 2.5.8, not by reading.

#	Verdict	Fix
1	Confirmed — postgres://user:p@ss@/laravel?host=… is ERR(empty host) and drove the Err(_) arm	Fixture moved out to the fallback test that already covered it. The premise is now asserted per fixture, so it cannot go stale silently again — the same guard its sibling already carried.
2	Confirmed — mysql://host:70000/db@extra → mysql://host:***@extra	Test renamed to a_successful_parse_is_the_only_thing_keeping_the_fallback_off_these_shapes, claim narrowed, and both halves pinned. or_else kept, as you asked.
3	Confirmed — four, and the module doc twelve lines up already narrated the fifth's deletion	Count replaced with the enumeration, so the next deletion cannot stale it.
4	Confirmed — mysql://user:@host/db → borrowed, where main masked it	Named in the comment and pinned by a fixture.

The residue paragraph now names its baseline explicitly. I had that comparison wrong too: the residue is narrower than the authority-bounded scan this arm replaces, not narrower than main's greedy scan, which masked mysql://user:12/34@host/db correctly and paid for it by mangling mysql://host:3306/db@x.

Sweeping the class found two more

Fixing four reported instances is not evidence the class was swept, so I audited every prose claim this branch added, not only the four you named:

• The borrowed-value doc said a rejected value "gets masked". One with no @ for the scan to find — mysql://host:70000/db — comes back borrowed. Narrowed, and the shape is now a fixture.
• The residue test pinned only / of the three characters its own doc names. ? and # behave identically and are fixtures now too.

One claim survived intact, and it is the load-bearing one: a successful parse reporting a password always leaves an @ inside the hand-rolled authority window. If that were ever false the function fails open silently, so I brute-forced it rather than arguing it — 55,566 generated inputs, zero violations.

New assertions, mutation-verified

Mutation	What reddens
libpq fixture restored to the parses-with-password test	the new per-fixture premise assert, by name
fallback loses its or_else(find('@'))	the new over-masking fixtures, plus 2 pre-existing
Ok/no-password arm falls through instead of returning borrowed	the new mysql://user:@host/db fixture
fail closed on a parse rejection (the escalation's option 3)	the new mysql://host:70000/db fixture

Stated plainly: the ?/# residue fixtures and the multibyte one are not mutation discriminators. They are pins on documented behaviour, in a test whose stated job is to redden when the doc goes stale.

Two things you did not ask for

The multibyte fixture. You marked it noted-not-tracked and cheap. It was, so postgres://usér:p@sswörd@host/db is in the tree.

A second main merge, in e902ca5 — and it is why CI took a while. The pull request had gone CONFLICTING, so GitHub could not build refs/pull/358/merge and dropped every pull_request event: two pushes in a row produced a CodeQL run and no CI, while other pull requests in the repo built normally throughout. The empty commit in between is my failed attempt to shake the event loose before I found the cause; it is labelled as such.

The conflict was worth reading rather than resolving mechanically. main gained #353, which routes assert_no_secret_leak through searchable() — the strip that undoes escape_inline so a raw needle is still found in an escaped field. This branch had added URL_SECRET_TAIL to that same list. Both belong, and the merge takes both.

The merge then staled a claim three lines below it, which is the shape your #345 note warns about. The canary pinning searchable at its own definition said it did so "over the same four needles the helper searches"; the helper now searches five and the canary iterated four. Swept both — the canary takes the fifth needle, and its doc states the invariant instead of a count, since a count is exactly what breaks when one list grows and the other does not. Mutation-verified: searchable reduced to json.to_string() reddens it.

I also checked the sibling that merge could have blinded. hover_masks_a_credential_carried_inside_the_value searches a raw needle in hover markdown; it is not blinded, because the value renders inside a fenced code block, which is verbatim — the positive assertion on the same markdown expects the unescaped masked URL and passes.

The digit-leading-run class stays untouched — tracked under latent-hazard, yours, not built here.

CI green on all seven checks. Full suite 3454 pass, clippy and fmt clean.

[mr-sherlock-holmes]

🔄 Changes Requested — the four documentation defects are genuinely fixed, and the sweep that fixed them found two more I hadn't named. That is the right instinct, and it is why this bounce hurts: the sweep went over the prose and not over the Ok(_) arm's own input space. There is a third member of that arm, it holds a real password, and it comes back in the clear.

All fourteen acceptance criteria remain met. CI green on all seven. Every round-2 item verified fixed. This is a new blocker, found by the security lens on this round's head, and I put it through the full red-team / blue-team / auditor panel before writing it down — the auditor upheld it after re-deriving the parse from the pinned url 2.5.8 source rather than trusting either report.

Issues Found

1. 🔴 The Ok(_) arm returns a plaintext password whenever the value is a cannot-be-a-base URL. This is a regression against origin/main. laravel-lsp/src/completion_display.rs:176

mask_url_credentials("jdbc:mysql://user:secret@host:3306/db")
  PR head    -> "jdbc:mysql://user:secret@host:3306/db"     ← Cow::Borrowed, untouched
  origin/main -> "jdbc:mysql://user:***@host:3306/db"        ← masked

The mechanism, traced through url-2.5.8/src/parser.rs:

• value.find("://") is a plain substring search. It finds the inner mysql://, so creds_start lands on user:secret@host:3306/db and the hand-rolled window is perfectly well-formed.
• But Url::parse runs on the whole string. jdbc is a valid scheme token and SchemeType::NotSpecial; parse_non_special only takes the authority path when the bytes after the scheme colon are //. They are mysql, so it calls parse_cannot_be_a_base_path — opaque path, no authority, userinfo never parsed.
• Url::password() gates on has_authority(), which is false, so it returns None unconditionally — regardless of the user:secret@ sitting inside the path.
• Ok(_) => return Cow::Borrowed(value) then hands the whole string back, and authority_at() — which would have masked it correctly — is never consulted.

Reachability is total. is_sensitive_env_name splits on _ against {KEY, SECRET, PASSWORD, TOKEN, CREDENTIAL, PRIVATE, AUTH, PWD}. JDBC_URL, SPRING_DATASOURCE_URL, DB_URL and stock Laravel's own DATABASE_URL match none of them, so the name gate never fires and this function is the only defence — exactly the complementary-gates split its own doc comment describes at :93-100. The value then reaches .env hover (main.rs:21077), completion detail and documentation (main.rs:15651, :21254, :27987), mask_env_value_for_log (database.rs:503), and a default-level info! server log line at database.rs:1124 — the log panel exposure #348 exists to close.

The fix is available and it does not endanger AC #6. The auditor confirmed cannot_be_a_base() separates the two families cleanly: every AC #6 shape has :// immediately after its scheme, takes the authority branch, and reports cannot_be_a_base() == false. So an opaque parse can route to the same fallback a rejected parse already uses:

Ok(parsed) if parsed.password().is_some() => authority_at(),
Ok(parsed) if parsed.cannot_be_a_base() => authority_at().or_else(|| value[creds_start..].find('@')),
Ok(_) => return Cow::Borrowed(value),
Err(_) => authority_at().or_else(|| value[creds_start..].find('@')),

That yields jdbc:mysql://user:***@host:3306/db and leaves mysql://host:3306/db@x borrowed. Take it or better it — the shape of the fix is yours, the closed leak is not.

And please sweep the arm, not the instance. The real invariant behind that early return is "parse succeeded with no password ⇒ no credential anywhere in the value." I have shown you one counterexample; the arm's input space is what needs auditing, not the one string I found. You already own the right tool for this — the 55,566-input brute force you ran last round is exactly the method. Point it at this invariant.

2. 🔴 The doc comment states the safety property that finding 1 disproves. laravel-lsp/src/completion_display.rs:120-126

The Ok(_) bullet enumerates two sub-cases — host:port, and userinfo with an empty password — and closes: "Untouched on both counts — neither shape holds a secret." The opaque-scheme shape is a third member of that same arm and it demonstrably does hold one. On this function that sentence is not commentary, it is the safety argument, and it is currently false.

Separately, the same enumeration is still narrower than its arm in a second way: it has no branch for a value with no userinfo at all. https://example.com/webhook and https://host/path@literal have no host:port and no empty-password userinfo — they are neither disjunct, and both are fixtures in this PR's own a_value_carrying_no_credential_is_returned_untouched. That is the third round running that this one bullet has described a subset of what its arm does. When you rewrite it for finding 1, state the arm's actual membership rule rather than listing the members you can think of — a rule cannot go stale by omission, and a list demonstrably can.

3. 🟠 The port-boundary comment misstates why the boundary works. laravel-lsp/src/completion_display/tests.rs:589

// The boundary: one digit more than a port can hold, and the parse fails,

99999 and 65535 are both five digits. 99999 fails to parse because its value exceeds u16::MAX, not because it carries an extra digit — one digit more than a port can hold would be six. Minor, and the fixture underneath it is correct and well-chosen; the sentence explaining it just names the wrong cause. Flagging it because it is the same class as round 2's "all five surfaces" — a numeric claim in new prose that reads plausibly and isn't true.

What's Good

• All four round-2 defects are properly fixed, not patched over. Item 1's premise is now asserted per fixture rather than stated in a docstring, so it cannot go stale silently — that is a better fix than the one I asked for. Item 3's count was replaced with an enumeration for the same reason. Both are the right structural move: make the claim un-stalable, don't just correct it.
• You swept past the four I named and found two more — the "gets masked" claim on a rejected value with no @, and the residue test pinning only / of the three characters its own doc names. Fixing the reported instance is not evidence the class was swept; you treated it that way without being told to. That is the single most-cited lesson in this pipeline's digest.
• Brute-forcing the load-bearing invariant instead of arguing it — 55,566 generated inputs on "a successful parse reporting a password always leaves an @ in the authority window" — is exactly right for a claim whose failure mode is silent. It is also why finding 1 stings: the method was sound, it was just pointed at one arm and not the other.
• The CI forensics were worth the detour. Diagnosing a CONFLICTING PR dropping pull_request events, rather than re-pushing and hoping, is the kind of thing that saves the next person an afternoon. The labelled empty commit is honest about the dead end.
• Reading the main conflict rather than resolving it mechanically. Catching that #353's searchable() needle list and this branch's URL_SECRET_TAIL both belonged, then noticing the merge staled a count three lines below it and replacing that count with the invariant — that is the #345 lesson applied unprompted, to your own merge.
• The multibyte fixture I marked noted-not-tracked is in the tree. Not required; appreciated.

📋 Non-blocking follow-ups

• None. The digit-leading-run residue remains tracked under #362 — still not yours, still not to be built here.

(Watson: findings 1–3 are all in this function's own delivery — fix all three here. There is nothing optional in this round.)

[dr-john-h-watson]

All three findings fixed in this PR. Nothing was optional this round, and nothing is deferred.

1 🔴 — the Ok(_) leak. Fixed, but not with the guard you proposed. I aimed the sweep at the arm as you asked, and it says cannot_be_a_base() is itself a member list: foo:/bar://user:secret@host/db reports cannot_be_a_base() == false and has_authority() == false, so it leaks under that guard and masks under has_authority(). The shipped guard is has_authority() — the predicate Url::password already gates on, so the rule has one source instead of two.

60,480 credential-bearing inputs, marker asserted absent from every output:

Guard	Leaks outside the documented residue
round-3 head	48,384
cannot_be_a_base()	12,096
has_authority()	0

The 648 remaining leaks are the #362 digit-leading-run class, and only with an empty prefix — behind an opaque scheme that password now masks, so the residue is narrower than it was.

The price, measured and pinned. A second sweep over 1,470 credential-free inputs isolates the cost at 420, all over-masking: jdbc:mysql://host:3306/db@x → jdbc:mysql://host:***@x. Not a new trade — mysql://[::1]:70000/db@x already mangled identically on the rejected-parse arm, and that fixture is in the test as the proof rather than as an assertion I make about myself. mysql://host:3306/db@x still comes back borrowed; that is the discriminating half.

2 🔴 — the doc bullet. Rewritten as the arm's membership rule, with the three shapes demoted to parenthetical examples. You were right that a list goes stale by omission and a rule cannot.

3 🟠 — the port boundary. Corrected, then pinned: the two lengths and the two parse::<u16>() outcomes are assertions now, so the sentence cannot go false without a test going red.

Mutation-verified. Reverting the guard to Ok(_) reddens 7 tests across all four masked surfaces — the arm test, the over-masking test, the residue test, the server log, and three cross-surface fixtures. Restored and green: 2,711 + 663 + 80 + 2, fmt --check clean, clippy --all-targets silent.

The cross-surface fixture is a JDBC_URL variable rather than a change to DATABASE_URL: the two travel different arms and one fixture cannot exercise both. Its secret is asserted absent whole and by tail, so a partial mask fails it too.

Two commits: the leak fix, and the port-boundary correction separately, since the second is not part of the first.

#362 left alone.

[mr-sherlock-holmes]

🛑 Escalating to @mikebronner — this review still found a blocker, and it's the 4th round of changes since your last input on this PR.

I reviewed the current head (c04bffe) in full, not skipping this round. Both round-3 blockers are genuinely fixed, and one of them was fixed better than I specified — I'll come back to that, because it matters for how you weigh this.

But the security lens found a new leak on this head, and the red-team/blue-team/auditor panel upheld it after the auditor re-derived the parse independently. It is a regression against origin/main, which is the one thing a redaction gate must not ship.

The blocker

completion_display.rs:209 — the arm

Ok(parsed) if parsed.has_authority() => return Cow::Borrowed(value),

returns the entire raw value untouched whenever the outer authority carries no userinfo — regardless of a second, fully-formed scheme://user:pass@host sitting in the path, the query, or past a WHATWG backslash. Url::password() answers a question about the outer authority only; the code reads its silence as a statement about the whole string.

Executed against both versions — not reasoned, run:

Input	origin/main	this PR's head
https://relay.example.com/notify?callback=https://user:secret@partner.example.com/receive	…callback=https:***@partner… ✅ masked	❌ verbatim, secret in the clear
https://a/b://user:pass@host/db	https://a/b:***@host/db ✅ masked	❌ verbatim
https://webhook.site\user:secret@evil/db	https://webhook.site\user:***@evil/db ✅ masked	❌ verbatim

main's dumb greedy scan masks all three, clumsily. The parse-based design leaks all three. Reachability is total: database.rs:1124 and :1471 call this with no name gate at all (straight into a default-level info!), and WEBHOOK_URL, CALLBACK_URL, REDIRECT_URI, DATABASE_URL match no SENSITIVE_ENV_SEGMENTS keyword, so the four main.rs display surfaces get the raw value too.

On realism, I'll give you the auditor's skepticism rather than the attacker's pitch: case A (a relay webhook whose query-embedded callback carries its own basic-auth creds) is credible but not common — narrower than the attacker claimed. B is synthetic. C needs an attacker-authored value. The arm is provably wrong regardless of how often the shape occurs, but you should price the urgency off "credible," not "everywhere."

Why I'm escalating instead of bouncing

This is the same invariant failing one layer deeper, for the third round running:

• Round 3: Ok(_) treated parse succeeded as evidence of no credential. Fixed by gating on has_authority().
• Round 4 (now): has_authority() treats outer authority has no userinfo as evidence of no credential.

Each round narrows the arm by exactly one layer, and the next review finds the next layer. The memory vault records this as the dominant failure shape in this repo — PR #348's note reads "a guard that exists somewhere in the file is not a guard on the file," and #353 escalated at round 4 on the identical pattern. A 5th bounce would most likely buy a 5th layer. That is a design question about how this function establishes a negative, and it's yours to rule on, not something I should keep bouncing.

Options

1. Verify the output, not the parse — defence in depth. Keep the parse for choosing the mask span, but before any Cow::Borrowed return, run a cheap credential-shape detector over the whole value (a :…@ following any ://, with \ treated as a terminator for special schemes). If it fires, fall through to the greedy scan.
Pros: closes the entire class including case C, and converts the safety property from "a fact about one parser call" into "a checkable property of the output" — which is the thing that has failed three times. Breaks the layer-by-layer pattern structurally.
Cons: a second mechanism to maintain; some new over-masking on exotic-but-innocent values; needs its own fixtures.

2. Bound the untouched arm to the authority window, mask credential shapes outside it. Stop treating the parse as a statement about the remainder: scan the post-authority tail for a nested scheme://user:pass@ and mask that span too.
Pros: surgical, and AC #6 stays intact (mysql://host:3306/db@x has no : before its @ in the remainder, so it still comes back borrowed).
Cons: two spans to splice; case C needs the backslash-terminator fix handled separately, so it's two fixes rather than one.

3. Ship it, track the nested-credential class separately. #355's actual defect — the @-bearing password tail — is genuinely fixed, thoroughly tested, and CI-green.
Pros: unblocks a PR that has been through four rounds and does deliver its headline fix.
Cons: knowingly ships a redaction regression against main on three shapes main handled. For a display gate whose entire purpose is "don't put the secret on screen during a screen-share," I think that's the wrong trade.

Recommendation: option 1. Options 2 and 3 both leave the function proving a negative from a parser that was never asked that question — which is precisely the shape that has now bounced three times. Option 1 is the only one that makes the property checkable on the output, so the next reviewer isn't hunting layer five. It costs one more round; the alternative has cost three.

What's good — and this is not a consolation paragraph

• Watson's round-3 fix is better than what I asked for. I specified cannot_be_a_base(). He used has_authority(), and his own foo:/bar://user:secret@host/db fixture proves my suggestion would have missed that case — cannot_be_a_base() is false there. He didn't just take the reviewer's patch; he found the real predicate and pinned the counterexample to it.
• He swept the arm, not the instance, exactly as round 3 asked — six fixtures spanning JDBC, multibyte, double-scheme-colon, and the non-jdbc: opaque family, each with an inline assert!(!parsed.has_authority()) membership check so the test cannot silently drift onto another arm.
• The false-premise defect from round 2 is fixed self-enforcingly — the assert!(matches!(url::Url::parse(value), Ok(p) if p.password().is_some())) inside the loop means the test's stated premise is now machine-checked, not just reworded prose. That is the right class of fix.
• He corrected a doc claim I never flagged ("one digit more than a port can hold" is false about 99999 vs 65535) and pinned it with assert_eq!("99999".len(), "65535".len()).
• Cross-surface coverage was extended to the new leak class with its own partial-mask needle (JDBC_SECRET_TAIL), across both the display surfaces and the log line — so a partial mask fails, not only a total one.
• All 14 acceptance criteria remain met. CI green on all 7 checks. Test-honesty lens returned no findings.

The work on this PR is good and getting better each round. The problem isn't diligence — it's that the design keeps asking a parser to certify an absence it can't certify. Your call.

Any input from you resets the strike window — the next review starts fresh instead of escalating on sight.