fix: keep attachment file extension when saving - #11268
Conversation
Saving an attachment launched ACTION_CREATE_DOCUMENT with the MIME type declared in the mail Content-Type header. When that type did not round-trip to the display name's extension, the Storage Access Framework appended the declared type's preferred extension, so appointment.ics declared as text/plain was written as appointment.ics.txt. Derive a MIME type that is consistent with the display name's extension (the same approach the VIEW flow already uses in ViewIntentFinder) via a new MimeTypeUtil.getMimeTypeForFilename helper and pass it to the create-document intent, so the provider keeps the filename unchanged. When the name has no extension, the declared type (or the default) is used and the provider may add a suitable extension as before. Fixes thunderbird#7968
|
✅ Validation Passed: All report and feature-flag labels are correctly set. |
rafaeltonholo
left a comment
rafaeltonholo
left a comment
There was a problem hiding this comment.
Thanks for adding this contribution; however, there are a few points we need to address before being able to merge it.
| * {@link #DEFAULT_ATTACHMENT_MIME_TYPE} when it is {@code null}) is returned and the provider may add a suitable | ||
| * extension. | ||
| */ | ||
| public static String getMimeTypeForFilename(String displayName, String declaredMimeType) { |
There was a problem hiding this comment.
If the return can be nullable, the method on java must always be annotated with @Nullable.
| public static String getMimeTypeForFilename(String displayName, String declaredMimeType) { | |
| @Nullable | |
| public static String getMimeTypeForFilename(String displayName, String declaredMimeType) { |
| mimeType = requireNotNull(attachment.mimeType) { | ||
| "Invalid attachment type. The mimeType is null. Attachment = $attachment" | ||
| }, | ||
| mimeType = MimeTypeUtil.getMimeTypeForFilename(attachment.displayName, attachment.mimeType), |
There was a problem hiding this comment.
There are two issues with this code:
- We should not transform data in a view. The view should only consume the data provided to it and perform validations, in case of a need to display something to the user. This should be moved to where the
attachmentis created. - This code can potentially cause a NullPointerException without an explanation why. Since the mimetype can return null at this point. The best approach here would be to wrap it using a
requireNotNull, so we can add a proper messaging that would help us to understand what actually caused the NullPointerException.
Saving an attachment could drop its extension when the name carried no suffix, leaving the file without the type information the viewer had. Resolve the extension from the attachment's MIME type when the supplied name lacks one, in MimeTypeUtil, and apply it where the name is built in AttachmentInfoExtractor so both the message view and the save path agree. Covered by MimeTypeUtilTest and AttachmentInfoExtractorTest.
|
Updated in 678ada5. The extension is now resolved from the attachment's MIME type when the supplied name has no suffix, in That second file is beyond what I first touched. Keeping the change inside
|
3 participants
Contribution Summary
Linked Issue/Ticket: Closes #7968
RFC / Technical Design (if applicable): N/A (small bug fix)
Description
When saving a mail attachment to the filesystem, an extra file extension was appended to the file on disk:
appointment.icswas written asappointment.ics.txt(and a corroborating report showedsomething.pkpasswritten with a spurious trailing extension). The save dialog showed the correct name, but the saved file gained an extra extension.Root cause
MessageViewFragment.onSaveAttachmentlaunchesIntent.ACTION_CREATE_DOCUMENT(viaCreateDocumentResultContract) passing the attachment's declared MIME type, which comes from the mailContent-Typeheader (e.g.text/plain), together with the display name (appointment.ics). Android's Storage Access Framework (FileSystemProvider.splitFileName) checks whether the display name's extension round-trips to the supplied MIME type. Because.icsdoes not map back totext/plain, the provider appends the MIME type's preferred extension (text/plain->.txt), producingappointment.ics.txt.Fix
Pass a MIME type that is consistent with the display name's extension, so the SAF provider leaves the filename unchanged. This mirrors the pattern the VIEW flow already uses (
ViewIntentFinder.getBestViewIntentderivesMimeTypeUtil.getMimeTypeByExtension(displayName)).MimeTypeUtil.getMimeTypeForFilename(displayName, declaredMimeType)that returns the extension-derived MIME type when the display name has an extension, and otherwise falls back to the declared MIME type (orapplication/octet-streamwhen that isnull).onSaveAttachmentnow builds theCreateDocumentResultContract.InputwithmimeType = MimeTypeUtil.getMimeTypeForFilename(attachment.displayName, attachment.mimeType).Because the derived MIME type round-trips with the extension Android computes, the provider no longer appends an extra extension. The actual byte copy in
AttachmentController.writeAttachmentis unaffected, and the unrelated EML-export path (which passes its own explicitmessage/rfc822) is untouched.Notes on edge cases (all covered by the new unit test):
appointment.ics,boardingpass.pkpass): resolves to the extension-consistent type (text/calendar,application/vnd.apple.pkpass) so the filename is preserved.noname): falls back to the declared type, and toapplication/octet-streamwhen the declared type isnull— the provider may then add a suitable extension, which is the desired behaviour.mystery.unknownext): resolves toapplication/octet-stream. This is deliberate: SAF treatsoctet-streamas matching any unrecognised extension, so it keeps the filename as-is rather than appending a spurious extension. (Returning the declared type here would reintroduce the double-extension bug.)This also removes a latent
requireNotNull(attachment.mimeType)that could throw for an attachment with a null declared MIME type; the new helper handles that case gracefully.Screen Shots
No UI changes (the fix affects the filename passed to the create-document intent, not any visible screen).
AI Disclosure
Select one of the following (mandatory)
Contribution Checklist
MimeTypeUtilJava utility class alongside the closely relatedgetMimeTypeByExtensionit builds on, to keep the change minimal and consistent)./gradlew :legacy:core:spotlessCheck :legacy:ui:legacy:spotlessCheckpasses;./gradlew :legacy:core:detekt :legacy:ui:legacy:detektpasses)./gradlew :legacy:core:testDebugUnitTest :legacy:ui:legacy:testDebugUnitTestpasses: 751 and 245 tests respectively, 0 failures)MimeTypeUtilTestwith 6 cases covering known/unknown/missing extensions)Testing
Run locally with JDK 21 and Android SDK 36:
./gradlew :legacy:core:testDebugUnitTest --tests "com.fsck.k9.helper.MimeTypeUtilTest"— 6 passed, 0 failed./gradlew :legacy:core:testDebugUnitTest :legacy:ui:legacy:testDebugUnitTest— 996 passed (6 skipped, pre-existing), 0 failed./gradlew :legacy:core:spotlessCheck :legacy:ui:legacy:spotlessCheck— passed./gradlew :legacy:core:detekt :legacy:ui:legacy:detekt— passedconnectedAndroidTest(instrumented) was not run; there is no device/emulator in this environment. The SAF filename behaviour itself is provided by the Android platform and is exercised via the new unit test on the MIME-resolution helper that drives it.AI was used for assistance.