From 01cd6c59829e7c75732fded825042460cc4b5916 Mon Sep 17 00:00:00 2001 From: GGOBP Date: Wed, 12 Aug 2026 11:32:35 +0900 Subject: [PATCH] fix: bootstrap PostgreSQL without a service manager (closes #823) Fall back to a user-owned initdb/pg_ctl cluster under ~/.mxcli/postgres when no service-managed PostgreSQL becomes ready. Provision through its private Unix socket without requiring a postgres OS account or sudo, while retaining the sudo path for system clusters. Harden the fallback for both review rounds: bound service probes, remove the invalid pg_ctlcluster placeholder, avoid competing with an occupied port, use SCRAM on TCP, enforce 0700 socket access, persist the endpoint in postgresql.conf, reject legacy host-trust clusters, and validate running-cluster endpoints and ports. Add focused regression coverage and update the run-local skill, user documentation, changelog, and fix-issue record. --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/run-local.md | 10 +- CHANGELOG.md | 1 + cmd/mxcli/docker/ensuredb.go | 428 +++++++++++++++++++-- cmd/mxcli/docker/ensuredb_test.go | 599 +++++++++++++++++++++++++++++ docs-site/src/tools/run-local.md | 12 +- 6 files changed, 1012 insertions(+), 39 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index 4bc455552..0513a261b 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -473,3 +473,4 @@ extracting `OffsetExpression`/`LimitExpression`. | An association's line anchors — where the connector attaches to the entity boxes in the domain model editor — are absent from `DESCRIBE ASSOCIATION`, and manual adjustments made in Studio Pro do not survive an mxcli round trip | `DomainModels$Association.ParentConnection`/`ChildConnection` (the string `"x;y"`) were **hardcoded** to `"0;50"`/`"100;50"` in BOTH writers and never read by either parser. Because every association write rebuilds the whole element, this was not an omission but active destruction: a documentation-only `alter association … set comment` reset them | `sdk/domainmodel/connection.go` (new: `ParseConnectionPoint`/`FormatConnectionPoint`, `Default*Connection`), `sdk/domainmodel/domainmodel.go` (fields → `*model.Point`), `sdk/mpr/parser_domainmodel.go` + `sdk/mpr/writer_domainmodel.go`, `mdl/backend/modelsdk/domainmodel.go` + `domainmodel_write.go`, `mdl/executor/cmd_associations.go` (`describeConnectionPoints`) | **A feature request that says "X is not exposed" may be hiding "X is destroyed"** — check the write path before scoping the read path. The A/B that settled it: a blank 11.13 app's own `Administration.AccountPasswordData_Account` stores `0;54/100;54`, so a Studio-Pro-authored association is a free fixture for "did mxcli overwrite this?" — no Studio Pro needed. **Learn the value's constraints from the LOADER, not from the shape**: hand-patch and run `mx check` — `"0.5;50"` dies with `StorageLoadException` (integers required) while `"0;500"` and `"-20;50"` load with 0 errors (no range check), so out-of-range values must round-trip untouched. **A zero value is not an absent value** — `{0,0}` is a real anchor (top-left), which forces the field to be a POINTER; a plain `model.Point` cannot distinguish "unset" from "top-left" and would silently rewrite it. **Fix both engines**: they share the semantic model, and a fix in one is invisible to a user on the other. **Emit unauthorable data as a COMMENT** — DESCRIBE output must stay re-executable, and inventing syntax (`@anchor(parent: bottom-left, …)`) would bake in a vocabulary the storage does not have: the pair is CONTINUOUS, not 8 named anchors (observed x values 0 9 11 17 18 47 49 50 65 77 78 84 87 100). **The marketplace is the sample** when you need to know what Studio Pro actually writes: `mxcli marketplace download ` gives real Mendix-authored models, and a module .mpk holds either a raw BSON `project.mpr` or an MPR v1 SQLite one — 88 coordinate pairs from three modules turned "looks like percentages" into a measurement (all 0..100; 85 of 88 pin one coordinate to exactly 0 or 100). **Rule a unit out from the model, not the values**: pixels is impossible because `DomainModels$EntityImpl` stores only `Location` and NO size — the box is sized by the editor from the name and attribute list, so a pixel anchor would have nothing to measure against. Not applicable to `CrossAssociation`, which has no connection properties and crashes Studio Pro if given them (#50). Tests `sdk/domainmodel/connection_test.go`, `mdl/backend/modelsdk/association_connection_test.go`, `sdk/mpr/writer_domainmodel_test.go`, example `mdl-examples/bug-tests/872-association-line-anchors.mdl`. upstream #872 | | An association's line anchors can be preserved but not AUTHORED — a scripted domain model cannot lay out its own connector lines, so `@Position(x, y)` gets you boxes and nothing gets you the lines between them | Feature gap, not a defect. `DomainModels$Association.ParentConnection`/`ChildConnection` had no MDL surface | `mdl/grammar/domains/MDLDomainModel.g4` (`SET ANCHOR`/`anchorPoint` — the ONLY grammar change), `mdl/visitor/visitor_association.go` (`anchorAnnotation`, `annotationParenPoint`, `anchorCoord`), `mdl/ast/ast_association.go` (`FromAnchor`/`ToAnchor` on both create and alter), `mdl/executor/cmd_associations.go` (`applyAnchors`, `describeConnectionPoints`) | **Look for an existing annotation before inventing one** — `@anchor(from:, to:)` already existed for microflow sequence flows, asking the same question (where does the connector attach), and `annotationParamName` already admitted FROM and TO, and `(x, y)` was already `annotationParenValue`: CREATE needed **zero** grammar. The two forms cannot be confused because the microflow one names its inner params (`(from: right, to: left)`) while a coordinate pair is positional. **Let the storage pick the value type**: the measured pair is continuous (x takes 14 distinct values across 88 samples), so named anchors were never an option — see the preservation row above for how that was established. **Silence must mean "preserve", not "default"** — naming one end sets it and omitting one keeps what is stored, which is what stops a `create or modify association` about the delete behaviour from flattening a hand-tuned line; the AST carries POINTERS so "not mentioned" and "mentioned as (0, 0)" stay distinguishable. **Reject what the LOADER rejects, at check time**: a fractional coordinate must error, not be truncated to 0 — Mendix refuses to open such a project, and a silently-wrong value in a file that still loads is the worse failure. **Prove DESCRIBE round-trips by parsing its own output** — asserting on a string literal passes against a formatter emitting something nothing can read. Tests `mdl/visitor/visitor_association_anchor_test.go`, `mdl/executor/cmd_associations_anchor_test.go`, example `mdl-examples/bug-tests/872-association-line-anchors.mdl`. upstream #872 | | `ALTER PAGE` over `--mcp` fails against Studio Pro **11.13** with `pg_patch_page: … PROP_NOT_PRIMITIVE: Property 'widgets' is not a primitive property`. `CREATE PAGE` is fine; the page itself is left intact | 11.13 gave `pg_read_page` a **`depth` argument defaulting to 4**, replacing anything deeper with the literal string `"..."`. ALTER PAGE is read-modify-**replace-whole-page**, so the truncated read went straight back as the new page body. Measured live: `Administration.Account_Overview` read 32,594 bytes at full depth but **1,052 bytes** at the default, its entire tree reduced to `{"widgets":["...","..."]}`. Every ordinary page truncates — three of three PgTest pages did | `mdl/backend/mcp/page.go` (`pgReadPage`, `pgReadFullDepth`, `hasTruncationSentinel`), `mdl/backend/mcp/client.go` (`SupportsToolArg`) | Request the full depth, and **guard rather than trust it**: refuse a read still carrying the sentinel instead of letting a partial page reach a write (ADR-0005 guard-don't-drop). Two traps. (1) **Do not send `depth` unconditionally** — 11.11/11.12 declare `pg_read_page` `additionalProperties:false` without it, so the whole call fails; gate on a live `tools/list` probe of the tool's input schema, because `serverInfo.version` is frozen at `1.0.0` across 11.11/11.12/11.13 and cannot discriminate releases. (2) **Match the sentinel only as an array element** — a caption or title legitimately reading `"..."` is real content, and a naive substring scan rejects valid pages. The release notes announced none of this, exactly as 11.12 silently removed `pg_write_page` (#697): on any Studio Pro upgrade, re-probe `tools/list` and diff the input schemas, not just the tool names. Tests `mdl/backend/mcp/page_depth_test.go`; controls: stub the depth arg (full-depth test fails) and stub the guard (truncation test fails) | +| `mxcli run --ensure-db` fails to start PostgreSQL when no service manager becomes ready (e.g. Arch): `exec: "pg_ctlcluster": executable file not found in $PATH`, though `initdb`/`pg_ctl`/`psql` are present | `startLocalPostgres` only knew the `service`/`pg_ctlcluster` helpers (the latter a placeholder that could never run), and role/database provisioning assumed `sudo -u postgres`, which a user-owned cluster does not need | `cmd/mxcli/docker/ensuredb.go` (`startLocalPostgres`, `startUserCluster`, `resolveSuperuser`) | Fall back to a user-owned `initdb`/`pg_ctl` cluster under `~/.mxcli/postgres`, but never initialize a competitor when the requested TCP port is already owned by a service-started server that is still recovering. Enforce the passwordless-superuser boundary instead of assuming it: explicitly tighten an existing socket directory to `0700`, set the socket itself to `0700`, and persist `listen_addresses`/`port`/`unix_socket_directories` in `postgresql.conf` so a plain later `pg_ctl start` stays safe. Refuse, with a cleanup path, a cluster made by the earlier development revision if its host authentication is still `trust`. Reuse a running cluster only when both the port and socket directory in `postmaster.pid` match. Keep `sudo -u postgres` for system clusters. Tests stub every external tool and exercise slow service startup, legacy-cluster refusal, permissions, persisted settings, and repeated/running cases. Issue #823 | diff --git a/.claude/skills/mendix/run-local.md b/.claude/skills/mendix/run-local.md index c9513672a..ece332191 100644 --- a/.claude/skills/mendix/run-local.md +++ b/.claude/skills/mendix/run-local.md @@ -50,8 +50,14 @@ association catalog only at startup; behavioural changes are hot-reloaded. - A **PostgreSQL** database (defaults: `127.0.0.1:5432`, user `mendix`, db derived from the project name; override with `--db-host/--db-name/--db-user/--db-password`). - **`--ensure-db`** provisions it for a fresh session: starts local Postgres if the - port is down and creates the role + database if missing (local superuser via - `sudo -u postgres`). Remote hosts are only checked, not provisioned. + port is down and creates the role + database if missing. It uses a service + manager, or a user-owned `initdb`/`pg_ctl` cluster under `~/.mxcli/postgres` + when no service becomes ready (e.g. Arch) — needing no `postgres` OS account or `sudo`. + Remote hosts are only checked, not provisioned. + The user-owned cluster persists across sessions; its server log is + `~/.mxcli/postgres/server.log`. Stop it with + `pg_ctl -D "$HOME/.mxcli/postgres/data" stop`. To remove it, stop it first and + then delete `~/.mxcli/postgres` (this permanently deletes its databases). - Without `--ensure-db`, create it once and the command errors if it's unreachable: ```bash diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c9e2c6a..1c187bac3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Headline: **A full Mendix build-and-test loop that fits on an iPad** — you can - **`run --local` robustness** — the web client bundle is verified after boot, not before; stale ports are refused and the holding process is named; a relative project path resolves where MxBuild is called; the browser bundle is re-served after a structural `--watch` change; boot uses live-preview flags so `mxcli oql` can reach the app; the hub tunnel retries forever and re-registers on a heartbeat 404. - **Theme** — `mxcli theme remove` with no name now reads the installed theme from the `mxcli:theme` markers instead of removing the built-in default and reporting a silent no-op; switching themes no longer orphans the previous theme's block in `_mxcli-atlas-map.scss`; the topbar language selector, filter-operator popovers, login page, and Data Grid 2 now follow the palette (the language selector went from 1.13:1 to 17.79:1 / 19.47:1 measured contrast). - **JavaScript action sources were written to the wrong directory** — `CREATE JAVASCRIPT ACTION` wrote to `javascriptsource//actions/`, but Mendix reads a **lowercased** module directory. MxBuild found nothing there, generated a stub whose body throws `JavaScript action was not implemented`, and bundled that — so the action parsed, passed `mxcli check`, built cleanly, and threw the moment it ran. Only reproduced on a case-sensitive filesystem, which is why it went unnoticed on macOS and Windows. +- **`mxcli run --ensure-db` can start PostgreSQL without a working service manager (#823)** — on hosts that ship neither `service` nor Debian's `pg_ctlcluster` (e.g. Arch Linux), `--ensure-db` failed with `exec: "pg_ctlcluster": executable file not found in $PATH` even though the portable `initdb`/`pg_ctl`/`psql` tools were present. `startLocalPostgres` now falls back to a user-owned cluster under `~/.mxcli/postgres` when no service becomes ready, but never starts a competitor while another process owns the requested port. The cluster is idempotent and needs neither a `postgres` OS account nor passwordless `sudo`: its listen address, port, private socket directory, and `0700` socket permissions persist in PostgreSQL's own configuration; loopback TCP uses SCRAM while role/database provisioning uses local trust only through that private socket. Anyone who ran an earlier development revision of this fix must stop PostgreSQL, remove `~/.mxcli/postgres`, and rerun `--ensure-db`; reuse detects and refuses its insecure host-trust records. The existing `sudo -u postgres psql` path remains for system clusters. - **Widget sync wrote duplicate GUIDs and corrupted the project** — reconciliation now removes stale properties and syncs attributes via `AugmentTemplate` so the widget `Type` is exact. ## [0.16.0] - 2026-07-12 diff --git a/cmd/mxcli/docker/ensuredb.go b/cmd/mxcli/docker/ensuredb.go index 6869bfa34..a3ee8261b 100644 --- a/cmd/mxcli/docker/ensuredb.go +++ b/cmd/mxcli/docker/ensuredb.go @@ -5,25 +5,37 @@ package docker import ( "fmt" "io" + "net" "os" "os/exec" + "path/filepath" "regexp" + "strconv" "strings" "time" ) // ensuredb.go provisions the local PostgreSQL a standalone runtime needs, so a // fresh session comes up testable without a manual createdb (slice 2 of the -// warm-loop proposal). It is best-effort and devcontainer-shaped: it starts the -// local Postgres service if the port is down, then ensures the app role and -// database exist via a superuser (`sudo -u postgres psql`). For a non-local DB -// host it does nothing but verify reachability — provisioning a remote database -// is not mxcli's business. +// warm-loop proposal). It is best-effort: it starts the local Postgres service +// if the port is down — via a service manager, or a user-owned initdb/pg_ctl +// cluster when no service becomes ready (e.g. Arch, #823) — then ensures the app +// role and database exist via a superuser (a trust connection over the +// user-owned cluster's private socket, or `sudo -u postgres psql` for a system +// cluster). For a non-local DB host it does nothing but verify reachability — +// provisioning a remote database is not mxcli's business. // pgIdent is a conservative PostgreSQL identifier (unquoted): a safe database or // role name. We refuse anything else rather than quote/escape it into DDL. var pgIdent = regexp.MustCompile(`^[a-z_][a-z0-9_]*$`) +// serviceReadyTimeout bounds how long we probe for a service-manager-started +// server before falling back to a user-owned cluster. It is deliberately short: +// the single authoritative readiness wait lives in EnsureDatabase, so a dead or +// slow attempt must not multiply the overall deadline (#823 review). A variable +// so tests can shrink it further. +var serviceReadyTimeout = 3 * time.Second + // splitHostPort splits "host:port" into host and port, defaulting the port to // 5432 when absent. func splitHostPort(hostPort string) (host, port string) { @@ -33,6 +45,17 @@ func splitHostPort(hostPort string) (host, port string) { return hostPort, "5432" } +// normalizePostgresPort validates a user-supplied PostgreSQL port and returns +// its canonical decimal form, so an invalid value cannot reach PostgreSQL's +// command arguments or managed configuration. +func normalizePostgresPort(port string) (string, error) { + n, err := strconv.ParseUint(port, 10, 16) + if err != nil || n == 0 { + return "", fmt.Errorf("invalid PostgreSQL port %q (expected 1-65535)", port) + } + return strconv.FormatUint(n, 10), nil +} + // isLocalHost reports whether host refers to the local machine (so it is safe to // start a service / use a local superuser). func isLocalHost(host string) bool { @@ -64,6 +87,16 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { return fmt.Errorf("unsafe database user %q (expected %s)", db.User, pgIdent) } host, port := splitHostPort(db.Host) + port, err := normalizePostgresPort(port) + if err != nil { + return fmt.Errorf("invalid --db-host %q: %w", db.Host, err) + } + // An empty host (e.g. --db-host ":5432") means loopback; make it explicit so + // every downstream psql/pg_isready gets a real -h. + if host == "" { + host = "127.0.0.1" + db.Host = net.JoinHostPort(host, port) + } // Already usable? Then we're done. if canConnectDB(db) { @@ -77,7 +110,7 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { "start it and create the %q database (user %q)", db.Host, db.Name, db.User) } fmt.Fprintln(w, " Starting local PostgreSQL...") - if err := startLocalPostgres(); err != nil { + if err := startLocalPostgres(host, port, w); err != nil { return fmt.Errorf("starting local PostgreSQL: %w", err) } if err := waitPGReady(host, port, 20*time.Second); err != nil { @@ -86,10 +119,14 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { } // Ensure the role and database exist (needs a local superuser). - if err := ensureRole(db, w); err != nil { + su, err := resolveSuperuser(host, port) + if err != nil { + return err + } + if err := ensureRole(su, db, w); err != nil { return err } - if err := ensureDatabase(db, w); err != nil { + if err := ensureDatabase(su, db, w); err != nil { return err } @@ -111,33 +148,304 @@ func canConnectDB(db DBConfig) bool { } // startLocalPostgres starts the local PostgreSQL service, trying the common -// service managers in turn. Success is confirmed later by waitPGReady. -func startLocalPostgres() error { +// service managers in turn. When none is present — e.g. on Arch — or they do +// not produce a ready server, it falls back to a user-owned cluster started with +// the portable initdb/pg_ctl tools (#823). +func startLocalPostgres(host, port string, w io.Writer) error { + var err error + port, err = normalizePostgresPort(port) + if err != nil { + return err + } + // Only real, portable service managers belong here. The old + // {"pg_ctlcluster", "--", "start"} entry was a placeholder whose args could + // never start a cluster, so it only ever burned a readiness timeout before + // the fallback — dropped (#823 review). attempts := [][]string{ {"service", "postgresql", "start"}, - {"pg_ctlcluster", "--", "start"}, // placeholder; real cluster args vary } - var lastErr error + var serviceDiag []string for _, a := range attempts { if _, err := exec.LookPath(a[0]); err != nil { - lastErr = err continue } - cmd := exec.Command(a[0], a[1:]...) - if err := cmd.Run(); err == nil { + // The command may exit non-zero yet still bring Postgres up, so a short + // readiness probe decides — not the exit code. The probe is intentionally + // short: the single authoritative 20s wait is in EnsureDatabase, so a + // slow-but-working manager is honoured there rather than paid for here. + out, _ := exec.Command(a[0], a[1:]...).CombinedOutput() + if waitPGReady(host, port, serviceReadyTimeout) == nil { return nil - } else { - lastErr = err } - // `service postgresql start` is the reliable path in the devcontainer; if - // it ran (even non-zero) Postgres may still be coming up — let waitPGReady - // decide rather than failing here. + if d := strings.TrimSpace(string(out)); d != "" { + serviceDiag = append(serviceDiag, a[0]+": "+d) + } + } + + // A service-started server may own the port while crash recovery still makes + // pg_isready report "not ready". Never initialize a competitor in that case: + // EnsureDatabase's authoritative 20-second wait decides whether it recovers. + // This guard also covers a process that won the port between the caller's + // initial reachability check and this fallback. + if pingTCP(net.JoinHostPort(host, port), time.Second) == nil { return nil } - if lastErr != nil { - return lastErr + + // No service manager made PostgreSQL ready: start a user-owned cluster with + // the portable tools. This needs neither a `postgres` OS account nor sudo. + if err := startUserCluster(host, port, w); err != nil { + if len(serviceDiag) > 0 { + // Surface what the service manager said; otherwise the user sees only + // an initdb/pg_ctl error from two steps later. + return fmt.Errorf("%w\n(a service manager ran first but Postgres did not "+ + "become ready:\n%s)", err, strings.Join(serviceDiag, "\n")) + } + return err + } + return nil +} + +// userClusterDirs returns the state, data, and socket directories for the +// user-owned cluster under ~/.mxcli/postgres. The socket dir is separate and +// short because some systems cap the Unix-socket path length. +func userClusterDirs() (stateDir, dataDir, sockDir string, err error) { + home, err := os.UserHomeDir() + if err != nil { + return "", "", "", fmt.Errorf("determining home directory: %w", err) + } + stateDir = filepath.Join(home, ".mxcli", "postgres") + return stateDir, filepath.Join(stateDir, "data"), filepath.Join(stateDir, "sock"), nil +} + +// writeFileAtomic replaces a cluster configuration file without exposing a +// partially-written file to a concurrent or subsequent postgres start. +func writeFileAtomic(path string, data []byte, mode os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*") + if err != nil { + return err + } + tmpPath := tmp.Name() + keep := false + defer func() { + _ = tmp.Close() + if !keep { + _ = os.Remove(tmpPath) + } + }() + if err := tmp.Chmod(mode); err != nil { + return err + } + if _, err := tmp.Write(data); err != nil { + return err + } + if err := tmp.Sync(); err != nil { + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Rename(tmpPath, path); err != nil { + return err } - return fmt.Errorf("no known service manager found to start PostgreSQL") + keep = true + return nil +} + +func postgresConfigString(value string) (string, error) { + if strings.ContainsAny(value, "\x00\r\n") { + return "", fmt.Errorf("value contains a line break or NUL byte") + } + return "'" + strings.ReplaceAll(value, "'", "''") + "'", nil +} + +const ( + mxcliPostgresConfigBegin = "# BEGIN mxcli managed connection settings" + mxcliPostgresConfigEnd = "# END mxcli managed connection settings" +) + +// persistUserClusterConfig makes the private endpoint a property of the +// cluster, so it also holds for a later plain `pg_ctl -D ... start`. +func persistUserClusterConfig(dataDir, host, port, sockDir string) error { + hostValue, err := postgresConfigString(host) + if err != nil { + return fmt.Errorf("quoting listen address: %w", err) + } + sockValue, err := postgresConfigString(sockDir) + if err != nil { + return fmt.Errorf("quoting socket directory: %w", err) + } + managed := fmt.Sprintf("%s\n"+ + "listen_addresses = %s\n"+ + "port = %s\n"+ + "unix_socket_directories = %s\n"+ + "unix_socket_permissions = 0700\n"+ + "%s\n", mxcliPostgresConfigBegin, hostValue, port, sockValue, mxcliPostgresConfigEnd) + + confPath := filepath.Join(dataDir, "postgresql.conf") + conf, err := os.ReadFile(confPath) + if err != nil { + return fmt.Errorf("reading postgresql.conf: %w", err) + } + for { + start := strings.Index(string(conf), mxcliPostgresConfigBegin) + if start < 0 { + break + } + relEnd := strings.Index(string(conf[start:]), mxcliPostgresConfigEnd) + if relEnd < 0 { + return fmt.Errorf("managed block in postgresql.conf is missing its end marker") + } + end := start + relEnd + len(mxcliPostgresConfigEnd) + if end < len(conf) && conf[end] == '\n' { + end++ + } + conf = append(conf[:start], conf[end:]...) + } + conf = []byte(strings.TrimRight(string(conf), "\r\n")) + if len(conf) > 0 { + conf = append(conf, '\n', '\n') + } + conf = append(conf, managed...) + if err := writeFileAtomic(confPath, conf, 0o600); err != nil { + return fmt.Errorf("updating postgresql.conf: %w", err) + } + return nil +} + +// rejectLegacyHostTrust prevents silent reuse of a cluster initialized by an +// earlier development revision, which used trust for loopback TCP. Local trust +// is expected and remains confined to the private Unix socket. +func rejectLegacyHostTrust(dataDir string) error { + hbaPath := filepath.Join(dataDir, "pg_hba.conf") + hba, err := os.ReadFile(hbaPath) + if err != nil { + return fmt.Errorf("reading pg_hba.conf: %w", err) + } + for lineNo, line := range strings.Split(string(hba), "\n") { + fields := strings.Fields(strings.SplitN(line, "#", 2)[0]) + if len(fields) < 5 || !strings.HasPrefix(fields[0], "host") { + continue + } + for _, field := range fields[4:] { + if field == "trust" { + return fmt.Errorf("unsafe host trust authentication in %s line %d; stop PostgreSQL, "+ + "remove ~/.mxcli/postgres, and rerun --ensure-db", hbaPath, lineNo+1) + } + } + } + return nil +} + +// clusterStatus reports whether a server is running from dataDir and, when +// readable, its TCP port and Unix-socket directory from postmaster.pid. +// `pg_ctl status` alone only proves that some server runs from this data directory. +func clusterStatus(dataDir string) (running bool, port, sockDir string) { + if exec.Command("pg_ctl", "-D", dataDir, "status").Run() != nil { + return false, "", "" + } + data, err := os.ReadFile(filepath.Join(dataDir, "postmaster.pid")) + if err != nil { + return true, "", "" // running, but its endpoint is unknown + } + // postmaster.pid: line 1 PID, 2 data dir, 3 start time, 4 port, 5 socket dir… + if lines := strings.Split(string(data), "\n"); len(lines) >= 5 { + return true, strings.TrimSpace(lines[3]), strings.TrimSpace(lines[4]) + } + return true, "", "" +} + +// startUserCluster initializes (once) and starts a PostgreSQL cluster owned by +// the current user under ~/.mxcli/postgres, listening on host:port. It is safe +// to run repeatedly: an initialized data directory is reused and an already +// running server is left alone. +func startUserCluster(host, port string, w io.Writer) error { + stateDir, dataDir, sockDir, err := userClusterDirs() + if err != nil { + return err + } + // Normalise an empty host so the persisted listen address is explicit even if + // this helper is called directly with a bare ":port". + if host == "" { + host = "127.0.0.1" + } + if !isLocalHost(host) { + return fmt.Errorf("refusing to start a user-owned PostgreSQL cluster on non-local host %q", host) + } + port, err = normalizePostgresPort(port) + if err != nil { + return err + } + if err := os.MkdirAll(sockDir, 0o700); err != nil { + return fmt.Errorf("creating PostgreSQL socket directory %s: %w", sockDir, err) + } + // MkdirAll ignores its mode for an existing directory. Tighten it explicitly: + // local trust authentication relies on this directory as its access boundary. + if err := os.Chmod(sockDir, 0o700); err != nil { + return fmt.Errorf("securing PostgreSQL socket directory %s: %w", sockDir, err) + } + + // Initialize once — PG_VERSION marks a data directory initdb has populated. + if _, err := os.Stat(filepath.Join(dataDir, "PG_VERSION")); os.IsNotExist(err) { + fmt.Fprintln(w, " Initializing user-owned PostgreSQL cluster...") + // Bootstrap superuser "postgres". trust over the private 0700 socket keeps + // our own provisioning password-free, but loopback TCP is scram-sha-256: + // binding 127.0.0.1 is not an access control on a multi-user host, so trust + // there would let any local account act as the postgres superuser. + init := exec.Command("initdb", "-D", dataDir, "-U", "postgres", + "--auth-local=trust", "--auth-host=scram-sha-256", "--encoding=UTF8") + if out, err := init.CombinedOutput(); err != nil { + return fmt.Errorf("initializing PostgreSQL cluster in %s: %w\n%s", + dataDir, err, strings.TrimSpace(string(out))) + } + } else if err != nil { + return fmt.Errorf("checking PostgreSQL cluster in %s: %w", dataDir, err) + } + + if err := rejectLegacyHostTrust(dataDir); err != nil { + return err + } + + // A cluster left from an earlier --db-host satisfies `pg_ctl status` even when + // its endpoint differs. Validate the live endpoint before changing its future + // restart configuration, so a rejected request has no hidden side effect. + running, livePort, liveSockDir := clusterStatus(dataDir) + if running { + if livePort == "" { + return fmt.Errorf("a user-owned PostgreSQL cluster is running from %s, but its port "+ + "could not be read from %s", dataDir, filepath.Join(dataDir, "postmaster.pid")) + } + if livePort != port { + return fmt.Errorf("a user-owned PostgreSQL cluster is already running from %s on "+ + "port %s, but port %s was requested — stop it with `pg_ctl -D %s stop` or rerun "+ + "with --db-host %s", dataDir, livePort, port, dataDir, net.JoinHostPort(host, livePort)) + } + if liveSockDir == "" { + return fmt.Errorf("a user-owned PostgreSQL cluster is running from %s, but its socket "+ + "directory could not be read from %s", dataDir, filepath.Join(dataDir, "postmaster.pid")) + } + if filepath.Clean(liveSockDir) != filepath.Clean(sockDir) { + return fmt.Errorf("a user-owned PostgreSQL cluster is running with socket directory %s, "+ + "but the private directory %s is required — stop it with `pg_ctl -D %s stop` and "+ + "rerun --ensure-db", liveSockDir, sockDir, dataDir) + } + } + + if err := persistUserClusterConfig(dataDir, host, port, sockDir); err != nil { + return fmt.Errorf("persisting PostgreSQL connection settings in %s: %w", dataDir, err) + } + if running { + return nil + } + + fmt.Fprintln(w, " Starting user-owned PostgreSQL cluster...") + logPath := filepath.Join(stateDir, "server.log") + start := exec.Command("pg_ctl", "-D", dataDir, "-w", + "-t", "30", "-l", logPath, "start") + if out, err := start.CombinedOutput(); err != nil { + return fmt.Errorf("starting PostgreSQL cluster in %s: %w\n%s\n (see the server "+ + "log at %s)", dataDir, err, strings.TrimSpace(string(out)), logPath) + } + return nil } // waitPGReady polls pg_isready until the server accepts connections or timeout. @@ -169,40 +477,92 @@ func waitTCP(hostPort string, timeout time.Duration) error { return fmt.Errorf("%s did not accept connections within %s", hostPort, timeout) } -// superuserPSQL runs a psql command as the postgres superuser (sudo -u postgres). -func superuserPSQL(args ...string) *exec.Cmd { - full := append([]string{"-u", "postgres", "psql", "-v", "ON_ERROR_STOP=1"}, args...) - return exec.Command("sudo", full...) +// superuser is how we reach a PostgreSQL superuser to provision the role and +// database: a direct `psql -U postgres` against the user-owned cluster (over its +// private socket), or the original `sudo -u postgres psql` for a system cluster. +type superuser struct { + host, port string + sock string // Unix-socket dir for the user-owned cluster; preferred over TCP + sudo bool +} + +// psql builds a psql command for the superuser. ON_ERROR_STOP makes a failed +// statement a non-zero exit rather than a silent success. +func (s superuser) psql(args ...string) *exec.Cmd { + if s.sudo { + return exec.Command("sudo", + append([]string{"-u", "postgres", "psql", "-v", "ON_ERROR_STOP=1"}, args...)...) + } + // Prefer the cluster's private socket: initdb set --auth-local=trust there, + // while loopback TCP is scram-sha-256, so the passwordless superuser can only + // reach itself over the socket. -w never prompts, so probing an unrelated + // password-protected system cluster fails fast instead of hanging. + host := s.host + if s.sock != "" { + host = s.sock + } + base := []string{"-v", "ON_ERROR_STOP=1", "-w", + "-h", host, "-p", s.port, "-U", "postgres", "-d", "postgres"} + return exec.Command("psql", append(base, args...)...) +} + +// resolveSuperuser picks a working superuser path: a direct connection (the +// user-owned initdb cluster over its trust socket, or a plain loopback cluster — +// the only paths that work without elevation on Arch) or `sudo -u postgres` for +// a system cluster. +func resolveSuperuser(host, port string) (superuser, error) { + if isLocalHost(host) { + // Prefer our cluster's private socket (trust); fall back to a plain + // loopback connection for a pre-existing cluster a user pointed us at. + candidates := []superuser{{host: host, port: port}} + if _, _, sockDir, err := userClusterDirs(); err == nil { + candidates = append([]superuser{{host: host, port: port, sock: sockDir}}, candidates...) + } + for _, direct := range candidates { + if direct.psql("-tAc", "select 1").Run() == nil { + return direct, nil + } + } + } + if _, err := exec.LookPath("sudo"); err == nil { + sudo := superuser{host: host, port: port, sudo: true} + if sudo.psql("-tAc", "select 1").Run() == nil { + return sudo, nil + } + } + return superuser{}, fmt.Errorf("no local PostgreSQL superuser available to create the " + + "role/database (tried a direct 'psql -U postgres' connection over the cluster socket " + + "and TCP, and 'sudo -u postgres')") } // ensureRole creates the app login role if it does not already exist. -func ensureRole(db DBConfig, w io.Writer) error { - check := superuserPSQL("-tAc", fmt.Sprintf("select 1 from pg_roles where rolname='%s'", db.User)) +func ensureRole(su superuser, db DBConfig, w io.Writer) error { + check := su.psql("-tAc", fmt.Sprintf("select 1 from pg_roles where rolname='%s'", db.User)) out, _ := check.Output() if strings.TrimSpace(string(out)) == "1" { return nil } fmt.Fprintf(w, " Creating role %q...\n", db.User) ddl := fmt.Sprintf("CREATE ROLE %s WITH LOGIN PASSWORD %s CREATEDB", db.User, quoteSQLString(db.Password)) - cmd := superuserPSQL("-c", ddl) + cmd := su.psql("-c", ddl) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("creating role %q: %w\n%s\n"+ - " (need a local postgres superuser via 'sudo -u postgres'; create the role manually if unavailable)", + " (need a local postgres superuser; create the role manually if unavailable)", db.User, err, strings.TrimSpace(string(out))) } return nil } // ensureDatabase creates the app database owned by the app role if it is absent. -func ensureDatabase(db DBConfig, w io.Writer) error { - check := superuserPSQL("-tAc", fmt.Sprintf("select 1 from pg_database where datname='%s'", db.Name)) +func ensureDatabase(su superuser, db DBConfig, w io.Writer) error { + check := su.psql("-tAc", fmt.Sprintf("select 1 from pg_database where datname='%s'", db.Name)) out, _ := check.Output() if strings.TrimSpace(string(out)) == "1" { return nil } fmt.Fprintf(w, " Creating database %q owned by %q...\n", db.Name, db.User) ddl := fmt.Sprintf("CREATE DATABASE %s OWNER %s", db.Name, db.User) - cmd := superuserPSQL("-c", ddl) + cmd := su.psql("-c", ddl) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("creating database %q: %w\n%s", db.Name, err, strings.TrimSpace(string(out))) } diff --git a/cmd/mxcli/docker/ensuredb_test.go b/cmd/mxcli/docker/ensuredb_test.go index 5cd0ab470..2006c5a3d 100644 --- a/cmd/mxcli/docker/ensuredb_test.go +++ b/cmd/mxcli/docker/ensuredb_test.go @@ -4,7 +4,14 @@ package docker import ( "io" + "net" + "os" + "path/filepath" + "runtime" + "strconv" + "strings" "testing" + "time" ) func TestSplitHostPort(t *testing.T) { @@ -21,6 +28,16 @@ func TestSplitHostPort(t *testing.T) { } } +func TestPostgresConfigString(t *testing.T) { + got, err := postgresConfigString("/home/user's db") + if err != nil || got != "'/home/user''s db'" { + t.Fatalf("postgresConfigString escaped value = (%q, %v)", got, err) + } + if _, err := postgresConfigString("safe'\nport = 1"); err == nil { + t.Fatal("configuration line injection should be rejected") + } +} + func TestIsLocalHost(t *testing.T) { for _, h := range []string{"127.0.0.1", "localhost", "::1", ""} { if !isLocalHost(h) { @@ -55,4 +72,586 @@ func TestEnsureDatabase_Validation(t *testing.T) { if err := EnsureDatabase(DBConfig{Type: "PostgreSQL", Name: "ok", User: "bad user", Host: "127.0.0.1:5432"}, io.Discard); err == nil { t.Error("expected error for unsafe database user") } + for _, host := range []string{"127.0.0.1:0", "127.0.0.1:5432 -c fsync=off"} { + if err := EnsureDatabase(DBConfig{Type: "PostgreSQL", Name: "ok", User: "ok", Host: host}, io.Discard); err == nil || !strings.Contains(err.Error(), "invalid --db-host") { + t.Errorf("expected an actionable error for invalid host %q, got %v", host, err) + } + } +} + +// --- #823: initdb/pg_ctl fallback --- + +func newStubPATH(t *testing.T) (dir, logPath string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell-stub test not supported on Windows") + } + dir = t.TempDir() + t.Setenv("PATH", dir) + t.Setenv("HOME", t.TempDir()) + return dir, filepath.Join(dir, "calls") +} + +func writeStub(t *testing.T, dir, name, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil { + t.Fatalf("writing stub %s: %v", name, err) + } +} + +func initdbStub(logPath string) string { + return `data=""; next=0 +for arg in "$@"; do + [ "$next" = 1 ] && { data="$arg"; next=0; } + [ "$arg" = "-D" ] && next=1 +done +/bin/mkdir -p "$data" +echo 16 > "$data/PG_VERSION" +echo "# PostgreSQL configuration" > "$data/postgresql.conf" +echo "local all all trust" > "$data/pg_hba.conf" +echo "host all all 127.0.0.1/32 scram-sha-256" >> "$data/pg_hba.conf" +echo "host all all ::1/128 scram-sha-256" >> "$data/pg_hba.conf" +echo initdb >> "` + logPath + `"` +} + +func pgctlStub(logPath string, statusCode, startCode int) string { + return `last=""; for arg in "$@"; do last="$arg"; done +[ "$last" = status ] && exit ` + strconv.Itoa(statusCode) + ` +echo pg_ctl_start >> "` + logPath + `" +exit ` + strconv.Itoa(startCode) +} + +func readCalls(t *testing.T, logPath string) string { + t.Helper() + b, err := os.ReadFile(logPath) + if os.IsNotExist(err) { + return "" + } + if err != nil { + t.Fatal(err) + } + return string(b) +} + +// unusedTCPPort returns a currently unbound loopback port. Fallback tests must +// not depend on the developer or CI host having nothing listening on 5432: an +// occupied port intentionally suppresses the user-cluster fallback. +func unusedTCPPort(t *testing.T) string { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + port := strconv.Itoa(listener.Addr().(*net.TCPAddr).Port) + if err := listener.Close(); err != nil { + t.Fatal(err) + } + return port +} + +func TestStartLocalPostgres_ServicePaths(t *testing.T) { + t.Run("ready service skips fallback", func(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"; exit 1`) + writeStub(t, dir, "pg_isready", "exit 0") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", "5432", io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "service") || strings.Contains(calls, "initdb") { + t.Fatalf("unexpected calls:\n%s", calls) + } + }) + + t.Run("non-ready service uses fallback", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + oldTimeout := serviceReadyTimeout + serviceReadyTimeout = 50 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = oldTimeout }) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + for _, want := range []string{"service", "initdb", "pg_ctl_start"} { + if !strings.Contains(calls, want) { + t.Fatalf("%s was not called:\n%s", want, calls) + } + } + }) +} + +// A service-started PostgreSQL can own the TCP port while pg_isready still +// reports "the database system is starting up" (for example during crash +// recovery). The short service probe must not initialize a competing cluster; +// EnsureDatabase's longer readiness wait is authoritative in this case. +func TestStartLocalPostgres_OccupiedPortSkipsFallback(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = listener.Close() }) + port := strconv.Itoa(listener.Addr().(*net.TCPAddr).Port) + + dir, logPath := newStubPATH(t) + oldTimeout := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = oldTimeout }) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "service") { + t.Fatalf("service manager was not attempted:\n%s", calls) + } + if strings.Contains(calls, "initdb") || strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("an occupied port must suppress the user-cluster fallback:\n%s", calls) + } +} + +func TestStartLocalPostgres_Fallback(t *testing.T) { + tests := []struct { + name string + tools, initialized bool + statusCode, startCode int + wantErr bool + wantInit, wantStart bool + }{ + {name: "missing tools", wantErr: true}, + {name: "first init", tools: true, statusCode: 3, wantInit: true, wantStart: true}, + {name: "repeated stopped", tools: true, initialized: true, statusCode: 3, wantStart: true}, + {name: "already running", tools: true, initialized: true}, + {name: "start failure", tools: true, statusCode: 3, startCode: 1, wantErr: true, wantInit: true, wantStart: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + if tt.initialized { + dataDir := initClusterDir(t) + if tt.statusCode == 0 { + writePostmasterPID(t, dataDir, port) + } + } + if tt.tools { + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, tt.statusCode, tt.startCode)) + } + + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if (err != nil) != tt.wantErr { + t.Fatalf("error = %v, wantErr %v", err, tt.wantErr) + } + calls := readCalls(t, logPath) + if strings.Contains(calls, "initdb") != tt.wantInit { + t.Errorf("initdb calls = %q, want %v", calls, tt.wantInit) + } + if strings.Contains(calls, "pg_ctl_start") != tt.wantStart { + t.Errorf("pg_ctl start calls = %q, want %v", calls, tt.wantStart) + } + }) + } +} + +func TestResolveSuperuser(t *testing.T) { + t.Run("direct", func(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo "$@" >> "`+logPath+`"`) + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"`) + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil || su.sudo { + t.Fatalf("su=%+v err=%v", su, err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "-U postgres") || strings.Contains(calls, "sudo") { + t.Fatalf("unexpected calls:\n%s", calls) + } + }) + + t.Run("sudo fallback", func(t *testing.T) { + dir, _ := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") + writeStub(t, dir, "sudo", "exit 0") + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil || !su.sudo { + t.Fatalf("su=%+v err=%v", su, err) + } + }) + + t.Run("unavailable", func(t *testing.T) { + dir, _ := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") + if _, err := resolveSuperuser("127.0.0.1", "5432"); err == nil { + t.Fatal("expected an error") + } + }) +} + +// writePostmasterPID writes a minimal postmaster.pid whose 4th line is the port, +// matching what a running server publishes (PID, data dir, start time, port, …). +func writePostmasterPID(t *testing.T, dataDir, port string) { + t.Helper() + home, _ := os.UserHomeDir() + writePostmasterPIDWithSocket(t, dataDir, port, filepath.Join(home, ".mxcli", "postgres", "sock")) +} + +func writePostmasterPIDWithSocket(t *testing.T, dataDir, port, sockDir string) { + t.Helper() + body := "12345\n" + dataDir + "\n1700000000\n" + port + "\n" + sockDir + "\n" + if err := os.WriteFile(filepath.Join(dataDir, "postmaster.pid"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +// initClusterDir pre-creates an initialized data directory under $HOME so the +// initdb branch is skipped. +func initClusterDir(t *testing.T) string { + t.Helper() + home, _ := os.UserHomeDir() + dataDir := filepath.Join(home, ".mxcli", "postgres", "data") + if err := os.MkdirAll(dataDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "PG_VERSION"), []byte("16\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "postgresql.conf"), []byte("# PostgreSQL configuration\n"), 0o600); err != nil { + t.Fatal(err) + } + hba := "local all all trust\n" + + "host all all 127.0.0.1/32 scram-sha-256\n" + + "host all all ::1/128 scram-sha-256\n" + if err := os.WriteFile(filepath.Join(dataDir, "pg_hba.conf"), []byte(hba), 0o600); err != nil { + t.Fatal(err) + } + return dataDir +} + +// --- #823 review: the dead pg_ctlcluster placeholder is gone --- + +// The old attempts list carried {"pg_ctlcluster", "--", "start"} — a placeholder +// whose args can never start a cluster, so it only burned a readiness timeout. +// Even when present on PATH it must never be executed now. +func TestStartLocalPostgres_NeverRunsPgCtlCluster(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + old := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = old }) + // No `service` on PATH; a pg_ctlcluster that would log if ever called. + writeStub(t, dir, "pg_ctlcluster", `echo pg_ctlcluster >> "`+logPath+`"; exit 0`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if strings.Contains(calls, "pg_ctlcluster") { + t.Fatalf("pg_ctlcluster must never run, got:\n%s", calls) + } + if !strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("expected fallback to start a user cluster, got:\n%s", calls) + } +} + +// A failing service manager's own output must reach the final error rather than +// being replaced by an unrelated initdb/pg_ctl error two steps later. +func TestStartLocalPostgres_SurfacesServiceOutput(t *testing.T) { + dir, _ := newStubPATH(t) + port := unusedTCPPort(t) + old := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = old }) + writeStub(t, dir, "service", `echo "postgresql.service is masked"; exit 1`) + writeStub(t, dir, "pg_isready", "exit 1") + // No initdb/pg_ctl on PATH, so the fallback fails — the error should still + // carry the service manager's message. + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected an error when neither a service nor the portable tools work") + } + if !strings.Contains(err.Error(), "postgresql.service is masked") { + t.Fatalf("service output not surfaced: %v", err) + } +} + +// --- #823 review: initdb hardens loopback TCP (scram), trust only on the socket --- + +func TestStartUserCluster_InitdbAuthArgs(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + argsPath := filepath.Join(dir, "initdb.args") + writeStub(t, dir, "initdb", `echo "$@" >> "`+argsPath+`"; `+initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + args := readCalls(t, argsPath) + if !strings.Contains(args, "--auth-host=scram-sha-256") { + t.Errorf("loopback TCP must not be trust; initdb args = %q", args) + } + if strings.Contains(args, "--auth-host=trust") { + t.Errorf("initdb must not set --auth-host=trust; args = %q", args) + } + if !strings.Contains(args, "--auth-local=trust") { + t.Errorf("socket connections should stay trust; args = %q", args) + } +} + +// The socket, listen address, and port must live in the cluster configuration, +// not only in pg_ctl -o flags, so a later plain `pg_ctl -D ... start` remains +// private and starts on the expected endpoint. +func TestStartUserCluster_PersistsSecureConnectionSettings(t *testing.T) { + dir, logPath := newStubPATH(t) + home, _ := os.UserHomeDir() + sockDir := filepath.Join(home, ".mxcli", "postgres", "sock") + if err := os.MkdirAll(sockDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(sockDir, 0o755); err != nil { + t.Fatal(err) + } + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startUserCluster("127.0.0.1", "5544", io.Discard); err != nil { + t.Fatal(err) + } + dataDir := filepath.Join(home, ".mxcli", "postgres", "data") + info, err := os.Stat(sockDir) + if err != nil || info.Mode().Perm() != 0o700 { + t.Fatalf("socket directory = %v, %v; want mode 0700", info, err) + } + postgresConf, err := os.ReadFile(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(postgresConf), mxcliPostgresConfigBegin); got != 1 { + t.Fatalf("postgresql.conf should contain one mxcli block, got %d:\n%s", got, postgresConf) + } + firstConfig := string(postgresConf) + for _, want := range []string{ + "listen_addresses = '127.0.0.1'", + "port = 5544", + "unix_socket_directories = '" + filepath.Join(home, ".mxcli", "postgres", "sock") + "'", + "unix_socket_permissions = 0700", + } { + if !strings.Contains(string(postgresConf), want) { + t.Errorf("postgresql.conf missing %q:\n%s", want, postgresConf) + } + } + info, err = os.Stat(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("postgresql.conf mode = %04o, want 0600", got) + } + + // Reapplying the configuration must replace, not duplicate, the managed block. + if err := startUserCluster("127.0.0.1", "5544", io.Discard); err != nil { + t.Fatal(err) + } + postgresConf, err = os.ReadFile(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(postgresConf), mxcliPostgresConfigBegin); got != 1 { + t.Fatalf("repeated configuration added duplicate managed blocks: %d\n%s", got, postgresConf) + } + if string(postgresConf) != firstConfig { + t.Fatalf("repeated configuration was not byte-stable:\n%s", postgresConf) + } +} + +// A cluster initialized by the pre-review branch used host trust. Reuse must +// fail safely with a cleanup path instead of silently retaining that bypass. +func TestStartUserCluster_RejectsLegacyHostTrust(t *testing.T) { + for _, record := range []string{ + "host all all 127.0.0.1/32 trust", + "host all all 127.0.0.1 255.255.255.255 trust", + } { + t.Run(record, func(t *testing.T) { + _, _ = newStubPATH(t) + dataDir := initClusterDir(t) + hba := "local all all trust\n" + record + "\n" + if err := os.WriteFile(filepath.Join(dataDir, "pg_hba.conf"), []byte(hba), 0o600); err != nil { + t.Fatal(err) + } + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil || !strings.Contains(err.Error(), "unsafe host trust") || + !strings.Contains(err.Error(), "remove ~/.mxcli/postgres") { + t.Fatalf("expected an actionable host-trust error, got %v", err) + } + }) + } +} + +func TestStartUserCluster_RefusesNonLocalHost(t *testing.T) { + if err := startUserCluster("0.0.0.0", "5432", io.Discard); err == nil || + !strings.Contains(err.Error(), "non-local host") { + t.Fatalf("expected a non-local bind refusal, got %v", err) + } +} + +// The direct superuser must reach the cluster over its private trust socket, not +// over loopback TCP (which is now scram-sha-256 and would reject a passwordless +// connection). +func TestResolveSuperuser_PrefersClusterSocket(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo "$@" >> "`+logPath+`"`) // exits 0 + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"`) + + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil { + t.Fatal(err) + } + home, _ := os.UserHomeDir() + sockDir := filepath.Join(home, ".mxcli", "postgres", "sock") + if su.sock != sockDir { + t.Fatalf("superuser should use the cluster socket %q, got %+v", sockDir, su) + } + if calls := readCalls(t, logPath); !strings.Contains(calls, "-h "+sockDir) { + t.Fatalf("psql should connect over the socket dir, calls:\n%s", calls) + } +} + +// --- #823 review: an empty host normalises to loopback in the server config --- + +func TestStartUserCluster_EmptyHostNormalised(t *testing.T) { + dir, logPath := newStubPATH(t) + initClusterDir(t) // skip initdb; status=3 => not running => start + writeStub(t, dir, "pg_ctl", `last=""; for a in "$@"; do last="$a"; done +[ "$last" = status ] && exit 3 +echo pg_ctl_start >> "`+logPath+`" +exit 0`) + + if err := startUserCluster("", "5432", io.Discard); err != nil { + t.Fatal(err) + } + home, _ := os.UserHomeDir() + conf, err := os.ReadFile(filepath.Join(home, ".mxcli", "postgres", "data", "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(conf), "listen_addresses = '127.0.0.1'") { + t.Fatalf("empty host should become 127.0.0.1 in persisted config:\n%s", conf) + } +} + +// --- #823 review: a running cluster is only reused when its port matches --- + +func TestStartUserCluster_RunningPortGuard(t *testing.T) { + t.Run("matching port is left alone", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + dataDir := initClusterDir(t) + writePostmasterPID(t, dataDir, port) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("a matching running cluster must not be restarted:\n%s", calls) + } + }) + + t.Run("mismatched port errors instead of waiting", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + portNumber, err := strconv.Atoi(port) + if err != nil { + t.Fatal(err) + } + livePort := strconv.Itoa(portNumber%65535 + 1) + dataDir := initClusterDir(t) + writePostmasterPID(t, dataDir, livePort) + configPath := filepath.Join(dataDir, "postgresql.conf") + beforeConfig, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running + + err = startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected an error when a cluster runs on a different port") + } + if !strings.Contains(err.Error(), livePort) || !strings.Contains(err.Error(), port) { + t.Fatalf("error should name both the live and requested ports: %v", err) + } + if strings.Contains(err.Error(), "--db-port") || !strings.Contains(err.Error(), "--db-host") { + t.Fatalf("error should recommend the real --db-host flag: %v", err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("a wrong-port cluster must not be restarted:\n%s", calls) + } + afterConfig, readErr := os.ReadFile(configPath) + if readErr != nil { + t.Fatal(readErr) + } + if string(afterConfig) != string(beforeConfig) { + t.Fatalf("a rejected endpoint must not rewrite postgresql.conf:\n%s", afterConfig) + } + }) + + t.Run("unknown running port errors", func(t *testing.T) { + dir, logPath := newStubPATH(t) + initClusterDir(t) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // running, no readable pid file + + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil || !strings.Contains(err.Error(), "port could not be read") { + t.Fatalf("expected an unknown-port error, got %v", err) + } + }) + + t.Run("unexpected socket directory errors", func(t *testing.T) { + dir, logPath := newStubPATH(t) + dataDir := initClusterDir(t) + writePostmasterPIDWithSocket(t, dataDir, "5432", "/tmp") + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // running on an unsafe socket + + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil { + t.Fatal("expected an error for a cluster using an unexpected socket directory") + } + if !strings.Contains(err.Error(), "/tmp") || !strings.Contains(err.Error(), "stop") { + t.Fatalf("error should name the unsafe socket and how to restart safely: %v", err) + } + }) +} + +// --- #823 review: the start error names the server log --- + +func TestStartUserCluster_StartErrorNamesLog(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + initClusterDir(t) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 1)) // not running, start fails + + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected a start failure") + } + if !strings.Contains(err.Error(), "server.log") { + t.Fatalf("start error should name the server log: %v", err) + } } diff --git a/docs-site/src/tools/run-local.md b/docs-site/src/tools/run-local.md index f08e2153c..861cdb7f3 100644 --- a/docs-site/src/tools/run-local.md +++ b/docs-site/src/tools/run-local.md @@ -47,9 +47,15 @@ so structural changes need a restart; behavioural changes do not. - A **PostgreSQL** database. Defaults: `127.0.0.1:5432`, user `mendix`, database derived from the project file name (`App1112.mpr` → `app1112`). Two ways to have it: - **`--ensure-db`** (recommended for a fresh session) provisions it: starts the - local Postgres service if the port is down, and creates the app role + database - if missing (via a local `sudo -u postgres` superuser). For a non-local `--db-host` - it only verifies reachability — mxcli won't provision a remote database. + local Postgres server if the port is down, and creates the app role + database + if missing. It uses a service manager, or a user-owned `initdb`/`pg_ctl` cluster + under `~/.mxcli/postgres` when no service becomes ready (e.g. Arch) — no + `postgres` OS account or `sudo` required. For a non-local `--db-host` it only verifies + reachability — mxcli won't provision a remote database. + The user-owned cluster persists across sessions; its server log is + `~/.mxcli/postgres/server.log`. Stop it with + `pg_ctl -D "$HOME/.mxcli/postgres/data" stop`. To remove it, stop it first and + then delete `~/.mxcli/postgres` (this permanently deletes its databases). - Otherwise create it once yourself; without `--ensure-db`, `run --local` stops with an actionable message if the DB is unreachable: