Sometimes Satispress creates an archive of the folder, not the content of the folder

[patrick-leb]

I don't know why, it's intermittent and I'm unable to reproduce on command (aka, the perfect bug...)

Sometimes, the archive for a plugin updated by WordPress is created like this:

drwxrwxr-x  4 wpcli wpcli 4096 Jan 21 08:45 ./
drwxrwxr-x 51 wpcli wpcli 4096 Jan 21 08:45 ../
drwxrwxr-x  3 wpcli wpcli 4096 Jan 14 10:44 __MACOSX/
drwxr-xr-x  9 wpcli wpcli 4096 Jan 14 10:44 woocommerce-product-search/

Where as it should be:

drwxrwxr-x  9 wpcli wpcli  4096 Jan 21 08:46 ./
drwxrwxr-x 51 wpcli wpcli  4096 Jan 21 08:46 ../
-rw-rw-r--  1 wpcli wpcli 39668 Jan 15 11:00 changelog.txt
-rw-rw-r--  1 wpcli wpcli  3060 Jan 15 11:00 COPYRIGHT.txt
drwxrwxr-x  3 wpcli wpcli  4096 Jan 15 11:00 css/
drwxrwxr-x  2 wpcli wpcli  4096 Jan 15 11:00 fonts/
drwxrwxr-x  2 wpcli wpcli  4096 Jan 15 11:00 images/
drwxrwxr-x  3 wpcli wpcli  4096 Jan 15 11:00 js/
drwxrwxr-x  2 wpcli wpcli  4096 Jan 15 11:00 languages/
drwxrwxr-x  7 wpcli wpcli  4096 Jan 15 11:00 lib/
-rw-rw-r--  1 wpcli wpcli 37790 Jan 15 11:00 LICENSE.txt
-rw-rw-r--  1 wpcli wpcli  1920 Jan 15 11:00 readme.txt
-rw-rw-r--  1 wpcli wpcli  4373 Jan 15 11:00 woocommerce-product-search.php
drwxrwxr-x  2 wpcli wpcli  4096 Jan 15 11:00 woo-includes/

All I have to do is delete the archive on my satispress install:

drwxrwsr-x  2 www-data web         4096 Jan 15 04:00 ./
drwxrwsr-x 78 www-data web         4096 Jan 12 10:51 ../
-rw-rw-r--  1 www-data www-data 1456341 Nov 14 16:00 woocommerce-product-search-3.3.0.zip
-rw-rw-r--  1 www-data www-data 1451881 Dec 19 16:00 woocommerce-product-search-3.4.0.zip
-rw-rw-r--  1 www-data www-data 1452693 Jan 11 12:00 woocommerce-product-search-3.5.0.zip
-rw-rw-r--  1 www-data www-data 1452190 Jan 15 04:00 woocommerce-product-search-3.5.1.zip

# rm -rf woocommerce-product-search-3.5.1.zip 

Go on the plugin list, uncheck Satispress, check it again and the new archive is constructed properly.

Has this happened to anyone else?

Both my servers are on Ubuntu 20.04 and I don't understand why this __MACOSX dir is there.

Thanks!

[bradyvercher]

It sounds like the vendor for that plugin is right clicking the folder on a Mac machine to create the zip file, which is why it has that __MAXOSX directory and is nested. WordPress seems to handle packages structured like that, most likely for a better user experience, but the files really should be at the top level.

SatisPress downloads updates as soon as they become available and caches them. When you delete that zip file, then uncheck/check the plugin, SatisPress will create a zip from the plugin source as it's installed in your WordPress instance instead. So the WordPress installer essentially normalizes the folder structure during installation.

The next release has a basic validator for zip files, but ideally it would check for this scenario and either skip caching it or fix the structure. I think is pretty much the same as #127.

[patrick-leb]

SatisPress downloads updates as soon as they become available and caches them.

It doesn't use the WordPress updater to do so?

SatisPress will create a zip from the plugin source as it's installed in your WordPress instance instead. So the WordPress installer essentially normalizes the folder structure during installation.

