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.

[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?

[vaurdan]

@GaryJones thanks for all the feedback! I did another refactor, and changed the Syndication_Encryptor to an interface, and moved the encryptor loading logic to the plugin root file, along with all the requires and includes.

I also followed your suggestion and tried to make the unit tests as specific as possible, and tested every single public method and encryption/decryption functions separately. I ended up doing an abstract EncryptorTestCase that should take care of all the basic encrypt/decrypt tests for Encryptors, and each Encryptor test extends from it.

Let me know your thoughts :)

[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. :slightly_smiling_face:

Thank you!

[vaurdan]

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