Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter "real" attachments by being referenced #9472

Merged
merged 2 commits into from
Jul 21, 2024

Conversation

pabzm
Copy link
Member

@pabzm pabzm commented May 30, 2024

This changes the way in which attachments are determined to be shown as downloadable ("standalone"), or not ("inline").
In theory this should be determined by their Content-Disposition, but in reality this often doesn't work.

Previously, on top of the default standard handling, edge cases have been fixed by checking for their concrete details.

Now the code checks if the Content-ID or Content-Location of the attachment is actually being used in other parts of the message. If not, the attachment is considered to be "standalone". This might make some images be shown as downloadable, which have a "inline"-disposition – but it'll happen only in cases where the message is broken, or they wouldn't have been shown at all previously. So I consider this a progression while staying a comprehensible and review-able changeset.

This fixes #9443.

I didn't find an email that it breaks. I also don't see how it could.

It helps #9465, too.

And it (unintendedly) also helps #9326, because now the information, which image is actually referenced in an HTML-part, ist available already after message parsing.

program/lib/Roundcube/rcube_message.php Outdated Show resolved Hide resolved
program/lib/Roundcube/rcube_message.php Show resolved Hide resolved
program/lib/Roundcube/rcube_message.php Show resolved Hide resolved
program/lib/Roundcube/rcube_message.php Outdated Show resolved Hide resolved
@pabzm pabzm requested a review from alecpl June 6, 2024 08:55
@pabzm pabzm force-pushed the filter-attachments-by-reference branch from d7adcf0 to 72ff9f3 Compare June 6, 2024 10:03
@alecpl
Copy link
Member

alecpl commented Jun 16, 2024

Looks to me that it fixes #9443 only partially. With inline_images=true I expect the image listed on attachments list to be displayed not only on the list, but also below the body (here a non-existent body).

I think for inline_images feature we'll really need to look into the body and display the image when: 1) there's no HTML part or, 2) there's HTML part but it does not have a reference to the image part.

@pabzm pabzm modified the milestones: later, 1.7-beta Jun 18, 2024
@pabzm
Copy link
Member Author

pabzm commented Jun 20, 2024

Looks to me that it fixes #9443 only partially

That's because the message was considered empty and thus no body was rendered. An issue that is not exactly in scope of this PR, but because it is related to handling mime-parts I fixed it nonetheless in here.

Now the image is shown inline as expected.

I'd be happy to clean up the commit history before merging if you'd agree.

@pabzm
Copy link
Member Author

pabzm commented Jun 20, 2024

Wait a moment, I'll have to rebase to "master" anyway, and then I can clean the history a little, too.

@pabzm pabzm force-pushed the filter-attachments-by-reference branch from 49b3de4 to 6723329 Compare June 20, 2024 09:09
This changes the way in which attachments are determined to be shown as
such ("standalone"), or not ("inline").
In theory this should be determined by their Content-Disposition, but in
reality this often doesn't work.
Now we check if the Content-ID or Content-Location of the attachment is
actually being used in other parts of the message. If not, the
attachment is considered to be "standalone".
Previously only `parts` and `body` were checked, so mime-parts that were
classified into `attachments` and `inline_parts` didn't count – thus
messages that contained only those parts were shown blank.
@pabzm pabzm force-pushed the filter-attachments-by-reference branch from 6723329 to 355bcca Compare July 15, 2024 12:42
@pabzm
Copy link
Member Author

pabzm commented Jul 15, 2024

@alecpl I rebased onto the latest master. Could you merge this or let me know what's blocking it, please?

@alecpl alecpl merged commit 8f9f1f1 into roundcube:master Jul 21, 2024
16 checks passed
@pabzm
Copy link
Member Author

pabzm commented Jul 23, 2024

@alecpl Just a note: I would prefer if you wouldn't use squash-commits when merging, because it makes rebasing much harder (git can't automatically identify the commits that were used before). If you don't like a list of commits please just ask to clean it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot download attachment for some multipart/mixed messages with base64-encoded jpg images
2 participants