From 58c2a5e675a7d367b64d72e1d35680d202ff5c9f Mon Sep 17 00:00:00 2001 From: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Date: Mon, 1 Jun 2026 17:26:16 +0200 Subject: [PATCH] fix(server): enforce ipWhitelist for Socket.IO too (#4169) ipWhitelist was only applied to HTTP routes, so Socket.IO module namespaces could still be reached from disallowed clients. This adds the same whitelist check to Socket.IO handshakes (allowRequest), and reuses the same client IP resolution for both HTTP and Socket.IO (forwarded IP is only trusted for loopback peers). Also adds tests for handshake allow/deny and forwarded-header behavior. Fixes: GHSA-w26r-fwg8-rcp3 --- js/ip_access_control.js | 52 +++++++++++- js/server.js | 3 +- tests/e2e/ipWhitelist_spec.js | 12 +++ .../unit/functions/ip_access_control_spec.js | 79 +++++++++++++++++++ 4 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 tests/unit/functions/ip_access_control_spec.js diff --git a/js/ip_access_control.js b/js/ip_access_control.js index 0fef1a4e..3357d378 100644 --- a/js/ip_access_control.js +++ b/js/ip_access_control.js @@ -33,6 +33,27 @@ function isAllowed (clientIp, whitelist) { } } +/** + * Resolves a client IP for both Express and Socket.IO requests. + * If the direct peer is loopback, trust the first X-Forwarded-For value (local reverse proxy case). + * Otherwise ignore X-Forwarded-For to prevent spoofing. + * @param {object} req - Incoming request object (Express request or Socket.IO handshake request) + * @returns {string} The resolved client IP address + */ +function resolveClientIp (req) { + const directIp = req.socket?.remoteAddress || req.connection?.remoteAddress || req.ip; + const LOOPBACK_WHITELIST = ["127.0.0.1", "::ffff:127.0.0.1", "::1"]; + + if (isAllowed(directIp, LOOPBACK_WHITELIST)) { + const forwardedFor = req.headers?.["x-forwarded-for"]; + if (typeof forwardedFor === "string" && forwardedFor.trim().length > 0) { + return forwardedFor.split(",")[0].trim(); + } + } + + return directIp; +} + /** * Creates an Express middleware for IP whitelisting * @param {string[]} whitelist - Array of allowed IP addresses or CIDR ranges @@ -48,16 +69,41 @@ function ipAccessControl (whitelist) { } return function (req, res, next) { - const clientIp = req.ip || req.socket.remoteAddress; + const clientIp = resolveClientIp(req); if (isAllowed(clientIp, whitelist)) { res.header("Access-Control-Allow-Origin", "*"); next(); } else { - Log.log(`IP ${clientIp} is not allowed to access the mirror`); + 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."); } }; } -module.exports = { ipAccessControl }; +/** + * 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. + * @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); + } + }; +} + +module.exports = { ipAccessControl, socketIpAccessControl }; diff --git a/js/server.js b/js/server.js index f8a51e71..9aa264c0 100644 --- a/js/server.js +++ b/js/server.js @@ -7,7 +7,7 @@ const helmet = require("helmet"); const socketio = require("socket.io"); const Log = require("logger"); -const { ipAccessControl } = require("./ip_access_control"); +const { ipAccessControl, socketIpAccessControl } = require("./ip_access_control"); const vendor = require("./vendor"); @@ -41,6 +41,7 @@ function Server (configObj) { server = http.Server(app); } const io = socketio(server, { + allowRequest: socketIpAccessControl(config.ipWhitelist), cors: { origin: /.*$/, credentials: true diff --git a/tests/e2e/ipWhitelist_spec.js b/tests/e2e/ipWhitelist_spec.js index e83809c6..3a7c9e42 100644 --- a/tests/e2e/ipWhitelist_spec.js +++ b/tests/e2e/ipWhitelist_spec.js @@ -15,6 +15,12 @@ describe("ipWhitelist directive configuration", () => { const res = await fetch(`http://localhost:${port}`); expect(res.status).toBe(403); }); + + it("should also reject Socket.IO handshake with 403 (Forbidden) — not just HTTP routes", async () => { + const port = global.testPort || 8080; + const res = await fetch(`http://localhost:${port}/socket.io/?EIO=4&transport=polling`); + expect(res.status).toBe(403); + }); }); describe("When whitelist is empty (allow all IPs)", () => { @@ -31,5 +37,11 @@ describe("ipWhitelist directive configuration", () => { const res = await fetch(`http://localhost:${port}`); expect(res.status).toBe(200); }); + + it("should also allow Socket.IO handshake with 200 (OK) — not just HTTP routes", async () => { + const port = global.testPort || 8080; + const res = await fetch(`http://localhost:${port}/socket.io/?EIO=4&transport=polling`); + expect(res.status).toBe(200); + }); }); }); diff --git a/tests/unit/functions/ip_access_control_spec.js b/tests/unit/functions/ip_access_control_spec.js new file mode 100644 index 00000000..fa6e1a62 --- /dev/null +++ b/tests/unit/functions/ip_access_control_spec.js @@ -0,0 +1,79 @@ +import { describe, expect, it, vi } from "vitest"; + +import { ipAccessControl, socketIpAccessControl } from "../../../js/ip_access_control"; + +/** + * Creates a minimal Express-like response mock used by the middleware tests. + * @returns {{ header: ReturnType, status: ReturnType, send: ReturnType }} Mock response object. + */ +function createResponseMock () { + return { + header: vi.fn(), + status: vi.fn(function () { + return this; + }), + send: vi.fn() + }; +} + +describe("ip_access_control", () => { + describe("ipAccessControl", () => { + it("trusts first X-Forwarded-For entry when direct peer is loopback", () => { + const middleware = ipAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "127.0.0.1" }, + headers: { "x-forwarded-for": "203.0.113.10, 10.0.0.2" } + }; + const res = createResponseMock(); + const next = vi.fn(); + + middleware(req, res, next); + + expect(next).toHaveBeenCalledOnce(); + expect(res.status).not.toHaveBeenCalled(); + }); + + it("ignores X-Forwarded-For when direct peer is not loopback", () => { + const middleware = ipAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "198.51.100.7" }, + headers: { "x-forwarded-for": "203.0.113.10" } + }; + const res = createResponseMock(); + const next = vi.fn(); + + middleware(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + }); + + describe("socketIpAccessControl", () => { + it("accepts socket handshake using forwarded client IP when direct peer is loopback", () => { + const allowRequest = socketIpAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "::1" }, + headers: { "x-forwarded-for": "203.0.113.10, 10.0.0.2" } + }; + const callback = vi.fn(); + + allowRequest(req, callback); + + expect(callback).toHaveBeenCalledWith(null, true); + }); + + it("rejects socket handshake when only forwarded IP matches whitelist", () => { + const allowRequest = socketIpAccessControl(["203.0.113.10"]); + const req = { + socket: { remoteAddress: "198.51.100.7" }, + headers: { "x-forwarded-for": "203.0.113.10" } + }; + const callback = vi.fn(); + + allowRequest(req, callback); + + expect(callback).toHaveBeenCalledWith("This device is not allowed to access your mirror.", false); + }); + }); +});