cedaro / satispress

Expose installed WordPress plugins and themes as Composer packages.
500 stars 48 forks source link

Packages downloaded directly from upstream don't have corrected permissions the same way that update packages installed by WordPress do #203

Closed ethanclevenger91 closed 11 months ago

ethanclevenger91 commented 1 year ago

We have centralized SatisPress running, but are running into issues where the archives that get created are missing some vital read permissions on the files they contain, which persists to all projects downstream.

For example, here's a screenshot of a dependency we installed locally:

image

The same results can be achieved by downloading the SatisPress zip directly and unzipping, rather than installing via Composer.

The expected permissions are:

image

My experience has been that if I delete the latest archive (usually that's the one I'm trying to install anyway) and get SatisPress to regenerate it, it will contain files with the correct permissions. Usually by the time this happens, the latest version of the plugin has also been installed on the central site, so if SatisPress is zipping from there rather than grabbing from source, that could have something to do with the difference in behavior.

ethanclevenger91 commented 1 year ago

I've replicated this error with at least one plugin and one theme. The issue seems to happen specifically when SatisPress is downloading a package remotely. I was able to pull down the remote zip myself and see that the permissions issues originate with the upstream zip.

But WordPress is doing something to change the permissions to what it apparently deems a more appropriate set of permissions when it installs these themes/plugins, because if I install the update on the SatisPress install, the permissions in the theme directory are correct.

Digging deeper to see what WP is doing to change permissions.

ethanclevenger91 commented 1 year ago

Alrighty. So in most circumstances, WordPress copies over update packages on a file-by-file basis rather than moving them wholesale. The logic for that can be found here.

The actual copying of a file is passed along to the instantiated filesystem interface along with the constant FS_CHMOD_FILE, which is set early on in the WP lifecycle.

The direct filesystem will then reset the file or directory permissions. Same thing with directories.

I assume the other filesystem interfaces do the same.

To fix this and ensure that its archives match what WordPress would install rather than exactly what came from upstream, SatisPress would need to replicate this WordPress behavior, unzipping and rezipping archives.

If I were a maintainer, I'd probably call this a wontfix. As a user, getting upstream providers to fix this on their end will surely be a nightmare, and I don't look forward to it. So if someone wanted to take up the banner and fix it, I'd be eternally grateful.

bradyvercher commented 1 year ago

Hi @ethanclevenger91, thanks for the detailed write-up and troubleshooting! I did a bit of cursory research and it from what I can tell permission handling in zip archives is unreliable. I'm not sure relying on the permissions set by the vendors is the correct approach, either.

Instead you may want to explicitly set permissions during deployment or use a post install Composer script to set the correct permissions.

Have you been able to replicate this in more than one environment? Or is it only an issue on your local server?

ethanclevenger91 commented 1 year ago

Locally, I've identified zips that contain files/directories w/ incorrect permissions, though they don't result in 403 errors if fetched as part of a local WP page load - presumably because the user running the web server also owns the files (Local by Flywheel, FWIW).

In a Kinsta-hosted environment, also able to replicate permissions issues, and they do cause 403 errors. Presumably different file owner vs user that runs the web server.

The Composer documentation, re: permissions, says these features are not supported by ZipArchive, but doesn't really say what that means, exactly. Presumably it'll still unzip them, but maybe it just assigns them some default set of permissions?

I can say that unzip is available in the Kinsta environment, and Composer prefers it when available.

I can't find any documentation on the specific shortcomings of ZipArchive and whether those shortcomings are at zip or unzip time, and what the implications are. But given that the archives I'm getting are a mix of correct and incorrect permissions and that Composer is presumably using unzip in both environments I've replicated the issue in, I have to assume that permissions are being persisted through the upstream archiving process and the unarchiving process on my end.

bradyvercher commented 1 year ago

From what I'm able to gather, unzip does persist permissions, while ZipArchive doesn't support them when unzipping. Considering that the zip utilities handle permissions differently and vendors may not set them correctly -- and given that WordPress explicitly sets them -- I don't think you should rely on the persisted permissions to be correct. This sounds like an issue that would affect any private Composer repository and not just SatisPress.

If you're running Composer directly in your environments, then a post install script to correct file permissions sounds like your best bet.

ethanclevenger91 commented 11 months ago

I ended up doing this on the post-autoload-dump hook to catch both update and install:

"scripts": {
    "post-autoload-dump": [
      "find ./web/wp-content/plugins/memberpress -type d -exec chmod 755 {} \\;",
      "find ./web/wp-content/plugins/memberpress -type f -exec chmod 644 {} \\;"
    ]
  }