We have a problem similar to 2556683 but with Drupal 8.
The file permissions check fails on instances with multiple site folders because FilePermissions::getIgnoreList() only whitelists the file folder for the current site.
Create several site folders:
Then run drush @foo security-review.
The check should whitelist every site file folder.
I will try to provide a patch.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | security_review-3179937-2.patch | 975 bytes | prudloff |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
prudloff commentedThe attached patch adds every site file folder to the ignore list.
However, it will not work if
sites.phpuses some heavy logic or if the sites are not insites/.Comment #3
smustgrave commentedComment #4
smustgrave commentedWonder if you can check to see if this is still an issue?
I have a multisite setup and security review runs fine.
Comment #5
wrd-oaitsd commentedDo you mean in 2.0.0 or 2.0.x-dev? It's definitely still happening to me in 2.0.0. No files in ./sites/(current site)/files are listed, but all other files subdirectories are.
Hosting provider is Acquia, so there's a symlink involved pointing to another disk...I believe I remember seeing another issue posted that suggested that could complicate matters.
Comment #6
smustgrave commentedMoving back to NW. Do you my chance know if the patch in #2 works? Would have to reroll for testing most likely;
Comment #7
wrd-oaitsd commentedI don't, I'm afraid, but if I can get some spare time today I'll try applying those changes to 2.0.x-dev and see what happens.
Comment #8
smustgrave commentedAppreciate it. I'm running a multisite setup on my local with ddev and don't run into any issue. So seems to be a combo of symlinks and multisite I believe.
Comment #9
wrd-oaitsd commentedYes, the patch applies to 2.0.x-dev, and the only files that are still reported as being writable are:
./files/config_379e3322527d5df308dd4acb9423e91462be95c3/active
./files/config_379e3322527d5df308dd4acb9423e91462be95c3/staging
./files/config_379e3322527d5df308dd4acb9423e91462be95c3/sync
./files/config_379e3322527d5df308dd4acb9423e91462be95c3
./files/config_ae26d0a8465d9e41d0994809fa41ba9e76759f2f/active
./files/config_ae26d0a8465d9e41d0994809fa41ba9e76759f2f/staging
./files/config_ae26d0a8465d9e41d0994809fa41ba9e76759f2f/sync
./files/config_ae26d0a8465d9e41d0994809fa41ba9e76759f2f
./files/config_b951f8bc7460b537440ee2d86861db2db9875e17/active
./files/config_b951f8bc7460b537440ee2d86861db2db9875e17/staging
./files/config_b951f8bc7460b537440ee2d86861db2db9875e17/sync
./files/config_b951f8bc7460b537440ee2d86861db2db9875e17
./files
Comment #10
smustgrave commentedWhich is a valid report I think. Config should be stored outsides the default folder I believe.
Comment #11
wrd-oaitsd commentedYeah, which is a little odd, since I have that location set to ../sync/default. These might be very old config files leftover from some long-ago upload. I'll need to look into that.
But regardless, it seems that with the patch applied, I'm getting a good result.
Comment #12
smustgrave commentedOk I'll do some testing on my end on a regular site to make sure nothing is broken (just to double check) and if good we can merge.
Comment #15
smustgrave commented