davsclaus commented on code in PR #26189:
URL: https://github.com/apache/camel/pull/26189#discussion_r3956209701
##########
components/camel-jetty/src/main/java/org/apache/camel/component/jetty12/AttachmentHttpBinding.java:
##########
@@ -59,6 +61,16 @@ protected void populateAttachments(HttpServletRequest
request, Message message)
try {
parts = request.getParts();
for (Part part : parts) {
+ // the whitelist accepts file name extensions, so it must
be checked against the submitted
+ // file name and not against Part.getName(), which is the
multipart field name
+ String fileName = part.getSubmittedFileName();
+ if (!isFileNameAccepted(fileName)) {
+ LOG.debug(
+ "Cannot add file as attachment: {} because the
file is not accepted according to fileNameExtWhitelist: {}",
+ fileName, getFileNameExtWhitelist());
Review Comment:
`fileName` is now `part.getSubmittedFileName()`, which is fully
client-controlled, and it goes into the log raw.
This exact file has a deliberate convention against that, added twice:
`76557040cd10` ("sanitize user values before logging") and `9ab764c15b93`
("prevents potential injections"), both wrapping user-supplied values in
`HttpHelper.sanitizeLog(...)`. `HttpHelper` is already imported here.
`CLAUDE.md`'s Quality Expectations also ask that contributions avoid
introducing new CWE issues, and this is CWE-117 (log injection).
```suggestion
LOG.debug(
"Cannot add file as attachment: {} because
the file is not accepted according to fileNameExtWhitelist: {}",
HttpHelper.sanitizeLog(fileName),
getFileNameExtWhitelist());
```
##########
components/camel-jetty/src/main/java/org/apache/camel/component/jetty12/AttachmentHttpBinding.java:
##########
@@ -67,23 +79,42 @@ protected void populateAttachments(HttpServletRequest
request, Message message)
}
}
AttachmentMessage am = new
DefaultAttachmentMessage(message);
- am.addAttachmentObject(part.getName(), attachment);
- String name = part.getSubmittedFileName();
- Object value = am.getAttachment(name);
- Map<String, Object> headers = message.getHeaders();
- if (getHeaderFilterStrategy() != null
- &&
!getHeaderFilterStrategy().applyFilterToExternalHeaders(name, value,
message.getExchange())
- && name != null) {
- HttpHelper.appendHeader(headers, name, value);
+ String name = part.getName();
+ am.addAttachmentObject(name, attachment);
+ // a file part is also exposed as a header carrying the
DataHandler. The attachment is keyed on
+ // the multipart field name, so the header must be looked
up and named by that same key and not
+ // by the client supplied file name. A plain form field
carries no file name and is mapped by
+ // populateRequestParameters instead, so it is left alone
here.
+ if (fileName != null && name != null) {
+ Object value = am.getAttachment(name);
+ Map<String, Object> headers = message.getHeaders();
+ if (getHeaderFilterStrategy() != null
+ &&
!getHeaderFilterStrategy().applyFilterToExternalHeaders(name, value,
+ message.getExchange())) {
+ HttpHelper.appendHeader(headers, name, value);
+ }
}
-
}
} catch (Exception e) {
throw new RuntimeCamelException("Cannot populate attachments",
e);
}
}
}
+ private boolean isFileNameAccepted(String fileName) {
+ String whitelist = getFileNameExtWhitelist();
+ if (whitelist == null) {
+ return true;
+ }
+ String ext = FileUtil.onlyExt(fileName);
+ if (ext == null) {
+ return true;
+ }
+ ext = ext.toLowerCase(Locale.US);
+ whitelist = whitelist.toLowerCase(Locale.US);
+ return whitelist.equals("*") || whitelist.contains(ext);
Review Comment:
Two follow-up thoughts, neither a blocker for this PR.
**The match is a substring test.** With `fileNameExtWhitelist=txt`, an
upload named `evil.x`, `evil.t` or `evil.tx` is accepted, because
`"txt".contains("x")` is true. Relatedly, `FileUtil.onlyExt` runs in non-single
mode (everything after the *first* dot), so `archive.tar.gz` yields `tar.gz`
and a whitelist of `gz` rejects it. Neither is introduced here — both mirror
`servlet/AttachmentHttpBinding` and `VertxPlatformHttpConsumer` — but the
option just went from "never rejects" to "actually enforces", which makes the
looseness newly reachable. Splitting the whitelist on `,` and comparing tokens
would be the fix; a separate JIRA seems right to keep this PR focused.
**This is now the third copy of the block.** The premise of this PR is that
three implementations disagreed, and `isFileNameAccepted` adds a third copy
(camel-servlet inline, vertx inline, jetty here). Lifting it into
`DefaultHttpBinding` — which both bindings extend — and having camel-servlet
call it too would leave one implementation and prevent the next drift. Arguably
out of scope, but it is the natural home.
##########
components/camel-servlet/pom.xml:
##########
@@ -69,6 +69,11 @@
<artifactId>camel-test-junit6</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Could the new camel-servlet test use JUnit assertions instead, so this
dependency isn't needed?
camel-servlet currently has 42 test classes using
`org.junit.jupiter.api.Assertions` and none using AssertJ — the closest
sibling, `MultipartUploadTest`, uses `assertEquals`. `CLAUDE.md` is explicit
here: *"if they are predominantly JUnit assertions, write new tests in that
same JUnit style rather than introducing AssertJ as an outlier"*.
`project-standards.md` also asks not to add dependencies without justification,
and switching the two `assertThat(...).isEqualTo(...)` calls in
`MultipartUploadFileNameExtWhitelistTest` to `assertEquals` removes the need
for this block entirely.
Same reasoning applies to `MultiPartFormFileNameExtWhitelistTest` on the
camel-jetty side (180 JUnit test classes vs 1 AssertJ), though the dependency
is already present there so the cost is lower.
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc:
##########
@@ -1713,3 +1713,27 @@ Routes that set `useRecovery=false` are unaffected.
Routes that left recovery en
default, stop seeing in-progress aggregations re-delivered, and start seeing
genuine recovery. The cache
now also holds one entry per completed and not yet confirmed exchange; those
entries are removed on
confirmation.
+
+=== camel-servlet, camel-jetty - the multipart upload whitelist is enforced
against the submitted file name
+
+`fileNameExtWhitelist` accepts file name extensions, but `camel-servlet`'s
`AttachmentHttpBinding`
+checked it against `Part.getName()`, which is the multipart *field* name
rather than the submitted
+file name. A field named `file` carries no extension, so the check found
nothing to compare and every
+upload was accepted. The option is now checked against
`Part.getSubmittedFileName()`, which is what
+`camel-platform-http-vertx` already does.
+
+A `camel-servlet` consumer that sets `fileNameExtWhitelist` together with
`attachmentMultipartBinding=true`
+therefore starts rejecting uploads whose file extension is not listed, which
is what the option always
+advertised. Uploads with no file name, such as plain form fields, are
unaffected, and a route that does
+not set the option is unaffected. Review the configured extension list before
upgrading.
+
+The `camel-jetty` binding performed no whitelist check at all, although
`fileNameExtWhitelist` can be
+set on its `HttpBinding`. It now applies the same check.
+
+The `camel-jetty` binding also stored the attachment under the multipart field
name but looked it up
+again by the submitted file name, and passed that file name to
`HttpHelper.appendHeader`. The lookup
+therefore only succeeded when the two happened to be equal, and when it did
the header was named by
+the client-supplied file name. The attachment is now looked up and exposed
under the field name it is
+stored with, and only for parts that carry a file name — a plain form field is
mapped by
+`populateRequestParameters` as before. A route that read the attachment header
under the uploaded file
+name must read it under the multipart field name instead.
Review Comment:
Small wording nit: as written this reads as though a working behaviour is
being taken away, but when the two names differed the old lookup returned
`null`, so the header was set under the file name with a **null** value — no
route could have read a usable `DataHandler` there. When the names were equal,
this PR doesn't change the header name at all.
```suggestion
`populateRequestParameters` as before. A route that read the attachment
header under the uploaded file
name was in fact reading a `null` value, because that lookup always missed;
the header must now be read
under the multipart field name. A client-supplied file name no longer
becomes a header name.
```
--
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]
