-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
Fixed #36546 -- Deprecated django.utils.crypto.constant_time_compare()
in favor of hmac.compare_digest()
.
#19745
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
base: main
Are you sure you want to change the base?
Conversation
7b3bc05
to
6cac2e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to use hmac.compare_digest()
since the secrets
version is merely an alias of it. The earlier version of your patch that removed usage of constant_time_compare()
was correct to do so.
See also Deprecating a feature checklist.
0206d4f
to
925b937
Compare
925b937
to
5b29b66
Compare
django.utils.crypto.constant_time_compare()
in favor of hmac.compare_digest()
.
6f3340d
to
4b453a1
Compare
Hi @timgraham — thank you for the guidance and for taking the time. I’ve reviewed the “Deprecating a feature” checklist and applied the changes across the codebase as suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Have initial comments 👍
e9159a4
to
ddf191f
Compare
Can you either grant me permission to push or make sure this is a single squashed commit (no merge commit)? |
I’ve granted you permission to push now. Please let me know if you run into any issues! |
4f4b28f
to
dae3531
Compare
…) in favor of hmac.compare_digest(). Signed-off-by: SaJH <wogur981208@gmail.com>
dae3531
to
c3fcda4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@@ -570,6 +570,9 @@ Miscellaneous | |||
* The undocumented ``django.core.mail.forbid_multi_line_headers()`` and | |||
``django.core.mail.message.sanitize_address()`` functions are deprecated. | |||
|
|||
* The ``django.utils.crypto.constant_time_compare()`` function is deprecated. | |||
Use ``hmac.compare_digest()`` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ``hmac.compare_digest()`` instead. | |
Use :py:func:`hmac.compare_digest` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always wondered about the "py" prefix since it's used inconsistently in Django's docs. AI says, "Since the Python domain is the default in Sphinx, you can often omit the :py: prefix when referring to Python objects, for example, using :func: instead of :py:func:. However, explicitly including :py: ensures clarity and avoids potential ambiguity if other domains are also in use." I'd be in favor of omitting it since Django's documentation doesn't use other domains.
Btw, I would say, "The django.utils.crypto.constant_time_compare()
function is deprecated because it's merely an alias of hmac.compare_digest()
." (This gives a bit more context for readers and helps them understand that the functions behavior identically and there is no adaptation required.)
Trac ticket number
ticket-36546
Branch description
django.utils.crypto.constant_time_compare() is now just a duplicate wrapper around the standard library’s hmac.compare_digest(), so it is being deprecated in favor of using the standard function directly.
Checklist
main
branch.