oscerd commented on PR #28214:
URL: https://github.com/apache/camel/pull/28214#issuecomment-6477910997
Thanks Claus. There are two new commits on top of 2c285fb99d. The numbers
below follow your review.
**1. Link targets that use `\` (blocking)**: `CAMEL-25886: camel-ftp - skip
a symbolic link whose target uses a backslash`
A target that contains `\` can no longer be resolved.
`resolveSymbolicLinks()` throws for it, so the entry is skipped like a dangling
link and logged as "symbolic link cannot be resolved". The check sits in
`AbstractSftpConsumer`, so it covers both `sftp` and `mina-sftp`.
I did not split on both separators. On a server where `\` is part of a name,
the extra segments can cancel a real `..`. For example, `x\y/../../z`
normalizes to a path inside the starting directory when split on both. On such
a server, though, `x\y` is one directory, so the server resolves the target one
level above the starting directory. Which reading applies depends on the
server, so skipping the link is the only safe choice.
New test:
`RemoteFileConsumerStartingDirectoryJailTest.shouldSkipSymbolicLinkWhoseTargetContainsBackslash`.
It uses two stubs:
- One takes `\` as a separator. It has `..\..\outside.txt`, which that
server follows outside, and `sub\file.txt`, which it follows inside.
- One takes `\` as part of a name, with `a\b.txt` present inside.
All three links are skipped and reported as unresolvable. With the check
disabled, I confirmed separately that each of the three is accepted. The sftp
and mina-sftp pages and the 4.24 upgrade note now say that such a link is
skipped.
**2. JSch side of `isTransportError()`**: `CAMEL-25886: camel-ftp - review
follow-ups`
I confirmed this against jsch 2.28.8:
- `lstat()`, `readlink()` and `getHome()` turn any exception that is not an
`SftpException` into `new SftpException(SSH_FX_FAILURE, e.toString(), e)`.
- A server status becomes `new SftpException(id, message)`, without a cause.
- JSch never raises `SSH_FX_NO_CONNECTION` or `SSH_FX_CONNECTION_LOST`
itself. They appear only as constants and in the code-to-name mapping.
So `SftpOperations.isTransportError()` is now `!isConnected()`, or a cause
that is an `SftpException` with a cause of its own, as you suggested. I dropped
the two codes on the JSch side instead of keeping them. JSch's own comment on
these constants says they can only be generated locally and must not be
returned by servers. Since JSch never generates them, a JSch exception with
either code can only be a status reply, and then the server did answer.
`isTransportErrorClassification` now uses `new SftpException(SSH_FX_FAILURE,
"...", new IOException("broken pipe"))`, and it fails without the change.
Status-only exceptions, including the two pseudo-errors, are not transport
errors. On the `mina-sftp` side, only the imports changed.
**3. Tests**: same commit
Both `SftpConsumerJailStartingDirectorySymlinkIT` and
`MinaSftpConsumerJailStartingDirectorySymlinkIT` have three new ITs against the
embedded server:
- `consumesLinksNamedWithWildcardsResolvingInsideStartingDirectory`: links
named `star*.txt` and `what?.txt` that point to `hello.txt` are consumed.
- `consumesLinksResolvingInsideThroughWildcardNamedDirectories`: `star.txt`
→ `sub*dir/report.txt` and `question.txt` → `sub?dir/report.txt` are consumed,
with both directories inside. Read as patterns, `sub*dir` and `sub?dir` each
match both directories. Without the JSch quoting, the lookups are therefore not
unique, both links are skipped, and the sftp IT fails (1 message of 3).
mina-sftp passes either way, because it uses literal paths.
- `skipsLinksNamedWithWildcardsResolvingOutsideStartingDirectory`: these are
all skipped:
- `star*.txt` and `what?.txt` pointing outside
- an `arch*ve` link to the directory outside
- `latest.txt` → `arch*ve/report.txt`
A name without `\` always matches itself as a pattern. So for a link whose
own name contains `*` or `?`, an unquoted lookup either finds the same entry,
or fails as not unique and the link is skipped. That is why the named-link ITs
pass with or without the quoting, and the IT that resolves through such a name
is the one that pins it.
Because of point 1,
`skipsLinkWhoseTargetStepsThroughBackslashNamedLinkOutsideStartingDirectory`
now skips `latest.txt` before any lookup. That IT still guards the case, but it
no longer depends on the quoting.
**Nits**
- `SftpOperations`: the fully qualified `java.io.IOException` went away with
the rewrite of `isTransportError()`.
- `MinaSftpOperations`: `SftpException` and `SftpConstants` are now imports.
- `GenericFileConsumer.isValidFile()`: picks the message once and logs it at
WARN or DEBUG. The texts are unchanged.
`FileConsumerJailStartingDirectorySymlinkTest` in camel-core, which matches on
the WARN text, passes.
All of these pass:
- unit tests of camel-ftp (136, 9 skipped)
- unit tests of camel-mina-sftp (31, 1 skipped)
- both jail ITs (12 each)
The full `mvn clean install -DskipTests -DskipITs` passes, and the
regenerated catalog copies of the two pages are committed.
_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]
