From 85efd0c8f047d00b40e89264e685e98f2fce2b63 Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Fri, 13 Oct 2023 17:24:14 +0200 Subject: [PATCH 1/6] added test case for unexpected closure of dataa socket --- test/downloadSpec.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/downloadSpec.js b/test/downloadSpec.js index a698ce8..4e76dee 100644 --- a/test/downloadSpec.js +++ b/test/downloadSpec.js @@ -146,6 +146,22 @@ describe("Download to stream", function() { await this.client.downloadTo(buf, FILENAME) dataSocket.destroy(new Error("Error that should be ignored because task has completed successfully")) }) + it("handles early data socket closure", async () => { + this.server.addHandlers({ + "pasv": () => `227 Entering Passive Mode (${this.server.dataAddressForPasvResponse})`, + "retr": ({arg}) => { + //close data connection such that client receives ECONNRESET + this.server.dataConn.resetAndDestroy() + + return `550 ${arg}: No such file or directory.` + } + }) + + const buf = new StringWriter() + await this.client.downloadTo(buf, FILENAME) + //control socket should still be open + assert(this.client.ftp.socket?.writable) + }) it("stops tracking timeout after failure") it("can get a directory listing") From 1d8f1913ae09fd832122b29deab61c73b60f6436 Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Fri, 13 Oct 2023 22:09:59 +0200 Subject: [PATCH 2/6] fixed control socket closing when data socket unexpectedly closes --- src/FtpContext.ts | 22 ++++++++++++++++++---- test/downloadSpec.js | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/FtpContext.ts b/src/FtpContext.ts index 70f0e79..768f463 100644 --- a/src/FtpContext.ts +++ b/src/FtpContext.ts @@ -1,7 +1,7 @@ import { Socket } from "net" import { ConnectionOptions as TLSConnectionOptions, TLSSocket } from "tls" -import { parseControlResponse } from "./parseControlResponse" import { StringEncoding } from "./StringEncoding" +import { parseControlResponse } from "./parseControlResponse" interface Task { /** Handles a response for a task. */ @@ -361,16 +361,30 @@ export class FTPContext { protected _setupDefaultErrorHandlers(socket: Socket, identifier: string) { socket.once("error", error => { error.message += ` (${identifier})` - this.closeWithError(error) + if(identifier == "control socket") { + this.closeWithError(error) + } else { + //only close data socket, not the control socket as well + this._closeSocket(socket) + } }) socket.once("close", hadError => { if (hadError) { - this.closeWithError(new Error(`Socket closed due to transmission error (${identifier})`)) + if(identifier == "control socket") { + this.closeWithError(new Error(`Socket closed due to transmission error (${identifier})`)) + } else { + //only close data socket, not the control socket as well + this._closeSocket(socket) + } } }) socket.once("timeout", () => { socket.destroy() - this.closeWithError(new Error(`Timeout (${identifier})`)) + if(identifier == "control socket") { + this.closeWithError(new Error(`Timeout (${identifier})`)) + } else { + this._closeSocket(socket) + } }) } diff --git a/test/downloadSpec.js b/test/downloadSpec.js index 4e76dee..d391c03 100644 --- a/test/downloadSpec.js +++ b/test/downloadSpec.js @@ -158,7 +158,7 @@ describe("Download to stream", function() { }) const buf = new StringWriter() - await this.client.downloadTo(buf, FILENAME) + await this.client.downloadTo(buf, FILENAME).catch(err => {}) //control socket should still be open assert(this.client.ftp.socket?.writable) }) From 3cbc0ad63c149291685516a82f277531d607626e Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Fri, 13 Oct 2023 22:56:53 +0200 Subject: [PATCH 3/6] installable --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index d4fcc1e..f8b0c99 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "lint-fix": "eslint --fix \"./src/**/*.ts\"", "dev": "npm run clean && tsc --watch", "tdd": "mocha --watch", + "prepare": "tsc", "buildOnly": "tsc" }, "repository": { From 602668380655625a82c321236e1f3a7b7e3e7b12 Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Sun, 15 Oct 2023 20:19:48 +0200 Subject: [PATCH 4/6] fix Relays data socket timeout & error event --- src/FtpContext.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/FtpContext.ts b/src/FtpContext.ts index 768f463..b051770 100644 --- a/src/FtpContext.ts +++ b/src/FtpContext.ts @@ -366,6 +366,7 @@ export class FTPContext { } else { //only close data socket, not the control socket as well this._closeSocket(socket) + this._passToHandler(error) } }) socket.once("close", hadError => { @@ -375,6 +376,7 @@ export class FTPContext { } else { //only close data socket, not the control socket as well this._closeSocket(socket) + this._passToHandler(new Error(`Socket closed due to transmission error (${identifier})`)) } } }) @@ -384,6 +386,7 @@ export class FTPContext { this.closeWithError(new Error(`Timeout (${identifier})`)) } else { this._closeSocket(socket) + this._passToHandler(new Error(`Timeout (${identifier})`)) } }) } From d07711670a7d5c267e5d82c6e41ba4989b100fbe Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Sun, 15 Oct 2023 20:26:36 +0200 Subject: [PATCH 5/6] fix workflow --- test/downloadSpec.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/downloadSpec.js b/test/downloadSpec.js index d391c03..5845560 100644 --- a/test/downloadSpec.js +++ b/test/downloadSpec.js @@ -4,6 +4,7 @@ const { StringWriter } = require("../dist/StringWriter"); const MockFtpServer = require("./MockFtpServer"); const { Writable } = require("stream") const fs = require("fs"); +const { Socket } = require("net"); const FILENAME = "file.txt" const TIMEOUT = 1000 @@ -151,7 +152,7 @@ describe("Download to stream", function() { "pasv": () => `227 Entering Passive Mode (${this.server.dataAddressForPasvResponse})`, "retr": ({arg}) => { //close data connection such that client receives ECONNRESET - this.server.dataConn.resetAndDestroy() + this.server.dataConn.resetAndDestroy?.() return `550 ${arg}: No such file or directory.` } From 17322c26d81758d5286affd8e9f57c7d57132e43 Mon Sep 17 00:00:00 2001 From: RythenGlyth Date: Mon, 16 Oct 2023 19:15:02 +0200 Subject: [PATCH 6/6] inject resetAndDestroy into socket in test for older versions of node --- test/MockFtpServer.js | 9 +++++++++ test/downloadSpec.js | 8 +++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/test/MockFtpServer.js b/test/MockFtpServer.js index 541501e..dc1aaab 100644 --- a/test/MockFtpServer.js +++ b/test/MockFtpServer.js @@ -39,6 +39,15 @@ module.exports = class MockFtpServer { }) this.dataConn = undefined this.dataServer = net.createServer(conn => { + if(!conn.resetAndDestroy) { + conn.resetAndDestroy = () => { + conn._handle.reset(() => { + this.conn.emit("close") + }) + conn._handle.onread = () =>{}; + conn._handle = null; + } + } this.dataConn = conn this.connections.push(conn) this.didOpenDataConn() diff --git a/test/downloadSpec.js b/test/downloadSpec.js index 5845560..fc6c67a 100644 --- a/test/downloadSpec.js +++ b/test/downloadSpec.js @@ -148,11 +148,17 @@ describe("Download to stream", function() { dataSocket.destroy(new Error("Error that should be ignored because task has completed successfully")) }) it("handles early data socket closure", async () => { + /** + * type of this.client + * @type {Client} + */ + this.client; + this.server.addHandlers({ "pasv": () => `227 Entering Passive Mode (${this.server.dataAddressForPasvResponse})`, "retr": ({arg}) => { //close data connection such that client receives ECONNRESET - this.server.dataConn.resetAndDestroy?.() + this.server.dataConn.resetAndDestroy() return `550 ${arg}: No such file or directory.` }