diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e6107e..7bdac57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ 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`. A token is described only by where it + 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 + somebody to log in against a server that is not running yet. Hosts that already + have a configuration keep the previous, shorter hint. + ### Changed - Migrate GitHub repository, Release, issue, badge, and raw-content coordinates @@ -53,6 +75,22 @@ The project publishes 0.x prerelease versions; a stable release line is not yet ### 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, 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 801e41d..25d0b57 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -163,6 +163,42 @@ 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), and a +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 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..e7f1566 --- /dev/null +++ b/server/cmd/mem/cmds_doctor.go @@ -0,0 +1,377 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "os" + "strings" + "time" + + "github.com/PeterGuy326/mem/server/internal/apiclient" + "github.com/PeterGuy326/mem/server/internal/redact" + "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. +// 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. +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, + } + + reachCtx, cancel := context.WithTimeout(cmd.Context(), timeout) + reach := probeReachability(reachCtx, cfg.Server) + cancel() + 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: + 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) + + 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: " + sanitizeProbeError(err), 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 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 { + 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 { + return redact.TransportError(err, redact.APIURLs) +} + +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..b9743db --- /dev/null +++ b/server/cmd/mem/cmds_doctor_test.go @@ -0,0 +1,748 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "github.com/PeterGuy326/mem/server/internal/redact" + "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) + } + 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 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 { + 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, 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) + } + } + } +} + +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" + } + ] +} 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..eeaff4a 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=REDACTED", }, { 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=REDACTED", + }, + { + // 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..df01390 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,51 @@ 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)), + } +} + +// 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 + } + 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/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(), diff --git a/server/internal/redact/redact.go b/server/internal/redact/redact.go new file mode 100644 index 0000000..c324715 --- /dev/null +++ b/server/internal/redact/redact.go @@ -0,0 +1,182 @@ +// 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, 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. +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. +// +// 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) { + 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) + } + // 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 + // 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..1f6392b --- /dev/null +++ b/server/internal/redact/redact_test.go @@ -0,0 +1,291 @@ +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=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. + { + 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) + } +} + +// 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 _, 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) + } + } +} + +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) }