oscerd commented on code in PR #26661:
URL: https://github.com/apache/camel/pull/26661#discussion_r4080634049
##########
components/camel-mail/src/main/java/org/apache/camel/dataformat/mime/multipart/MimeMultipartDataFormat.java:
##########
@@ -417,7 +418,12 @@ private String getAttachmentKey(BodyPart bp) throws
MessagingException, Unsuppor
if (key == null) {
key = UUID.randomUUID() + "@camel.apache.org";
}
- return MimeUtility.decodeText(key);
+ key = MimeUtility.decodeText(key);
+ // The name is chosen by the sender of the message being unmarshalled,
so normalise it the same
+ // way MailBinding.extractAndNormalizeFileName does before it is used
to identify the attachment:
+ // strip control characters, then reduce it to a leaf name so it
cannot carry path components.
+ key = key.replaceAll("[\n\r\t]", "_");
+ return FileUtil.stripPath(key);
Review Comment:
Fixed in 55b4ad6 — `getAttachmentKey` now reduces the key to a leaf name
only when it comes from the sender-chosen file name. A Content-ID (or a
generated id) is decoded but left intact, so `<a/[email protected]>` keeps its
`/` and `cid:` references in a `multipart/related` message are not broken.
Added `unmarshalContentIdKeyKeepsSlashesBecauseItIsNotAPath`, which fails
against the strip-everything version (verified), while the file-name traversal
test still passes.
_Claude Code on behalf of oscerd_
##########
components/camel-mail/src/main/java/org/apache/camel/component/mail/MailBinding.java:
##########
@@ -724,8 +725,12 @@ protected void addAttachmentsToMultipart(
LOG.trace("Attachment #{}: Using content type
resolver: {} resolved content type as: {}", i,
contentTypeResolver, contentType);
if (contentType != null) {
- String value = contentType + "; name=" +
attachmentFilename;
- messageBodyPart.setHeader("Content-Type", value);
+ // The file name comes from the message being
relayed, so it must go out as a
+ // parameter value rather than be concatenated
into the header. ParameterList
+ // quotes and escapes anything that would
otherwise change the header's structure.
+ ContentType parsed = new ContentType(contentType);
Review Comment:
Good question — to be precise about the before/after: previously
`contentType + "; name=" + attachmentFilename` was written to the header raw,
so an unparsable content type produced an *invalid* `Content-Type` header. That
was "successfully ignored" only when the receiving mail client happened to be
lenient; a strict one failed later. The new parse both closes a
header-injection vector (the sender-chosen file name was being concatenated
into the header structure) and, as @davsclaus noted, surfaces a genuinely
invalid content type at marshal time rather than downstream.
I've kept the fail-fast — reverting to raw concatenation would reopen the
injection — and documented the behaviour change in the 4.23 upgrade guide (new
`camel-mail` note), so a custom `ContentTypeResolver` returning an unparsable
value is a known migration point. Ordinary resolvers returning valid MIME types
are unaffected.
_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]
