You might want to remove that restriction for /assets/modules/, and there are some other files that may also be affected (e.g., assets/js/htcmime.php, possibly some files in /assets/plugins/tinymcexxx/, and any custom PHP files saved in /assets/site/, /assets/templates/, etc. For example, I have a dynamic CSS file on one site that is a PHP file, and it’s in the main assets folder. I think I’ve also put some PHP audio jukebox scripts in /assets/flash in the past...
So it might cause more grief than it’s worth for some folks, although it’s good to have it out there as an option for them that want it.
To me this once again reinforces the trouble with mixing code and other files in the current assets folder, which thankfully will be resolved when Revolution comes out.
Quote from: tuatara at Dec 17, 2008, 03:31 PM
You can also turn RewriteEngine off, so MODx won’t be left responding to 404s in the assets folder, which is expensive.
That plus an ErrorDocument 404 seems like a very good idea indeed.
Quote from: tuatara at Dec 17, 2008, 05:20 PM
From a security perspective, I think it’s better to disallow access; those who are skilled enough to know what they’re doing will easily be able to allow specific script access.
I’m not saying it’s a bad idea; just trying to identify things that would need to be addressed in order to not break people’s existing installations. There certainly are other ways to deal with the htcmime.php file, so it would just need to be done differently in future releases. The assets/modules/ folder, on the other hand, would probably need to be an exception altogether.
As long as this isn’t the only thing done to resolve this issue I think it’s fine. Not everyone uses Apache (and not all Apache installations allow for local .htaccess overrides), so while it would work for a large percentage of users it wouldn’t cover everyone. The most important change that I’d like to see in future releases is that all files with snippet code that is included just to be copied into the database be .txt files (not .php).
Quote from: ZAP at Dec 17, 2008, 05:29 PM
Quote from: tuatara at Dec 17, 2008, 05:20 PM
From a security perspective, I think it’s better to disallow access; those who are skilled enough to know what they’re doing will easily be able to allow specific script access.
I’m not saying it’s a bad idea; just trying to identify things that would need to be addressed in order to not break people’s existing installations. There certainly are other ways to deal with the htcmime.php file, so it would just need to be done differently in future releases. The assets/modules/ folder, on the other hand, would probably need to be an exception altogether.
As long as this isn’t the only thing done to resolve this issue I think it’s fine. Not everyone uses Apache (and not all Apache installations allow for local .htaccess overrides), so while it would work for a large percentage of users it wouldn’t cover everyone. The most important change that I’d like to see in future releases is that all files with snippet code that is included just to be copied into the database be .txt files (not .php).
Having them be .txt files can be a pain for development since it means they can’t be executed in a debugger, may open in the wrong editor, and may not have the proper highlighting once loaded editor. They could be switched to .txt files on install but during development, it’s no fun to keep switching them back and forth. It might be better to have them be .inc files (or .cls for class files).
-
MODX Staff
- 2,502 Posts
I am no expert at security but it seems to me that for the benefit of security all distributed files as snippets should remain as txt files. Yes it is a pain in development but you as the dev can easily rename it as php for dev and then either delete it or rename it for installation. I think that convenience for knowledgeable devs should come 2nd to security for unknowing users and installers.
I do see lots of great debate here and a number of possible solutions.
Author of zero books. Formerly of many strange things. Pairs well with meats. Conversations are magical experiences. He's dangerous around code but a markup
magician.
Blog ✦
Twitter ✦
LinkedIn ✦
GitHub
Quote from: BobRay at Dec 17, 2008, 05:42 PM
It might be better to have them be .inc files (or .cls for class files).
As far as security is concerned they could be shipped with any extension that’s not generally executable as PHP, but you also want users to be able to easily open them so that they can copy and paste the code into the Manager so I think .txt is still the best option.
-
MODX Staff
- 10,725 Posts
The best idea is not to ship components with the core at all so it’s not associated with the code that is vulnerable, and to make sure when you do author php files that are intended to be executed directly or as a snippet that you take necessary precautions in the code. Renaming php files to other file extensions and all these other workarounds are not only inconvenient and limiting IMO, but completely unnecessary unless you have register_globals On or some other attack vector (besides just some snippet php script) open in your environment.
I’ve had major hacking problems with the reflect snippet.
dont know how they did it (im not a hacker) but they’ve launched DOS attacks, used my server as a spam machine, insert trojan horses.
I’d advise people to uninstall the reflect snippet until more is know about this!