Stupid suggestion but instead of having this validation/normalization on Satispress side, would it make sense to schedule a wp-cron to create the cache from WP instance on its next run? This way we let WP do all the heavy lifting of validation/normalization and then get the end result. The downside is that it would delay updates via Composer update by the next wp-cron. Personally, it would be no issues whatsoever but I don't know about the rest of the community.

[bradyvercher]

It doesn't use the WordPress updater to do so?

No, it uses the URL supplied to the WordPress updater, which should be a publicly accessible URL to a zip file. Some vendors don't package things correctly or implement non-standard update processes, though.

Stupid suggestion but instead of having this validation/normalization on Satispress side, would it make sense to schedule a wp-cron to create the cache from WP instance on its next run?

Packages are already cached from source on upgrade if the release doesn't exist, so wp-cron probably wouldn't do much here. Not downloading updates directly from the remote source means they can't be exposed to Composer unless the theme or plugin is updated in WordPress first. It also means some versions may not be cached if they haven't been in installed in WordPress, so a complete history wouldn't be available.

Personally, I use SatisPress on a production website and don't want to update plugins before testing them in development, so there are some downsides to relying on the installed source only. I really think the package delivered directly from the vendor should be considered canonical in most situations.

[patrick-leb]

No, it uses the URL supplied to the WordPress updater, which should be a publicly accessible URL to a zip file. Some vendors don't package things correctly or implement non-standard update processes, though.

That's super interesting! I thought it did, so my "wp repo" has every single plugin activated and is slow as hell. So I can deactivate all of them?!

I really think the package delivered directly from the vendor should be considered canonical in most situations.

I totally get it now that I understand the underlying philosophy better - thanks for expanding on that. I update my production servers automatically for minor releases, but bigger ones I do check locally first indeed.

For what it's worth, I host maybe 30 odd 3rd party plugins, and every single time I had that issue, it's been that problem of "Pluginname.zip/Pluginname" which, as you said, seems to be the same issue as #127

[bradyvercher]

I thought it did, so my "wp repo" has every single plugin activated and is slow as hell. So I can deactivate all of them?!

Unfortunately, no. The code has to run for the custom updaters to run, so the plugins need to be active. I guess if there were significant performance benefits, it would be possible to write adapters for each vendor and the plugin might not even have to be installed in the first place, but that sounds like it'd be a maintenance nightmare.

For what it's worth, I host maybe 30 odd 3rd party plugins, and every single time I had that issue, it's been that problem of "Pluginname.zip/Pluginname" which, as you said, seems to be the same issue as #127

At the very least, it should be possible to check if the top level only contains directories and skip updates from the remote URL in that case. Normalization could be implemented in a future release.

[bradyvercher]

@patrick-leb I'm pretty sure the HiddenDirectoryValidator in the latest release should prevent this from happening in the future. Updates for plugins that do contain the __MACOSX directory in the top level won't be cached until after the plugin is upgraded in WordPress, then it will be cached from the installed source instead of the remote URL. Let me know if you still have issues with this after updating.

[patrick-leb]

@bradyvercher I thought we were out of the woods with that one but it's still happening :( I had 2 occurrences of packages with the containing the nested directory e.g:

app/plugins/b2b/b2b/
app/plugins/b2b/__MACOSX/

It happened with another plugin as well (woocommerce-product-search, AGAIN).

Seems like it's always the same vendors not packaging their thing properly.

[bradyvercher]

@patrick-leb Dang! I thought that validator would handle that scenario. Do you have any idea why it's making it past that?

[patrick-leb]

I have no idea - I'm not even sure how to reproduce it... 😞

I was wondering since, by nomenclature (I believe...), WP is requiring the plugin entry file to be the same name as the folder. e.g: if my plugin folder name is woocommerce-product-search it will require a woocommerce-product-search.php file in the directory (please correct me if I'm crazy).

Maybe we can do a validator that checks that before building the archive?

[bradyvercher]

The validator is standalone, so you just need to feed it the path to a zip file and a release instance and it should let you know whether it's valid. There's a test for it here.

Maybe the validator logic needs to be tweaked to account for different directory separators? Or the exception could be getting caught at some point?

WordPress doesn't put any restrictions on the name of the plugin file. It could be index.php or I think a lot of plugins used plugin.php at one point. You could even have multiple plugin files in the same directory.

[patrick-leb]

You could even have multiple plugin files in the same directory. Didn't think about that... of course.

I've downloaded the plugin from Woocommerce.com but it's zipped properly (aka no double folder, no __MACOSX folder). I don't know what they are doing. I don't know if they have a CRON that cleans up the archive and in some cases, my WP installs the update before it had a chance to run.

Yet again, it should be caught by the validator. 🤷

I don't have time to investigate a lot these days, but I'll to put some logging on the live site to see what gives.

[patrick-leb]

I tried to debug the problem but your code is too smart for me :)

