Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid loading WordPress in `wp core verify-checksums` #2459

Merged
merged 10 commits into from Feb 9, 2016

Conversation

@mbovel
Copy link
Contributor

commented Feb 8, 2016

Hi,

I thought it would be nice not to load WordPress in wp core verify-checksums.

With this change, you can do:

wp core download
wp core verify-checksums

Before, you had to either specify version with --version or install and load WordPress before calling wp core verify-checksums.

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

Before, you had to either specify version with --version or install and load WordPress before calling wp core verify-checksums.

See #2086 (comment)

Using --version=<version> assumes you're passing the correct locale string if you need to be. I suppose we could document that better?

@mbovel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2016

Oh sorry, I didn't see the issue you're referring to.

I see the problem for $wp_local_package.

I understood how to use --version and --local, but I'm concerned about the security of doing wp core verify-checksums alone without arguments. Is it not a security issue to execute the files before checking them?

Couldn't we verify the checksums directly during wp core download, as we have all needed information about version and locale here?

'wp' => $wp_version,
'db' => $wp_db_version,
'tinymce' => $tinymce_version,
'local_package' => $wp_local_package

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Feb 8, 2016

Member

Sorry, to clarify, it's unsafe to use the $wp_local_package global without loading WordPress.

This comment has been minimized.

Copy link
@mbovel

mbovel Feb 8, 2016

Author Contributor

Why is this unsafer than loading the whole WordPress installation? Does WordPress redefine this global somewhere? (I've done a quick search and it doesn't seem to be the case.)

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Feb 8, 2016

Member

Does WordPress redefine this global somewhere? (I've done a quick search and it doesn't seem to be the case.)

I was under the assumption it could be repopulated by the database. But, on second thought, I don't see why that should impact verifying checksums.

@szepeviktor Thoughts?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Feb 8, 2016

Member

'local_package' => $wp_local_package

We should only use $wp_local_package if it's defined, which it's not in unlocalized downloads.

This comment has been minimized.

Copy link
@szepeviktor

szepeviktor Feb 9, 2016

Contributor

@danielbachhuber $wp_local_package is used throughout core but is a read-only variable.

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

Is it not a security issue to execute the files before checking them?

wp core verify-checksums isn't solely used for security. In fact, it can be a helpful tool, but I can't recommend it as a security solution as there are many ways to compromise WordPress while working around checksum verification.

Furthermore, you're including wp-includes/version.php, which could itself be compromised.

Couldn't we verify the checksums directly during wp core download, as we have all needed information about version and locale here?

wp core verify-checksums is intended a secondary, informational command.

* @type string $tinymce The TinyMCE version.
* }
*/
private static function get_version() {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Feb 8, 2016

Member

Can we rename this method to get_wp_details() or similar, given it's returning more than the version?

This comment has been minimized.

Copy link
@mbovel

mbovel Feb 8, 2016

Author Contributor

I hesitated for the name, I choose get_version() because, after all, the file is named version.php. But get_wp_details() sound perfectly OK to me too.

@@ -37,9 +37,9 @@ Feature: Validate checksums for WordPress install
And I run `wp core download --version=4.3`

When I try `wp core verify-checksums`

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Feb 8, 2016

Member

I try should be changed to I run, given you're changing this to a success statement.

@danielbachhuber danielbachhuber added this to the 0.23.0 milestone Feb 9, 2016

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

Thanks for your work on this!

danielbachhuber added a commit that referenced this pull request Feb 9, 2016
Merge pull request #2459 from mbovel/master
Avoid loading WordPress in `wp core verify-checksums`

@danielbachhuber danielbachhuber merged commit 6534ba7 into wp-cli:master Feb 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mbovel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

You're welcome, thanks to you for your amazing tool :)

I'm sorry I read CONTRIBUTING.md too late, I juste noticed that I should have made a separate branch for my changes, is there any way I can correct this or is it too late?

Also, would you like me to write documentation block for the find_var method? If yes, can I do it in mbovel/master?

Have a nice day!

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

I'm sorry I read CONTRIBUTING.md too late, I juste noticed that I should have made a separate branch for my changes, is there any way I can correct this or is it too late?

No, that's alright. It's fine as it is.

Also, would you like me to write documentation block for the find_var method? If yes, can I do it in mbovel/master?

Sure! You'll need to pull wp-cli/master into your master branch, then create a feature branch with your changes, then submit a new PR.

Pross added a commit to Pross/ClassicPress that referenced this pull request Mar 5, 2019
Move variables back to top of file.
WPCLI only fetches the first 2048 bytes of the version file with file_get_contents and so is returning invalid versions.
See https://forums.classicpress.net/t/wp-cli-compatibility/891/
This is where the parse instead of include was merged into wpcli wp-cli/wp-cli#2459
@Pross Pross referenced this pull request Mar 5, 2019
2 of 6 tasks complete
nylen added a commit to ClassicPress/ClassicPress that referenced this pull request Mar 6, 2019
Fix WP-CLI `wp core version --extra` (#396)
WP-CLI only fetches the first 2048 bytes of the `version.php` file with `file_get_contents` and so is returning invalid versions.  We can fix this by moving the variables WP-CLI wants to read back towards the top of `version.php`.

See:
- https://forums.classicpress.net/t/wp-cli-compatibility/891/
- wp-cli/wp-cli#2459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.