Croway commented on code in PR #29855:
URL: https://github.com/apache/camel/pull/29855#discussion_r4568597635
##########
components/camel-file/src/main/java/org/apache/camel/component/file/strategy/GenericFileProcessStrategySupport.java:
##########
@@ -127,6 +128,12 @@ public void
setExclusiveReadLockStrategy(GenericFileExclusiveReadLockStrategy<T>
}
protected GenericFile<T> renameFile(GenericFileOperations<T> operations,
GenericFile<T> from, GenericFile<T> to) {
+ if (operations instanceof FileOperations fileOperations) {
+ // check before the existing target is deleted, its parent
directories are created and the file is renamed,
+ // as a symbolic link on the target path could otherwise move the
file outside the starting directory
+ fileOperations.jailMoveToStartingDirectory(new
File(to.getAbsoluteFilePath()));
Review Comment:
**A refused commit move or `moveFailed` makes the file be processed again on
every poll**
This check runs in `commit` and `rollback`, after the route has already
processed the file. When it throws, `GenericFileOnCompletion` only passes the
error to the exception handler and removes the file from the in-progress
repository. The file is still in the inbox, so the next poll picks it up again,
with no limit.
Reproduced against 28d47042 (`initialDelay=0&delay=100`, route
`convertBodyTo(String)` → `mock:result`):
| Setup | Deliveries of the same file within ~0.5 s |
|---|---|
| `inbox/.camel` is a link to a directory outside `inbox` | 5 (5 errors,
file stays in `inbox`) |
| `moveFailed=failed`, `inbox/failed` is a link outside, route throws | 5 |
| No link (control) | 1 |
The `moveFailed` case is the worst one, as users configure `moveFailed`
precisely to stop a failing file from being retried.
Any failed commit move already behaves like this, but this check fails on
every poll, for good. Where the target can be evaluated up front, checking it
when the file is listed or in `begin` and skipping the file (as
`FileConsumer.isWithinStartingDirectory` does for a listed link that resolves
outside) would avoid processing the file at all. Where it cannot (the
expression depends on the exchange), the docs should at least say the file is
consumed again on every poll.
_Claude Code on behalf of Federico Mariani_
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_18.adoc:
##########
@@ -77,6 +77,16 @@ at `DEBUG` level with the message only. Checkpoint failures
triggered by batch s
The webhook consumer still logs the body it validates at `DEBUG`, but no
longer the signature from the `X-Signature` header, nor the signature it
calculates.
The `InvalidMessageSignatureException` of a failed check no longer carries the
calculated signature: `getCalculatedSignature()` returns `null` and is
deprecated, as is the constructor that takes it.
+=== camel-file - the consumer's move, preMove and moveFailed resolve symbolic
links for jailStartingDirectory
+
+With `jailStartingDirectory` enabled (the default), the file consumer now
resolves symbolic links on the target of
+`move`, `preMove` and `moveFailed`, including the default `.camel` folder,
when that target is inside the starting
+directory. A target that resolves outside the starting directory through a
link is not used: the move fails like any
+other failed move, so the file stays where it is and the error is passed to
the exception handler. Before, the existing
Review Comment:
**Behaviour change in patch releases for links set up on purpose, and the
guide does not say the file is processed again**
Some deployments keep processed files on another volume by linking `.camel`
(or `done` with `move=done`). That link is set up by the operator, who is
trusted under the security model. After upgrading to 4.18.6 / 4.22.3, every
file consumed in such a setup fails its move and is processed again on every
poll (see the comment on `GenericFileProcessStrategySupport`).
Reproduced with `inbox/.camel` as a link to an `archive-volume` directory:
- `jailStartingDirectory=false` (the code path before this PR, as the new
check returns early): delivered once, file archived.
- Default: delivered 5 times within ~0.5 s, never archived.
"the file stays where it is" reads as harmless. I'd suggest saying
explicitly that the file is consumed again on every poll until the link is
removed or `jailStartingDirectory=false` is set, in the 4.18, 4.22 and 4.24
guides.
_Claude Code on behalf of Federico Mariani_
##########
components/camel-file/src/main/java/org/apache/camel/component/file/strategy/GenericFileProcessStrategySupport.java:
##########
@@ -127,6 +128,12 @@ public void
setExclusiveReadLockStrategy(GenericFileExclusiveReadLockStrategy<T>
}
protected GenericFile<T> renameFile(GenericFileOperations<T> operations,
GenericFile<T> from, GenericFile<T> to) {
+ if (operations instanceof FileOperations fileOperations) {
Review Comment:
**Producer renames and deletes bypass this check (predates this PR, from
CAMEL-25412, but relevant to where the check lives)**
Because the check sits in the consumer strategy base class, the producer's
own `deleteFile`/`renameFile` calls are not covered. Two cases reproduced
against 28d47042, both changing files outside the starting directory without
any error:
1. **`moveExisting`**:
`file:out?fileExist=Move&moveExisting=archive/${file:onlyname}`, `out/archive`
is a link to `outside/`, and both `out/cfg.txt` and `outside/cfg.txt` exist.
Writing `cfg.txt`: `existsFile`/`deleteFile` in
`GenericFileDefaultMoveExistingFileStrategy` delete the real `outside/cfg.txt`,
and the old `out/cfg.txt` is renamed onto it.
2. **`tempFileName`**: `file:out?tempFileName=../tmp-${file:onlyname}` with
`CamelFileName=sub/hello.txt`, `out/sub` is a link to `outside/`. The temp file
`out/tmp-hello.txt` passes the check, then the eager delete and
`operations.renameFile(tempTarget, target)` in `GenericFileProducer` replace
`outside/hello.txt` with the new content. The same target without
`tempFileName` is refused by `storeFile` ("Cannot write file: … as it resolves
outside the starting directory").
Neither is a regression from this PR, so a follow-up is fine. A single check
in `FileOperations` (or a no-op hook on `GenericFileOperations` that
`FileOperations` overrides) that runs before a target is deleted or renamed
onto would cover these callers and the consumer strategies in one place,
without the `instanceof` in the generic base class. In both producer paths the
existing target is deleted before the rename, so the check has to run before
the delete.
_Claude Code on behalf of Federico Mariani_
--
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]
