This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
The following commit(s) were added to refs/heads/master by this push:
new f908189c4 GH-428/GH-392 SCP client fails silently when error signalled
due to missing file or lacking permissions
f908189c4 is described below
commit f908189c41a26f2b9b08ea62199a579771d04134
Author: Lyor Goldstein <lgoldst...@apache.org>
AuthorDate: Fri Nov 10 11:09:07 2023 +0200
GH-428/GH-392 SCP client fails silently when error signalled due to missing
file or lacking permissions
---
CHANGES.md | 9 +++
.../org/apache/sshd/cli/client/ScpCommandMain.java | 1 -
.../apache/sshd/scp/client/DefaultScpClient.java | 4 +-
.../java/org/apache/sshd/scp/common/ScpHelper.java | 78 ++++++++++++----------
.../sshd/scp/common/ScpTransferEventListener.java | 18 +++++
.../apache/sshd/scp/common/helpers/ScpAckInfo.java | 10 ++-
.../org/apache/sshd/scp/server/ScpCommand.java | 2 +-
.../java/org/apache/sshd/scp/server/ScpShell.java | 10 +--
.../java/org/apache/sshd/scp/client/ScpTest.java | 23 +++++++
9 files changed, 107 insertions(+), 48 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 180d1de8e..97f72b924 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -28,10 +28,19 @@
## Bug Fixes
+* [GH-428/GH-392](https://github.com/apache/mina-sshd/issues/428) SCP client
fails silently when error signalled due to missing file or lacking permissions
+
## New Features
# Behavioral changes and enhancements
+## New `ScpTransferEventListener` callback method
+
+Following [GH-428/GH-392](https://github.com/apache/mina-sshd/issues/428) a
new `handleReceiveCommandAckInfo` method has been added to enable users to
inspect
+acknowledgements of a `receive` related command. The user is free to inspect
the command that was attempted as well as the response code and decide how
+to handle it - including even throwing an exception if OK status (if this
makes sense for whatever reason). The default implementation checks for ERROR
code and throws
+an exception if so.
+
## Potential compatibility issues
### Server-side SFTP file handle encoding
diff --git
a/sshd-cli/src/main/java/org/apache/sshd/cli/client/ScpCommandMain.java
b/sshd-cli/src/main/java/org/apache/sshd/cli/client/ScpCommandMain.java
index b089386e2..72102c8cf 100644
--- a/sshd-cli/src/main/java/org/apache/sshd/cli/client/ScpCommandMain.java
+++ b/sshd-cli/src/main/java/org/apache/sshd/cli/client/ScpCommandMain.java
@@ -322,7 +322,6 @@ public class ScpCommandMain extends SshClientCliSupport {
} finally {
session.close();
}
-
}
/*
--------------------------------------------------------------------------------
*/
diff --git
a/sshd-scp/src/main/java/org/apache/sshd/scp/client/DefaultScpClient.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/client/DefaultScpClient.java
index ead682748..7a794f41e 100644
--- a/sshd-scp/src/main/java/org/apache/sshd/scp/client/DefaultScpClient.java
+++ b/sshd-scp/src/main/java/org/apache/sshd/scp/client/DefaultScpClient.java
@@ -74,7 +74,7 @@ public class DefaultScpClient extends AbstractScpClient {
OutputStream invIn = channel.getInvertedIn()) {
// NOTE: we use a mock file system since we expect no invocations
for it
ScpHelper helper = new ScpHelper(session, invOut, invIn, new
MockFileSystem(remote), opener, listener);
- helper.receiveFileStream(local,
ScpHelper.DEFAULT_RECEIVE_BUFFER_SIZE);
+ helper.receiveFileStream(cmd, local,
ScpHelper.DEFAULT_RECEIVE_BUFFER_SIZE);
handleCommandExitStatus(cmd, channel);
} finally {
channel.close(false);
@@ -89,7 +89,7 @@ public class DefaultScpClient extends AbstractScpClient {
try (InputStream invOut = channel.getInvertedOut();
OutputStream invIn = channel.getInvertedIn()) {
ScpHelper helper = new ScpHelper(session, invOut, invIn, fs,
opener, listener);
- helper.receive(local,
+ helper.receive(cmd, local,
options.contains(Option.Recursive),
options.contains(Option.TargetIsDirectory),
options.contains(Option.PreserveAttributes),
diff --git a/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpHelper.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpHelper.java
index 0973df780..f6f37b917 100644
--- a/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpHelper.java
+++ b/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpHelper.java
@@ -115,8 +115,8 @@ public class ScpHelper extends AbstractLoggingBean
implements SessionHolder<Sess
return sessionInstance;
}
- public void receiveFileStream(OutputStream local, int bufferSize) throws
IOException {
- receive((session, line, isDir, timestamp) -> {
+ public void receiveFileStream(String command, OutputStream local, int
bufferSize) throws IOException {
+ receive(command, (session, line, isDir, timestamp) -> {
if (isDir) {
throw new StreamCorruptedException("Cannot download a
directory into a file stream: " + line);
}
@@ -163,11 +163,11 @@ public class ScpHelper extends AbstractLoggingBean
implements SessionHolder<Sess
});
}
- public void receive(Path local, boolean recursive, boolean shouldBeDir,
boolean preserve, int bufferSize)
+ public void receive(String cmd, Path local, boolean recursive, boolean
shouldBeDir, boolean preserve, int bufferSize)
throws IOException {
Path localPath = Objects.requireNonNull(local, "No local
path").normalize().toAbsolutePath();
Path path = opener.resolveIncomingReceiveLocation(getSession(),
localPath, recursive, shouldBeDir, preserve);
- receive((session, line, isDir, time) -> {
+ receive(cmd, (session, line, isDir, time) -> {
if (recursive && isDir) {
receiveDir(line, path, time, preserve, bufferSize);
} else {
@@ -179,60 +179,71 @@ public class ScpHelper extends AbstractLoggingBean
implements SessionHolder<Sess
/**
* Reads command line(s) and invokes the handler until EOF or and
"E" command is received
*
+ * @param cmd The receive command being attempted
* @param handler The {@link ScpReceiveLineHandler} to invoke when a
command has been read
* @throws IOException If failed to read/write
*/
- protected void receive(ScpReceiveLineHandler handler) throws IOException {
+ protected void receive(String cmd, ScpReceiveLineHandler handler) throws
IOException {
sendOk();
boolean debugEnabled = log.isDebugEnabled();
Session session = getSession();
for (ScpTimestampCommandDetails time = null;; debugEnabled =
log.isDebugEnabled()) {
- String line;
- boolean isDir = false;
+ ScpAckInfo ackInfo = receiveNextCmd();
+ if (ackInfo == null) {
+ return;
+ }
- int c = receiveNextCmd();
+ int c = ackInfo.getStatusCode();
+ String line = ackInfo.getLine();
+ // NOTE: we rely on the fact that an SCP command does not start
with an ACK code
switch (c) {
- case -1:
- return;
+ case ScpAckInfo.OK:
+ if (debugEnabled) {
+ log.debug("receive({})[{}] ack={}", this, cmd,
ackInfo);
+ }
+ listener.handleReceiveCommandAckInfo(session, cmd,
ackInfo);
+ continue;
+ case ScpAckInfo.WARNING:
+ log.warn("receive({})[{}] ack={}", this, cmd, ackInfo);
+ listener.handleReceiveCommandAckInfo(session, cmd,
ackInfo);
+ continue;
+ case ScpAckInfo.ERROR:
+ log.error("receive({})[{}] bad ack: {}", this, cmd,
ackInfo);
+ listener.handleReceiveCommandAckInfo(session, cmd,
ackInfo);
+ continue;
case ScpReceiveDirCommandDetails.COMMAND_NAME:
- line = ScpIoUtils.readLine(in, csIn);
- line = Character.toString((char) c) + line;
- isDir = true;
if (debugEnabled) {
log.debug("receive({}) - Received 'D' header: {}",
this, line);
}
break;
case ScpReceiveFileCommandDetails.COMMAND_NAME:
- line = ScpIoUtils.readLine(in, csIn);
- line = Character.toString((char) c) + line;
if (debugEnabled) {
log.debug("receive({}) - Received 'C' header: {}",
this, line);
}
break;
case ScpTimestampCommandDetails.COMMAND_NAME:
- line = ScpIoUtils.readLine(in, csIn);
- line = Character.toString((char) c) + line;
if (debugEnabled) {
log.debug("receive({}) - Received 'T' header: {}",
this, line);
}
time = ScpTimestampCommandDetails.parse(line);
+ cmd = line; // next ACK might be for this command if
recursive receive
sendOk();
continue;
case ScpDirEndCommandDetails.COMMAND_NAME:
- line = ScpIoUtils.readLine(in, csIn);
- line = Character.toString((char) c) + line;
if (debugEnabled) {
log.debug("receive({}) - Received 'E' header: {}",
this, line);
}
sendOk();
return;
default:
- // a real ack that has been acted upon already
- continue;
+ log.error("receive({}) - Unsupported command: {}", this,
line);
+ throw new ScpException("Unsupported command: " + line,
ScpAckInfo.ERROR);
}
try {
+ boolean isDir = c == ScpReceiveDirCommandDetails.COMMAND_NAME;
+ cmd = line; // next ACK might be for this command if recursive
receive
handler.process(session, line, isDir, time);
} finally {
time = null;
@@ -240,25 +251,20 @@ public class ScpHelper extends AbstractLoggingBean
implements SessionHolder<Sess
}
}
- // NOTE: we rely on the fact that an SCP command does not start with an
ACK code
- protected int receiveNextCmd() throws IOException {
+ protected ScpAckInfo receiveNextCmd() throws IOException {
int c = in.read();
if (c == -1) {
- return c;
- }
-
- if (c == ScpAckInfo.OK) {
- return c;
- }
-
- if ((c == ScpAckInfo.WARNING) || (c == ScpAckInfo.ERROR)) {
+ return null;
+ } else if (c == ScpAckInfo.OK) {
+ // OK status has no extra data
+ return ScpAckInfo.OK_ACK_INFO;
+ } else if ((c == ScpAckInfo.WARNING) || (c == ScpAckInfo.ERROR)) {
String line = ScpIoUtils.readLine(in, csIn, true);
- if (log.isDebugEnabled()) {
- log.debug("receiveNextCmd - ACK={}", new ScpAckInfo(c, line));
- }
+ return new ScpAckInfo(c, line);
+ } else {
+ String line = ScpIoUtils.readLine(in, csIn, false);
+ return new ScpAckInfo(c, Character.toString((char) c) + line);
}
-
- return c;
}
public void receiveDir(String header, Path local,
ScpTimestampCommandDetails time, boolean preserve, int bufferSize)
diff --git
a/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpTransferEventListener.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpTransferEventListener.java
index 2cf9996ae..25a7fbdb2 100644
---
a/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpTransferEventListener.java
+++
b/sshd-scp/src/main/java/org/apache/sshd/scp/common/ScpTransferEventListener.java
@@ -96,6 +96,24 @@ public interface ScpTransferEventListener extends
SshdEventListener {
// ignored
}
+ /**
+ * Called after a receive related command has bee acknowledged by the peer
+ *
+ * @param session The client/server {@link Session} through which the
transfer is being executed
+ * @param command The <U>raw</U> command that was attempted
+ * @param ackInfo The {@link ScpAckInfo} received after command
execution - including if {@link ScpAckInfo#OK
+ * OK}. By default it throws an {@link ScpException}
if {@link ScpAckInfo#ERROR ERROR} status
+ * code, but the user is free to override this
behavior (see
+ * <A
HREF="https://github.com/apache/mina-sshd/issues/428";>ScpClient download fails
silently
+ * when the remote files does not exist </A>) -
including throwing an exception if OK status...
+ * @throws IOException If bad acknowledgment
+ */
+ default void handleReceiveCommandAckInfo(
+ Session session, String command, ScpAckInfo ackInfo)
+ throws IOException {
+ ackInfo.validateCommandStatusCode(command, "receive");
+ }
+
/**
* @param session The client/server {@link Session} through which the
transfer is being executed
* @param op The {@link FileOperation}
diff --git
a/sshd-scp/src/main/java/org/apache/sshd/scp/common/helpers/ScpAckInfo.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/common/helpers/ScpAckInfo.java
index 9b0b986a1..2451094ce 100644
--- a/sshd-scp/src/main/java/org/apache/sshd/scp/common/helpers/ScpAckInfo.java
+++ b/sshd-scp/src/main/java/org/apache/sshd/scp/common/helpers/ScpAckInfo.java
@@ -38,6 +38,8 @@ public class ScpAckInfo {
public static final int WARNING = 1;
public static final int ERROR = 2;
+ public static final ScpAckInfo OK_ACK_INFO = new ScpAckInfo(OK, null);
+
private final int statusCode;
private final String line;
@@ -77,10 +79,12 @@ public class ScpAckInfo {
int code = getStatusCode();
String l = getLine();
// OK code has no line
- if ((code == OK) || GenericUtils.isEmpty(l)) {
+ if (code == OK) {
return Integer.toString(code);
+ } else if ((code == WARNING) || (code == ERROR)) {
+ return GenericUtils.isEmpty(l) ? Integer.toString(code) : code +
": " + l;
} else {
- return code + ": " + l;
+ return l;
}
}
@@ -94,7 +98,7 @@ public class ScpAckInfo {
}
if (statusCode == OK) {
- return new ScpAckInfo(statusCode); // OK status has no extra data
+ return OK_ACK_INFO; // OK status has no extra data
}
String line = ScpIoUtils.readLine(in, cs);
diff --git a/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpCommand.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpCommand.java
index 41fc6b2d6..b7f8847b6 100644
--- a/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpCommand.java
+++ b/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpCommand.java
@@ -176,7 +176,7 @@ public class ScpCommand extends AbstractFileSystemCommand
implements ServerChann
session, getInputStream(), getOutputStream(), fileSystem,
opener, listener);
try {
if (optT) {
- helper.receive(helper.resolveLocalPath(path), optR, optD,
optP, receiveBufferSize);
+ helper.receive(command, helper.resolveLocalPath(path), optR,
optD, optP, receiveBufferSize);
} else if (optF) {
helper.send(Collections.singletonList(path), optR, optP,
sendBufferSize);
} else {
diff --git a/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpShell.java
b/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpShell.java
index 316410d50..98ead95bf 100644
--- a/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpShell.java
+++ b/sshd-scp/src/main/java/org/apache/sshd/scp/server/ScpShell.java
@@ -272,7 +272,7 @@ public class ScpShell extends AbstractFileSystemCommand
implements ServerChannel
ls(argv);
break;
case "scp":
- scp(argv);
+ scp(command, argv);
break;
case "groups":
variables.put(STATUS, 0);
@@ -414,7 +414,7 @@ public class ScpShell extends AbstractFileSystemCommand
implements ServerChannel
variables.put(STATUS, (varValue == null) ? 1 : 0);
}
- protected void scp(String[] argv) throws Exception {
+ protected void scp(String command, String[] argv) throws Exception {
boolean optR = false;
boolean optT = false;
boolean optF = false;
@@ -471,11 +471,11 @@ public class ScpShell extends AbstractFileSystemCommand
implements ServerChannel
return;
}
- doScp(path, optR, optT, optF, optD, optP);
+ doScp(command, path, optR, optT, optF, optD, optP);
}
protected void doScp(
- String path, boolean optR, boolean optT, boolean optF, boolean
optD, boolean optP)
+ String command, String path, boolean optR, boolean optT, boolean
optF, boolean optD, boolean optP)
throws Exception {
try {
ChannelSession channel = getServerChannelSession();
@@ -484,7 +484,7 @@ public class ScpShell extends AbstractFileSystemCommand
implements ServerChannel
fileSystem, opener, listener);
Path localPath = currentDir.resolve(path);
if (optT) {
- helper.receive(localPath, optR, optD, optP, receiveBufferSize);
+ helper.receive(command, localPath, optR, optD, optP,
receiveBufferSize);
} else {
helper.send(Collections.singletonList(localPath.toString()),
optR, optP, sendBufferSize);
}
diff --git a/sshd-scp/src/test/java/org/apache/sshd/scp/client/ScpTest.java
b/sshd-scp/src/test/java/org/apache/sshd/scp/client/ScpTest.java
index 42e274b9a..dce02f74f 100644
--- a/sshd-scp/src/test/java/org/apache/sshd/scp/client/ScpTest.java
+++ b/sshd-scp/src/test/java/org/apache/sshd/scp/client/ScpTest.java
@@ -823,6 +823,29 @@ public class ScpTest extends AbstractScpTestSupport {
}
}
+ @Test // see GH-428
+ public void testMissingRemoteFile() throws Exception {
+ Path targetPath = detectTargetFolder();
+ Path scpRoot = CommonTestSupportUtils.resolve(targetPath,
+ ScpHelper.SCP_COMMAND_PREFIX, getClass().getSimpleName(),
getCurrentTestName());
+ Path localDir =
assertHierarchyTargetFolderExists(scpRoot.resolve("local"));
+ Path remoteDir =
assertHierarchyTargetFolderExists(scpRoot.resolve("remote"));
+ Path missingLocal = localDir.resolve("missing.txt");
+ Path missingRemote = remoteDir.resolve(missingLocal.getFileName());
+ Files.deleteIfExists(missingLocal);
+ Files.deleteIfExists(missingRemote);
+ assertFalse("Remote file not deleted", Files.exists(missingRemote));
+
+ String remotePath =
CommonTestSupportUtils.resolveRelativeRemotePath(targetPath.getParent(),
missingRemote);
+ try (CloseableScpClient scp = createCloseableScpClient()) {
+ scp.download(remotePath, missingLocal.toString());
+ fail("Unexpected success to copy non-existent remote file");
+ } catch (ScpException e) {
+ assertEquals("Mismatched SCP failure code", ScpAckInfo.ERROR,
e.getExitStatus().intValue());
+ }
+
+ }
+
@Test
public void testJschScp() throws Exception {
com.jcraft.jsch.Session session = getJschSession();
