The mimemail_extract_files function regex $pattern matches more items than it should. In our case, it matched the "src" suffix of a query parameter on a link.

Example: <a href="http://www.example.com?extsrc=foo"> was transformed into <a href="http://www.example.com?extsrc=Array">

because the "src=[\'"]?" clause of the regex matched.

CommentFileSizeAuthor
#6 mimemail.597448_02.patch742 bytessgabe
#5 mimemail.597448_01.patch739 bytessgabe

Comments

allie micka’s picture

allie micka
she/her
commented
Status: Active » Closed (duplicate)

#319229: src="Array" if path to image is broken appears to be a duplicate of this issue, but it contains more followup, so I am closing this one and referring to followup there.

rmjiv’s picture

rmjiv commented
Status: Closed (duplicate) » Active

I just noticed that this issue was marked as a duplicate of #319229. Unfortunately, having read that thread, I don't think that it is. That thread is dealing with a problem related to private download method for files. This issue is problem where the regex match is overly aggressive and matches the characters "src" in places that it shouldn't.

I'm relatively terrible at regular expressions, but it may be as easy as changing line 87 from
$pattern = '/(<link[^>]+href=[\'"]?|<object[^>]+codebase=[\'"]?|@import |src=[\'"]?)([^\'>"]+)([\'"]?)/emis'; to
$pattern = '/(<link[^>]+href=[\'"]?|<object[^>]+codebase=[\'"]?|@import | src=[\'"]?)([^\'>"]+)([\'"]?)/emis';
The goal being to match "src" only if it is preceded with a space.

But I'm not sure that is even 100% right. Really what should happen is that src is only matched as an attribute of an Only local images are allowed. tag.

jerdavis’s picture

jerdavis
Location Minneapolis, MN
commented

This code has changed in the latest release, alpha2 for a security advisory. Please review your change against this code. You should update your site to use a derivative of alpha2 as soon as possible.

rmjiv’s picture

rmjiv commented

Thanks for the heads up on the security advisory. But that only removes the /e switch from the pattern which doesn't affect this either way. The pattern is still wrong in alpha 2.

sgabe’s picture

sgabe commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new mimemail.597448_01.patch739 bytes

I can confirm this issue. I am attaching a patch against current HEAD with the fix in #2.

sgabe’s picture

sgabe commented
StatusFileSize
new mimemail.597448_02.patch742 bytes

The previous patch is not good enough, it misses the case in which there is a line break or tab before the attribute. I'm attaching a new one which checks src attributes with any (\s) preceding white-space character.

sgabe’s picture

sgabe commented
Status: Needs review » Fixed

Committed to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.