From 0dbccd387e00291ce34a2fa799800a0abdc6b823 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 07:47:33 +0100 Subject: [PATCH 1/4] fix(server): handle listen errors with a clear message and exit 1 app.listen() had no error handling, so a port conflict surfaced as an uncaught EADDRINUSE with a raw stack trace. Attach an error handler that maps EADDRINUSE, EACCES and EADDRNOTAVAIL to a readable explanation, logs it through the structured logger, and exits 1 so process supervisors treat the start-up as failed. Closes #206 --- src/__tests__/listen-errors.test.ts | 68 +++++++++++++++++++++++++++++ src/index.ts | 5 +++ src/lib/listen-errors.ts | 38 ++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 src/__tests__/listen-errors.test.ts create mode 100644 src/lib/listen-errors.ts diff --git a/src/__tests__/listen-errors.test.ts b/src/__tests__/listen-errors.test.ts new file mode 100644 index 0000000..07834be --- /dev/null +++ b/src/__tests__/listen-errors.test.ts @@ -0,0 +1,68 @@ +import { describeListenError, handleListenError } from "../lib/listen-errors"; + +function errno(code: string, message = "listen error"): NodeJS.ErrnoException { + const err = new Error(message) as NodeJS.ErrnoException; + err.code = code; + return err; +} + +describe("listen error handling (#206)", () => { + describe("describeListenError", () => { + it("explains EADDRINUSE with the conflicting port", () => { + expect(describeListenError(errno("EADDRINUSE"), 3001)).toBe( + "Port 3001 is already in use. Stop the process using it or set PORT to a free port.", + ); + }); + + it("explains EACCES as a privilege problem", () => { + const message = describeListenError(errno("EACCES"), 80); + expect(message).toContain("Port 80"); + expect(message).toContain("elevated privileges"); + }); + + it("explains EADDRNOTAVAIL", () => { + expect(describeListenError(errno("EADDRNOTAVAIL"), 3001)).toContain("not available"); + }); + + it("falls back to the underlying message for other listen errors", () => { + const message = describeListenError(errno("EPERM", "operation not permitted"), 3001); + expect(message).toBe("Failed to bind to port 3001: operation not permitted"); + }); + + it("handles errors without a code", () => { + const err = new Error("boom") as NodeJS.ErrnoException; + expect(describeListenError(err, 3001)).toBe("Failed to bind to port 3001: boom"); + }); + }); + + describe("handleListenError", () => { + let errorSpy: jest.SpyInstance; + + beforeEach(() => { + errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + }); + + it("exits with status code 1 on a bind failure", () => { + const exit = jest.fn(); + handleListenError(errno("EADDRINUSE"), 3001, exit); + expect(exit).toHaveBeenCalledWith(1); + }); + + it("logs a clear port-conflict message", () => { + handleListenError(errno("EADDRINUSE"), 3001, jest.fn()); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain("Port 3001 is already in use"); + expect(JSON.parse(logged)).toMatchObject({ level: "error", error_code: "EADDRINUSE" }); + }); + + it("exits on non-EADDRINUSE listen errors too", () => { + const exit = jest.fn(); + handleListenError(errno("EACCES"), 80, exit); + expect(exit).toHaveBeenCalledWith(1); + }); + }); +}); diff --git a/src/index.ts b/src/index.ts index 67af7eb..bdc71b2 100644 --- a/src/index.ts +++ b/src/index.ts @@ -73,6 +73,7 @@ import { getMigrationStatus, runMigrations, rollbackMigration } from "./lib/migr import { featureFlagContext, registerFlagRoutes } from "./middleware/featureFlags"; import { loadFlags, getFlagAnalytics } from "./lib/feature-flags"; import { compressionMiddleware, getCompressionMetrics } from "./middleware/compression"; +import { handleListenError } from "./lib/listen-errors"; const env = initEnv(); @@ -451,6 +452,10 @@ const server = app.listen(PORT, () => { logger.info(`Heliobond backend listening on port ${PORT}`); }); +// Bind failures (EADDRINUSE, EACCES, …) surface here instead of as an uncaught +// exception with a raw stack trace. Exits 1 so supervisors treat it as a failure. +server.on("error", (err: NodeJS.ErrnoException) => handleListenError(err, PORT)); + // Real-time score updates over WebSocket (ws:///ws) attachWebSocketServer(server); diff --git a/src/lib/listen-errors.ts b/src/lib/listen-errors.ts new file mode 100644 index 0000000..f949005 --- /dev/null +++ b/src/lib/listen-errors.ts @@ -0,0 +1,38 @@ +import { logger } from "./logger"; + +/** + * Human-readable explanation for a `net.Server` "error" event raised while the + * HTTP server is binding. Node's default behaviour is an uncaught exception with + * a stack trace like `Error: listen EADDRINUSE: address already in use :::3001`, + * which buries the actual problem — and the fix — in noise. + */ +export function describeListenError(err: NodeJS.ErrnoException, port: number | string): string { + switch (err.code) { + case "EADDRINUSE": + return `Port ${port} is already in use. Stop the process using it or set PORT to a free port.`; + case "EACCES": + return `Port ${port} requires elevated privileges. Use a port above 1023 or run with the required permissions.`; + case "EADDRNOTAVAIL": + return `The address for port ${port} is not available on this host.`; + default: + return `Failed to bind to port ${port}: ${err.message}`; + } +} + +/** + * Log a clear message for a server bind failure and terminate with a non-zero + * status so process managers (Docker, systemd, k8s) see the start-up as failed. + * + * `exit` is injectable so the behaviour can be tested without killing the runner. + */ +export function handleListenError( + err: NodeJS.ErrnoException, + port: number | string, + exit: (code: number) => void = (code) => process.exit(code), +): void { + logger.error(`[startup] ${describeListenError(err, port)}`, { + error_code: err.code ?? "UNKNOWN", + port, + }); + exit(1); +} From 755633ebfe17d4060f937e41e956bd2c969577d4 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 07:50:38 +0100 Subject: [PATCH 2/4] chore(runtime): pin Node.js and Bun versions The project targets Node.js 20 (README, Dockerfile) but nothing enforced it: no engines field, no .nvmrc, and setup-bun ran unpinned in CI. Add engines.node >=20.0.0 and engines.bun >=1.0.0, an .nvmrc for nvm users, and pin bun-version: "1.x" on every setup-bun step so CI, release and the security audit all resolve the same runtime. Closes #207 --- .github/workflows/ci.yml | 4 ++ .github/workflows/release.yml | 2 + .github/workflows/security-audit.yml | 4 ++ .nvmrc | 1 + package.json | 4 ++ src/__tests__/runtime-version-pinning.test.ts | 53 +++++++++++++++++++ 6 files changed, 68 insertions(+) create mode 100644 .nvmrc create mode 100644 src/__tests__/runtime-version-pinning.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 14676ab..60cb226 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,8 @@ jobs: - name: Install bun uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.x" - name: Install dependencies run: bun install --frozen-lockfile @@ -36,6 +38,8 @@ jobs: - name: Install bun uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.x" - name: Install dependencies run: bun install --frozen-lockfile diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a912762..f12dfbf 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,6 +35,8 @@ jobs: - name: Install bun uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.x" - name: Install dependencies run: bun install --frozen-lockfile diff --git a/.github/workflows/security-audit.yml b/.github/workflows/security-audit.yml index bddc322..c2e02ca 100644 --- a/.github/workflows/security-audit.yml +++ b/.github/workflows/security-audit.yml @@ -18,6 +18,8 @@ jobs: - name: Install bun uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.x" - name: Install dependencies run: bun install --frozen-lockfile @@ -99,6 +101,8 @@ jobs: - name: Install bun uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.x" - name: Install dependencies run: bun install --frozen-lockfile diff --git a/.nvmrc b/.nvmrc new file mode 100644 index 0000000..209e3ef --- /dev/null +++ b/.nvmrc @@ -0,0 +1 @@ +20 diff --git a/package.json b/package.json index 0106881..d38f15e 100644 --- a/package.json +++ b/package.json @@ -2,6 +2,10 @@ "name": "heliobond-backend", "version": "1.0.0", "private": true, + "engines": { + "node": ">=20.0.0", + "bun": ">=1.0.0" + }, "scripts": { "dev": "ts-node src/index.ts", "build": "tsc", diff --git a/src/__tests__/runtime-version-pinning.test.ts b/src/__tests__/runtime-version-pinning.test.ts new file mode 100644 index 0000000..112dee9 --- /dev/null +++ b/src/__tests__/runtime-version-pinning.test.ts @@ -0,0 +1,53 @@ +import * as fs from "fs"; +import * as path from "path"; + +const ROOT = path.resolve(__dirname, "../../"); + +function read(relativePath: string): string { + return fs.readFileSync(path.join(ROOT, relativePath), "utf8"); +} + +describe("runtime version pinning (#207)", () => { + describe("package.json engines", () => { + const pkg = JSON.parse(read("package.json")) as { + engines?: { node?: string; bun?: string }; + }; + + it("declares an engines.node range", () => { + expect(pkg.engines?.node).toBeDefined(); + }); + + it("requires Node.js 20 or newer", () => { + expect(pkg.engines?.node).toBe(">=20.0.0"); + }); + + it("declares a bun range for the bun-based scripts", () => { + expect(pkg.engines?.bun).toMatch(/^>=1\./); + }); + }); + + describe(".nvmrc", () => { + it("exists", () => { + expect(fs.existsSync(path.join(ROOT, ".nvmrc"))).toBe(true); + }); + + it("pins Node.js 20, matching engines and the Dockerfile", () => { + expect(read(".nvmrc").trim()).toBe("20"); + expect(read("Dockerfile")).toContain("node:20-alpine"); + }); + }); + + describe("CI workflows", () => { + const workflows = ["ci.yml", "release.yml", "security-audit.yml"]; + + it.each(workflows)("%s pins a bun-version for every setup-bun step", (workflow) => { + const content = read(path.join(".github", "workflows", workflow)); + const setupSteps = content.match(/uses: oven-sh\/setup-bun@v\d+[\s\S]*?(?=\n\n|\n {6}- |$)/g); + + expect(setupSteps).not.toBeNull(); + for (const step of setupSteps ?? []) { + expect(step).toMatch(/bun-version:\s*"1\.x"/); + } + }); + }); +}); From 9c3ac5359a8f4b28f989d422bbc4b0e7187d8fb3 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 07:51:05 +0100 Subject: [PATCH 3/4] fix(security): send X-Frame-Options: DENY helmet already covered the header set, but frameguard was configured as SAMEORIGIN. This service is a JSON API and is never framed, and the CSP already declares frame-ancestors 'none', so DENY is both stricter and consistent with the policy already advertised. Closes #208 --- src/__tests__/securityHeaders.test.ts | 12 ++++++------ src/middleware/securityHeaders.ts | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/__tests__/securityHeaders.test.ts b/src/__tests__/securityHeaders.test.ts index 0d853d3..15a0efc 100644 --- a/src/__tests__/securityHeaders.test.ts +++ b/src/__tests__/securityHeaders.test.ts @@ -18,9 +18,9 @@ describe("securityHeaders middleware", () => { expect(res.headers["x-content-type-options"]).toBe("nosniff"); }); - it("sets X-Frame-Options: SAMEORIGIN", async () => { + it("sets X-Frame-Options: DENY", async () => { const res = await request(app).get("/test"); - expect(res.headers["x-frame-options"]).toBe("SAMEORIGIN"); + expect(res.headers["x-frame-options"]).toBe("DENY"); }); it("sets X-XSS-Protection header", async () => { @@ -44,20 +44,20 @@ describe("securityHeaders middleware", () => { const jsonRes = await request(app).get("/json"); expect(jsonRes.headers["x-content-type-options"]).toBe("nosniff"); - expect(jsonRes.headers["x-frame-options"]).toBe("SAMEORIGIN"); + expect(jsonRes.headers["x-frame-options"]).toBe("DENY"); const textRes = await request(app).get("/text"); expect(textRes.headers["x-content-type-options"]).toBe("nosniff"); - expect(textRes.headers["x-frame-options"]).toBe("SAMEORIGIN"); + expect(textRes.headers["x-frame-options"]).toBe("DENY"); const postRes = await request(app).post("/post"); expect(postRes.headers["x-content-type-options"]).toBe("nosniff"); - expect(postRes.headers["x-frame-options"]).toBe("SAMEORIGIN"); + expect(postRes.headers["x-frame-options"]).toBe("DENY"); }); it("sets all expected security headers on every request", async () => { const res = await request(app).get("/test"); - + const expectedHeaders = [ "x-content-type-options", "x-frame-options", diff --git a/src/middleware/securityHeaders.ts b/src/middleware/securityHeaders.ts index b98833c..c5897c1 100644 --- a/src/middleware/securityHeaders.ts +++ b/src/middleware/securityHeaders.ts @@ -4,7 +4,7 @@ import { RequestHandler } from "express"; /** * Composed helmet middleware that sets the following security headers: * Content-Security-Policy — restricts resource origins - * X-Frame-Options — blocks clickjacking (SAMEORIGIN) + * X-Frame-Options — blocks clickjacking (DENY; this is an API, never framed) * X-Content-Type-Options — prevents MIME sniffing * Strict-Transport-Security — enforces HTTPS for 1 year * X-XSS-Protection — legacy browser XSS filter @@ -26,7 +26,7 @@ export const securityHeaders: RequestHandler = helmet({ formAction: ["'self'"], }, }, - frameguard: { action: "sameorigin" }, + frameguard: { action: "deny" }, noSniff: true, hsts: { maxAge: 31_536_000, From aed09dc9ce0ac7cbc1dc854ad89a03a086bdb474 Mon Sep 17 00:00:00 2001 From: Joycejay17 Date: Thu, 30 Jul 2026 07:51:07 +0100 Subject: [PATCH 4/4] fix(security): use timing-safe comparison for admin bearer token The admin auth middleware compared the Authorization header to the API key with !==, which short-circuits on the first differing byte and leaks how many leading characters matched. Add timingSafeCompare, which hashes both values to a fixed 32-byte digest before crypto.timingSafeEqual so neither the contents nor the length of the token affect the comparison time. The auth flow and both error responses are unchanged. Closes #209 --- src/__tests__/admin.test.ts | 35 ++++++++++++++- src/__tests__/timing-safe.test.ts | 72 +++++++++++++++++++++++++++++++ src/lib/timing-safe.ts | 16 +++++++ src/routes/admin.ts | 5 ++- 4 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/timing-safe.test.ts create mode 100644 src/lib/timing-safe.ts diff --git a/src/__tests__/admin.test.ts b/src/__tests__/admin.test.ts index d2e5f07..20a28aa 100644 --- a/src/__tests__/admin.test.ts +++ b/src/__tests__/admin.test.ts @@ -133,11 +133,11 @@ describe("admin routes", () => { }); it("documents current behavior for a token with trailing whitespace", async () => { - // The middleware itself does a strict `!==` comparison with no + // The middleware itself does an exact constant-time comparison with no // trimming. However, HTTP header values are trimmed of leading/ // trailing whitespace by the underlying HTTP parser before the // handler ever sees them (per RFC 7230), so in practice a trailing - // space on the wire does NOT survive to reach the `!==` check — the + // space on the wire does NOT survive to reach the comparison — the // request passes through. This test pins down that actual observed // behavior; it is not asserting this is the desired security posture. const res = await request(app) @@ -155,6 +155,37 @@ describe("admin routes", () => { expect(res.status).toBe(401); expect(res.body.error.code).toBe("unauthorized"); }); + + // ── Constant-time comparison (#209) ──────────────────────────────────── + // The comparison is timing-safe, so near-miss tokens must be rejected the + // same way as completely wrong ones — no early exit on the first mismatch. + + it("rejects a token that differs only in the last character", async () => { + const res = await request(app) + .post("/api/admin/update-scores") + .set("Authorization", "Bearer test-keZ") + .send({}); + expect(res.status).toBe(401); + expect(res.body.error.code).toBe("unauthorized"); + }); + + it("rejects a token that is a prefix of the real key", async () => { + const res = await request(app) + .post("/api/admin/update-scores") + .set("Authorization", "Bearer test-k") + .send({}); + expect(res.status).toBe(401); + expect(res.body.error.code).toBe("unauthorized"); + }); + + it("rejects a token that extends the real key", async () => { + const res = await request(app) + .post("/api/admin/update-scores") + .set("Authorization", "Bearer test-key-extra") + .send({}); + expect(res.status).toBe(401); + expect(res.body.error.code).toBe("unauthorized"); + }); }); // ── POST /update-scores ────────────────────────────────────────────────── diff --git a/src/__tests__/timing-safe.test.ts b/src/__tests__/timing-safe.test.ts new file mode 100644 index 0000000..bc4bf26 --- /dev/null +++ b/src/__tests__/timing-safe.test.ts @@ -0,0 +1,72 @@ +// `crypto`'s exports are non-configurable, so the constant-time assertions below +// wrap timingSafeEqual through a module mock that still runs the real function. +const mockTimingSafeEqual = jest.fn(); + +jest.mock("crypto", () => { + const actual = jest.requireActual("crypto"); + return { + ...actual, + timingSafeEqual: (a: NodeJS.ArrayBufferView, b: NodeJS.ArrayBufferView) => { + mockTimingSafeEqual(a, b); + return actual.timingSafeEqual(a, b); + }, + }; +}); + +import { timingSafeCompare } from "../lib/timing-safe"; + +describe("timingSafeCompare (#209)", () => { + it("returns true for identical strings", () => { + expect(timingSafeCompare("Bearer secret-key", "Bearer secret-key")).toBe(true); + }); + + it("returns false when a single character differs", () => { + expect(timingSafeCompare("Bearer secret-key", "Bearer secret-keY")).toBe(false); + }); + + it("returns false for different lengths without throwing", () => { + expect(timingSafeCompare("short", "a-much-longer-value")).toBe(false); + expect(timingSafeCompare("a-much-longer-value", "short")).toBe(false); + }); + + it("handles empty strings", () => { + expect(timingSafeCompare("", "")).toBe(true); + expect(timingSafeCompare("", "Bearer key")).toBe(false); + }); + + it("is case sensitive", () => { + expect(timingSafeCompare("bearer key", "Bearer key")).toBe(false); + }); + + it("compares multi-byte characters correctly", () => { + expect(timingSafeCompare("clé-secrète", "clé-secrète")).toBe(true); + expect(timingSafeCompare("clé-secrète", "cle-secrete")).toBe(false); + }); + + describe("constant-time guarantees", () => { + beforeEach(() => { + mockTimingSafeEqual.mockClear(); + }); + + it("delegates to crypto.timingSafeEqual rather than ===", () => { + timingSafeCompare("Bearer key", "Bearer key"); + expect(mockTimingSafeEqual).toHaveBeenCalledTimes(1); + }); + + it("never short-circuits: mismatches at any position still reach the full compare", () => { + const key = "a".repeat(64); + timingSafeCompare("b" + "a".repeat(63), key); // wrong at the first byte + timingSafeCompare("a".repeat(63) + "b", key); // wrong at the last byte + expect(mockTimingSafeEqual).toHaveBeenCalledTimes(2); + }); + + it("always compares equal-length buffers, so no length is leaked", () => { + timingSafeCompare("x", "a-considerably-longer-secret-value"); + expect(mockTimingSafeEqual).toHaveBeenCalledTimes(1); + + const [a, b] = mockTimingSafeEqual.mock.calls[0] as [Buffer, Buffer]; + expect(a).toHaveLength(32); + expect(b).toHaveLength(32); + }); + }); +}); diff --git a/src/lib/timing-safe.ts b/src/lib/timing-safe.ts new file mode 100644 index 0000000..d20fcad --- /dev/null +++ b/src/lib/timing-safe.ts @@ -0,0 +1,16 @@ +import { createHash, timingSafeEqual } from "crypto"; + +/** + * Constant-time string comparison. + * + * A plain `a === b` short-circuits on the first differing byte, so the time it + * takes to reject a value leaks how many leading characters were correct — enough + * for an attacker to recover a secret byte-by-byte. Both inputs are hashed to a + * fixed 32-byte digest first so `timingSafeEqual` always compares equal-length + * buffers and the comparison never leaks the length of either input either. + */ +export function timingSafeCompare(a: string, b: string): boolean { + const digestA = createHash("sha256").update(a, "utf8").digest(); + const digestB = createHash("sha256").update(b, "utf8").digest(); + return timingSafeEqual(digestA, digestB); +} diff --git a/src/routes/admin.ts b/src/routes/admin.ts index 5e9c3fa..9378d65 100644 --- a/src/routes/admin.ts +++ b/src/routes/admin.ts @@ -8,6 +8,7 @@ import { tryBeginUpdate, markCompleted, markFailed } from "../lib/duplicate-dete import { withProjectLock } from "../lib/request-queue"; import { config } from "../config"; import { logger } from "../lib/logger"; +import { timingSafeCompare } from "../lib/timing-safe"; const router = Router(); @@ -19,7 +20,9 @@ router.use((req: Request, res: Response, next: NextFunction) => { .status(500) .json(errorBody("server_misconfigured", "Admin API key is not configured")); } - if (req.headers.authorization !== `Bearer ${apiKey}`) { + // Constant-time compare so response timing can't be used to guess the key. + const authorization = req.headers.authorization ?? ""; + if (!timingSafeCompare(authorization, `Bearer ${apiKey}`)) { return res.status(401).json(errorBody("unauthorized", "Missing or invalid bearer token")); } next();