Email attachments broken since 7.10.31 for configurable upload folder

[tsmgeek]

https://github.com/salesagility/SuiteCRM/pull/9123

This patch seens towas not properly applied to both 7.11 & 7.10, leading to attachments not being sent on emails.

https://github.com/salesagility/SuiteCRM/blob/master/modules/Emails/Email.php#L3036-L3091

https://github.com/salesagility/SuiteCRM/blob/hotfix-7.10.x/modules/Emails/Email.php#L3036-L3091

This is not even a valid file stream path, file/ is a valid path, 'file://' is but for streams, in theory if the path is relative to root of site you can just strip it off if not using with streams.

Instead of hardcoding the paths when using internal streams maybe this is a better alternative.

https://www.php.net/manual/en/function.stream-resolve-include-path.php

[pgorod]

As I commented on #9123, I really don't think it's a good idea to go changing these streams into hard-coded directories.

Especially since we're not doing it for the entire codebase, we're just doing it in parts of the code... and that breaks stuff. To make things worse, it's not obvious to discover all the places where this is happening, since these paths are formed from variables in many different ways. We can search, but we always risk missing some occurrences.

I believe we should revert this change, understand the way we're using the stream wrapper class, and solve whatever problem we had with PHPMailer in the way I proposed here. This might seem more complicated, but it will actually save us a bunch of time and will be more backwards compatible with any installations out there that are not using the default upload dir.

[tsmgeek]

@pgorod As mailing goes though SugarPHPMailer it may be better to revert the prior changes, override the "AddAttachments" method (and any other that takes in a filepath) and unwrap the path there using stream_resolve_include_path after checking if its a URI (ie filter_var($string, FILTER_VALIDATE_URL)) or just a normal path.

[samus-aran]

We are currently looking at this, would be great to get this into the next release.

[samus-aran]

Hey folk @tsmgeek & @pgorod Anyone able to help test the proposed PR.

[pgorod]

I can't test currently, but looking at the code, I would expect to not find any paths built with upload/ in the new code, since we should revert all of those to upload://.

In other words: if we test with a non-standard upload dir (and we should), I don't think this code can work.

But maybe I am missing something?

[samus-aran]

Re-reading this all again I'm getting a better understanding. IMHO Yes we should be looking at resolving this from a configurable stand point. The less of the two evils is that it works but not at the moment configurable. I would prefer that we can at least give users that functionality rather than waiting too long for either a patch or a follow release.

[SuiteBot]

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/sugarphpmailer-encountered-an-error-could-not-access-file-upload/68091/6

[pstevens71]

Thanks all, I just applied the patch and can confirm it works.

[tsmgeek]

I am using this now in production as updated to latest LTS release, so far no reported issues.

[samus-aran]

I think the issue is now that we need to provide a fix for configurable upload_dir values hence why I hadn't closed it :)

[tsmgeek]

@samus-aran @JackBuchanan https://github.com/salesagility/SuiteCRM/commit/9a029755967f103aa4ce9102a07cfe07d3124c49#diff-3e480a6fd72ec3580851e53fb6c2c4f2818181c02c4a73fb7bb96b35f9bef595

I think this commit was not pulled over into 7.12.0 release, if so will it be causing issues?

Im just doing a merge/compare with the changes I have in 7.10.x release and noticed its not there.

[clemente-raposo]

Hi @tsmgeek,

Created the following MR to pull this fix into 7.12.

https://github.com/salesagility/SuiteCRM/pull/9389

@pgorod Also searched the app and replaced all hardcoded usages of upload/ with a new utils function that looks into the config.

If you see any thing that would be good to change please let me know.

[pgorod]

Are we deprecating the UploadStream scheme? Introduction here.

If so, why? If not, why have two different systems of resolving these paths?

I'm not totally a fan of the UploadStream because it is quite obscure. It took me a long time to catch on, it's a sneaky function call intervening in every reference to that upload:// path.

But I have to concede that it has advantages. It's a neat layer that PHP includes exactly for this purpose, and is way more powerful than just playing with path strings. If we used this scheme consistently (we currently don't), we could divert uploads to totally different media (cloud, database), hook into every operation, add encryption/decryption, apply more logic such as clean-ups of old stuff, etc., without touching all the different points in the code that use this directory...

[pgorod]

It just occurred to me, a simple way of reducing the obscurity of the UploadStream scheme would be to do a quick find-and-replace changing every occurrence of upload:// to UploadStream://. Then, of course, change the stream name in the class, and update also the references to this regarding the suhosin whitelist.

This simple, riskless name change should be enough to avoid people confusing the stream name with the directory name (upload:// is too similar to upload/) and to provide a hint of where in the codebase we need to search in order to understand what's going on.

[tsmgeek]

@pgorod would it not be an idea to investigate replacing all the file storage handling with a library such as ... https://github.com/KnpLabs/Gaufrette - has built in streamwrapper or https://github.com/thephpleague/flysystem - (3rd party streamwrapper https://github.com/m2mtech/flysystem-stream-wrapper/) going this way would make it easy to store files on any type of storage as its plugable

[pgorod]

Technically, this sounds good, but in practical terms, I don't think the project can handle the effort involved in that (who would do it? And shouldn't that person rather be fixing something else?). I'd be happy to just get the existing solution back on its feet.

[SuiteBot]

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/cannot-import-products-aos-products-no/92839/7