Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion app/src/app/api/connections/[id]/test/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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");
Expand Down
8 changes: 7 additions & 1 deletion app/src/app/api/connections/test-inline/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,90 @@ 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 and host.docker.internal in the hint", () => {
const hint = hintForConnectionErrorCode("container_loopback");
expect(hint).toMatch(/host\.docker\.internal/);
expect(hint).toMatch(/container/i);
});
});
31 changes: 31 additions & 0 deletions app/src/lib/connector/__tests__/connection-test-result.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
44 changes: 44 additions & 0 deletions app/src/lib/connector/__tests__/is-containerised.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
56 changes: 54 additions & 2 deletions app/src/lib/connector/connection-error-classifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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";
}

Expand All @@ -94,6 +144,8 @@ const HINTS: Record<ConnectionErrorCode, string> = {
"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:
"NeoBoard is running inside a container, so `localhost` means the container itself — not your machine. To reach a database running on your host, use `host.docker.internal` instead of `localhost` (e.g. `neo4j://host.docker.internal:7687`). A database in the same Docker network can be reached by its service name.",
unknown:
"Connection test failed for an unrecognised reason. Check the server logs for more detail.",
};
Expand Down
6 changes: 5 additions & 1 deletion app/src/lib/connector/connection-test-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
classifyConnectionError,
CONNECTION_CHECK_FALSE_MESSAGE,
type ConnectionErrorCode,
type ConnectionErrorContext,
} from "@/lib/connector/connection-error-classifier";

/**
Expand All @@ -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 };
}
25 changes: 25 additions & 0 deletions app/src/lib/connector/is-containerised.ts
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 6 additions & 0 deletions docker/docker-compose.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ services:
context: ..
dockerfile: Dockerfile
container_name: neoboard-app
# host.docker.internal resolves automatically on Docker Desktop but NOT
# on Linux without this. The connection-failure hint names it when a
# user points at a database on the host, so it has to resolve
# everywhere or the hint sends them somewhere that does not exist (#1346).
extra_hosts:
- "host.docker.internal:host-gateway"
ports:
- "${NEOBOARD_PORT_APP:-3000}:3000"
environment:
Expand Down
6 changes: 6 additions & 0 deletions docker/docker-compose.prod-full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ services:
build:
context: ..
dockerfile: Dockerfile
# host.docker.internal resolves automatically on Docker Desktop but NOT
# on Linux without this. The connection-failure hint names it when a
# user points at a database on the host, so it has to resolve
# everywhere or the hint sends them somewhere that does not exist (#1346).
extra_hosts:
- "host.docker.internal:host-gateway"
ports:
- "${PORT:-3000}:3000"
environment:
Expand Down
6 changes: 6 additions & 0 deletions docker/docker-compose.prod.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ services:
build:
context: ..
dockerfile: Dockerfile
# host.docker.internal resolves automatically on Docker Desktop but NOT
# on Linux without this. The connection-failure hint names it when a
# user points at a database on the host, so it has to resolve
# everywhere or the hint sends them somewhere that does not exist (#1346).
extra_hosts:
- "host.docker.internal:host-gateway"
ports:
- "${PORT:-3000}:3000"
environment:
Expand Down
Loading
Loading