From 661a89355f1d355f410feffcfe234b8974a125b7 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 6 Oct 2020 19:44:34 -0400 Subject: [PATCH] socketio: Mimic what Express does to get client IP address This also makes it easier for plugins to get the client IP address. --- src/node/handler/PadMessageHandler.js | 31 ++++++--------------------- src/node/handler/SocketIORouter.js | 10 --------- src/node/hooks/express/socketio.js | 9 ++++++++ src/node/utils/RemoteAddress.js | 1 - src/package.json | 1 + 5 files changed, 17 insertions(+), 35 deletions(-) delete mode 100644 src/node/utils/RemoteAddress.js diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 0794a7b1d..78a8b9858 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -35,7 +35,6 @@ var _ = require('underscore'); var hooks = require("ep_etherpad-lite/static/js/pluginfw/hooks.js"); var channels = require("channels"); var stats = require('../stats'); -var remoteAddress = require("../utils/RemoteAddress").remoteAddress; const assert = require('assert').strict; const nodeify = require("nodeify"); const { RateLimiterMemory } = require('rate-limiter-flexible'); @@ -127,19 +126,11 @@ exports.handleDisconnect = async function(client) // if this connection was already etablished with a handshake, send a disconnect message to the others if (session && session.author) { - // Get the IP address from our persistant object - let ip = remoteAddress[client.id]; - - // Anonymize the IP address if IP logging is disabled - if (settings.disableIPlogging) { - ip = 'ANONYMOUS'; - } - const {session: {user} = {}} = client.client.request; accessLogger.info('[LEAVE]' + ` pad:${session.padId}` + ` socket:${client.id}` + - ` IP:${ip}` + + ` IP:${settings.disableIPlogging ? 'ANONYMOUS' : client.request.ip}` + ` authorID:${session.author}` + ((user && user.username) ? ` username:${user.username}` : '')); @@ -181,11 +172,11 @@ exports.handleMessage = async function(client, message) var env = process.env.NODE_ENV || 'development'; if (env === 'production') { - const clientIPAddress = remoteAddress[client.id]; try { - await rateLimiter.consume(clientIPAddress); // consume 1 point per event from IP - }catch(e){ - console.warn("Rate limited: ", clientIPAddress, " to reduce the amount of rate limiting that happens edit the rateLimit values in settings.json"); + await rateLimiter.consume(client.request.ip); // consume 1 point per event from IP + } catch (e) { + console.warn(`Rate limited: ${client.request.ip} to reduce the amount of rate limiting ` + + 'that happens edit the rateLimit values in settings.json'); stats.meter('rateLimited').mark(); client.json.send({disconnect:"rateLimited"}); return; @@ -239,7 +230,7 @@ exports.handleMessage = async function(client, message) 'Rejecting message from client because the author ID changed mid-session.' + ' Bad or missing token or sessionID?' + ` socket:${client.id}` + - ` IP:${settings.disableIPlogging ? ANONYMOUS : remoteAddress[client.id]}` + + ` IP:${settings.disableIPlogging ? 'ANONYMOUS' : client.request.ip}` + ` originalAuthorID:${thisSession.author}` + ` newAuthorID:${authorID}` + ((user && user.username) ? ` username:${user.username}` : '') + @@ -967,19 +958,11 @@ async function handleClientReady(client, message, authorID) sessionInfo.readonly = padIds.readonly || !webaccess.userCanModify(message.padId, client.client.request); - // Log creation/(re-)entering of a pad - let ip = remoteAddress[client.id]; - - // Anonymize the IP address if IP logging is disabled - if (settings.disableIPlogging) { - ip = 'ANONYMOUS'; - } - const {session: {user} = {}} = client.client.request; accessLogger.info(`[${pad.head > 0 ? 'ENTER' : 'CREATE'}]` + ` pad:${padIds.padId}` + ` socket:${client.id}` + - ` IP:${ip}` + + ` IP:${settings.disableIPlogging ? 'ANONYMOUS' : client.request.ip}` + ` authorID:${authorID}` + ((user && user.username) ? ` username:${user.username}` : '')); diff --git a/src/node/handler/SocketIORouter.js b/src/node/handler/SocketIORouter.js index a52393634..d7712b8ee 100644 --- a/src/node/handler/SocketIORouter.js +++ b/src/node/handler/SocketIORouter.js @@ -23,7 +23,6 @@ var log4js = require('log4js'); var messageLogger = log4js.getLogger("message"); var securityManager = require("../db/SecurityManager"); var readOnlyManager = require("../db/ReadOnlyManager"); -var remoteAddress = require("../utils/RemoteAddress").remoteAddress; var settings = require('../utils/Settings'); /** @@ -56,15 +55,6 @@ exports.setSocketIO = function(_socket) { socket.sockets.on('connection', function(client) { - // Broken: See http://stackoverflow.com/questions/4647348/send-message-to-specific-client-with-socket-io-and-node-js - // Fixed by having a persistant object, ideally this would actually be in the database layer - // TODO move to database layer - if (settings.trustProxy && client.handshake.headers['x-forwarded-for'] !== undefined) { - remoteAddress[client.id] = client.handshake.headers['x-forwarded-for']; - } else { - remoteAddress[client.id] = client.handshake.address; - } - // wrap the original send function to log the messages client._send = client.send; client.send = function(message) { diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index f5964f18a..aae450da3 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -1,4 +1,5 @@ const express = require("../express"); +const proxyaddr = require('proxy-addr'); var settings = require('../../utils/Settings'); var socketio = require('socket.io'); var socketIORouter = require("../../handler/SocketIORouter"); @@ -38,6 +39,14 @@ exports.expressCreateServer = function (hook_name, args, cb) { io.use((socket, next) => { const req = socket.request; + // Express sets req.ip but socket.io does not. Replicate Express's behavior here. + if (req.ip == null) { + if (settings.trustProxy) { + req.ip = proxyaddr(req, args.app.get('trust proxy fn')); + } else { + req.ip = socket.handshake.address; + } + } if (!req.headers.cookie) { // socketio.js-client on node.js doesn't support cookies (see https://git.io/JU8u9), so the // token and express_sid cookies have to be passed via a query parameter for unit tests. diff --git a/src/node/utils/RemoteAddress.js b/src/node/utils/RemoteAddress.js deleted file mode 100644 index 86a4a5b26..000000000 --- a/src/node/utils/RemoteAddress.js +++ /dev/null @@ -1 +0,0 @@ -exports.remoteAddress = {}; diff --git a/src/package.json b/src/package.json index 155638161..1d57e6116 100644 --- a/src/package.json +++ b/src/package.json @@ -55,6 +55,7 @@ "nodeify": "1.0.1", "npm": "6.14.8", "openapi-backend": "2.4.1", + "proxy-addr": "^2.0.6", "rate-limiter-flexible": "^2.1.4", "rehype": "^10.0.0", "rehype-format": "^3.0.1",