As a result of #1914018: hook_requirements() for un-protected configuration directories I believe file_scan_directory() need to exclude the config_*
folder in all cases. The nomask param need to be extended to remove all config_*
folders..
$options += array(
'nomask' => '/^CVS$/',
'callback' => 0,
'recurse' => TRUE,
'key' => 'uri',
'min_depth' => 0,
);
Comment | File | Size | Author |
---|---|---|---|
#21 | core-config-exclude-doc-1914914-21.patch | 825 bytes | jason_purdy |
#19 | core-config-exclude-doc-1914914-19.patch | 826 bytes | jason_purdy |
#17 | core-config-exclude-doc-1914914-17.patch | 813 bytes | jason_purdy |
#14 | core-config-exclude-doc-1914914-14.patch | 764 bytes | jason_purdy |
#9 | 1914914-documentation.patch | 740 bytes | meba |
Comments
Comment #1
gddThe config folders can be renamed to something else, so removing config_* folders would not suffice. It would also open up the possibility of stripping out folders that begin with 'config_' and are not related to core. A better option (if it is determined this is necessary at all) would be to add the directories from the $config_directories global.
I'm still not convinced this is necessary though. There may be legitimate reasons to list this directory (for instance when building a deployment tool.) I think it is a better option to leave this to contributed module authors to manage.
Comment #2
gddComment #3
gregglesbetter tags.
Comment #4
chx CreditAttribution: chx commentedWe need to amend the doxygen to something like "scanning the whole public files directory with file_scan_directory can lead to severe perfomance problems. if you must, use your own subidrectory but it's better not use this function on user uploaded content at all -- rather use the file managed API to keep a record of your files". That's all is needed here and that's a useful addition anyways.
Comment #5
gddRetagging because I'd still like some more opinions.
Comment #6
hass CreditAttribution: hass commentedWhy have you renamed the title? Please read the issue where I came from.
If admin can rename the config_* folder we have more troubles than I expected as no module maintainer can solve this issue on it's own to keep the config dir secured and hidden from their users prying eyes. Just think about ICME where everyone is able to read all config files.
This config files in the file system become more and more troubles. Core need to provide something to maintainers of file management modules.
Comment #7
gddDid you actually read what I said above? Any file management module can get the current config directory from the $config_directories global and hide it themselves if they feel it is necessary for their use case. I believe this is the appropriate measure. chx is on the security team and he agrees. I have asked other members of the security team to comment if they feel there is a problem with this.
Comment #8
jhodgdonSo... What are we needing to add to the documentation? I'm confused.
Comment #9
meba CreditAttribution: meba commentedSomething like this
Comment #10
jhodgdonUm. I think it would be fairly obvious that scanning the entire public files directory would take some time? Why do we need to say this?
Also, if you want to say to use the Managed Files API, it needs to be a link or have a @see, or better yet some explanation of why this is better or what it buys you, or which function to use to scan for files using that API.
And there are problems with punctuation and capitalization in the last line. Should be "module-related files" and "Managed Files API".
Comment #11
hass CreditAttribution: hass commentedComment #12
jhodgdonNormally, documentation changes are left in the documentation component.
Comment #13
jason_purdy CreditAttribution: jason_purdy commentedComment #14
jason_purdy CreditAttribution: jason_purdy commentedAdded documentation to file_scan_directory() to remind people that config_ directories could be included under the right conditions. I also noted how they could make sure their code is more secure by using the right parameters.
Comment #15
jason_purdy CreditAttribution: jason_purdy commentedComment #16
jhodgdonThanks!
This patch has a few typographical issues:
a) assured ==> ensured
b) needs a comma after "recursion" in the 3rd line (we use serial commas in the Drupal project by convention)... but see (c) below.
c) And also, as noted above, this is not really accurate. Limiting recursion will not actually help with security, but only with the performance issues noted above. And adding "config" specifically is not going to work for someone who has used a different config directory. So I think we should take out the recursion part, and not talk about "config_" specifically, instead say something more generic about "configuration directories" and "use the nomask option to skip configuration directories".
Comment #17
jason_purdy CreditAttribution: jason_purdy commentedAwesome, thanks for the great feedback. I'm trying again with another patch (attached).
Comment #18
jhodgdonLooks pretty good! You need a comma before "and" here:
And... Maybe it would be clearer to say something like "you can use the ... options to improve performance..."?
Comment #19
jason_purdy CreditAttribution: jason_purdy commentedI'm getting there! :)
Ok, another patch file attached. Thanks for the feedback/review! :)
Comment #20
jhodgdonThanks -- text looks good!
There is an extra space at the end of one of the lines though. Most programming editors can be configured to remove or highlight spaces at the ends of lines -- if you are going to continue working on Drupal patches (and I hope you will!), you might want to look for that setting.
Comment #21
jason_purdy CreditAttribution: jason_purdy commentedD'oh! Ok, removed. Thanks for catching that.
Comment #22
jhodgdonThanks!
Comment #23
jhodgdonThanks again everyone! Committed to 8.x.
Comment #24
jhodgdon