From 7056193e6e9cd7e5fc0a416b06b7fa6cf5eab455 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 23:59:34 +0000 Subject: [PATCH 1/5] fix(security): close access-control, credential and upload issues A security review of the API surface. Each fix has tests covering the behaviour that was wrong. Access control - The admin user-update route guarded only the primary role, so `secondaryRoleIds` could attach a root role without `can_edit_admin` - `loadStaffPermissions` reads primary and secondary roles alike, so an administrator holding `users:can_edit` could make themselves root. Every role being assigned now goes through the guard, which also recognises root and moderator-granting roles. - The admin queue list selected every column, including `payload` - for `send-email` jobs the fully rendered message, live password-reset links included - for anyone with `queue:can_view`. It now selects the columns its response schema declares. - `POST /admin/notifications/send` required only an admin session, letting any restricted administrator push arbitrary in-product notifications to any user. Gated on `dashboard:can_edit`, like its sibling widget route. Credentials - Password-reset tokens were written to the database in plaintext (the hashing helper existed and was never called), so any read of the table was account takeover. Only the digest is stored now, and a completed reset revokes the user's sessions. - `CRON_SECRET` falls back to a constant published in this repository, so an install that never set it ran every cron job for anyone. Refused outside development, along with the scaffolded `.env.example` placeholder; the comparison is timing-safe and the `Bearer` prefix is matched rather than substring-replaced. - Sign-in answered "no such email" without hashing, timing-disclosing which addresses hold accounts. Both paths now derive a key. - `verifyPassword` continued after rejecting and threw a 500 on a malformed stored hash; the salt widens to 16 bytes for new hashes. Rate limiting and identity - The limiter was registered before the middleware that set `ipAddress`, so every request in the deployment shared one bucket named `undefined` - no per-client throttling, and a global kill switch at 80 requests a minute. Its unit test set `ipAddress` first, the opposite of the real wiring. - The client address was read from the first of sixteen client-settable headers, so any caller could choose their own bucket and their own line in the audit trail. Resolution is socket-based unless `trustProxy` says how many proxies are in front, and counts from the right so a forged chain is stepped over. Runtimes with no connection info now warn. Uploads and transport - The stored extension came from the client filename while the type came from the client `Content-Type`, so a file accepted as `image/gif` could be written as `.html` and served as a page from the app's own origin. The extension is now bound to the validated media type, and the uploads mount sends `Content-Security-Policy: sandbox` and `nosniff`. - The `/api/ws` handshake is cookie-authenticated but validated no Origin, and `csrf()` does not cover a GET - any site could open a socket as a visiting user. Added an origin check. - Auth cookies stated no `SameSite`; set to `Lax` explicitly. - The reCAPTCHA token was interpolated unencoded into the verification URL alongside the secret key; both now travel in a form-encoded body, and a missing secret key fails closed. - Swagger UI and the OpenAPI document were served unconditionally, publishing the whole attack surface. Off in production unless asked for. - Removed `POST /users/test`, an unauthenticated debug route that wrote a log row per call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014QtoqPREn6SsE5M4oFnQH5 --- apps/api/src/index.ts | 15 +- packages/vitnode/src/api/config.ts | 40 ++-- packages/vitnode/src/api/lib/auth-cookie.ts | 8 + .../vitnode/src/api/lib/client-ip.test.ts | 176 ++++++++++++++++++ packages/vitnode/src/api/lib/client-ip.ts | 159 ++++++++++++++++ .../src/api/middlewares/captcha.middleware.ts | 31 ++- .../middlewares/cron-auth.middleware.test.ts | 124 ++++++++++++ .../api/middlewares/cron-auth.middleware.ts | 59 +++++- .../src/api/middlewares/global.middleware.ts | 44 ++--- .../middlewares/storage-static.middleware.ts | 33 ++++ .../websocket-origin.middleware.test.ts | 91 +++++++++ .../websocket-origin.middleware.ts | 73 ++++++++ .../vitnode/src/api/models/password.test.ts | 142 ++++++++++++++ packages/vitnode/src/api/models/password.ts | 88 ++++++--- .../vitnode/src/api/models/session-revoke.ts | 62 ++++++ packages/vitnode/src/api/models/storage.ts | 1 + .../api/models/user/sign-in-with-passwords.ts | 13 +- .../admin/advanced/queue/routes/get.route.ts | 26 ++- .../admin/routes/notifications.route.ts | 7 + .../lib/assert-edit-user-permission.test.ts | 127 +++++++++++++ .../users/lib/assert-edit-user-permission.ts | 93 +++++++-- .../admin/users/routes/update.route.ts | 12 +- .../users/routes/change-password.route.ts | 17 +- .../users/routes/reset-passowrd.route.ts | 11 +- .../api/modules/users/routes/test.route.ts | 36 ---- .../src/api/modules/users/users.module.ts | 2 - packages/vitnode/src/lib/api/upload.test.ts | 99 ++++++++++ packages/vitnode/src/lib/api/upload.ts | 129 ++++++++++++- packages/vitnode/src/lib/config.ts | 19 ++ packages/vitnode/src/vitnode.config.ts | 33 ++++ 30 files changed, 1628 insertions(+), 142 deletions(-) create mode 100644 packages/vitnode/src/api/lib/client-ip.test.ts create mode 100644 packages/vitnode/src/api/lib/client-ip.ts create mode 100644 packages/vitnode/src/api/middlewares/cron-auth.middleware.test.ts create mode 100644 packages/vitnode/src/api/middlewares/storage-static.middleware.ts create mode 100644 packages/vitnode/src/api/middlewares/websocket-origin.middleware.test.ts create mode 100644 packages/vitnode/src/api/middlewares/websocket-origin.middleware.ts create mode 100644 packages/vitnode/src/api/models/password.test.ts create mode 100644 packages/vitnode/src/api/models/session-revoke.ts create mode 100644 packages/vitnode/src/api/modules/admin/users/lib/assert-edit-user-permission.test.ts delete mode 100644 packages/vitnode/src/api/modules/users/routes/test.route.ts diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 5d7ad7916..1b69f74ed 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -4,6 +4,8 @@ import { serve, upgradeWebSocket } from "@hono/node-server"; import { serveStatic } from "@hono/node-server/serve-static"; import { OpenAPIHono } from "@hono/zod-openapi"; import { VitNodeAPI } from "@vitnode/core/api/config"; +import { storageStaticHeadersMiddleware } from "@vitnode/core/api/middlewares/storage-static.middleware"; +import { websocketOriginMiddleware } from "@vitnode/core/api/middlewares/websocket-origin.middleware"; import { handleVitNodeWebSocket } from "@vitnode/core/ws/handle"; import { mkdirSync } from "node:fs"; import { WebSocketServer } from "ws"; @@ -24,6 +26,10 @@ if (staticStorage) { mkdirSync(staticStorage.root, { recursive: true }); app.get( staticStorage.mountPath, + // Stored files are served from this origin - the one the session cookie + // belongs to - so anything the browser would treat as a document has to be + // stopped from executing in it. See the middleware. + storageStaticHeadersMiddleware(), serveStatic({ root: staticStorage.root, rewriteRequestPath: path => @@ -48,7 +54,14 @@ VitNodeAPI({ const wss = new WebSocketServer({ noServer: true }); -app.get("/ws", upgradeWebSocket(handleVitNodeWebSocket())); +// The handshake is a cookie-authenticated GET, which Hono's `csrf()` does not +// cover and the same-origin policy does not apply to. Without this any site +// could open a socket as a visiting user - see the middleware. +app.get( + "/ws", + websocketOriginMiddleware({ origin: [webOrigin] }), + upgradeWebSocket(handleVitNodeWebSocket()), +); serve( { diff --git a/packages/vitnode/src/api/config.ts b/packages/vitnode/src/api/config.ts index b3c54d4d0..8b9a3ead4 100644 --- a/packages/vitnode/src/api/config.ts +++ b/packages/vitnode/src/api/config.ts @@ -9,10 +9,12 @@ import { HTTPException } from "hono/http-exception"; import type { VitNodeApiConfig } from "@/vitnode.config"; import { createCacheClient } from "@/api/lib/cache-client"; +import { clientIpMiddleware } from "@/api/lib/client-ip"; import { collectCronJobs } from "@/api/lib/cron"; import { describeError } from "@/api/lib/error-details"; import { newBuildPluginApiCore } from "@/api/plugin"; import { CONFIG_PLUGIN } from "@/config"; +import { CONFIG } from "@/lib/config"; import { initRealtimePubSub } from "@/ws/registry"; import { @@ -62,23 +64,38 @@ export function VitNodeAPI({ const plugins = [newBuildPluginApiCore, ...vitNodeApiConfig.plugins]; - app.doc("/swagger/doc", { - openapi: "3.0.0", - info: { - version: CONFIG_PLUGIN.version, - title: "VitNode API", - }, - tags: plugins.flatMap( - plugin => plugin.openApiTags?.map(name => ({ name })) ?? [], - ), - }); + // The generated document names every route, parameter and response shape in + // the install, including the admin tree - a map of the attack surface, handed + // out unauthenticated. Published in development, where it is the point, and + // in production only when an install asks for it via `docs: { enabled: true }`. + const docsEnabled = vitNodeApiConfig.docs?.enabled ?? CONFIG.node_development; + + if (docsEnabled) { + app.doc("/swagger/doc", { + openapi: "3.0.0", + info: { + version: CONFIG_PLUGIN.version, + title: "VitNode API", + }, + tags: plugins.flatMap( + plugin => plugin.openApiTags?.map(name => ({ name })) ?? [], + ), + }); + } + app.use(cors(corsOptions)); app.use(csrf(csrfOptions)); + // Before the rate limiter, which keys its buckets on `ipAddress`. Resolving it + // later - as `globalMiddleware` used to - left every request in the + // deployment sharing one bucket named after `undefined`. + app.use("*", clientIpMiddleware(vitNodeApiConfig.trustProxy)); app.use( "*", rateLimiterMiddleware(vitNodeApiConfig.rateLimiter, redisClient), ); - app.get("/swagger", swaggerUI({ url: "/api/swagger/doc" })); + if (docsEnabled) { + app.get("/swagger", swaggerUI({ url: "/api/swagger/doc" })); + } app.use( "*", globalMiddleware({ @@ -96,6 +113,7 @@ export function VitNodeAPI({ storage: vitNodeApiConfig.storage, plugins, cacheClient: redisClient, + trustProxy: vitNodeApiConfig.trustProxy, }), ); app.use(async (c, next) => { diff --git a/packages/vitnode/src/api/lib/auth-cookie.ts b/packages/vitnode/src/api/lib/auth-cookie.ts index f426a8a5e..f5b4af317 100644 --- a/packages/vitnode/src/api/lib/auth-cookie.ts +++ b/packages/vitnode/src/api/lib/auth-cookie.ts @@ -33,6 +33,14 @@ const authCookieOptions = (c: Context): CookieOptions => { domain: cookieDomain, httpOnly: true, path: "/", + // Stated rather than left to the browser. Chrome and Firefox default an + // omitted `SameSite` to `Lax`, but that is a default and not a rule: Safari + // and older engines have their own, and a cookie whose cross-site behaviour + // depends on which browser is reading it is one nobody can reason about. + // `Lax` and not `Strict` because the SSO round trip lands here as a + // top-level cross-site GET - `Strict` would drop the state cookie on the way + // back from the provider and break every social sign-in. + sameSite: "Lax", secure: cookieSecure, }; }; diff --git a/packages/vitnode/src/api/lib/client-ip.test.ts b/packages/vitnode/src/api/lib/client-ip.test.ts new file mode 100644 index 000000000..8f3c1b732 --- /dev/null +++ b/packages/vitnode/src/api/lib/client-ip.test.ts @@ -0,0 +1,176 @@ +import { Hono } from "hono"; +import { describe, expect, it } from "vitest"; + +import type { TrustProxyConfig } from "./client-ip"; + +import { clientIpMiddleware } from "./client-ip"; + +interface Env { + Variables: { ipAddress: string }; +} + +/** + * Runs one request through the middleware and reports the address it settled + * on. `socket` stands in for the runtime's connection info, in the shape + * `@hono/node-server` exposes it. + */ +const resolve = async ({ + headers, + socket, + trustProxy, +}: { + headers?: Record; + socket?: string; + trustProxy?: TrustProxyConfig; +}): Promise => { + const app = new Hono(); + app.use("*", clientIpMiddleware(trustProxy)); + app.get("/", c => c.text(c.get("ipAddress"))); + + const res = await app.request( + "/", + { headers }, + socket === undefined + ? undefined + : { incoming: { socket: { remoteAddress: socket } } }, + ); + + return await res.text(); +}; + +describe("clientIpMiddleware", () => { + describe("with no proxy configured", () => { + it("uses the socket address", async () => { + await expect(resolve({ socket: "203.0.113.7" })).resolves.toBe( + "203.0.113.7", + ); + }); + + it("ignores a forwarded header entirely", async () => { + // The regression this guards: the old resolver walked sixteen + // client-settable headers and took the first one present, so any caller + // could name themselves and get a fresh rate-limit bucket per request. + await expect( + resolve({ + socket: "203.0.113.7", + headers: { "x-forwarded-for": "9.9.9.9" }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it.each([ + "x-real-ip", + "cf-connecting-ip", + "true-client-ip", + "client-ip", + "forwarded", + ])("ignores %s", async header => { + await expect( + resolve({ socket: "203.0.113.7", headers: { [header]: "9.9.9.9" } }), + ).resolves.toBe("203.0.113.7"); + }); + + it("falls back to localhost when the runtime exposes no socket", async () => { + await expect(resolve({})).resolves.toBe("127.0.0.1"); + }); + }); + + describe("behind one proxy", () => { + it("reads the address the proxy observed", async () => { + await expect( + resolve({ + trustProxy: true, + socket: "10.0.0.1", + headers: { "x-forwarded-for": "203.0.113.7" }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("steps over an address the client forged", async () => { + // The client sent `9.9.9.9`; the proxy appended what it actually saw. One + // hop means one entry from the right, which is the proxy's word and not + // the client's. + await expect( + resolve({ + trustProxy: true, + socket: "10.0.0.1", + headers: { "x-forwarded-for": "9.9.9.9, 203.0.113.7" }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("survives a forged chain of any length", async () => { + const forged = Array.from({ length: 20 }, (_, i) => `9.9.9.${i}`).join( + ", ", + ); + + await expect( + resolve({ + trustProxy: 1, + socket: "10.0.0.1", + headers: { "x-forwarded-for": `${forged}, 203.0.113.7` }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("falls back to the socket when the proxy sent no header", async () => { + await expect( + resolve({ trustProxy: true, socket: "10.0.0.1" }), + ).resolves.toBe("10.0.0.1"); + }); + }); + + describe("behind two proxies", () => { + it("reads past both of them", async () => { + await expect( + resolve({ + trustProxy: 2, + socket: "10.0.0.1", + headers: { "x-forwarded-for": "203.0.113.7, 198.51.100.4" }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("steps over a forgery, still", async () => { + await expect( + resolve({ + trustProxy: 2, + socket: "10.0.0.1", + headers: { + "x-forwarded-for": "9.9.9.9, 203.0.113.7, 198.51.100.4", + }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("takes the leftmost entry when the chain is shorter than the hop count", async () => { + await expect( + resolve({ + trustProxy: 5, + socket: "10.0.0.1", + headers: { "x-forwarded-for": "203.0.113.7" }, + }), + ).resolves.toBe("203.0.113.7"); + }); + }); + + it("trims whitespace around chain entries", async () => { + await expect( + resolve({ + trustProxy: true, + socket: "10.0.0.1", + headers: { "x-forwarded-for": " 203.0.113.7 " }, + }), + ).resolves.toBe("203.0.113.7"); + }); + + it("ignores empty entries in the chain", async () => { + await expect( + resolve({ + trustProxy: true, + socket: "10.0.0.1", + headers: { "x-forwarded-for": "203.0.113.7, , " }, + }), + ).resolves.toBe("203.0.113.7"); + }); +}); diff --git a/packages/vitnode/src/api/lib/client-ip.ts b/packages/vitnode/src/api/lib/client-ip.ts new file mode 100644 index 000000000..de78418f5 --- /dev/null +++ b/packages/vitnode/src/api/lib/client-ip.ts @@ -0,0 +1,159 @@ +import type { Context } from "hono"; + +/** + * How many reverse proxies sit between the internet and this API, or `false` + * when it is reached directly. + * + * `true` is shorthand for one hop, which is the ordinary "nginx / Traefik / + * platform edge in front of the app" deployment. + */ +export type TrustProxyConfig = boolean | number; + +/** + * Used when neither the socket nor a trusted header can name the peer - a test + * harness, or a runtime that exposes no connection info at all. + */ +const UNKNOWN_CLIENT_IP = "127.0.0.1"; + +/** + * The peer address of the TCP connection, read from whichever runtime is + * hosting the app. + * + * This is the only address a client cannot choose for itself, which is what + * makes it the base case: every header below is something the *sender* wrote. + * Each runtime exposes it somewhere different and none of them are typed on + * `Context`, so the shapes are probed rather than imported - importing + * `@hono/node-server/conninfo` here would tie this package to Node and break + * the Bun and edge entry points. + */ +const socketAddress = (c: Context): string | undefined => { + const env: unknown = c.env; + if (typeof env !== "object" || env === null) return undefined; + + // Node (`@hono/node-server`): the raw `IncomingMessage`. + const incoming = (env as { incoming?: unknown }).incoming; + if (typeof incoming === "object" && incoming !== null) { + const socket = (incoming as { socket?: unknown }).socket; + if (typeof socket === "object" && socket !== null) { + const address = (socket as { remoteAddress?: unknown }).remoteAddress; + if (typeof address === "string" && address.length > 0) return address; + } + } + + // Bun: `server.requestIP(request)`. + const server = (env as { server?: unknown }).server; + if (typeof server === "object" && server !== null) { + const requestIP = (server as { requestIP?: unknown }).requestIP; + if (typeof requestIP === "function") { + const info: unknown = ( + requestIP as (request: Request) => null | { address?: unknown } + )(c.req.raw); + const address = + typeof info === "object" && info !== null + ? (info as { address?: unknown }).address + : undefined; + if (typeof address === "string" && address.length > 0) return address; + } + } + + // Deno: `Deno.ServeHandlerInfo`. + const remoteAddr = (env as { remoteAddr?: unknown }).remoteAddr; + if (typeof remoteAddr === "object" && remoteAddr !== null) { + const hostname = (remoteAddr as { hostname?: unknown }).hostname; + if (typeof hostname === "string" && hostname.length > 0) return hostname; + } + + return undefined; +}; + +/** + * The address of the client that made this request. + * + * ## Why the headers are not simply read + * + * `X-Forwarded-For` and its sixteen cousins are request headers, so anybody can + * send any of them. Taking the first one present - which is what this used to do + * - hands every caller a free hand in choosing their own identity, and the two + * things that identity is *for* are the rate limiter's bucket key and the audit + * trail on a password-reset row. A limiter keyed on a value the attacker picks + * is not a limiter, and a reset email that reports "requested from 1.2.3.4" + * because the requester said so is worse than one that reports nothing. + * + * So the socket address is the default, and a forwarded header is read **only** + * when the install says it is behind a proxy, via `security.trustProxy`. + * + * ## Why it counts from the right + * + * A proxy *appends* the address it saw to `X-Forwarded-For`, so the chain reads + * oldest-first and the rightmost entry is the one written by the proxy closest + * to this server - the only entry in the list that a trusted machine vouched + * for. Anything the client sent arrives to its left, still in the header, + * indistinguishable from a real hop by content alone. + * + * Counting `hops` from the right is what makes that survivable. Behind one + * proxy, a client sending `X-Forwarded-For: 9.9.9.9` produces + * `9.9.9.9, ` once the proxy appends, and the entry one from the + * right is the real client - the forgery is still in the header and is + * deliberately stepped over. Reading the leftmost entry instead would return + * `9.9.9.9`, which is the bug this shape exists to prevent. + */ +export const resolveClientIp = ( + c: Context, + trustProxy: TrustProxyConfig | undefined, +): string => { + const socket = socketAddress(c); + + if (!trustProxy) return socket ?? UNKNOWN_CLIENT_IP; + + const hops = trustProxy === true ? 1 : Math.max(1, Math.trunc(trustProxy)); + const chain = (c.req.header("x-forwarded-for") ?? "") + .split(",") + .map(entry => entry.trim()) + .filter(entry => entry.length > 0); + + if (chain.length === 0) return socket ?? UNKNOWN_CLIENT_IP; + + // A chain shorter than the configured hop count means a proxy did not append + // what it was expected to. The leftmost entry is then the least-worst answer, + // and it is still bounded by however many proxies really did write to it. + const index = Math.max(0, chain.length - hops); + + return chain[index] ?? socket ?? UNKNOWN_CLIENT_IP; +}; + +/** + * Sets `ipAddress` for the rest of the request. + * + * Registered **before** the rate limiter rather than inside `globalMiddleware`, + * because the limiter reads `ipAddress` to build its bucket key and Hono runs + * middleware in registration order: resolving it later left the limiter keying + * every request in the deployment on `undefined`, i.e. one shared bucket for the + * whole site. + */ +export const clientIpMiddleware = ( + trustProxy: TrustProxyConfig | undefined, +) => { + let warned = false; + + return async (c: Context, next: () => Promise) => { + const ipAddress = resolveClientIp(c, trustProxy); + + // Some hosts hand Hono a bare `Request` with no connection info at all - the + // TanStack Start mount calls `app.fetch(request)` directly, and there is no + // socket behind it. Without a socket *and* without `trustProxy`, every + // caller resolves to the same fallback, which quietly turns the rate limiter + // back into one shared bucket for the whole site. That is worth saying out + // loud once, because nothing else about it is visible. + if (!warned && !trustProxy && !socketAddress(c)) { + warned = true; + // eslint-disable-next-line no-console + console.warn( + `\x1b[34m[VitNode]\x1b[0m \x1b[33mCannot see the client's address:\x1b[0m this runtime exposes no connection info, so every request is being rate-limited as ${UNKNOWN_CLIENT_IP}. Set \`trustProxy\` in the API config to the number of proxies in front of this app so \`X-Forwarded-For\` is read instead.`, + ); + } + + c.set("ipAddress", ipAddress); + + await next(); + }; +}; diff --git a/packages/vitnode/src/api/middlewares/captcha.middleware.ts b/packages/vitnode/src/api/middlewares/captcha.middleware.ts index 09628d4c5..40a0cc947 100644 --- a/packages/vitnode/src/api/middlewares/captcha.middleware.ts +++ b/packages/vitnode/src/api/middlewares/captcha.middleware.ts @@ -13,13 +13,22 @@ const getResFromReCaptcha = async ({ token: string; userIp: string; }): Promise<{ "error-codes"?: string[]; score: number; success: boolean }> => { + // An install that configured a captcha but no secret key cannot verify + // anything. Said here rather than left to the provider to reject a malformed + // request: the answer is the same either way, but only one of them is a + // decision this code made on purpose. + const { secretKey } = captchaConfig; + if (!secretKey) { + return { success: false, score: 0, "error-codes": ["missing-secret-key"] }; + } + if (captchaConfig.type === "cloudflare_turnstile") { const res = await fetch( "https://challenges.cloudflare.com/turnstile/v0/siteverify", { method: "POST", body: JSON.stringify({ - secret: captchaConfig.secretKey, + secret: secretKey, response: token, remoteip: userIp, }), @@ -41,12 +50,22 @@ const getResFromReCaptcha = async ({ }; } if (captchaConfig.type === "recaptcha_v3") { - const res = await fetch( - `https://www.google.com/recaptcha/api/siteverify?secret=${captchaConfig.secretKey}&response=${token}&remoteip=${userIp}`, - { - method: "POST", + // Form-encoded body rather than a query string. Interpolating the + // client-supplied token straight into the URL let it carry `&` and add + // parameters of its own to the request - and it put the site's secret key in + // a URL, which is the part of a request that ends up in proxy logs and + // error reports. + const res = await fetch("https://www.google.com/recaptcha/api/siteverify", { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", }, - ); + body: new URLSearchParams({ + secret: secretKey, + response: token, + remoteip: userIp, + }), + }); const data: { "error-codes"?: string[]; diff --git a/packages/vitnode/src/api/middlewares/cron-auth.middleware.test.ts b/packages/vitnode/src/api/middlewares/cron-auth.middleware.test.ts new file mode 100644 index 000000000..f363ccf42 --- /dev/null +++ b/packages/vitnode/src/api/middlewares/cron-auth.middleware.test.ts @@ -0,0 +1,124 @@ +import { Hono } from "hono"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + INSECURE_CRON_SECRETS, + INSECURE_DEFAULT_CRON_SECRET, +} from "@/lib/config"; + +import { cronAuthMiddleware } from "./cron-auth.middleware"; + +interface Env { + Variables: { core: { cronSecret?: string } }; +} + +const buildApp = (cronSecret: string | undefined) => { + const app = new Hono(); + app.use("*", async (c, next) => { + c.set("core", { cronSecret }); + + return next(); + }); + app.use("*", cronAuthMiddleware()); + app.post("/", c => c.text("ran")); + + return app; +}; + +const post = async ( + cronSecret: string | undefined, + authorization?: string, +): Promise => + await buildApp(cronSecret).request("/", { + method: "POST", + headers: authorization === undefined ? {} : { authorization }, + }); + +describe("cronAuthMiddleware", () => { + beforeEach(() => { + vi.stubEnv("NODE_ENV", "production"); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("runs the job for the configured secret", async () => { + const res = await post("s3cret-value", "Bearer s3cret-value"); + + expect(res.status).toBe(200); + }); + + it("refuses a wrong secret", async () => { + const res = await post("s3cret-value", "Bearer wrong"); + + expect(res.status).toBe(403); + }); + + it("refuses a missing authorization header", async () => { + const res = await post("s3cret-value"); + + expect(res.status).toBe(403); + }); + + it("refuses when no secret is configured at all", async () => { + const res = await post(undefined, "Bearer anything"); + + expect(res.status).toBe(403); + }); + + describe("the built-in default secret", () => { + it("is refused in production", async () => { + // The regression this guards: `CONFIG.cronJobSecret` falls back to a + // constant published in this repository, so an install that never set + // `CRON_SECRET` would run every registered cron job for anyone who read + // the source. + const res = await post( + INSECURE_DEFAULT_CRON_SECRET, + `Bearer ${INSECURE_DEFAULT_CRON_SECRET}`, + ); + + expect(res.status).toBe(403); + expect(await res.text()).toContain("CRON_SECRET"); + }); + + it.each([...INSECURE_CRON_SECRETS])( + "refuses the published placeholder %s in production", + async secret => { + // The `.env.example` the scaffolder ships carries its own placeholder, + // so recognising only the code fallback left every install that copied + // that file and never edited the line just as open. + const res = await post(secret, `Bearer ${secret}`); + + expect(res.status).toBe(403); + }, + ); + + it("still works in development", async () => { + vi.stubEnv("NODE_ENV", "development"); + const res = await post( + INSECURE_DEFAULT_CRON_SECRET, + `Bearer ${INSECURE_DEFAULT_CRON_SECRET}`, + ); + + expect(res.status).toBe(200); + }); + }); + + describe("header parsing", () => { + it("does not accept `Bearer` appearing mid-header", async () => { + // `authHeader.replace("Bearer ", "")` used to strip the first occurrence + // wherever it sat, so this parsed as the secret. + const res = await post("s3cret-value", "Basic Bearer s3cret-value"); + + expect(res.status).toBe(403); + }); + + it("does not mangle a secret containing the scheme name", async () => { + const secret = "a Bearer b"; + const res = await post(secret, `Bearer ${secret}`); + + expect(res.status).toBe(200); + }); + }); +}); diff --git a/packages/vitnode/src/api/middlewares/cron-auth.middleware.ts b/packages/vitnode/src/api/middlewares/cron-auth.middleware.ts index 556f4b705..29c05d920 100644 --- a/packages/vitnode/src/api/middlewares/cron-auth.middleware.ts +++ b/packages/vitnode/src/api/middlewares/cron-auth.middleware.ts @@ -1,6 +1,44 @@ import type { Context, Next } from "hono"; import { HTTPException } from "hono/http-exception"; +import { timingSafeEqual } from "node:crypto"; + +import { CONFIG, INSECURE_CRON_SECRETS } from "@/lib/config"; + +/** + * Constant-time comparison of two secrets. + * + * `timingSafeEqual` throws on a length mismatch, so the lengths are compared + * first - and then both branches still run a comparison, because returning + * early on a length difference is itself a signal about the secret. + */ +const secretsMatch = (provided: string, expected: string): boolean => { + const a = Buffer.from(provided, "utf8"); + const b = Buffer.from(expected, "utf8"); + + if (a.length !== b.length) { + timingSafeEqual(b, b); + + return false; + } + + return timingSafeEqual(a, b); +}; + +/** + * `Bearer `, or nothing. + * + * Matched as a *prefix* rather than stripped with `replace`, which removed the + * first `"Bearer "` found anywhere in the header - so `"x Bearer y"` parsed as a + * credential, and a secret that happened to contain the word lost part of + * itself. + */ +const bearerToken = (header: string | undefined): string | undefined => { + if (!header) return undefined; + const match = /^Bearer (.+)$/.exec(header); + + return match?.[1]; +}; export const cronAuthMiddleware = () => { return async (c: Context, next: Next) => { @@ -9,10 +47,25 @@ export const cronAuthMiddleware = () => { throw new HTTPException(403, { message: "Cron access not configured" }); } - const authHeader = c.req.header("authorization"); - const providedSecret = authHeader?.replace("Bearer ", ""); + // `CONFIG.cronJobSecret` falls back to a published constant so that a fresh + // checkout runs its cron jobs without configuration. Outside development + // that fallback is not a weak secret, it is *no* secret: the value is in the + // repository, so anyone could post to this endpoint and run every registered + // job. The admin panel flags it, but a warning nobody reads is not a control, + // so production refuses the request outright. + if ( + INSECURE_CRON_SECRETS.includes(cronSecret) && + !CONFIG.node_development + ) { + throw new HTTPException(403, { + message: + "Cron access is disabled because CRON_SECRET is still the built-in default. Set CRON_SECRET to a random value.", + }); + } + + const providedSecret = bearerToken(c.req.header("authorization")); - if (providedSecret !== cronSecret) { + if (!providedSecret || !secretsMatch(providedSecret, cronSecret)) { throw new HTTPException(403, { message: "Invalid cron authorization" }); } diff --git a/packages/vitnode/src/api/middlewares/global.middleware.ts b/packages/vitnode/src/api/middlewares/global.middleware.ts index cdf2f15d6..157fcd3e3 100644 --- a/packages/vitnode/src/api/middlewares/global.middleware.ts +++ b/packages/vitnode/src/api/middlewares/global.middleware.ts @@ -34,6 +34,7 @@ import { collectLocaleCodes } from "@/lib/i18n/load-messages"; import { buildApiMessagesSources } from "@/lib/i18n/sources"; import { realtime } from "@/ws/registry"; +import type { TrustProxyConfig } from "../lib/client-ip"; import type { BuildCronReturn } from "../lib/cron"; import type { EventListenerConfig } from "../lib/events"; import type { PermissionStaffCatalogEntry } from "../lib/permission-staff"; @@ -46,6 +47,7 @@ import type { } from "../models/search"; import type { SSOApiPlugin } from "../models/sso"; +import { resolveClientIp } from "../lib/client-ip"; import { collectCronJobs } from "../lib/cron"; import { loggerMiddleware, @@ -182,6 +184,7 @@ export const globalMiddleware = ({ search, storage, cacheClient, + trustProxy, }: Pick< VitNodeApiConfig, | "ai" @@ -197,7 +200,10 @@ export const globalMiddleware = ({ | "search" | "storage" > & - Pick & { cacheClient: CacheClient | null }) => { + Pick & { + cacheClient: CacheClient | null; + trustProxy: TrustProxyConfig | undefined; + }) => { const pluginsMetadata = plugins.map(plugin => ({ id: plugin.pluginId, })); @@ -324,38 +330,14 @@ export const globalMiddleware = ({ }), ); - const ipHeaderKeys = [ - "x-forwarded-for", - "x-real-ip", - "cf-connecting-ip", - "x-client-ip", - "x-forwarded", - "x-cluster-client-ip", - "forwarded-for", - "forwarded", - "via", - "remote-addr", - "client-ip", - "ip", - "x-ip", - "true-client-ip", - "fastly-client-ip", - "x-fastly-client-ip", - ]; - return async (c: Context, next: Next) => { - let ipAddress: string | undefined; - - for (const key of ipHeaderKeys) { - ipAddress = c.req.header(key); - if (ipAddress) break; - - ipAddress = c.req.raw.headers.get(key) ?? undefined; - if (ipAddress) break; + // Normally already resolved by `clientIpMiddleware`, which `VitNodeAPI` + // registers ahead of the rate limiter. Repeated here only so that composing + // this middleware by hand still yields an `ipAddress`, rather than leaving + // an `undefined` one to be used silently as a rate-limit key. + if (!c.get("ipAddress")) { + c.set("ipAddress", resolveClientIp(c, trustProxy)); } - - // Fallback to localhost if nothing found - c.set("ipAddress", ipAddress ?? "127.0.0.1"); c.set("db", dbProvider); c.set("ai", new AIModel(c)); c.set("cache", new CacheModel(cacheClient, c)); diff --git a/packages/vitnode/src/api/middlewares/storage-static.middleware.ts b/packages/vitnode/src/api/middlewares/storage-static.middleware.ts new file mode 100644 index 000000000..73530f871 --- /dev/null +++ b/packages/vitnode/src/api/middlewares/storage-static.middleware.ts @@ -0,0 +1,33 @@ +import type { Context, Next } from "hono"; + +/** + * Headers for the route that serves stored uploads off disk. + * + * Uploads are served from the API's own origin, which is the origin the session + * cookie belongs to. That makes any stored file the browser is willing to treat + * as a *document* - HTML, an SVG carrying a `