oscerd commented on code in PR #26189:
URL: https://github.com/apache/camel/pull/26189#discussion_r3957442029
##########
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:
Fixed — `HttpHelper.sanitizeLog(fileName)` is now used in the jetty
binding's LOG.debug. Thanks, this was a genuine CWE-117 introduced by switching
to the client-controlled submitted file name.
_Claude Code on behalf of oscerd_
##########
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:
Done — both new tests use JUnit assertions to match the surrounding modules,
and the `assertj-core` dependency has been dropped from `camel-servlet/pom.xml`
entirely.
_Claude Code on behalf of oscerd_
##########
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:
Reworded as suggested — the guide now says the old lookup always missed and
returned `null`, rather than implying a working behaviour is being removed.
_Claude Code on behalf of oscerd_
##########
components/camel-servlet/src/main/java/org/apache/camel/component/servlet/AttachmentHttpBinding.java:
##########
@@ -54,7 +54,9 @@ protected void populateAttachments(HttpServletRequest
request, Message message)
try {
Collection<Part> parts = request.getParts();
for (Part part : parts) {
- String fileName = part.getName();
+ // 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();
Review Comment:
Fixed — `org.apache.camel.http.common.HttpHelper` is now imported in the
servlet binding and `HttpHelper.sanitizeLog(fileName)` is used in the LOG.debug
block.
_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]
