diff --git a/app/src/app/api/connections/[id]/test/route.ts b/app/src/app/api/connections/[id]/test/route.ts index 4a56938b..8ab9b9d8 100644 --- a/app/src/app/api/connections/[id]/test/route.ts +++ b/app/src/app/api/connections/[id]/test/route.ts @@ -11,6 +11,7 @@ import { connectionCheckFalseResult, connectionTestErrorResult, } from "@/lib/connector/connection-test-result"; +import { isContainerised } from "@/lib/connector/is-containerised"; export async function POST( _request: Request, @@ -60,7 +61,12 @@ export async function POST( success ? { success: true } : connectionCheckFalseResult(), ); } catch (testError) { - return apiSuccess(connectionTestErrorResult(testError)); + return apiSuccess( + connectionTestErrorResult(testError, { + uri: credentials.uri, + containerised: isContainerised(), + }), + ); } } catch (error) { return handleRouteError(error, "Connection test failed"); diff --git a/app/src/app/api/connections/test-inline/route.ts b/app/src/app/api/connections/test-inline/route.ts index 351b7258..478e546b 100644 --- a/app/src/app/api/connections/test-inline/route.ts +++ b/app/src/app/api/connections/test-inline/route.ts @@ -9,6 +9,7 @@ import { connectionCheckFalseResult, connectionTestErrorResult, } from "@/lib/connector/connection-test-result"; +import { isContainerised } from "@/lib/connector/is-containerised"; export async function POST(request: Request) { try { @@ -43,7 +44,12 @@ export async function POST(request: Request) { success ? { success: true } : connectionCheckFalseResult(), ); } catch (testError) { - return apiSuccess(connectionTestErrorResult(testError)); + return apiSuccess( + connectionTestErrorResult(testError, { + uri: config.uri, + containerised: isContainerised(), + }), + ); } } catch (error) { return handleRouteError(error, "Connection test failed"); diff --git a/app/src/lib/__tests__/connector/container-host.test.ts b/app/src/lib/__tests__/connector/container-host.test.ts new file mode 100644 index 00000000..f7a0ae11 --- /dev/null +++ b/app/src/lib/__tests__/connector/container-host.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockIsContainerised = vi.fn(); +vi.mock("@/lib/connector/is-containerised", () => ({ + isContainerised: () => mockIsContainerised(), +})); + +import { resolveContainerHost } from "@/lib/connector/container-host"; + +beforeEach(() => vi.clearAllMocks()); + +describe("resolveContainerHost (#1346)", () => { + describe("inside a container", () => { + beforeEach(() => mockIsContainerised.mockReturnValue(true)); + + it.each([ + ["neo4j://localhost:7688", "neo4j://host.docker.internal:7688"], + ["bolt://127.0.0.1:7687", "bolt://host.docker.internal:7687"], + ["neo4j://LOCALHOST:7687", "neo4j://host.docker.internal:7687"], + ])("rewrites %s", (input, expected) => { + expect(resolveContainerHost(input)).toBe(expected); + }); + + it("keeps credentials, port, path and query intact", () => { + // A rewrite that drops the password or the database name trades one + // failure for a more confusing one. + expect( + resolveContainerHost( + "postgresql://u:p%40ss@localhost:5432/app?sslmode=require", + ), + ).toBe( + "postgresql://u:p%40ss@host.docker.internal:5432/app?sslmode=require", + ); + }); + + it.each([ + ["a remote host", "neo4j://db.example.com:7687"], + ["a compose service name", "neo4j://neo4j:7687"], + ["a LAN address", "postgresql://192.168.1.50:5432/app"], + [ + "a host merely containing 'localhost'", + "neo4j://my-localhost.example.com:7687", + ], + ])("leaves %s alone", (_l, uri) => { + expect(resolveContainerHost(uri)).toBe(uri); + }); + + it.each([ + ["an unparseable uri", "not a uri"], + ["an empty string", ""], + ])("returns %s unchanged rather than throwing", (_l, uri) => { + expect(() => resolveContainerHost(uri)).not.toThrow(); + expect(resolveContainerHost(uri)).toBe(uri); + }); + }); + + it("leaves loopback alone when NOT containerised", () => { + // Local mode runs on the host, where localhost is exactly right. Rewriting + // it there would break a working setup. + mockIsContainerised.mockReturnValue(false); + expect(resolveContainerHost("neo4j://localhost:7688")).toBe( + "neo4j://localhost:7688", + ); + }); +}); diff --git a/app/src/lib/connector/__tests__/connection-error-classifier.test.ts b/app/src/lib/connector/__tests__/connection-error-classifier.test.ts index 09eba135..769a6fff 100644 --- a/app/src/lib/connector/__tests__/connection-error-classifier.test.ts +++ b/app/src/lib/connector/__tests__/connection-error-classifier.test.ts @@ -117,3 +117,105 @@ describe("hintForConnectionErrorCode", () => { } }); }); + +// The most common thing a user does after `neoboard demo` is connect their own +// database. On a Docker install that database is on the HOST, so they type +// neo4j://localhost:7688 — and localhost inside the app container is the +// container. The driver says "Could not perform discovery. No routing servers +// available", which classified as `network`, whose hint told them to verify the +// host, the port, that the database is running, and their firewall. All four +// are already correct. There was no thread to pull (#1346). +describe("loopback from inside a container (#1346)", () => { + const DISCOVERY_ERROR = + "Could not perform discovery. No routing servers available."; + + it.each([ + ["localhost", "neo4j://localhost:7688"], + ["127.0.0.1", "postgresql://127.0.0.1:5432/app"], + ["::1", "neo4j://[::1]:7687"], + ["with credentials in the URI", "postgresql://u:p@localhost:5432/app"], + ["uppercase host", "neo4j://LOCALHOST:7687"], + ])("codes a network failure to %s as container_loopback", (_l, uri) => { + expect( + classifyConnectionError(DISCOVERY_ERROR, { uri, containerised: true }), + ).toBe("container_loopback"); + }); + + it("stays `network` when the app is NOT containerised", () => { + // The regression that matters. In local mode the app runs on the host, + // where localhost is exactly right — telling that user to use a Docker + // hostname would send them somewhere that does not exist. + expect( + classifyConnectionError(DISCOVERY_ERROR, { + uri: "neo4j://localhost:7688", + containerised: false, + }), + ).toBe("network"); + }); + + it.each([ + ["a remote host", "neo4j://db.example.com:7687"], + ["a compose service name", "neo4j://neo4j:7687"], + ["a LAN address", "postgresql://192.168.1.50:5432/app"], + ])("stays `network` for %s", (_l, uri) => { + expect( + classifyConnectionError(DISCOVERY_ERROR, { uri, containerised: true }), + ).toBe("network"); + }); + + it("does not outrank auth or bad_uri", () => { + // Priority order is unchanged: a loopback auth failure is still an auth + // failure, and the Docker hint would be a misdiagnosis. + expect( + classifyConnectionError("Authentication failure", { + uri: "neo4j://localhost:7688", + containerised: true, + }), + ).toBe("auth_failed"); + expect( + classifyConnectionError("Invalid URI scheme", { + uri: "wat://localhost:7688", + containerised: true, + }), + ).toBe("bad_uri"); + }); + + it.each([ + ["a malformed URI", "not a uri at all"], + ["an empty URI", ""], + ])("degrades to `network` for %s rather than throwing", (_l, uri) => { + // This runs on an error path. A classifier that throws replaces a bad + // message with a 500. + expect(() => + classifyConnectionError(DISCOVERY_ERROR, { uri, containerised: true }), + ).not.toThrow(); + expect( + classifyConnectionError(DISCOVERY_ERROR, { uri, containerised: true }), + ).toBe("network"); + }); + + it("is unchanged when no context is passed at all", () => { + expect(classifyConnectionError(DISCOVERY_ERROR)).toBe("network"); + }); + + it("names Docker, the CLI flag, and host.docker.internal in the hint", () => { + const hint = hintForConnectionErrorCode("container_loopback"); + expect(hint).toMatch(/host\.docker\.internal/); + expect(hint).toMatch(/container/i); + // The hostname only resolves on Linux when the overlay is applied, so the + // hint has to say how to apply it. + expect(hint).toMatch(/--expose-host/); + }); + + it("says WHOSE localhost, because the URI is resolved server-side", () => { + // The connection is opened by the NeoBoard server, not the browser. On a + // deployed instance, a user typing `localhost` means the SERVER's + // localhost — and host.docker.internal is the server's host too, not + // theirs. A hint saying "not your machine" reads as though their own + // laptop were reachable. It is not, and the copy has to say so. + const hint = hintForConnectionErrorCode("container_loopback"); + expect(hint).toMatch(/server/i); + expect(hint).toMatch(/your own computer/i); + expect(hint).toMatch(/not reachable|cannot see your machine/i); + }); +}); diff --git a/app/src/lib/connector/__tests__/connection-test-result.test.ts b/app/src/lib/connector/__tests__/connection-test-result.test.ts index 5e6f7a9a..08e07c48 100644 --- a/app/src/lib/connector/__tests__/connection-test-result.test.ts +++ b/app/src/lib/connector/__tests__/connection-test-result.test.ts @@ -32,4 +32,35 @@ describe("connection-test-result (#1043)", () => { expect(r.success).toBe(false); expect(r.code).toBe("unknown"); }); + + // The URI has to reach the classifier for it to spot a Docker networking + // miss — the route already has it, and passing it is the whole wiring (#1346). + it("passes the URI and container flag through to the classifier", () => { + expect( + connectionTestErrorResult( + new Error("Could not perform discovery. No routing servers available."), + { uri: "neo4j://localhost:7688", containerised: true }, + ).code, + ).toBe("container_loopback"); + }); + + it("still classifies as network when no context is given", () => { + // Both call sites must keep working unchanged if the context is absent. + expect( + connectionTestErrorResult( + new Error("Could not perform discovery. No routing servers available."), + ).code, + ).toBe("network"); + }); + + it("never echoes the URI into the user-facing error", () => { + // A URI can carry a password. The classifier reads it; the result must not + // carry it back out. + const r = connectionTestErrorResult(new Error("ECONNREFUSED"), { + uri: "postgresql://admin:hunter2@localhost:5432/app", + containerised: true, + }); + expect(JSON.stringify(r)).not.toContain("hunter2"); + expect(JSON.stringify(r)).not.toContain("localhost:5432"); + }); }); diff --git a/app/src/lib/connector/__tests__/is-containerised.test.ts b/app/src/lib/connector/__tests__/is-containerised.test.ts new file mode 100644 index 00000000..550889a6 --- /dev/null +++ b/app/src/lib/connector/__tests__/is-containerised.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockExistsSync = vi.fn(); +vi.mock("node:fs", () => ({ existsSync: (p: string) => mockExistsSync(p) })); + +import { isContainerised, _resetContainerisedCache } from "../is-containerised"; + +beforeEach(() => { + vi.clearAllMocks(); + _resetContainerisedCache(); +}); + +describe("isContainerised (#1346)", () => { + it("reports true when /.dockerenv exists", () => { + mockExistsSync.mockReturnValue(true); + expect(isContainerised()).toBe(true); + expect(mockExistsSync).toHaveBeenCalledWith("/.dockerenv"); + }); + + it("reports false on a host install", () => { + // The case that keeps the hint honest: a local-mode user pointing at + // localhost is correct, and must not be told to use a Docker hostname. + mockExistsSync.mockReturnValue(false); + expect(isContainerised()).toBe(false); + }); + + it("checks the filesystem only once", () => { + // Called from an error path; the answer cannot change while the process + // runs, so a stat per failed connection test would be pure waste. + mockExistsSync.mockReturnValue(true); + isContainerised(); + isContainerised(); + isContainerised(); + expect(mockExistsSync).toHaveBeenCalledTimes(1); + }); + + it("caches false as firmly as true", () => { + // `cached ??= …` treats a cached false as unset if written naively. + mockExistsSync.mockReturnValue(false); + expect(isContainerised()).toBe(false); + expect(isContainerised()).toBe(false); + expect(mockExistsSync).toHaveBeenCalledTimes(1); + }); +}); diff --git a/app/src/lib/connector/connection-error-classifier.ts b/app/src/lib/connector/connection-error-classifier.ts index 719292be..64394c66 100644 --- a/app/src/lib/connector/connection-error-classifier.ts +++ b/app/src/lib/connector/connection-error-classifier.ts @@ -15,8 +15,44 @@ export type ConnectionErrorCode = | "auth_failed" | "network" | "bad_uri" + /** A loopback host, unreachable because we are inside a container (#1346). */ + | "container_loopback" | "unknown"; +/** What the classifier needs beyond the message to spot a Docker networking miss. */ +export interface ConnectionErrorContext { + /** The URI the user entered. */ + uri?: string; + /** Whether the app itself is running inside a container. */ + containerised?: boolean; +} + +/** + * Is this URI pointed at the machine it is running on? + * + * Parsed, not substring-matched: "myhost-localhost.example.com" contains + * "localhost" and is not loopback. Returns false for anything unparseable — + * this runs on an error path, where a throw would replace a bad message with + * a 500, and a malformed URI is already better served by `bad_uri`. + */ +function isLoopbackUri(uri: string | undefined): boolean { + if (!uri) return false; + let host: string; + try { + host = new URL(uri).hostname.toLowerCase(); + } catch { + return false; + } + // URL strips the brackets from [::1]; both forms normalise to "::1". + return ( + host === "localhost" || + host === "127.0.0.1" || + host === "::1" || + host === "[::1]" || + host.endsWith(".localhost") + ); +} + /** * Shown when a connector's check returns false *without* throwing — there's no * driver message to classify, so the old "Connection check returned false" was @@ -77,13 +113,27 @@ function containsAny(text: string, keywords: string[]): boolean { * Why auth above network: failed auth attempts can be reported on top of * transient network warnings; the user's first step is to fix credentials. */ -export function classifyConnectionError(message: string): ConnectionErrorCode { +export function classifyConnectionError( + message: string, + context?: ConnectionErrorContext, +): ConnectionErrorCode { if (!message) return "unknown"; const m = message.toLowerCase(); if (containsAny(m, BAD_URI_KEYWORDS)) return "bad_uri"; if (containsAny(m, AUTH_KEYWORDS)) return "auth_failed"; - if (containsAny(m, NETWORK_KEYWORDS)) return "network"; + if (containsAny(m, NETWORK_KEYWORDS)) { + // Narrowing a network failure, never overriding auth or bad_uri: a + // loopback auth failure is still an auth failure, and pointing at Docker + // there would be a misdiagnosis. + // + // The containerised check is what keeps this honest. In local mode the app + // runs on the host, where localhost is exactly right — that user must not + // be sent to a Docker hostname that does not exist for them. + return context?.containerised && isLoopbackUri(context.uri) + ? "container_loopback" + : "network"; + } return "unknown"; } @@ -94,6 +144,8 @@ const HINTS: Record = { "The server is unreachable. Verify the host and port, confirm the database is running, and check that no firewall is blocking the connection.", bad_uri: "The connection URI looks malformed. Confirm the scheme (e.g. `bolt://` or `neo4j+s://` for Neo4j, `postgresql://` for PostgreSQL) and that the host/port are present.", + container_loopback: + "The connection is opened by the NeoBoard **server**, not by your browser — so `localhost` means the machine NeoBoard runs on, and right now that is the container it runs inside. If the database is on that same host, restart NeoBoard with `neoboard start --full --expose-host` and use `host.docker.internal` in place of `localhost` (e.g. `neo4j://host.docker.internal:7687`). A database in the same Docker network is reached by its service name. If the database is on **your own computer** and NeoBoard is deployed elsewhere, it is not reachable at all — the server cannot see your machine; expose it at a routable address first.", unknown: "Connection test failed for an unrecognised reason. Check the server logs for more detail.", }; diff --git a/app/src/lib/connector/connection-test-result.ts b/app/src/lib/connector/connection-test-result.ts index 2e5c4af5..c33c94cc 100644 --- a/app/src/lib/connector/connection-test-result.ts +++ b/app/src/lib/connector/connection-test-result.ts @@ -3,6 +3,7 @@ import { classifyConnectionError, CONNECTION_CHECK_FALSE_MESSAGE, type ConnectionErrorCode, + type ConnectionErrorContext, } from "@/lib/connector/connection-error-classifier"; /** @@ -28,11 +29,14 @@ export function connectionCheckFalseResult(): ConnectionTestResult { /** A thrown driver error — classify for a targeted hint, then sanitize for display. */ export function connectionTestErrorResult( thrown: unknown, + context?: ConnectionErrorContext, ): ConnectionTestResult { const rawMessage = thrown instanceof Error ? thrown.message : "Connection test failed"; // Classify BEFORE sanitization — the classifier needs the raw driver text. - const code = classifyConnectionError(rawMessage); + // The context is read here and never returned: a URI can carry a password, + // so it informs the code and goes no further (#1346). + const code = classifyConnectionError(rawMessage, context); const error = sanitizeErrorMessage(rawMessage, "Connection test failed"); return { success: false, code, error }; } diff --git a/app/src/lib/connector/container-host.ts b/app/src/lib/connector/container-host.ts new file mode 100644 index 00000000..c3de9920 --- /dev/null +++ b/app/src/lib/connector/container-host.ts @@ -0,0 +1,37 @@ +import { isContainerised } from "./is-containerised"; + +/** + * Rewrite a loopback host to `host.docker.internal` when we are containerised. + * + * From inside a container, `localhost` is the container — so a loopback + * database URI is never a useful target in this deployment, and the user + * almost always meant "the machine NeoBoard runs on". Explaining that in an + * error message was the first half of #1346; this makes it work. + * + * Not a silent lie: the stored connection keeps exactly what the user typed, + * and this applies only at the moment a driver is built. On Linux the rewritten + * host resolves only when the stack was started with `--expose-host`, and the + * failure then still classifies as container_loopback because callers classify + * against the ORIGINAL uri — so the hint naming that flag still fires. + * + * Untouched outside a container: in local mode the app runs on the host, where + * `localhost` is exactly right. + */ +export const CONTAINER_HOST_ALIAS = "host.docker.internal"; + +const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "::1", "[::1]"]); + +export function resolveContainerHost(uri: string): string { + if (!uri || !isContainerised()) return uri; + let parsed: URL; + try { + parsed = new URL(uri); + } catch { + // Unparseable: leave it alone and let the driver report it. Rewriting a + // string we do not understand is how a bad URI becomes a confusing one. + return uri; + } + if (!LOOPBACK_HOSTS.has(parsed.hostname.toLowerCase())) return uri; + parsed.hostname = CONTAINER_HOST_ALIAS; + return parsed.toString(); +} diff --git a/app/src/lib/connector/is-containerised.ts b/app/src/lib/connector/is-containerised.ts new file mode 100644 index 00000000..3bf289c1 --- /dev/null +++ b/app/src/lib/connector/is-containerised.ts @@ -0,0 +1,25 @@ +import { existsSync } from "node:fs"; + +/** + * Is this process running inside a container? + * + * Used to tell two identical-looking failures apart: a connection to + * `localhost` that fails from the host is a real network problem, while the + * same failure from inside a container usually means `localhost` resolved to + * the container rather than the user's machine (#1346). + * + * `/.dockerenv` is written by the Docker runtime and is the cheap, stable + * signal. Evaluated once — the answer cannot change while the process runs, + * and this is called from an error path. + */ +let cached: boolean | undefined; + +export function isContainerised(): boolean { + cached ??= existsSync("/.dockerenv"); + return cached; +} + +/** @internal — test-only, since the answer is cached for the process lifetime. */ +export function _resetContainerisedCache(): void { + cached = undefined; +} diff --git a/app/src/lib/query/query-executor.ts b/app/src/lib/query/query-executor.ts index 24781782..db6e153c 100644 --- a/app/src/lib/query/query-executor.ts +++ b/app/src/lib/query/query-executor.ts @@ -7,6 +7,7 @@ import { ensureDatabaseInUri, rewriteParamsForPostgres } from "./query-params"; import { QueryStatus } from "@neoboard/connection"; import { createHash } from "node:crypto"; +import { resolveContainerHost } from "@/lib/connector/container-host"; /** * Default row cap applied to read queries when a connection doesn't * specify its own `maxRows`. Matches the connection package's own @@ -216,7 +217,13 @@ function getOrCreateModule( } const authConfig = { - uri: ensureDatabaseInUri(credentials.uri, credentials.database), + // resolveContainerHost LAST: from inside a container `localhost` is the + // container, so a loopback URI can never reach the user's database. The + // stored connection keeps what they typed; only the driver sees the + // rewrite (#1346). + uri: resolveContainerHost( + ensureDatabaseInUri(credentials.uri, credentials.database), + ), username: credentials.username, password: credentials.password, authType: 1, // NATIVE diff --git a/cli/src/__tests__/commands/start.test.ts b/cli/src/__tests__/commands/start.test.ts index 089a10b4..e30a8354 100644 --- a/cli/src/__tests__/commands/start.test.ts +++ b/cli/src/__tests__/commands/start.test.ts @@ -89,7 +89,10 @@ describe("runStart", () => { it("starts DB containers (not full stack) in docker mode", async () => { await runStart(); - expect(mockComposeUp).toHaveBeenCalledWith({ full: false }); + expect(mockComposeUp).toHaveBeenCalledWith({ + full: false, + exposeHost: false, + }); }); it("skips composeUp in local mode", async () => { @@ -270,4 +273,49 @@ describe("runStart", () => { const lines = mockBanner.mock.calls[0][0]; expect(lines.some((l) => l.includes("docker/.env"))).toBe(false); }); + + it("passes --expose-host through to compose (#1346)", () => { + // Off by default above; on only when asked for. + return runStart({ full: true, exposeHost: true }).then(() => { + expect(mockComposeUp).toHaveBeenCalledWith({ + full: true, + exposeHost: true, + }); + }); + }); + + // --expose-host overlays extra_hosts onto the `neoboard` service, which only + // the FULL compose defines. Without --full, compose refuses the entire + // project with "service neoboard has neither an image nor a build context + // specified" — an error about the wrong thing entirely. Verified against + // real `docker compose config` (#1346). + describe("--expose-host requires Docker full-stack mode", () => { + it("rejects --expose-host without --full, naming the fix", async () => { + vi.mocked(getMode).mockReturnValue("docker"); + const ok = await runStart({ full: false, exposeHost: true }); + expect(ok).toBe(false); + expect(process.exitCode).toBe(1); + expect(vi.mocked(error).mock.calls[0][0]).toContain("--full"); + expect(mockComposeUp).not.toHaveBeenCalled(); + }); + + it("rejects --expose-host in local mode, saying why it is moot", async () => { + // The app runs on the host there, so localhost already reaches the + // databases and the flag has nothing to do. + vi.mocked(getMode).mockReturnValue("local"); + const ok = await runStart({ full: true, exposeHost: true }); + expect(ok).toBe(false); + expect(vi.mocked(error).mock.calls[0][0]).toMatch(/local mode/i); + expect(mockComposeUp).not.toHaveBeenCalled(); + }); + + it("allows the valid combination", async () => { + vi.mocked(getMode).mockReturnValue("docker"); + await runStart({ full: true, exposeHost: true }); + expect(mockComposeUp).toHaveBeenCalledWith({ + full: true, + exposeHost: true, + }); + }); + }); }); diff --git a/cli/src/__tests__/lib/docker.test.ts b/cli/src/__tests__/lib/docker.test.ts index 9bc95f76..89a2d30e 100644 --- a/cli/src/__tests__/lib/docker.test.ts +++ b/cli/src/__tests__/lib/docker.test.ts @@ -96,6 +96,43 @@ describe("composeUp", () => { ); }); + // Reaching a database on the HOST is opt-in per run (#1346): most installs + // do not need it — a database in the same compose network is reached by its + // service name — and it punches a route from the container to the host's + // network, so it should not be on by default for everyone. + describe("--expose-host overlay (#1346)", () => { + const cmd = (n = 0) => vi.mocked(run).mock.calls[n][0] as string; + + it("is absent unless asked for", () => { + composeUp({ full: true }); + expect(cmd()).not.toContain("expose-host"); + }); + + it.each([[false], [true]])( + "adds the overlay when requested (full=%s)", + (full) => { + composeUp({ full, exposeHost: true }); + expect(cmd()).toContain("docker-compose.expose-host.yml"); + }, + ); + + it("layers the overlay AFTER the base file", () => { + // Compose merges left to right; an overlay listed first would be + // overridden by the base and silently do nothing. + composeUp({ full: true, exposeHost: true }); + const c = cmd(); + expect(c.indexOf("docker-compose.full.yml")).toBeLessThan( + c.indexOf("docker-compose.expose-host.yml"), + ); + }); + + it("keeps the generated env-file for the full stack", () => { + // The overlay must not displace the per-install secrets (#970). + composeUp({ full: true, exposeHost: true }); + expect(cmd()).toContain('--env-file "/project/docker/.env"'); + }); + }); + // The configured ports were consumed by every readiness probe, the generated // DATABASE_URL and the banner URLs — but NOT by the thing that binds them. // `neoboard config set ports.app 4000` published on 3000, polled 4000, and diff --git a/cli/src/commands/start.ts b/cli/src/commands/start.ts index 13b74de7..92ce7cbc 100644 --- a/cli/src/commands/start.ts +++ b/cli/src/commands/start.ts @@ -26,6 +26,12 @@ export interface StartOptions { * Only applies to Docker mode. */ full?: boolean; + /** + * Let the app container reach databases on the HOST machine, by mapping + * host.docker.internal (#1346). Opt-in: most installs do not need it, and it + * routes from the container out to the host's network. + */ + exposeHost?: boolean; } /** @@ -37,6 +43,23 @@ export async function runStart(opts?: StartOptions): Promise { const mode = getMode(); const config = readProjectConfig(); const full = opts?.full ?? false; + const exposeHost = opts?.exposeHost ?? false; + + // --expose-host overlays extra_hosts onto the `neoboard` service, which only + // the FULL docker compose defines. Without --full the overlay lands on a + // service that does not exist and compose refuses the whole project with + // "service neoboard has neither an image nor a build context specified" — + // an error about the wrong thing entirely. In local mode the app runs on the + // host, where localhost already reaches the host and the flag is meaningless. + if (exposeHost && (mode !== "docker" || !full)) { + error( + mode === "docker" + ? "--expose-host needs --full: it maps a hostname for the app container, which only the full stack starts. Try: neoboard start --full --expose-host" + : "--expose-host applies to Docker mode only. In local mode the app runs on this machine, so `localhost` already reaches your databases.", + ); + process.exitCode = 1; + return false; + } // 1. Prerequisite checks const results = await runDoctor(); @@ -53,7 +76,7 @@ export async function runStart(opts?: StartOptions): Promise { } else { info("Starting database containers via Docker Compose..."); } - composeUp({ full }); + composeUp({ full, exposeHost }); } else { info( "Local mode — skipping Docker. Ensure PostgreSQL and Neo4j are running.", diff --git a/cli/src/index.ts b/cli/src/index.ts index 218818e9..180e57ee 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -48,12 +48,20 @@ program .description( "Start NeoBoard services, run migrations\n" + " Docker mode: starts database containers (add --full for the app)\n" + + " --expose-host: reach a database on this machine, outside Docker\n" + " Local mode: connects to your running PostgreSQL + Neo4j", ) .option("--full", "Docker mode: also start the app container (#968)", false) + .option( + "--expose-host", + "Let the app reach databases running on this machine, outside Docker " + + "(maps host.docker.internal). Then connect with " + + "host.docker.internal instead of localhost.", + false, + ) .action(async (opts) => { const { runStart } = await import("./commands/start.js"); - await runStart({ full: opts.full }); + await runStart({ full: opts.full, exposeHost: opts.exposeHost }); }); program diff --git a/cli/src/lib/docker.ts b/cli/src/lib/docker.ts index a1427732..69b6bcf0 100644 --- a/cli/src/lib/docker.ts +++ b/cli/src/lib/docker.ts @@ -59,20 +59,45 @@ function composeEnv(): NodeJS.ProcessEnv { }; } -export function composeUp(opts?: { full?: boolean }): void { +/** + * Overlay that lets the app container reach databases on the HOST (#1346). + * + * Opt-in per run rather than baked into the base compose files: most installs + * do not need it — a database in the same compose network is reached by its + * service name, a remote one by its hostname — and it routes from the + * container to the host's network, which is not something to enable for + * everyone by default. + * + * Layered AFTER the base file: compose merges left to right, so an overlay + * listed first would be overridden by the base and silently do nothing. + */ +function exposeHostFlag(exposeHost: boolean | undefined): string { + return exposeHost + ? ` -f "${join(paths.dockerDir, "docker-compose.expose-host.yml")}"` + : ""; +} + +export function composeUp(opts?: { + full?: boolean; + exposeHost?: boolean; +}): void { const file = composeFile(opts?.full); + const overlay = exposeHostFlag(opts?.exposeHost); const env = composeEnv(); if (opts?.full) { // The full stack needs per-install secrets (#970); generated once, // reused forever. OS env still overrides --env-file values (CI). const envFile = ensureDockerEnvFile(); - run(`docker compose -f "${file}" --env-file "${envFile}" up -d --build`, { - cwd: paths.root, - env, - }); + run( + `docker compose -f "${file}"${overlay} --env-file "${envFile}" up -d --build`, + { cwd: paths.root, env }, + ); return; } - run(`docker compose -f "${file}" up -d --build`, { cwd: paths.root, env }); + run(`docker compose -f "${file}"${overlay} up -d --build`, { + cwd: paths.root, + env, + }); } export function composeDown(opts?: { volumes?: boolean }): void { diff --git a/docker/docker-compose.expose-host.yml b/docker/docker-compose.expose-host.yml new file mode 100644 index 00000000..a17ef119 --- /dev/null +++ b/docker/docker-compose.expose-host.yml @@ -0,0 +1,19 @@ +# Opt-in overlay: let the app container reach databases on the HOST machine. +# +# Applied only when you pass `--expose-host`: +# +# neoboard start --full --expose-host +# +# Off by default deliberately. This punches a route from the container to the +# host's network, which most installs do not need — a database inside the same +# compose network is reached by its service name, and a remote one by its real +# hostname. Only reach for this when the database is running on your own +# machine, outside Docker. +# +# On Docker Desktop (macOS/Windows) `host.docker.internal` already resolves +# without this file; on Linux it does not, and this mapping is what makes the +# connection-failure hint (#1346) true there rather than a dead end. +services: + neoboard: + extra_hosts: + - "host.docker.internal:host-gateway" diff --git a/scripts/__tests__/docs-accuracy.test.mjs b/scripts/__tests__/docs-accuracy.test.mjs index 28bf2562..362763a2 100644 --- a/scripts/__tests__/docs-accuracy.test.mjs +++ b/scripts/__tests__/docs-accuracy.test.mjs @@ -219,3 +219,69 @@ describe("docs accuracy guards (#1316)", () => { expect(broken).toEqual([]); }); }); + +// Not docs, but the same class of silent-wrong: a hint that names a hostname +// which does not resolve is worse than no hint. The connection-failure hint +// tells users to reach a host database via host.docker.internal, which Docker +// Desktop provides automatically and Linux does NOT — it needs an explicit +// host-gateway mapping. The CLI supplies that via an opt-in overlay, so the +// overlay is the thing that has to keep existing and keep saying it (#1346). +describe("the --expose-host overlay backs the hint that names it", () => { + it("maps host.docker.internal on the APP service", () => { + // Resolved by compose, not substring-matched: the raw string would also + // pass from a comment, an unrelated service, or malformed YAML — and a + // mapping on the wrong service is exactly the failure this guards. + const resolved = JSON.parse( + execFileSync( + "docker", + [ + "compose", + "-f", + join(ROOT, "docker/docker-compose.full.yml"), + "-f", + join(ROOT, "docker/docker-compose.expose-host.yml"), + "config", + "--format", + "json", + ], + { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + // Placeholders for the compose file's `:?` required vars, rather + // than --env-file docker/.env: that file is gitignored and + // CLI-generated, so it exists on a developer machine and NOT in CI. + // Depending on it is how this check passed locally and failed there + // — the same trap as #1221, in the test written to avoid traps. + env: { + ...process.env, + ENCRYPTION_KEY: "0".repeat(64), + NEXTAUTH_SECRET: "test-secret", + API_KEY_HMAC_SECRET: "0".repeat(64), + }, + }, + ), + ); + // Compose normalises extra_hosts differently across versions and shapes: + // an array with `:`, an array with `=`, or a host->target map. CI emits + // `host.docker.internal=host-gateway` where this machine emits `:`, which + // failed a literal comparison. Compare the MAPPING, not the spelling. + const raw = resolved.services.neoboard.extra_hosts ?? []; + const mappings = ( + Array.isArray(raw) ? raw : Object.entries(raw).map((e) => e.join("=")) + ).map((entry) => entry.replace(/[:=]/, "=")); + + expect(mappings).toContain("host.docker.internal=host-gateway"); + }, 60_000); + + it("is the ONLY place that maps it — otherwise the flag is a no-op", () => { + // If a base compose file also carried the mapping, --expose-host would + // appear to work while actually doing nothing, and removing the overlay + // would break nothing visible until a Linux user hit it. + const carriers = readdirSync(join(ROOT, "docker")) + .filter((f) => f.endsWith(".yml")) + .filter((f) => + readFileSync(join(ROOT, "docker", f), "utf8").includes("host-gateway"), + ); + expect(carriers).toEqual(["docker-compose.expose-host.yml"]); + }); +});