If client-side verify_key is not enabled, don't check it automatically #547
Conversation
…ey verification is disabled to avoid injection issues
There was a problem hiding this comment.
Implementation wise, this LGTM. In terms of:
Note that I am still not completely sold on making this change.
I wanna note that as an impacted user: explicitly documenting previously the breaking change and leaving it in place, or making us default to enabling MEMCACHED_BEHAVIOR_VERIFY_KEY and calling out that change are both 1000% acceptable to me! As long as I have a heads up that there are key requirement changes when I upgrade, I think it's totally acceptable, the only thing that is scary is upgrading and finding a new requirement without knowing what the requirements are or why keys started failing.
| @@ -231,14 +231,21 @@ zend_bool s_memc_valid_key_binary(zend_string *key) | |||
| } | |||
|
|
|||
| static | |||
| zend_bool s_memc_valid_key_ascii(zend_string *key) | |||
| zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key) | |||
There was a problem hiding this comment.
I was going to make this suggestion, but then I realized the setting is defined as 64-bits. So it goes!
Line 4229 in 7fefcb7
| zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key) | |
| zend_bool s_memc_valid_key_ascii(zend_string *key, zend_bool verify_key) |
There was a problem hiding this comment.
Yes, that is why I made it a uint64_t, but perhaps we should flip that one to a REGISTER_MEMC_CLASS_CONST_BOOL ?
libmemcached's key verification is only done if OPT_VERIFY_KEY is enabled. We should honour this setting in the PHP extension as well and not do it automatically.
This addresses #546
Note that I am still not completely sold on making this change.