fix(cli-memd): withhold URLs whose credentials cannot be attributed

[waterbro-8]

Refs #112

Summary

A configured URL that carries credentials in a shape url.Parse does not report
as userinfo reaches output today. This adds one shared gate that redacts such
a value when it can prove it is a transport URL, and withholds the value whole
when it cannot, and wires it into all three egresses R3 names: the API client,
memd's log lines, and mem doctor.

The adjudicated direction this implements is recorded on #112 as R3
(comment 5536659086): gate on "parses as a recognized transport scheme with
userinfo redacted, otherwise withhold the whole value".

Scope: all three named egresses

R3 names three egresses — doctor, apiclient, memd's log line. All three are
covered here.

egress	on main at the base	covered here
apiclient — request construction	leaks	gated
apiclient — all 4 http.Client.Do sites	leaks, ungated entirely	gated, one test case per site
apiclient — newRequest on the workspace-transfer path	leaks	gated (arrived with #164's workspace_transfer.go)
memd startup log line	leaks (User != nil gate)	gated
memd fatal log line	leaks via a third-party error	gated
mem doctor (text + JSON)	not present	gated through the same package

How doctor got onto this branch

mem doctor is not on main; it only existed on #164 (which sits on #131's
original doctor commit). Because #131 was closed as superseded by #164, doctor
had exactly one carrier, so the surviving branch had to absorb it before #164
could be closed. Rather than re-author it, this branch merges the work:

c9e0e63  docs: correct doctor's stated secret guarantee                        (this change)
d30f1ff  fix(cli): route mem doctor's URL reporting through the shared gate   (this change)
ad4e78f  Merge origin/main
d7f2dcb  Merge #164 (mem doctor) into the credential-url-gate branch
b229537  fix(client,memd): withhold URLs whose credentials cannot be attributed
fffcd4c  fix(cli): redact malformed URLs and request build errors   ← #164, author PeterGuy326
46499b2  feat(cli): add mem doctor and first-run guidance           ← #131, author waterbro-8

Both earlier commits are ancestors of this head, so #164's and #131's authorship
is preserved on the merge rather than claimed as mine. #164's unrelated
per-probe --timeout fix and its workspace_transfer.go coverage came across
with the merge and are kept.

What d30f1ff itself does is replace doctor's two local string-level helpers
with calls into internal/redact, so there is one policy rather than three:

func redactURL(raw string) string          { return redact.URL(raw, redact.APIURLs) }
func sanitizeProbeError(err error) string  { return redact.TransportError(err, redact.APIURLs) }

c9e0e63 is prose only. It corrects three statements — in CHANGELOG.md,
docs/DEPLOYMENT.md and a comment in cmds_doctor.go — that asserted the doctor
report contains no secret value. That was never true of the query-parameter
shape described under Known gap, measured at 3 sentinel occurrences per output
format on this head. The non-goal stands; the promise was the thing that was
wrong, so the docs now name the residual instead of denying it.

The shape, and why the previous gate missed it

url.Parse("admin:pw@host")
→ Scheme="admin"  Opaque="pw@host"  User=nil

A gate written as if parsed.User == nil { return raw } therefore echoes the
credential verbatim — not as a corner case, but as the normal path for any
value whose scheme happens to contain a colon.

The gate also refuses to scrub error text, because that cannot be made tight:
url.Error renders with %q, so a " inside a password arrives escaped and a
scanner that pairs quotes mis-pairs, replaces nothing, and leaves its cursor
inside the URL. Withholding is chosen over partial trimming for the same reason:
a delimiter inside a credential splits the message into pieces that no longer
look like a URL, and the piece without the @ is exactly the half that leaked.

memd's fatal line is included because queue.NewClient wraps asynq's parse
error, which embeds the whole DSN:

queue: parse redis url: asynq: could not parse redis uri:
  parse "redis://:PASS@ho st:6379/0": invalid character " " in host name

slog renders an error value as its text, so that reaches the log unmodified.

Known gap, stated rather than smoothed over

