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

Append target file ext/name to tmp file (speedup antivirus scans) #667

Closed
wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented May 28, 2021

No file extension causes issues with Microsoft Antivirus, this simple update increase performance about 5x.

Was super hard to debug, but once ".php" suffix is added, performance is fixed consistently on Windows.

A bugfix release should be done once merged.

@wisskid
Copy link
Contributor

wisskid commented Aug 18, 2021

@mvorisek Smarty_Internal_Runtime_WriteFile is a general purpose class, not specifically designated for php files. Would a '.tmp' extension work too?

@mvorisek mvorisek changed the title Use ".php" suffix for tmp file Append target file ext/name to tmp file Aug 18, 2021
@mvorisek
Copy link
Contributor Author

@mvorisek Smarty_Internal_Runtime_WriteFile is a general purpose class, not specifically designated for php files. Would a '.tmp' extension work too?

Makes sense. I did some experiments, and yes, if explicitly defined as excluded in https://docs.microsoft.com/en-us/microsoft-365/security/defender-endpoint/configure-extension-file-exclusions-microsoft-defender-antivirus basically any extension can be used.

It is however impractical. So I changed it to append the whole basename. Whole basename instead of basename extension only as the exclusion rules may be defined for some longer file part than the the part after the last dot in general.

Any concerns about this approach?

No or unexpected file extension causes issues with Microsoft Antivirus, this simple update increase performance about 5x.
@mvorisek mvorisek changed the title Append target file ext/name to tmp file Append target file ext/name to tmp file (to speedup antivirus scans) Aug 18, 2021
@mvorisek mvorisek changed the title Append target file ext/name to tmp file (to speedup antivirus scans) Append target file ext/name to tmp file (speedup antivirus scans) Aug 21, 2021
@mvorisek
Copy link
Contributor Author

mvorisek commented Feb 7, 2022

@wisskid can you please merge?

@wisskid
Copy link
Contributor

wisskid commented Feb 13, 2022

@mvorisek I'm not sure this is a good idea. Nothing is broken, it's not a bug and I'm not 100% sure this wouldn't break anything or cause issues for someone else. Another antivirus application might consider to do an indepth scan "your" filenames, for example. Since this is basically appears to be a Microsoft Antivirus issue, it would seem to better to fix it there.

@wisskid wisskid closed this Feb 13, 2022
@mvorisek
Copy link
Contributor Author

@wisskid can you please reconsider? Additionally, naming the files with php ext will present the file sources to be downloadable.

@mvorisek
Copy link
Contributor Author

@wisskid can you please kind reopen this PR? The speedup on Windows is really dramatic and isn't is actually better to sue the original filename extension?

@wisskid wisskid reopened this Apr 16, 2023
@wisskid
Copy link
Contributor

wisskid commented Apr 30, 2023

@mvorisek I've reconsidered and still don't want to change this just because some Antivirus application doesn't get it. I read the page at your link and I noticed you can easily exclude a directory. I propose you configure the cache and compile dirs to be excluded from scanning.
You also mention "Additionally, naming the files with php ext will present the file sources to be downloadable.". You probably mean "prevent". In any case: please configure your web application to not store cached or compiled templates under the web root. They should never be downloadable or runnable.

@wisskid wisskid closed this Apr 30, 2023
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 30, 2023

@wisskid The problem is degraded performace very, very hard to identify. Many users do not have a clue the slowdown is caused by antivirus, thus they do not/cannot take any action/add an exclude antivirus rule. And it is never good to advise to disable antivirus, it does sound insecure and some company politics even forbids that explicitly. So please reconsider this PR vs. the disadvantages, there is no major disadvantage againts.

@mvorisek
Copy link
Contributor Author

@wisskid can this PR be reopened and merged, please?

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.

None yet

2 participants