gnodet-bot commented on code in PR #28214:
URL: https://github.com/apache/camel/pull/28214#discussion_r4386999229
##########
components/camel-ftp/src/main/java/org/apache/camel/component/file/remote/SftpOperations.java:
##########
@@ -939,6 +939,91 @@ public SftpRemoteFile[] listFiles(String path) throws
GenericFileOperationFailed
}
}
+ @Override
+ public String getHomeDirectory() throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ return channel.getHome();
+ } catch (SftpException e) {
+ throw new GenericFileOperationFailedException("Cannot get the home
directory", e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public boolean isSymbolicLink(String path) throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ return channel.lstat(quotePathForJsch(path)).isLink();
+ } catch (SftpException e) {
+ throw new GenericFileOperationFailedException("Cannot get the
attributes of: " + path, e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public String readSymbolicLink(String path) throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ return channel.readlink(quotePathForJsch(path));
+ } catch (SftpException e) {
+ throw new GenericFileOperationFailedException("Cannot read the
symbolic link: " + path, e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public boolean isTransportError(GenericFileOperationFailedException e) {
+ if (!isConnected()) {
+ return true;
+ }
+ Throwable cause = e.getCause();
+ if (cause instanceof SftpException sftp) {
+ if (sftp.id == ChannelSftp.SSH_FX_NO_SUCH_FILE || sftp.id ==
ChannelSftp.SSH_FX_PERMISSION_DENIED) {
+ return false;
+ }
+ return true;
Review Comment:
💡 **Suggestion — over-broad transport error classification**
The blacklist approach here (`return true` for any `SftpException` that
isn't `NO_SUCH_FILE` / `PERMISSION_DENIED`) over-classifies server-side
application errors as transport errors:
- `SSH_FX_FAILURE` — generic server-side failure; transport is alive
- `SSH_FX_BAD_MESSAGE` — protocol framing error from the server; transport
may still be up
- `SSH_FX_OP_UNSUPPORTED` — server doesn't support the operation; transport
is alive
Only `SSH_FX_NO_CONNECTION` and `SSH_FX_CONNECTION_LOST` are actual
indicators that the transport is down. A whitelist approach would be more
precise:
```suggestion
if (sftp.id == ChannelSftp.SSH_FX_NO_CONNECTION
|| sftp.id == ChannelSftp.SSH_FX_CONNECTION_LOST) {
return true;
}
return false;
```
##########
components/camel-mina-sftp/src/main/java/org/apache/camel/component/file/remote/mina/MinaSftpOperations.java:
##########
@@ -1180,6 +1181,59 @@ public SftpRemoteFile[] listFiles(String path) throws
GenericFileOperationFailed
}
}
+ @Override
+ public String getHomeDirectory() throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ // the server resolves relative paths against its default directory
+ return sftpClient.canonicalPath(".");
+ } catch (IOException e) {
+ throw new GenericFileOperationFailedException("Cannot get the home
directory", e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public boolean isSymbolicLink(String path) throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ return sftpClient.lstat(path).isSymbolicLink();
+ } catch (IOException e) {
+ throw new GenericFileOperationFailedException("Cannot get the
attributes of: " + path, e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public String readSymbolicLink(String path) throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
+ return sftpClient.readLink(path);
+ } catch (IOException e) {
+ throw new GenericFileOperationFailedException("Cannot read the
symbolic link: " + path, e);
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ @Override
+ public boolean isTransportError(GenericFileOperationFailedException e) {
+ if (!isConnected()) {
+ return true;
+ }
+ Throwable cause = e.getCause();
+ if (cause instanceof org.apache.sshd.sftp.common.SftpException sftp) {
+ if (sftp.getStatus() ==
org.apache.sshd.sftp.common.SftpConstants.SSH_FX_NO_SUCH_FILE
+ || sftp.getStatus() ==
org.apache.sshd.sftp.common.SftpConstants.SSH_FX_PERMISSION_DENIED) {
+ return false;
+ }
+ return true;
Review Comment:
💡 **Suggestion — over-broad transport error classification (MINA)**
Same issue as in `SftpOperations`: the blacklist approach returns `true` for
any `SftpException` not matching `NO_SUCH_FILE` / `PERMISSION_DENIED`.
Server-side codes like `SSH_FX_FAILURE`, `SSH_FX_BAD_MESSAGE`,
`SSH_FX_OP_UNSUPPORTED` indicate application-level errors, not transport
failures.
Prefer a whitelist using `SSH_FX_NO_CONNECTION` / `SSH_FX_CONNECTION_LOST`:
```suggestion
if (sftp.getStatus() ==
org.apache.sshd.sftp.common.SftpConstants.SSH_FX_NO_CONNECTION
|| sftp.getStatus() ==
org.apache.sshd.sftp.common.SftpConstants.SSH_FX_CONNECTION_LOST) {
return true;
}
return false;
```
##########
components/camel-ftp/src/test/java/org/apache/camel/component/file/remote/RemoteFileConsumerStartingDirectoryJailTest.java:
##########
@@ -74,4 +92,155 @@ void shouldRejectUpwardsPathsWhenPollingTheSessionRoot()
throws Exception {
// no directory is configured, but navigating above the session root
still escapes
assertFalse(consumer.isWithinStartingDirectory("../secret.txt"));
}
+
+ @Test
+ void shouldNotResolveListedFilesThatAreNotSymbolicLinks() {
+ StubSftpOperations server = new
StubSftpOperations().file("/home/camel/poll/hello.txt");
+ AbstractSftpConsumer consumer = sftpConsumer("sftp://hostname/poll";,
server);
+
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"hello.txt", false), "poll/hello.txt"));
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"sub/hello.txt", false), "poll/sub/hello.txt"));
+ // the path built from the listed name is still checked
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"../hello.txt", false), "poll/../hello.txt"));
+ // without asking the server
+ assertEquals(0, server.lookups);
+ }
+
+ @Test
+ void shouldAcceptSymbolicLinkResolvingInsideTheStartingDirectory() {
+ StubSftpOperations server = new StubSftpOperations()
+ .file("/home/camel/poll/hello.txt")
+ .file("/home/camel/poll/data/report.txt")
+ .link("/home/camel/poll/latest.txt", "hello.txt")
+ .link("/home/camel/poll/report.txt",
"/home/camel/poll/data/report.txt")
+ .link("/home/camel/poll/current", "data")
+ .link("/home/camel/poll/current.txt", "current/report.txt")
+ .link("/home/camel/poll/again.txt", "../poll/latest.txt")
+ .link("/home/camel/poll/data/hello.txt", "../hello.txt");
+ AbstractSftpConsumer consumer = sftpConsumer("sftp://hostname/poll";,
server);
+
+ // relative to the directory of the link, absolute, through a link to
a directory, and to another link
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"latest.txt", true), "poll/latest.txt"));
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"report.txt", true), "poll/report.txt"));
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"current.txt", true), "poll/current.txt"));
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"again.txt", true), "poll/again.txt"));
+ // listed in a sub directory (recursive)
+ assertTrue(consumer.isWithinStartingDirectory(listed("poll",
"data/hello.txt", true), "poll/data/hello.txt"));
+
+ // a starting directory that is itself a link
+ StubSftpOperations linkedServer = new StubSftpOperations()
+ .file("/srv/inbox/hello.txt")
+ .link("/home/camel/inbox", "/srv/inbox")
+ .link("/srv/inbox/latest.txt", "hello.txt");
+ AbstractSftpConsumer linkedConsumer =
sftpConsumer("sftp://hostname/inbox";, linkedServer);
+
+ assertTrue(linkedConsumer.isWithinStartingDirectory(listed("inbox",
"latest.txt", true), "inbox/latest.txt"));
+ }
+
+ @Test
+ void shouldSkipSymbolicLinkResolvingOutsideTheStartingDirectory() {
+ StubSftpOperations server = new StubSftpOperations()
+ .file("/home/camel/poll/hello.txt")
+ .file("/home/camel/archive/report.txt")
+ .file("/home/camel/polling/report.txt")
+ .link("/home/camel/poll/report.txt", "../archive/report.txt")
+ .link("/home/camel/poll/absolute.txt",
"/home/camel/archive/report.txt")
+ .link("/home/camel/poll/archive", "../archive")
+ .link("/home/camel/poll/through.txt", "archive/report.txt")
+ .link("/home/camel/poll/polling.txt", "../polling/report.txt")
+ .link("/home/camel/poll/sub/report.txt",
"../../archive/report.txt");
+ AbstractSftpConsumer consumer = sftpConsumer("sftp://hostname/poll";,
server);
+
+ // relative upwards, absolute, and through a link to a directory
outside
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"report.txt", true), "poll/report.txt"));
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"absolute.txt", true), "poll/absolute.txt"));
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"through.txt", true), "poll/through.txt"));
+ // a sibling directory whose name merely extends the polled directory
name is not contained
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"polling.txt", true), "poll/polling.txt"));
+ // listed in a sub directory (recursive)
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"sub/report.txt", true), "poll/sub/report.txt"));
+ }
+
+ @Test
+ void shouldSkipSymbolicLinkThatCannotBeResolved() {
+ StubSftpOperations server = new StubSftpOperations()
+ .file("/home/camel/poll/hello.txt")
+ .link("/home/camel/poll/missing.txt", "gone.txt")
+ .link("/home/camel/poll/ping.txt", "pong.txt")
+ .link("/home/camel/poll/pong.txt", "ping.txt")
+ .link("/home/camel/poll/empty.txt", "");
+ AbstractSftpConsumer consumer = sftpConsumer("sftp://hostname/poll";,
server);
+
+ // a dangling link, a loop, and a link without a target
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"missing.txt", true), "poll/missing.txt"));
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"ping.txt", true), "poll/ping.txt"));
+ assertFalse(consumer.isWithinStartingDirectory(listed("poll",
"empty.txt", true), "poll/empty.txt"));
+ }
+
+ private static Supplier<GenericFile<SftpRemoteFile>> listed(
+ String directory, String relativeFilePath, boolean symbolicLink) {
+ RemoteFile<SftpRemoteFile> file = new RemoteFile<>();
+ file.setFile(new
SftpRemoteFileSingle(FileUtil.stripPath(relativeFilePath)) {
+ @Override
+ public boolean isSymbolicLink() {
+ return symbolicLink;
+ }
+ });
+ file.setEndpointPath(directory);
+ file.setRelativeFilePath(relativeFilePath);
+ file.setAbsoluteFilePath(directory + "/" + relativeFilePath);
+ return () -> file;
+ }
+
+ /**
+ * The files of an SFTP server whose home directory is /home/camel: the
paths that exist, and the symbolic links
+ * among them.
+ */
+ private static final class StubSftpOperations extends SftpOperations {
+
+ private final Set<String> files = new HashSet<>();
+ private final Map<String, String> links = new HashMap<>();
+ private int lookups;
+
+ StubSftpOperations file(String path) {
+ // the file and the directories it is in
+ for (String name = path; name != null && !"/".equals(name); name =
FileUtil.onlyPath(name)) {
+ files.add(name);
+ }
+ return this;
+ }
+
+ StubSftpOperations link(String path, String target) {
+ file(FileUtil.onlyPath(path));
+ links.put(path, target);
+ return this;
+ }
+
+ @Override
+ public boolean isConnected() {
+ return true;
+ }
+
+ @Override
+ public String getHomeDirectory() {
+ return "/home/camel";
+ }
+
+ @Override
+ public boolean isSymbolicLink(String path) {
+ lookups++;
+ if (links.containsKey(path)) {
+ return true;
+ }
+ if (files.contains(path)) {
+ return false;
+ }
+ throw new GenericFileOperationFailedException("No such file: " +
path);
+ }
+
+ @Override
+ public String readSymbolicLink(String path) {
+ return links.get(path);
+ }
+ }
Review Comment:
💡 **Missing test coverage for `isTransportError`**
`StubSftpOperations` inherits the default `isTransportError()` from
`SftpSymbolicLinkOperations` (which always returns `false`). The
error-classification logic introduced in `SftpOperations.isTransportError()` —
the `!isConnected()` path and the `SftpException` status code checks — has no
unit test coverage.
Consider adding a `@Test` that exercises the different classification paths:
transport-down case (`isConnected() = false`), transport-error codes
(`SSH_FX_NO_CONNECTION` / `SSH_FX_CONNECTION_LOST`), and application-error
codes (`SSH_FX_FAILURE`, `SSH_FX_NO_SUCH_FILE`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
