From ca576acf2eb3c2e5196a041eb61f04e5911ea8a4 Mon Sep 17 00:00:00 2001 From: Tyler Stewart Date: Mon, 20 Jul 2020 16:30:04 -0600 Subject: [PATCH] fix: correctly destroy socket on close (#208) * chore: update owner references * fix: correctly destroy socket on close This fixes an issue where the client would never close, hanging the server and preventing shutdown * fix: only set timeout if greater than 0 * fix: move dependency to top * fix: notify of command that caused error * fix: emit disconnect event on client close --- README.md | 6 +++--- package.json | 2 +- src/commands/index.js | 10 +++++----- src/commands/registration/feat.js | 2 +- src/connection.js | 9 ++++++--- src/index.js | 4 ++-- 6 files changed, 18 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 9966bb99..1fcf746f 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@

- + ftp-srv

@@ -14,8 +14,8 @@ npm - - circleci + + circleci

diff --git a/package.json b/package.json index 79d6ac10..0577cec7 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "types": "./ftp-srv.d.ts", "repository": { "type": "git", - "url": "https://github.com/trs/ftp-srv" + "url": "https://github.com/autovance/ftp-srv" }, "scripts": { "pre-release": "npm run verify", diff --git a/src/commands/index.js b/src/commands/index.js index fa8a6ba0..a0cd02a0 100644 --- a/src/commands/index.js +++ b/src/commands/index.js @@ -45,25 +45,25 @@ class FtpCommands { log.trace({command: logCommand}, 'Handle command'); if (!REGISTRY.hasOwnProperty(command.directive)) { - return this.connection.reply(502, 'Command not allowed'); + return this.connection.reply(502, `Command not allowed: ${command.directive}`); } if (_.includes(this.blacklist, command.directive)) { - return this.connection.reply(502, 'Command blacklisted'); + return this.connection.reply(502, `Command blacklisted: ${command.directive}`); } if (this.whitelist.length > 0 && !_.includes(this.whitelist, command.directive)) { - return this.connection.reply(502, 'Command not whitelisted'); + return this.connection.reply(502, `Command not whitelisted: ${command.directive}`); } const commandRegister = REGISTRY[command.directive]; const commandFlags = _.get(commandRegister, 'flags', {}); if (!commandFlags.no_auth && !this.connection.authenticated) { - return this.connection.reply(530, 'Command requires authentication'); + return this.connection.reply(530, `Command requires authentication: ${command.directive}`); } if (!commandRegister.handler) { - return this.connection.reply(502, 'Handler not set on command'); + return this.connection.reply(502, `Handler not set on command: ${command.directive}`); } const handler = commandRegister.handler.bind(this.connection); diff --git a/src/commands/registration/feat.js b/src/commands/registration/feat.js index 596cd244..a5fc6af5 100644 --- a/src/commands/registration/feat.js +++ b/src/commands/registration/feat.js @@ -1,9 +1,9 @@ const _ = require('lodash'); +const registry = require('../registry'); module.exports = { directive: 'FEAT', handler: function () { - const registry = require('../registry'); const features = Object.keys(registry) .reduce((feats, cmd) => { const feat = _.get(registry[cmd], 'flags.feat', null); diff --git a/src/connection.js b/src/connection.js index c11a8a4a..eeecdbd6 100644 --- a/src/connection.js +++ b/src/connection.js @@ -32,7 +32,7 @@ class FtpConnection extends EventEmitter { this.commandSocket.on('data', this._handleData.bind(this)); this.commandSocket.on('timeout', () => { this.log.trace('Client timeout'); - this.close().catch((e) => this.log.trace(e, 'Client close error')); + this.close(); }); this.commandSocket.on('close', () => { if (this.connector) this.connector.end(); @@ -72,7 +72,7 @@ class FtpConnection extends EventEmitter { close(code = 421, message = 'Closing connection') { return Promise.resolve(code) .then((_code) => _code && this.reply(_code, message)) - .then(() => this.commandSocket && this.commandSocket.end()); + .finally(() => this.commandSocket && this.commandSocket.destroy()); } login(username, password) { @@ -136,7 +136,10 @@ class FtpConnection extends EventEmitter { } resolve(); }); - } else reject(new errors.SocketError('Socket not writable')); + } else { + this.log.trace({message: letter.message}, 'Could not write message'); + reject(new errors.SocketError('Socket not writable')); + } }); }; diff --git a/src/index.js b/src/index.js index e744395c..aae5d038 100644 --- a/src/index.js +++ b/src/index.js @@ -44,11 +44,12 @@ class FtpServer extends EventEmitter { this.options.timeout = isNaN(timeout) ? 0 : Number(timeout); const serverConnectionHandler = (socket) => { - socket.setTimeout(this.options.timeout); + this.options.timeout > 0 && socket.setTimeout(this.options.timeout); let connection = new Connection(this, {log: this.log, socket}); this.connections[connection.id] = connection; socket.on('close', () => this.disconnectClient(connection.id)); + socket.once('close', () => this.emit('disconnect', {connection, id: connection.id})); const greeting = this._greeting || []; const features = this._features || 'Ready'; @@ -119,7 +120,6 @@ class FtpServer extends EventEmitter { return new Promise((resolve) => { const client = this.connections[id]; if (!client) return resolve(); - this.emit('disconnect', {connection: client, id}); delete this.connections[id]; try { client.close(0);