Deprecation of constant_time_compare broke usage with mixed-type arguments.

[Sage Abdullah]

The deprecation of constant_time_compare in #36546 (0246f478882c26bc1fe293224653074cd46a90d0) removed the force_bytes conversion of the arguments passed to the function. The function now raises an error if passed arguments of different types, e.g. bytes and str. Test:

tests/utils_tests/test_crypto.py

@@ -21 +21 @@
         self.assertFalse(constant_time_compare(b"spam", b"eggs"))
         self.assertTrue(constant_time_compare("spam", "spam"))
         self.assertFalse(constant_time_compare("spam", "eggs"))
+        self.assertTrue(constant_time_compare(b"spam", "spam"))
+        self.assertFalse(constant_time_compare("spam", b"eggs"))
 
     def test_constant_time_compare_deprecated(self):
         msg = (

The fix on my side is trivial (ensure both arguments are the same type), but I'm not sure if this was intentional for the deprecation process. If it were intentional, I'm happy to close this as a wontfix. Otherwise, I'm also happy to send a PR that adds the force_bytes back in.

[Sarah Boyce]

This was mentioned in the ticket description #36546

constant_time_compare() does call force_bytes() on its arguments but this was a workaround for Python 2.7 (7e3cf3cfd27e53ced0a1fc65a02849f78a292d3d) and no tests in Django's test suite fail with those calls removed.

Is this something you spotted and made you concerned or has this broken something on a project?

[Sage Abdullah]

Is this something you spotted and made you concerned or has this broken something on a project?

I spotted this in Wagtail (ref: ​https://github.com/wagtail/wagtail/pull/13363) because apparently our use of the function involves passing mixed-type arguments to the function, which likely was unintentional.

However, I can see another issue with leaving out force_bytes: the str vs. str use of hmac.compare_digest() ​only works with ASCII characters. This means the following test now also errors:

tests/utils_tests/test_crypto.py

@@ -21 +21 @@
         self.assertFalse(constant_time_compare(b"spam", b"eggs"))
         self.assertTrue(constant_time_compare("spam", "spam"))
         self.assertFalse(constant_time_compare("spam", "eggs"))
+        self.assertTrue(constant_time_compare(b"spam", "spam"))
+        self.assertFalse(constant_time_compare("spam", b"eggs"))
+        self.assertTrue(constant_time_compare("ありがとう", "ありがとう"))
+        self.assertFalse(constant_time_compare("ありがとう", "おはよう"))
 
     def test_constant_time_compare_deprecated(self):
         msg = (

If developers use the utility function to e.g. compare passwords, and they just pass plain strings to the function, it no longer works if the string contains non-ASCII characters.

[Tim Graham]

I hadn't noticed that the original patch changed the implementation of constant_time_compare() (removing force_bytes()). With this new information, that's certainly inappropriate.

I suppose there's a larger question of whether we keep the constant_time_compare() API since we now learned it provides genuinely different functionality that third-party code is relying on (comparing strings with non-ASCII characters and arguments of different type) than hmac.compare_digest(). In argument against keeping it, it forces third-parties to identify these areas and add their own casting rather than imposing a casting penalty where it's unneeded.

As far as I see, Django itself doesn't need to use constant_time_compare() internally since in always compares digests. Well, hmac.compare_digest(request.session.get(HASH_SESSION_KEY, ""), session_auth_hash) looks like the empty string needs to become a bytestring.

At this point in the release cycle, probably it's best to revert the deprecation to consider this more carefully. A later first step could be to replace internal usage of constant_time_compare() with compare_digest() (part of the original patch, but checked more carefully to make sure a str/bytes comparison can't happen).

[Jake Howard]

I agree with reverting #36546 and reviewing each case more closely. An alternative would be to explicitly state using force_bytes in the deprecation message, but that ends up quite verbose)

Given the complexities around using compare_digest with the correct types, I'd be in favour of keeping constant_time_compare around to hide those from users, even if we slowly port Django's internal uses to compare_digest separately.

[Sarah Boyce <42296566+sarahboyce@…>]

In d0e4dd5c:

Fixed #36572 -- Revert "Fixed #36546 -- Deprecated django.utils.crypto.constant_time_compare() in favor of hmac.compare_digest()."

This reverts commit 0246f478882c26bc1fe293224653074cd46a90d0.