Restrict temporary files created by text editors

Perhaps we should add a comment for why that part of the filter is there?

I agree a comment is unnecessary for this hunk.

A followup issue could be created to comment the whole thing if someone feels like it.

re-roll attached with a somewhat less greedy regex. It's up for discussion IMO if the regex should also be limited specifically to filenames that are likely to contain php (*.php,module,inc) or even just variations on settings.php. (However, this would not protect someone who includes sites/all/db.inc or the like from a settings.php file)

I'm surprised. Doesn't it make more sense for us to use a whitelist for this? Aren't we always going to be playing "catch-up" with the future, which may bring us new IDEs with new backup file extensions, new version control systems, et cetera? At some point, we could even allow modules to modify the whitelist under certain conditions.

See #581706: Protect .git, .hg and .bzr directories in .htaccess for a related discussion.

I agree in principle with #7. However, instituting a whitelist seems at least on its face to be a much more major change, with potential for a correspondingly longer development/testing/acceptance period. My personal feeling is that it should be a separate issue, and we shouldn't let perfect be the enemy of good in this case. (and probably #581706: Protect .git, .hg and .bzr directories in .htaccess as well)

My personal feeling is that it should be a separate issue, and we shouldn't let perfect be the enemy of good in this case.

@acrollet, yes, a separate issue is probably best. I added #1907934: Allowlist, instead of denylist, sensitive files and directories.

@acrollet I lean towards:

1) Blacklisting backups of *.php files
2) (and) Blacklisting backups of otherwise blacklisted extensions (*.inc, *.module, etc -- a list which doesn't actually include *.php)

Rationale: Under D7+MAMP, I observed that the blacklist applies to both the file-system and to Drupal content (e.g. nodes published with path.module). I think it's legitimate for a node to have the path "example.bak" -- but the file "example.php.bak" should not be accessible. The attached tightens the blacklist per #1 & #2. It also adds other common extensions (e.g. *.php.bak, *.php.orig)

Re: .git and .svn -- on MAMP and Debian Squeeze, all dot-files appear to be prohibited by default (with or without Drupal's .htaccess). This policy seems to provide pretty broad protection for current/future SCMs.

Re: .git and .svn -- on MAMP and Debian Squeeze, all dot-files appear to be prohibited by default (with or without Drupal's .htaccess). This policy seems to provide pretty broad protection for current/future SCMs

@totten, of course, this is a good thing, if we can rely on protection in particular Linux distributions. However, doesn't Drupal have a much wider user base?

#1: restrict-text-editor-files-1907704-1.patch queued for re-testing.

Sorry if I introduce noise here but wouldn't it be better to use dedicated .htaccess files in sites/default and sites/default/files to deny all accesses in default directory and allow all accesses in files directory (principle close to the one used in sites/default/files/private)?

If such approach is kept, the procedure given at http://drupal.org/documentation/install/multi-site would probably be impacted.

I saw the same article co-incidentally yesterday and started looking at the Security Review module as a place to make checks.

The article also mentions nano '.save' files as well as vim #settings.php# which the patch seems to miss.

I think it catches the #settings.php# but not .save.

Needs work for the .save files.

re #13 - it's hard to say whether this should impact the whole site or just specific directories. I'd rather it affect everything for now. I've definitely seen sites where important IP addresses or the keys for various services were hard-coded into .module files or .php files inside a module folder and it seems great if we can protect those while we're at it.

ghezlewood: The regex '^#.*#$' should catch '#settings.php#'.

re #15 - Understood. I am just afraid you miss some cases using the current approach.
Anyways, I will probably add some Directory tags in my Apache configuration to make sure that nothing can be downloaded from sites/default/.

re-rolled to additionally restrict .save files.

@acrollet you beat me to it :) Was unsure as to why there is repetition in there though, why are the parts repeated?

ghazlewood: the first section restricts back-up versions of the files that are already restricted (module,inc,tpl, etc) - The second section restricts back-up versions of .php files, which can not be restricted whole-sale, or index.php would not load :)

Thanks, I had a feeling it was for good reason.

This seems like a better tag.

This sounds good. That regex is starting to get ugly as hell though. :)

Committed to 8.x. I'll push once testbot has caught up a bit.

Seems this might be worth a backport to D7?

D7 patch is identical to the D8 version, RTBC'ing

Wow, that's complicated-looking but seems to make sense and work as expected. Nice work.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/a16de0d (I removed the trailing spaces on commit, by the way; those weren't in the Drupal 8 patch but somehow wound up in the Drupal 7 one.)

Not much is getting backported to Drupal 6 these days, but as a security improvement maybe this one would be a good idea still?

Apparently this patch broke things on Apache 1.x. Reviews welcome at #1962780: 500 Internal server error on Apache 1.x servers after updating to Drupal 7.22 if anyone has time.

Oops, would need that issue fixed first, but I didn't mean to remove the tag.