Encode non-ASCII SMTP attachment file names according to RFC 2231 - #6757
ashrafiucse wants to merge 2 commits into
Conversation
The SMTP Sampler used javax.mail 1.5.0-b01, which mangles attachment file names that contain non-ASCII characters (for example a file named "текст.txt" was sent as "B5:AB.txt"), so recipients could not see the original file name. Update javax.mail to 1.6.2 (com.sun.mail:javax.mail), which encodes non-ASCII file name parameters according to RFC 2231. The javax.mail namespace is unchanged, so no code changes are needed. Closes apache#6652
vlsi
left a comment
There was a problem hiding this comment.
The upgrade fixes the attachment name only when the JVM default charset is UTF-8, and the release build fails on the jar list. The inline comments cover the diff; the points below concern code and behavior outside it.
The file name is still mangled on Java 17 with a non-UTF-8 default charset. MimeBodyPart.setFileName encodes the parameter with MimeUtility.getDefaultMIMECharset(), which reads the mail.mime.charset system property and falls back to file.encoding. JMeter 6 supports Java 17, where the Windows default is windows-1252. javax.mail 1.6.2 on Java 17 with -Dfile.encoding=Cp1252 produces:
Content-Type: text/plain; charset=us-ascii; name="?????.txt"
Content-Disposition: attachment; filename*=Cp1252''%3F%3F%3F%3F%3F.txt
This is the symptom from #6652 in another form, and Cp1252 is not an IANA charset name either. Please make SendMailCommand (the attach.setFileName(f.getName()) call in buildMessage) encode the name as UTF-8 on every platform, for example by building the filename parameter of Content-Disposition and the name parameter of Content-Type with ParameterList.set(name, value, "UTF-8"). Setting mail.mime.charset as a system property would change the default charset for every javax.mail user in the JVM, so it is not a substitute.
The upgrade changes more than parameter encoding, and the changelog should say so:
- Since JavaMail 1.5.6,
InternetAddress.getLocalAddressusesInetAddress.getCanonicalHostName()when neithermail.fromnormail.hostis set, andSendMailCommandsets neither. The domain in the generatedMessage-IDchanges from the host name to the canonical host name, andprepareMessage()performs a reverse DNS lookup. - A parameter longer than about 60 characters is split into RFC 2231 continuations (
filename*0*=...; filename*1*=...), becausemail.mime.splitlongparametersdefaults totrue. Older mail clients handle continuations worse than a single parameter. - The jar in
lib/is renamed frommail-1.5.0-b01.jartojavax.mail-1.6.2.jar, and the published POMs ofApacheJMeter_mailandApacheJMeter_componentsdepend oncom.sun.mail:javax.mailinstead ofjavax.mail:mail. An installation upgraded by unpacking over the old one, or a Maven build that also pullsjavax.mail:mail(jmeter-maven-plugin, third-party plugins), ends up with both jars on the classpath, since dependency resolution cannot merge two different group and artifact ids.
The PR description. The root cause is the old default of mail.mime.encodeparameters: JavaMail 1.5 changed it to true, and the 1.5.0-b01 beta predates that change. Please state that, and revise "Bug fix (non-breaking change)" in light of the changes listed above.
| api("jakarta.jms:jakarta.jms-api:3.1.0") | ||
| api("javax.activation:javax.activation-api:1.2.0") | ||
| api("javax.mail:mail:1.5.0-b01") | ||
| api("com.sun.mail:javax.mail:1.6.2") |
There was a problem hiding this comment.
src/dist/src/dist/expected_release_jars.csv still lists mail-1.5.0-b01.jar, so :src:dist:verifyReleaseDependencies fails in a release build. For a -SNAPSHOT version the task only logs the difference, so CI stays green:
+ 659031 javax.mail-1.6.2.jar
- 519087 mail-1.5.0-b01.jar
Please regenerate the file with ./gradlew :src:dist:verifyReleaseDependencies -PupdateExpectedJars.
com.sun.mail:javax.mail:1.6.2 (2018) is also the last release under these coordinates, so Renovate will never propose an update. The same javax.mail package continues as com.sun.mail:jakarta.mail 1.6.x (latest 1.6.8). That artifact depends on com.sun.activation:jakarta.activation, which ships the javax.activation classes JMeter already gets from com.sun.activation:javax.activation:1.2.0, so it needs an exclude and a Renovate rule that keeps it on 1.6.x. Please consider it, or explain in the description why 1.6.2 is preferred.
| File tempDir; | ||
|
|
||
| @Test | ||
| void testNonAsciiAttachmentFileNameIsEncodedPerRfc2231() throws Exception { |
There was a problem hiding this comment.
The file name is carried in two headers, and the test checks only Content-Disposition. setFileName also sets the name parameter of Content-Type; please assert it as well. A name longer than about 60 characters takes a different path (RFC 2231 continuations, filename*0*=) and deserves its own case.
The test prefix repeats what @Test already says; a name such as nonAsciiAttachmentFileNameIsEncodedAsUtf8 reads better in the report. The same applies to testAsciiAttachmentFileNameIsNotEncoded.
| // The file name must be encoded according to RFC 2231, so that the | ||
| // recipients see the original file name (see issue #6652) | ||
| assertTrue( | ||
| rawMessage.contains("filename*=UTF-8''%D1%82%D0%B5%D0%BA%D1%81%D1%82.txt"), |
There was a problem hiding this comment.
This expectation holds only when the JVM default charset is UTF-8. On Java 17 with a non-UTF-8 default (windows-1252 on Windows), the header is filename*=Cp1252''%3F%3F%3F%3F%3F.txt and the test fails, on a CI job with that setup and on a contributor's machine. Once SendMailCommand encodes the name as UTF-8 explicitly (see the review summary), the assertion no longer depends on the platform. Please run the test on Java 17 with -Dfile.encoding=windows-1252 to confirm.
| rawMessage.contains("filename*=UTF-8''%D1%82%D0%B5%D0%BA%D1%81%D1%82.txt"), | ||
| "filename* parameter with RFC 2231 encoded file name expected in:\n" + rawMessage); | ||
| // The mangled name must not appear anywhere | ||
| assertFalse(rawMessage.contains("B5:AB"), "mangled file name found in:\n" + rawMessage); |
There was a problem hiding this comment.
This assertion pins one particular way the old version mangled the name and misses the others: the ?????.txt output above passes it. Please parse the message back and compare the decoded name, which fails on any mangling and prints both values:
MimeMessage parsed = new MimeMessage(null, new ByteArrayInputStream(rawBytes));
BodyPart attachment = ((Multipart) parsed.getContent()).getBodyPart(1);
assertEquals(NON_ASCII_FILE_NAME, attachment.getFileName(), "decoded attachment file name");Keep the filename*= check next to it if the test is meant to pin the RFC 2231 form.
|
|
||
| private static String writeMessageToString(Message message) throws Exception { | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| try (outputStream) { |
There was a problem hiding this comment.
ByteArrayOutputStream.close() has no effect, so the try block can go; a plain message.writeTo(outputStream); is enough. Returning the bytes instead of a String would also let the round-trip check above parse them directly.
| <li>Update json-path to 2.10.0 for JSON query expressions.</li> | ||
| <li>Update Neo4j Java driver to 6.x for Bolt-based database tests.</li> | ||
| <li>Update Rhino JavaScript engine to 1.8.0 for JSR-223 JavaScript execution.</li> | ||
| <li>Update <code>javax.mail</code> to 1.6.2 from 1.5.0-b01, so mail attachments with non-ASCII file names are encoded correctly.</li> |
There was a problem hiding this comment.
This repeats the Bug fixes entry. Please reduce this line to the version bump (Update javax.mail to 1.6.2 from 1.5.0-b01) and use this section for the changes users can observe: the jar in lib/ becomes javax.mail-1.6.2.jar, the Maven coordinates become com.sun.mail:javax.mail, and the domain in the generated Message-ID becomes the canonical host name.
| <ch_section>Bug fixes</ch_section> | ||
| <h3>General</h3> | ||
| <ul> | ||
| <li><issue>6652</issue>SMTP Sampler encoded non-ASCII attachment file names incorrectly. Attachment file names are now encoded according to RFC 2231 by upgrading <code>javax.mail</code> to 1.6.2.</li> |
There was a problem hiding this comment.
Please move this entry to an Other samplers section, the heading earlier releases use for non-HTTP samplers, and add <pr>6757</pr> and the contributor credit. The entry should state what users get rather than how it was done, for example:
<li><issue>6652</issue><pr>6757</pr>Encode non-ASCII attachment file names in SMTP Sampler according to RFC 2231, so recipients see the original name. Contributed by Ashraf Ali (github.com/ashrafiucse)</li>MimeBodyPart.setFileName encodes the filename parameter of Content-Disposition and the name parameter of Content-Type with the default MIME charset of the JVM, which falls back to file.encoding. On a JVM with a non-UTF-8 default charset (for example windows-1252, the Windows default on Java 17), characters that the charset cannot represent are replaced, so recipients still see a mangled file name after the javax.mail upgrade. Build both parameters with ParameterList.set(name, value, "UTF-8") in SendMailCommand instead, so the name is encoded as UTF-8 according to RFC 2231 regardless of the platform charset. Move javax.mail to com.sun.mail:jakarta.mail:1.6.8 (same javax.mail namespace, still maintained coordinates), exclude the transitive com.sun.activation:jakarta.activation, pin the artifact below 2.0.0 in Renovate, trust its signing key, update the license override to the EPL-2.0 / EDL-1.0 / GPL2 w/ CPE set declared since 1.6.3, regenerate expected_release_jars.csv, and document the observable changes. Closes apache#6652
|
All points are addressed in d68608f:
One side effect worth flagging: text attachments no longer get an automatic |
Description
Encode SMTP Sampler attachment file names that contain non-ASCII characters as UTF-8 according to RFC 2231, so recipients see the original name on every platform.
Two changes combine for the fix:
javax.mailfrom the 2013 betajavax.mail:mail:1.5.0-b01tocom.sun.mail:jakarta.mail:1.6.8. The root cause of the mangled names was the beta predating JavaMail 1.5's flip of themail.mime.encodeparametersdefault totrue, which is what makesMimeBodyPart.setFileNameemitfilename*=charset''...at all. Thejavax.mail.*namespace is unchanged injakarta.mail1.6.x, so no code changes come from the artifact itself.SendMailCommand: thefilenameparameter ofContent-Dispositionand thenameparameter ofContent-Typeare now built withParameterList.set(name, value, "UTF-8").MimeBodyPart.setFileNameencodes withMimeUtility.getDefaultMIMECharset(), which falls back to the JVM default charset — on Java 17 withfile.encoding=Cp1252(the Windows default), the name was still mangled even after the upgrade (filename*=Cp1252''%3F%3F%3F%3F%3F.txt). Settingmail.mime.charsetas a system property is not an alternative, as it would change the default charset for every javax.mail user in the JVM.com.sun.mail:jakarta.mail:1.6.8is used instead ofcom.sun.mail:javax.mail:1.6.2(the last release under the old coordinates, so Renovate would never propose further updates): 1.6.8 is the same code line, stilljavax.mail-namespace, with three more years of fixes. Its transitivecom.sun.activation:jakarta.activationis excluded everywhere (it duplicates thejavax.activationclasses JMeter already gets fromcom.sun.activation:javax.activation:1.2.0), a Renovate rule pins the artifact to< 2.0.0(2.x renames the packages tojakarta.mail), the signing key485E371CB07EABE6D5778D4B0C27E8FAC93B3B19(Eclipse Project for JavaMail) is added to the verification keyring, and the license override is updated because 1.6.8 declares EPL-2.0 / EDL-1.0 / GPL2 w/ CPE instead of the old CDDL + GPLv2+CE.Motivation and Context
Fixes #6652
The SMTP Sampler mangled attachment file names that contain non-ASCII characters. Sending an attachment named
текст.txtproduced:so recipients saw garbage instead of the original name. With the upgrade plus the explicit UTF-8 encoding, the same message now carries:
regardless of the JVM default charset (verified on Java 17 with
file.encoding=UTF-8andfile.encoding=Cp1252).Behavior changes users can observe (not purely non-breaking)
lib/becomesjakarta.mail-1.6.8.jar(wasmail-1.5.0-b01.jar); an installation upgraded by unpacking over the old one keeps the stale jar on the classpath, so old jars should be removed.ApacheJMeter_mailandApacheJMeter_componentsdepend oncom.sun.mail:jakarta.mailinstead ofjavax.mail:mail; Maven builds that pull both coordinates (e.g. via jmeter-maven-plugin or third-party plugins) end up with two jars, as dependency resolution cannot merge different group/artifact ids.Message-IDbecomes the canonical host name (InternetAddress.getLocalAddressusesInetAddress.getCanonicalHostName()since JavaMail 1.5.6 when neithermail.fromnormail.hostis set), andprepareMessage()performs a reverse DNS lookup for it.filename*0=...; filename*1=...), sincemail.mime.splitlongparametersdefaults totrue; encoded non-ASCII names are never split (singlefilename*=parameter).charset=us-asciiparameter: theContent-Typeof attachments is now set together with the encodednameparameter beforesaveChanges(), soupdateHeaders()no longer rewrites it.In light of the above, the PR is a bug fix with the compatibility notes listed here, not a plain "non-breaking change".
How Has This Been Tested?
Unit tests in
SendMailCommandTest(new): non-ASCII names are encoded as UTF-8 in both thefilenameparameter ofContent-Dispositionand thenameparameter ofContent-Type; ASCII names stay plain; a long ASCII name is split into continuations and reassembles on parse-back; a long non-ASCII name stays a single encoded parameter. Every case also parses the produced message back and compares the decodedgetFileName()with the original, which fails on any mangling. The suite passes on Java 17 underfile.encoding=UTF-8andfile.encoding=Cp1252; the oldsetFileNamepath was verified to producefilename*=Cp1252''%3F%3F%3F%3F%3F.txtunderCp1252before the change.:src:dist:verifyReleaseDependenciespasses with the regeneratedexpected_release_jars.csv, and the license gather tasks pass with the updated override.Screenshots (if appropriate, else remove this section)
N/A
Checklist
./gradlew classes stylepasses locallyxdocs/changes.xmldescribes the fix and observable changes, in the sections used by earlier releases