Fix mcrypt fatal - Removed on PHP 7.2

[vaurdan]

mcrypt library has been deprecated since PHP 7.1, and removed on PHP 7.2. This PR aims to replace the mcrypt implementation with the alternative openssl library. It also aims to keep backwards compatibility when mcrypt is still available on the system.

There will be issues decrypting old data, however: if the original data was encrypted with mcrypt, it will not be compatible with openssl, as the Rijndael AES is not available on openssl and it's not compatible with aes-256-cbc (source). If there is a way to make the old encrypted data retro compatible, I'm open to suggestions.

I also replaced the usage of serialize with wp_json_encode for PHPCS compliance.

Testing

I did a quick test on my PHP environment (PHP 7.4.21), and the encrypt/decrypt functions are working as intended:

$simple_data = "this is a simple example";
$complex_data = [ 
    'test' => 100, 
    'test_array' => [ 
        'other' => 'test', 
        123
     ]
];

Testing the simple string:

wp> $data = push_syndicate_encrypt( $simple_data );
=> string(60) "b2REUStDTGJISTY1Q0FwRXBJWWNKTkJ1R3Q5ekNSeUl3VUtOQUN5UGJHcz0="
wp> push_syndicate_decrypt ( $data );
=> string(24) "this is a simple example"

Testing with a complex array:

wp> $data = push_syndicate_encrypt( $complex_data );
=> string(120) "amRrU1l0Z1dWakdvZ1ByTng2KzNMYWo1QUNjdm5sREVRQmlHQWdIZ0Y2a0ZYcktLNUFwNW5NcDAzblU5d0c2ZzVCdThGOTFLeVMycFJ5YVNMZ0Q1UWc9PQ=="
wp> push_syndicate_decrypt( $data );
=> object(stdClass)#2883 (2) {
  ["test"]=>
  int(100)
  ["test_array"]=>
  object(stdClass)#2884 (2) {
    ["other"]=>
    string(4) "test"
    ["0"]=>
    int(123)
  }
}

TODO/Questions

• If there is no openssl implementation (or cipher) available, the push_syndicate_get_cipher returns false. We should add a warning on the dashboard disclaiming that the access tokens will not be encrypted.
• Is aes-256-cbc the best cipher for this? I believe this is the closest to the original MCRYPT_RIJNDAEL_256.
• How can we handle the potential backwards compatibility issues? Or is it a non-issue?

This PR will address and close #80, #144, and #150.

[GaryJones]

I think this is going to need unit and / integration tests to demonstrate that the encrypt/decrypt functions work on a range of strings, on a range of PHP versions.

How can we handle the potential backwards compatibility issues?

The plugin's readme.txt currently says it requires WP 3.4 - and that version of WP supports down to PHP 5.2, so ideally all of those versions would be tested in a GitHub Action workflow for the integration tests.

[vaurdan]

Thanks, @GaryJones! Actually writing a unit test for this functionality is a great idea, and I just added the first draft with 3 tests and 13 assertions. Let me know your thoughts.

My concern with backward compatibility is also on the plugin update action. When a user running PHP 7.0 updates to the new version of Push Syndication, they will have the data encrypted with the old cipher, and if they update PHP to 7.2 or later, it will be just garbage. We might add some code to the upgrade() method on the WP_Push_Syndication_Server class, but this migration will only be possible if the server is running PHP 7.1 or older, as it will need mcrypt to do the upgrade, as the MCRYPT_RIJNDAEL_256 cipher is not available on OpenSSL. ( my idea was to decrypt the option with mcrypt, encrypt it again with openssl, and save )

[vaurdan]

Thanks for the feedback @GaryJones! I just went ahead and refactored the code with your suggestions. It's possible that I might have overengineered it a little bit, but I'm open to more suggestions.

I have added a few more test cases, but I was wondering if I should simplify it with smaller tests but with more test cases. What are your thoughts on that?

[GaryJones]

I like where it's heading, but I've left a few more comments.

re tests - smaller is better, but more focused is even better - instead of only testing push_syndicate_encrypt, test the public methods of each individual class instead to test their functionality and responses - and then test push_syndicate_encrypt() to test the logic which decides which strategy is inserted in which context.

[vaurdan]

@GaryJones thanks! I was a bit conflicted if I should make it not static, so your input was decisive into doing that change. For now, it's using a global variable to store the object instance ( $push_syndication_encryption ), which is not the best approach. But I added a TODO so when we refactor the code we can store it as an attribute of the WP_Push_Syndication_Server object, for example.

Regarding the filename, I haven't change it from interface-syndication-encryptor.php to class-syndication-encryptor.php for a couple of reasons: there's already an interface (Syndication_Client ) using the same name scheme, and WPCS sniff for the class filename will only run on files with the class keyword, so the interface file doesn't trigger any CS warning.

Thanks for the PHP_VERSION_ID tip, way simpler than using version_compare!

[vaurdan]

@GaryJones take a look at the changes. I changed the EncryptionTest to stop using the global. However, I left an inline question on your original comment on that matter, if you can take a look. 🙂

Thank you!

[GaryJones]

I think we're getting there.

[vaurdan]

@GaryJones is there anything else missing on this one?
i think the @depends annotation was already added here.

[GaryJones]

One minor copypasta, and I'd otherwise love to see namespaces used for the new code, but these don't have to be a blocker.

The only consideration would be what happens if someone has data encrypted with mcrypt under PHP 7.1, then updates to PHP 7.2, and finds they can't decrypt using the OpenSSL?

.

[GaryJones]

There will be issues decrypting old data, however: if the original data was encrypted with mcrypt, it will not be compatible with openssl, as the Rijndael AES is not available on openssl and it's not compatible with aes-256-cbc (source). If there is a way to make the old encrypted data retro compatible, I'm open to suggestions.

This is still an issue.