search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

Wrong permissions set on directories created through SS v5 #550

Closed lars-lemon8 closed 1 year ago

lars-lemon8 commented 1 year ago

It seems that new 'asset' directories created by SS getting a 'private' visibility. (0700 permissions instead of 0775). This makes the directory unreachable from the web if apache runs as a different user. The exact same server does not have this problem with SS4.

PRs

GuySartorelli commented 1 year ago

Can you please provide more details? Especially specific directories involved (I ask because in discussions on slack you explicitly mentioned folders related to tinymce) and steps to reproduce this from a fresh installation.

derkgort commented 1 year ago

First you run dev/build, this will remove the _tinymce dir in the assets folder. Then edit a page, this will create the _tinymce folder. Now the _tinymce has the permissions: 0700 drwx------ 2 test TB23-001 38 May 17 11:33 _tinymce The same for when you create a new folder in the assets and publish an asset in there:

drwx------ 2 test TB23-001 140 May 12 12:38 test

derkgort commented 1 year ago

Also note, in vagrant this cannot be reproduced. That's because there is some magic happening in the filesystem layer.

kinglozzer commented 1 year ago

I can reproduce this, just published an image in a directory named test:

-rw-rw-r--+ 1 www-data     www-data  822 Mar 14 16:07 error-404.html
-rw-rw-r--+ 1 www-data     www-data  717 Mar 14 16:07 error-500.html
-rw-rw-r--+ 1 www-data     www-data 1226 May 11 11:01 .htaccess
drwx--S---+ 2 www-data     www-data 4096 May 17 10:40 test

Permissions should be inherited from this config, but I guess that’s not happening for some reason:

https://github.com/silverstripe/silverstripe-assets/blob/de229ea2a80e1489029c309a97a2d5d27e0228fd/src/Flysystem/AssetAdapter.php#L39-L54

kinglozzer commented 1 year ago

Okay it looks like those config settings are being picked up when the adapter is constructed, I wonder if this could actually be related to this issue: https://github.com/thephpleague/flysystem/issues/1584

lars-lemon8 commented 1 year ago

Yeah, this sounds fishy indeed:

"The problem is that setVisibility() will only be called if the Filesystem instance Config key visibility is set"

emteknetnz commented 1 year ago

I debugged this to work out what's going on in both CMS 4.13 and CMS 5.0

Both versions use the same config

    private static $file_permissions = [
        'file' => [
            'public' => 0664,
            'private' => 0600,
        ],
        'dir' => [
            'public' => 0775,
            'private' => 0700,
        ]
    ];

CMS 4.13 uses league/flysystem v1 and CMS 5.0 uses league flysystem v3

Observed behaviour when uploading a jpg to a new folder called 'abc':

CMS 4.13:

CMS 5.0

It looks like publishing the file copies over permissions to the new folder created in the public asset store. This is an assumption on my part I didn't go as far as debugging to validate this.

Essentially CMS 4.13 was using the 'public' config instead of the 'private' config in the protected asset store. I debugged the reason for this to here in league/flysystem v1 - https://github.com/thephpleague/flysystem/blob/1.x/src/Adapter/Local.php#L103 -- it's hardcoded to 'public'

league/flysystem v3 isn't hardcoded and actually does what you'd expect it to, unfortunately this totally breaks the filesystem if you don't have the exact same user doing everything

Since for CMS 4 we've been using 0775 permissions the entire time, I think we should change the 0700 to 0775 and call it a bugfix and release this as a patch. With CMS 4 we've (likely unintentionally) created an expectation that you can create folders with a non www-data user which makes things easier to deploy, particularly on hosting setups where the developer has little to no control over the webserver itself. We don't seem to mention anything in the webserver configuration docs that says otherwise.

emteknetnz commented 1 year ago

Note that file's (rather than folders) seem to work correctly in CMS 4.13 with leauge/flysystem v1. CMS 5.0 behaves correctly too

i.e. upload a draft file - permissions on the file are 0600 publish the file - permissions on the file are 0664

GuySartorelli commented 1 year ago

PR merged and tagged as 2.0.1