The gate proves the absence of userinfo, not of every credential. A secret
supplied as a query parameter (redis://host:6379/?password=x) parses as a
clean URL with User == nil and is still echoed. This is the adjudicated
scope, not an oversight, and closing it is a separate decision. It is pinned by
a named characterization test
(TestTextKnownGapQueryParameterCredentialsAreEchoed) that fails if someone
closes the gap without updating the expectation, so the residual cannot become
folklore.

Withholding also has a real diagnosability cost, accepted by design: the reason
a request failed is still reported, the host is not.

Error: Get [withheld]: unsupported protocol scheme "admin"

Validation ledger

Evidence level claimed: E3 — independently reproduced. Not E4: I am the
author, and E4 requires a non-author to verify the acceptance criteria end to
end.

Environment: Linux x64, Go 1.25.0, exact tree
9fa9c14de074b4ddb5320d4f83803f1657918b34 — HEAD^{tree} of the current head
c9e0e63, read back from GET /repos/bytefolk/mem/commits/c9e0e63… and equal to
it byte-for-byte, so the tree measured here and the tree on the PR are the same
object. (This head was pushed through the Git Data API because github.com:443
was down on this box; the local commit sha differs from the remote one, the tree
does not.)

Check	Result
go build ./...	pass
go vet ./...	pass
gofmt -l .	no output
go test -count=1 ./...	32 packages ok, 0 FAIL
cmd/mem (includes 16 mem doctor tests)	102 tests, 0 fail
internal/redact	30 tests, 0 fail
internal/apiclient	45 tests, 0 fail
cmd/memd	15 tests, 0 fail
git merge-base --is-ancestor origin/main HEAD	yes (main not silently reverted)
CI on this head	15/15 check names success on c9e0e63 (run_attempt 1), including Go and PostgreSQL integration; also 15/15 on the immediately preceding d30f1ff

mem doctor end-to-end, measured on built binaries

Seven malformed credential URL shapes, each run through
mem doctor --server <shape> --timeout 1s in both text and json,
counting occurrences of a sentinel password in everything the process wrote:

#	shape	#164 head fffcd4c	this head c9e0e63
1	http://admin:PW@127.0.0.1:1	0	0
2	http://admin:PW@127.0.0.1:%zz	0	0
3	http://admin:PW@ho st.example.com	0	0
4	http://admin:PW x@127.0.0.1:1	0	0
5	http://admin:PW@%	0	0
6	http://admin:PW@mem.internal:99999999	0	0
7	admin:PW@mem.internal:8787 (no scheme)	6	0

Shape 7 is the url.Parse → User=nil case from the section above: #164
prints the configured URL three times (server, detail, hint) in each of
the two formats, so 6 occurrences is one leak per field.

A zero from a binary that has no doctor command means nothing. This table
was first produced against a build of b229537 and came back 0/0/0/0/0/0/0,
which looked like a pass — the actual output was
Error: unknown command "doctor" for "mem", because that head had no doctor at
all. The harness now counts unknown command alongside unknown flag and every
row above was taken with both counters at 0.

Mutation controls

Run against a copy, with the mutation confirmed present in the source before the
suite is trusted:

1. Reverting the gate to the pre-fix User != nil form turns 10 named cases
   red across 3 packages (internal/redact, cmd/memd, cmd/mem).
2. Removing the gate at one apiclient Do site turns red exactly that
   site's test case and leaves the other three green. The four cases map
   one-to-one onto the four sites, so a regression at a single site cannot hide.
   Control 2 was found by this method — an earlier version of this port left one
   site raw and the table caught it.
3. Putting fix(cli): redact malformed URLs and request build errors #164's original redactURL / sanitizeProbeError back into this
   tree — i.e. merging fix(cli): redact malformed URLs and request build errors #164 and not rewiring doctor — fails exactly one
   test, TestDoctorSchemelessServerURLDoesNotLeakCredentials, and its built
   binary leaks 2 sentinel occurrences on shape 7. Before that test was added,
   the same mutation failed zero tests. So the merge on its own was
   unprotected, and the one new test is what closes that.

Two pre-existing assertions were relaxed — named, because one is not mine

The gate withholds a value it cannot prove is a transport URL, where #164's
local helper echoed a partially cleaned one. Two assertions demanded the echo
shape specifically, so they were widened to accept redact or withhold:

location	test	author of the assertion as written
cmds_doctor_test.go:418	TestRedactURLStripsUserinfo	waterbro-8 (#131)
cmds_doctor_test.go:456	TestDoctorMalformedServerURLDoesNotLeakCredentials	PeterGuy326 (#164)

Both keep their original first clause — the secret must not appear, and a
cleaned URL must still carry REDACTED@<host>. Only the form of an
acceptable answer widened. Concretely at :418, strings.Contains(got, secret)
still fails the test, and the REDACTED@mem.internal:8787 check two lines above
it is untouched.

No assertion was deleted, and the count went up, not down: #164's 15 doctor
tests are all present on this head (0 dropped), plus TestRedactURLStripsUserinfo
and the new TestDoctorSchemelessServerURLDoesNotLeakCredentials = 16. The
way to check this claim is mutation control 3 above: if the widening had quietly
weakened coverage, restoring #164's scrubber would have gone green, and instead
it fails a test.

Anyone who considers the withhold form unacceptable for :456 should say so on
#164's thread rather than assume I settled it unilaterally — I changed an
assertion another author wrote to pass my gate.

What I did not verify

• The leak table was rebuilt and re-run against c9e0e63 after the docs commit;
  that commit is prose and comments only, and the result is unchanged (0 on all
  seven shapes). Earlier heads d30f1ff and b229537 each had 15/15 CI at the
  time they were written.
• There is no prior CI baseline for the doctor half to compare against.
  fix(cli): redact malformed URLs and request build errors #164's head fffcd4c has 0 check-runs and no workflow run has ever been
  recorded for it, so doctor has never been green in CI anywhere — including
  here, where it is now inside the Go job for the first time. That is a gain,
  not a regression, but it does mean no one has seen this code pass CI before.
• Nothing was exercised on Windows or macOS. All results are Linux x64.
• memd was not driven end-to-end to its fatal redis path. Reaching it
  requires a reachable PostgreSQL (the queue client is constructed after db Open), and the only local server on 5432 is an unrelated instance whose
  credentials I do not have — my attempt failed at db open: ... failed SASL auth before touching the queue. That egress is therefore evidenced by
  executing the gate on the asynq error string measured from the real library
  in this tree, not by a live memd run.
• No claim is made about #112's exit-code contract, R2/R3 acceptance, or
  feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path #131/fix(cli): redact malformed URLs and request build errors #164 merge readiness.

Non-goals

• No change to query-string credential handling (see Known gap).
• No change to #164's or #131's branches; nothing here is pushed to anybody
  else's ref. fix(cli): redact malformed URLs and request build errors #164 is pulled into this branch by merge, which is why fix(cli): redact malformed URLs and request build errors #164 can
  be closed without losing its work.
• No re-adjudication of R3. Doctor's --timeout behaviour, its check set and
  its JSON schema are fix(cli): redact malformed URLs and request build errors #164's, unchanged except where they called the old local
  helpers.

This PR stays Refs on #112 rather than Closes, because merging it would not
by itself settle #112's acceptance — that needs a reviewer's decision, not just
a diff landing.

Review strength of this PR, stated plainly: 15/15 checks pass on
c9e0e63, and I am the author of b229537, d30f1ff and c9e0e63 — automated checks and
my own sign-off are not a review. What this PR is missing is exactly one
independent human review; nothing else blocks it (mergeable=true, no conflict
with main, which is an ancestor of this head). It needs one human approval,
and I am not that reviewer — on the doctor half especially, since I edited an
assertion PeterGuy326 wrote.

[waterbro-8]

Scope widened: this branch now carries mem doctor too

The head moved twice since this PR was last read:
b229537 → d30f1ff → c9e0e63. The body has been updated to match; this
comment is the notification, because body edits are silent.

Why

#131 was closed as superseded by #164, and mem doctor exists only on #164.
So #164 could not be closed until its doctor came here — otherwise absorbing one
credential fix would have deleted a command. Rather than re-author it, this
branch merges #164, which keeps fffcd4c (author PeterGuy326) and
46499b2 (#131's, author waterbro-8) as ancestors with their authorship
intact. d30f1ff then replaces doctor's two local string helpers with the shared
gate:

func redactURL(raw string) string         { return redact.URL(raw, redact.APIURLs) }
func sanitizeProbeError(err error) string { return redact.TransportError(err, redact.APIURLs) }

The title went fix(client-memd) → fix(cli-memd) to cover the CLI half.
(pr-policy.yml:34 allows [a-z0-9._/-] in the scope, so a comma-form
fix(cli,memd) would have failed the gate — checked before renaming.)

Doctor measured end-to-end, on built binaries

Seven malformed credential URL shapes × mem doctor --server <shape> × two
output formats, counting a sentinel password in everything the process wrote:

shape	#164 head fffcd4c	this head
well-formed http://admin:PW@127.0.0.1:1	0	0
http://admin:PW@127.0.0.1:%zz	0	0
space in host	0	0
space in password	0	0
http://admin:PW@%	0	0
non-numeric port	0	0
admin:PW@mem.internal:8787 (no scheme)	6	0

That table nearly misled me and I want the failure mode on record. Run against
b229537 — this PR's previous head — it returned all zeros, and the only
reason it looked wrong is that I checked: that build has no doctor subcommand
and answers Error: unknown command "doctor" for "mem". The guard only counted
unknown flag, which is a different cobra error. It counts unknown command
now, and every row above was taken with both counters at 0.

Mutation control. Restoring #164's original redactURL /
sanitizeProbeError into this tree fails exactly one test —
TestDoctorSchemelessServerURLDoesNotLeakCredentials — and its binary leaks 2 on
shape 7. Before that test was added the same mutation failed zero tests, so
the merge by itself was unprotected. Full detail, including the two assertions I
widened (one of them authored by PeterGuy326), is on #164.

One extra commit, and why it is here

c9e0e63 is documentation-only, and it fixes a claim I wrote. CHANGELOG.md,
docs/DEPLOYMENT.md:194 and a comment in cmds_doctor.go all asserted the
doctor report contains no secret value. Measured on this head:

mem doctor --server "http://mem.internal:8787?password=s3ntinel…"
  → 3 sentinel occurrences per format, text and json alike

A credential in the query string parses as a clean URL with User == nil, so
the gate has nothing to act on. This PR has always declared query-string handling
a non-goal — that stands, and it is pinned by
TestTextKnownGapQueryParameterCredentialsAreEchoed. What was wrong is the prose
promising a property the code never had. The three statements now say what is
guaranteed and name the residual. The docs under-promise rather than
over-promise; no new behavior, so I did not add a CLI-level test for a gap the
gate-level characterization test already pins.

A review point of mine was wrong — retracted

I had carried probeVersion → apiclient.New(server, "") (empty token) as an
open defect from #131. It is not one: /v1/version is registered on the
public router at internal/api/api.go:211, outside the
r.Group(... r.Use(s.authMiddleware) ...) block at :220, and apiclient.New's
doc names it as a valid unauthenticated call. All three client sites in
cmds_doctor.go are correct. Retracted on #131 as well.

Current state

head	c9e0e63, tree 9fa9c14de074b4ddb5320d4f83803f1657918b34 — equal to the remote head's tree
go test -count=1 ./...	32 packages ok, 0 FAIL
gofmt -l . / go vet ./...	clean / pass
doctor tests	16 (14 from #131, +1 from #164, +1 new) — none dropped
origin/main (7a194f1) ancestor of HEAD	yes — no conflict, nothing silently reverted
CI	15/15 success on c9e0e63 at run_attempt 1, including Go and PostgreSQL integration; also 15/15 on d30f1ff
mergeable_state	blocked

What this PR needs is one independent human approval, and I am not it. I
authored b229537, d30f1ff and c9e0e63; 15/15 checks and my own sign-off are
not a review. The doctor half in particular should not be approved by me, since
I changed an assertion another author wrote in order to make the gate pass.

Not a merge authorization and not acceptance of #112, #131 or #164.

[PeterGuy326]

Blocking security review finding:\n\n leaves query strings untouched after parsing a URL, and the PR's characterization test explicitly accepts secrets in and . That means a credential-bearing server/DSN URL can still be emitted by the doctor/error surfaces. The body documents this as a known gap, but this PR cannot be approved as a complete credential egress gate while that path is intentionally echoed.\n\nPlease either extend the gate to withhold/redact credential-bearing query/fragment values (with regression tests), or split this into an explicitly narrow, non-security-complete change with a separately tracked blocking follow-up. Do not treat the current green checks as proof of no credential leakage.

[PeterGuy326]

Blocking security review finding:

server/internal/redact/redact.go leaves query strings untouched after parsing a URL, and the PR's characterization test explicitly accepts secrets in redis://.../?password=... and postgres://...?password=.... That means a credential-bearing server/DSN URL can still be emitted by the doctor/error surfaces. The body documents this as a known gap, but this PR cannot be approved as a complete credential egress gate while that path is intentionally echoed.

Please either extend the gate to withhold/redact credential-bearing query/fragment values (with regression tests), or split this into an explicitly narrow, non-security-complete change with a separately tracked blocking follow-up. Do not treat the current green checks as proof of no credential leakage.

[wadrzl]

Local Verification Summary for Issue #112

Branch tested: pr-165 at head c9e0e635abf20ce221c3cdb829599cfab37ddca3
Base: main (with merged #164 doctor implementation)

Issue #112 Requirements Coverage

Requirement	Status	Evidence
REQ-001: mem doctor read-only diagnostic command	PASS	Command exists with 4 fixed checks: server_reachability, credential, workspace, version_skew
REQ-001: --format json with versioned schema	PASS	mem.doctor/v1 schema with schema_version: 1
REQ-001: SPEC §7.1 exit codes (0/2/3/4/5)	PASS	Exit code 5 for unreachable server, 3 for auth failure
REQ-002: First-run guidance names deploy/compose path	PASS	All hints reference deploy/compose, see docs/DEPLOYMENT.md
REQ-003: Diagnosis only, zero writes	PASS	Write-guard transport test proves no non-GET requests

Acceptance Criteria Coverage

AC	Status	Evidence
AC-001: Fixture per finding class	PASS	14 doctor tests covering all finding classes
AC-002: No write requests	PASS	TestDoctorPerformsNoWriteRequests - stub transport fails on non-GET
AC-002: Never prints secrets	PASS	TestDoctorSchemelessServerURLDoesNotLeakCredentials - schemeless URL with credentials leaks 0 occurrences
AC-003: Golden file validation	PASS	TestDoctorJSONMatchesCheckedInSchema validates against testdata/doctor_healthy.golden.json

Test Results

Test Suite	Result
go test -run Doctor ./cmd/mem/...	14 tests PASS
go test ./cmd/mem/...	PASS (0.680s)
go test ./internal/redact/...	PASS
go test ./internal/apiclient/...	PASS (0.398s)
go test ./... (full server)	All packages PASS
gofmt -l .	PASS (no formatting issues)
go vet ./...	PASS

Manual Verification

Text output (unreachable server):

mem doctor (mem.doctor v1)
server: http://127.0.0.1:1
server_reachability  fail     cannot reach the configured server: ...
                              hint: start the documented container path: deploy/compose, see docs/DEPLOYMENT.md
credential           fail     no token configured
                              hint: run `mem auth login` first; ...
workspace            skipped  not evaluated: server_reachability is failing
version_skew         skipped  not evaluated: server_reachability is failing
Exit code: 5

JSON output: Correctly structured with contract: "mem.doctor", schema_version: 1, fixed check order, and proper exit codes.

Credential redaction test:

• Input: admin:password123@mem.internal:8787 (schemeless URL)
• Result: 0 occurrences of "password123" in output - credential properly withheld

Key Implementation Details

1. Shared redaction gate (internal/redact): One policy used by CLI, apiclient, and memd
2. Withhold vs redact: URLs that cannot be proven safe are withheld entirely, not partially trimmed
3. Exit codes follow SPEC §7.1: 0 ok, 2 not_found, 3 auth, 4 plan/quota, 5 provider/timeout
4. Skipped checks: Dependent probes report "skipped" with blocker name instead of guessing

Schema and Golden File

• Schema: docs/schemas/mem-doctor.v1.schema.json (JSON Schema Draft 2020-12)
• Golden file: server/cmd/mem/testdata/doctor_healthy.golden.json
• Both validate correctly against the implementation

Notes

• PR fix(cli-memd): withhold URLs whose credentials cannot be attributed #165 merges the doctor implementation from closed PR fix(cli): redact malformed URLs and request build errors #164
• The credential URL gate fix addresses the R3 adjudication's security concern
• Known gap (stated in PR): query-parameter credentials are not redacted (out of scope, pinned by characterization test)

[waterbro-8]

Fixed on e0b7972b4fe5 (one commit, 6 files, +79/-44). The finding is accurate, and one executed number makes it worse than the write-up suggests.

Why this is not a documentable residual. pgx v5's pgxpool.ParseConfig honours ?password= as the real database password; asynq's ParseRedisURI ignores it. I ran both drivers against the shapes in question:

shape	driver result
postgres://mem@db.internal:5432/mem?sslmode=require&password=<sentinel>	pgx ACCEPT, Config.ConnConfig.Password = <sentinel>
redis://queue.internal:6379/0?password=<sentinel>	asynq ACCEPT, Password = ""

So the one shape the userinfo gate lets through is exactly the shape where the value is a live credential. On the binary built from the previous head, memd's own startup line read:

"db":"postgres://REDACTED@db.internal:5432/mem?password=<sentinel>&sslmode=require"

The userinfo half of the gate fired correctly on the same string. mem doctor --server 'http://mem.internal:8787/?password=<sentinel>' echoed the sentinel 3 times in each of the text and JSON reports, which is the number this PR's own body quotes - so the gap was correctly described, just not as something a release can carry a "no secret in this document" claim next to.

What the gate does now. rewrite() blanks every query value to REDACTED and keeps only the parameter names, and withholds the URL whole when a fragment is present (a fragment has no name worth keeping). Deliberately no key-name blacklist: ?secret=/?token=/?apikey= versus a driver-specific spelling is a guess, and a guess here is fail-open. This is the same posture the package already takes for a URL it cannot prove is a credential-free transport URL - withhold rather than partially trim. The re-parse round-trip still runs on the blanked form, so String() re-encoding cannot reintroduce a value.

Text()'s doc paragraph, UserMarker's doc, the top-of-file caveat in cmds_doctor.go, two CHANGELOG.md entries and the docs/DEPLOYMENT.md paragraph that told readers not to trust a doctor report without reading it first are all updated to describe the behaviour rather than the gap.

Evidence, with both sides anchored to a tree sha. The "before" numbers come from a checkout whose git write-tree equals the head commit's tree 9fa9c14de074b4ddb5320d4f83803f1657918b34; the "after" numbers come from the fixed tree 5e3bce7a303f47898aced6f925230620a34f9076, which is byte-identical to the tree this commit now carries on the remote.

• go test ./... from server/: 32 packages ok, 0 fail. gofmt -l empty, go vet clean.
• TestQueryAndFragmentCredentialsAreWithheld replaces the characterization test that pinned the leak. Mutation control: deleting the new block turns exactly 2 test functions / 7 assertions red; restoring it goes green again - the test is load-bearing, which the one it replaced could not have been.
• Same binary, same input, before vs after: mem doctor 3 sentinel occurrences -> 0 in both formats; memd startup log 1 -> 0.
• rc=5 on an unreachable --server is unchanged, so feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path #112's R2 exit-code question is untouched by this commit and still open there.

Residual I am not claiming away. Parameter names still egress, so a secret smuggled into a name would still show. No driver I can find treats a name position as a credential, which is why I took the trade rather than withholding every URL that has a query string - ?sslmode=... and friends are what make the startup line diagnosable.

I am the author, so I have not voted on this and this comment carries no approval. Both CHANGES_REQUESTED still stand against the old head - @PeterGuy326, a re-review on e0b7972b4fe5 is what this needs. All 15 check-runs on this exact head completed success, attempt 1.

[PeterGuy326]

Review result

APPROVE for the current head e0b7972b4fe5951a754e1ca8329e2edcbfba2158.

The follow-up now blanks query values and withholds fragment-bearing URLs. I ran go test ./internal/redact ./cmd/mem locally: both packages passed. The current head has green Go, PostgreSQL integration, HTTP/CLI/MCP, Web, Worker, wrapper-compatibility, CodeQL, dependency-review, and policy checks. This approval supersedes the earlier changes-requested review at the old head.