Check if attachment is actually(!) referred to #9585
Conversation
pabzm
force-pushed
the
check-if-attachment-is-actually-referred-to
branch
from
d9fe09b to
73cbc95
Compare
pabzm
changed the title
pabzm
force-pushed
the
check-if-attachment-is-actually-referred-to
branch
from
73cbc95 to
636dcdf
Compare
pabzm
force-pushed
the
check-if-attachment-is-actually-referred-to
branch
2 times, most recently
from
1373f57 to
8f12091
Compare
pabzm
force-pushed
the
check-if-attachment-is-actually-referred-to
branch
2 times, most recently
from
698570d to
321d1c6
Compare
|
I would like to integrate a lot more messages into the "Message Rendering" tests, but the rest of the code is ready as it is, and I can supplement additional tests later, too. |
|
@alecpl Could you have another look at this, please? |
...Rendering/data/greenmail/test-message-rendering@localhost/INBOX/spam-with-image-data-uri.eml
Outdated
Show resolved
Hide resolved
|
I've tested a bit with various messages. I've found one regression: a forwarded message with eml attachment - there's a difference in displaying it with this patch and without. The attached message headers section is missing. Also if you open the eml attachment the headers will be missing. ps. I noticed that we FETCH BODY.PEEK[2] for such messages, i.e. we fetch the whole attached message. It seems not optimal if it contains big attachments (I'd have to investigate that separately) - it's the same in release-1.6, so nothing new, I suppose. |
Thank you for finding this! It's present in the "master" branch already, though. I'll try to find out which change is to blame and how to fix it. |
It works fine in master. |
I double-checked and still see the wrong behaviour on the lastest state of the "master"-branch. I'm testing with the attached message, can you please check if it's displayed properly in your setup? And can you provide me with your test message? |
|
Ah, sorry. You're right. It works in release-1.6, but not in master. My bad. |
|
This is what I see with Roundcubemail v1.6.9: 
And this is what I see with Roundcubemail "master" branch: 
|
|
The bug was introduced in c47accb from a year ago. Looks like a simple logic error. I made a fix for the problem – but applied to the latest "master", the problem still occurs, so there seems to be another code issue. I'l continue the search. |
|
Actually my fix does fix the bug if I work thoroughly: #9753 |
If there's no reference to it in a sibling HTML part then we handle it as a classic attachment (which is shown as downloadable).
Previously all headers were only fetched for message/rfc822, or if the Content-Type's "name" parameter was set, or if a Content-ID was set. The RFC doesn't require neither the "name" parameter nor a Content-ID for using Content-Location, though, so we shouldn't depend on those. Instead now all headers are also fetched if the main part of the Content-Type is "image", to catch more cases.
We want it tested!
We decide later, which attachment is considered "inline" and which isn't.
It doesn't change much, but there's also no sense in decoding big images that we don't use.
pabzm
force-pushed
the
check-if-attachment-is-actually-referred-to
branch
from
321d1c6 to
9d05bfe
Compare
|
Rebased onto the "master" branch to fix a conflict |
This finishes what #9472 intended to do – but didn't actually do, as I found out.
The code now checks for each non-text mime-part in a multipart-part if its
Content-IDorContent-Locationis (probably) used in a sibling HTML-part, and only if that matches the respective mime-part is considered an "inline" attachment (that won't show up as downloadable or below the message content).The second commit in this PR makes sure that for all image-parts, all mime-part-headers are loaded from the server, in order to actually get hands on the
Content-Location-header (which isn't always fetched in the first place). It is limited to image-parts because those are the most common ones maybe having aContent-Location-header and I assumed that we shouldn't load the headers for every mime-part, so this seems like a workable real-world distinction for me.One could probably change how the
BODYSTRUCTUREresponse is fetched and parsed to ensure aContent-Location-header is always fetched in the first place, but I didn't dare to touch that code.This fix closes #9565