STOR.js reply(150) before waitForConnection

[inkeytech]

Hello,

Thanks again for the great package.
I've implemented an FTP Service using the package, but have recently encountered an FTP Client issue.
This client, after sending a STOR command in PASV mode, does not attempt to immediatly connect to the passive port, but appears to wait for the "150" confirmation.
This causes an issue because STOR.js does not send a 150 reply until it receives a connection on the passive port.
If I understand that bit of code correctly :)
This results in both the Client and Server waiting.

I'm not sure if the FTP RFC dictates that an FTP client should wait for 150 before connecting to the port, but this client has decided it is the correct approach :)

I've tried fiddling with the code of STOR.js - inserting the reply(150) before the waitForConnection - with limited results. I'm very unfamiliar with the Bluebird Promise functions being used.

My question: Is it possible to tweak STOR.js so that it sends a 150 before waitForConnection, without breaking the rest of the command flow.

Any pointers on how I might do that, if it is possible would be greatly appreciated.

Thanks,

Ian

[ideadesignmedia]

Figured it out:

const Promise = require('bluebird');

module.exports = {
  directive: 'STOR',
  handler: function ({log, command} = {}) {
    if (!this.fs) return this.reply(550, 'File system not instantiated');
    if (!this.fs.write) return this.reply(402, 'Not supported by file system');

    const append = command.directive === 'APPE';
    const fileName = command.arg;
	this.reply(150).then(() => this.connector.socket && this.connector.socket.resume())
    return this.connector.waitForConnection()
    .tap(() => this.commandSocket.pause())
    .then(() => Promise.try(() => this.fs.write(fileName, {append, start: this.restByteCount})))
    .then((fsResponse) => {
      let {stream, clientPath} = fsResponse;
      if (!stream && !clientPath) {
        stream = fsResponse;
        clientPath = fileName;
      }
      const serverPath = stream.path || fileName;

      const destroyConnection = (connection, reject) => (err) => {
        try {
          if (connection) {
            if (connection.writable) connection.end();
            connection.destroy(err);
          }
        } finally {
          reject(err);
        }
      };

      const streamPromise = new Promise((resolve, reject) => {
        stream.once('error', destroyConnection(this.connector.socket, reject));
        stream.once('finish', () => resolve());
      });

      const socketPromise = new Promise((resolve, reject) => {
        this.connector.socket.pipe(stream, {end: false});
        this.connector.socket.once('end', () => {
          if (stream.listenerCount('close')) stream.emit('close');
          else stream.end();
          resolve();
        });
        this.connector.socket.once('error', destroyConnection(stream, reject));
      });

      this.restByteCount = 0;

      return Promise.all([streamPromise, socketPromise])
      .tap(() => this.emit('STOR', null, serverPath))
      .then(() => this.reply(226, clientPath))
      .finally(() => stream.destroy && stream.destroy());
    })
    .catch(Promise.TimeoutError, (err) => {
      log.error(err);
      return this.reply(425, 'No connection established');
    })
    .catch((err) => {
      log.error(err);
      this.emit('STOR', err);
      return this.reply(550, err.message);
    })
    .finally(() => {
      this.connector.end();
      this.commandSocket.resume();
    });
  },
  syntax: '{{cmd}} <path>',
  description: 'Store data as a file at the server site'
};

[matt-forster]

I'd love to see a pull request to best understand the difference between the default command handle and this one!

[inkeytech]

Hello @matt-forster ,

Sorry for the long delay in reply - I've been trying to get a version of the offending FTP client to replicate and test with.
No luck on that front.

I have managed to replicate the issue, I think, based on a transcript of the "conversation"

I can provide some code of my test, if you want.

It would appear that the client is dropping the control connection too soon after (or possibly even before) the data connection is dropped.

According to the RFC (https://www.w3.org/Protocols/rfc959/),

"....In general, it is the server's responsibility to maintain the data connection-........The server MUST close the data connection under the following conditions:.....
4. The control connection is closed legally or otherwise.

So it appears the package is operating to spec.

But, for my use-case (catering for legacy clients that still want to connect), I'd like to try and add an X millisecond delay before the data connection (passive) is CONSIDERED closed by the package.

This may well introduce a security flaw - as I'd have to implicitly trust the next connection on a specific port for an instant, without having a control connection.

Any pointers would be appreciated.

Thanks,

Ian

[matt-forster]

Perhaps as a legacy affordance, we can add an optional path for that - that users can enable intentionally and with understanding of potential issues.

We already have an issue with sockets and holding onto ports, I'm not sure if that would make it worse or now.

[inkeytech]

Hello @matt-forster ,

Thank you for the feedback and info.

I wonder if my issue could somehow be related to sockets holding onto ports.

I cannot vouch for the workings of the telnet-client library I'm using to replicate the original issue, but I can replicate it like this:

By inserting a delay between ending the passive data connection and then ending the control connection, I can manage to get my tests to work against the FTP-SRV.

Is it possible that the data connection is not dropping the port immediatly, but only after a few milliseconds, resulting in the control connection effectively being dropped BEFORE the data connection is perceived to be dropped.

The above is purely speculative. I'll have look at the "socket holding onto ports" issue.

Thanks,

Ian

[matt-forster]

Potentially related to #312

[matt-forster]

Which client are you two using?

[ideadesignmedia]

I was running into this problem using WinSCP.

[inkeytech]

This was experienced with OpenInsight's FTPSEND BASIC+ command.

I didn't get to replicate my client's error as the evaluation version of OpenInsight doesn't seem to feature that command.