Add plugin checksums verification #26

schlessera · 2017-12-21T14:56:20Z

Initial implementation of the plugin checksum verification.

The WordPress.org infrastructure now calculates MD5 and SHA-256 checksums for all plugin files and stores them in a publically accessible way. You can find a specification of the current endpoint to retrieve the checksums here.

The wp checksum plugin command goes through some or all of the plugins installed on a machine, downloads the checksums for each plugin, and then verifies the downloaded checksums against freshly generated ones.

The output on STDOUT will provide a list of checksum mismatches or added/removed files.
STDERR will contain warnings about skipped plugins.
The exit code will return 0 if all compared checksums were valid, and 1 otherwise.

Uses https://meta.trac.wordpress.org/ticket/3192

Resolves #15

… into plugin-checksums

…d not be retrieved.

…ieved.

See #22

Multiple checksums act as a whitelist, accepting any one of the available checksums as proof for the file's integrity. Fixes #24

swissspidy · 2017-12-21T19:55:57Z

src/Checksum_Base_Command.php

+	/**
+	 * Recursively get the list of files for a given path.
+	 *
+	 * @param string $path Root parse to start the recursive traversal in.


Fixed in 454a227

gitlost

It'd be nice maybe to have a --report option or similar to give more feedback but that's for another day...

gitlost · 2017-12-22T09:42:03Z

features/checksum-plugin.feature

+  Scenario: Multiple checksums for a single file are supported
+    Given a WP install
+
+    When I run `wp plugin install wptouch --version=4.3.22`


Could you add a comment here that wptouch has sha256 (and md5) checksums with multiple values per file as it's not immediately obvious what's being tested here...

I thought the scenario name Multiple checksums for a single file are supported already gave that away...?

Ha yeah it does, it just wasn't obvious to me why wptouch should test it... ignore at will!

Oh, I see. The choice of the plugin was based on the issue that reported the error: #24
I might add comments to link the tests to the relevant source issues then.

Added in 464be76

gitlost · 2017-12-22T09:48:00Z

src/Checksum_Plugin_Command.php

+		$plugins = $fetcher->get_many( $all ? $this->get_all_plugin_names() : $args );
+
+		if ( empty( $plugins ) && ! $all ) {
+			WP_CLI::error( 'You need to specify either one or more plugin slugs to check or use the --all flag to check all plugins.' );


Could you add a test to provoke this error (and similarly for other errors/warnings - where feasible!)?

Added in 9ef77d6

gitlost · 2017-12-22T09:54:07Z

src/Checksum_Plugin_Command.php

+
+		if ( empty( $this->errors ) ) {
+			WP_CLI::success(
+				count( $plugins ) > 1


At the moment if you do wp checksum plugin hello then you get a warning about checksums not being available and Success: Plugin verifies against checksums. at the end. It'd probably be better to use Utils\report_batch_operation_results() instead once (after the error report if any), and also have a skip count to give it.

Hmm...
I just tried using report_batch_operation_results() instead, but in the above case, that then tells us the plugin was already verified, which is nonsense of course:

If you keep a $skips count and then pass it it should work ok eg:

if ( ! empty( $this->errors ) ) { $formatter = new \WP_CLI\Formatter( $assoc_args, array( 'plugin_name', 'file', 'message' ) ); $formatter->display_items( $this->errors ); } $total = count( $plugins ); $failures = count( $this->errors ); $successes = $total - $failures - $skips; Utils\report_batch_operation_results( 'plugin', 'verify', $total, $successes, $failures, $skips );

Changed in 73efdd4

gitlost · 2017-12-22T09:56:29Z

src/Checksum_Plugin_Command.php

+	 */
+	private function get_all_plugin_names() {
+		$names = array();
+		foreach ( get_plugins() as $file => $details ) {


I'm wondering if the all_plugins filter should be applied here or not??

I think that would introduce a security vulnerability, as a malicious plugin could just filter itself out of the integrity check.

gitlost · 2017-12-22T09:59:37Z

src/Checksum_Plugin_Command.php

+	 * @return array<string> Array of files with their relative paths.
+	 */
+	private function get_plugin_files( $path ) {
+		// TODO: Make sure this works for all types of plugins (single files, must-use, ...)


I think for future proofing it should check here that $path includes a directory and isn't just a bare file like hello.php.

Ah, yes, thanks for catching the TODO, that does indeed still need an implementation.

So, I just checked, and the current code just works with bare files like hello.php as well. I don't think that we want to force the need for a folder for plugins.

However, must-use plugins are currently being ignored...

But $folder = trailingslashit( dirname( $this->get_absolute_path( $path ) ) ); where $path === "hello.php" will just cause the plugins directory to be traversed. I was thinking of it doing something like:

if ( false === strpos( $path, '/' ) ) { return array( $this->get_absolute_path( $path ) ); }

However, must-use plugins are currently being ignored...

I was wondering if they're ever stored on wordpress.org/plugins, and would checksums ever be provided?

Re. single file. Yes, you are right. I did some deeper checking, and it only worked right now because the checksum could not be retrieved for hello.php (as the hello-dolly plugin is different on the repository).

Re. must-use plugins, I know that some people just move existing plugins into the must-use folder for client sites, to make sure they can't be disabled. This is usually done by adding something like the bedrock mu-plugins autoloader.

I have fixed the single file issue (commits incoming). I suggest skipping the mu-checking for now and adding a separate issue for that, it might need more discussion.

Single-file support: 9251a68

Issue re. mu-plugins: #27

gitlost · 2017-12-22T10:05:39Z

src/Checksum_Base_Command.php

+			foreach ( $files as $file_info ) {
+				$pathname = substr( $file_info->getPathname(), strlen( $path ) );
+				if ( $file_info->isFile() && $this->filter_file( $pathname ) ) {
+					$filtered_files[] = str_replace( $path, '', $file_info->getPathname() );


Could you reuse $pathname here (also technically more correct!)...

I originally built it this way to have wp-content/plugins be the root and everything else related to that.
This means that you have the plugin's folder included in the file name for error messages, like akismet/akismet.php.
This serves two purposes:

Allow proper delineation between plugins in a folder, and single-file plugins that don't have their own folder.

Show whatever custom folder name the plugin happens to be installed in. Potentially, you can even have the same plugin installed in multiple versions, with differing checksum errors. Showing the folder helps you identify which one is being checked.

Can you elaborate why you think your suggestion is "technically more correct" ?

I just meant to do $filtered_files[] = $pathname rather than the str_replace() - it's a pet bugbear of mine when I see this str_replace() pattern used on paths when it should be the "correct" substr() - as used here for $pathanme - but totally ignorable...

I guess I can replace the str_replace() with a substr() anyway...

Oh, nvm, I just noticed it myself... Doing the change now! :)

Fixed in 22bc73e

gitlost · 2017-12-22T15:27:07Z

Good stuff. We have our first headline release feature!

nateinaction · 2018-01-08T19:55:28Z

Hey guys, really love this feature. I didn't see it explicitly mentioned in any of the changes for 1.5. Will this be included in WP CLI 1.5?

gitlost · 2018-01-08T19:56:54Z

Absolutely!

schlessera · 2018-01-08T19:57:01Z

@nateinaction Yes, it will be part of 1.5, coming soon!

nateinaction · 2018-01-08T19:57:29Z

Thanks again guys!

gitlost · 2018-01-10T20:33:10Z

@schlessera was wondering should a release be made of checksum-command now/shortly so that people can try it in the nightly?

Add plugin checksums verification

schlessera and others added 30 commits November 22, 2017 20:39

Rename Checksum_Command into Core_Command.

f48fe77

Make Core command executable.

cf5f83b

Add checksum namespace.

ce1ffd0

Add separate plugin command and provide base class.

00f3254

created plugin function same as core

85098d7

Merge branch 'plugin-checksums' of github.com:wp-cli/checksum-command…

42ecdf5

… into plugin-checksums

Remove copied plugin method again.

5f6f759

Add basic feature tests.

8d1406b

Provide basic plugin argument parsing.

98acdeb

Provide "--all" paramter parsing.

b13e467

Add arguments to synopsis.

7a37c7e

Move _read method into base class (and make it protected).

3321bc7

Move recursive file retrieval into base class.

2e62bd1

Load WordPress to execute the command, as we need access to plugin data.

6ad5600

added get_plugin_checksums function

a3cec9c

Use a URL template string to build the endpoint URL.

3be5b27

Always use braces for conditional code.

aa4360e

Adapt to latest JSON file structure.

13bfff3

Iterate over plugins and fetch checksums.

4981850

Fetch actual version string from plugin data.

b8df360

Scaffold the remaining logic.

49c06c6

Make sure either one or more plugins or the --all flag are provided.

4f8fda5

Print a warning if a plugin is being skipped because the version coul…

04dce50

…d not be retrieved.

Print a warning if the checksums for a given plugin could not be retr…

39df3ef

…ieved.

Display found errors in several different formats.

43d00c2

Add beginnings of the checksum check.

ea9c177

Iterate over checksum values, not keys.

e615c1b

Fix the path and file names handling.

f3703da

Add MD5 calculation.

5c70e99

Add SHA-256 algorithm.

0bfa0c4

schlessera added 3 commits December 14, 2017 04:19

Implement soft changes and --strict parameter.

b4f0785

See #22

Add --strict flag documentation.

f277f9c

Add support for multiple checksums for a single file.

e3ad28a

Multiple checksums act as a whitelist, accepting any one of the available checksums as proof for the file's integrity. Fixes #24

schlessera added the command:checksum-plugin label Dec 21, 2017

schlessera added this to the 1.0.6 milestone Dec 21, 2017

schlessera requested a review from a team December 21, 2017 14:56

schlessera mentioned this pull request Dec 21, 2017

Plugin / theme checksum verification wp-cli/ideas#6

Closed

Fix case mismatch in multiple checksum test.

5c30552

swissspidy reviewed Dec 21, 2017

View reviewed changes

Fix typo: parse -> path

454a227

gitlost suggested changes Dec 22, 2017

View reviewed changes

schlessera added 4 commits December 22, 2017 13:47

Add a reference to why wptouch plugin is being used in tests.

464be76

Replace redundant str_replace() with direct variable reference.

22bc73e

Add test for parameter logic check.

9ef77d6

Fix buggy behavior for single-file plugins.

9251a68

schlessera mentioned this pull request Dec 22, 2017

Add support for checksum verification of must-use plugins #27

Open

schlessera added 4 commits December 22, 2017 15:20

Use report_batch_operation_results() to report on verified plugins.

73efdd4

Second try at fixing single-file plugin support.

9326466

Adapt tests to reporting change.

cd77f6e

Fix failures count (number of unique plugins, not total failures).

1d8a74a

gitlost approved these changes Dec 22, 2017

View reviewed changes

gitlost merged commit 05e2f8e into master Dec 22, 2017

gitlost deleted the plugin-checksums branch December 22, 2017 15:25

schlessera pushed a commit that referenced this pull request Dec 23, 2021

Merge pull request #26 from wp-cli/plugin-checksums

e937b1e

Add plugin checksums verification

Add plugin checksums verification #26

Add plugin checksums verification #26

Conversation

schlessera commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitlost left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitlost Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitlost commented Dec 22, 2017

nateinaction commented Jan 8, 2018

gitlost commented Jan 8, 2018

schlessera commented Jan 8, 2018

nateinaction commented Jan 8, 2018

gitlost commented Jan 10, 2018

gitlost left a comment •

edited

Loading

gitlost Dec 22, 2017 •

edited

Loading