From 9de8745748a7d3ad486dd60eded63204c6b37a76 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Tue, 4 Mar 2025 11:17:51 +0200 Subject: [PATCH 01/12] Add support for env variables with SFTP client. --- lib/client.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/client.js b/lib/client.js index aa94ace0..9036a7fd 100644 --- a/lib/client.js +++ b/lib/client.js @@ -146,6 +146,7 @@ class Client extends EventEmitter { || Buffer.isBuffer(cfg.ident) ? cfg.ident : undefined); + this.config.sftpEnv = cfg.sftpEnv; const algorithms = { kex: undefined, @@ -1562,6 +1563,11 @@ class Client extends EventEmitter { return; } + const sftpEnv = this.config.sftpEnv; + if (typeof sftpEnv === 'object' && sftpEnv !== null) { + reqEnv(sftp, sftpEnv); + } + reqSubsystem(sftp, 'sftp', (err, sftp_) => { if (err) { cb(err); From 9a9b18ccd4b77e1e4d5a78a7bab732ccb2474921 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Wed, 5 Mar 2025 11:36:12 +0200 Subject: [PATCH 02/12] Update approach for passing env. --- lib/client.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/client.js b/lib/client.js index 9036a7fd..aff9aad9 100644 --- a/lib/client.js +++ b/lib/client.js @@ -146,7 +146,6 @@ class Client extends EventEmitter { || Buffer.isBuffer(cfg.ident) ? cfg.ident : undefined); - this.config.sftpEnv = cfg.sftpEnv; const algorithms = { kex: undefined, @@ -1553,7 +1552,7 @@ class Client extends EventEmitter { return this; } - sftp(cb) { + sftp(env, cb) { if (!this._sock || !isWritable(this._sock)) throw new Error('Not connected'); @@ -1563,9 +1562,8 @@ class Client extends EventEmitter { return; } - const sftpEnv = this.config.sftpEnv; - if (typeof sftpEnv === 'object' && sftpEnv !== null) { - reqEnv(sftp, sftpEnv); + if (typeof env === 'object' && env !== null) { + reqEnv(sftp, env); } reqSubsystem(sftp, 'sftp', (err, sftp_) => { From d0068fad0e548da421c0e38a05f99c6bcb0e06b4 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Wed, 5 Mar 2025 17:09:53 +0200 Subject: [PATCH 03/12] Make env optional. --- lib/client.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/client.js b/lib/client.js index aff9aad9..3cb63b24 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1556,6 +1556,11 @@ class Client extends EventEmitter { if (!this._sock || !isWritable(this._sock)) throw new Error('Not connected'); + if (typeof env === 'function') { + cb = env; + env = undefined; + } + openChannel(this, 'sftp', (err, sftp) => { if (err) { cb(err); From 55905be24e9623778f07acccf0cd2d9e8589be0e Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Wed, 5 Mar 2025 17:16:33 +0200 Subject: [PATCH 04/12] Update main README with sftp interface changes. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d0e6f7fc..3a0dd4b5 100644 --- a/README.md +++ b/README.md @@ -1028,7 +1028,7 @@ You can find more examples in the `examples` directory of this repository. * **setNoDelay**([< _boolean_ >noDelay]) - _Client_ - Calls [`setNoDelay()`](https://nodejs.org/docs/latest/api/net.html#socketsetnodelaynodelay) on the underlying socket. Disabling Nagle's algorithm improves latency at the expense of lower throughput. -* **sftp**(< _function_ >callback) - _(void)_ - Starts an SFTP session. `callback` has 2 parameters: < _Error_ >err, < _SFTP_ >sftp. For methods available on `sftp`, see the [`SFTP` client documentation](https://github.com/mscdex/ssh2/blob/master/SFTP.md). +* **sftp**([< _object_ >env], < _function_ >callback) - _(void)_ - Starts an SFTP session. `env` is an environment to use when executing `sftp` methods. `callback` has 2 parameters: < _Error_ >err, < _SFTP_ >sftp. For methods available on `sftp`, see the [`SFTP` client documentation](https://github.com/mscdex/ssh2/blob/master/SFTP.md). * **shell**([[< _mixed_ >window,] < _object_ >options]< _function_ >callback) - _(void)_ - Starts an interactive shell session on the server, with an optional `window` object containing pseudo-tty settings (see 'Pseudo-TTY settings'). If `window === false`, then no pseudo-tty is allocated. `options` supports the `x11` and `env` options as described in `exec()`. `callback` has 2 parameters: < _Error_ >err, < _Channel_ >stream. From 5f85ae11a20b8f5ed9103d915d0f71e578335281 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Thu, 13 Mar 2025 13:27:26 +0200 Subject: [PATCH 05/12] Ensure execution order. --- lib/client.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index 3cb63b24..532897ef 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1567,11 +1567,7 @@ class Client extends EventEmitter { return; } - if (typeof env === 'object' && env !== null) { - reqEnv(sftp, env); - } - - reqSubsystem(sftp, 'sftp', (err, sftp_) => { + const reqSubsystemCb = (err, sftp_) => { if (err) { cb(err); return; @@ -1617,7 +1613,14 @@ class Client extends EventEmitter { .on('close', onExit); sftp._init(); - }); + }; + + if (typeof env === 'object' && env !== null) { + reqEnv(sftp, env, () => reqSubsystem(sftp, 'sftp', reqSubsystemCb)); + + } else { + reqSubsystem(sftp, 'sftp', reqSubsystemCb); + } }); return this; @@ -1851,7 +1854,7 @@ function reqExec(chan, cmd, opts, cb) { chan._client._protocol.exec(chan.outgoing.id, cmd, true); } -function reqEnv(chan, env) { +function reqEnv(chan, env, cb) { if (chan.outgoing.state !== 'open') return; @@ -1862,6 +1865,8 @@ function reqEnv(chan, env) { const val = env[key]; chan._client._protocol.env(chan.outgoing.id, key, val, false); } + + typeof cb === 'function' && cb(); } function reqSubsystem(chan, name, cb) { From ab9dc45683f25372792ab376910596cdc3cdb4a0 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Tue, 25 Mar 2025 11:34:15 +0200 Subject: [PATCH 06/12] Improve callback handling. --- lib/client.js | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/client.js b/lib/client.js index 532897ef..16a0f2d0 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1855,18 +1855,33 @@ function reqExec(chan, cmd, opts, cb) { } function reqEnv(chan, env, cb) { - if (chan.outgoing.state !== 'open') + const wantReply = (typeof cb === 'function'); + + if (chan.outgoing.state !== 'open') { + if (wantReply) + cb(new Error('Channel is not open')); return; + } + + if (wantReply) { + chan._callbacks.push((had_err) => { + if (had_err) { + cb(had_err !== true + ? had_err + : new Error('Unable to set env')); + return; + } + cb(); + }); + } const keys = Object.keys(env || {}); for (let i = 0; i < keys.length; ++i) { const key = keys[i]; const val = env[key]; - chan._client._protocol.env(chan.outgoing.id, key, val, false); + chan._client._protocol.env(chan.outgoing.id, key, val, wantReply); } - - typeof cb === 'function' && cb(); } function reqSubsystem(chan, name, cb) { From 0b62ea6efc960057f439e02b37aeb42f89ffbdaf Mon Sep 17 00:00:00 2001 From: dimitar <60298689+di-ov@users.noreply.github.com> Date: Tue, 25 Mar 2025 11:37:42 +0200 Subject: [PATCH 07/12] Update README.md Co-authored-by: mscdex --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3a0dd4b5..e08c929e 100644 --- a/README.md +++ b/README.md @@ -1028,7 +1028,7 @@ You can find more examples in the `examples` directory of this repository. * **setNoDelay**([< _boolean_ >noDelay]) - _Client_ - Calls [`setNoDelay()`](https://nodejs.org/docs/latest/api/net.html#socketsetnodelaynodelay) on the underlying socket. Disabling Nagle's algorithm improves latency at the expense of lower throughput. -* **sftp**([< _object_ >env], < _function_ >callback) - _(void)_ - Starts an SFTP session. `env` is an environment to use when executing `sftp` methods. `callback` has 2 parameters: < _Error_ >err, < _SFTP_ >sftp. For methods available on `sftp`, see the [`SFTP` client documentation](https://github.com/mscdex/ssh2/blob/master/SFTP.md). +* **sftp**([< _object_ >env, ]< _function_ >callback) - _(void)_ - Starts an SFTP session. `env` is an environment to use when executing `sftp` methods. `callback` has 2 parameters: < _Error_ >err, < _SFTP_ >sftp. For methods available on `sftp`, see the [`SFTP` client documentation](https://github.com/mscdex/ssh2/blob/master/SFTP.md). * **shell**([[< _mixed_ >window,] < _object_ >options]< _function_ >callback) - _(void)_ - Starts an interactive shell session on the server, with an optional `window` object containing pseudo-tty settings (see 'Pseudo-TTY settings'). If `window === false`, then no pseudo-tty is allocated. `options` supports the `x11` and `env` options as described in `exec()`. `callback` has 2 parameters: < _Error_ >err, < _Channel_ >stream. From 464335196e152c21c68d4bd6082e869ea329f424 Mon Sep 17 00:00:00 2001 From: dimitar <60298689+di-ov@users.noreply.github.com> Date: Tue, 25 Mar 2025 16:45:13 +0200 Subject: [PATCH 08/12] Update lib/client.js Co-authored-by: mscdex --- lib/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 16a0f2d0..483a6f6e 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1868,7 +1868,7 @@ function reqEnv(chan, env, cb) { if (had_err) { cb(had_err !== true ? had_err - : new Error('Unable to set env')); + : new Error('Unable to set environment')); return; } cb(); From f176a9c99b3ad4dd1f983fa737fd8786d83af791 Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Wed, 26 Mar 2025 11:18:27 +0200 Subject: [PATCH 09/12] Improve error handling. --- lib/client.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 483a6f6e..f3e321e5 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1616,8 +1616,14 @@ class Client extends EventEmitter { }; if (typeof env === 'object' && env !== null) { - reqEnv(sftp, env, () => reqSubsystem(sftp, 'sftp', reqSubsystemCb)); + reqEnv(sftp, env, (err) => { + if (err) { + cb(err); + return; + } + reqSubsystem(sftp, 'sftp', reqSubsystemCb); + }); } else { reqSubsystem(sftp, 'sftp', reqSubsystemCb); } From 36592691ad3e779c0f918fdc81d2be078e2ab4ab Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Fri, 28 Mar 2025 12:02:18 +0200 Subject: [PATCH 10/12] Add unit test. --- test/test-sftp.js | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/test-sftp.js b/test/test-sftp.js index 1bda7668..0715c937 100644 --- a/test/test-sftp.js +++ b/test/test-sftp.js @@ -755,6 +755,48 @@ setup('WriteStream', mustCall((client, server) => { })); } +{ + const { client, server } = setup_( + 'SFTP server sets environment and aborts with exit-status', + { + client: { username: 'foo', password: 'bar' }, + server: { hostKeys: [ fixture('ssh_host_rsa_key') ] }, + }, + ); + + const env = { SSH2NODETEST: 'foo' }; + + server.on('connection', mustCall((conn) => { + conn.on('authentication', mustCall((ctx) => { + ctx.accept(); + })).on('ready', mustCall(() => { + conn.on('session', mustCall((accept, reject) => { + accept().on('env', mustCall((accept, reject, info) => { + accept && accept(); + assert(info.key === Object.keys(env)[0], 'Wrong env key'); + assert(info.val === Object.values(env)[0], 'Wrong env value'); + })).on('sftp', mustCall((accept, reject) => { + const sftp = accept(); + + // XXX: hack + sftp._protocol.exitStatus(sftp.outgoing.id, 127); + sftp._protocol.channelClose(sftp.outgoing.id); + })); + })); + })); + })); + + client.on('ready', mustCall(() => { + const timeout = setTimeout(mustNotCall(), 1000); + client.sftp(env, mustCall((err, sftp) => { + clearTimeout(timeout); + assert(err, 'Expected error'); + assert(err.code === 127, `Expected exit code 127, saw: ${err.code}`); + client.end(); + })); + })); +} + // ============================================================================= function setup(title, cb) { From 67c6f19e36f0b03d088743ecb76b3c3495c576fb Mon Sep 17 00:00:00 2001 From: Dimitar Markov Date: Fri, 28 Mar 2025 16:33:14 +0200 Subject: [PATCH 11/12] Unit test improvements. --- test/test-sftp.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/test-sftp.js b/test/test-sftp.js index 0715c937..595e5091 100644 --- a/test/test-sftp.js +++ b/test/test-sftp.js @@ -757,7 +757,7 @@ setup('WriteStream', mustCall((client, server) => { { const { client, server } = setup_( - 'SFTP server sets environment and aborts with exit-status', + 'SFTP client sets environment', { client: { username: 'foo', password: 'bar' }, server: { hostKeys: [ fixture('ssh_host_rsa_key') ] }, @@ -776,11 +776,7 @@ setup('WriteStream', mustCall((client, server) => { assert(info.key === Object.keys(env)[0], 'Wrong env key'); assert(info.val === Object.values(env)[0], 'Wrong env value'); })).on('sftp', mustCall((accept, reject) => { - const sftp = accept(); - - // XXX: hack - sftp._protocol.exitStatus(sftp.outgoing.id, 127); - sftp._protocol.channelClose(sftp.outgoing.id); + accept(); })); })); })); @@ -790,14 +786,12 @@ setup('WriteStream', mustCall((client, server) => { const timeout = setTimeout(mustNotCall(), 1000); client.sftp(env, mustCall((err, sftp) => { clearTimeout(timeout); - assert(err, 'Expected error'); - assert(err.code === 127, `Expected exit code 127, saw: ${err.code}`); + assert(!err, `Unexpected exec error: ${err}`); client.end(); })); })); } - // ============================================================================= function setup(title, cb) { const { client, server } = setupSimple(DEBUG, title); From 4d74693ff548e509aad1d153103f2fb7e3fb6ad3 Mon Sep 17 00:00:00 2001 From: dimitar <60298689+di-ov@users.noreply.github.com> Date: Fri, 28 Mar 2025 22:16:49 +0200 Subject: [PATCH 12/12] Improve formatting. Co-authored-by: mscdex --- lib/client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index f3e321e5..7291c2ce 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1622,8 +1622,8 @@ class Client extends EventEmitter { return; } - reqSubsystem(sftp, 'sftp', reqSubsystemCb); - }); + reqSubsystem(sftp, 'sftp', reqSubsystemCb); + }); } else { reqSubsystem(sftp, 'sftp', reqSubsystemCb); }