oscerd commented on code in PR #28214:
URL: https://github.com/apache/camel/pull/28214#discussion_r4387226933
##########
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:
Updated `SftpOperations.isTransportError()` to use a whitelist of connection
errors (`SSH_FX_NO_CONNECTION` and `SSH_FX_CONNECTION_LOST`), treating
application-level errors as non-transport.
_Claude Code on behalf of oscerd_
##########
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:
Updated `MinaSftpOperations.isTransportError()` to whitelist
`SSH_FX_NO_CONNECTION` and `SSH_FX_CONNECTION_LOST`.
_Claude Code on behalf of oscerd_
##########
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:
Added test coverage for `isTransportError` in both
`RemoteFileConsumerStartingDirectoryJailTest` and
`MinaSftpOperationsDisconnectTest` covering disconnected state, whitelisted
transport errors, and application error status codes.
_Claude Code on behalf of oscerd_
--
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]