The validator works in the unit tests with my invalid archives. so that's not the problem.

It looks like to me, the code should work as intended. I don't know why it doesn't.

Is there a way to prevent Satispress from checking the source URL and check the installed source instead?

Something like having a flag in src/ReleaseManager.php:L88 that allows you to bypass the source_url and go directly to the "archive_from_source" call?

[bradyvercher]

Here's something you could try:

1. Downgrade one of the offending plugins to the previously installed version.
2. Delete the invalid artifact for the latest release from the cache directory.
3. Visit the Plugins screen or the Dashboard → Updates screen.

That should trigger an update check and automatically cache the latest release. If you inspect that file and it's fixed, then the vendor may have cleaned things up after the initial release.

It still doesn't explain why the validator didn't work. Are you certain the invalid artifacts were cached after upgrading to a version of SatisPress with the validator present?

There isn't a way to bypass that check at the moment. Archiving from source currently only happens when adding a package to the SatisPress repo or if one of those validators fails and the theme or plugin is updated in WordPress, so it requires manual action. Although with automatic updates enabled, that could potentially work similar to updating from the remote URL.

[patrick-leb]

So I do something similar (I guess) when it happens:

1. Downgrade the version on the target machine
2. delete invalid zip file from the upload dir of satispress
3. Go in Options > Satispress and uncheck/check the offending plugin.

It works as well.

Are you certain the invalid artifacts were cached after upgrading to a version of SatisPress with the validator present?

Yes, it happened with 0.7 and version 1.0 It happened again on April 6th. A customer called me asking why the B2B plugin disappeared (ouch...)

Although with automatic updates enabled, that could potentially work similarly to updating from the remote URL.

For my use case, it would be perfect. My Satispress install is basically a headless install and its sole purpose is to have auto-update activated and update all my paid plugins.

https://github.com/cedaro/satispress/blob/06678c7cd6fa5f16767d5d5e4f301383d12f0547/src/ReleaseManager.php#L87-L93

This looks where we could bypass it, no? something (pseudo coding) like

        if ( ! empty( $source_url ) && ! $get_from_local_only ) {
            $filename = $this->archiver->archive_from_url( $release );
        } elseif ( $package->is_installed() && $package->is_installed_release( $release ) ) {
            $filename = $this->archiver->archive_from_source( $package, $release->get_version() );
        } else {
            throw InvalidReleaseSource::forRelease( $release );
        }

What are your thoughts?

[bradyvercher]

The method I suggested was more for troubleshooting the source URL to double check that it was still coming from the vendor with the hidden directory. By deleting the cached artifact and the update_plugins transient, you could also drop some logging in there and repeatedly test why it was making it past the validator.

There are several use cases where automatic updates wouldn't be desired. Even for a headless SatisPress repo, a bad update could bring the server down.

I'm not sure adding a flag there would totally fix the issue because SatisPress would still try to cache available updates, so it'd fall through and throw an exception in the else statement. There are also a couple parts of the code that try to cache updates just-in-time if they don't exist. So the logic would need to be tweaked a little.

My thinking is this is a bug that and the validators should handle it. I'd prefer to squash that instead of introduce a feature to side step it and potentially introduce more unexpected behavior. Squashing the bug should benefit all the use cases as well.

If you really, really want to disable caching from remote URLs, you could register your own validator:

add_filter( 'satispress_artifact_validators', function() {
    return [
        new class implements \SatisPress\Validator\ArtifactValidator {
            public function validate( string $filename, \SatisPress\Release $release ): bool {
                return false;
            }
        }
    ];
} );

Or filter the package download URLs to prevent them from even being downloaded:

