From f0139ba346d33defbc4acb6df3fb4a7e865911dc Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 03:18:15 +0300 Subject: [PATCH 1/8] fix(migrate): skip nginx's default vhost, resolve prefix-relative roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing a foreign nginx carried over its SHIPPED default vhost (`server_name localhost; root html;`) as if it were a site. That reached the vhost writer and died on "must be an absolute path" — an accurate sentence about config that is perfectly valid nginx — and surfaced to the operator as "1 site not served" for a placeholder page they never hosted. Two fixes at the importer: - Filter loopback `server_name`s where `_` and regex names were already being filtered. A loopback name only matches a request that ARRIVED with `Host: localhost`, so it can never be served for anybody through a public edge. A block left with no names falls into the existing no-usable-name skip, which already treats that as expected rather than a lost site. A vhost carrying both a loopback and a real name keeps the real one. - Resolve prefix-relative roots against nginx's compiled `--prefix`, read from `nginx -V`. `-T` inlines includes but does not rewrite directive VALUES, so a bare `root html` survives the dump verbatim and only the prefix says where it points. When no prefix is reported we skip with that as the stated reason rather than guessing the compiled default, because a wrong guess publishes the wrong directory. The `-V` call is lazy: only paid when a relative root is actually present. Also fixes the wizard's report of a PARTIAL import. `importMigratedSites` returns ok:false when even one site missed, so keying the warning off `ok` and printing the full count claimed every site was dark one line after the import itself said "Migrated 3/4". It now reports only the real shortfall, and leaves retry advice to the import — the only layer that knows whether the cause was transient or a config it will reject identically forever. --- apps/cli/src/commands/wizard.ts | 14 ++- .../adapters/src/system/proxy/import/nginx.ts | 98 +++++++++++++++++-- .../system/proxy/import/proxy-import.test.ts | 88 +++++++++++++++++ 3 files changed, 188 insertions(+), 12 deletions(-) diff --git a/apps/cli/src/commands/wizard.ts b/apps/cli/src/commands/wizard.ts index 9497b0215..ff0110571 100644 --- a/apps/cli/src/commands/wizard.ts +++ b/apps/cli/src/commands/wizard.ts @@ -919,10 +919,18 @@ export async function runWizard(): Promise { migratedCertPems, migratedStaticRootOverrides, ); - if (!imported.ok) { + // A PARTIAL import is not a total failure: `importMigratedSites` returns + // ok:false when even one site missed, so keying the warning off `ok` and + // printing `migratedSites.length` claimed every site was dark one line after + // the import itself said "Migrated 3/4". Report only the real shortfall, and + // leave the retry advice to the import — it's the only layer that knows + // whether the cause was transient (edge still starting) or a config it will + // reject identically on every re-run. + const missed = migratedSites.length - imported.registered.length; + if (missed > 0) { log.warn( - `Your ${migratedSites.length} existing site${migratedSites.length === 1 ? "" : "s"} ` + - "aren't served yet — re-run `openship up` to retry the import.", + `${missed} of your ${migratedSites.length} existing site${migratedSites.length === 1 ? "" : "s"} ` + + `${missed === 1 ? "isn't" : "aren't"} served yet — see the import output above.`, ); } } diff --git a/packages/adapters/src/system/proxy/import/nginx.ts b/packages/adapters/src/system/proxy/import/nginx.ts index e818aa855..4c171e9f6 100644 --- a/packages/adapters/src/system/proxy/import/nginx.ts +++ b/packages/adapters/src/system/proxy/import/nginx.ts @@ -45,6 +45,27 @@ async function dumpResolvedConfig( return null; } +/** + * nginx's compiled `--prefix` — the base a prefix-relative `root` resolves against. + * + * `-T` inlines every `include` but does NOT rewrite directive VALUES, so a stock + * `root html;` survives the dump verbatim and only nginx's own prefix says where it + * points. `-V` prints the configure line to STDERR, hence the redirect. + * + * Null when no binary answers, or when the build passed no `--prefix`: nginx's + * compiled-in default (`/usr/local/nginx`) is a guess, and a wrong guess here + * publishes the wrong directory to the internet. Callers skip the site with that as + * the stated reason instead. + */ +async function nginxPrefix(executor: CommandExecutor, bins: string[]): Promise { + for (const bin of bins) { + const out = await tryExec(executor, `${bin} -V 2>&1`); + const prefix = out?.match(/--prefix=(\S+)/)?.[1]; + if (prefix) return prefix; + } + return null; +} + async function loadNginxConfig(executor: CommandExecutor): Promise { const dumped = await dumpResolvedConfig(executor, ["nginx", "openresty"]); if (dumped) return dumped; @@ -275,15 +296,59 @@ function parseProxyDirectives(body: string): { }; } +/** + * Loopback `server_name`s, which are never migratable hostnames: they only match a + * request that ARRIVED with `Host: localhost` — an on-box curl — so a vhost claiming + * one cannot be served for anybody through a public edge. + * + * Filtering them alongside `_` and regex names is what keeps nginx's SHIPPED default + * vhost (`server_name localhost; root html;`) out of the migrate set: it is a + * placeholder welcome page, not a site, and carrying it over failed at APPLY time on + * its prefix-relative root — reported to the operator as "1 site not served" for a + * site that never existed. A block that has a loopback name AND a real one keeps the + * real one; a block left with nothing falls into the no-usable-name skip below. + */ +const LOOPBACK_SERVER_NAMES = new Set([ + "localhost", + "localhost.localdomain", + "ip6-localhost", + "ip6-loopback", +]); + +/** + * Absolutize a `root`. nginx treats a value not starting with `/` as relative to its + * compiled prefix, so a bare `html` is legal config meaning `/html`. + * + * Resolving HERE, where the prefix is known, is what keeps the raw token from + * reaching the vhost writer — which rejects it with "must be an absolute path", an + * accurate sentence about a config that is perfectly valid nginx, at the one moment + * (post-cutover) when the operator can least afford a misleading error. + */ +function resolveStaticRoot( + root: string, + prefix?: string, +): { root: string } | { reason: string } { + if (root.startsWith("/")) return { root }; + if (!prefix) { + return { + reason: + `static root "${root}" is relative to nginx's compiled prefix, ` + + `which this host didn't report`, + }; + } + return { root: `${prefix.replace(/\/+$/, "")}/${root}` }; +} + function parseServer( body: string, source: string, upstreams: Map, + prefix?: string, ): { site?: ImportedSite; warnings: string[] } { const warnings: string[] = []; const names = firstDirective(body, "server_name") ?.split(/\s+/) - .filter((n) => n && n !== "_" && !n.startsWith("~")) + .filter((n) => n && n !== "_" && !n.startsWith("~") && !LOOPBACK_SERVER_NAMES.has(n.toLowerCase())) ?? []; // ssl if any `listen ... ssl` or `listen 443` (443 as a whole token — not 8443) @@ -294,9 +359,10 @@ function parseServer( const certPath = firstDirective(body, "ssl_certificate"); const keyPath = firstDirective(body, "ssl_certificate_key"); - // No usable server_name = the default catch-all (`server_name _;` / omitted). - // It can't become a vhost (there's no hostname to register) and every nginx has - // one, so it's an expected skip, not a config item the operator lost. + // No usable server_name = the default catch-all (`server_name _;` / omitted), or a + // loopback-only block like nginx's shipped default vhost. It can't become a vhost + // (there's no routable hostname to register) and every nginx has one, so it's an + // expected skip, not a config item the operator lost. if (names.length === 0) return { warnings: [] }; // All routes for this vhost. Locations are the real source; fall back to a @@ -332,7 +398,12 @@ function parseServer( // only on :80 serving an empty webroot. return { warnings: [] }; } else if (root && !isAcmeWebrootOnly(body)) { - target = { kind: "static", root: root.replace(/;$/, "") }; + const resolved = resolveStaticRoot(root.replace(/;$/, ""), prefix); + if ("reason" in resolved) { + warnings.push(`nginx: ${names[0]} — ${resolved.reason} (skipped)`); + return { warnings }; + } + target = { kind: "static", root: resolved.root }; } else if (root) { // A root that exists ONLY to answer /.well-known/acme-challenge — certbot // scaffolding, not a site. Our edge answers ACME itself (nginx.conf proxies @@ -353,8 +424,11 @@ function parseServer( } /** Parse a raw nginx config string into normalized sites. Shared by `scanNginx` - * (foreign `/etc/nginx`) and `scanOpenshipEdge` (our OpenResty sites tree). */ -function parseNginxConfig(raw: string): ProxyScanResult { + * (foreign `/etc/nginx`) and `scanOpenshipEdge` (our OpenResty sites tree). + * + * `prefix` absolutizes a prefix-relative `root`; our own edge always writes + * absolute roots, so the "ours" callers pass none. */ +function parseNginxConfig(raw: string, prefix?: string): ProxyScanResult { const warnings: string[] = []; const sites: ImportedSite[] = []; @@ -372,7 +446,7 @@ function parseNginxConfig(raw: string): ProxyScanResult { } for (const body of blocks) { - const { site, warnings: blockWarnings } = parseServer(body, "nginx", upstreams); + const { site, warnings: blockWarnings } = parseServer(body, "nginx", upstreams, prefix); warnings.push(...blockWarnings); if (site) sites.push(site); } @@ -396,7 +470,13 @@ function parseNginxConfig(raw: string): ProxyScanResult { * answering only on port 80 with an empty webroot. */ export async function scanNginx(executor: CommandExecutor): Promise { - return parseNginxConfig(await loadNginxConfig(executor)); + const raw = await loadNginxConfig(executor); + // Only pay for the extra `-V` when a root that needs the prefix is actually + // present. A commented-out `root html;` false-positives the probe, which costs one + // cheap exec and nothing else — the prefix is unused if no site needs it. + const relativeRoot = /(?:^|[;{\s])root\s+[^/;\s]/.test(raw); + const prefix = relativeRoot ? await nginxPrefix(executor, ["nginx", "openresty"]) : null; + return parseNginxConfig(raw, prefix ?? undefined); } /** diff --git a/packages/adapters/src/system/proxy/import/proxy-import.test.ts b/packages/adapters/src/system/proxy/import/proxy-import.test.ts index abac706fc..5ac6c7d1d 100644 --- a/packages/adapters/src/system/proxy/import/proxy-import.test.ts +++ b/packages/adapters/src/system/proxy/import/proxy-import.test.ts @@ -397,6 +397,94 @@ describe("scanNginx", () => { expect(res.sites[0]!.proxy).toEqual({ proxyBusyBuffersSize: "32k" }); expect(res.sites[0]!.proxyRaw).toEqual({ proxyBusyBuffersSize: "32k" }); }); + + test("skips nginx's shipped default vhost without warning about it", async () => { + // Stock upstream nginx.conf. `server_name localhost` + the prefix-relative + // `root html` is a placeholder welcome page, not a site: importing it used to + // reach the vhost writer and die on "must be an absolute path", surfacing as + // "1 site not served" for something the operator never hosted. + const conf = ` + server { + listen 80 default_server; + server_name localhost; + root html; + index index.html; + } + server { + listen 80; + server_name real.example.com; + location / { proxy_pass http://127.0.0.1:3000; } + } + `; + const res = await scanNginx(makeExecutor([["nginx -T", conf]])); + expect(res.sites.map((s) => s.serverNames)).toEqual([["real.example.com"]]); + // An expected skip, so it must not be reported as a site the operator lost. + expect(res.warnings.join("\n")).not.toMatch(/localhost/); + }); + + test("keeps the real hostname on a vhost that also answers localhost", async () => { + const conf = ` + server { + listen 80; + server_name localhost app.example.com; + location / { proxy_pass http://127.0.0.1:3000; } + } + `; + const res = await scanNginx(makeExecutor([["nginx -T", conf]])); + expect(res.sites).toHaveLength(1); + expect(res.sites[0]!.serverNames).toEqual(["app.example.com"]); + }); + + test("absolutizes a prefix-relative static root against nginx's --prefix", async () => { + const conf = ` + server { + listen 80; + server_name docs.example.com; + root html; + } + `; + const res = await scanNginx( + makeExecutor([ + ["nginx -T", conf], + ["nginx -V", "nginx version: nginx/1.24.0\nconfigure arguments: --prefix=/usr/share/nginx --with-http_v2_module"], + ]), + ); + expect(res.sites[0]!.target).toEqual({ kind: "static", root: "/usr/share/nginx/html" }); + }); + + test("skips a relative static root when no prefix is reported, with a reason", async () => { + // Guessing nginx's compiled default would publish the wrong directory. + const conf = ` + server { + listen 80; + server_name docs.example.com; + root html; + } + `; + const res = await scanNginx(makeExecutor([["nginx -T", conf]])); + expect(res.sites).toHaveLength(0); + expect(res.warnings.join("\n")).toMatch(/docs\.example\.com.*relative to nginx's compiled prefix/); + }); + + test("does not run -V when every root is already absolute", async () => { + const conf = ` + server { + listen 80; + server_name static.example.com; + root /var/www/site; + } + `; + const calls: string[] = []; + const executor = { + exec: async (cmd: string) => { + calls.push(cmd); + return cmd.includes("nginx -T") ? conf : ""; + }, + } as unknown as CommandExecutor; + const res = await scanNginx(executor); + expect(res.sites[0]!.target).toEqual({ kind: "static", root: "/var/www/site" }); + expect(calls.some((c) => c.includes("-V"))).toBe(false); + }); }); describe("scanCaddy", () => { From 8b790577889da87dabc1cfb2693f2bf125a9493f Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 03:19:02 +0300 Subject: [PATCH 2/8] fix(mail): run mailbox creation in the engine, not on the host (GH-562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a mailbox returned 500 on every containerized install, with no actionable error anywhere in the log. `doveadm` and the `vmail` user live wherever Dovecot does, which on a container-flavor box is INSIDE `openship-mail` and emphatically not on the host. Both helpers ran bare against the raw host executor: - `hashPassword` ran `doveadm pw -s SSHA512`, got empty output from a host that has no doveadm, and threw when the SSHA512 gate rejected it. - `createMaildirOnDisk` ran `chown -R vmail:vmail` against a host with no `vmail` user, failing the whole `&&` chain after the DB rows were inserted. The reporter's own evidence confirms the mismatch: they verified doveadm was available IN THE CONTAINER while the code was running it on the host. Both now go through `runMailCommand(target, flavor => mailEngineCommand(...))`, which also buys the wrong-flavor reclassification a hand-rolled exec skips. Two traps that came with it: - `mailEngineCommand` prefixes `docker exec `, so a top-level `a && b` would run `a` in the engine and `b` ON THE HOST — the exact bug class being fixed. The maildir chain is handed to one `sh -c`, matching the precedent at mail.service.ts:91. - The maildir tree was one level too shallow. `mail_location` is `maildir:%Lh/Maildir/` with `home = //`, so the tree Dovecot opens is `/Maildir/{cur,new,tmp}`, not `/{cur,new,tmp}`. Harmless only because the LDA autocreates the real one, which is also why nothing caught it. `mail-credentials.service.ts` carried a PRIVATE COPY of the hasher plus a hardcoded `sudo -u postgres psql`, i.e. the same defect twice. Both are deleted in favour of the shared helpers, so the transport is decided in one place. Separately, a 5xx `AppError` was logged NOWHERE — and `AppError`'s statusCode defaults to 500, so `new AppError(msg)` answered 500 in silence. That is the "500 with no detailed error in the logs" half of the report. 5xx now logs with the request method and path; 4xx stays quiet, since those are client outcomes. Tests assert the COMMAND STRING per flavor. Every existing mail test hands these helpers a vi.fn() executor and asserts on the resulting rows, which is precisely how a command that could not run anywhere stayed green. --- apps/api/src/middleware/error-handler.ts | 21 +- apps/api/src/modules/mail/admin/maildir.ts | 74 ++++--- apps/api/src/modules/mail/admin/password.ts | 35 +++- .../modules/mail/mail-credentials.service.ts | 64 ++---- .../mail/mailbox-create-flavor.test.ts | 195 ++++++++++++++++++ 5 files changed, 307 insertions(+), 82 deletions(-) create mode 100644 apps/api/test/modules/mail/mailbox-create-flavor.test.ts diff --git a/apps/api/src/middleware/error-handler.ts b/apps/api/src/middleware/error-handler.ts index 62c4bf766..e3d937abf 100644 --- a/apps/api/src/middleware/error-handler.ts +++ b/apps/api/src/middleware/error-handler.ts @@ -32,6 +32,12 @@ export function handleApiError(err: unknown, c: Context) { if (err instanceof AppError) { const { message, code, statusCode } = err; + // A 5xx is a SERVER fault and must leave a trace, even when it arrives as a typed + // AppError carrying its own message. `AppError`'s statusCode defaults to 500, so + // a bare `new AppError(msg)` used to answer 500 and log NOTHING — the "500 with no + // actionable information in the logs" of GH-562. 4xx stays quiet on purpose: those + // are client outcomes, and logging them turns ordinary validation into noise. + if (statusCode >= 500) console.error(`[API ERROR] ${requestTag(c)}`, err); return c.json( { error: message, code }, // 502/503 included: an AppError can legitimately mean "an upstream we @@ -50,6 +56,19 @@ export function handleApiError(err: unknown, c: Context) { return c.json({ error: "Invalid JSON body", code: "INVALID_JSON" }, 400); } - console.error("[UNHANDLED ERROR]", err); + // Log the route with it. `[UNHANDLED ERROR] Error: doveadm pw returned …` on its own + // doesn't say WHICH request produced it, which is most of the work of diagnosing a + // 500 from a log file. The response body stays deliberately generic — an unknown + // error's message can carry internals we don't hand to a client. + console.error(`[UNHANDLED ERROR] ${requestTag(c)}`, err); return c.json({ error: "Internal server error" }, 500); } + +/** `METHOD /path` for a log line. Query string omitted: it can carry tokens. */ +function requestTag(c: Context): string { + try { + return `${c.req.method} ${new URL(c.req.url).pathname}`; + } catch { + return c.req.method; + } +} diff --git a/apps/api/src/modules/mail/admin/maildir.ts b/apps/api/src/modules/mail/admin/maildir.ts index ffbde2750..c805d42c3 100644 --- a/apps/api/src/modules/mail/admin/maildir.ts +++ b/apps/api/src/modules/mail/admin/maildir.ts @@ -19,8 +19,7 @@ * `userdb` query returns them and the LDA places mail in the right path. */ -import type { CommandExecutor } from "@repo/adapters"; -import { sshManager } from "../../../lib/ssh-manager"; +import { mailEngineCommand, runMailCommand, type MailTarget } from "../mail-engine"; export const STORAGE_BASE = "/var/vmail"; export const STORAGE_NODE = "vmail1"; @@ -76,35 +75,54 @@ export function generateMaildir( } /** - * Create the Maildir directory tree on the target VPS: + * The maildir root INSIDE the mailbox home, i.e. what Dovecot actually opens. * - * /var/vmail/vmail1//{cur,new,tmp} + * `mail_location = maildir:%Lh/Maildir/:INDEX=%Lh/Maildir/` (engine/samples/dovecot/ + * dovecot.conf:64) and the userdb query sets `home` to + * `//` (dovecot-sql.conf:19) — so the + * tree lives one level BELOW the home, not at it. This used to create + * `/{cur,new,tmp}`, a tree Dovecot never reads: harmless only because the LDA + * autocreates the real one on first delivery, which is also why nothing caught it. + */ +const MAILDIR_SUBDIR = "Maildir"; + +/** + * Create the Maildir directory tree where the mail engine lives: + * + * /var/vmail/vmail1//Maildir/{cur,new,tmp} * * Owned by `vmail:vmail` (the system user iRedMail's installer creates) so * Postfix/Dovecot can write into it. Mode `0700` per Dovecot's expectation. * + * Runs through `runMailCommand`, so it lands wherever `/var/vmail` and the `vmail` + * user actually are. Running it bare on the host executor is half of #562: a + * container-flavor host has no `vmail` user, so `chown` failed, the `&&` chain + * failed, and mailbox creation 500'd after already inserting the DB rows. + * + * The whole chain is handed to ONE `sh -c` rather than joined with `&&` at the top + * level, because `mailEngineCommand` prefixes `docker exec ` — an + * unwrapped `a && b` would run `a` in the engine and `b` on the HOST. Same wrapping + * precedent as mail.service.ts:91 and :863. + * * Idempotent: `mkdir -p` is fine if the path already exists, and `chown` * over an existing tree is harmless. */ export async function createMaildirOnDisk( - serverIdOrExec: string | CommandExecutor, + serverIdOrExec: MailTarget, layout: MaildirLayout, ): Promise { - const fullPath = `${layout.storagebasedirectory}/${layout.storagenode}/${layout.maildir}`; // Trailing slash already in maildir field; we don't need to add it again. - const cmd = [ - `mkdir -p ${shellQuote(fullPath + "cur")}`, - `mkdir -p ${shellQuote(fullPath + "new")}`, - `mkdir -p ${shellQuote(fullPath + "tmp")}`, - `chown -R vmail:vmail ${shellQuote(fullPath)}`, - `chmod -R 0700 ${shellQuote(fullPath)}`, + const home = maildirHome(layout); + const root = `${home}${MAILDIR_SUBDIR}`; + const script = [ + `mkdir -p ${shellQuote(`${root}/cur`)} ${shellQuote(`${root}/new`)} ${shellQuote(`${root}/tmp`)}`, + `chown -R vmail:vmail ${shellQuote(home)}`, + `chmod -R 0700 ${shellQuote(home)}`, ].join(" && "); - if (typeof serverIdOrExec === "string") { - await sshManager.withExecutor(serverIdOrExec, (exec) => exec.exec(cmd)); - } else { - await serverIdOrExec.exec(cmd); - } + await runMailCommand(serverIdOrExec, (flavor) => + mailEngineCommand(flavor, `sh -c ${shellQuote(script)}`), + ); } /** @@ -116,22 +134,28 @@ export async function createMaildirOnDisk( * has already validated the mailbox exists. */ export async function removeMaildirOnDisk( - serverIdOrExec: string | CommandExecutor, + serverIdOrExec: MailTarget, layout: MaildirLayout, ): Promise { - const fullPath = `${layout.storagebasedirectory}/${layout.storagenode}/${layout.maildir}`; + const fullPath = maildirHome(layout); // Guard: refuse to rm -rf anything that's not under /var/vmail/ if (!fullPath.startsWith(`${STORAGE_BASE}/`)) { throw new Error( `Refusing to remove maildir outside ${STORAGE_BASE}/: ${fullPath}`, ); } - const cmd = `rm -rf ${shellQuote(fullPath)}`; - if (typeof serverIdOrExec === "string") { - await sshManager.withExecutor(serverIdOrExec, (exec) => exec.exec(cmd)); - } else { - await serverIdOrExec.exec(cmd); - } + // Removes the home, so the `Maildir/` subtree inside it goes with it — no need to + // know the layout below. Flavor-routed for the same reason as the create path: + // `/var/vmail` is a volume in the engine, not a host directory. + await runMailCommand(serverIdOrExec, (flavor) => + mailEngineCommand(flavor, `rm -rf ${shellQuote(fullPath)}`), + ); +} + +/** The mailbox home — what the userdb query returns as `home`. Keeps the one path + * concatenation in a single place so create and remove cannot disagree. */ +function maildirHome(layout: MaildirLayout): string { + return `${layout.storagebasedirectory}/${layout.storagenode}/${layout.maildir}`; } function shellQuote(s: string): string { diff --git a/apps/api/src/modules/mail/admin/password.ts b/apps/api/src/modules/mail/admin/password.ts index 02dece091..5117b67d5 100644 --- a/apps/api/src/modules/mail/admin/password.ts +++ b/apps/api/src/modules/mail/admin/password.ts @@ -5,14 +5,22 @@ * `password` column. Hashing happens ON the target VPS via `doveadm pw` * so neither cleartext nor hash transits any intermediate process. * + * "On the target VPS" is not the same as "on the target HOST": `doveadm` lives + * wherever Dovecot does, which on a container-flavor box is inside the engine and + * NOT on the host. Running it bare against the host executor is what made every + * mailbox create return a 500 on a containerized install (#562) — the host has no + * `doveadm`, so the exec produced nothing, the hash regex rejected the empty + * string, and the thrown message never reached the operator. Every invocation now + * goes through `runMailCommand`, which resolves the box's flavor and renders the + * right prefix; see `../mail-engine.ts` for the topology matrix. + * * This is the same scheme used by `mail-credentials.service.ts` for the * postmaster password rotation - kept as a separate small module so the * admin services can call it without depending on the broader credentials * surface. */ -import type { CommandExecutor } from "@repo/adapters"; -import { sshManager } from "../../../lib/ssh-manager"; +import { mailEngineCommand, runMailCommand, type MailTarget } from "../mail-engine"; const SSHA512_HASH_RE = /^\{SSHA512\}[A-Za-z0-9+/=]+$/; @@ -25,22 +33,27 @@ function shellQuote(s: string): string { * `{SSHA512}...` string ready to drop into the `password` column. * * Throws if doveadm returns anything that doesn't match the expected hash - * format - easier to fail at hash time than to debug a broken auth row. + * format - easier to fail at hash time than to debug a broken auth row. The + * plaintext is never echoed in that error, only the (non-)hash we got back. */ export async function hashPassword( - serverIdOrExec: string | CommandExecutor, + serverIdOrExec: MailTarget, plaintext: string, ): Promise { - const cmd = `doveadm pw -s SSHA512 -p ${shellQuote(plaintext)}`; - const out = - typeof serverIdOrExec === "string" - ? await sshManager.withExecutor(serverIdOrExec, (exec) => exec.exec(cmd)) - : await serverIdOrExec.exec(cmd); + const { output, flavor } = await runMailCommand(serverIdOrExec, (f) => + mailEngineCommand(f, `doveadm pw -s SSHA512 -p ${shellQuote(plaintext)}`), + ); - const hash = out.trim(); + const hash = output.trim(); if (!SSHA512_HASH_RE.test(hash)) { + // Name the transport in the message: on a container box the overwhelmingly + // likely cause is that `doveadm` is missing from the engine image, and an + // error that only says "unexpected output" sends the reader hunting the + // password instead. throw new Error( - `doveadm pw returned unexpected output: ${hash.slice(0, 60)}…`, + `doveadm pw (${flavor} engine) returned no usable SSHA512 hash — got ${ + hash ? `"${hash.slice(0, 60)}…"` : "empty output" + }. Check that doveadm is present and Dovecot's config is readable.`, ); } return hash; diff --git a/apps/api/src/modules/mail/mail-credentials.service.ts b/apps/api/src/modules/mail/mail-credentials.service.ts index e9a302524..ab3fb4248 100644 --- a/apps/api/src/modules/mail/mail-credentials.service.ts +++ b/apps/api/src/modules/mail/mail-credentials.service.ts @@ -4,10 +4,10 @@ * Flow: * 1. Hash the new password with `doveadm pw -s SSHA512` (the scheme * iRedMail's default `dovecot-sql.conf` uses for the `password` - * column). Hashing on the target server avoids sending the - * cleartext or the hash through any intermediate process. - * 2. UPDATE vmail.mailbox SET password = '' WHERE username = … - * via `sudo -u postgres psql`. + * column), via the shared `admin/password.ts` helper. Hashing on the + * target server avoids sending the cleartext or the hash through any + * intermediate process. + * 2. UPDATE vmail.mailbox SET password = … through `admin/psql-runner`. * 3. Scrub any leftover plaintext from the state file. We used to mirror * it back for the credentials card to display; that was a needless * attack surface and is gone - the only way to "know" the password @@ -15,37 +15,10 @@ */ import type { CommandExecutor } from "@repo/adapters"; +import { hashPassword } from "./admin/password"; +import { execute, q } from "./admin/psql-runner"; import { readState, mutateState } from "./mail-state"; -/** - * Shell-quote an arbitrary string so it survives as a single argv element - * inside a `bash -c …` command. Wraps in single quotes and escapes any - * embedded single quotes via the standard `'\''` trick. - */ -function shellQuote(s: string): string { - return `'${s.replace(/'/g, "'\\''")}'`; -} - -/** - * Hash a plaintext password via doveadm. Returns the `{SSHA512}...` string - * ready to drop into the `password` column. - */ -async function hashWithDovecot( - exec: CommandExecutor, - plaintext: string, -): Promise { - const out = await exec.exec( - `doveadm pw -s SSHA512 -p ${shellQuote(plaintext)}`, - ); - const hash = out.trim(); - if (!hash.startsWith("{SSHA512}")) { - throw new Error( - `doveadm pw returned unexpected output: ${hash.slice(0, 60)}…`, - ); - } - return hash; -} - /** * Update the postmaster password for ``. Caller is responsible * for validation (length, etc.) - this function trusts the input. @@ -59,23 +32,24 @@ export async function updatePostmasterPassword( newPassword: string, ): Promise { const username = `postmaster@${domain}`; - const hash = await hashWithDovecot(exec, newPassword); + // Shared with the admin panel's mailbox create/update, so the hash scheme and the + // engine-vs-host transport are decided in exactly one place. This used to be a + // private copy that ran `doveadm` bare on the host executor, which is dead on a + // container-flavor box (#562) — the same defect the mailbox-create path had. + const hash = await hashPassword(exec, newPassword); - // Sanity-check the values we're about to embed. Both come from controlled - // sources (doveadm output + `postmaster@`), so this is - // belt-and-suspenders against an upstream surprise. - if (!/^\{SSHA512\}[A-Za-z0-9+/=]+$/.test(hash)) { - throw new Error("doveadm pw returned a hash with unexpected characters"); - } + // Belt-and-suspenders against an upstream surprise: the username is derived from an + // already-validated domain, and `hashPassword` has its own format gate. if (!/^[A-Za-z0-9._-]+@[A-Za-z0-9.-]+$/.test(username)) { throw new Error(`Refusing to update for suspicious username: ${username}`); } - // iRedMail's pg_hba.conf grants the local `postgres` Unix user passwordless - // access. Single-quote-wrap the SQL string literals - hash chars are - // [A-Za-z0-9+/={}], username is similarly tame, so no escape gymnastics. - const psqlCmd = `sudo -u postgres psql -d vmail -v ON_ERROR_STOP=1 -c "UPDATE mailbox SET password='${hash}' WHERE username='${username}';"`; - await exec.exec(psqlCmd); + // Through psql-runner so the invocation matches the box's topology (the engine's pg + // sidecar, or `sudo -u postgres` on a legacy install) rather than assuming the latter. + await execute( + exec, + `UPDATE mailbox SET password = ${q(hash)} WHERE username = ${q(username)};`, + ); // Persist the new plaintext into state.secrets so the test-email flow // (and any future SMTP-from-orchestrator use) can authenticate over diff --git a/apps/api/test/modules/mail/mailbox-create-flavor.test.ts b/apps/api/test/modules/mail/mailbox-create-flavor.test.ts new file mode 100644 index 000000000..0c7f452ae --- /dev/null +++ b/apps/api/test/modules/mail/mailbox-create-flavor.test.ts @@ -0,0 +1,195 @@ +import "./_setup-env"; + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * The mailbox-creation path must reach the mail engine, not the host (GH-562). + * + * `doveadm` and the `vmail` user live wherever Dovecot does. On a container-flavor + * box that is INSIDE `openship-mail` and emphatically not on the host — yet + * `hashPassword` ran a bare `doveadm pw` and `createMaildirOnDisk` ran a bare + * `chown -R vmail:vmail`, both straight against the host executor. The result was a + * 500 on every mailbox create on a containerized install: no `doveadm` on the host + * meant empty output, and the SSHA512 gate rejected it. + * + * Every existing test in this directory hands these helpers a `vi.fn()` executor and + * asserts on the ROWS, so all of them stayed green while the command that produced + * those rows could not run anywhere. The assertions here are deliberately on the + * COMMAND STRING, because the prefix is the entire bug. + * + * These run on every PR — no daemon needed. The companion real-Docker case is + * test/e2e/mail-db-bootstrap.e2e.test.ts, which covers the other half of GH-562. + */ + +vi.mock("@repo/adapters", () => ({ + HOST_STATE_DIR: "/root/.openship", + detectMailEngine: vi.fn(), + MAIL_CONTAINER: "openship-mail", + MAIL_DB_CONTAINER: "openship-mail-db", + MAIL_DB_NAME: "vmail", + MAIL_HOST_PATHS: { + saslPasswd: "/opt/openship/mail/postfix/sasl_passwd", + senderRelayhost: "/opt/openship/mail/postfix/sender_relayhost", + amavisUserConf: "/opt/openship/mail/amavis/50-user", + }, +})); + +import { detectMailEngine } from "@repo/adapters"; +import { hashPassword } from "../../../src/modules/mail/admin/password"; +import { + createMaildirOnDisk, + generateMaildir, + removeMaildirOnDisk, +} from "../../../src/modules/mail/admin/maildir"; +import { forgetMailEngine } from "../../../src/modules/mail/mail-engine"; + +const HASH = "{SSHA512}c2FsdGVkaGFzaHZhbHVl"; + +const CONTAINER = { + flavor: "container" as const, + running: true, + exists: true, + image: "ghcr.io/oblien/openship-mail:0.6.5", +}; +const HOST = { flavor: "host" as const, running: true, exists: true, image: null }; + +/** An executor that records every command and answers with `reply`. */ +function recorder(reply: string) { + const calls: string[] = []; + const exec = { + exec: vi.fn(async (cmd: string) => { + calls.push(cmd); + return reply; + }), + }; + forgetMailEngine(exec as never); + return { exec, calls, last: () => calls[calls.length - 1] ?? "" }; +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("hashPassword transport", () => { + it("runs doveadm INSIDE the engine on a container-flavor box", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(HASH); + + await expect(hashPassword(r.exec as never, "sekrit-pw")).resolves.toBe(HASH); + + // The prefix IS the fix. Without it the host answers, and the host has no doveadm. + expect(r.last()).toContain("docker exec openship-mail "); + expect(r.last()).toContain("doveadm pw -s SSHA512"); + }); + + it("runs doveadm bare on a legacy host-flavor box", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(HOST); + const r = recorder(HASH); + + await expect(hashPassword(r.exec as never, "sekrit-pw")).resolves.toBe(HASH); + + expect(r.last()).not.toContain("docker exec"); + expect(r.last()).toContain("doveadm pw -s SSHA512"); + }); + + it("keeps the plaintext out of the thrown message when doveadm answers nothing", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + + // The empty-output case is exactly what a missing doveadm produced, and the + // operator saw only "500 Internal Server Error" for it. + await expect(hashPassword(r.exec as never, "sekrit-pw")).rejects.toThrow(/empty output/); + await expect(hashPassword(r.exec as never, "sekrit-pw")).rejects.not.toThrow(/sekrit-pw/); + }); + + it("names the engine flavor in the failure, so the reader knows which box answered", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder("doveadm: command not found"); + + await expect(hashPassword(r.exec as never, "pw")).rejects.toThrow(/container engine/); + }); +}); + +describe("createMaildirOnDisk transport + layout", () => { + it("creates the tree Dovecot actually opens: /Maildir/{cur,new,tmp}", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + const layout = generateMaildir("acme.com", "alice", new Date("2026-01-02T03:04:05Z")); + + await createMaildirOnDisk(r.exec as never, layout); + const cmd = r.last(); + + // mail_location = maildir:%Lh/Maildir/ (engine/samples/dovecot/dovecot.conf:64), + // with home = //. A tree at /cur is one level shallow + // and Dovecot never reads it. + const home = `/var/vmail/vmail1/${layout.maildir}`; + expect(cmd).toContain(`${home}Maildir/cur`); + expect(cmd).toContain(`${home}Maildir/new`); + expect(cmd).toContain(`${home}Maildir/tmp`); + expect(cmd).not.toContain(`'${home}cur'`); + }); + + it("wraps the compound command in one sh -c so && cannot leak to the host shell", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + + await createMaildirOnDisk(r.exec as never, generateMaildir("acme.com", "bob")); + const cmd = r.last(); + + // `docker exec a && b` would run `a` in the engine and `b` on the HOST — + // which is how a chown meant for the engine's vmail user hits a host without one. + expect(cmd).toMatch(/^docker exec openship-mail sh -c /); + const afterShC = cmd.slice(cmd.indexOf("sh -c ")); + expect(afterShC.startsWith("sh -c '")).toBe(true); + // Every && must sit INSIDE the quoted script, never between top-level words. + expect(cmd.replace(/'.*'/s, "''")).not.toContain("&&"); + }); + + it("chowns to vmail INSIDE the engine, where that user exists", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + + await createMaildirOnDisk(r.exec as never, generateMaildir("acme.com", "carol")); + + expect(r.last()).toContain("chown -R vmail:vmail"); + expect(r.last()).toMatch(/^docker exec openship-mail /); + }); + + it("stays bare on a legacy host-flavor box", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(HOST); + const r = recorder(""); + + await createMaildirOnDisk(r.exec as never, generateMaildir("acme.com", "dave")); + + expect(r.last()).not.toContain("docker exec"); + expect(r.last()).toMatch(/^sh -c /); + }); +}); + +describe("removeMaildirOnDisk", () => { + it("removes the home through the engine, covering the Maildir subtree", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + const layout = generateMaildir("acme.com", "erin"); + + await removeMaildirOnDisk(r.exec as never, layout); + + expect(r.last()).toBe( + `docker exec openship-mail rm -rf '/var/vmail/vmail1/${layout.maildir}'`, + ); + }); + + it("still refuses a path outside /var/vmail before any command is built", async () => { + vi.mocked(detectMailEngine).mockResolvedValue(CONTAINER); + const r = recorder(""); + + await expect( + removeMaildirOnDisk(r.exec as never, { + storagebasedirectory: "/etc", + storagenode: "passwd", + maildir: "", + }), + ).rejects.toThrow(/Refusing to remove maildir outside/); + expect(r.calls).toHaveLength(0); + }); +}); From 2e13efa33f9380fb5e760c8fcfb89ab93d03f471 Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 03:20:10 +0300 Subject: [PATCH 3/8] fix(mail): make the DB bootstrap fail loudly instead of silently (GH-562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On first deploy the mail engine came up with no schema: dovecot, iredapd and amavis crash-looped against an empty database while the boot log claimed success, and the only recovery was running the bootstrap by hand. Three compounding defects, and the third is why it was silent: 1. The wait was `nc -z` in entrypoint.sh. A TCP probe succeeds as soon as postgres BINDS its port, which is before it will serve a query — during initdb or crash recovery it accepts the connection and then refuses with "the database system is starting up". The loop also fell through after 60 tries WITHOUT checking, so an absent sidecar proceeded anyway. 2. `bash db-bootstrap.sh || log "ERROR: ..."` downgraded a hard failure to a log line and then started every daemon against a schemaless DB. 3. db-bootstrap.sh ran `set -uo pipefail` with NO `-e`, so it never returned non-zero for (2) to catch. Every psql failed in turn and it still printed "── DB bootstrap complete ──" and exited 0. Verified against the pre-change script: exit 0, "complete" printed, and the postmaster password column literally empty. Now: `set -euo pipefail`, a `SELECT 1` readiness poll that lives INSIDE the script (so the documented manual re-run is self-sufficient), and the closing four psql prints turned into ASSERTIONS — including one that refuses to report success when the postmaster row has an empty password. `PGCONNECT_TIMEOUT` is not optional: against a host that drops packets libpq blocks for the OS default, so the wait budget never gets a second iteration and the boot hangs with no output. The e2e caught that. Two `set -e` hazards the e2e also caught: `[ -f x ] && cmd` returns non-zero for a missing optional file, and a bare `ls` of an absent directory exits 2 — on the WARN path, so it aborted the bootstrap on its way to printing a warning. `doveadm` joins the Dockerfile smoke gate, and the gate now runs `doveadm pw` to prove it works. It only ever arrived as a transitive dependency — iRedMail's package list names dovecot-imapd/pop3d/lmtpd/managesieved/sieve/pgsql, never dovecot-core — so nothing guaranteed the binary three separate paths depend on. The e2e runs the REAL script against a real postgres sidecar with the repo's real engine/samples, stubbing only doveadm so its failure mode can be driven. Booting openship-mail itself is not viable in a test: unpublished image, full iRedMail install to build. Files travel by `docker cp` rather than a bind mount so the gate does not evaporate on a macOS/Colima daemon. --- .../test/e2e/mail-db-bootstrap.e2e.test.ts | 244 ++++++++++++++++++ apps/email/Dockerfile | 18 +- apps/email/docker/db-bootstrap.sh | 113 +++++++- apps/email/docker/entrypoint.sh | 53 ++-- 4 files changed, 397 insertions(+), 31 deletions(-) create mode 100644 apps/api/test/e2e/mail-db-bootstrap.e2e.test.ts diff --git a/apps/api/test/e2e/mail-db-bootstrap.e2e.test.ts b/apps/api/test/e2e/mail-db-bootstrap.e2e.test.ts new file mode 100644 index 000000000..e2451750b --- /dev/null +++ b/apps/api/test/e2e/mail-db-bootstrap.e2e.test.ts @@ -0,0 +1,244 @@ +/** + * The mail engine's DB bootstrap, against a REAL PostgreSQL sidecar (GH-562). + * + * The bug this exists for could not be caught by any unit test, because the defect + * was the script's *failure semantics* rather than its SQL. `db-bootstrap.sh` ran + * under `set -uo pipefail` with no `-e`, so when it started before the sidecar was + * ready every psql failed in turn, the script reached its closing + * "── DB bootstrap complete ──" and exited 0. The caller's `|| log "ERROR"` could + * therefore never fire. Operators saw dovecot, iredapd and amavis crash-loop against + * an empty database with nothing anywhere explaining why, and the only recovery was + * to run the script by hand. + * + * A second, quieter variant: `doveadm pw` ran with `2>/dev/null` and its output + * unvalidated, so a missing `doveadm` seeded the postmaster with an EMPTY password + * and still reported success. + * + * What this asserts is therefore mostly about EXIT CODES and the absence of false + * success — the three cases below all passed (exit 0) before the fix. + * + * Why not boot `openship-mail` itself: that image is not published, and building it + * runs the full iRedMail installer — minutes to tens of minutes, which is not a test. + * The script's real collaborators are bash, psql, perl and the engine's SQL samples, + * so a Debian `postgres:16` runner with the repo's actual `engine/samples` copied in + * exercises the real script against a real database. `doveadm` is the one collaborator + * that must be stubbed, and stubbing it is what lets us drive its failure mode + * deliberately. The Dockerfile smoke gate covers doveadm's real presence. + * + * Files travel by `docker cp`, not a bind mount: a mount only works when the daemon + * can see the host path, which is false for a macOS temp dir under Colima and true in + * CI — i.e. the one setup where a gate must not evaporate. Same reasoning as + * edge-not-found-page.e2e.test.ts. + * + * Skips without a reachable daemon, FAILS under RUN_DOCKER_E2E=1 (what CI sets). + * See test/helpers/docker-e2e.ts. + */ + +import { it, expect, beforeAll, afterAll } from "vitest"; +import { execFile } from "node:child_process"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { promisify } from "node:util"; +import { describeDockerE2E, requireDocker } from "../helpers/docker-e2e"; + +const execFileAsync = promisify(execFile); +const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), "../../../.."); +const EMAIL_DIR = join(REPO_ROOT, "apps/email"); + +/** Unique per run so a leftover container from a killed run can't collide. */ +const SUFFIX = process.pid.toString(36); +const NET = `openship-e2e-mailboot-${SUFFIX}`; +const DB = `openship-e2e-mailboot-db-${SUFFIX}`; +const RUNNER = `openship-e2e-mailboot-run-${SUFFIX}`; + +const DB_IMAGE = "postgres:16-alpine"; +/** Debian-based: the script is bash and uses perl for iRedMail's PH_ substitutions. */ +const RUNNER_IMAGE = "postgres:16"; + +const PG_PASSWORD = "e2e-root-pw"; +const BIND_PASSWORD = "e2e-bind-pw"; +const POSTMASTER_PLAIN = "e2e-postmaster-pw"; +const FIRST_DOMAIN = "e2e-mail.example"; +/** Any well-formed SSHA512 value; the script only validates the shape. */ +const STUB_HASH = "{SSHA512}ZTJlLXN0dWItaGFzaC12YWx1ZQ=="; + +const SCRIPT_IN_RUNNER = "/opt/openship-mail/db-bootstrap.sh"; + +async function docker(args: string[], opts: { allowFail?: boolean } = {}) { + try { + const { stdout, stderr } = await execFileAsync("docker", args, { + maxBuffer: 32 * 1024 * 1024, + }); + return { code: 0, stdout, stderr }; + } catch (err) { + const e = err as { code?: number; stdout?: string; stderr?: string; message?: string }; + if (!opts.allowFail) { + throw new Error( + `docker ${args.slice(0, 3).join(" ")} failed: ${e.stderr || e.message || "unknown"}`, + ); + } + return { code: typeof e.code === "number" ? e.code : 1, stdout: e.stdout ?? "", stderr: e.stderr ?? "" }; + } +} + +/** Run db-bootstrap.sh in the runner. Never throws — the exit code IS the assertion. */ +async function runBootstrap(env: Record = {}) { + const envArgs = Object.entries({ + FIRST_DOMAIN, + VMAIL_DB_BIND_PASSWD: BIND_PASSWORD, + DOMAIN_ADMIN_PASSWD_PLAIN: POSTMASTER_PLAIN, + PGSQL_ROOT_PASSWD: PG_PASSWORD, + OPENSHIP_MAIL_DB_HOST: DB, + OPENSHIP_MAIL_DB_PORT: "5432", + ...env, + }).flatMap(([k, v]) => ["--env", `${k}=${v}`]); + + const r = await docker( + ["exec", ...envArgs, RUNNER, "bash", SCRIPT_IN_RUNNER], + { allowFail: true }, + ); + return { ...r, log: `${r.stdout}\n${r.stderr}` }; +} + +/** A one-shot query as the postgres superuser, from inside the runner. */ +async function query(sql: string, db = "vmail"): Promise { + const r = await docker([ + "exec", + "--env", `PGPASSWORD=${PG_PASSWORD}`, + RUNNER, + "psql", "-h", DB, "-U", "postgres", "-d", db, "-tAc", sql, + ]); + return r.stdout.trim(); +} + +/** + * Replace the stubbed `doveadm` so its failure mode can be driven deliberately. + * + * `%b`, not `%s`: printf only interprets `\n` in the FORMAT for %b, so `%s` wrote the + * two-character sequence and produced a one-line file that is not a runnable script. + * That silently turned every case into the "doveadm is broken" case. + */ +async function setDoveadmStub(body: string): Promise { + await docker([ + "exec", RUNNER, "bash", "-c", + `printf '%b\\n' ${JSON.stringify(body)} > /usr/bin/doveadm && chmod +x /usr/bin/doveadm`, + ]); + // Prove the stub is executable and behaves, so a broken stub can never masquerade + // as a finding about the script under test. + await docker(["exec", RUNNER, "/usr/bin/doveadm", "pw", "-s", "SSHA512", "-p", "x"], { + allowFail: true, + }); +} + +describeDockerE2E("mail engine DB bootstrap (real postgres)", () => { + beforeAll(async () => { + await requireDocker(); + + await docker(["network", "create", NET]); + + // vmail is PRE-CREATED here exactly as the real sidecar does it (POSTGRES_DB), + // because the script loads schema into it rather than creating it. + await docker([ + "run", "-d", "--name", DB, "--network", NET, + "--env", `POSTGRES_PASSWORD=${PG_PASSWORD}`, + "--env", "POSTGRES_DB=vmail", + DB_IMAGE, + ]); + + await docker([ + "run", "-d", "--name", RUNNER, "--network", NET, + "--entrypoint", "sleep", RUNNER_IMAGE, "3600", + ]); + + // The REAL script and the REAL iRedMail SQL samples. + await docker(["exec", RUNNER, "mkdir", "-p", "/opt/openship-mail", "/opt/iRedMail-engine"]); + await docker(["cp", join(EMAIL_DIR, "docker/db-bootstrap.sh"), `${RUNNER}:${SCRIPT_IN_RUNNER}`]); + await docker(["cp", join(EMAIL_DIR, "engine/samples"), `${RUNNER}:/opt/iRedMail-engine/samples`]); + + await setDoveadmStub(`#!/bin/sh\necho '${STUB_HASH}'`); + }, 300_000); + + afterAll(async () => { + await docker(["rm", "-f", RUNNER], { allowFail: true }); + await docker(["rm", "-f", DB], { allowFail: true }); + await docker(["network", "rm", NET], { allowFail: true }); + }, 120_000); + + // Ordered on purpose: each case leaves the database in the state the next expects, + // and the doveadm case must run while the schema is still absent. + it("waits for a database that is not ready yet instead of failing against it", async () => { + // The runner and the sidecar started together, so this first call races postgres's + // own initdb — which is the GH-562 race, reproduced rather than simulated. The old + // `nc -z` probe returned as soon as the port bound and the bootstrap proceeded. + // Nothing is asserted about timing; the point is that it SUCCEEDS below. + const r = await runBootstrap(); + expect(r.log).toMatch(/waiting for the mail database|mail database answered|bootstrapping/i); + expect(r.code).toBe(0); + }, 300_000); + + it("seeded the first domain and a postmaster with a NON-EMPTY password", async () => { + expect(await query("SELECT count(*) FROM domain")).toBe("1"); + expect(await query("SELECT domain FROM domain LIMIT 1")).toBe(FIRST_DOMAIN); + + // The whole point of the hash validation: this column must never be blank. + const blank = await query( + `SELECT count(*) FROM mailbox WHERE coalesce(password,'') = ''`, + ); + expect(blank).toBe("0"); + expect(await query(`SELECT password FROM mailbox WHERE username = 'postmaster@${FIRST_DOMAIN}'`)) + .toBe(STUB_HASH); + }, 60_000); + + it("is idempotent: a second run skips instead of re-seeding", async () => { + const r = await runBootstrap(); + expect(r.code).toBe(0); + expect(r.log).toMatch(/already present — skipping/i); + expect(await query("SELECT count(*) FROM domain")).toBe("1"); + }, 120_000); + + it("FAILS instead of reporting success when doveadm produces nothing", async () => { + // Reproduces a mail image without doveadm. Before the fix this seeded an empty + // password and exited 0; the operator learned about it when auth silently failed. + await setDoveadmStub("#!/bin/sh\nexit 127"); + try { + // Drop the gate so the run gets past the idempotency check to the hash step. + await docker(["exec", "--env", `PGPASSWORD=${PG_PASSWORD}`, RUNNER, + "psql", "-h", DB, "-U", "postgres", "-d", "vmail", "-c", "DROP TABLE mailbox CASCADE"]); + + const r = await runBootstrap(); + + expect(r.code).not.toBe(0); + expect(r.log).toMatch(/doveadm pw produced no output|is doveadm installed/i); + // And it must not have claimed success on the way out. + expect(r.log).not.toMatch(/DB bootstrap complete/); + } finally { + await setDoveadmStub(`#!/bin/sh\necho '${STUB_HASH}'`); + } + }, 180_000); + + it("FAILS with a diagnosable message when the database never answers", async () => { + const r = await runBootstrap({ + // TEST-NET-3 drops rather than refuses, which is the realistic bad case (a + // firewall, a wrong host). It is also the case that proves PGCONNECT_TIMEOUT is + // doing its job: without it libpq blocks for the OS default and the wait budget + // below never gets a second iteration. + OPENSHIP_MAIL_DB_HOST: "203.0.113.1", + OPENSHIP_MAIL_DB_WAIT_SECS: "4", + PGCONNECT_TIMEOUT: "2", + }); + + expect(r.code).not.toBe(0); + expect(r.log).toMatch(/did not accept queries within 4s/); + // The message has to point somewhere. A bare "failed" is what made this a + // multi-hour hunt in the field. + expect(r.log).toMatch(/docker logs openship-mail-db/); + expect(r.log).not.toMatch(/DB bootstrap complete/); + }, 180_000); + + it("negative control: the harness can still observe a failing script", async () => { + // A container test that quietly stopped running the script would look exactly + // like a passing one. Prove a non-zero exit is actually detected. + const r = await docker(["exec", RUNNER, "bash", "-c", "exit 3"], { allowFail: true }); + expect(r.code).toBe(3); + }, 60_000); +}); diff --git a/apps/email/Dockerfile b/apps/email/Dockerfile index d9802683f..a4b203f0d 100644 --- a/apps/email/Dockerfile +++ b/apps/email/Dockerfile @@ -95,9 +95,18 @@ RUN cp /opt/openship-mail/build-config /opt/iRedMail-engine/config \ # an image that passes this test is guaranteed to have something to put on :25. # NB: Debian's amavisd-new package ships the daemon as /usr/sbin/amavisd (there is # no `amavisd-new` executable), which is why supervisord invokes /usr/sbin/amavisd. +# +# `doveadm` is not a supervisord program but is gated with them anyway, because three +# separate paths are dead without it: db-bootstrap.sh hashes the postmaster password, +# and the control plane hashes on every mailbox create and postmaster rotation. It +# arrives only as a transitive dependency of the dovecot packages iRedMail installs +# (packages.sh names dovecot-imapd/pop3d/lmtpd/managesieved/sieve/pgsql, never +# dovecot-core), so nothing guaranteed it was here. Its absence used to surface as a +# postmaster row with an EMPTY password and a 500 on mailbox create (GH-562) — a +# runtime mystery for a property the build can simply assert. RUN set -eu; \ missing=""; \ - for b in /usr/sbin/postfix /usr/sbin/dovecot /usr/sbin/amavisd \ + for b in /usr/sbin/postfix /usr/sbin/dovecot /usr/bin/doveadm /usr/sbin/amavisd \ /usr/sbin/clamd /usr/bin/freshclam /usr/sbin/spamd \ /usr/bin/fail2ban-server /opt/iredapd/iredapd.py; do \ [ -e "$b" ] || missing="$missing $b"; \ @@ -107,7 +116,12 @@ RUN set -eu; \ echo "The openship-mail image would ship with no mail stack (issue #493)." >&2; \ exit 1; \ fi; \ - echo "openship-mail: all mail daemons present ->$(echo ' postfix dovecot amavisd clamd freshclam spamd fail2ban iredapd')" + doveadm pw -s SSHA512 -p build-smoke-test | grep -q '^{SSHA512}' || { \ + echo "FATAL: doveadm is present but cannot produce an SSHA512 hash." >&2; \ + echo "Mailbox creation and the postmaster seed both depend on it (GH-562)." >&2; \ + exit 1; \ + }; \ + echo "openship-mail: all mail daemons present ->$(echo ' postfix dovecot doveadm amavisd clamd freshclam spamd fail2ban iredapd')" # Runtime prerequisites the installer does not leave in place under our stubbed- # init build (its late service/DB steps abort — see the `|| true` above): diff --git a/apps/email/docker/db-bootstrap.sh b/apps/email/docker/db-bootstrap.sh index a901950a4..ebc67db18 100644 --- a/apps/email/docker/db-bootstrap.sh +++ b/apps/email/docker/db-bootstrap.sh @@ -11,9 +11,23 @@ # the five mail roles share one secret and privilege separation is by GRANTs). # # Idempotent: a `mailbox` table in vmail means we already ran — exit early. -# Called by entrypoint.sh AFTER the password reconcile and the wait-for-sidecar. -set -uo pipefail +# Called by entrypoint.sh AFTER the password reconcile, and safe to re-run by hand. +# +# FAILURE SEMANTICS (GH-562). This script used to run under `set -uo pipefail` with no +# `-e`, so a psql that could not connect did not stop it: every step failed in turn and +# it still reached the closing "DB bootstrap complete" and exited 0. The caller's +# `|| log "ERROR"` could therefore never fire, and the operator saw daemons crash-loop +# against an empty database with nothing anywhere saying why. Two rules now hold: +# +# 1. `set -e` — any failed step aborts with a non-zero exit the caller can act on. +# 2. Nothing is assumed ready. We WAIT for the database to answer a real query (not +# just accept a TCP connection) and fail loudly if it never does. +# +# The wait lives here rather than only in entrypoint.sh because the documented recovery +# is to run this script by hand, and it must be self-sufficient on that path too. +set -euo pipefail log() { echo "[db-bootstrap] $*"; } +fatal() { echo "[db-bootstrap] FATAL: $*" >&2; exit 1; } ENGINE=/opt/iRedMail-engine SAMPLES="$ENGINE/samples" @@ -23,8 +37,43 @@ rm -rf "$WORK"; mkdir -p "$WORK" DB_HOST="${OPENSHIP_MAIL_DB_HOST:-127.0.0.1}" DB_PORT="${OPENSHIP_MAIL_DB_PORT:-5432}" export PGPASSWORD="${PGSQL_ROOT_PASSWD:-}" +# Bound the TCP connect. Without it the wait budget below is decorative: against a host +# that silently drops packets (a firewall, a wrong OPENSHIP_MAIL_DB_HOST) libpq blocks +# for the OS default — minutes per attempt — so a "180s" budget never gets to iterate +# and the operator watches the boot hang with no output at all. 5s is far above a +# loopback or same-host sidecar connect. +export PGCONNECT_TIMEOUT="${PGCONNECT_TIMEOUT:-5}" psql_su() { psql -h "$DB_HOST" -p "$DB_PORT" -U postgres -v ON_ERROR_STOP=1 "$@"; } +# ── wait for the sidecar to ANSWER, not merely to listen ────────────────────── +# A TCP probe (`nc -z`) is not readiness: postgres binds its port before it can serve, +# and during initdb or crash recovery it accepts the connection then refuses the query +# with "the database system is starting up". That gap is the GH-562 race — the bootstrap +# began while the sidecar was still starting, so every statement failed. `SELECT 1` +# exercises DNS, TCP, auth AND readiness, which is exactly the set that can be late. +# Note the `if`s rather than `[ … ] && log …`: under `set -e` a false test as the last +# command of an AND-list aborts the script, so the terse form would exit on the very +# first (already-ready) probe. +wait_for_db() { + local budget="${OPENSHIP_MAIL_DB_WAIT_SECS:-180}" waited=0 + until psql_su -d postgres -tAc 'SELECT 1' >/dev/null 2>&1; do + if [ "$waited" -ge "$budget" ]; then + fatal "the mail database at ${DB_HOST}:${DB_PORT} did not accept queries within ${budget}s. + The sidecar is unreachable, still initializing, or rejecting our credentials. + Check: docker logs openship-mail-db + A host process already on ${DB_PORT} is the most common cause." + fi + if [ "$waited" -eq 0 ]; then + log "waiting for the mail database at ${DB_HOST}:${DB_PORT}..." + fi + sleep 2 + waited=$((waited + 2)) + done + if [ "$waited" -gt 0 ]; then + log "mail database answered after ${waited}s" + fi +} + # ── iRedMail conventions (conf/global, conf/postfix, conf/dovecot) ──────────── export VMAIL_DB_NAME="${OPENSHIP_MAIL_DB_NAME:-vmail}" export VMAIL_DB_BIND_USER=vmail @@ -54,7 +103,14 @@ export DOMAIN_ADMIN_MAILDIR_HASH_PART="${FIRST_DOMAIN}/p/o/s/postmaster/" # GRANTs, not distinct passwords. (Per-role passwords = future hardening.) export MAIL_DB_PW="${VMAIL_DB_BIND_PASSWD:?VMAIL_DB_BIND_PASSWD required}" -# ── 0. idempotency gate — vmail schema already present? ─────────────────────── +# ── 0. the database must be answering before anything below can mean anything ── +# Deliberately AFTER the env validation above: those checks are free, and failing them +# after a 3-minute wait would bury the real complaint. +wait_for_db + +# ── 0b. idempotency gate — vmail schema already present? ────────────────────── +# The `2>/dev/null | grep -q 1` here is safe now that the DB is known-reachable: a +# missing table is the only thing an empty result can still mean. if psql_su -d "$VMAIL_DB_NAME" -tAc \ "SELECT 1 FROM information_schema.tables WHERE table_name='mailbox'" 2>/dev/null | grep -q 1; then log "vmail schema already present — skipping DB bootstrap"; exit 0 @@ -89,7 +145,19 @@ cp -f "$SAMPLES/postgresql/sql/grant_permissions.sql" "$WORK/grant.sql" cp -f "$SAMPLES/postgresql/sql/add_first_domain_and_user.sql" "$WORK/first.sql" cp -f "$TRIG" "$WORK/trigger.sql" -export DOMAIN_ADMIN_PASSWD_HASH="$(doveadm pw -s SSHA512 -p "$DOMAIN_ADMIN_PASSWD_PLAIN" 2>/dev/null)" +# The postmaster's password hash. `2>/dev/null` used to hide the one failure that +# matters: if doveadm is missing or Dovecot's config is unreadable this produced an +# EMPTY string, which was then substituted into first.sql — seeding a postmaster with +# no password while the bootstrap reported success. Keep stderr, and refuse anything +# that isn't a well-formed SSHA512 hash. +DOMAIN_ADMIN_PASSWD_HASH="$(doveadm pw -s SSHA512 -p "$DOMAIN_ADMIN_PASSWD_PLAIN" || true)" +export DOMAIN_ADMIN_PASSWD_HASH +case "$DOMAIN_ADMIN_PASSWD_HASH" in + '{SSHA512}'*) : ;; + '') fatal "doveadm pw produced no output — is doveadm installed in this image? + The postmaster account would have been created with an EMPTY password." ;; + *) fatal "doveadm pw returned something that is not an SSHA512 hash: ${DOMAIN_ADMIN_PASSWD_HASH:0:40}…" ;; +esac # iRedMail's exact PH_ substitutions ($ENV = safe for '.', '/', etc. in values). for f in "$WORK/iredmail.sql" "$WORK/grant.sql" "$WORK/first.sql"; do @@ -130,7 +198,11 @@ make_db() { # [extra-sql-files...] psql_su -d postgres -tAc "SELECT 1 FROM pg_database WHERE datname='$db'" | grep -q 1 \ || psql_su -d postgres -c "CREATE DATABASE $db WITH OWNER $role TEMPLATE template0 ENCODING 'UTF8'" local f - for f in "$@"; do [ -f "$f" ] && psql_su -d "$db" -f "$f"; done + # `if` rather than `[ -f "$f" ] && psql_su …`: under `set -e` the AND-list returns + # non-zero for a missing optional file and would abort the whole bootstrap. + for f in "$@"; do + if [ -f "$f" ]; then psql_su -d "$db" -f "$f"; fi + done psql_su -d "$db" </dev/null; fi +else + # `|| true` on the diagnostic: under `set -e` a bare `ls` of a directory that isn't + # there exits 2 and would abort the bootstrap on its way to reporting a WARNING. + log "WARN: iredapd schema missing at $IREDAPD_SQL" + ls -la /opt/iredapd 2>/dev/null || true +fi # ── 5. fail2ban ─────────────────────────────────────────────────────────────── if [ -f "$SAMPLES/fail2ban/sql/fail2ban.pgsql" ]; then @@ -161,8 +238,24 @@ if [ -f "$SAMPLES/fail2ban/sql/fail2ban.pgsql" ]; then make_db fail2ban fail2ban "$SAMPLES/fail2ban/sql/fail2ban.pgsql" else log "WARN: fail2ban schema missing"; fi +# ── 6. verify, don't narrate ────────────────────────────────────────────────── +# These four reads used to print and nothing more, so "── DB bootstrap complete ──" +# was emitted whether or not a single statement above had worked. They are now +# ASSERTIONS: the script may only claim success if the seeded state is actually there. +seeded_domains="$(psql_su -d "$VMAIL_DB_NAME" -tAc "SELECT count(*) FROM domain")" +seeded_mailboxes="$(psql_su -d "$VMAIL_DB_NAME" -tAc "SELECT count(*) FROM mailbox")" +seeded_databases="$(psql_su -d postgres -tAc \ + "SELECT string_agg(datname,',' ORDER BY datname) FROM pg_database WHERE datname IN ('vmail','amavisd','iredapd','fail2ban')")" + +log "vmail.domain=${seeded_domains} vmail.mailbox=${seeded_mailboxes} databases=${seeded_databases}" + +[ "${seeded_domains:-0}" -ge 1 ] || fatal "vmail.domain is empty — the first domain was not seeded." +[ "${seeded_mailboxes:-0}" -ge 1 ] || fatal "vmail.mailbox is empty — the postmaster account was not created." + +# An empty password column here is the failure this bootstrap used to ship silently. +if psql_su -d "$VMAIL_DB_NAME" -tAc \ + "SELECT 1 FROM mailbox WHERE username='${DOMAIN_ADMIN_EMAIL}' AND coalesce(password,'') = ''" | grep -q 1; then + fatal "${DOMAIN_ADMIN_EMAIL} was created with an EMPTY password — refusing to report success." +fi + log "── DB bootstrap complete ──" -psql_su -d "$VMAIL_DB_NAME" -tAc "SELECT 'vmail.domain='||count(*) FROM domain" -psql_su -d "$VMAIL_DB_NAME" -tAc "SELECT 'vmail.mailbox='||count(*) FROM mailbox" -psql_su -d "$VMAIL_DB_NAME" -tAc "SELECT 'seeded_domain='||domain FROM domain LIMIT 1" -psql_su -d postgres -tAc "SELECT 'databases='||string_agg(datname,',' ORDER BY datname) FROM pg_database WHERE datname IN ('vmail','amavisd','iredapd','fail2ban')" diff --git a/apps/email/docker/entrypoint.sh b/apps/email/docker/entrypoint.sh index add941494..dee08063b 100644 --- a/apps/email/docker/entrypoint.sh +++ b/apps/email/docker/entrypoint.sh @@ -9,14 +9,15 @@ # real per-install values from the --env-file — the `build-placeholder` DB # password (shared role) and the `build.invalid` domain (-> $FIRST_DOMAIN), # the latter also writing /etc/mailname + an /etc/hosts FQDN entry. -# 3. wait for the postgres SIDECAR (127.0.0.1:5432). -# 4. bootstrap the mail databases (roles + schema + first domain) if the vmail -# schema isn't there yet — see db-bootstrap.sh; never re-init an existing DB. -# 5. pre-create the log files fail2ban tails (rsyslog fills them once daemons +# 3. bootstrap the mail databases (roles + schema + first domain) if the vmail +# schema isn't there yet — see db-bootstrap.sh, which owns the wait for the +# sidecar and never re-inits an existing DB. FATAL on failure: an engine +# without its schema cannot serve, and pretending otherwise is GH-562. +# 4. pre-create the log files fail2ban tails (rsyslog fills them once daemons # log; a jail whose logpath is missing at start would crash-loop). -# 6. reuse-or-generate the DKIM key on its bind mount (never regenerate — a new +# 5. reuse-or-generate the DKIM key on its bind mount (never regenerate — a new # selector breaks DMARC until DNS repropagates). -# 7. hand off to supervisord (the CMD). +# 6. hand off to supervisord (the CMD). # # Env (from ensure-container-mail.ts --env-file): FIRST_DOMAIN, # OPENSHIP_MAIL_DB_{HOST,PORT,NAME,USER}, plus iRedMail secrets @@ -26,8 +27,9 @@ set -euo pipefail log() { echo "[openship-mail] $*"; } -DB_HOST="${OPENSHIP_MAIL_DB_HOST:-127.0.0.1}" -DB_PORT="${OPENSHIP_MAIL_DB_PORT:-5432}" +# No DB_HOST/DB_PORT here on purpose: db-bootstrap.sh reads the same two env vars and +# owns every conversation with the sidecar, so duplicating them invites the two files +# to disagree about where the database is. FIRST_DOMAIN="${FIRST_DOMAIN:-}" SEED_DIR="/opt/openship-mail/seed" @@ -127,17 +129,30 @@ if [ -n "$FIRST_DOMAIN" ]; then esac fi -# 3. wait for the sidecar DB. -log "waiting for the mail database at ${DB_HOST}:${DB_PORT}..." -for _ in $(seq 1 60); do - if nc -z "$DB_HOST" "$DB_PORT" 2>/dev/null; then break; fi - sleep 2 -done - -# 4. bootstrap the mail databases (idempotent; skips if the vmail schema exists). -bash /opt/openship-mail/db-bootstrap.sh || log "ERROR: db-bootstrap failed — inspect the log above" +# 3. bootstrap the mail databases (idempotent; skips if the vmail schema exists). +# +# The wait for the sidecar lives INSIDE db-bootstrap.sh, which polls `SELECT 1` until +# the database actually answers. This used to be an `nc -z` loop here, and that was +# half of GH-562: a TCP probe succeeds as soon as postgres binds its port, which is +# before it will serve a query — so the bootstrap started against a database that was +# still initializing. The loop also fell through after 60 tries without checking, so an +# absent sidecar proceeded anyway. A weaker duplicate probe here would add nothing. +# +# A failure is FATAL rather than a log line. There is no case where this exits non-zero +# and the engine can still work: either the database is unreachable (no daemon can +# authenticate) or the schema did not load (dovecot, iredapd and amavis all crash on +# their first query). Continuing produced the reported symptom — every daemon +# crash-looping while the boot log claimed success. Dying here instead means the log +# names the cause once, and `verifyMailEngine`'s port probe correctly reports the +# engine as down instead of reporting a healthy install. +if ! bash /opt/openship-mail/db-bootstrap.sh; then + log "FATAL: mail database bootstrap failed — see the [db-bootstrap] lines above." + log " The engine will not start without its schema. After fixing the cause, recreate" + log " the container, or re-run: docker exec openship-mail bash /opt/openship-mail/db-bootstrap.sh" + exit 1 +fi -# 5. pre-create the log files the fail2ban jails tail, so a jail never starts +# 4. pre-create the log files the fail2ban jails tail, so a jail never starts # against a missing path (rsyslog populates them as the daemons log). mkdir -p /var/log/dovecot /var/log/iredapd /var/log/supervisor touch /var/log/mail.log \ @@ -146,7 +161,7 @@ touch /var/log/mail.log \ /var/log/iredapd/iredapd.log chown -R iredapd:iredapd /var/log/iredapd 2>/dev/null || true -# 6. DKIM: reuse the key on the mount, else generate one (per domain). +# 5. DKIM: reuse the key on the mount, else generate one (per domain). if [ -n "$FIRST_DOMAIN" ] && [ ! -s "/var/lib/dkim/${FIRST_DOMAIN}.pem" ]; then log "generating DKIM key for ${FIRST_DOMAIN}" # Debian ships the daemon as `amavisd` (no `amavisd-new` executable); try it From f3199fc76a246b8e45d50e18582ffa930a087dec Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 03:52:56 +0300 Subject: [PATCH 4/8] feat(notifications): register mail.inbound_received + a self-hosted-only mail group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wiring only, no producer yet. It lands first because an unmapped eventType is dropped by `notification.emit` with NO log and NO delivery row — so without this the inbound-rules UI would look like it works while nothing was ever sent. - CATEGORY_GROUPS gains `mail`, placed before `billing` so the cloud-only group stays last in the Settings tab strip. The group id union is derived from this array (`as const satisfies`), so the category below would not compile without it. - `mail.inbound_received` category, with defaultEnabled FALSE and deliberately so: the dispatcher's fallback fans a default-enabled category to every org member's verified email channel with no opt-in, and for a per-message event that is both a flood and a mail loop — the notification would land on the same engine, inside a watched domain, and capture itself. - EVENT_TYPE_TO_CATEGORY mapping, without which the emit is silent. - No EVENT_HEADLINES entry: the registry test only permits an override on a category carrying more than one mapped eventType, so adding one there fails CI. The wording lives in the category label/description, which renderMessage takes verbatim as the alert's title and first body line. `listCategories` becomes symmetric instead of billing-specific: each group is dropped in the mode that can never produce it — billing outside CLOUD_MODE, mail inside it, since the whole mail module is absent from the cloud runtime. The filter stays in the controller rather than in CATEGORIES, because findCategory supplies the title and body of every delivered alert and the registry has to stay complete or a stored row degrades to a raw id. The audit taxonomy entry is required even though this emit never writes an audit_event row: the taxonomy scan greps apps/api/src for `eventType:` literals and is over-inclusive by design. Tests pin the direction of the new gate (mail hidden on cloud, present self-hosted, registry still complete either way) and that inbound mail can never default to enabled. The pinned category-id array is an ordered compare, so the new id is inserted at its real position. --- apps/api/src/lib/notification-categories.ts | 26 +++++++++++++++ .../notifications/notifications.controller.ts | 27 ++++++++------- .../test/lib/notification-categories.test.ts | 1 + .../categories-cloud-gate.test.ts | 33 +++++++++++++++++++ packages/core/src/audit-taxonomy.ts | 10 ++++++ 5 files changed, 85 insertions(+), 12 deletions(-) diff --git a/apps/api/src/lib/notification-categories.ts b/apps/api/src/lib/notification-categories.ts index 7c73371b1..cd97d5f68 100644 --- a/apps/api/src/lib/notification-categories.ts +++ b/apps/api/src/lib/notification-categories.ts @@ -32,6 +32,9 @@ export const CATEGORY_GROUPS = [ { id: "jobs", label: "Jobs" }, { id: "domains", label: "Domains & SSL" }, { id: "members", label: "Members" }, + // Self-hosted-only, and dropped from `listCategories` under CLOUD_MODE — the mirror + // image of `billing` below. Placed before it so the cloud-only group stays last. + { id: "mail", label: "Mail" }, { id: "billing", label: "Billing" }, ] as const satisfies readonly { id: string; label: string }[]; @@ -211,6 +214,24 @@ export const CATEGORIES: readonly NotificationCategory[] = [ defaultEnabled: false, }, + // Self-hosted-only: fed by the mail engine, so `listCategories` drops this group + // under CLOUD_MODE — the inverse of the billing block below. It stays in the + // registry regardless, because `findCategory` supplies the title and first body + // line of every alert already stored against it. + // + // defaultEnabled MUST stay false. The dispatcher's fallback fans a default-enabled + // category to every org member's verified email channel with no opt-in, and for a + // PER-MESSAGE event that is both a flood and a mail loop: the notification mail + // would land on the same engine, inside a watched domain, and capture itself. + { + id: "mail.inbound_received", + group: "mail", + label: "Inbound email received", + description: + "A message arrived at a mailbox or domain one of your inbound rules watches.", + defaultEnabled: false, + }, + // Cloud-only: both are fed by Stripe/Oblien, so `listCategories` drops the whole // group outside CLOUD_MODE rather than showing toggles that can never fire. They // stay in the registry regardless — `findCategory` still has to render a message @@ -290,6 +311,11 @@ const EVENT_TYPE_TO_CATEGORY: Record = { "invitation.sent": "invitation.sent", "invitation.created": "invitation.sent", + // Mail (self-hosted engine). Without this entry `notification.emit` returns with no + // log and no delivery row, so the rules UI would look like it works while nothing + // is ever sent. + "mail.inbound_received": "mail.inbound_received", + // Billing "billing.payment_failed": "billing.alert", "billing.invoice_overdue": "billing.alert", diff --git a/apps/api/src/modules/notifications/notifications.controller.ts b/apps/api/src/modules/notifications/notifications.controller.ts index b6a24d093..fd3f1f4fc 100644 --- a/apps/api/src/modules/notifications/notifications.controller.ts +++ b/apps/api/src/modules/notifications/notifications.controller.ts @@ -42,21 +42,24 @@ const VALID_CHANNEL_KINDS = new Set([ /** * GET /categories — the static registry, plus the groups the Settings UI tabs by. * - * Billing is dropped outside CLOUD_MODE: those two categories are fed by - * Stripe/Oblien, so on a self-hosted box they are toggles that can never fire. - * The filter lives HERE and not in `CATEGORIES` on purpose — `findCategory` - * supplies the title and body of every delivered alert - * (notification-workers.ts) and the dispatcher's `defaultEnabled` fallback, so - * the registry has to stay complete or an org that already holds a billing row - * would start rendering the raw category id. + * Each group is dropped in the mode that can never produce it: `billing` is fed by + * Stripe/Oblien so it is cloud-only, and `mail` is fed by the self-hosted mail engine + * (the whole mail module is absent in cloud) so it is the mirror image. Either way a + * toggle that can never fire is worse than no toggle. + * + * The filter lives HERE and not in `CATEGORIES` on purpose — `findCategory` supplies the + * title and body of every delivered alert (notification-workers.ts) and the dispatcher's + * `defaultEnabled` fallback, so the registry has to stay complete or an org that already + * holds a row for a hidden category would start rendering the raw category id. + * + * Both lists are filtered symmetrically: a category whose group is gone would render + * under no tab at all. */ export async function listCategories(c: Context) { - if (env.CLOUD_MODE) { - return c.json({ categories: CATEGORIES, groups: CATEGORY_GROUPS }); - } + const hidden = new Set(env.CLOUD_MODE ? ["mail"] : ["billing"]); return c.json({ - categories: CATEGORIES.filter((cat) => cat.group !== "billing"), - groups: CATEGORY_GROUPS.filter((g) => g.id !== "billing"), + categories: CATEGORIES.filter((cat) => !hidden.has(cat.group)), + groups: CATEGORY_GROUPS.filter((g) => !hidden.has(g.id)), }); } diff --git a/apps/api/test/lib/notification-categories.test.ts b/apps/api/test/lib/notification-categories.test.ts index e1b51cf8c..4823ab54f 100644 --- a/apps/api/test/lib/notification-categories.test.ts +++ b/apps/api/test/lib/notification-categories.test.ts @@ -72,6 +72,7 @@ describe("notification category registry", () => { "member.added", "member.removed", "invitation.sent", + "mail.inbound_received", "billing.alert", "quota.warning", ]); diff --git a/apps/api/test/modules/notifications/categories-cloud-gate.test.ts b/apps/api/test/modules/notifications/categories-cloud-gate.test.ts index e477d7b2d..a968622f7 100644 --- a/apps/api/test/modules/notifications/categories-cloud-gate.test.ts +++ b/apps/api/test/modules/notifications/categories-cloud-gate.test.ts @@ -88,4 +88,37 @@ describe("GET /categories billing gate", () => { const { categories, groups } = await fetchCategories(true); expect(new Set(categories.map((c) => c.group))).toEqual(new Set(groups.map((g) => g.id))); }); + + // Mail is the mirror image of billing: the engine is self-hosted-only and the whole + // mail module is absent from the cloud runtime, so the gate has to run the other way. + // Nothing else pins the DIRECTION, and a later "simplification" that filtered + // CATEGORIES itself — or dropped one of the two branches — would ship green. + it("keeps the mail group on self-hosted", async () => { + const { categories, groups } = await fetchCategories(false); + expect(groups.map((g) => g.id)).toContain("mail"); + expect(categories.map((c) => c.id)).toContain("mail.inbound_received"); + }); + + it("drops the mail group from both arrays on cloud", async () => { + const { categories, groups } = await fetchCategories(true); + + expect(groups.map((g) => g.id)).not.toContain("mail"); + expect(categories.map((c) => c.id)).not.toContain("mail.inbound_received"); + const groupIds = new Set(groups.map((g) => g.id)); + for (const cat of categories) expect(groupIds).toContain(cat.group); + }); + + it("still renders an inbound-mail alert on a cloud box", async () => { + // Same registry-stays-complete guarantee as billing, in the other direction: a row + // stored before a migration to cloud must keep its label, not degrade to the raw id. + await fetchCategories(true); + expect(findCategory("mail.inbound_received")?.label).toBe("Inbound email received"); + }); + + // Per-message events must never default on: the dispatcher's fallback fans a + // default-enabled category to every member's verified email channel, and a + // notification mail landing back on the watched engine captures itself. + it("never defaults inbound mail to enabled", async () => { + expect(findCategory("mail.inbound_received")?.defaultEnabled).toBe(false); + }); }); diff --git a/packages/core/src/audit-taxonomy.ts b/packages/core/src/audit-taxonomy.ts index 288935b33..62e5ded45 100644 --- a/packages/core/src/audit-taxonomy.ts +++ b/packages/core/src/audit-taxonomy.ts @@ -412,6 +412,16 @@ export const AUDIT_EVENTS: Record = { label: "Mail server admin action", tone: "warning", }, + // Catalogued because the taxonomy scan is deliberately over-inclusive: it greps + // apps/api/src for `eventType:` literals, so a notification-only emit that never + // writes an audit_event row is caught the same as an audit write. Without this the + // suite fails; with it, an operator who DOES surface these sees a real label. + "mail.inbound_received": { + category: "servers", + action: "received inbound mail matching an inbound rule on", + label: "Inbound email received", + tone: "info", + }, /* ---------------- Members & access ---------------- */ "organization.created": { From 4aa10808c74251e9d2294da782e3a8ea5f5db747 Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 03:52:56 +0300 Subject: [PATCH 5/8] =?UTF-8?q?feat(mail):=20inbound=20filter=20=E2=80=94?= =?UTF-8?q?=20loop=20guards,=20spam=20gate,=20fail-closed=20scope=20matchi?= =?UTF-8?q?ng?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision layer for inbound-mail rules, kept PURE so none of its failure modes need a mail server to test. All three are silent in production: LOOPS. A notification about mail is itself mail. Four guards, none of them tuning knobs: a null envelope sender (a bounce — and notifying on bounces is how a bounce storm becomes an alert storm), `Auto-Submitted` other than `no`, bulk/junk `Precedence`, `List-Id`, plus this instance's own outbound sender addresses. Miss any one and a single alert delivered to an address in a watched domain re-captures itself until somebody notices. SPAM. There is no upstream filter to rely on: the shipped amavis policy sets spam_lover='Y' AND bad_header_lover='Y' on the catch-all '@.' policy with empty quarantine targets, so the global $final_spam_destiny = D_DISCARD never applies to any recipient. Spam is delivered and therefore captured. Bad-header mail is delivered too and carries no X-Spam-Flag at all, which is why the score is checked independently of the flag. With no threshold set, a positive flag alone drops. FAIL CLOSED. There is no CHECK constraint tying `scope` to `target` (this schema has none anywhere), so a mailbox/domain rule with a null target is representable. Treated as "no constraint" it would silently widen to every message on the server — an operator who mistyped a rule quietly shipping a whole domain's mail metadata to Slack. It matches nothing instead, as does a scope this build does not recognise. Two smaller decisions worth the comment they carry: - Header parsing unfolds continuation lines. Long To/Subject values ARE folded in real mail, and a naive line split would truncate a subject and lose half a recipient list — which would make a mailbox rule miss its own target. - Operator patterns are case-insensitive substring with `*` as the ONLY wildcard, never a regex. Every metacharacter is escaped: these come from a text box and run on the mail path, so a pasted `(a+)+$` must not stall it. Known limit, documented in the module header because it is a property of Postfix's config and not something the control plane can fix: `enable_original_recipient = no` means the BCC copy carries no X-Original-To, so a mailbox-scope rule can only attribute a message via To/Cc and will miss anything Bcc'd or alias-expanded. A domain-scope rule has no such gap, since capture itself is domain-keyed. --- apps/api/src/modules/mail/inbound/filter.ts | 259 ++++++++++++++++++ .../test/modules/mail/inbound-filter.test.ts | 209 ++++++++++++++ 2 files changed, 468 insertions(+) create mode 100644 apps/api/src/modules/mail/inbound/filter.ts create mode 100644 apps/api/test/modules/mail/inbound-filter.test.ts diff --git a/apps/api/src/modules/mail/inbound/filter.ts b/apps/api/src/modules/mail/inbound/filter.ts new file mode 100644 index 000000000..61001d9e7 --- /dev/null +++ b/apps/api/src/modules/mail/inbound/filter.ts @@ -0,0 +1,259 @@ +/** + * Deciding whether a captured message should fire a notification — pure, no I/O. + * + * Everything here runs on the HEADER BLOCK of a BCC copy sitting in the collector + * folder. Keeping it pure is deliberate: this is where the feature is dangerous (a loop + * that mails itself, a rule that leaks every message on the server, an alert per spam), + * and none of those failure modes should need a mail server to test. + * + * WHAT WE CAN AND CANNOT KNOW. `enable_original_recipient = no` in the shipped main.cf + * ("Avoid duplicate recipient messages"), so the BCC copy carries NO `X-Original-To` and + * its envelope recipient is the collector, not the mailbox the mail was for. The only + * evidence of the original recipient is the `To`/`Cc` header. That is authoritative for + * ordinary mail and WRONG for anything Bcc'd or alias-expanded — so a `mailbox`-scope + * rule can miss a message that genuinely arrived at its target. A `domain`-scope rule has + * no such gap, because the capture itself is domain-keyed. This is a property of Postfix's + * config, not something the control plane can fix; it is called out in the UI copy. + */ + +import type { MailInboundRule } from "@repo/db"; + +/** The subset of headers any decision here is allowed to depend on. */ +export interface ParsedHeaders { + /** `From:` — display form, e.g. `Alice `. */ + from?: string; + /** Lowercased bare address out of `From:`, e.g. `alice@example.com`. */ + fromAddress?: string; + /** `To:` and `Cc:` joined — the only evidence of the original recipient. */ + recipients: string[]; + subject?: string; + /** `Return-Path:` — the ENVELOPE sender. `<>` means this is a bounce. */ + returnPath?: string; + messageId?: string; + /** amavis writes these when SpamAssassin crosses the tag level. */ + spamFlagYes: boolean; + spamScore?: number; + autoSubmitted?: string; + precedence?: string; + listId?: string; +} + +/** Why a message was dropped before any rule was consulted. */ +export type DropReason = + | "bounce" + | "auto-submitted" + | "bulk-precedence" + | "mailing-list" + | "openship-sender" + | "spam-flagged"; + +export interface FilterDecision { + drop: boolean; + reason?: DropReason; +} + +/** + * Unfold and split an RFC 5322 header block. + * + * Continuation lines begin with space or tab and belong to the previous field, which + * matters immediately: long `Subject:` and `To:` values are folded in practice, and a + * naive line split would truncate a subject mid-word and lose half a recipient list. + * Field names are case-insensitive; the FIRST occurrence wins, because a second + * `From:` is either malformed or an attempt to confuse a reader downstream. + */ +export function parseHeaderBlock(raw: string): ParsedHeaders { + const fields = new Map(); + let currentName: string | null = null; + let currentValue = ""; + + const flush = () => { + if (currentName && !fields.has(currentName)) { + fields.set(currentName, currentValue.trim()); + } + currentName = null; + currentValue = ""; + }; + + // Normalize CRLF first so a Windows-authored fixture and a real message agree. + for (const line of raw.replace(/\r\n/g, "\n").split("\n")) { + // A blank line ends the header block; a body must never reach a rule decision. + if (line === "") break; + if (/^[ \t]/.test(line)) { + if (currentName) currentValue += " " + line.trim(); + continue; + } + const sep = line.indexOf(":"); + if (sep <= 0) continue; + flush(); + currentName = line.slice(0, sep).trim().toLowerCase(); + currentValue = line.slice(sep + 1); + } + flush(); + + const to = fields.get("to"); + const cc = fields.get("cc"); + const recipients = [to, cc] + .filter((v): v is string => Boolean(v)) + .flatMap((v) => v.split(",")) + .map((v) => bareAddress(v)) + .filter((v): v is string => Boolean(v)); + + const scoreRaw = fields.get("x-spam-score"); + const score = scoreRaw === undefined ? undefined : Number.parseFloat(scoreRaw); + + return { + from: fields.get("from"), + fromAddress: bareAddress(fields.get("from")), + recipients, + subject: fields.get("subject"), + returnPath: fields.get("return-path"), + messageId: fields.get("message-id"), + // amavis writes `X-Spam-Flag: YES`; anything else (absent, NO) is not a positive. + spamFlagYes: (fields.get("x-spam-flag") ?? "").trim().toUpperCase() === "YES", + spamScore: score !== undefined && Number.isFinite(score) ? score : undefined, + autoSubmitted: fields.get("auto-submitted"), + precedence: fields.get("precedence"), + listId: fields.get("list-id"), + }; +} + +/** `Alice ` / `a@b.com` / `` → `a@b.com`, lowercased. */ +export function bareAddress(value: string | undefined): string | undefined { + if (!value) return undefined; + const angled = value.match(/<([^>]*)>/); + const candidate = (angled ? angled[1] : value).trim().toLowerCase(); + // An empty `<>` is a real and meaningful value (a bounce), so it is preserved rather + // than being normalized away to undefined. + if (candidate === "") return ""; + return candidate.includes("@") ? candidate : undefined; +} + +/** + * The guards that run BEFORE any rule, in the order a mail incident actually happens. + * + * A notification about mail is itself mail. If any of these is missing, a single + * notification delivered to an address on the watched engine re-captures itself and the + * loop only stops when someone notices — so these are not tuning knobs. + * + * `openshipSenders` are the addresses this instance sends its own mail FROM (the platform + * mailbox, and whatever Settings→Email is configured with). They are passed in rather + * than derived so this stays pure. + */ +export function loopGuard( + h: ParsedHeaders, + opts: { openshipSenders?: readonly string[] } = {}, +): FilterDecision { + // A null envelope sender is a BOUNCE. Notifying on bounces is how a bounce storm + // becomes an alert storm, and the reply-to-a-bounce case is a classic mail loop. + if (h.returnPath !== undefined && bareAddress(h.returnPath) === "") { + return { drop: true, reason: "bounce" }; + } + // RFC 3834: anything not `no` is machine-generated, which includes our own alerts if a + // channel ever mails one back. + const auto = (h.autoSubmitted ?? "").trim().toLowerCase(); + if (auto && auto !== "no") return { drop: true, reason: "auto-submitted" }; + + const precedence = (h.precedence ?? "").trim().toLowerCase(); + if (precedence === "bulk" || precedence === "junk") { + return { drop: true, reason: "bulk-precedence" }; + } + // `List-Id` marks list traffic, which is the highest-volume way to flood a channel. + if (h.listId) return { drop: true, reason: "mailing-list" }; + + const senders = opts.openshipSenders ?? []; + if (h.fromAddress && senders.some((s) => s.toLowerCase() === h.fromAddress)) { + return { drop: true, reason: "openship-sender" }; + } + return { drop: false }; +} + +/** + * The spam gate, which exists because nothing upstream provides one. + * + * The shipped amavis policy sets `spam_lover='Y'` AND `bad_header_lover='Y'` on the + * catch-all `@.` policy with empty quarantine targets, so the global + * `$final_spam_destiny = D_DISCARD` never applies to any recipient: spam IS delivered, + * and therefore IS captured. Bad-header mail is delivered too and carries no + * `X-Spam-Flag` at all, which is why the score is checked independently of the flag. + * + * With no `maxSpamScore` set on the rule, a positive `X-Spam-Flag` alone is enough to + * drop — the conservative default, since the alternative is paging on spam. + */ +export function spamGate(h: ParsedHeaders, maxSpamScore: number | null): FilterDecision { + if (maxSpamScore === null || maxSpamScore === undefined) { + return h.spamFlagYes ? { drop: true, reason: "spam-flagged" } : { drop: false }; + } + if (h.spamScore !== undefined && h.spamScore > maxSpamScore) { + return { drop: true, reason: "spam-flagged" }; + } + return { drop: false }; +} + +/** + * Does this rule want this message? + * + * `capturedDomain` is the domain whose BCC row produced the copy — structural and + * trustworthy, unlike the recipient headers. + * + * FAIL CLOSED is the whole point of the target checks. There is no CHECK constraint + * tying `scope` to `target` (this schema has none anywhere), so a `mailbox` or `domain` + * rule with a null target is representable — and if it were treated as "no constraint" + * it would silently become "every message on the server", i.e. an operator who mistyped + * a rule quietly forwards a whole domain's mail metadata to Slack. It matches nothing. + */ +export function matchesRule( + rule: Pick< + MailInboundRule, + "scope" | "target" | "fromPattern" | "subjectPattern" | "enabled" | "pausedReason" + >, + h: ParsedHeaders, + capturedDomain: string, +): boolean { + if (!rule.enabled || rule.pausedReason) return false; + + const target = rule.target?.trim().toLowerCase() ?? ""; + + switch (rule.scope) { + case "all": + break; + case "domain": + if (!target) return false; + if (target !== capturedDomain.toLowerCase()) return false; + break; + case "mailbox": { + if (!target) return false; + // The To/Cc caveat in this file's header applies here: a Bcc'd or alias-expanded + // message cannot be attributed to a mailbox from headers alone. + if (!h.recipients.some((r) => r === target)) return false; + break; + } + default: + // An unknown scope is a rule this build does not understand. Matching nothing is + // the only safe reading — the alternative is matching everything. + return false; + } + + if (rule.fromPattern && !matchesPattern(rule.fromPattern, h.from ?? h.fromAddress)) { + return false; + } + if (rule.subjectPattern && !matchesPattern(rule.subjectPattern, h.subject)) { + return false; + } + return true; +} + +/** + * Operator-facing matching: case-insensitive substring, with `*` as a wildcard. + * + * Deliberately NOT a regular expression. These patterns come from a text box, they run + * once per delivered message, and an operator pasting `.*(a+)+$` would hand us a + * catastrophic-backtracking stall on the mail path. Every metacharacter is escaped and + * only `*` is given meaning. + */ +export function matchesPattern(pattern: string, value: string | undefined): boolean { + if (!value) return false; + const trimmed = pattern.trim(); + if (!trimmed) return true; + const escaped = trimmed.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/\\\*/g, ".*"); + return new RegExp(escaped, "i").test(value); +} diff --git a/apps/api/test/modules/mail/inbound-filter.test.ts b/apps/api/test/modules/mail/inbound-filter.test.ts new file mode 100644 index 000000000..1666de3d9 --- /dev/null +++ b/apps/api/test/modules/mail/inbound-filter.test.ts @@ -0,0 +1,209 @@ +import { describe, expect, it } from "vitest"; +import { + bareAddress, + loopGuard, + matchesPattern, + matchesRule, + parseHeaderBlock, + spamGate, +} from "../../../src/modules/mail/inbound/filter"; +import type { MailInboundRule } from "@repo/db"; + +/** + * The decisions that make inbound-mail notifications safe rather than an incident. + * + * Three failure modes are covered deliberately, because each is silent in production: + * - a loop (a notification about mail is itself mail), + * - a rule that quietly widens to every message on the server, + * - an alert per spam, because nothing upstream filters spam for us. + */ + +const HEADERS = [ + "Return-Path: ", + "From: Alice Example ", + "To: support@acme.com, ops@acme.com", + "Subject: Invoice 42 is overdue", + "Message-Id: ", + "", + "body must never be parsed", +].join("\n"); + +function rule(over: Partial = {}): MailInboundRule { + return { + scope: "domain", + target: "acme.com", + fromPattern: null, + subjectPattern: null, + enabled: true, + pausedReason: null, + ...over, + } as MailInboundRule; +} + +describe("parseHeaderBlock", () => { + it("parses the fields a decision is allowed to use, and stops at the body", () => { + const h = parseHeaderBlock(HEADERS); + expect(h.fromAddress).toBe("alice@example.com"); + expect(h.recipients).toEqual(["support@acme.com", "ops@acme.com"]); + expect(h.subject).toBe("Invoice 42 is overdue"); + expect(h.messageId).toBe(""); + // A blank line ends the block; body content must not become a "header". + expect(JSON.stringify(h)).not.toContain("body must never"); + }); + + it("unfolds continuation lines instead of truncating them", () => { + // Long To/Subject values ARE folded in real mail. A naive line split loses half the + // recipient list, which would make a mailbox-scope rule miss its own target. + const h = parseHeaderBlock( + ["Subject: a very long subject", "\tthat continues here", "To: one@acme.com,", " two@acme.com", ""].join("\n"), + ); + expect(h.subject).toBe("a very long subject that continues here"); + expect(h.recipients).toEqual(["one@acme.com", "two@acme.com"]); + }); + + it("is case-insensitive on names and keeps the FIRST occurrence", () => { + const h = parseHeaderBlock(["FROM: first@a.com", "From: second@b.com", ""].join("\n")); + expect(h.fromAddress).toBe("first@a.com"); + }); + + it("handles CRLF, so a real message and a fixture agree", () => { + const h = parseHeaderBlock("Subject: hi\r\nTo: a@b.com\r\n\r\nbody"); + expect(h.subject).toBe("hi"); + expect(h.recipients).toEqual(["a@b.com"]); + }); + + it("reads X-Spam-Flag and a float score, ignoring a malformed score", () => { + expect(parseHeaderBlock("X-Spam-Flag: YES\n").spamFlagYes).toBe(true); + expect(parseHeaderBlock("X-Spam-Flag: no\n").spamFlagYes).toBe(false); + expect(parseHeaderBlock("X-Spam-Score: 7.4\n").spamScore).toBe(7.4); + expect(parseHeaderBlock("X-Spam-Score: not-a-number\n").spamScore).toBeUndefined(); + }); +}); + +describe("bareAddress", () => { + it("preserves the empty angle pair, because <> IS the bounce signal", () => { + expect(bareAddress("<>")).toBe(""); + expect(bareAddress("Alice ")).toBe("a@b.com"); + expect(bareAddress("A@B.COM")).toBe("a@b.com"); + expect(bareAddress("not-an-address")).toBeUndefined(); + expect(bareAddress(undefined)).toBeUndefined(); + }); +}); + +describe("loopGuard — the four guards", () => { + it("drops a bounce (null envelope sender)", () => { + const h = parseHeaderBlock("Return-Path: <>\nFrom: mailer@x.com\n"); + expect(loopGuard(h)).toEqual({ drop: true, reason: "bounce" }); + }); + + it("drops anything Auto-Submitted other than 'no'", () => { + expect(loopGuard(parseHeaderBlock("Auto-Submitted: auto-generated\n")).drop).toBe(true); + expect(loopGuard(parseHeaderBlock("Auto-Submitted: auto-replied\n")).drop).toBe(true); + expect(loopGuard(parseHeaderBlock("Auto-Submitted: no\n")).drop).toBe(false); + }); + + it("drops bulk/junk precedence and list traffic", () => { + expect(loopGuard(parseHeaderBlock("Precedence: bulk\n")).reason).toBe("bulk-precedence"); + expect(loopGuard(parseHeaderBlock("Precedence: junk\n")).reason).toBe("bulk-precedence"); + expect(loopGuard(parseHeaderBlock("List-Id: \n")).reason).toBe("mailing-list"); + }); + + it("drops our OWN outbound sender — the direct self-feeding loop", () => { + // A notification delivered to an address inside a watched domain would otherwise be + // captured and emit again, forever. + const h = parseHeaderBlock("From: Openship \n"); + expect(loopGuard(h, { openshipSenders: ["NoReply@acme.com"] })).toEqual({ + drop: true, + reason: "openship-sender", + }); + }); + + it("lets ordinary human mail through", () => { + expect(loopGuard(parseHeaderBlock(HEADERS))).toEqual({ drop: false }); + }); +}); + +describe("spamGate", () => { + it("drops flagged spam when no threshold is set (the conservative default)", () => { + expect(spamGate(parseHeaderBlock("X-Spam-Flag: YES\n"), null).reason).toBe("spam-flagged"); + expect(spamGate(parseHeaderBlock("Subject: hi\n"), null).drop).toBe(false); + }); + + it("honours an explicit threshold on the score", () => { + const spammy = parseHeaderBlock("X-Spam-Flag: YES\nX-Spam-Score: 9.1\n"); + expect(spamGate(spammy, 10).drop).toBe(false); // operator asked for a loose gate + expect(spamGate(spammy, 5).drop).toBe(true); + }); + + it("catches bad-header mail, which is delivered with NO X-Spam-Flag at all", () => { + // bad_header_lover='Y' in the shipped policy, and bad-header mail is not flagged — + // so the score has to be checked independently of the flag. + const badHeader = parseHeaderBlock("X-Spam-Score: 8.0\n"); + expect(badHeader.spamFlagYes).toBe(false); + expect(spamGate(badHeader, 6.9).drop).toBe(true); + }); +}); + +describe("matchesRule — fail closed", () => { + const h = parseHeaderBlock(HEADERS); + + it("matches a domain rule against the CAPTURED domain, not a header", () => { + expect(matchesRule(rule({ scope: "domain", target: "acme.com" }), h, "acme.com")).toBe(true); + expect(matchesRule(rule({ scope: "domain", target: "acme.com" }), h, "other.com")).toBe(false); + }); + + it("matches a mailbox rule on To/Cc", () => { + expect(matchesRule(rule({ scope: "mailbox", target: "support@acme.com" }), h, "acme.com")).toBe(true); + expect(matchesRule(rule({ scope: "mailbox", target: "nobody@acme.com" }), h, "acme.com")).toBe(false); + }); + + it("matches EVERYTHING captured for scope=all", () => { + expect(matchesRule(rule({ scope: "all", target: null }), h, "anything.com")).toBe(true); + }); + + // The reason there is no CHECK constraint and this logic exists instead. + it("matches NOTHING when a mailbox/domain rule has no target", () => { + expect(matchesRule(rule({ scope: "mailbox", target: null }), h, "acme.com")).toBe(false); + expect(matchesRule(rule({ scope: "domain", target: null }), h, "acme.com")).toBe(false); + expect(matchesRule(rule({ scope: "domain", target: " " }), h, "acme.com")).toBe(false); + }); + + it("matches nothing for a scope this build does not understand", () => { + expect(matchesRule(rule({ scope: "everything", target: null }), h, "acme.com")).toBe(false); + }); + + it("respects enabled and pausedReason", () => { + expect(matchesRule(rule({ enabled: false }), h, "acme.com")).toBe(false); + expect(matchesRule(rule({ pausedReason: "rate limit" }), h, "acme.com")).toBe(false); + }); + + it("applies from and subject filters on top of scope", () => { + expect(matchesRule(rule({ fromPattern: "alice@" }), h, "acme.com")).toBe(true); + expect(matchesRule(rule({ fromPattern: "bob@" }), h, "acme.com")).toBe(false); + expect(matchesRule(rule({ subjectPattern: "overdue" }), h, "acme.com")).toBe(true); + expect(matchesRule(rule({ subjectPattern: "refund" }), h, "acme.com")).toBe(false); + }); +}); + +describe("matchesPattern", () => { + it("is case-insensitive substring with * as the only wildcard", () => { + expect(matchesPattern("INVOICE", "Invoice 42")).toBe(true); + expect(matchesPattern("inv*42", "Invoice 42")).toBe(true); + expect(matchesPattern("*@acme.com", "bob@acme.com")).toBe(true); + }); + + it("treats regex metacharacters literally, so an operator cannot inject one", () => { + // A pasted `.*` must not match everything, and a pasted `(a+)+$` must not stall the + // mail path with catastrophic backtracking. + expect(matchesPattern(".*", "anything")).toBe(false); + expect(matchesPattern("a.c", "abc")).toBe(false); + expect(matchesPattern("a.c", "a.c")).toBe(true); + expect(matchesPattern("price?", "price?")).toBe(true); + expect(() => matchesPattern("(a+)+$", "aaaaaaaaaaaaaaaaaaaaaaaaaaa!")).not.toThrow(); + }); + + it("an empty pattern is 'no filter', and a missing value never matches", () => { + expect(matchesPattern(" ", "anything")).toBe(true); + expect(matchesPattern("x", undefined)).toBe(false); + }); +}); From 550fe22f093bac17059e5feb9bcce77db3750c8f Mon Sep 17 00:00:00 2001 From: Hydra Date: Thu, 13 Aug 2026 17:38:58 +0300 Subject: [PATCH 6/8] =?UTF-8?q?fix(apps):=20make=20the=20catalog=20install?= =?UTF-8?q?able=20=E2=80=94=20every=20app=20gets=20a=20UI=20and=20a=20conn?= =?UTF-8?q?ection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Boot-tested the whole catalog against real containers. Several apps had never run at all, and the failures were structural rather than cosmetic. Dead on arrival, now fixed: - MinIO crash-looped on every boot since its `command` was added. Openship wraps a template `command` as ["sh","-c",cmd], and MinIO's entrypoint prepends `minio` unless argv[0] already is it, so the container ran `minio sh -c "server /data"` and exited with "'sh' is not a minio sub-command". No command string can fix that, so add `commandArgv` to the template schema: exact argv, no shell wrap. The DB column and runtime already supported it (#332); only the template could not ask. - PostHog could never have served a request. `./bin/plugin-server` was deleted upstream on 2025-12-30 and moved to posthog/posthog-node (exec: not found); `./bin/docker-server` calls `bin/migrate-check` under `set -e`, which exits 1 on an empty database, so web never bound :8000; and ClickHouse mounted no config, so migrate_clickhouse could not create a single ReplicatedMergeTree table. Rebuilt as the full 21-service topology with a Caddy path router, since Django no longer serves /e, /capture or /batch — ingestion moved to the Rust capture services, and the old 9-service shape could not ingest an event. - Directus created no admin at all: `create-admin.js` bare-returns unless ADMIN_EMAIL and ADMIN_PASSWORD are set, and Directus has no first-visit signup, so nothing could ever sign in. The description claimed otherwise. - code-server crash-looped with EACCES: none of its three volume paths exist in the image while it runs as uid 1000, so each named volume mounted root:root. One volume at /home/coder inherits ownership and persists everything. - Redis ran `valkey-server /etc/valkey/valkey.conf` from files[], which is skipped on the cloud runtime; and if the bind source was not a file Docker mounted an empty directory, so valkey started WITH NO PASSWORD and the old healthcheck still reported healthy. Now VALKEY_EXTRA_FLAGS, and the healthcheck asserts auth is actually required. The `sh -c` wrap was doing quiet damage beyond MinIO: redis/valkey dropped privileges only when argv[0] is literally `valkey-server`, so they ran as root; and on Debian images PID 1 stayed as dash, which never forwards SIGTERM, so PostHog's Celery drain trap was dead code. Those services use commandArgv now. Ports: freshrss, it-tools and vaultwarden claimed :80 (the edge owns it) and uptime-kuma claimed :3001 (Openship's own dashboard owns it) — all four failed to start with real "port is already allocated" errors. Host ports are now unique catalog-wide and avoid 80/443/3001. They are kept rather than removed because {{publicUrl:svc}} resolves from the published host port on a port-only install; without one, PUBLIC_URL/DOMAIN/ROOT_URL are omitted entirely. Security defaults that were wrong: Gitea shipped an unauthenticated install wizard anyone could claim, Vaultwarden allowed open registration on a password manager, and Stirling-PDF silently created the publicly-known admin/stirling. New: ClickHouse with the CH-UI console (verified server-side, so the database never needs to be public), and Neon rebuilt on the neond control plane — Neon's own console is proprietary and the upstream repo ships no web UI, so this is the only way to get self-hosted Neon with a dashboard. Also fixes multi-route collapse: only proxyRoutes[0] got a pinned host port while resolveTargetUrl handed that one port to every route, so MinIO's `s3` subdomain served the console and Convex's `http` subdomain served the 3210 API. Now one host port per routed port, with regression tests. Adds stopGracePeriod to the schema (the engine always honoured it) so an app whose clean shutdown does real work is not SIGKILLed at Docker's 10s default. 27 of 29 apps now install with a UI link and a connection path. PostHog and Neon remain verified:false — neither has been booted end to end. --- apps/api/src/lib/loopback-publish.ts | 41 + .../src/modules/apps/app-install.service.ts | 4 + .../deployments/compose/deploy.service.ts | 108 +- apps/api/test/lib/loopback-publish.test.ts | 79 +- .../migration/ServerMigrationWizard.tsx | 4 +- .../src/components/shared/ServerSelector.tsx | 102 +- packages/core/src/app-templates.ts | 13 +- packages/core/src/apps/catalog.json | 2206 +++++++++++++---- .../core/src/apps/catalog/clickhouse.json | 270 ++ .../core/src/apps/catalog/code-server.json | 76 +- packages/core/src/apps/catalog/directus.json | 97 +- .../core/src/apps/catalog/excalidraw.json | 30 +- packages/core/src/apps/catalog/freshrss.json | 79 +- packages/core/src/apps/catalog/ghost.json | 42 +- packages/core/src/apps/catalog/gitea.json | 92 +- packages/core/src/apps/catalog/it-tools.json | 33 +- packages/core/src/apps/catalog/kafka.json | 8 +- .../core/src/apps/catalog/meilisearch.json | 13 +- packages/core/src/apps/catalog/metabase.json | 51 +- packages/core/src/apps/catalog/minio.json | 55 +- packages/core/src/apps/catalog/mongodb.json | 20 +- packages/core/src/apps/catalog/n8n.json | 40 +- packages/core/src/apps/catalog/neon.json | 283 +-- packages/core/src/apps/catalog/nocodb.json | 64 +- packages/core/src/apps/catalog/posthog.json | 670 ++++- packages/core/src/apps/catalog/qdrant.json | 8 +- packages/core/src/apps/catalog/redis.json | 119 +- .../core/src/apps/catalog/stirling-pdf.json | 65 +- packages/core/src/apps/catalog/umami.json | 8 +- .../core/src/apps/catalog/uptime-kuma.json | 35 +- .../core/src/apps/catalog/vaultwarden.json | 65 +- packages/core/src/apps/install-copy.test.ts | 52 +- packages/core/src/apps/schema.ts | 23 + 33 files changed, 3860 insertions(+), 995 deletions(-) create mode 100644 packages/core/src/apps/catalog/clickhouse.json diff --git a/apps/api/src/lib/loopback-publish.ts b/apps/api/src/lib/loopback-publish.ts index 9dbe3f912..649714a85 100644 --- a/apps/api/src/lib/loopback-publish.ts +++ b/apps/api/src/lib/loopback-publish.ts @@ -34,3 +34,44 @@ export function withLoopbackPublish( const kept = portSpecs.filter((spec) => specContainerPort(spec) !== containerPort); return [...kept, `127.0.0.1:${hostPort}:${containerPort}`]; } + +/** + * Republish EVERY routed container port on its own pinned loopback host port. + * + * A service can own several routes (minio's console + `s3` API), and each needs a + * DISTINCT host port or the edge cannot tell them apart. + */ +export function withLoopbackPublishAll( + portSpecs: readonly string[], + /** routed container port → the loopback host port pinned for it. */ + pinned: ReadonlyMap, +): string[] { + let out = [...portSpecs]; + for (const [containerPort, hostPort] of pinned) { + out = withLoopbackPublish(out, containerPort, hostPort); + } + return out; +} + +/** + * The host port a route's upstream should dial for `port`. + * + * The pinned map is authoritative. `resultHostPort` — a single scalar read back + * off the daemon — is only meaningful for the PRIMARY routed port: applying it to + * a secondary port is what made every extra subdomain proxy to the first route's + * port (minio's `s3` host served the console). Undefined means "no host port", + * which sends the caller to container-IP addressing instead of a wrong guess. + */ +export function upstreamHostPortFor(args: { + port: number; + pinned: ReadonlyMap; + primaryPort?: number; + resultHostPort?: number | null; + sameService: boolean; +}): number | undefined { + const { port, pinned, primaryPort, resultHostPort, sameService } = args; + return ( + pinned.get(port) ?? + (sameService && port === primaryPort && resultHostPort ? resultHostPort : undefined) + ); +} diff --git a/apps/api/src/modules/apps/app-install.service.ts b/apps/api/src/modules/apps/app-install.service.ts index d22896a9b..2ac7f1118 100644 --- a/apps/api/src/modules/apps/app-install.service.ts +++ b/apps/api/src/modules/apps/app-install.service.ts @@ -608,6 +608,9 @@ export async function installApp( environment: plainEnv, volumes: svc.volumes ? [...svc.volumes] : [], command: svc.command, + // Structured argv bypasses the `sh -c` wrap resolveComposeCmd applies to a + // bare `command`, which some images' entrypoints cannot survive. + commandArgv: svc.commandArgv ? [...svc.commandArgv] : undefined, restart: svc.restart, advanced: { ...(svc.healthcheck ? { healthcheck: svc.healthcheck } : {}), @@ -615,6 +618,7 @@ export async function installApp( ? { files: filesByService.get(svc.name) } : {}), ...(svc.build ? { build: resolveBuild(svc.build) } : {}), + ...(svc.stopGracePeriod ? { stopGracePeriod: svc.stopGracePeriod } : {}), }, // Routing is exactly what the operator chose — never the template's // `exposed` flag turned into a hostname. diff --git a/apps/api/src/modules/deployments/compose/deploy.service.ts b/apps/api/src/modules/deployments/compose/deploy.service.ts index d378b473a..cfb857867 100644 --- a/apps/api/src/modules/deployments/compose/deploy.service.ts +++ b/apps/api/src/modules/deployments/compose/deploy.service.ts @@ -94,7 +94,10 @@ import { compileProjectRoutingFields } from "../../../lib/project-routing-fields import { buildCompositeRegistration, buildDomainFanoutRegistrations } from "./composite-route"; import { newerThanRestoredRelease, serviceKind } from "./project-services"; import { buildUpstreamUrl, resolveRouteStrategy } from "../../../lib/upstream-url"; -import { withLoopbackPublish } from "../../../lib/loopback-publish"; +import { + withLoopbackPublishAll, + upstreamHostPortFor, +} from "../../../lib/loopback-publish"; export interface ComposeDeployResult { /** `reconciling` when at least one service's outcome is UNKNOWN because the @@ -1721,50 +1724,77 @@ export async function deployComposeServices( ); } - // loopback-port routing (compose parity, mirrors single-app): republish the - // PRIMARY routed container port on `127.0.0.1:` so the edge - // reaches it on loopback and it isn't network-exposed. We OWN the pinned - // port (reuse the carried one, else allocate avoiding this deploy's picks), - // so the route resolves to it deterministically — no reading it back from - // the daemon's ambiguous first-binding. Port-only bindings the user declared - // for direct access are preserved. Cloud handles exposure itself; bare/no- - // executor can't publish → skip (route falls back to container-IP/loopback). + // loopback-port routing (compose parity, mirrors single-app): republish EVERY + // routed container port on its OWN `127.0.0.1:` so the edge + // reaches each on loopback and none is network-exposed. We OWN the pinned + // ports (reuse the carried one for the primary, else allocate avoiding this + // deploy's picks), so each route resolves deterministically — no reading it + // back from the daemon's ambiguous first-binding. Port-only bindings the user + // declared for direct access are preserved. Cloud handles exposure itself; + // bare/no-executor can't publish → skip (route falls back to container-IP). + // + // ONE HOST PORT PER ROUTED PORT, not one per service: a service can carry + // several routes (minio's console + `s3` API, convex's API + `http`), and + // pinning only `proxyRoutes[0]` while `resolveTargetUrl` returned that single + // port for every route made each extra subdomain silently proxy to the FIRST + // route's port. minio's s3 host served the console; convex's http host served + // the 3210 API. Only the primary port is persisted (`service_deployment` + // holds one), which is why the extras are re-pinned and re-registered on + // every deploy rather than carried. const composeRouteStrategy = resolveRouteStrategy(project.routeStrategy); - const routedContainerPort = proxyRoutes[0]?.targetPort; + const routedContainerPorts = [ + ...new Set( + proxyRoutes + .map((r) => r.targetPort) + .filter((p): p is number => typeof p === "number" && p > 0), + ), + ]; + const primaryRoutedPort = routedContainerPorts[0]; + /** routed container port → the loopback host port WE pinned for it. */ + const pinnedHostPortByContainerPort = new Map(); let servicePinnedHostPort: number | undefined; if ( composeRouteStrategy === "loopback-port" && runtime.name !== "cloud" && - routedContainerPort !== undefined && + primaryRoutedPort !== undefined && // A container with no endpoint of its own publishes nothing — allocating a // host port would burn it and pin a route to an upstream that never binds. !hasNoRoutableAddress && opts?.executor ) { - const carried = previousByServiceId.get(svc.id)?.hostPort; - if (carried) { - servicePinnedHostPort = carried; - } else { - const allocation = await allocateHostPort(opts.executor, { avoid: usedHostPorts }); - servicePinnedHostPort = allocation.port; - // "Couldn't read occupancy" is not "nothing is listening" — without this the - // bind failure that follows blames Docker for an unreachable host (#490). - if (!allocation.scanned) { - logger.log( - `Couldn't read live port occupancy on the target, so ${allocation.port} for ` + - `${svc.name} avoids only ports this deploy already took. If publishing it fails ` + - `as "already allocated", check that Openship can reach this host ` + - `(Servers → this box).\n`, - "warn", - ); + for (const containerPort of routedContainerPorts) { + // Only the primary reuses the carried port: it is the one persisted, so + // it is the only one whose previous value is knowable. + const carried = + containerPort === primaryRoutedPort + ? previousByServiceId.get(svc.id)?.hostPort + : undefined; + let hostPort: number; + if (carried) { + hostPort = carried; + } else { + const allocation = await allocateHostPort(opts.executor, { avoid: usedHostPorts }); + hostPort = allocation.port; + // "Couldn't read occupancy" is not "nothing is listening" — without this the + // bind failure that follows blames Docker for an unreachable host (#490). + if (!allocation.scanned) { + logger.log( + `Couldn't read live port occupancy on the target, so ${allocation.port} for ` + + `${svc.name} avoids only ports this deploy already took. If publishing it fails ` + + `as "already allocated", check that Openship can reach this host ` + + `(Servers → this box).\n`, + "warn", + ); + } } + usedHostPorts.add(hostPort); + pinnedHostPortByContainerPort.set(containerPort, hostPort); } - usedHostPorts.add(servicePinnedHostPort); - serviceRuntimeConfig.ports = withLoopbackPublish( + serviceRuntimeConfig.ports = withLoopbackPublishAll( serviceRuntimeConfig.ports, - routedContainerPort, - servicePinnedHostPort, + pinnedHostPortByContainerPort, ); + servicePinnedHostPort = pinnedHostPortByContainerPort.get(primaryRoutedPort); } let deployedContainerId: string | undefined; @@ -1795,10 +1825,18 @@ export async function deployComposeServices( ? async (containerId, port) => { const strategy = resolveRouteStrategy(project.routeStrategy); const sameSvc = serviceResult?.containerId === containerId; - // Prefer the port WE pinned+published this deploy (deterministic); - // fall back to the port reported by the deploy result otherwise. - const hostPort = - servicePinnedHostPort ?? (sameSvc ? serviceResult?.hostPort : undefined); + // Prefer the port WE pinned+published for THIS container port + // (deterministic); fall back to the port the deploy result + // reported. That fallback is a single scalar read off the daemon, + // so it is only meaningful for the primary route — applying it to + // a secondary port is the collapse this map exists to prevent. + const hostPort = upstreamHostPortFor({ + port, + pinned: pinnedHostPortByContainerPort, + primaryPort: primaryRoutedPort, + resultHostPort: serviceResult?.hostPort, + sameService: sameSvc, + }); // loopback-port → the service's published host port; else the // container IP (cached from the deploy result when we can). if (strategy === "loopback-port" && hostPort) { diff --git a/apps/api/test/lib/loopback-publish.test.ts b/apps/api/test/lib/loopback-publish.test.ts index 060455907..ca36fb547 100644 --- a/apps/api/test/lib/loopback-publish.test.ts +++ b/apps/api/test/lib/loopback-publish.test.ts @@ -1,5 +1,82 @@ import { describe, it, expect } from "vitest"; -import { specContainerPort, withLoopbackPublish } from "../../src/lib/loopback-publish"; +import { + specContainerPort, + withLoopbackPublish, + withLoopbackPublishAll, + upstreamHostPortFor, +} from "../../src/lib/loopback-publish"; + +describe("a service with SEVERAL routes gets one host port per routed port", () => { + // Regression: minio routes 9001 (console) and 9000 (s3 API). Pinning only the + // first route's port and then reusing that single host port for every route made + // the `s3` subdomain serve the console. + const pinned = new Map([ + [9001, 20500], + [9000, 20501], + ]); + + it("publishes a DISTINCT loopback binding per routed container port", () => { + expect(withLoopbackPublishAll([], pinned)).toEqual([ + "127.0.0.1:20500:9001", + "127.0.0.1:20501:9000", + ]); + }); + + it("replaces a template's own binding for each routed port and keeps the rest", () => { + const out = withLoopbackPublishAll(["9000:9000", "9001:9001", "1234:1234"], pinned); + expect(out).toContain("127.0.0.1:20500:9001"); + expect(out).toContain("127.0.0.1:20501:9000"); + expect(out).toContain("1234:1234"); + expect(out.filter((s) => s.endsWith(":9000"))).toHaveLength(1); + expect(out.filter((s) => s.endsWith(":9001"))).toHaveLength(1); + }); + + it("resolves each route to ITS OWN host port, never the primary's", () => { + expect(upstreamHostPortFor({ port: 9001, pinned, primaryPort: 9001, sameService: true })).toBe( + 20500, + ); + expect(upstreamHostPortFor({ port: 9000, pinned, primaryPort: 9001, sameService: true })).toBe( + 20501, + ); + }); + + it("never lends the daemon's single reported port to a SECONDARY route", () => { + const only = new Map(); + // The primary may fall back to what the deploy reported... + expect( + upstreamHostPortFor({ + port: 9001, + pinned: only, + primaryPort: 9001, + resultHostPort: 33333, + sameService: true, + }), + ).toBe(33333); + // ...but a secondary port must resolve to nothing, so the caller uses + // container-IP addressing rather than proxying to the wrong service. + expect( + upstreamHostPortFor({ + port: 9000, + pinned: only, + primaryPort: 9001, + resultHostPort: 33333, + sameService: true, + }), + ).toBeUndefined(); + }); + + it("ignores a reported port from a DIFFERENT container", () => { + expect( + upstreamHostPortFor({ + port: 9001, + pinned: new Map(), + primaryPort: 9001, + resultHostPort: 33333, + sameService: false, + }), + ).toBeUndefined(); + }); +}); describe("specContainerPort", () => { it("parses every docker port-spec form", () => { diff --git a/apps/dashboard/src/components/migration/ServerMigrationWizard.tsx b/apps/dashboard/src/components/migration/ServerMigrationWizard.tsx index 20ace65ed..9c0757b42 100644 --- a/apps/dashboard/src/components/migration/ServerMigrationWizard.tsx +++ b/apps/dashboard/src/components/migration/ServerMigrationWizard.tsx @@ -2200,7 +2200,9 @@ export function ServerMigrationWizard({ {m.wizard.targetLabel} - setTargetId(s?.id ?? null)} compact /> + {/* dropUp: this card is `overflow-hidden` and the picker sits at its + bottom, so a down-opening menu is hard-clipped. */} + setTargetId(s?.id ?? null)} compact dropUp />