From d4d166df7ce0975852e69205a43d460106f51203 Mon Sep 17 00:00:00 2001 From: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Date: Sat, 22 Aug 2026 19:40:53 +0200 Subject: [PATCH] fix(server): validate request origins --- js/ip_access_control.js | 83 +++++++++++-------- js/server.js | 4 - .../unit/functions/ip_access_control_spec.js | 50 ++++++++++- 3 files changed, 96 insertions(+), 41 deletions(-) diff --git a/js/ip_access_control.js b/js/ip_access_control.js index 3357d37873..e53b186e49 100644 --- a/js/ip_access_control.js +++ b/js/ip_access_control.js @@ -55,54 +55,71 @@ function resolveClientIp (req) { } /** - * Creates an Express middleware for IP whitelisting + * Checks whether a browser Origin matches the host serving the mirror. + * Non-browser clients (Electron clientonly, curl, node_helpers) send no Origin and are allowed. + * @param {object} req - Incoming request object + * @returns {boolean} True if the origin is same-host or absent + */ +function isSameOrigin (req) { + const origin = req.headers?.origin; + if (!origin) return true; + + const host = req.headers?.host; + if (!host) return false; + + try { + return new URL(origin).host === new URL(`http://${host}`).host; + } catch { + return false; + } +} + +/** + * Determines why a request is denied, or null if it is allowed. + * Enforces same-origin first (CSRF protection), then the optional IP whitelist. + * @param {object} req - Incoming Express or Socket.IO request + * @param {string[]} whitelist - Array of allowed IP addresses or CIDR ranges (empty = any IP) + * @returns {string|null} A human-readable denial reason, or null when allowed + */ +function accessDenialReason (req, whitelist) { + // Strip control characters from the attacker-controlled Origin header before logging it + if (!isSameOrigin(req)) return `Origin ${String(req.headers?.origin).replace(/[\r\n]/g, "")} is not allowed`; + + if (Array.isArray(whitelist) && whitelist.length > 0) { + const clientIp = resolveClientIp(req); + if (!isAllowed(clientIp, whitelist)) return `IP ${clientIp} is not allowed`; + } + + return null; +} + +/** + * Creates an Express middleware enforcing same-origin and the IP whitelist. * @param {string[]} whitelist - Array of allowed IP addresses or CIDR ranges * @returns {import("express").RequestHandler} Express middleware function */ function ipAccessControl (whitelist) { - // Empty whitelist means allow all - if (!Array.isArray(whitelist) || whitelist.length === 0) { - return function (req, res, next) { - res.header("Access-Control-Allow-Origin", "*"); - next(); - }; - } - return function (req, res, next) { - const clientIp = resolveClientIp(req); + const reason = accessDenialReason(req, whitelist); + if (!reason) return next(); - if (isAllowed(clientIp, whitelist)) { - res.header("Access-Control-Allow-Origin", "*"); - next(); - } else { - Log.warn(`IP ${clientIp} is not allowed to access the mirror`); - res.status(403).send("This device is not allowed to access your mirror.
Please check your config.js or config.js.sample to change this."); - } + Log.warn(`${reason} to access the mirror`); + res.status(403).send("This device is not allowed to access your mirror.
Please check your config.js or config.js.sample to change this."); }; } /** - * Creates a Socket.IO `allowRequest` handler that enforces the same IP whitelist as the HTTP middleware. - * This closes the gap where Socket.IO handshakes bypassed the Express-only `ipAccessControl` middleware. + * Creates a Socket.IO `allowRequest` handler enforcing the same rules as the HTTP middleware. * @param {string[]} whitelist - Array of allowed IP addresses or CIDR ranges * @returns {(req: object, callback: (err: string | null, success: boolean) => void) => void} Socket.IO allowRequest handler */ function socketIpAccessControl (whitelist) { - // Empty whitelist means allow all - if (!Array.isArray(whitelist) || whitelist.length === 0) { - return function (req, callback) { - callback(null, true); // allow the connection - }; - } - return function (req, callback) { - const clientIp = resolveClientIp(req); - if (isAllowed(clientIp, whitelist)) { - callback(null, true); // allow the connection - } else { - Log.warn(`IP ${clientIp} is not allowed to connect to the mirror socket`); - callback("This device is not allowed to access your mirror.", false); - } + const reason = accessDenialReason(req, whitelist); + if (!reason) return callback(null, true); + + Log.warn(`${reason} to connect to the mirror socket`); + callback("This device is not allowed to access your mirror.", false); }; } diff --git a/js/server.js b/js/server.js index 21d9b5903a..7b3c428cc2 100644 --- a/js/server.js +++ b/js/server.js @@ -42,10 +42,6 @@ function Server (configObj) { } const io = socketio(server, { allowRequest: socketIpAccessControl(config.ipWhitelist), - cors: { - origin: /.*$/, - credentials: true - }, allowEIO3: true, pingInterval: 120000, // server → client ping every 2 mins pingTimeout: 120000 // wait up to 2 mins for client pong diff --git a/tests/unit/functions/ip_access_control_spec.js b/tests/unit/functions/ip_access_control_spec.js index fa6e1a62b8..2a129fba71 100644 --- a/tests/unit/functions/ip_access_control_spec.js +++ b/tests/unit/functions/ip_access_control_spec.js @@ -4,11 +4,10 @@ import { ipAccessControl, socketIpAccessControl } from "../../../js/ip_access_co /** * Creates a minimal Express-like response mock used by the middleware tests. - * @returns {{ header: ReturnType, status: ReturnType, send: ReturnType }} Mock response object. + * @returns {{ status: ReturnType, send: ReturnType }} Mock response object. */ function createResponseMock () { return { - header: vi.fn(), status: vi.fn(function () { return this; }), @@ -47,6 +46,36 @@ describe("ip_access_control", () => { expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(403); }); + + it("rejects cross-origin HTTP requests even when the IP matches", () => { + const middleware = ipAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "203.0.113.10" }, + headers: { host: "localhost:8080", origin: "https://evil.example" } + }; + const res = createResponseMock(); + const next = vi.fn(); + + middleware(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it("rejects cross-origin HTTP requests even with an empty whitelist", () => { + const middleware = ipAccessControl([]); + const req = { + socket: { remoteAddress: "198.51.100.7" }, + headers: { host: "localhost:8080", origin: "https://evil.example" } + }; + const res = createResponseMock(); + const next = vi.fn(); + + middleware(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); }); describe("socketIpAccessControl", () => { @@ -54,7 +83,7 @@ describe("ip_access_control", () => { const allowRequest = socketIpAccessControl(["203.0.113.10"]); const req = { socket: { remoteAddress: "::1" }, - headers: { "x-forwarded-for": "203.0.113.10, 10.0.0.2" } + headers: { host: "localhost:8080", "x-forwarded-for": "203.0.113.10, 10.0.0.2", origin: "http://localhost:8080" } }; const callback = vi.fn(); @@ -67,7 +96,20 @@ describe("ip_access_control", () => { const allowRequest = socketIpAccessControl(["203.0.113.10"]); const req = { socket: { remoteAddress: "198.51.100.7" }, - headers: { "x-forwarded-for": "203.0.113.10" } + headers: { host: "localhost:8080", "x-forwarded-for": "203.0.113.10", origin: "http://localhost:8080" } + }; + const callback = vi.fn(); + + allowRequest(req, callback); + + expect(callback).toHaveBeenCalledWith("This device is not allowed to access your mirror.", false); + }); + + it("rejects cross-origin socket handshakes even when the IP matches", () => { + const allowRequest = socketIpAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "203.0.113.10" }, + headers: { host: "localhost:8080", origin: "https://evil.example" } }; const callback = vi.fn();