oscerd commented on code in PR #27119:
URL: https://github.com/apache/camel/pull/27119#discussion_r4165198373
##########
components/camel-google/camel-google-storage/src/main/java/org/apache/camel/component/google/storage/GoogleCloudStorageConsumer.java:
##########
@@ -360,13 +360,30 @@ protected String evaluateFileExpression(Exchange
exchange, String downloadFileNa
// use blob as file name
exchange.getMessage().setHeader(GoogleCloudStorageConstants.FILE_NAME,
blogName);
+ // the local path is resolved from
GoogleCloudStorageConstants.FILE_NAME set above, which carries the remote
+ // object name and is therefore untrusted input, no matter whether the
token is appended here or already part
+ // of the configured downloadFileName. An absolute object name, or one
with a .. segment, is rejected whatever
+ // the configuration looks like, and the resolved path is then
confined:
+ // - plain directory (no expression): the object name is appended to
it and the configured value itself is the
+ // directory the download must stay within
+ // - directory followed by an expression (for example
/tmp/downloads/${file:name}): the static directory
+ // prefix before the first expression token is the directory the
download must stay within
+ // - fully dynamic value with no static directory prefix (for example
${file:name}): the route author did not
+ // configure any directory, so a relative result must stay within
the working directory
+ if (blogName != null) {
+ GoogleCloudStorageFileNameHelper.assertSafeObjectName(blogName);
+ }
Review Comment:
You're right, and the failed poll was the bigger problem: one object with
such a name stopped the rest of the bucket from being consumed, which is a lot
more than "objects with such names change behaviour".
I went with your second option, in `99486e0a4298`. The strict name check
stays, since rejecting absolute and `..` names whatever the template looks like
is the intended behaviour (and is what the upgrade guide describes). What
changed is that a rejected object is now handled on its own:
`createExchanges(List<Blob>)` reports it through
`getExceptionHandler().handleException(...)` and skips it, so the other objects
of the poll are consumed as usual. The exchange already created for the
rejected object is released rather than left behind.
Your example is now a test
(`objectWithARejectedNameIsSkippedWithoutStoppingTheOthers`): with
`downloadFileName=<dir>/${exchangeId}.bin` and objects `a.txt`, `b/../c.txt`
and `d.txt`, the exchanges for `a.txt` and `d.txt` are created and `b/../c.txt`
is reported once. The upgrade guide now also says what happens to such an
object: it is skipped and reported on every poll until it is removed or renamed.
On `hasDriveLetter`: intended. The check is deliberately the same on every
OS, so a bucket consumed from Windows and Linux deployments behaves the same,
and `C:report.txt` is rejected on Linux as well, as the upgrade guide says.
_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]
