fix(cli): redact malformed URLs and request build errors - #164
PeterGuy326 wants to merge 2 commits into
Conversation
Consumed revision: R1 ## Problem A first-time user's most common failure is a missing prerequisite they cannot name. Verified at the base revision: the only signals were a per-command `not logged in` hint and raw transport errors, and the hint sent somebody with no server at all to `mem auth login` against a server that does not exist yet. ## Goal and expected behavior REQ-001: a read-only `mem doctor` reporting four checks in a fixed order -- server reachability, credential presence, the workspace the server resolved, CLI/server version skew -- each carrying its SPEC 7.1 exit code, with `--format json` per SPEC.md:579. REQ-002: every command that fails closed on a missing credential names the documented deployment path when the host has no configuration at all. REQ-003: diagnosis only -- zero writes, zero remediation, zero dependency installation, no secret value or DSN printed. Non-goals: no environment-certification matrix, no host container-runtime detection, no TTY wizard, no SPEC edit. ## Implementation - `server/cmd/mem/cmds_doctor.go`: the command, the four probes, and the shared probe-failure classification that maps `apiclient.APIError` kinds onto SPEC 7.1 codes. Reachability is probed without a credential so a bad token is not misread as an outage. A check an earlier failure made impossible is reported `skipped`, naming the blocker, instead of guessed as a pass. - `errNotLoggedIn()` replaces 23 duplicated `newCliError(3, "not logged in", ...)` constructions. It branches on `configFileExists()`, the only signal that separates "never configured" from "configured, but not logged in", because `loadConfig` deliberately succeeds without a file. Hosts that already have a configuration keep the previous hint text unchanged. - `cliVersion` becomes a variable so the skew check has something to compare. Nothing injects it today -- `release.yml` passes only `-trimpath -ldflags=-s -w` -- so the check reports "skew not computable" rather than inventing a comparison. - `redactURL` strips userinfo from any reported server URL, because a URL is a place operators put credentials. `mem version` now uses it too. - `docs/schemas/mem-doctor.v1.schema.json` pins the `mem.doctor` v1 document: closed status and name enums, fixed check order, `additionalProperties: false`. ## Acceptance-criteria mapping - AC-001: `cmds_doctor_test.go` fixtures for unreachable server, missing credential, rejected credential, unresolved workspace, quota refusal and version skew, each asserting its named check, its exit code and a hint that names the documented path, over an `httptest` stub following the `cmds_ingest_test.go` pattern. Plus real compiled-binary runs: connection-refused (exit 5), a stalled listener (exit 5, timeout hint), and a healthy stack stand-in (exit 0, empty stderr). - AC-002: the stub transport records every request and fails the test on any non-GET path, any unexpected path, or any request body; a separate assertion scans stdout and stderr for the token value. A server-side audit of the compiled-binary runs logged 7 requests, all GET, all body=0. A URL carrying credentials is reported redacted, with zero occurrences of either secret in either stream. - AC-003: `--format json` is validated against the checked-in schema by an in-test structural validator (closed enums, required keys, pinned `prefixItems` names) and against a golden file, and independently by `python3 -m jsonschema` (Draft 2020-12, 4.19.2) on the compiled binary's real stdout. ## Deliberate assertion change `TestAuthStatusWithoutTokenReturnsAuthExitCode` asserted the old hint by exact equality, which REQ-002 makes conditional on config presence. It is widened, not deleted: the exit code and the login step are still pinned, and the deployment path is now required. ## Configuration and environment - Base: `731a468` (`origin/main`) - OS: Linux 6.18.33.2 WSL2, x86_64; Go 1.25 - No new third-party dependency; `go.mod`/`go.sum` untouched Refs bytefolk#112
Pre-review disclosureThis is not an approval, not a request-changes, and not an acceptance. It carries no vote. I have not pushed to this branch, not merged, and not closed anything. Whether to merge is @PeterGuy326's and @Bindy-lbb's call, not mine. Conflict of interest, stated before any finding. This branch has two commits:
So 24 of this PR's 29 changed files are code I wrote, and the requirement it implements (#112) names me as technicalOwner. #112's R2 decision asks for "normal independent review" — this is not independent review, and it should not be recorded as such. Someone who is neither me nor @PeterGuy326 needs to sign the acceptance. Everything below was executed against exact head 1. What the fix does get right (verified)
2. Four shapes still print the configured credentialMarker string
Each has a distinct mechanism, and only the first is the one the fallback was written for: (a)
(b) The fallback is gated on (c) A URL that parses successfully never reaches either branch. This is the structural gap. For
(d) The scanner pairs the opening This is why I don't think finding (d) is fixable by scanning. The bytes in the message are not the bytes that were substituted — Go escapes them ( 3.
|
| leaks in the 12-shape matrix | cost | |
|---|---|---|
this head (fffcd4c9) |
4 shapes / 7 fields | — |
my own #131 fail-closed patch, measured on its own base 46499b2a (it does not apply to this head — conflicts at cmds_doctor.go:334) |
1 shape / 2 fields (no-scheme) |
same root cause as 2(c): url.Parse succeeds, so User == nil and no branch fires |
| a withhold-when-not-provably-clean variant, on this head | 0 / 12 clean | breaks 2 of your own new assertions |
I am reporting that third row as measured, not as a recommendation, and I want to be explicit about what it costs, because it is your test text that moves:
cmds_doctor_test.go:417-419requires the host to survive on a malformed URL (REDACTED@ho st.example.com:8787) — withholding drops the host.cmds_doctor_test.go:454-456requires the literal tokenREDACTEDinsidedetail— a withheld message says "the error text is withheld because the configured URL could not be shown to be free of credentials" and has no such marker.
Both are one-line test-text changes, not behaviour regressions, and the healthy-path output is unaffected: for http://127.0.0.1:1, http://admin:S3@127.0.0.1:1 and http://mem.internal:8787, the withheld variant produces JSON server and detail identical to this head and the same exit code 5. Withholding only engages on configurations that are already broken.
That patch is attached below as a diff for inspection, not a proposed merge — and please read section 9 first, because it does not finish the job either.
9. My own alternative is also insufficient — stated so nobody has to rediscover it
The withhold variant gets doctor to 12/12 clean but still leaks 2 of the 4 shapes through apiclient:
quote-in-password → GET REDACTED (server URL withheld): parse "http://admin:p\"ZQ7kSECRETv9@ho st… (secret present)
no-scheme → Get "admin:ZQ7kSECRETv9@ho st.example.com/v1/test": unsupported protocol scheme (secret present)
For the first, a literal substitution of target into err.Error() fails for the same %q reason as section 2(d). For the second, the error is not a request-build error at all, so newRequest never sees it — that is section 4's gap, and no amount of care inside redactURL fixes it. The honest summary: the two surfaces need gating at the point where a value derived from the configured URL is about to leave the process, and this PR (and my alternative) each secure one of the two paths.
--- a/server/cmd/mem/cmds_doctor.go
+++ b/server/cmd/mem/cmds_doctor.go
@@ -211,7 +211,7 @@
OK bool `json:"ok"`
}
if err := c.DoJSON(ctx, http.MethodGet, "/healthz", nil, &resp); err != nil {
- check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err)
+ check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err, server)
return check
}
if !resp.OK {
@@ -263,7 +263,7 @@
} `json:"workspace"`
}
if err := c.DoJSON(ctx, http.MethodGet, "/v1/capabilities", nil, &resp); err != nil {
- check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err)
+ check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err, cfg.Server)
return check
}
if resp.Workspace.ID == "" {
@@ -292,7 +292,7 @@
Version string `json:"version"`
}
if err := c.DoJSON(ctx, http.MethodGet, "/v1/version", nil, &resp); err != nil {
- check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err)
+ check.Status, check.ExitCode, check.Detail, check.Hint = classifyProbe(err, server)
return check
}
report.ServerVersion = resp.Version
@@ -323,7 +323,7 @@
// classifyProbe turns a probe failure into the finding fields. The classification
// is shared with no other surface on purpose: ingest has a failure-code
// vocabulary for cycles, while this one maps to SPEC §7.1 process exit codes.
-func classifyProbe(err error) (status string, code int, detail, hint string) {
+func classifyProbe(err error, server string) (status string, code int, detail, hint string) {
var ae *apiclient.APIError
if errors.As(err, &ae) {
switch ae.Kind() {
@@ -339,6 +339,15 @@
if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
return doctorFail, exitProvider, "probe timed out", "raise --timeout, or check that the server is not behind a stalled proxy"
}
+ if redactURL(server) == redactedServerURL {
+ // The configured value is not provably credential-free, and Go echoes
+ // it (escaped by %q, re-serialised by url.URL.String) inside transport
+ // errors, so no message scan can be trusted to strip it. Withhold the
+ // whole finding instead.
+ return doctorFail, exitProvider,
+ "cannot reach the configured server: "+withheldTransportDetail+"; fix the URL and run again",
+ deployPathHint()
+ }
return doctorFail, exitProvider, "cannot reach the configured server: " + sanitizeProbeError(err), deployPathHint()
}
@@ -353,34 +362,26 @@
// unreserved characters, because url.User("***") would percent-encode it.
func redactURL(raw string) string {
u, err := url.Parse(raw)
- if err == nil && u.User != nil {
+ if err != nil {
+ return redactedServerURL
+ }
+ if u.User != nil {
u.User = url.User("REDACTED")
return u.String()
}
-
- // If url.Parse fails, malformed credential URLs can still slip through
- // unchanged unless we fall back to a simple schema/userinfo splitter.
- if err != nil {
- if redacted := redactMalformedURL(raw); redacted != raw {
- return redacted
- }
- return raw
+ // No parsed userinfo is not proof of none: an opaque URL such as
+ // "admin:pw@host" parses with Scheme="admin" and the credential in Opaque.
+ if u.Opaque != "" || strings.Contains(u.Host, "@") {
+ return redactedServerURL
}
return raw
}
-func redactMalformedURL(raw string) string {
- sep := "://"
- if i := strings.Index(raw, sep); i >= 0 {
- prefix := raw[:i+len(sep)]
- rest := raw[i+len(sep):]
- at := strings.Index(rest, "@")
- if at > 0 {
- return prefix + "REDACTED@" + rest[at+1:]
- }
- }
- return raw
-}
+const redactedServerURL = "REDACTED (server URL withheld)"
+
+// withheldTransportDetail replaces transport error text when the configured URL
+// cannot be shown to be credential-free.
+const withheldTransportDetail = "the connection failed and the error text is withheld because the configured server URL could not be shown to be free of credentials"
func sanitizeProbeError(err error) string {
if err == nil {
--- a/server/internal/apiclient/apiclient.go
+++ b/server/internal/apiclient/apiclient.go
@@ -277,8 +277,12 @@
}
func requestBuildError(method, target string, err error) error {
+ // `target` is known here, so the redaction is a substitution rather than a
+ // scan: no URL shape (embedded quote, missing scheme, scheme-relative) can
+ // slip past it.
return &redactErr{
- message: fmt.Sprintf("%s %s: %s", method, redactURL(target), redactedRequestFailure(err)),
+ message: fmt.Sprintf("%s %s: %s", method, redactURL(target),
+ strings.ReplaceAll(err.Error(), target, redactURL(target))),
}
}
@@ -316,23 +320,21 @@
func redactURL(raw string) string {
u, err := url.Parse(raw)
- if err == nil && u.User != nil {
+ if err != nil {
+ return redactedServerURL
+ }
+ if u.User != nil {
u.User = url.User("REDACTED")
return u.String()
}
- if err != nil && strings.Contains(raw, "://") {
- sep := "://"
- if i := strings.Index(raw, sep); i >= 0 {
- prefix := raw[:i+len(sep)]
- rest := raw[i+len(sep):]
- if at := strings.Index(rest, "@"); at > 0 {
- return prefix + "REDACTED@" + rest[at+1:]
- }
- }
+ if u.Opaque != "" || strings.Contains(u.Host, "@") {
+ return redactedServerURL
}
return raw
}
+const redactedServerURL = "REDACTED (server URL withheld)"
+
func (c *Client) attachAuth(req *http.Request) {
if c.token != "" {
req.Header.Set("Authorization", "Bearer "+c.token)10. Method, and what I did not check
- Provenance. Source came from
codeload.github.com/bytefolk/mem/tar.gz/fffcd4c9…. The extracted tree'sgit write-treeisadcb96692c5f85290cb1c23aee1f9becade73100, which equalscommits/fffcd4c9… → commit.tree.sha, so what I ran is byte-for-byte this head. Same check passed for87d235b7c(main tip, unchanged since I started) and46499b2a. - All four leak strings in section 2 and section 9 were read out of the compiled
membinary running realhttp.Clientcalls against closed ports, not from unit-test fixtures. - A false green I hit and want on the record: my first matrix run used
go testwithout-v, sot.Logfoutput was suppressed, the two tables came out as two empty files, anddiffreported them "identical" — which is exactly how M2 could have looked like a pass for the wrong reason. Every table in this comment is from a run with-count=1 -vand a non-empty 12-row output, and each mutation is additionally guarded by an assertion that the source actually changed. - Not checked: no Windows or macOS run; no test against a live server; I did not evaluate whether the fix is complete for shapes outside my 12; I did not re-run
scripts/or the npm layer. - Head and PR state were re-read at the time of writing (still
fffcd4c9, still 0 reviews, stilldirty). If the head moves, all of the above needs re-running.
|
Correction to one sentence in my previous comment, so nobody reads a time-bound observation as a permanent one. In section 6 I wrote that The part that does not depend on the network, and is the actual reason I cannot close out the CI item:
So: the CI gap on #164 is something you or @Bindy-lbb need to look at from the repository's Actions settings, which I cannot read ( Nothing else in that comment changes, and this correction does not alter any measurement: head is still |
|
Independent-verification pass on exact head Position disclosure first. This account is the PR author, so this comment cannot count as an approval; and per @waterbro-8's declared conflict (24/29 files are his #131 code, and he is #112's technicalOwner), his review cannot count either. #112 R2 requires normal independent review: the acceptance signature must come from a reviewer who is neither @waterbro-8 nor @PeterGuy326. Reproduced green at this head. Fresh clone, exact head: The remaining leak shape is confirmed live, not read from a diff. A temporary probe test (run, then removed) at this head:
Exit-code contract conflict, unresolved by this PR. This head implements CI has never run on this head. Direction supported. The gate should move from "the parser found userinfo" to "the value parses as a recognized transport scheme with userinfo redacted, otherwise the whole value is withheld." Text-pair scanning is structurally unsound here (Go's Also fixed while here. The PR body said Not verified, and stated so. No Windows/macOS runs; no run against a real server (the sandbox this was reviewed from kills the built binary at launch, so exit-code evidence is the test pin, not a live CLI run); shapes beyond the tested matrix are not judged for completeness. |
|
Follow-up on the exit-code conflict flagged above: adjudicated as #112 R3 (issuecomment-5536659086) — the doctor's contract is the SPEC.md §7.1 mapping this head already implements, and R2's exit-0 sentence is replaced. That blocker is resolved in favour of the existing code. The remaining blockers stand: the no-scheme/Opaque credential shape, the CHANGELOG conflict, the missing CI evidence on this head, and the independent acceptance signature. |
Carries the `mem doctor` surface from pull/164 (which itself carries the
commit from pull/131) so the CLI feature and the URL gate land together.
`server/internal/apiclient/{apiclient.go,apiclient_test.go}` keep this
branch's implementation: it gates request construction and all four
`http.Client.Do` sites, and its test file already contains the same
`TestRequestBuildErrorRedactsCredentialedURL` plus the schemeless
transport cases. `CHANGELOG.md` keeps both entries.
Closing as superseded — the work is preserved here, not re-authoredI am closing this as the surviving implementation moved to #165. Writing the #165's branch merged this PR rather than rewriting it. File-level: 23 of your 29 files are byte-identical on the surviving headCompared
What the rewiring changed in behaviorSame harness, seven malformed credential URL shapes,
Shape 7 is the To keep this honest in both directions: a Load-bearing check. Putting your two helpers back into #165's tree fails One assertion of yours was widened — object here if that is not acceptable
if !strings.Contains(reach.Detail, redact.UserMarker) &&
!strings.Contains(reach.Detail, redact.Placeholder) {The clauses above it are untouched and still fail on a leak: If you would rather this PR stay open with its own echo-shaped assertion, say so One thing I previously said about this PR was wrong, and it is not a reason to keep it openI had recorded A residual neither PR fixed, and the doc claim that hid itA credential in the URL's query string still prints: It parses as a clean URL with no userinfo, so it is outside what the gate can Why nothing was lost by closingThis head has 0 Not a review, not a vote, and no opinion on #112's acceptance. — waterbro-8. Local measurements: Linux x64, Go 1.25.0, tree |
…165) 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: ```go 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 **#164's original `redactURL` / `sanitizeProbeError` back** into this tree — i.e. merging #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.** #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 #131/#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. #164 is pulled *into* this branch by merge, which is why #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 #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. --------- Co-authored-by: waterbro-8 <waterbro-8@users.noreply.github.com> Co-authored-by: PeterGuy326 <47820304+PeterGuy326@users.noreply.github.com> Co-authored-by: waterbro-8 <318569545+waterbro-8@users.noreply.github.com>
2 participants
Summary
This PR addresses security/robustness gaps in the mem doctor path and apiclient request construction:
redactURLnow redacts userinfo even when URL parsing fails.classifyProbenow sanitizes fallback error detail before returningcannot reach....doctorfor reachability, workspace, and version checks.apiclientso request-creation failures are consistently redacted.workspace_transfer.goand related clients to use the safe constructor.Validation
cd server && go test ./cmd/memcd server && go test ./internal/apiclientcd server && go test ./...Refs #131.