allthingssecurity opened a new pull request, #27522: URL: https://github.com/apache/camel/pull/27522
# Description [CAMEL-25412](https://issues.apache.org/jira/browse/CAMEL-25412) Consistency follow-up to CAMEL-24627 (and CAMEL-24487). Thanks @oscerd for filing the issue. `jailStartingDirectory` (default `true`) is checked lexically for the file producer (`GenericFileProducer.createFileName`) and was a no-op for the local file consumer (`GenericFileConsumer.isWithinStartingDirectory` returns `true` and `FileConsumer` did not override it), while the file I/O follows symbolic links. CAMEL-24627 already made the same option resolve links for `localWorkDirectory` downloads, and CAMEL-24487 gave the remote consumers their own containment check. This change brings the local file producer target and the local consumer listing to the same level: - `GenericFileHelper.isWithinDirectoryResolvingLinks(Path, Path)` (new, public) reuses the CAMEL-24627 helper `resolveExistingPathSegments` (now package-private) on both paths. - `FileConsumer` overrides `isWithinStartingDirectory`: a listed file, or with `recursive=true` a listed directory, whose resolved path lies outside the resolved starting directory is skipped. A link that cannot be resolved (dangling) is skipped as well. The starting directory is resolved once per poll. - `FileOperations.storeFile` and `storeFileDirectly` (the checksum file of `checksumFileAlgorithm`) reject a target that is lexically inside the starting directory but resolves outside of it, including a dangling link, with `GenericFileOperationFailedException`. Targets that are lexically outside the starting directory are not checked, so a `tempFileName` such as `../work/...` keeps working (CAMEL-15544, `FileProduceTempFileNameTest.testParentTempFileName`). - `GenericFileConsumer.isValidFile` logs a file skipped by the containment check at WARN the first time and at DEBUG on later polls (a bounded LRU of 1000 paths, cleared on stop), as such a file usually stays in place and is listed again by every poll. This applies to the remote file consumers too. Links that resolve inside the starting directory, and a starting directory that is itself reached through a link, work as before. As with CAMEL-24627 there is no new option; `jailStartingDirectory=false` keeps the previous behaviour (for the producer it also turns off the lexical `../` check). Upgrade-guide note under a new `=== camel-file - jailStartingDirectory resolves symbolic links` placed next to the CAMEL-24627 entry, and a short section in `file-component.adoc` (catalog copy updated). Cost: with `jailStartingDirectory=true`, one `toRealPath` per listed entry for the local consumer (plus one per poll for the starting directory), and two per written file (and per checksum file) for the producer. Remote producers and consumers are unchanged apart from the log level of repeated skips. Limits (not changed here): - The check and the following read or write are not atomic; a complete solution would open with `NOFOLLOW_LINKS`, which is a larger change. - With `autoCreate=true` the producer creates missing parent directories before `storeFile` runs, so for a name like `link/new/x.txt` the empty directory `new` can be created in the link target before the write is rejected (from reading the code, not tested). - The rename of a `tempFileName` to the final name is not checked. With the default rename it replaces a link at the final name rather than following it. The temp file is checked when it is written, unless it is lexically outside the starting directory: the temp name is placed next to the target, so only a `tempFileName` that climbs out by more levels than the target is deep (for example `../../work/${file:onlyname}` for `link/x.txt`) would be written outside and then renamed through the linked directory. - Consumer moves (`move`, `preMove`, `moveFailed`) are not checked. Tests (camel-core, package-private JUnit 5, no sleeps; each test aborts through an assumption where symbolic links cannot be created, such as Windows without the privilege): - `FileProducerJailStartingDirectorySymlinkTest` (8): a directory link, a file link, a dangling link, `link/../x.txt`, and a checksum file link to outside are rejected and nothing outside is written; links inside the starting directory are followed; `../` is still rejected by the lexical check; `jailStartingDirectory=false` follows a directory link. - `FileConsumerJailStartingDirectorySymlinkTest` (5): a file link to outside is not part of the batch and is reported at WARN once over at least three polls (Awaitility on the captured log events); a directory link to outside is not entered with `recursive=true`; file and directory links inside are consumed; a starting directory reached through a link is consumed; `jailStartingDirectory=false` consumes a link to outside. On main's production code (run twice, same result each time) 7 of the 13 tests fail; the other 6 are the controls above and pass on main by design: ``` FileConsumerJailStartingDirectorySymlinkTest.directoryLinkToOutsideIsNotEnteredWhenRecursive ... exchangeProperty(CamelBatchSize) == 1 evaluated as: 2 == 1 FileConsumerJailStartingDirectorySymlinkTest.fileLinkToOutsideIsSkippedAndReportedOnce mock://result Body of message: 0. Expected: <Hello World> but was: <Outside> FileProducerJailStartingDirectorySymlinkTest.checksumFileLinkToOutsideIsRejected ... Expected org.apache.camel.CamelExecutionException to be thrown, but nothing was thrown. FileProducerJailStartingDirectorySymlinkTest.danglingLinkToOutsideIsRejected ... (same) FileProducerJailStartingDirectorySymlinkTest.directoryLinkToOutsideIsRejected ... (same) FileProducerJailStartingDirectorySymlinkTest.fileLinkToOutsideIsRejected ... (same) FileProducerJailStartingDirectorySymlinkTest.parentSegmentAfterDirectoryLinkIsRejected ... (same) ``` With the change: camel-core `org.apache.camel.component.file.**` 457 tests pass (12 skipped, none of them new), camel-file 24 pass. camel-ftp: 80 unit tests (9 skipped) and 374 integration tests against the embedded FTP/FTPS/SFTP servers (17 skipped) pass, including `RemoteFileConsumerStartingDirectoryJailTest` and `FtpProducerJailStartingDirectoryIT`. The camel-smb tests need a Samba container and were skipped here (no Docker). Run on macOS with JDK 21. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested the affected modules with `-am`, including the formatter and import-sort plugins, and copied the changed component doc to the catalog. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
