gnodet-bot commented on code in PR #28214:
URL: https://github.com/apache/camel/pull/28214#discussion_r4377839703
##########
components/camel-ftp-common/src/main/java/org/apache/camel/component/file/remote/AbstractSftpConsumer.java:
##########
@@ -41,8 +45,18 @@ public abstract class AbstractSftpConsumer extends
RemoteFileConsumer<SftpRemote
private static final Logger LOG =
LoggerFactory.getLogger(AbstractSftpConsumer.class);
+ /**
+ * The most symbolic links followed to resolve a path, as a longer chain
is most likely a loop
+ */
+ private static final int MAX_SYMBOLIC_LINKS = 32;
+
protected String endpointPath;
+ // the starting directory with its symbolic links resolved on the server,
computed once per poll
+ private volatile String resolvedStartingDirectory;
+
+ private final Set<String> unresolvableLinks =
ConcurrentHashMap.newKeySet();
Review Comment:
⚠️ **Memory leak / stale log messages:** `unresolvableLinks` is an unbounded
set that is never cleared — not between polls, not in `doStop()`. In contrast,
the parent class's `skippedOutsideStartingDirectory` uses
`LRUCacheFactory.newLRUCache(1000)` to bound growth. On a long-running consumer
polling a server where dangling links are created and removed, this set grows
without bound.
There's also a correctness gap: if a path was previously a dangling link
(added to `unresolvableLinks`), then later replaced by a valid-but-outside-jail
link, the log will report "symbolic link cannot be resolved" instead of
"resolves outside the starting directory" — wrong message, correct outcome
(still skipped), but confusing to operators.
Fix: clear `unresolvableLinks` at the top of `pollDirectory` alongside
`resolvedStartingDirectory` (see also the inline comment on line 98).
##########
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;
+ }
+ return cause != null;
Review Comment:
⚠️ **Over-broad transport error classification:** The fallback `return cause
!= null` classifies any `GenericFileOperationFailedException` with a non-null,
non-`SftpException` cause as a transport error, causing it to be rethrown and
aborting the poll. In practice the JSch code only throws `SftpException`, but
if any future code path wraps a `RuntimeException` (e.g.
`NullPointerException`) in `GenericFileOperationFailedException`, the poll
aborts instead of treating the file as unresolvable.
Transport errors are actual connection/IO failures — tighten to
`IOException`:
```suggestion
return cause instanceof java.io.IOException;
```
##########
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;
+ }
+ return cause != null;
Review Comment:
⚠️ **Same issue as `SftpOperations.isTransportError`:** `return cause !=
null` is too broad. MINA transport failures are `IOException`; tighten to that:
```suggestion
return cause instanceof IOException;
```
##########
components/camel-ftp-common/src/main/java/org/apache/camel/component/file/remote/AbstractSftpConsumer.java:
##########
@@ -80,6 +94,9 @@ protected void doStart() throws Exception {
@Override
protected boolean pollDirectory(Exchange dynamic, String fileName,
List<GenericFile<SftpRemoteFile>> fileList, int depth) {
+ // resolved lazily during this poll on the first symbolic link
encountered, as the starting directory can be replaced between polls
+ resolvedStartingDirectory = null;
Review Comment:
💡 **Clear `unresolvableLinks` per-poll:** Should be cleared here alongside
`resolvedStartingDirectory` to avoid stale entries from previous polls
producing wrong log messages when a path is reused with a different link target.
```suggestion
resolvedStartingDirectory = null;
unresolvableLinks.clear();
```
--
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]
