From 46499b2adacfc66999cddaf7e2e9852292f12ece Mon Sep 17 00:00:00 2001 From: waterbro-8 Date: Mon, 31 Aug 2026 13:02:29 +0800 Subject: [PATCH 1/6] feat(cli): add mem doctor and first-run guidance 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 #112 --- CHANGELOG.md | 20 + docs/DEPLOYMENT.md | 31 + docs/schemas/mem-doctor.v1.schema.json | 84 +++ server/cmd/mem/client.go | 19 + server/cmd/mem/cmds_auth.go | 4 +- server/cmd/mem/cmds_auth_test.go | 13 +- server/cmd/mem/cmds_context.go | 2 +- server/cmd/mem/cmds_doctor.go | 368 ++++++++++ server/cmd/mem/cmds_doctor_test.go | 670 ++++++++++++++++++ server/cmd/mem/cmds_face.go | 2 +- server/cmd/mem/cmds_file.go | 6 +- server/cmd/mem/cmds_file_annotations.go | 2 +- server/cmd/mem/cmds_folder.go | 2 +- server/cmd/mem/cmds_handoff.go | 4 +- server/cmd/mem/cmds_ingest.go | 2 +- server/cmd/mem/cmds_memory.go | 4 +- server/cmd/mem/cmds_model.go | 2 +- server/cmd/mem/cmds_profile.go | 2 +- server/cmd/mem/cmds_provider.go | 8 +- server/cmd/mem/cmds_related.go | 4 +- server/cmd/mem/cmds_remember.go | 2 +- server/cmd/mem/cmds_search.go | 2 +- server/cmd/mem/cmds_timeline.go | 2 +- server/cmd/mem/config.go | 13 + server/cmd/mem/main.go | 12 + .../mem/testdata/doctor_healthy.golden.json | 34 + 26 files changed, 1287 insertions(+), 27 deletions(-) create mode 100644 docs/schemas/mem-doctor.v1.schema.json create mode 100644 server/cmd/mem/cmds_doctor.go create mode 100644 server/cmd/mem/cmds_doctor_test.go create mode 100644 server/cmd/mem/testdata/doctor_healthy.golden.json diff --git a/CHANGELOG.md b/CHANGELOG.md index b55e0c4..09d9870 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ The project publishes 0.x prerelease versions; a stable release line is not yet ## [Unreleased] +### Added + +- `mem doctor` — a read-only diagnosis of why the CLI cannot talk to a working + server (`#112`). It reports four checks in a fixed order: reachability of the + configured server URL, whether a credential exists, the workspace the server + resolved for that credential, and CLI/server version skew. Each finding carries + the SPEC §7.1 exit code it contributes (`0` ok · `2` not_found · `3` auth · + `4` plan/quota · `5` provider/timeout), and a check that an earlier failure made + impossible is reported as `skipped` instead of guessed. It issues only `GET` + requests and never writes configuration, starts a container, or installs a + dependency; `--format json` emits the `mem.doctor` v1 document described by + `docs/schemas/mem-doctor.v1.schema.json`, with no secret value in it (the + configured URL is reported with credentials removed). See + `docs/DEPLOYMENT.md`. +- First-run guidance: a command that fails because no credential exists now says + so on a machine with no configuration at all by naming the documented + deployment path (`deploy/compose`, `docs/DEPLOYMENT.md`), instead of telling + somebody to log in against a server that is not running yet. Hosts that already + have a configuration keep the previous, shorter hint. + ### Fixed - The npm installer now verifies the selected Release binary against the diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 23ae246..63cf3cb 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -155,6 +155,37 @@ docker compose --env-file .env -f compose.yaml ps docker compose --env-file .env -f compose.yaml logs --tail=200 migrate memd worker web ``` +### Diagnose from the client side + +`mem doctor` answers the client half of the same question: why the CLI cannot +reach a working server. It issues only `GET` requests, and it never writes +configuration, starts or stops a container, or installs a dependency — a failed +diagnosis changes nothing on the machine. + +```bash +mem doctor +mem doctor --format json +``` + +It reports four checks in a fixed order and stops guessing after the first +failure: reachability of the configured server URL (`/healthz`, probed without a +credential so a bad token is not misread as an outage), whether a credential +exists, the workspace the server resolved for that credential +(`/v1/capabilities`), and CLI/server version skew (`/v1/version`). A check that +an earlier failure made impossible is reported as `skipped`, naming the blocking +check, rather than as an inferred pass. + +The process exits with the first failing check's SPEC §7.1 code — `0` ok · +`2` not_found · `3` auth · `4` plan/quota · `5` provider/timeout — so a wrapper +can branch on it. Version skew is advisory and contributes `0`; it is also not +computable in builds that do not inject a CLI version, which today includes +release builds, so the check reports that limit instead of claiming agreement. + +`--format json` emits the `mem.doctor` v1 document validated by +[`schemas/mem-doctor.v1.schema.json`](schemas/mem-doctor.v1.schema.json). It +contains no secret value: the configured URL is reported with any credentials +removed, and a token is described only by where it came from. + ### First account and login The default `MEM_REGISTRATION_MODE=first_user` atomically allows exactly one diff --git a/docs/schemas/mem-doctor.v1.schema.json b/docs/schemas/mem-doctor.v1.schema.json new file mode 100644 index 0000000..3f8dd30 --- /dev/null +++ b/docs/schemas/mem-doctor.v1.schema.json @@ -0,0 +1,84 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://getmem.dev/schemas/mem-doctor.v1.schema.json", + "title": "mem doctor report v1", + "description": "Read-only diagnosis emitted by `mem doctor --format json`. Mirrors doctorReport and doctorCheck in server/cmd/mem/cmds_doctor.go. Contains no credential, token or DSN value: the server URL is reported with userinfo removed.", + "type": "object", + "additionalProperties": false, + "required": [ + "contract", + "schema_version", + "server", + "cli_version", + "exit_code", + "checks" + ], + "properties": { + "contract": { + "const": "mem.doctor" + }, + "schema_version": { + "const": 1 + }, + "server": { + "type": "string", + "description": "Configured memd base URL, userinfo redacted.", + "minLength": 1 + }, + "cli_version": { + "type": "string", + "description": "Version this CLI build reports; \"dev\" when none was injected at build time." + }, + "server_version": { + "type": "string", + "description": "Version the server reported. Absent when the version probe did not run." + }, + "exit_code": { + "type": "integer", + "description": "SPEC 7.1 process exit code: first failing check's code, else 0.", + "enum": [0, 2, 3, 4, 5] + }, + "checks": { + "type": "array", + "minItems": 4, + "maxItems": 4, + "description": "Fixed ordered list, never a wizard: server_reachability, credential, workspace, version_skew.", + "prefixItems": [ + { "allOf": [ { "$ref": "#/$defs/check" }, { "properties": { "name": { "const": "server_reachability" } } } ] }, + { "allOf": [ { "$ref": "#/$defs/check" }, { "properties": { "name": { "const": "credential" } } } ] }, + { "allOf": [ { "$ref": "#/$defs/check" }, { "properties": { "name": { "const": "workspace" } } } ] }, + { "allOf": [ { "$ref": "#/$defs/check" }, { "properties": { "name": { "const": "version_skew" } } } ] } + ], + "items": { "$ref": "#/$defs/check" } + } + }, + "$defs": { + "check": { + "type": "object", + "additionalProperties": false, + "required": ["name", "status", "exit_code", "detail"], + "properties": { + "name": { + "enum": ["server_reachability", "credential", "workspace", "version_skew"] + }, + "status": { + "enum": ["ok", "warn", "fail", "skipped"], + "description": "\"skipped\" means an earlier failure made this check unrunnable; it is never an inferred pass." + }, + "exit_code": { + "type": "integer", + "description": "This finding's contribution to the process exit code. Advisory and skipped findings contribute 0.", + "enum": [0, 2, 3, 4, 5] + }, + "detail": { + "type": "string", + "minLength": 1 + }, + "hint": { + "type": "string", + "description": "Actionable next step. First-run hints name the documented container path." + } + } + } + } +} diff --git a/server/cmd/mem/client.go b/server/cmd/mem/client.go index cda2fd4..ee8625c 100644 --- a/server/cmd/mem/client.go +++ b/server/cmd/mem/client.go @@ -24,6 +24,25 @@ func newCliError(code int, msg, hint string) *cliError { return &cliError{code: code, msg: msg, hint: hint} } +// notLoggedInHint is the credential guidance that always applies. +const notLoggedInHint = "run `mem auth login` first" + +// firstRunDeployHint names the documented deployment path rather than a +// host-specific install recipe, so a machine that has never been configured is +// not sent off to build the bare-metal stack by hand. +const firstRunDeployHint = "no server configured yet — the documented path is deploy/compose, see docs/DEPLOYMENT.md" + +// errNotLoggedIn is the one fail-closed auth error for commands that need a +// credential. When no config file exists at all, the run is a first run: the +// hint additionally names the documented deployment path, because telling +// somebody to log in against a server that does not exist yet is not guidance. +func errNotLoggedIn() error { + if configFileExists() { + return newCliError(3, "not logged in", notLoggedInHint) + } + return newCliError(3, "not logged in", notLoggedInHint+"; "+firstRunDeployHint) +} + // fromAPIError maps an *apiclient.APIError to a *cliError with the SPEC §7.1 // exit code. Any other error is returned unchanged. func fromAPIError(err error) error { diff --git a/server/cmd/mem/cmds_auth.go b/server/cmd/mem/cmds_auth.go index 8631a17..5377d02 100644 --- a/server/cmd/mem/cmds_auth.go +++ b/server/cmd/mem/cmds_auth.go @@ -132,7 +132,7 @@ func newAuthStatusCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } var capabilities struct { @@ -241,7 +241,7 @@ func newTokenCreateCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } scopeList := splitCommas(scopes) body := map[string]any{ diff --git a/server/cmd/mem/cmds_auth_test.go b/server/cmd/mem/cmds_auth_test.go index 551cf49..46a3865 100644 --- a/server/cmd/mem/cmds_auth_test.go +++ b/server/cmd/mem/cmds_auth_test.go @@ -162,8 +162,17 @@ func TestAuthStatusWithoutTokenReturnsAuthExitCode(t *testing.T) { if !errors.As(err, &cliErr) { t.Fatalf("error type = %T, want *cliError", err) } - if cliErr.code != 3 || cliErr.hint != "run `mem auth login` first" { - t.Fatalf("cli error = %#v", cliErr) + // #112 REQ-002 changed this hint's text for a host with no config file at + // all, so the old exact-equality assertion is intentionally widened: the + // login step must stay, and the documented deployment path must now appear. + if cliErr.code != 3 { + t.Fatalf("cli error code = %d, want 3 (%#v)", cliErr.code, cliErr) + } + if !strings.HasPrefix(cliErr.hint, "run `mem auth login` first") { + t.Errorf("hint = %q, want it to keep the login step", cliErr.hint) + } + if !strings.Contains(cliErr.hint, "deploy/compose") || !strings.Contains(cliErr.hint, "docs/DEPLOYMENT.md") { + t.Errorf("hint = %q, want first-run guidance naming the documented path", cliErr.hint) } } diff --git a/server/cmd/mem/cmds_context.go b/server/cmd/mem/cmds_context.go index 45eaaa7..02cc2ce 100644 --- a/server/cmd/mem/cmds_context.go +++ b/server/cmd/mem/cmds_context.go @@ -82,7 +82,7 @@ Examples: return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } body := map[string]any{"query": strings.Join(args, " ")} if scope != "" { diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go new file mode 100644 index 0000000..f74cb82 --- /dev/null +++ b/server/cmd/mem/cmds_doctor.go @@ -0,0 +1,368 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "os" + "strings" + "time" + + "github.com/PeterGuy326/mem/server/internal/apiclient" + "github.com/spf13/cobra" +) + +// mem doctor is a read-only diagnosis surface (issue #112). It exists because a +// first-time user's most common failure is a missing prerequisite they cannot +// name: nothing listening at the configured URL, no credential, no workspace. +// Before this command the only signal was a per-command error. +// +// Contract, in the order the checks run: +// +// server_reachability GET /healthz with no credential +// credential is a token configured at all (no request) +// workspace GET /v1/capabilities +// version_skew GET /v1/version +// +// REQ-003 keeps this strictly diagnostic: every request is a GET, nothing is +// created, no dependency is installed, no Docker or compose command is issued, +// and no secret value or DSN is ever printed (the configured URL is reported +// with any userinfo removed). + +// doctorContract and doctorSchemaVersion follow the repo convention of naming a +// machine-readable surface and versioning it, mirroring docs/schemas. +const ( + doctorContract = "mem.doctor" + doctorSchemaVersion = 1 +) + +// Statuses are a closed set. "skipped" is explicit: a check that could not run +// because an earlier one failed says so, instead of reporting an OK it did not +// earn or a failure it did not observe. +const ( + doctorOK = "ok" + doctorWarn = "warn" + doctorFail = "fail" + doctorSkipped = "skipped" +) + +// exit codes, per SPEC §7.1: 0 ok · 2 not_found · 3 auth · 4 plan/quota · +// 5 provider/timeout. +const ( + exitOK = 0 + exitNotFound = 2 + exitAuth = 3 + exitPlanQuota = 4 + exitProvider = 5 +) + +type doctorCheck struct { + Name string `json:"name"` + Status string `json:"status"` + // ExitCode is this finding's contribution to the process exit status. + // Advisory findings contribute 0. + ExitCode int `json:"exit_code"` + Detail string `json:"detail"` + Hint string `json:"hint,omitempty"` +} + +type doctorReport struct { + Contract string `json:"contract"` + SchemaVersion int `json:"schema_version"` + Server string `json:"server"` + CLIVersion string `json:"cli_version"` + ServerVersion string `json:"server_version,omitempty"` + ExitCode int `json:"exit_code"` + Checks []doctorCheck `json:"checks"` +} + +func newDoctorCmd() *cobra.Command { + var timeout time.Duration + cmd := &cobra.Command{ + Use: "doctor", + Short: "Diagnose local configuration and server connectivity (read-only)", + Long: `Report why the CLI cannot talk to a working mem server. + +doctor issues only GET requests and writes nothing: no token, no file, no +container and no configuration. It checks, in order, reachability of the +configured server URL, whether a credential exists, which workspace the server +resolved for that credential, and CLI/server version skew. Each finding carries +the SPEC §7.1 exit code it contributes (0 ok · 2 not_found · 3 auth · +4 plan/quota · 5 provider/timeout); the process exits with the first failing +check's code. + +A check that an earlier failure made impossible is reported as "skipped" rather +than guessed. + +Example: + mem doctor + mem doctor --format json + mem doctor --server http://localhost:8787 --timeout 2s`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + format, err := rememberOutputFormat(cmd) + if err != nil { + return err + } + report, err := runDoctor(cmd, timeout) + if err != nil { + return err + } + if format == "json" { + enc := json.NewEncoder(cmd.OutOrStdout()) + enc.SetIndent("", " ") + if err := enc.Encode(report); err != nil { + return err + } + } else { + printDoctorReport(cmd, report) + } + if report.ExitCode != exitOK { + f := report.firstFailed() + return newCliError(report.ExitCode, "doctor: "+f.Name+" failed", f.Detail) + } + return nil + }, + } + cmd.Flags().DurationVar(&timeout, "timeout", 5*time.Second, "per-request budget for the read-only probes") + return cmd +} + +func (r doctorReport) firstFailed() doctorCheck { + for _, c := range r.Checks { + if c.Status == doctorFail { + return c + } + } + return doctorCheck{Name: "doctor", Detail: "a check failed"} +} + +func runDoctor(cmd *cobra.Command, timeout time.Duration) (doctorReport, error) { + if timeout <= 0 { + return doctorReport{}, newCliError(1, "--timeout must be positive", "") + } + cfg, err := resolveConfig("") + if err != nil { + return doctorReport{}, err + } + report := doctorReport{ + Contract: doctorContract, + SchemaVersion: doctorSchemaVersion, + Server: redactURL(cfg.Server), + CLIVersion: cliVersion, + } + ctx, cancel := context.WithTimeout(cmd.Context(), timeout) + defer cancel() + + reach := probeReachability(ctx, cfg.Server) + report.Checks = append(report.Checks, reach) + + cred := probeCredential(cfg) + report.Checks = append(report.Checks, cred) + + // The remaining checks need a live, authenticated connection. Reporting a + // fabricated result for them would be the exact failure mode this command + // exists to remove. + var ws, ver doctorCheck + switch { + case reach.Status == doctorFail: + ws, ver = skippedCheck("workspace", reach.Name), skippedCheck("version_skew", reach.Name) + case cred.Status == doctorFail: + ws, ver = skippedCheck("workspace", cred.Name), skippedCheck("version_skew", cred.Name) + default: + ws = probeWorkspace(ctx, cfg) + ver = probeVersion(ctx, cfg.Server, &report) + } + report.Checks = append(report.Checks, ws, ver) + + for _, c := range report.Checks { + if c.Status == doctorFail { + report.ExitCode = c.ExitCode + break + } + } + return report, nil +} + +// skippedCheck records a check that an earlier failure made impossible, and +// names the blocker so the text report stays actionable without the JSON. +func skippedCheck(name, blockedBy string) doctorCheck { + return doctorCheck{ + Name: name, + Status: doctorSkipped, + Detail: "not evaluated: " + blockedBy + " is failing", + } +} + +func probeReachability(ctx context.Context, server string) doctorCheck { + check := doctorCheck{Name: "server_reachability"} + // An unauthenticated probe: a 401 here would otherwise be read as "the + // server is down" by a user whose only problem is a bad token. + c := apiclient.New(server, "") + var resp struct { + 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) + return check + } + if !resp.OK { + check.Status = doctorFail + check.ExitCode = exitProvider + check.Detail = "healthz answered without ok:true" + check.Hint = deployPathHint() + return check + } + check.Status = doctorOK + check.Detail = "healthz ok at " + redactURL(server) + return check +} + +func probeCredential(cfg *cliConfig) doctorCheck { + check := doctorCheck{Name: "credential"} + if cfg.Token == "" { + check.Status = doctorFail + check.ExitCode = exitAuth + check.Detail = "no token configured" + check.Hint = notLoggedInHint + if !configFileExists() { + check.Hint += "; " + firstRunDeployHint + } + return check + } + // The value never leaves this function: only its origin is reported. + check.Status = doctorOK + check.Detail = "token present (from " + credentialSource() + ")" + return check +} + +// credentialSource names where the token came from without printing it. +func credentialSource() string { + if strings.TrimSpace(os.Getenv("MEM_TOKEN")) != "" { + return "$MEM_TOKEN" + } + return "config file" +} + +func probeWorkspace(ctx context.Context, cfg *cliConfig) doctorCheck { + check := doctorCheck{Name: "workspace"} + c := apiclient.New(cfg.Server, cfg.Token).WithWorkspace(cfg.Workspace) + var resp struct { + Workspace struct { + ID string `json:"id"` + Name string `json:"name"` + Role string `json:"role"` + } `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) + return check + } + if resp.Workspace.ID == "" { + check.Status = doctorFail + check.ExitCode = exitNotFound + check.Detail = "server resolved no workspace for this credential" + check.Hint = "select one with `mem auth login` or --workspace " + return check + } + check.Status = doctorOK + if cfg.Workspace == "" { + check.Detail = fmt.Sprintf( + "server-resolved workspace %s (%s), role %s; none configured locally, using the server default", + resp.Workspace.Name, resp.Workspace.ID, resp.Workspace.Role, + ) + return check + } + check.Detail = fmt.Sprintf("workspace %s (%s), role %s", resp.Workspace.Name, resp.Workspace.ID, resp.Workspace.Role) + return check +} + +func probeVersion(ctx context.Context, server string, report *doctorReport) doctorCheck { + check := doctorCheck{Name: "version_skew"} + c := apiclient.New(server, "") + var resp struct { + 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) + return check + } + report.ServerVersion = resp.Version + switch { + case resp.Version == "": + check.Status = doctorWarn + check.Detail = "server reported no version" + case cliVersion == "" || cliVersion == devCLIVersion: + // Honest limit, not a pass: release builds do not inject a CLI version + // yet, so there is nothing to compare against. + check.Status = doctorWarn + check.Detail = fmt.Sprintf( + "skew not computable: this CLI build reports %q (no version injected at build time); server reports %s", + cliVersion, resp.Version, + ) + check.Hint = "compare `mem version` against the release notes for the images you deployed" + case resp.Version == cliVersion: + check.Status = doctorOK + check.Detail = "CLI and server both report " + cliVersion + default: + check.Status = doctorWarn + check.Detail = fmt.Sprintf("CLI reports %s, server reports %s", cliVersion, resp.Version) + check.Hint = "upgrade the CLI or redeploy the server images so the two agree" + } + return check +} + +// 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) { + var ae *apiclient.APIError + if errors.As(err, &ae) { + switch ae.Kind() { + case apiclient.KindAuth: + return doctorFail, exitAuth, fmt.Sprintf("server rejected the request (HTTP %d)", ae.StatusCode), notLoggedInHint + case apiclient.KindNotFound: + return doctorFail, exitNotFound, fmt.Sprintf("no mem server at this URL (HTTP %d)", ae.StatusCode), deployPathHint() + case apiclient.KindPlan, apiclient.KindQuota: + return doctorFail, exitPlanQuota, fmt.Sprintf("server refused for plan or quota (HTTP %d)", ae.StatusCode), "" + } + return doctorFail, exitProvider, fmt.Sprintf("server error (HTTP %d): %s", ae.StatusCode, ae.Message), deployPathHint() + } + 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" + } + return doctorFail, exitProvider, "cannot reach the configured server: " + err.Error(), deployPathHint() +} + +// deployPathHint points at the container path the docs recommend, instead of a +// host-specific dependency recipe. +func deployPathHint() string { + return "start the documented container path: deploy/compose, see docs/DEPLOYMENT.md" +} + +// redactURL strips userinfo so a URL that carries credentials cannot be echoed +// into a report that an operator will paste into an issue. The marker uses only +// 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 { + return raw + } + u.User = url.User("REDACTED") + return u.String() +} + +func printDoctorReport(cmd *cobra.Command, r doctorReport) { + out := cmd.OutOrStdout() + fmt.Fprintf(out, "mem doctor (%s v%d)\n", r.Contract, r.SchemaVersion) + fmt.Fprintf(out, "server: %s\n", r.Server) + for _, c := range r.Checks { + fmt.Fprintf(out, "%-20s %-8s %s\n", c.Name, c.Status, c.Detail) + if c.Hint != "" { + fmt.Fprintf(out, "%-20s hint: %s\n", "", c.Hint) + } + } +} diff --git a/server/cmd/mem/cmds_doctor_test.go b/server/cmd/mem/cmds_doctor_test.go new file mode 100644 index 0000000..3c56236 --- /dev/null +++ b/server/cmd/mem/cmds_doctor_test.go @@ -0,0 +1,670 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// doctorStub answers the three endpoints mem doctor probes, and records every +// request. An unexpected method or path fails the test, which is how AC-002 +// ("performs no write request of any kind") is enforced. +type doctorStub struct { + mu sync.Mutex + requests []string + healthz func(http.ResponseWriter) + caps func(http.ResponseWriter) + version func(http.ResponseWriter) +} + +func newDoctorStub() *doctorStub { + s := &doctorStub{} + s.healthz = func(w http.ResponseWriter) { + _, _ = w.Write([]byte(`{"ok":true}`)) + } + s.caps = func(w http.ResponseWriter) { + _, _ = w.Write([]byte(`{"workspace":{"id":"11111111-1111-1111-1111-111111111111","name":"Personal","role":"owner"}}`)) + } + s.version = func(w http.ResponseWriter) { + _, _ = w.Write([]byte(`{"version":"0.1.0"}`)) + } + return s +} + +func (s *doctorStub) server(t *testing.T) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + if len(raw) > 0 { + t.Errorf("%s %s carried a request body: %s", r.Method, r.URL.Path, raw) + } + s.mu.Lock() + s.requests = append(s.requests, r.Method+" "+r.URL.Path) + s.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && r.URL.Path == "/healthz": + s.healthz(w) + case r.Method == http.MethodGet && r.URL.Path == "/v1/capabilities": + s.caps(w) + case r.Method == http.MethodGet && r.URL.Path == "/v1/version": + s.version(w) + default: + t.Errorf("doctor made an unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotImplemented) + } + })) +} + +func (s *doctorStub) seen() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.requests...) +} + +// configureDoctor points the CLI at server with the given credential state. +// writeConfig controls whether a config file exists on disk at all, which is the +// first-run distinction REQ-002 turns on. +func configureDoctor(t *testing.T, server, token string, writeConfig bool) { + t.Helper() + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + if writeConfig { + if err := os.WriteFile(cfgPath, []byte("server: "+server+"\n"), 0o600); err != nil { + t.Fatal(err) + } + } + t.Setenv("MEM_CONFIG", cfgPath) + t.Setenv("MEM_SERVER", server) + t.Setenv("MEM_WORKSPACE", "") + t.Setenv("MEM_TOKEN", token) +} + +// execDoctor runs `mem doctor` with args and returns what it printed on stdout, +// what it printed on stderr (cobra's own error and usage text), and the error. +// main.go merges the two, but the report and cobra's noise are different surfaces +// and the assertions need to tell them apart. +func execDoctor(t *testing.T, args ...string) (string, string, error) { + t.Helper() + var stdout, stderr bytes.Buffer + root := newRootCmd() + root.SetOut(&stdout) + root.SetErr(&stderr) + root.SetArgs(append([]string{"doctor"}, args...)) + err := root.Execute() + return stdout.String(), stderr.String(), err +} + +func decodeReport(t *testing.T, out string) doctorReport { + t.Helper() + // A failing command's output buffer also carries cobra's own "Error:" and + // usage block: cobra writes them via OutOrStderr, which is this same writer + // when a test routes output into a buffer. In production the report is on + // stdout and cobra's noise is on stderr. Decoding the first JSON value keeps + // the assertion about the report itself. + dec := json.NewDecoder(strings.NewReader(strings.TrimSpace(out))) + var rep doctorReport + if err := dec.Decode(&rep); err != nil { + t.Fatalf("decode doctor json: %v\n%s", err, out) + } + return rep +} + +func checkByName(t *testing.T, rep doctorReport, name string) doctorCheck { + t.Helper() + for _, c := range rep.Checks { + if c.Name == name { + return c + } + } + t.Fatalf("no check named %q in %+v", name, rep.Checks) + return doctorCheck{} +} + +func cliCode(t *testing.T, err error) int { + t.Helper() + var ce *cliError + if !errors.As(err, &ce) { + t.Fatalf("error = %#v, want *cliError", err) + } + return ce.code +} + +func TestDoctorHealthyReportsAllChecksAndExitsZero(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "secret-token-value", true) + t.Cleanup(func() { cliVersion = devCLIVersion }) + cliVersion = "0.1.0" + + out, _, err := execDoctor(t, "--format", "json") + if err != nil { + t.Fatalf("healthy doctor returned %v\n%s", err, out) + } + rep := decodeReport(t, out) + if rep.ExitCode != exitOK { + t.Errorf("report exit_code = %d, want 0", rep.ExitCode) + } + want := []string{"server_reachability", "credential", "workspace", "version_skew"} + if len(rep.Checks) != len(want) { + t.Fatalf("checks = %d, want %d: %+v", len(rep.Checks), len(want), rep.Checks) + } + for i, name := range want { + if rep.Checks[i].Name != name { + t.Errorf("check %d = %q, want %q", i, rep.Checks[i].Name, name) + } + if rep.Checks[i].Status != doctorOK { + t.Errorf("%s status = %q (%s), want ok", name, rep.Checks[i].Status, rep.Checks[i].Detail) + } + } + // AC-002: exactly the three read probes, in order. + if got, wantReq := stub.seen(), []string{"GET /healthz", "GET /v1/capabilities", "GET /v1/version"}; strings.Join(got, ",") != strings.Join(wantReq, ",") { + t.Errorf("requests = %v, want %v", got, wantReq) + } + if strings.Contains(out, "secret-token-value") { + t.Errorf("report leaked the token value:\n%s", out) + } +} + +func TestDoctorUnreachableServer(t *testing.T) { + closed := httptest.NewServer(nil) + addr := closed.URL + closed.Close() + configureDoctor(t, addr, "tok", true) + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("doctor should exit non-zero for an unreachable server\n%s", out) + } + if code := cliCode(t, err); code != exitProvider { + t.Fatalf("exit code = %d, want %d", code, exitProvider) + } + rep := decodeReport(t, out) + c := checkByName(t, rep, "server_reachability") + if c.Status != doctorFail || c.ExitCode != exitProvider { + t.Errorf("reachability = %s/%d, want fail/%d", c.Status, c.ExitCode, exitProvider) + } + // The hint must name the documented container path, not a host recipe. + if !strings.Contains(c.Hint, "deploy/compose") || !strings.Contains(c.Hint, "docs/DEPLOYMENT.md") { + t.Errorf("hint = %q, want the documented deployment path", c.Hint) + } + for _, name := range []string{"workspace", "version_skew"} { + got := checkByName(t, rep, name) + if got.Status != doctorSkipped { + t.Errorf("%s = %s, want skipped", name, got.Status) + } + if !strings.Contains(got.Detail, "server_reachability") { + t.Errorf("%s detail = %q, want it to name the blocking check", name, got.Detail) + } + } +} + +func TestDoctorMissingCredential(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "", false) // no config file: first run + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("doctor should exit non-zero without a credential\n%s", out) + } + if code := cliCode(t, err); code != exitAuth { + t.Fatalf("exit code = %d, want %d", code, exitAuth) + } + rep := decodeReport(t, out) + c := checkByName(t, rep, "credential") + if c.Status != doctorFail || c.ExitCode != exitAuth { + t.Errorf("credential = %s/%d, want fail/%d", c.Status, c.ExitCode, exitAuth) + } + if !strings.Contains(c.Hint, "mem auth login") { + t.Errorf("hint = %q, want it to name `mem auth login`", c.Hint) + } + if !strings.Contains(c.Hint, "deploy/compose") { + t.Errorf("hint = %q, want first-run guidance naming the documented path", c.Hint) + } + if got, wantReq := stub.seen(), "GET /healthz"; strings.Join(got, ",") != wantReq { + t.Errorf("requests = %v, want only the health probe", got) + } +} + +// A machine that already has a config is not a first run: it must not be told to +// deploy a stack it is evidently already talking to. +func TestDoctorMissingCredentialOnConfiguredHost(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "", true) + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("want non-zero exit\n%s", out) + } + c := checkByName(t, decodeReport(t, out), "credential") + if !strings.Contains(c.Hint, "mem auth login") { + t.Errorf("hint = %q, want the login step", c.Hint) + } + if strings.Contains(c.Hint, "deploy/compose") { + t.Errorf("hint = %q, must not suggest deploying on an already-configured host", c.Hint) + } +} + +func TestDoctorNoWorkspaceSelected(t *testing.T) { + stub := newDoctorStub() + stub.caps = func(w http.ResponseWriter) { + _, _ = w.Write([]byte(`{"workspace":{"id":"","name":"","role":""}}`)) + } + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "tok", true) + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("doctor should exit non-zero when no workspace resolves\n%s", out) + } + if code := cliCode(t, err); code != exitNotFound { + t.Fatalf("exit code = %d, want %d", code, exitNotFound) + } + c := checkByName(t, decodeReport(t, out), "workspace") + if c.Status != doctorFail || c.ExitCode != exitNotFound { + t.Errorf("workspace = %s/%d, want fail/%d", c.Status, c.ExitCode, exitNotFound) + } +} + +func TestDoctorRejectedCredential(t *testing.T) { + stub := newDoctorStub() + stub.caps = func(w http.ResponseWriter) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"missing_bearer","code":"unauthorized"}`)) + } + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "expired-token", true) + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("doctor should exit non-zero on a rejected token\n%s", out) + } + if code := cliCode(t, err); code != exitAuth { + t.Fatalf("exit code = %d, want %d", code, exitAuth) + } + c := checkByName(t, decodeReport(t, out), "workspace") + if c.Status != doctorFail || c.ExitCode != exitAuth { + t.Errorf("workspace = %s/%d, want fail/%d", c.Status, c.ExitCode, exitAuth) + } +} + +// TestDoctorQuotaIsItsOwnCode pins the 4 (plan/quota) arm of the SPEC §7.1 map. +func TestDoctorQuotaIsItsOwnCode(t *testing.T) { + stub := newDoctorStub() + stub.caps = func(w http.ResponseWriter) { + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"error":"quota_exceeded","code":"quota"}`)) + } + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "tok", true) + + out, _, err := execDoctor(t, "--format", "json") + if err == nil { + t.Fatalf("want non-zero exit\n%s", out) + } + if code := cliCode(t, err); code != exitPlanQuota { + t.Fatalf("exit code = %d, want %d\n%s", code, exitPlanQuota, out) + } +} + +func TestDoctorVersionSkew(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "tok", true) + t.Cleanup(func() { cliVersion = devCLIVersion }) + + // A dev build does not know its own version, so the check must say the + // comparison is impossible instead of claiming agreement. + cliVersion = devCLIVersion + out, _, err := execDoctor(t, "--format", "json") + if err != nil { + t.Fatalf("an advisory skew must not fail the run: %v\n%s", err, out) + } + c := checkByName(t, decodeReport(t, out), "version_skew") + if c.Status != doctorWarn || !strings.Contains(c.Detail, "not computable") { + t.Errorf("dev-build skew = %s (%s), want warn / not computable", c.Status, c.Detail) + } + + cliVersion = "0.0.9" + out, _, err = execDoctor(t, "--format", "json") + if err != nil { + t.Fatalf("skew should stay advisory: %v\n%s", err, out) + } + rep := decodeReport(t, out) + c = checkByName(t, rep, "version_skew") + if c.Status != doctorWarn || !strings.Contains(c.Detail, "0.0.9") || !strings.Contains(c.Detail, "0.1.0") { + t.Errorf("skew = %s (%s), want both versions named", c.Status, c.Detail) + } + if c.ExitCode != exitOK { + t.Errorf("skew exit contribution = %d, want 0 (advisory)", c.ExitCode) + } + if rep.ServerVersion != "0.1.0" { + t.Errorf("server_version = %q, want 0.1.0", rep.ServerVersion) + } + + cliVersion = "0.1.0" + out, _, _ = execDoctor(t, "--format", "json") + if c = checkByName(t, decodeReport(t, out), "version_skew"); c.Status != doctorOK { + t.Errorf("matching skew = %s (%s), want ok", c.Status, c.Detail) + } +} + +func TestDoctorNeverPrintsSecretValues(t *testing.T) { + const token = "sup3r-s3cret-token" + stub := newDoctorStub() + stub.caps = func(w http.ResponseWriter) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"workspace_forbidden","code":"forbidden"}`)) + } + srv := stub.server(t) + defer srv.Close() + + dir := t.TempDir() + cfg := filepath.Join(dir, "config.yaml") + body := fmt.Sprintf("server: %s\nemail: ops@corp\ntoken: %s\nworkspace: w-1\n", srv.URL, token) + if err := os.WriteFile(cfg, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("MEM_CONFIG", cfg) + t.Setenv("MEM_SERVER", srv.URL) + t.Setenv("MEM_TOKEN", token) + t.Setenv("MEM_WORKSPACE", "") + + for _, format := range []string{"text", "json"} { + stdout, stderr, _ := execDoctor(t, "--format", format) + if stdout == "" { + t.Fatalf("%s run produced no output", format) + } + for label, out := range map[string]string{"stdout": stdout, "stderr": stderr} { + if strings.Contains(out, token) { + t.Errorf("%s %s leaks the token value:\n%s", format, label, out) + } + } + } +} + +func TestRedactURLStripsUserinfo(t *testing.T) { + const secret = "dsn-p4ssw0rd" + got := redactURL("http://admin:" + secret + "@mem.internal:8787") + if strings.Contains(got, secret) { + t.Errorf("redactURL = %q, still carries the password", got) + } + if !strings.Contains(got, "REDACTED@mem.internal:8787") { + t.Errorf("redactURL = %q, want the userinfo replaced and the host kept", got) + } + plain := "http://localhost:8787" + if redactURL(plain) != plain { + t.Errorf("redactURL(%q) = %q, want it unchanged", plain, redactURL(plain)) + } +} + +func TestDoctorTextOutputIsAFixedOrderedList(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "tok", true) + t.Cleanup(func() { cliVersion = devCLIVersion }) + + out, _, err := execDoctor(t) + if err != nil { + t.Fatalf("healthy doctor returned %v\n%s", err, out) + } + names := []string{"server_reachability", "credential", "workspace", "version_skew"} + prev := -1 + for _, n := range names { + at := strings.Index(out, n) + if at < 0 { + t.Fatalf("text output missing %q:\n%s", n, out) + } + if at < prev { + t.Errorf("check %q printed out of order:\n%s", n, out) + } + prev = at + } + if !strings.Contains(out, "mem doctor (mem.doctor v1)") { + t.Errorf("text output missing the contract header:\n%s", out) + } + if strings.Contains(out, `"checks"`) { + t.Errorf("text output contains JSON:\n%s", out) + } +} + +func TestDoctorRejectsBadFlags(t *testing.T) { + dir := t.TempDir() + t.Setenv("MEM_CONFIG", filepath.Join(dir, "missing.yaml")) + t.Setenv("MEM_TOKEN", "tok") + + if _, _, err := execDoctor(t, "--format", "yaml"); err == nil { + t.Error("--format yaml should be rejected") + } + if _, _, err := execDoctor(t, "--timeout", "0s"); err == nil { + t.Error("--timeout 0s should be rejected") + } +} + +// doctorSchema mirrors the parts of docs/schemas/mem-doctor.v1.schema.json that +// this test can enforce without a draft-2020-12 evaluator: required keys, the +// closed enums, key admission (additionalProperties:false) and the fixed check +// order. +type doctorSchema struct { + Required []string `json:"required"` + Properties map[string]doctorSchemaNode `json:"properties"` + Defs map[string]doctorSchemaNode `json:"$defs"` +} + +type doctorSchemaNode struct { + Type string `json:"type"` + Enum []json.RawMessage `json:"enum"` + Const json.RawMessage `json:"const"` + Required []string `json:"required"` + Properties map[string]doctorSchemaNode `json:"properties"` + PrefixItems []json.RawMessage `json:"prefixItems"` +} + +func loadDoctorSchema(t *testing.T) doctorSchema { + t.Helper() + b, err := os.ReadFile(filepath.Join("..", "..", "..", "docs", "schemas", "mem-doctor.v1.schema.json")) + if err != nil { + t.Fatal(err) + } + var s doctorSchema + if err := json.Unmarshal(b, &s); err != nil { + t.Fatalf("checked-in schema is not parseable: %v", err) + } + if len(s.Required) == 0 || len(s.Properties) == 0 { + t.Fatalf("schema did not declare required keys or properties: %s", b) + } + if s.Defs["check"].Type != "object" { + t.Fatalf("schema missing $defs.check object: %s", b) + } + return s +} + +// TestDoctorJSONMatchesCheckedInSchema is AC-003. +func TestDoctorJSONMatchesCheckedInSchema(t *testing.T) { + stub := newDoctorStub() + srv := stub.server(t) + defer srv.Close() + configureDoctor(t, srv.URL, "tok", true) + t.Cleanup(func() { cliVersion = devCLIVersion }) + cliVersion = "0.1.0" + + out, _, err := execDoctor(t, "--format", "json") + if err != nil { + t.Fatalf("healthy doctor returned %v\n%s", err, out) + } + schema := loadDoctorSchema(t) + validateDoctorDoc(t, schema, []byte(out)) + + // httptest assigns an ephemeral port, which the report echoes in two places + // (server and the reachability detail). The golden pins everything except + // that, so drift in shape, order, wording or codes still fails loudly. + normalized := strings.ReplaceAll(out, srv.URL, "http://127.0.0.1:PORT") + + golden := filepath.Join("testdata", "doctor_healthy.golden.json") + if os.Getenv("MEM_UPDATE_GOLDEN") != "" { + if err := os.MkdirAll(filepath.Dir(golden), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(golden, []byte(normalized), 0o600); err != nil { + t.Fatal(err) + } + } + want, err := os.ReadFile(golden) + if err != nil { + t.Fatalf("read golden: %v (create it with MEM_UPDATE_GOLDEN=1 go test ./cmd/mem/ -run TestDoctorJSON)", err) + } + if strings.TrimSpace(string(want)) != strings.TrimSpace(normalized) { + t.Errorf("doctor json drifted from %s\n--- want ---\n%s\n--- got ---\n%s", golden, want, normalized) + } +} + +func validateDoctorDoc(t *testing.T, s doctorSchema, doc []byte) { + t.Helper() + var obj map[string]json.RawMessage + if err := json.Unmarshal(doc, &obj); err != nil { + t.Fatalf("report is not a JSON object: %v", err) + } + for _, req := range s.Required { + if _, ok := obj[req]; !ok { + t.Errorf("report missing required key %q", req) + } + } + for key := range obj { + if _, ok := s.Properties[key]; !ok { + t.Errorf("report has key %q, which the schema forbids (additionalProperties:false)", key) + } + } + for _, key := range []string{"contract", "schema_version"} { + if !nodeAllows(s.Properties[key], obj[key]) { + t.Errorf("%s = %s, outside the schema's const", key, obj[key]) + } + } + if !nodeAllows(s.Properties["exit_code"], obj["exit_code"]) { + t.Errorf("exit_code = %s, outside the SPEC 7.1 set", obj["exit_code"]) + } + + var checks []map[string]json.RawMessage + if err := json.Unmarshal(obj["checks"], &checks); err != nil { + t.Fatalf("checks is not an array: %v", err) + } + if len(checks) != len(s.Properties["checks"].PrefixItems) { + t.Fatalf("checks length = %d, want %d", len(checks), len(s.Properties["checks"].PrefixItems)) + } + def := s.Defs["check"] + for i, c := range checks { + for _, req := range def.Required { + if _, ok := c[req]; !ok { + t.Errorf("checks[%d] missing required key %q", i, req) + } + } + for key := range c { + if _, ok := def.Properties[key]; !ok { + t.Errorf("checks[%d] has key %q the schema forbids", i, key) + } + } + for _, field := range []string{"name", "status", "exit_code"} { + if !nodeAllows(def.Properties[field], c[field]) { + t.Errorf("checks[%d].%s = %s, outside the schema's closed enum", i, field, c[field]) + } + } + // prefixItems pins the order, so a reordered report fails here. + var slot struct { + AllOf []struct { + Properties map[string]doctorSchemaNode `json:"properties"` + } `json:"allOf"` + } + if err := json.Unmarshal(s.Properties["checks"].PrefixItems[i], &slot); err != nil { + t.Fatalf("prefixItems[%d] unreadable: %v", i, err) + } + for _, sub := range slot.AllOf { + if want, ok := sub.Properties["name"]; ok && !nodeAllows(want, c["name"]) { + t.Errorf("checks[%d].name = %s, want %s (order is part of the contract)", i, c["name"], want.Enum) + } + } + } +} + +// nodeAllows reports whether raw satisfies a leaf schema doctorSchemaNode that constrains by +// const or enum. A leaf with neither declares no value constraint. +func nodeAllows(n doctorSchemaNode, raw json.RawMessage) bool { + value := strings.TrimSpace(string(raw)) + if len(n.Const) > 0 { + return strings.TrimSpace(string(n.Const)) == value + } + if len(n.Enum) > 0 { + for _, e := range n.Enum { + if strings.TrimSpace(string(e)) == value { + return true + } + } + return false + } + return true +} + +// TestNotLoggedInGuidance is REQ-002 on an existing command surface: the hint +// that used to stop at `mem auth login` must additionally name the documented +// deployment path, but only when no credential exists at all. +func TestNotLoggedInGuidance(t *testing.T) { + dir := t.TempDir() + missing := filepath.Join(dir, "missing.yaml") + existing := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(existing, []byte("server: http://127.0.0.1:1\n"), 0o600); err != nil { + t.Fatal(err) + } + + cases := []struct { + name string + cfgPath string + want []string + deny string + }{ + {name: "first run", cfgPath: missing, want: []string{"mem auth login", "deploy/compose", "docs/DEPLOYMENT.md"}}, + {name: "configured but logged out", cfgPath: existing, want: []string{"mem auth login"}, deny: "deploy/compose"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("MEM_CONFIG", tc.cfgPath) + t.Setenv("MEM_TOKEN", "") + t.Setenv("MEM_SERVER", "") + var out bytes.Buffer + root := newRootCmd() + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"search", "fy27 recruiting"}) + err := root.Execute() + var code int + if code = cliCode(t, err); code != exitAuth { + t.Fatalf("exit code = %d, want %d", code, exitAuth) + } + ce := err.(*cliError) + for _, want := range tc.want { + if !strings.Contains(ce.hint, want) { + t.Errorf("hint = %q, want it to name %q", ce.hint, want) + } + } + if tc.deny != "" && strings.Contains(ce.hint, tc.deny) { + t.Errorf("hint = %q, must not suggest deploying where a config exists", ce.hint) + } + }) + } +} diff --git a/server/cmd/mem/cmds_face.go b/server/cmd/mem/cmds_face.go index 4bed56c..fcb53ee 100644 --- a/server/cmd/mem/cmds_face.go +++ b/server/cmd/mem/cmds_face.go @@ -41,7 +41,7 @@ func newFaceListCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) var resp faceListResp diff --git a/server/cmd/mem/cmds_file.go b/server/cmd/mem/cmds_file.go index 37f30a1..f2ed097 100644 --- a/server/cmd/mem/cmds_file.go +++ b/server/cmd/mem/cmds_file.go @@ -42,7 +42,7 @@ func newPutCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) sourceMetadata, err := cliSourceMetadata( @@ -491,7 +491,7 @@ func newVersionCmd() *cobra.Command { Use: "version", Short: "Print client + server version", RunE: func(cmd *cobra.Command, args []string) error { - fmt.Println("mem CLI dev") + fmt.Printf("mem CLI %s\n", cliVersion) cfg, _ := resolveConfig("") if cfg != nil && cfg.Server != "" { c := newHTTPClient(cfg) @@ -499,7 +499,7 @@ func newVersionCmd() *cobra.Command { Version string `json:"version"` } if err := c.doJSON(http.MethodGet, "/v1/version", nil, &resp); err == nil { - fmt.Printf("server: %s (%s)\n", resp.Version, cfg.Server) + fmt.Printf("server: %s (%s)\n", resp.Version, redactURL(cfg.Server)) } } return nil diff --git a/server/cmd/mem/cmds_file_annotations.go b/server/cmd/mem/cmds_file_annotations.go index c27c3ea..d22e646 100644 --- a/server/cmd/mem/cmds_file_annotations.go +++ b/server/cmd/mem/cmds_file_annotations.go @@ -82,7 +82,7 @@ func configuredFileAnnotationClient() (*apiclient.Client, error) { return nil, err } if cfg.Token == "" { - return nil, newCliError(3, "not logged in", "run `mem auth login` first") + return nil, errNotLoggedIn() } return newHTTPClient(cfg).api, nil } diff --git a/server/cmd/mem/cmds_folder.go b/server/cmd/mem/cmds_folder.go index 08d2517..8bc138d 100644 --- a/server/cmd/mem/cmds_folder.go +++ b/server/cmd/mem/cmds_folder.go @@ -22,7 +22,7 @@ func newMkdirCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) var resp map[string]any diff --git a/server/cmd/mem/cmds_handoff.go b/server/cmd/mem/cmds_handoff.go index dc6133f..b094244 100644 --- a/server/cmd/mem/cmds_handoff.go +++ b/server/cmd/mem/cmds_handoff.go @@ -58,7 +58,7 @@ Examples: return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } raw, err := newHTTPClient(cfg).api.Checkpoint( commandContext(cmd), @@ -265,7 +265,7 @@ Examples: return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } raw, err := newHTTPClient(cfg).api.Resume( commandContext(cmd), diff --git a/server/cmd/mem/cmds_ingest.go b/server/cmd/mem/cmds_ingest.go index 2ed5b20..bb41a15 100644 --- a/server/cmd/mem/cmds_ingest.go +++ b/server/cmd/mem/cmds_ingest.go @@ -158,7 +158,7 @@ func runIngestQoder(cmd *cobra.Command, o ingestOptions) error { return err } if !o.dryRun && cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } client := newHTTPClient(cfg) stateDir := o.checkpointDir() diff --git a/server/cmd/mem/cmds_memory.go b/server/cmd/mem/cmds_memory.go index b842ef5..1c43a93 100644 --- a/server/cmd/mem/cmds_memory.go +++ b/server/cmd/mem/cmds_memory.go @@ -123,7 +123,7 @@ cursor and bounded memory summaries for scripts.`, return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } options := apiclient.MemoryListOptions{ @@ -393,7 +393,7 @@ func configuredMemoryClient() (*apiclient.Client, error) { return nil, err } if cfg.Token == "" { - return nil, newCliError(3, "not logged in", "run `mem auth login` first") + return nil, errNotLoggedIn() } return newHTTPClient(cfg).api, nil } diff --git a/server/cmd/mem/cmds_model.go b/server/cmd/mem/cmds_model.go index 9ff8237..84c1997 100644 --- a/server/cmd/mem/cmds_model.go +++ b/server/cmd/mem/cmds_model.go @@ -425,7 +425,7 @@ func activateLocalModelProfile( return providerSetResp{}, err } if cfg.Token == "" { - return providerSetResp{}, newCliError(3, "not logged in", "run `mem auth login` first") + return providerSetResp{}, errNotLoggedIn() } var response providerSetResp client := newHTTPClient(cfg) diff --git a/server/cmd/mem/cmds_profile.go b/server/cmd/mem/cmds_profile.go index a65ce6e..9092e14 100644 --- a/server/cmd/mem/cmds_profile.go +++ b/server/cmd/mem/cmds_profile.go @@ -143,7 +143,7 @@ func configuredWorkspaceAIProfileClient() (*httpClient, error) { return nil, err } if cfg.Token == "" { - return nil, newCliError(3, "not logged in", "run `mem auth login` first") + return nil, errNotLoggedIn() } return newHTTPClient(cfg), nil } diff --git a/server/cmd/mem/cmds_provider.go b/server/cmd/mem/cmds_provider.go index 322dac3..d31e3e2 100644 --- a/server/cmd/mem/cmds_provider.go +++ b/server/cmd/mem/cmds_provider.go @@ -56,7 +56,7 @@ func newProviderListCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) var resp providerListResp @@ -116,7 +116,7 @@ vectors cannot silently enter different spaces.`, return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) kind := args[0] @@ -164,7 +164,7 @@ historical provider identity was not recorded.`, return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } var resp struct { Provider string `json:"provider"` @@ -201,7 +201,7 @@ func newProviderTestCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) kind := args[0] diff --git a/server/cmd/mem/cmds_related.go b/server/cmd/mem/cmds_related.go index 4146a31..818c28c 100644 --- a/server/cmd/mem/cmds_related.go +++ b/server/cmd/mem/cmds_related.go @@ -82,7 +82,7 @@ Relation types currently supported: return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) path := "/v1/files/" + args[0] + "/related" @@ -161,7 +161,7 @@ outgoing rows before recomputing.`, return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) body := rebuildReq{FileID: file} diff --git a/server/cmd/mem/cmds_remember.go b/server/cmd/mem/cmds_remember.go index 345dadc..e736920 100644 --- a/server/cmd/mem/cmds_remember.go +++ b/server/cmd/mem/cmds_remember.go @@ -115,7 +115,7 @@ Examples: return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } var resp map[string]any diff --git a/server/cmd/mem/cmds_search.go b/server/cmd/mem/cmds_search.go index ac866df..47704ec 100644 --- a/server/cmd/mem/cmds_search.go +++ b/server/cmd/mem/cmds_search.go @@ -56,7 +56,7 @@ func newSearchCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) diff --git a/server/cmd/mem/cmds_timeline.go b/server/cmd/mem/cmds_timeline.go index 1edb1f3..eef049a 100644 --- a/server/cmd/mem/cmds_timeline.go +++ b/server/cmd/mem/cmds_timeline.go @@ -43,7 +43,7 @@ func newTimelineCmd() *cobra.Command { return err } if cfg.Token == "" { - return newCliError(3, "not logged in", "run `mem auth login` first") + return errNotLoggedIn() } c := newHTTPClient(cfg) var resp timelineResp diff --git a/server/cmd/mem/config.go b/server/cmd/mem/config.go index 38d75d3..7388795 100644 --- a/server/cmd/mem/config.go +++ b/server/cmd/mem/config.go @@ -91,3 +91,16 @@ func resolveConfig(serverOverride string) (*cliConfig, error) { } return c, nil } + +// configFileExists reports whether a CLI config file is present on disk. +// loadConfig deliberately succeeds without one, so this is the only signal that +// separates "never configured" from "configured, but not logged in" — the +// distinction first-run guidance has to get right. +func configFileExists() bool { + p, err := configPath() + if err != nil { + return false + } + _, err = os.Stat(p) + return err == nil +} diff --git a/server/cmd/mem/main.go b/server/cmd/mem/main.go index 2205c6d..e882e90 100644 --- a/server/cmd/mem/main.go +++ b/server/cmd/mem/main.go @@ -20,6 +20,17 @@ var ( cliWorkspaceOverride string ) +// devCLIVersion is the placeholder this build reports when no version was +// injected. +const devCLIVersion = "dev" + +// cliVersion is the CLI's reported version. Nothing injects it yet: +// .github/workflows/release.yml builds with `-s -w` only, so release binaries +// currently report devCLIVersion, and `mem doctor`'s version_skew check reports +// "skew not computable" instead of inventing a comparison. Wiring this up is a +// release-side change (GOFLAGS/-ldflags=-X main.cliVersion=…), not a CLI one. +var cliVersion = devCLIVersion + func main() { root := newRootCmd() ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) @@ -85,6 +96,7 @@ func newRootCmd() *cobra.Command { root.AddCommand(newModelCmd()) root.AddCommand(newTimelineCmd()) root.AddCommand(newWorkspaceCmd()) + root.AddCommand(newDoctorCmd()) root.AddCommand(newVersionCmd()) return root } diff --git a/server/cmd/mem/testdata/doctor_healthy.golden.json b/server/cmd/mem/testdata/doctor_healthy.golden.json new file mode 100644 index 0000000..7fcef97 --- /dev/null +++ b/server/cmd/mem/testdata/doctor_healthy.golden.json @@ -0,0 +1,34 @@ +{ + "contract": "mem.doctor", + "schema_version": 1, + "server": "http://127.0.0.1:PORT", + "cli_version": "0.1.0", + "server_version": "0.1.0", + "exit_code": 0, + "checks": [ + { + "name": "server_reachability", + "status": "ok", + "exit_code": 0, + "detail": "healthz ok at http://127.0.0.1:PORT" + }, + { + "name": "credential", + "status": "ok", + "exit_code": 0, + "detail": "token present (from $MEM_TOKEN)" + }, + { + "name": "workspace", + "status": "ok", + "exit_code": 0, + "detail": "server-resolved workspace Personal (11111111-1111-1111-1111-111111111111), role owner; none configured locally, using the server default" + }, + { + "name": "version_skew", + "status": "ok", + "exit_code": 0, + "detail": "CLI and server both report 0.1.0" + } + ] +} From fffcd4c9947874be45b8e2b7991d6815b96a6e20 Mon Sep 17 00:00:00 2001 From: PeterGuy326 <47820304+PeterGuy326@users.noreply.github.com> Date: Fri, 4 Sep 2026 09:52:35 +0800 Subject: [PATCH 2/6] fix(cli): redact malformed URLs and request build errors --- server/cmd/mem/cmds_doctor.go | 72 +++++++++++++++--- server/cmd/mem/cmds_doctor_test.go | 42 +++++++++++ server/internal/apiclient/apiclient.go | 73 ++++++++++++++++++- server/internal/apiclient/apiclient_test.go | 20 +++++ .../internal/apiclient/workspace_transfer.go | 4 +- 5 files changed, 196 insertions(+), 15 deletions(-) diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go index f74cb82..92d78bf 100644 --- a/server/cmd/mem/cmds_doctor.go +++ b/server/cmd/mem/cmds_doctor.go @@ -154,10 +154,10 @@ func runDoctor(cmd *cobra.Command, timeout time.Duration) (doctorReport, error) Server: redactURL(cfg.Server), CLIVersion: cliVersion, } - ctx, cancel := context.WithTimeout(cmd.Context(), timeout) - defer cancel() - reach := probeReachability(ctx, cfg.Server) + reachCtx, cancel := context.WithTimeout(cmd.Context(), timeout) + reach := probeReachability(reachCtx, cfg.Server) + cancel() report.Checks = append(report.Checks, reach) cred := probeCredential(cfg) @@ -173,8 +173,13 @@ func runDoctor(cmd *cobra.Command, timeout time.Duration) (doctorReport, error) case cred.Status == doctorFail: ws, ver = skippedCheck("workspace", cred.Name), skippedCheck("version_skew", cred.Name) default: - ws = probeWorkspace(ctx, cfg) - ver = probeVersion(ctx, cfg.Server, &report) + wsCtx, wsCancel := context.WithTimeout(cmd.Context(), timeout) + ws = probeWorkspace(wsCtx, cfg) + wsCancel() + + verCtx, verCancel := context.WithTimeout(cmd.Context(), timeout) + ver = probeVersion(verCtx, cfg.Server, &report) + verCancel() } report.Checks = append(report.Checks, ws, ver) @@ -334,7 +339,7 @@ func classifyProbe(err error) (status string, code int, detail, hint string) { 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" } - return doctorFail, exitProvider, "cannot reach the configured server: " + err.Error(), deployPathHint() + return doctorFail, exitProvider, "cannot reach the configured server: " + sanitizeProbeError(err), deployPathHint() } // deployPathHint points at the container path the docs recommend, instead of a @@ -348,11 +353,60 @@ func deployPathHint() string { // 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 && 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 } - u.User = url.User("REDACTED") - return u.String() + 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 +} + +func sanitizeProbeError(err error) string { + if err == nil { + return "" + } + msg := err.Error() + const quoted = "\"" + for start := 0; ; { + left := strings.Index(msg[start:], quoted) + if left < 0 { + return msg + } + left += start + right := strings.Index(msg[left+1:], quoted) + if right < 0 { + return msg + } + right += left + 1 + token := msg[left+1 : right] + if strings.Contains(token, "://") { + if redacted := redactURL(token); redacted != token { + msg = msg[:left+1] + redacted + msg[right:] + } + } + start = right + 1 + } } func printDoctorReport(cmd *cobra.Command, r doctorReport) { diff --git a/server/cmd/mem/cmds_doctor_test.go b/server/cmd/mem/cmds_doctor_test.go index 3c56236..b4cbffb 100644 --- a/server/cmd/mem/cmds_doctor_test.go +++ b/server/cmd/mem/cmds_doctor_test.go @@ -410,12 +410,54 @@ func TestRedactURLStripsUserinfo(t *testing.T) { if !strings.Contains(got, "REDACTED@mem.internal:8787") { t.Errorf("redactURL = %q, want the userinfo replaced and the host kept", got) } + got = redactURL("http://admin:" + secret + "@ho st.example.com:8787") + if strings.Contains(got, secret) { + t.Errorf("redactURL = %q, still carries the malformed-password value", got) + } + if !strings.Contains(got, "REDACTED@ho st.example.com:8787") { + t.Errorf("redactURL = %q, want malformed URLs to redact their userinfo", got) + } plain := "http://localhost:8787" if redactURL(plain) != plain { t.Errorf("redactURL(%q) = %q, want it unchanged", plain, redactURL(plain)) } } +func TestDoctorMalformedServerURLDoesNotLeakCredentials(t *testing.T) { + const secret = "malformed-psswd" + malformed := "http://admin:" + secret + "@ho st.example.com:1/healthz" + configureDoctor(t, malformed, "tok", true) + + for _, format := range []string{"json", "text"} { + stdout, stderr, err := execDoctor(t, "--format", format) + if err == nil { + t.Fatalf("doctor should fail with malformed server URL in %s format\n%s", format, stdout) + } + if strings.Contains(stdout, secret) { + t.Errorf("stdout leaked credential for %s: %s", format, stdout) + } + if strings.Contains(stderr, secret) { + t.Errorf("stderr leaked credential for %s: %s", format, stderr) + } + if strings.Contains(stdout, malformed) { + t.Errorf("stdout should not carry raw malformed URL in %s: %s", format, stdout) + } + if format == "json" { + rep := decodeReport(t, stdout) + reach := checkByName(t, rep, "server_reachability") + if strings.Contains(reach.Detail, secret) { + t.Errorf("server_reachability detail leaked secret: %s", reach.Detail) + } + if strings.Contains(reach.Detail, malformed) { + t.Errorf("server_reachability detail leaked raw URL: %s", reach.Detail) + } + if !strings.Contains(reach.Detail, "REDACTED") { + t.Errorf("server_reachability detail should redact credentials: %s", reach.Detail) + } + } + } +} + func TestDoctorTextOutputIsAFixedOrderedList(t *testing.T) { stub := newDoctorStub() srv := stub.server(t) diff --git a/server/internal/apiclient/apiclient.go b/server/internal/apiclient/apiclient.go index 38dfabd..442c99e 100644 --- a/server/internal/apiclient/apiclient.go +++ b/server/internal/apiclient/apiclient.go @@ -97,7 +97,7 @@ func (c *Client) DoJSONWithHeaders(ctx context.Context, method, path string, bod } rdr = bytes.NewReader(b) } - req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, rdr) + req, err := c.newRequest(ctx, method, c.baseURL+path, rdr) if err != nil { return err } @@ -167,7 +167,7 @@ func (c *Client) UploadMultipartWithSourceMetadata(ctx context.Context, name, mi errCh <- nil }() - req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/files", pr) + req, err := c.newRequest(ctx, http.MethodPost, c.baseURL+"/v1/files", pr) if err != nil { return err } @@ -214,7 +214,7 @@ func (c *Client) UploadStreamWithSourceMetadata(ctx context.Context, name, mimeT if err != nil { return err } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/files?"+q.Encode(), body) + req, err := c.newRequest(ctx, http.MethodPost, c.baseURL+"/v1/files?"+q.Encode(), body) if err != nil { return err } @@ -239,7 +239,7 @@ func (c *Client) UploadStreamWithSourceMetadata(ctx context.Context, name, mimeT // DownloadStream returns a streaming reader for GET /v1/files/{id}/content. // Callers MUST close the returned ReadCloser. func (c *Client) DownloadStream(ctx context.Context, fileID string) (io.ReadCloser, string, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/v1/files/"+fileID+"/content", nil) + req, err := c.newRequest(ctx, http.MethodGet, c.baseURL+"/v1/files/"+fileID+"/content", nil) if err != nil { return nil, "", err } @@ -268,6 +268,71 @@ func marshalSourceMetadata(sourceMetadata *FileSourceMetadata) (string, error) { return string(raw), nil } +func (c *Client) newRequest(ctx context.Context, method, target string, body io.Reader) (*http.Request, error) { + req, err := http.NewRequestWithContext(ctx, method, target, body) + if err == nil { + return req, nil + } + return nil, requestBuildError(method, target, err) +} + +func requestBuildError(method, target string, err error) error { + return &redactErr{ + message: fmt.Sprintf("%s %s: %s", method, redactURL(target), redactedRequestFailure(err)), + } +} + +type redactErr struct { + message string +} + +func (e *redactErr) Error() string { return e.message } + +func redactedRequestFailure(err error) string { + msg := err.Error() + const quoted = "\"" + for start := 0; ; { + left := strings.Index(msg[start:], quoted) + if left < 0 { + break + } + left += start + right := strings.Index(msg[left+1:], quoted) + if right < 0 { + break + } + right += left + 1 + token := msg[left+1 : right] + if strings.Contains(token, "://") { + if redacted := redactURL(token); redacted != token { + msg = msg[:left+1] + redacted + msg[right:] + return msg + } + } + start = right + 1 + } + return msg +} + +func redactURL(raw string) string { + u, err := url.Parse(raw) + if err == nil && 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:] + } + } + } + return raw +} + func (c *Client) attachAuth(req *http.Request) { if c.token != "" { req.Header.Set("Authorization", "Bearer "+c.token) diff --git a/server/internal/apiclient/apiclient_test.go b/server/internal/apiclient/apiclient_test.go index 0d61000..b7ad33b 100644 --- a/server/internal/apiclient/apiclient_test.go +++ b/server/internal/apiclient/apiclient_test.go @@ -155,3 +155,23 @@ func TestUploadStreamWithSourceMetadata(t *testing.T) { t.Fatal("source_metadata leaked into the request URL") } } + +func TestRequestBuildErrorRedactsCredentialedURL(t *testing.T) { + const secret = "bad-token-xyz" + err := New("http://admin:"+secret+"@ho st.example.com:1", "token").DoJSON( + context.Background(), + http.MethodGet, + "/v1/test", + nil, + nil, + ) + if err == nil { + t.Fatal("expected request construction to fail for malformed URL") + } + if strings.Contains(err.Error(), secret) { + t.Fatalf("request-build error leaked credential: %v", err) + } + if !strings.Contains(err.Error(), "REDACTED") { + t.Fatalf("request-build error should redact credentials: %v", err) + } +} diff --git a/server/internal/apiclient/workspace_transfer.go b/server/internal/apiclient/workspace_transfer.go index 6273a97..1c64dcc 100644 --- a/server/internal/apiclient/workspace_transfer.go +++ b/server/internal/apiclient/workspace_transfer.go @@ -70,7 +70,7 @@ type WorkspaceImportConflict struct { // in memory. The server does not publish response headers until its complete // archive has been built and validated. func (c *Client) ExportWorkspace(ctx context.Context) (*WorkspaceBundleDownload, error) { - req, err := http.NewRequestWithContext( + req, err := c.newRequest( ctx, http.MethodGet, c.baseURL+"/v1/workspaces/current/export", @@ -133,7 +133,7 @@ func (c *Client) ImportWorkspace( return nil, fmt.Errorf("workspace bundle size must be -1 or non-negative") } query := url.Values{"mode": []string{mode}} - req, err := http.NewRequestWithContext( + req, err := c.newRequest( ctx, http.MethodPost, c.baseURL+"/v1/workspaces/current/import?"+query.Encode(), From b22953716962b9331008c05561c9f6e06600379f Mon Sep 17 00:00:00 2001 From: waterbro-8 <318569545+waterbro-8@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:50:18 +0800 Subject: [PATCH 3/6] fix(client,memd): withhold URLs whose credentials cannot be attributed A configured URL that carries credentials in a shape url.Parse does not report as userinfo reaches output today. "admin:pw@host" parses as Scheme="admin" with the credential in Opaque and User unset, so any implementation that gates on User != nil echoes the credential verbatim. Measured on a binary built from 87d235b: `mem auth status` against such a base URL prints the password in clear text, and the same holds for every http.Client.Do failure in the client, which no redaction covered at all. Route both egresses through one shared gate that redacts a value it can prove is a transport URL and withholds the value whole otherwise. Error text is not scrubbed, because that cannot be made tight: url.Error renders with %q, so a quote inside a password arrives escaped and a scanner that pairs quotes mis-pairs and replaces nothing. memd's fatal line is included because it carries third-party errors that embed a whole DSN -- asynq puts the URI it failed to parse into the message -- and slog renders an error value as its text. The gate proves the absence of userinfo, not of every credential: a secret supplied as a query parameter parses as a clean URL and is still echoed. That shape is out of scope here and is pinned by a named characterization test rather than left as folklore. --- CHANGELOG.md | 18 ++ server/cmd/memd/main.go | 18 +- server/cmd/memd/main_test.go | 51 +++- server/internal/apiclient/apiclient.go | 58 +++- server/internal/apiclient/apiclient_test.go | 60 +++++ server/internal/redact/redact.go | 157 +++++++++++ server/internal/redact/redact_test.go | 277 ++++++++++++++++++++ 7 files changed, 613 insertions(+), 26 deletions(-) create mode 100644 server/internal/redact/redact.go create mode 100644 server/internal/redact/redact_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c094d3..9e48d6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,24 @@ The project publishes 0.x prerelease versions; a stable release line is not yet the headers off a running nginx, since neither failure mode is visible by reading the configuration. +### Fixed + +- A configured URL that carries credentials in a shape `url.Parse` does not + report as userinfo no longer reaches output. `admin:pw@host` parses as + `Scheme="admin"` with the credential in `Opaque` and `User` unset, so an + implementation that gates on `User != nil` echoes it verbatim. On this base it + leaked from the CLI API client — at request construction and at all four + `http.Client.Do` sites, which the previous error path did not cover — and from + `memd`'s startup log line and its fatal log line, the last of which additionally + carries third-party errors that embed a whole DSN. Both now route through one + shared gate that redacts a value it can prove is a transport URL and + **withholds the value whole** otherwise. It does not scrub credentials out of + error text, which cannot be made tight: `url.Error` renders with `%q`, so a + quote inside a password arrives escaped and a scanner that pairs quotes + mis-pairs and replaces nothing. Withholding costs some diagnosability by design; + why a request failed is still reported. A credential supplied as a query + parameter (`?password=`) is still echoed and is tracked separately. + ## [0.1.1] - 2026-08-31 ### Changed diff --git a/server/cmd/memd/main.go b/server/cmd/memd/main.go index 37cb47d..3e08444 100644 --- a/server/cmd/memd/main.go +++ b/server/cmd/memd/main.go @@ -8,7 +8,6 @@ import ( "fmt" "log/slog" "net/http" - "net/url" "os" "os/signal" "path/filepath" @@ -34,6 +33,7 @@ import ( "github.com/PeterGuy326/mem/server/internal/memory" "github.com/PeterGuy326/mem/server/internal/provider" "github.com/PeterGuy326/mem/server/internal/queue" + "github.com/PeterGuy326/mem/server/internal/redact" "github.com/PeterGuy326/mem/server/internal/relator" "github.com/PeterGuy326/mem/server/internal/search" "github.com/PeterGuy326/mem/server/internal/storage" @@ -45,7 +45,9 @@ import ( func main() { if err := run(); err != nil { - slog.Error("memd fatal", "err", err) + // run() wraps third-party errors that embed the configured DSN verbatim, + // and slog renders an error value as its text. + slog.Error("memd fatal", "err", redact.Text(err.Error(), redact.StoreURLs)) os.Exit(1) } } @@ -444,13 +446,9 @@ func redactDSN(s string) string { return redactURLCredentials(s) } +// redactURLCredentials gates the two DSNs this process logs. The store schemes +// are allowed here and nowhere else, so an API-shaped egress can never echo a +// database URL by accident. func redactURLCredentials(raw string) string { - parsed, err := url.Parse(raw) - if err != nil || parsed.User == nil { - return raw - } - if _, hasPassword := parsed.User.Password(); !hasPassword { - return raw - } - return parsed.Redacted() + return redact.URL(raw, redact.StoreURLs) } diff --git a/server/cmd/memd/main_test.go b/server/cmd/memd/main_test.go index f36a0fb..6fef4b5 100644 --- a/server/cmd/memd/main_test.go +++ b/server/cmd/memd/main_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/PeterGuy326/mem/server/internal/redact" "github.com/PeterGuy326/mem/server/internal/workspacebundle" ) @@ -15,14 +16,36 @@ func TestRedactURLCredentials(t *testing.T) { tests := []struct { name string raw string + want string }{ { name: "postgres", raw: "postgres://mem:database-secret@postgres:5432/mem?sslmode=disable", + want: "postgres://REDACTED@postgres:5432/mem?sslmode=disable", }, { name: "redis", raw: "redis://:redis-secret@redis:6379/0", + want: "redis://REDACTED@redis:6379/0", + }, + { + // The old scrubber echoed this raw because it only masked values with a + // password; the gate treats any userinfo as a credential. + name: "username only", + raw: "postgres://mem@postgres:5432/mem?sslmode=disable", + want: "postgres://REDACTED@postgres:5432/mem?sslmode=disable", + }, + { + // url.Parse reads the scheme as "redis" and parks the rest in Opaque, + // so a u.User check never fires. + name: "redis without a transport scheme", + raw: "redis:redis-secret@redis:6379/0", + want: redact.Placeholder, + }, + { + name: "postgres that fails to parse", + raw: "postgres://mem:database-secret@post gres:5432/mem", + want: redact.Placeholder, }, } for _, test := range tests { @@ -30,23 +53,22 @@ func TestRedactURLCredentials(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() got := redactURLCredentials(test.raw) + if got != test.want { + t.Fatalf("redactURLCredentials(%q) = %q, want %q", test.raw, got, test.want) + } if strings.Contains(got, "secret") { t.Fatalf("credentials leaked from %q: %q", test.raw, got) } - if !strings.Contains(got, "@") { - t.Fatalf("redacted URL lost its endpoint: %q", got) - } }) } } -func TestRedactURLCredentialsLeavesPasswordlessValuesAlone(t *testing.T) { +func TestRedactURLCredentialsLeavesCredentialFreeValuesAlone(t *testing.T) { t.Parallel() for _, raw := range []string{ "redis://redis:6379/0", - "redis:6379", - "://not-a-url", + "http://mem.internal:8787", } { if got := redactURLCredentials(raw); got != raw { t.Fatalf("redactURLCredentials(%q) = %q", raw, got) @@ -54,6 +76,23 @@ func TestRedactURLCredentialsLeavesPasswordlessValuesAlone(t *testing.T) { } } +// TestRedactURLCredentialsWithholdsWhatItCannotVerify pins the fail-closed half: +// these shapes carry no visible password today, but the gate cannot prove that +// from a parse it either failed or had to attribute to an unknown scheme, so +// logging them at all would be guessing. +func TestRedactURLCredentialsWithholdsWhatItCannotVerify(t *testing.T) { + t.Parallel() + + for _, raw := range []string{ + "redis:6379", + "://not-a-url", + } { + if got := redactURLCredentials(raw); got != redact.Placeholder { + t.Fatalf("redactURLCredentials(%q) = %q, want %q", raw, got, redact.Placeholder) + } + } +} + func TestWorkspaceTransferBundleLimitsAreConservativeAndConsistent(t *testing.T) { defaults := workspacebundle.DefaultLimits() limits := workspaceTransferBundleLimits() diff --git a/server/internal/apiclient/apiclient.go b/server/internal/apiclient/apiclient.go index 38dfabd..b9b47cd 100644 --- a/server/internal/apiclient/apiclient.go +++ b/server/internal/apiclient/apiclient.go @@ -12,6 +12,8 @@ import ( "strconv" "strings" "time" + + "github.com/PeterGuy326/mem/server/internal/redact" ) const sourceMetadataHeader = "X-Mem-Source-Metadata" @@ -99,7 +101,7 @@ func (c *Client) DoJSONWithHeaders(ctx context.Context, method, path string, bod } req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, rdr) if err != nil { - return err + return requestBuildError(method, c.baseURL+path, err) } if body != nil { req.Header.Set("Content-Type", "application/json") @@ -110,7 +112,7 @@ func (c *Client) DoJSONWithHeaders(ctx context.Context, method, path string, bod c.attachAuth(req) resp, err := c.hc.Do(req) if err != nil { - return err + return gateTransportError(err) } defer resp.Body.Close() return decode(resp, out) @@ -169,13 +171,13 @@ func (c *Client) UploadMultipartWithSourceMetadata(ctx context.Context, name, mi req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/files", pr) if err != nil { - return err + return requestBuildError(http.MethodPost, c.baseURL+"/v1/files", err) } req.Header.Set("Content-Type", mw.FormDataContentType()) c.attachAuth(req) resp, err := c.hc.Do(req) if err != nil { - return err + return gateTransportError(err) } defer resp.Body.Close() if werr := <-errCh; werr != nil { @@ -214,9 +216,10 @@ func (c *Client) UploadStreamWithSourceMetadata(ctx context.Context, name, mimeT if err != nil { return err } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/files?"+q.Encode(), body) + target := c.baseURL + "/v1/files?" + q.Encode() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, target, body) if err != nil { - return err + return requestBuildError(http.MethodPost, target, err) } if sourceJSON != "" { req.Header.Set(sourceMetadataHeader, sourceJSON) @@ -230,7 +233,7 @@ func (c *Client) UploadStreamWithSourceMetadata(ctx context.Context, name, mimeT c.attachAuth(req) resp, err := c.hc.Do(req) if err != nil { - return err + return gateTransportError(err) } defer resp.Body.Close() return decode(resp, out) @@ -239,14 +242,15 @@ func (c *Client) UploadStreamWithSourceMetadata(ctx context.Context, name, mimeT // DownloadStream returns a streaming reader for GET /v1/files/{id}/content. // Callers MUST close the returned ReadCloser. func (c *Client) DownloadStream(ctx context.Context, fileID string) (io.ReadCloser, string, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/v1/files/"+fileID+"/content", nil) + dlTarget := c.baseURL + "/v1/files/" + fileID + "/content" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, dlTarget, nil) if err != nil { - return nil, "", err + return nil, "", requestBuildError(http.MethodGet, dlTarget, err) } c.attachAuth(req) resp, err := c.hc.Do(req) if err != nil { - return nil, "", err + return nil, "", gateTransportError(err) } if resp.StatusCode >= 400 { defer resp.Body.Close() @@ -268,6 +272,40 @@ func marshalSourceMetadata(sourceMetadata *FileSourceMetadata) (string, error) { return string(raw), nil } +// requestBuildError and gateTransportError are the two halves of one rule: the +// configured base URL can carry credentials, and both http.NewRequestWithContext +// and http.Client.Do put that URL into the error they return. Go masks the +// password there but not the username, and masks nothing for a value it parses +// as an opaque scheme, so the URL goes through the shared gate instead. +// +// The wrappers keep the cause reachable through Unwrap so callers can still +// classify a timeout with errors.Is after the text has been rewritten. +func requestBuildError(method, target string, err error) error { + return &redactErr{ + cause: err, + message: fmt.Sprintf("%s %s: %s", method, redact.URL(target, redact.APIURLs), redact.Text(err.Error(), redact.APIURLs)), + } +} + +func gateTransportError(err error) error { + if err == nil { + return nil + } + return &redactErr{ + cause: err, + message: redact.TransportError(err, redact.APIURLs), + } +} + +type redactErr struct { + cause error + message string +} + +func (e *redactErr) Error() string { return e.message } + +func (e *redactErr) Unwrap() error { return e.cause } + func (c *Client) attachAuth(req *http.Request) { if c.token != "" { req.Header.Set("Authorization", "Bearer "+c.token) diff --git a/server/internal/apiclient/apiclient_test.go b/server/internal/apiclient/apiclient_test.go index 0d61000..def3d8a 100644 --- a/server/internal/apiclient/apiclient_test.go +++ b/server/internal/apiclient/apiclient_test.go @@ -155,3 +155,63 @@ func TestUploadStreamWithSourceMetadata(t *testing.T) { t.Fatal("source_metadata leaked into the request URL") } } + +func TestRequestBuildErrorRedactsCredentialedURL(t *testing.T) { + const secret = "bad-token-xyz" + err := New("http://admin:"+secret+"@ho st.example.com:1", "token").DoJSON( + context.Background(), + http.MethodGet, + "/v1/test", + nil, + nil, + ) + if err == nil { + t.Fatal("expected request construction to fail for malformed URL") + } + if strings.Contains(err.Error(), secret) { + t.Fatalf("request-build error leaked credential: %v", err) + } + if !strings.Contains(err.Error(), "REDACTED") { + t.Fatalf("request-build error should redact credentials: %v", err) + } +} + +// A URL whose scheme is really a username parses, so request construction +// succeeds and the failure comes from the transport instead. Every entry point +// has its own http.Client.Do site, so each needs its own case: covering request +// construction does not cover request execution. +func TestTransportErrorRedactsCredentialedURL(t *testing.T) { + const secret = "bad-token-xyz" + schemeless := "admin:" + secret + "@mem.invalid:8787" + + cases := []struct { + name string + call func(*Client) error + }{ + {"DoJSON", func(c *Client) error { + return c.DoJSON(context.Background(), http.MethodGet, "/v1/test", nil, nil) + }}, + {"UploadMultipart", func(c *Client) error { + return c.UploadMultipart(context.Background(), "f.txt", "text/plain", "", strings.NewReader("x"), nil, nil) + }}, + {"UploadStream", func(c *Client) error { + return c.UploadStream(context.Background(), "f.txt", "text/plain", "", 1, nil, strings.NewReader("x"), nil) + }}, + {"DownloadStream", func(c *Client) error { + _, _, err := c.DownloadStream(context.Background(), "file-1") + return err + }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := tc.call(New(schemeless, "token")) + if err == nil { + t.Fatal("expected the transport to fail for a schemeless base URL") + } + if strings.Contains(err.Error(), secret) { + t.Errorf("%s transport error leaked credential: %v", tc.name, err) + } + }) + } +} diff --git a/server/internal/redact/redact.go b/server/internal/redact/redact.go new file mode 100644 index 0000000..3652224 --- /dev/null +++ b/server/internal/redact/redact.go @@ -0,0 +1,157 @@ +// Package redact gates URLs on their way out of the process. +// +// The gate is fail-closed on purpose. A configured URL can carry a password in +// shapes that url.Parse does not report as userinfo: "admin:pw@host" parses as +// Scheme="admin", Opaque="pw@host", User=nil. So "the parser found no userinfo" +// is not evidence that a value is credential-free, and any implementation that +// gates on u.User == nil echoes the credential unchanged. A value the gate +// cannot positively prove safe is withheld whole. +// +// Scrubbing a message that already contains the URL is not a substitute: Go +// renders url.Error with %q, so a quote inside a password arrives escaped and a +// scanner that pairs quotes mis-pairs and replaces nothing. Callers hand text +// here instead and accept withholding when a piece cannot be verified. +package redact + +import ( + "errors" + "net/url" + "strings" +) + +// Placeholder replaces a value the gate cannot prove credential-free. It is a +// fixed token so an operator can tell withholding apart from a real host. +// Deliberately free of '<', '>' and '&': encoding/json escapes those, so an +// angle-bracketed marker would render differently in text and JSON output. +const Placeholder = "[withheld]" + +// UserMarker replaces the userinfo of a URL that is otherwise safe to echo. +// It uses only unreserved characters because url.User("***") would +// percent-encode the asterisks. +const UserMarker = "REDACTED" + +// APIURLs are the schemes a client base URL may legitimately use. +var APIURLs = []string{"http", "https"} + +// StoreURLs are the schemes memd logs: the database DSN and the queue DSN. +var StoreURLs = []string{"http", "https", "postgres", "postgresql", "redis", "rediss", "redis+unix", "unix"} + +// URL returns raw with userinfo replaced by UserMarker, or Placeholder when the +// value cannot be proven credential-free. +func URL(raw string, allowed []string) string { + safe, ok := rewrite(raw, allowed) + if !ok { + return Placeholder + } + return safe +} + +// Text renders diagnostic text — an error message, typically — so that no +// URL-shaped token inside it can carry userinfo out. A single unverifiable +// token withholds the entire message rather than trimming that token, because a +// delimiter inside a credential splits the text into pieces that no longer look +// like a URL, and the piece without the "@" is exactly the half that leaked. +// +// The gate proves the absence of userinfo, not of credentials: a secret in a +// query string (redis://host:6379/?password=x) parses cleanly with User == nil +// and is echoed. That shape is out of scope here and is reported as a residual. +func Text(msg string, allowed []string) string { + out := msg + for _, token := range urlTokens(msg) { + safe, ok := rewrite(token, allowed) + if !ok { + return Placeholder + } + if safe != token { + out = strings.Replace(out, token, safe, 1) + } + } + return out +} + +// TransportError renders an error returned by http.Client.Do. The URL travels +// through the gate rather than through Go's own masking, which strips the +// password but leaves the username, and the wrapped cause is kept so the +// message still names what failed. +func TransportError(err error, allowed []string) string { + if err == nil { + return "" + } + var ue *url.Error + if errors.As(err, &ue) && ue.Err != nil { + return Text(ue.Op, allowed) + " " + URL(ue.URL, allowed) + ": " + Text(ue.Err.Error(), allowed) + } + return Text(err.Error(), allowed) +} + +// rewrite reports whether raw is a URL we can prove carries no credential, and +// returns the form that is safe to echo. +func rewrite(raw string, allowed []string) (string, bool) { + if raw == "" { + return "", true + } + parsed, err := url.Parse(raw) + if err != nil { + return "", false + } + // Credentials hide in Opaque precisely when the scheme is really userinfo, + // and an empty or unknown scheme means we are not looking at a transport URL + // we can reason about. + if parsed.Opaque != "" || !allowedScheme(parsed.Scheme, allowed) { + return "", false + } + if parsed.User != nil { + parsed.User = url.User(UserMarker) + } + // What leaves the process is the re-serialised form, so verify that instead + // of trusting the first parse: String() can rebuild something different from + // the input, and an "@" surviving into the host means userinfo was never in + // the field we stripped. + rendered := parsed.String() + back, err := url.Parse(rendered) + if err != nil || back.Opaque != "" || back.Host != parsed.Host || + !strings.EqualFold(back.Scheme, parsed.Scheme) || strings.Contains(back.Host, "@") { + return "", false + } + if back.User != nil { + if _, hasPassword := back.User.Password(); hasPassword { + return "", false + } + if back.User.Username() != UserMarker { + return "", false + } + } + return rendered, true +} + +func allowedScheme(scheme string, allowed []string) bool { + for _, want := range allowed { + if strings.EqualFold(scheme, want) { + return true + } + } + return false +} + +// urlTokens returns the whitespace- and quote-delimited runs of msg that look +// like they could carry a host or userinfo. Splitting on delimiters is safe +// because any run produced this way still holds either the "@" or the "://" +// that marked the original as credential-shaped, unless the original held +// neither and was never a URL at all. +func urlTokens(msg string) []string { + var tokens []string + for _, token := range strings.FieldsFunc(msg, isDelimiter) { + if strings.Contains(token, "@") || strings.Contains(token, "://") { + tokens = append(tokens, token) + } + } + return tokens +} + +func isDelimiter(r rune) bool { + switch r { + case ' ', '\t', '\n', '\r', '"', '\'', '`', '(', ')', '[', ']', '{', '}', '<', '>', ',': + return true + } + return false +} diff --git a/server/internal/redact/redact_test.go b/server/internal/redact/redact_test.go new file mode 100644 index 0000000..a1f37a3 --- /dev/null +++ b/server/internal/redact/redact_test.go @@ -0,0 +1,277 @@ +package redact + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "testing" +) + +// secret marks every fixture below. No case may let it reach the returned +// string, and the tests fail closed on the marker rather than on a specific +// redaction shape. +const secret = "s3ntinel-p4ssw0rd" + +func TestURLWithholdsShapesTheParserCannotAttribute(t *testing.T) { + cases := []struct { + name string + raw string + allowed []string + want string + }{ + { + name: "userinfo on a recognised scheme", + raw: "http://admin:" + secret + "@mem.internal:8787", + allowed: APIURLs, + want: "http://REDACTED@mem.internal:8787", + }, + { + name: "username only", + raw: "http://admin@mem.internal:8787", + allowed: APIURLs, + want: "http://REDACTED@mem.internal:8787", + }, + { + name: "no credential at all is echoed unchanged", + raw: "http://localhost:8787", + allowed: APIURLs, + want: "http://localhost:8787", + }, + { + name: "empty value has nothing to leak", + raw: "", + allowed: APIURLs, + want: "", + }, + { + name: "store scheme is allowed for a DSN egress", + raw: "postgres://mem:" + secret + "@localhost:5432/mem?sslmode=disable", + allowed: StoreURLs, + want: "postgres://REDACTED@localhost:5432/mem?sslmode=disable", + }, + // The shape this package exists for: url.Parse succeeds, User is nil and + // the whole credential sits in Opaque, so a u.User != nil gate misses it. + { + name: "no scheme, credential in Opaque", + raw: "admin:" + secret + "@mem.internal:8787", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "scheme the egress does not use", + raw: "gopher://" + secret + "@mem.internal:8787", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "store scheme on an API egress", + raw: "postgres://mem:" + secret + "@localhost:5432/mem", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "parse failure with a space in the host", + raw: "http://admin:" + secret + "@ho st.example.com:8787", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "parse failure with a bad percent escape", + raw: "http://admin:" + secret + "@mem.internal:%zz", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "parse failure with a space in the password", + raw: "http://admin:" + secret + " x@mem.internal:8787", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "parse failure ending in an escape sign", + raw: "http://admin:" + secret + "@%", + allowed: APIURLs, + want: Placeholder, + }, + { + name: "out-of-range port still parses, so userinfo is stripped", + raw: "http://admin:" + secret + "@mem.internal:99999999", + allowed: APIURLs, + want: "http://REDACTED@mem.internal:99999999", + }, + { + name: "non-numeric port does not parse", + raw: "http://admin:" + secret + "@mem.internal:notaport", + allowed: APIURLs, + want: Placeholder, + }, + { + // No scheme is not a recognised transport scheme, so the adjudicated + // rule withholds it even though the credential did land in User. + name: "scheme-relative value has no scheme to check", + raw: "//admin:" + secret + "@mem.internal:8787", + allowed: APIURLs, + want: Placeholder, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := URL(tc.raw, tc.allowed) + if got != tc.want { + t.Errorf("URL(%q) = %q, want %q", tc.raw, got, tc.want) + } + if strings.Contains(got, secret) { + t.Errorf("URL(%q) = %q, leaks the sentinel", tc.raw, got) + } + }) + } +} + +func TestTextWithholdsWhenAnyCredentialShapedTokenIsUnverifiable(t *testing.T) { + cases := []struct { + name string + msg string + want string + }{ + { + name: "plain transport cause survives", + msg: `dial tcp: lookup mem.internal: no such host`, + want: `dial tcp: lookup mem.internal: no such host`, + }, + { + name: "well-formed credential URL is redacted in place", + msg: `Get "http://admin:` + secret + `@mem.internal:8787/healthz": dial tcp refused`, + want: `Get "http://REDACTED@mem.internal:8787/healthz": dial tcp refused`, + }, + { + name: "no-scheme shape withholds the whole line", + msg: `Get "admin:` + secret + `@mem.internal:8787": unsupported protocol scheme "admin"`, + want: Placeholder, + }, + { + name: "quote-escaped password withholds the whole line", + msg: `Get "http://admin:` + secret + `\"@mem.internal:8787": context deadline exceeded`, + want: Placeholder, + }, + { + name: "space-split password withholds the whole line", + msg: `Get "http://admin:` + secret + ` x@mem.internal:8787": dial tcp`, + want: Placeholder, + }, + { + name: "at sign in a path is not a credential", + msg: `GET https://mem.internal/v1/files/report%40mem.internal: permission denied`, + want: `GET https://mem.internal/v1/files/report%40mem.internal: permission denied`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Text(tc.msg, APIURLs) + if got != tc.want { + t.Errorf("Text(%q) = %q, want %q", tc.msg, got, tc.want) + } + if strings.Contains(got, secret) { + t.Errorf("Text(%q) = %q, leaks the sentinel", tc.msg, got) + } + }) + } +} + +// TestTextGatesAStoreDsnEmbeddedInAForeignError covers the shape that reaches +// memd's fatal log: asynq puts the whole DSN into its parse error and +// queue.NewClient wraps that verbatim. +func TestTextGatesAStoreDsnEmbeddedInAForeignError(t *testing.T) { + msg := `queue: parse redis url: asynq: could not parse redis uri: ` + + `parse "redis://:` + secret + `@ho st:6379/0": invalid character " " in host name` + + got := Text(msg, StoreURLs) + if strings.Contains(got, secret) { + t.Errorf("Text(%q) = %q, leaks the sentinel", msg, got) + } +} + +// TestTextKnownGapQueryParameterCredentialsAreEchoed characterizes a gap, it does +// not endorse it. The adjudicated rule is about userinfo, and a secret in a query +// parameter parses as an otherwise clean URL, so it is echoed. Closing it belongs +// in a separate decision; this test exists so whoever makes that decision sees a +// named expectation fail rather than rediscovering the behaviour by accident. +func TestTextKnownGapQueryParameterCredentialsAreEchoed(t *testing.T) { + cases := []string{ + "redis://queue.internal:6379/0?password=" + secret, + "postgres://mem@db.internal:5432/mem?sslmode=require&password=" + secret, + } + + for _, msg := range cases { + got := Text(msg, StoreURLs) + if !strings.Contains(got, secret) { + t.Errorf("query-string credential is no longer echoed; update this test to"+ + " assert withholding instead.\n in: %q\nout: %q", msg, got) + } + } +} + +func TestTransportErrorNamesTheFailureWithoutEchoingCredentials(t *testing.T) { + sentinel := "http://admin:" + secret + "@mem.internal:8787" + cases := []struct { + name string + err error + }{ + { + name: "url error carrying parsed userinfo", + err: &url.Error{Op: "Get", URL: sentinel, Err: errors.New("dial tcp: connection refused")}, + }, + { + name: "url error whose URL is not a transport scheme", + err: &url.Error{Op: "Get", Err: errors.New("unsupported protocol scheme \"admin\""), URL: "admin:" + secret + "@mem.internal:8787"}, + }, + { + name: "bare error mentioning the URL in prose", + err: fmt.Errorf("proxy returned 407 for %s", sentinel), + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := TransportError(tc.err, APIURLs) + if strings.Contains(got, secret) { + t.Errorf("TransportError(%v) = %q, leaks the sentinel", tc.err, got) + } + if got == "" { + t.Errorf("TransportError(%v) = %q, want the failure to stay diagnosable", tc.err, got) + } + }) + } + + if got := TransportError(nil, APIURLs); got != "" { + t.Errorf("TransportError(nil) = %q, want empty", got) + } +} + +// TestTransportErrorKeepsErrorIdentity pins that rendering does not replace the +// chain a caller classifies on: doctor maps a timeout to exit 5 via errors.Is on +// context.DeadlineExceeded, and that has to keep working after the text changes. +func TestTransportErrorKeepsErrorIdentity(t *testing.T) { + sentinel := "http://admin:" + secret + "@mem.internal:8787" + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return nil, &url.Error{Op: "Get", URL: sentinel, Err: context.DeadlineExceeded} + })} + _, err := client.Get(sentinel) + if err == nil { + t.Fatal("expected a transport failure") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("transport error lost its cause: %v", err) + } + if rendered := TransportError(err, APIURLs); strings.Contains(rendered, secret) { + t.Errorf("TransportError = %q, leaks the sentinel", rendered) + } +} + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } From d30f1ffd00148168bb9aa26ce1afb7e3e2ab4cdc Mon Sep 17 00:00:00 2001 From: waterbro-8 <318569545+waterbro-8@users.noreply.github.com> Date: Sun, 6 Sep 2026 09:47:32 +0800 Subject: [PATCH 4/6] fix(cli): route mem doctor's URL reporting through the shared gate `mem doctor` had its own copy of the redaction rule. Two of its shapes were wrong in the same way the API client was: `redactURL` returned the value unchanged whenever `url.Parse` succeeded without reporting userinfo, and `sanitizeProbeError` scanned error text for a quoted `://` substring, which cannot be made tight because `url.Error` renders with %q. Both now delegate to internal/redact, so the CLI, the API client and memd share one policy and there is no second implementation left to drift. TestRedactURLStripsUserinfo and the server_reachability assertion in TestDoctorMalformedServerURLDoesNotLeakCredentials required the value to come back as a cleaned URL. The gate withholds it instead, which is the stronger behavior, so both now accept either outcome while still asserting that neither the credential nor the raw URL appears. TestDoctorSchemelessServerURLDoesNotLeakCredentials is new and is what makes this change load-bearing: with the previous scrubber restored, it is the only test that fails. --- server/cmd/mem/cmds_doctor.go | 66 ++++---------------------- server/cmd/mem/cmds_doctor_test.go | 44 +++++++++++++++-- server/internal/apiclient/apiclient.go | 11 +++++ 3 files changed, 61 insertions(+), 60 deletions(-) diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go index 92d78bf..df3e06f 100644 --- a/server/cmd/mem/cmds_doctor.go +++ b/server/cmd/mem/cmds_doctor.go @@ -6,12 +6,12 @@ import ( "errors" "fmt" "net/http" - "net/url" "os" "strings" "time" "github.com/PeterGuy326/mem/server/internal/apiclient" + "github.com/PeterGuy326/mem/server/internal/redact" "github.com/spf13/cobra" ) @@ -348,65 +348,19 @@ func deployPathHint() string { return "start the documented container path: deploy/compose, see docs/DEPLOYMENT.md" } -// redactURL strips userinfo so a URL that carries credentials cannot be echoed -// into a report that an operator will paste into an issue. The marker uses only -// unreserved characters, because url.User("***") would percent-encode it. +// redactURL gates a configured URL on its way into a report an operator +// will paste into an issue. The policy lives in internal/redact so the CLI, the +// API client and memd share one implementation: a URL that can be positively +// proven to be a credential-free transport URL is echoed with its userinfo +// replaced, and one that cannot is withheld whole rather than scrubbed. func redactURL(raw string) string { - u, err := url.Parse(raw) - if err == nil && 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 - } - 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 + return redact.URL(raw, redact.APIURLs) } +// sanitizeProbeError renders a probe failure without letting the configured URL +// out, including the shapes url.Parse reports as neither an error nor userinfo. func sanitizeProbeError(err error) string { - if err == nil { - return "" - } - msg := err.Error() - const quoted = "\"" - for start := 0; ; { - left := strings.Index(msg[start:], quoted) - if left < 0 { - return msg - } - left += start - right := strings.Index(msg[left+1:], quoted) - if right < 0 { - return msg - } - right += left + 1 - token := msg[left+1 : right] - if strings.Contains(token, "://") { - if redacted := redactURL(token); redacted != token { - msg = msg[:left+1] + redacted + msg[right:] - } - } - start = right + 1 - } + return redact.TransportError(err, redact.APIURLs) } func printDoctorReport(cmd *cobra.Command, r doctorReport) { diff --git a/server/cmd/mem/cmds_doctor_test.go b/server/cmd/mem/cmds_doctor_test.go index b4cbffb..b9743db 100644 --- a/server/cmd/mem/cmds_doctor_test.go +++ b/server/cmd/mem/cmds_doctor_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/PeterGuy326/mem/server/internal/redact" "io" "net/http" "net/http/httptest" @@ -414,8 +415,9 @@ func TestRedactURLStripsUserinfo(t *testing.T) { if strings.Contains(got, secret) { t.Errorf("redactURL = %q, still carries the malformed-password value", got) } - if !strings.Contains(got, "REDACTED@ho st.example.com:8787") { - t.Errorf("redactURL = %q, want malformed URLs to redact their userinfo", got) + if got != redact.Placeholder && !strings.Contains(got, "REDACTED@ho st.example.com:8787") { + t.Errorf("redactURL = %q, want userinfo replaced, or the whole value withheld "+ + "once it can no longer be proven a transport URL", got) } plain := "http://localhost:8787" if redactURL(plain) != plain { @@ -451,8 +453,42 @@ func TestDoctorMalformedServerURLDoesNotLeakCredentials(t *testing.T) { if strings.Contains(reach.Detail, malformed) { t.Errorf("server_reachability detail leaked raw URL: %s", reach.Detail) } - if !strings.Contains(reach.Detail, "REDACTED") { - t.Errorf("server_reachability detail should redact credentials: %s", reach.Detail) + if !strings.Contains(reach.Detail, redact.UserMarker) && + !strings.Contains(reach.Detail, redact.Placeholder) { + t.Errorf("server_reachability detail should redact or withhold credentials: %s", reach.Detail) + } + } + } +} + +// TestDoctorSchemelessServerURLDoesNotLeakCredentials covers the shape the +// previous scrubber could not see: url.Parse succeeds on it and reports no +// userinfo, because "admin" is read as the scheme and the credential lands in +// Opaque. A gate that keys on User != nil echoes it verbatim. +func TestDoctorSchemelessServerURLDoesNotLeakCredentials(t *testing.T) { + const secret = "schemeless-psswd" + schemeless := "admin:" + secret + "@mem.invalid:8787" + configureDoctor(t, schemeless, "tok", true) + + for _, format := range []string{"json", "text"} { + stdout, stderr, err := execDoctor(t, "--format", format) + if err == nil { + t.Fatalf("doctor should fail against an unreachable schemeless URL in %s format\n%s", format, stdout) + } + if strings.Contains(stdout, secret) || strings.Contains(stderr, secret) { + t.Errorf("%s output leaked the credential: stdout=%s stderr=%s", format, stdout, stderr) + } + if strings.Contains(stdout, schemeless) || strings.Contains(stderr, schemeless) { + t.Errorf("%s output echoed the raw schemeless URL: stdout=%s stderr=%s", format, stdout, stderr) + } + if format == "json" { + rep := decodeReport(t, stdout) + if strings.Contains(rep.Server, secret) { + t.Errorf("report server field leaked the credential: %s", rep.Server) + } + reach := checkByName(t, rep, "server_reachability") + if strings.Contains(reach.Detail, secret) || strings.Contains(reach.Hint, secret) { + t.Errorf("detail/hint leaked the credential: detail=%s hint=%s", reach.Detail, reach.Hint) } } } diff --git a/server/internal/apiclient/apiclient.go b/server/internal/apiclient/apiclient.go index b9b47cd..df01390 100644 --- a/server/internal/apiclient/apiclient.go +++ b/server/internal/apiclient/apiclient.go @@ -287,6 +287,17 @@ func requestBuildError(method, target string, err error) error { } } +// newRequest is the construction site for the requests that are issued outside +// the DoJSON and Upload helpers, so a base URL that cannot be parsed fails +// through the same gate here as it does there. +func (c *Client) newRequest(ctx context.Context, method, target string, body io.Reader) (*http.Request, error) { + req, err := http.NewRequestWithContext(ctx, method, target, body) + if err != nil { + return nil, requestBuildError(method, target, err) + } + return req, nil +} + func gateTransportError(err error) error { if err == nil { return nil From c9e0e635abf20ce221c3cdb829599cfab37ddca3 Mon Sep 17 00:00:00 2001 From: waterbro-8 <318569545+waterbro-8@users.noreply.github.com> Date: Sun, 6 Sep 2026 02:05:26 +0000 Subject: [PATCH 5/6] docs: correct doctor's stated secret guarantee The CHANGELOG, DEPLOYMENT.md and cmds_doctor.go all claimed the doctor report contains no secret value. Measured on this tree, a credential in the server URL's query string is still printed verbatim, because it parses as a clean URL with no userinfo and so is outside what the gate proves. Narrow the claim to what is actually guaranteed and name the residual. --- CHANGELOG.md | 7 +++++-- docs/DEPLOYMENT.md | 14 +++++++++++--- server/cmd/mem/cmds_doctor.go | 8 +++++--- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36d939b..2f4773f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,11 @@ The project publishes 0.x prerelease versions; a stable release line is not yet impossible is reported as `skipped` instead of guessed. It issues only `GET` requests and never writes configuration, starts a container, or installs a dependency; `--format json` emits the `mem.doctor` v1 document described by - `docs/schemas/mem-doctor.v1.schema.json`, with no secret value in it (the - configured URL is reported with credentials removed). See + `docs/schemas/mem-doctor.v1.schema.json`. A token is described only by where it + came from, and a configured URL carrying credentials in its userinfo is + redacted or, when it cannot be proven to be a credential-free transport URL, + withheld whole. The document is not guaranteed free of every secret: a + credential supplied as a URL query parameter is still reported verbatim. See `docs/DEPLOYMENT.md`. - First-run guidance: a command that fails because no credential exists now says so on a machine with no configuration at all by naming the documented diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 61cad73..42ab3cf 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -190,9 +190,17 @@ computable in builds that do not inject a CLI version, which today includes release builds, so the check reports that limit instead of claiming agreement. `--format json` emits the `mem.doctor` v1 document validated by -[`schemas/mem-doctor.v1.schema.json`](schemas/mem-doctor.v1.schema.json). It -contains no secret value: the configured URL is reported with any credentials -removed, and a token is described only by where it came from. +[`schemas/mem-doctor.v1.schema.json`](schemas/mem-doctor.v1.schema.json), and a +token is described only by where it came from. A configured URL carrying +credentials in its userinfo is reported with those credentials replaced by +`REDACTED`, and a URL that cannot be proven to be a credential-free transport URL +is withheld whole as `[withheld]` rather than partially trimmed. + +This is not a guarantee that the document contains no secret. A credential +supplied as a URL **query parameter** — for example +`http://mem.internal:8787?password=…` — parses as a clean URL with no userinfo, +so it is reported verbatim. Do not treat a doctor report as safe to publish +without reading it first if your server URL carries a secret outside userinfo. ### First account and login diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go index df3e06f..1c7204a 100644 --- a/server/cmd/mem/cmds_doctor.go +++ b/server/cmd/mem/cmds_doctor.go @@ -28,9 +28,11 @@ import ( // version_skew GET /v1/version // // REQ-003 keeps this strictly diagnostic: every request is a GET, nothing is -// created, no dependency is installed, no Docker or compose command is issued, -// and no secret value or DSN is ever printed (the configured URL is reported -// with any userinfo removed). +// created, no dependency is installed, no Docker or compose command is issued. +// URLs pass through internal/redact on the way out: userinfo is redacted, and a +// URL that cannot be proven credential-free is withheld whole. That covers +// secrets in userinfo and DSNs, not a credential placed in the query string, +// which parses as a clean URL and is still printed. // doctorContract and doctorSchemaVersion follow the repo convention of naming a // machine-readable surface and versioning it, mirroring docs/schemas. From e0b7972b4fe5951a754e1ca8329e2edcbfba2158 Mon Sep 17 00:00:00 2001 From: waterbro-8 <318569545+waterbro-8@users.noreply.github.com> Date: Tue, 8 Sep 2026 10:42:46 +0800 Subject: [PATCH 6/6] fix(redact): blank query values and withhold URLs with a fragment pgx reads postgres://host/db?password=... as the real password, so a URL that parses cleanly with no userinfo is not thereby proven credential-free. The gate masked userinfo and let the parameter through, which put a database password in memd's startup log and in three fields of a doctor report. Every query value now goes to REDACTED and only the parameter names survive, so a log line still says which settings are on. A fragment has no name to keep, so such a URL is withheld whole. No key-name blacklist: that is fail-open. --- CHANGELOG.md | 14 ++++----- docs/DEPLOYMENT.md | 17 +++++------ server/cmd/mem/cmds_doctor.go | 7 ++--- server/cmd/memd/main_test.go | 4 +-- server/internal/redact/redact.go | 37 ++++++++++++++++++---- server/internal/redact/redact_test.go | 44 ++++++++++++++++++--------- 6 files changed, 79 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f4773f..bb087fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,11 +19,10 @@ The project publishes 0.x prerelease versions; a stable release line is not yet requests and never writes configuration, starts a container, or installs a dependency; `--format json` emits the `mem.doctor` v1 document described by `docs/schemas/mem-doctor.v1.schema.json`. A token is described only by where it - came from, and a configured URL carrying credentials in its userinfo is - redacted or, when it cannot be proven to be a credential-free transport URL, - withheld whole. The document is not guaranteed free of every secret: a - credential supplied as a URL query parameter is still reported verbatim. See - `docs/DEPLOYMENT.md`. + came from, and a configured URL has its userinfo and its query parameter values + replaced by `REDACTED` — a credential in a query parameter is the shape pgx + accepts as a real password — or, when the URL cannot be proven to be a + credential-free transport URL, is withheld whole. See `docs/DEPLOYMENT.md`. - First-run guidance: a command that fails because no credential exists now says so on a machine with no configuration at all by naming the documented deployment path (`deploy/compose`, `docs/DEPLOYMENT.md`), instead of telling @@ -80,8 +79,9 @@ The project publishes 0.x prerelease versions; a stable release line is not yet error text, which cannot be made tight: `url.Error` renders with `%q`, so a quote inside a password arrives escaped and a scanner that pairs quotes mis-pairs and replaces nothing. Withholding costs some diagnosability by design; - why a request failed is still reported. A credential supplied as a query - parameter (`?password=`) is still echoed and is tracked separately. + why a request failed is still reported, and a DSN still names the parameters it + sets — every query parameter *value* is replaced by `REDACTED`, including + `?password=`, which pgx honours as the real password. - The npm installer no longer aborts a concurrent first run on Windows. The per-asset cache lock previously treated only `EEXIST` as contention, but a contended `mkdir` on Windows may raise `EPERM` or `EACCES`, so a process diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 42ab3cf..25d0b57 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -191,16 +191,13 @@ release builds, so the check reports that limit instead of claiming agreement. `--format json` emits the `mem.doctor` v1 document validated by [`schemas/mem-doctor.v1.schema.json`](schemas/mem-doctor.v1.schema.json), and a -token is described only by where it came from. A configured URL carrying -credentials in its userinfo is reported with those credentials replaced by -`REDACTED`, and a URL that cannot be proven to be a credential-free transport URL -is withheld whole as `[withheld]` rather than partially trimmed. - -This is not a guarantee that the document contains no secret. A credential -supplied as a URL **query parameter** — for example -`http://mem.internal:8787?password=…` — parses as a clean URL with no userinfo, -so it is reported verbatim. Do not treat a doctor report as safe to publish -without reading it first if your server URL carries a secret outside userinfo. +token is described only by where it came from. For a configured URL, userinfo and +every query parameter **value** are replaced by `REDACTED` — the parameter names +survive so the report still says which settings are on — and a URL that cannot be +proven to be a credential-free transport URL is withheld whole as `[withheld]` +rather than partially trimmed. A secret supplied as a query parameter +(`http://mem.internal:8787?password=…`) is therefore not reported, which matters +because pgx accepts `postgres://host/db?password=…` as the real password. ### First account and login diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go index 1c7204a..e7f1566 100644 --- a/server/cmd/mem/cmds_doctor.go +++ b/server/cmd/mem/cmds_doctor.go @@ -29,10 +29,9 @@ import ( // // REQ-003 keeps this strictly diagnostic: every request is a GET, nothing is // created, no dependency is installed, no Docker or compose command is issued. -// URLs pass through internal/redact on the way out: userinfo is redacted, and a -// URL that cannot be proven credential-free is withheld whole. That covers -// secrets in userinfo and DSNs, not a credential placed in the query string, -// which parses as a clean URL and is still printed. +// URLs pass through internal/redact on the way out: userinfo is redacted, query +// parameter values are blanked, and a URL that cannot be proven credential-free +// is withheld whole. // doctorContract and doctorSchemaVersion follow the repo convention of naming a // machine-readable surface and versioning it, mirroring docs/schemas. diff --git a/server/cmd/memd/main_test.go b/server/cmd/memd/main_test.go index 6fef4b5..eeaff4a 100644 --- a/server/cmd/memd/main_test.go +++ b/server/cmd/memd/main_test.go @@ -21,7 +21,7 @@ func TestRedactURLCredentials(t *testing.T) { { name: "postgres", raw: "postgres://mem:database-secret@postgres:5432/mem?sslmode=disable", - want: "postgres://REDACTED@postgres:5432/mem?sslmode=disable", + want: "postgres://REDACTED@postgres:5432/mem?sslmode=REDACTED", }, { name: "redis", @@ -33,7 +33,7 @@ func TestRedactURLCredentials(t *testing.T) { // password; the gate treats any userinfo as a credential. name: "username only", raw: "postgres://mem@postgres:5432/mem?sslmode=disable", - want: "postgres://REDACTED@postgres:5432/mem?sslmode=disable", + want: "postgres://REDACTED@postgres:5432/mem?sslmode=REDACTED", }, { // url.Parse reads the scheme as "redis" and parks the rest in Opaque, diff --git a/server/internal/redact/redact.go b/server/internal/redact/redact.go index 3652224..c324715 100644 --- a/server/internal/redact/redact.go +++ b/server/internal/redact/redact.go @@ -25,9 +25,10 @@ import ( // angle-bracketed marker would render differently in text and JSON output. const Placeholder = "[withheld]" -// UserMarker replaces the userinfo of a URL that is otherwise safe to echo. -// It uses only unreserved characters because url.User("***") would -// percent-encode the asterisks. +// UserMarker replaces the userinfo of a URL, and the value of every query +// parameter, when the rest of the URL is safe to echo. It uses only unreserved +// characters because url.User("***") would percent-encode the asterisks and +// url.Values.Encode() would do the same to a marked-up query value. const UserMarker = "REDACTED" // APIURLs are the schemes a client base URL may legitimately use. @@ -52,9 +53,9 @@ func URL(raw string, allowed []string) string { // delimiter inside a credential splits the text into pieces that no longer look // like a URL, and the piece without the "@" is exactly the half that leaked. // -// The gate proves the absence of userinfo, not of credentials: a secret in a -// query string (redis://host:6379/?password=x) parses cleanly with User == nil -// and is echoed. That shape is out of scope here and is reported as a residual. +// Query and fragment values are part of the same problem: pgx honours +// postgres://host/db?password=x as the real password, so a value that parses as +// a clean URL is not thereby proven credential-free. func Text(msg string, allowed []string) string { out := msg for _, token := range urlTokens(msg) { @@ -103,6 +104,30 @@ func rewrite(raw string, allowed []string) (string, bool) { if parsed.User != nil { parsed.User = url.User(UserMarker) } + // A credential can travel as a connection parameter, and pgx honours + // postgres://host/db?password=… as the real password. Blanking only the keys + // that look secret would claim that we can prove some other value is not a + // credential, which is the claim this package refuses to make, so every query + // value goes and only the parameter names survive. A fragment has no name to + // keep, so it withholds the URL. + // + // ponytail: that costs ?sslmode=disable its value in memd's startup log. If + // it costs someone a debugging minute, keep an allowlist of parameters that + // cannot carry a secret and fail every unknown one to the marker. + if parsed.RawQuery != "" { + q, err := url.ParseQuery(parsed.RawQuery) + if err != nil { + return "", false + } + blanked := make(url.Values, len(q)) + for key := range q { + blanked[key] = []string{UserMarker} + } + parsed.RawQuery = blanked.Encode() + } + if parsed.Fragment != "" { + return "", false + } // What leaves the process is the re-serialised form, so verify that instead // of trusting the first parse: String() can rebuild something different from // the input, and an "@" surviving into the host means userinfo was never in diff --git a/server/internal/redact/redact_test.go b/server/internal/redact/redact_test.go index a1f37a3..1f6392b 100644 --- a/server/internal/redact/redact_test.go +++ b/server/internal/redact/redact_test.go @@ -50,7 +50,7 @@ func TestURLWithholdsShapesTheParserCannotAttribute(t *testing.T) { name: "store scheme is allowed for a DSN egress", raw: "postgres://mem:" + secret + "@localhost:5432/mem?sslmode=disable", allowed: StoreURLs, - want: "postgres://REDACTED@localhost:5432/mem?sslmode=disable", + want: "postgres://REDACTED@localhost:5432/mem?sslmode=REDACTED", }, // The shape this package exists for: url.Parse succeeds, User is nil and // the whole credential sits in Opaque, so a u.User != nil gate misses it. @@ -195,22 +195,36 @@ func TestTextGatesAStoreDsnEmbeddedInAForeignError(t *testing.T) { } } -// TestTextKnownGapQueryParameterCredentialsAreEchoed characterizes a gap, it does -// not endorse it. The adjudicated rule is about userinfo, and a secret in a query -// parameter parses as an otherwise clean URL, so it is echoed. Closing it belongs -// in a separate decision; this test exists so whoever makes that decision sees a -// named expectation fail rather than rediscovering the behaviour by accident. -func TestTextKnownGapQueryParameterCredentialsAreEchoed(t *testing.T) { - cases := []string{ - "redis://queue.internal:6379/0?password=" + secret, - "postgres://mem@db.internal:5432/mem?sslmode=require&password=" + secret, +// TestQueryAndFragmentCredentialsAreWithheld pins the shape the userinfo gate +// used to miss: pgx honours postgres://host/db?password=… as a real password, so +// a URL that parses cleanly with User == nil is not thereby proven safe. Names +// of parameters survive so a log line still says which settings are on; no value +// does, and a bare secret in a fragment withholds the URL. +func TestQueryAndFragmentCredentialsAreWithheld(t *testing.T) { + cases := []struct { + raw string + want string + }{ + { + raw: "redis://queue.internal:6379/0?password=" + secret, + want: "redis://queue.internal:6379/0?password=REDACTED", + }, + { + raw: "postgres://mem@db.internal:5432/mem?sslmode=require&password=" + secret, + want: "postgres://REDACTED@db.internal:5432/mem?password=REDACTED&sslmode=REDACTED", + }, + { + raw: "redis://queue.internal:6379/0#" + secret, + want: Placeholder, + }, } - for _, msg := range cases { - got := Text(msg, StoreURLs) - if !strings.Contains(got, secret) { - t.Errorf("query-string credential is no longer echoed; update this test to"+ - " assert withholding instead.\n in: %q\nout: %q", msg, got) + for _, tc := range cases { + if got := URL(tc.raw, StoreURLs); got != tc.want { + t.Errorf("URL(%q) = %q, want %q", tc.raw, got, tc.want) + } + if got := Text(tc.raw, StoreURLs); strings.Contains(got, secret) { + t.Errorf("Text(%q) = %q, leaks the sentinel", tc.raw, got) } } }