Conversation
There was a problem hiding this comment.
Pull request overview
Enables forwarding of inline images by preserving cid: ↔ attachment URL ↔ data-cid metadata across message rendering and compose, and by persisting attachment Content-ID / disposition so forwarded attachments can be reconstructed correctly.
Changes:
- Add support for inline attachments (Content-ID + disposition) end-to-end: IMAP parsing, attachment forwarding, MIME building, and compose UI handling.
- Extend HTML purification to rewrite
cid:to attachment URLs and add adata-cidmarker so forwarding can restorecid:references. - Add/adjust unit tests and test fixtures to cover encrypted/inline attachments, MIME building behavior, and HTMLPurifier transforms.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/decrypted-message-body-with-attachment.txt | Adds encrypted MIME fixture with both inline image and regular attachment parts. |
| tests/Unit/Service/TransmissionServiceTest.php | Updates tests for new LocalAttachment disposition handling. |
| tests/Unit/Service/MimeMessageTest.php | Adds coverage for inline attachments going into multipart/related and data-cid rewrite behavior. |
| tests/Unit/Service/MailManagerTest.php | Updates Attachment constructor usage with new fields. |
| tests/Unit/Service/HtmlPurify/TransformURLSchemeTest.php | Adjusts tests for new inline-attachments-based CID rewrite logic. |
| tests/Unit/Service/HtmlPurify/TransformCidDataAttrTest.php | New tests for adding data-cid based on inline attachment URLs. |
| tests/Unit/Service/Attachment/AttachmentServiceTest.php | Updates attachment creation/forwarding tests for contentId/disposition changes. |
| tests/Unit/IMAP/MessageMapperTest.php | Extends tests to validate content type and disposition for encrypted attachments and inline images. |
| tests/Unit/Db/LocalAttachmentTest.php | New unit tests for LocalAttachment disposition helper method. |
| tests/Unit/Controller/MessagesControllerTest.php | Updates Attachment constructor usage with new fields. |
| src/store/mainStore/actions.js | Prepares both normal and inline attachments when forwarding; adds compose HTML body fetch path. |
| src/service/MessageService.js | Adds a compose-specific HTML body fetch helper. |
| src/components/TextEditor.vue | Allows img[data-cid] through the editor’s HTML support config. |
| src/components/ComposerAttachments.vue | Preserves attachment type/databaseId for forwarding inline previews. |
| src/components/ComposerAttachment.vue | Computes preview source including forwarded inline image download URLs. |
| lib/Service/TransmissionService.php | Applies LocalAttachment disposition/contentId to MIME parts, supporting omitted Content-Disposition for iMIP. |
| lib/Service/MimeMessage.php | Separates inline attachments into multipart/related and rewrites data-cid back to cid:. |
| lib/Service/MailTransmission.php | Updates build() call signature for MIME message construction. |
| lib/Service/HtmlPurify/TransformURLScheme.php | Refactors CID URL rewriting to use inline attachments list. |
| lib/Service/HtmlPurify/TransformCidDataAttr.php | New HTMLPurifier transform to add data-cid based on inline attachment URLs. |
| lib/Service/Html.php | Updates sanitizer to use new transforms and inline attachment URL enrichment. |
| lib/Service/DataUri/DataUri.php | Documents data URI parameter shape for charset access. |
| lib/Service/Attachment/AttachmentService.php | Persists contentId/disposition on forwarded attachments; supports forwarding inline message attachments. |
| lib/Service/AntiSpamService.php | Updates build() call signature for MIME message construction. |
| lib/Provider/Command/MessageSend.php | Infers and persists attachment disposition when provider API lacks it. |
| lib/Model/IMAPMessage.php | Exposes inlineAttachments in full message payload; updates HTML sanitization call signature. |
| lib/Migration/Version5008Date20260320125737.php | Adds DB columns for content_id and disposition on mail_attachments. |
| lib/IMAP/MessageMapper.php | Adjusts attachment enumeration and attachment metadata extraction (type/disposition/content-id). |
| lib/Db/LocalAttachment.php | Adds contentId/disposition fields and helper for disposition checks. |
| lib/Controller/MessagesController.php | Minor formatting adjustment in HTML body retrieval path. |
| lib/Contracts/IAttachmentService.php | Improves return type doc for getAttachment() to include ISimpleFile. |
| lib/Attachment.php | Extends Attachment model to carry contentId/disposition; improves name/disposition normalization. |
| appinfo/info.xml | Bumps app version. |
| REUSE.toml | Adds REUSE annotation for the new test fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $inlineAttachment = array_find($this->inlineAttachments, static function ($attachment) use ($uri) { | ||
| return $attachment['cid'] === $uri->path; | ||
| }); |
There was a problem hiding this comment.
array_find() is used here, but it isn’t available in the PHP versions Nextcloud Mail typically supports and there’s no polyfill in this repository. This will cause a fatal error when rewriting cid: URLs. Please replace it with a simple foreach/array_filter-based lookup that works on all supported PHP versions.
| $inlineAttachment = array_find($this->inlineAttachments, static function ($attachment) use ($uri) { | |
| return $attachment['cid'] === $uri->path; | |
| }); | |
| $inlineAttachment = null; | |
| foreach ($this->inlineAttachments as $attachment) { | |
| if ($attachment['cid'] === $uri->path) { | |
| $inlineAttachment = $attachment; | |
| break; | |
| } | |
| } |
There was a problem hiding this comment.
kesselb
force-pushed
the
feat-forward-inline-images
branch
5 times, most recently
from
68179dc to
e4a5e21
Compare
kesselb
force-pushed
the
feat-forward-inline-images
branch
5 times, most recently
from
4837c77 to
592da19
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| try { | ||
| $localAttachment = $this->addFileFromString($account->getUserId(), $attachment['fileName'] ?? $attachmentMessage->getSubject() . '.eml', $mime, $fullText); | ||
| $localAttachment = $this->addFileFromString($account->getUserId(), $attachment['fileName'] ?? $attachmentMessage->getSubject() . '.eml', $mime, $fullText, null, null); |
There was a problem hiding this comment.
handleForwardedMessageAttachment() creates a LocalAttachment with disposition set to null. With the updated TransmissionService::handleAttachment() logic, this will omit both Content-Disposition and the attachment name when sending, which likely breaks forwarded .eml attachments (they should be sent as attachment). Pass LocalAttachment::DISPOSITION_ATTACHMENT here (and keep contentId null).
| $localAttachment = $this->addFileFromString($account->getUserId(), $attachment['fileName'] ?? $attachmentMessage->getSubject() . '.eml', $mime, $fullText, null, null); | |
| $localAttachment = $this->addFileFromString($account->getUserId(), $attachment['fileName'] ?? $attachmentMessage->getSubject() . '.eml', $mime, $fullText, null, LocalAttachment::DISPOSITION_ATTACHMENT); |
|
|
||
| try { | ||
| $localAttachment = $this->addFileFromString($account->getUserId(), $file->getName(), $file->getMimeType(), $file->getContent()); | ||
| $localAttachment = $this->addFileFromString($account->getUserId(), $file->getName(), $file->getMimeType(), $file->getContent(), null, null); |
There was a problem hiding this comment.
handleCloudAttachment() calls addFileFromString(..., null, null), leaving disposition unset. Since TransmissionService::handleAttachment() now only sets name/disposition when the DB field is attachment/inline, cloud attachments will be sent without a filename and without Content-Disposition. Set the disposition to LocalAttachment::DISPOSITION_ATTACHMENT for cloud attachments.
| $localAttachment = $this->addFileFromString($account->getUserId(), $file->getName(), $file->getMimeType(), $file->getContent(), null, null); | |
| $localAttachment = $this->addFileFromString($account->getUserId(), $file->getName(), $file->getMimeType(), $file->getContent(), null, LocalAttachment::DISPOSITION_ATTACHMENT); |
| /** | ||
| * Adds an emails attachments | ||
| * | ||
| * @param Account $account | ||
| * @param mixed[] $attachment | ||
| * @param \Horde_Imap_Client_Socket $client | ||
| * @return int | ||
| * @throws DoesNotExistException | ||
| */ | ||
| private function handleForwardedAttachment(Account $account, array $attachment, \Horde_Imap_Client_Socket $client): ?int { |
There was a problem hiding this comment.
The PHPDoc for handleForwardedAttachment() says @return int but the method signature is : ?int and it returns null on upload failure. Update the docblock to int|null to match the implementation.
| $attachmentsTable->addColumn('content_id', Types::STRING, [ | ||
| 'notnull' => false, | ||
| ]); | ||
| $attachmentsTable->addColumn('disposition', Types::STRING, [ | ||
| 'notnull' => false, | ||
| ]); |
There was a problem hiding this comment.
This migration adds content_id and disposition unconditionally once the table exists. If the migration partially ran (or the columns already exist for some reason), addColumn() will throw and block upgrades. Guard each addColumn() with hasColumn() checks (or use addColumn only when missing).
| $attachmentsTable->addColumn('content_id', Types::STRING, [ | |
| 'notnull' => false, | |
| ]); | |
| $attachmentsTable->addColumn('disposition', Types::STRING, [ | |
| 'notnull' => false, | |
| ]); | |
| if (!$attachmentsTable->hasColumn('content_id')) { | |
| $attachmentsTable->addColumn('content_id', Types::STRING, [ | |
| 'notnull' => false, | |
| ]); | |
| } | |
| if (!$attachmentsTable->hasColumn('disposition')) { | |
| $attachmentsTable->addColumn('disposition', Types::STRING, [ | |
| 'notnull' => false, | |
| ]); | |
| } |
| use OCP\AppFramework\Controller; | ||
| use OCP\AppFramework\Db\DoesNotExistException; | ||
| use OCP\AppFramework\Http; | ||
| use OCP\AppFramework\Http\Attribute\NoAdminRequired; |
There was a problem hiding this comment.
NoAdminRequired is imported but not used anywhere in this controller (the file still uses @NoAdminRequired docblock annotations, not the attribute). Please drop the unused import to avoid dead code and keep imports clean.
| use OCP\AppFramework\Http\Attribute\NoAdminRequired; |
| private array $inlineAttachments = [ | ||
| [ | ||
| 'cid' => 'image001@example.com', | ||
| 'url' => 'https://mail.example.com/index.php/apps/mail/api/messages/42/attachments/7', | ||
| ], | ||
| [ | ||
| 'cid' => 'image002@example.com', | ||
| 'url' => 'https://mail.example.com/index.php/apps/mail/api/messages/42/attachments/8', | ||
| ], |
There was a problem hiding this comment.
The test data uses an attachment URL path with /attachments/..., but the route in this app for a single attachment is /api/messages/{id}/attachment/{attachmentId} (singular attachment). Using the real route shape here makes the test more representative and avoids masking path-based matching issues.
| private array $inlineAttachments = [ | ||
| ['cid' => 'valid-cid', 'url' => 'https://mail.example.com/index.php/apps/mail/api/messages/42/attachments/123'], | ||
| ]; |
There was a problem hiding this comment.
The inline attachment URL in this test uses /attachments/123, but the actual download route is /api/messages/{id}/attachment/{attachmentId} (singular). Consider updating the fixture URL to match the real route so the test exercises the correct path structure.
| if ($localAttachment->isDispositionAttachmentOrInline()) { | ||
| $part->setDisposition($localAttachment->getDisposition()); | ||
| /* | ||
| * Setting a name implicitly adds a Content-Disposition header in Horde, | ||
| * which would override the intentional omission. Only set it for attachment/inline dispositions. |
There was a problem hiding this comment.
handleAttachment() now only sets Content-Disposition and the attachment name when LocalAttachment::disposition is explicitly 'attachment' or 'inline'. After introducing the new DB column, existing attachments (e.g. drafts created before the upgrade, or any rows not yet backfilled) will have disposition = null and will be sent without a filename / disposition. Consider treating null as the previous default (attachment) unless you intentionally need to omit it (e.g. for iMIP), or backfill existing rows in the migration.
| if ($localAttachment->isDispositionAttachmentOrInline()) { | |
| $part->setDisposition($localAttachment->getDisposition()); | |
| /* | |
| * Setting a name implicitly adds a Content-Disposition header in Horde, | |
| * which would override the intentional omission. Only set it for attachment/inline dispositions. | |
| $disposition = $localAttachment->getDisposition() ?? 'attachment'; | |
| if ($disposition === 'attachment' || $disposition === 'inline') { | |
| $part->setDisposition($disposition); | |
| /* | |
| * Setting a name implicitly adds a Content-Disposition header in Horde, | |
| * which would override the intentional omission. Only set it for attachment/inline dispositions. | |
| * | |
| * Treat a null disposition as the legacy default "attachment" so | |
| * pre-existing rows continue to be sent with a filename/disposition. |
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> AI-assisted: Claude Code (Claude Sonnet 4.6) and OpenCode (gpt-5.4)
The charset item is always there Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> fix: Skip link check for empty message Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb
force-pushed
the
feat-forward-inline-images
branch
from
592da19 to
8630efe
Compare
Fix #8351
Todo
Findings
MessageMapper::getAttachment()previously forced the Content-Type of all attachments to application/octet-stream, except for text/calendar. I assume this was originally introduced as a security hardening.For forwarding messages with inline attachments, however, preserving the original Content-Type is necessary. Mail clients generally do not render inline parts correctly when they are marked as application/octet-stream, so forwarded inline images would no longer be displayed inline.
AttachmentService::handleForwardedAttachment(), we previously tried to guess the Content-Type` from the attachment payload. Now that the original MIME metadata is preserved, this is no longer necessary. The trade-off is that we now rely on the provider-supplied MIME headers instead of re-deriving the type from the content, so this effectively trusts the original message metadata without additional validation.getAttachment() consumers
AttachmentService.handleForwardedAttachmentMessageController.downloadAttachmentMessageController.saveAttachmentMessageApiController.getAttachmentTest cases