add_filter( 'satispress_package_download_url', '__return_false' );

I haven't tested either of those methods, but they should work in theory...

[patrick-leb]

Well I found the problem and I can't believe I didn't catch earlier: The unit test for the macosx validator never runs! lol

https://github.com/cedaro/satispress/blob/develop/tests/phpunit/Integration/Validator/HiddenDirectoryValidator.php

The filename is missing Test -> it should be HiddenDirectoryValidatorTest.php

and THEN the test fails, the validator doesn't work :)

Edit: It works with your mockup file, but turns out the __MACOSX dir is not a dir according to zip... EG:

➜  b2b git:(master) ✗ unzip -l b2b-1.7.1.zip 
Archive:  b2b-1.7.1.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  04-08-2021 04:56   b2b/
      210  04-08-2021 04:56   __MACOSX/._b2b
      464  03-25-2021 21:37   b2b/class_afb2b_front.php
      212  03-25-2021 21:37   __MACOSX/b2b/._class_afb2b_front.php
        0  04-08-2021 04:56   b2b/additional_classes/
      210  04-08-2021 04:56   __MACOSX/b2b/._additional_classes
        0  04-08-2021 04:56   b2b/products-visibility-by-user-roles/
      210  04-08-2021 04:56   __MACOSX/b2b/._products-visibility-by-user-roles
     6148  03-25-2021 21:37   b2b/.DS_Store
      212  03-25-2021 21:37   __MACOSX/b2b/._.DS_Store
    18894  03-25-2021 21:37   b2b/class_afb2b_admin.php
      212  03-25-2021 21:37   __MACOSX/b2b/._class_afb2b_admin.php
        0  04-08-2021 04:56   b2b/images/

It's reported like that as well by pclZip:

{
    "filename": "b2b\/",
    "stored_filename": "b2b\/",
    "size": 0,
    "compressed_size": 0,
    "mtime": 1617890170,
    "comment": "",
    "folder": true,
    "index": 0,
    "status": "ok",
    "crc": 0
}

{
    "filename": "__MACOSX\/._b2b",
    "stored_filename": "__MACOSX\/._b2b",
    "size": 210,
    "compressed_size": 109,
    "mtime": 1617890170,
    "comment": "",
    "folder": false,
    "index": 1,
    "status": "ok",
    "crc": 2818760342
}

[bradyvercher]

Dang, I missed that, too! Thanks for digging into it more. Could you send me a copy of that plugin so I can update the test data and fix that validator (brady at blazersix dot com).

[patrick-leb]

Sent! thank you so much!

[patrick-leb]

@bradyvercher Just checking with you if you had the time to download the plugin before the link expired?

[bradyvercher]

Thanks for sending that link, @patrick-leb. I did have a chance to download it, so I'll try to look into that in the next day or two.

[JulienMelissas]

Hey! I just experienced this with another plugin and found this GitHub issue. I completed the following steps:

1. Updated to the latest version of SatisPress
2. Turned the plugin off from managed plugins in SatisPress settings
3. Downgraded to an older version of the plugin
4. Deleted the SatisPress archive of the newer plugin from the SatisPress server
5. Upgraded the version of the plugin
6. Turned the plugin back on from managed plugins in SatisPress settings
7. Visited the plugins page (which did regenerate the archive in SatisPress' cache)
8. Cleared all caches (including composer's cache)
9. Deleted the plugin folder from my local machine
10. Did a composer require satispress/plugin-name

And am still seeing the __MAXOSX folder

Anything I might be missing in my above steps? I'd be happy to help to debug or provide more information :) Thanks!

[bradyvercher]

Hi @JulienMelissas, sorry to hear you're having an issue with this!

Are you running the latest version of SatisPress? If so, could you send me a copy of the plugin and I'll try to do some debugging (brady at blazersix dot com).

[JulienMelissas]

Hey @bradyvercher - thanks for the quick reply. And thanks for the great plugin... I don't know what we'd do without it.

So I said I cleared all caches on last week but today after you wrote in I took a look and ran a composer clear-cache locally, deleted the plugin folder, ran a composer install and... it's working now. So it must have been a cache somewhere getting in the way. Thank you and sorry to revive this old thread!