From d381901de4a84a5b9df8df7528b02a135b97e3f6 Mon Sep 17 00:00:00 2001 From: Mahdi Date: Thu, 7 Aug 2025 22:29:07 +0330 Subject: [PATCH 1/9] Fix #9250: Prevent token overwrite and improve security - Fix key collision issue that could overwrite existing tokens - Use force_insert=True only for new token instances - Replace os.urandom with secrets.token_hex for better security - Add comprehensive test suite to verify fix and backward compatibility - Ensure existing tokens can still be updated without breaking changes --- rest_framework/authtoken/models.py | 24 +++++++-- tests/test_authtoken.py | 82 ++++++++++++++++++++++++++++-- 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/rest_framework/authtoken/models.py b/rest_framework/authtoken/models.py index 6a17c24525..2e4040833e 100644 --- a/rest_framework/authtoken/models.py +++ b/rest_framework/authtoken/models.py @@ -1,5 +1,4 @@ -import binascii -import os +import secrets from django.conf import settings from django.db import models @@ -28,13 +27,32 @@ class Meta: verbose_name_plural = _("Tokens") def save(self, *args, **kwargs): + """ + Save the token instance. + + If no key is provided, generates a cryptographically secure key. + For existing tokens with cleared keys, regenerates the key. + For new tokens, ensures they are inserted as new (not updated). + """ if not self.key: self.key = self.generate_key() + # For new objects, force INSERT to prevent overwriting existing tokens + if self._state.adding: + kwargs['force_insert'] = True return super().save(*args, **kwargs) @classmethod def generate_key(cls): - return binascii.hexlify(os.urandom(20)).decode() + """ + Generate a cryptographically secure token key. + + Uses secrets.token_hex(20) which provides 40 hexadecimal characters + (160 bits of entropy) suitable for authentication tokens. + + Returns: + str: A 40-character hexadecimal string + """ + return secrets.token_hex(20) def __str__(self): return self.key diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 30e416d653..14f3414f64 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -1,10 +1,13 @@ import importlib +import secrets from io import StringIO +from unittest import mock import pytest from django.contrib.admin import site from django.contrib.auth.models import User from django.core.management import CommandError, call_command +from django.db import IntegrityError from django.test import TestCase, modify_settings from rest_framework.authtoken.admin import TokenAdmin @@ -19,8 +22,13 @@ class AuthTokenTests(TestCase): def setUp(self): self.site = site - self.user = User.objects.create_user(username='test_user') - self.token = Token.objects.create(key='test token', user=self.user) + # CORRECTED: Only create the user. Each test will now create its own + # token(s) to ensure proper test isolation. + self.user = User.objects.create_user( + username='test_user', + email='test@example.com', + password='password' + ) def test_authtoken_can_be_imported_when_not_included_in_installed_apps(self): import rest_framework.authtoken.models @@ -31,12 +39,16 @@ def test_authtoken_can_be_imported_when_not_included_in_installed_apps(self): importlib.reload(rest_framework.authtoken.models) def test_model_admin_displayed_fields(self): + # Create a token specifically for this test. + token = Token.objects.create(user=self.user) mock_request = object() - token_admin = TokenAdmin(self.token, self.site) + token_admin = TokenAdmin(token, self.site) assert token_admin.get_fields(mock_request) == ('user',) def test_token_string_representation(self): - assert str(self.token) == 'test token' + # Create a token with a known key specifically for this test. + token = Token.objects.create(key='test token', user=self.user) + assert str(token) == 'test token' def test_validate_raise_error_if_no_credentials_provided(self): with pytest.raises(ValidationError): @@ -48,6 +60,68 @@ def test_whitespace_in_password(self): self.user.save() assert AuthTokenSerializer(data=data).is_valid() + # --- Tests for Issue #9250 and secrets module refactor --- + + def test_token_string_representation_is_randomly_generated_key(self): + """ + Ensure the string representation of a token is its key when auto-generated. + """ + token = Token.objects.create(user=self.user) + self.assertEqual(str(token), token.key) + + def test_token_creation_collision_raises_integrity_error(self): + """ + Verify that creating a token with an existing key raises IntegrityError. + """ + user2 = User.objects.create_user('user2', 'user2@example.com', 'p') + existing_token = Token.objects.create(user=user2) + + # Try to create another token with the same key + with self.assertRaises(IntegrityError): + Token.objects.create(key=existing_token.key, user=self.user) + + def test_key_regeneration_on_save_is_not_a_breaking_change(self): + """ + Verify that when a token is created without a key, it generates one correctly. + This tests the backward compatibility scenario where existing code might + create tokens without explicitly setting a key. + """ + # Create a token without a key - it should generate one automatically + token = Token(user=self.user) + token.key = "" # Explicitly clear the key + token.save() + + # Verify the key was generated + self.assertEqual(len(token.key), 40) + self.assertEqual(token.user, self.user) + + # Verify it's saved in the database + token.refresh_from_db() + self.assertEqual(len(token.key), 40) + self.assertEqual(token.user, self.user) + + def test_saving_existing_token_without_changes_does_not_alter_key(self): + """ + Ensure that calling save() on an existing token without modifications + does not change its key. + """ + token = Token.objects.create(user=self.user) + original_key = token.key + + token.save() + self.assertEqual(token.key, original_key) + + def test_generate_key_uses_secrets_module(self): + """ + Verify that `generate_key` correctly calls `secrets.token_hex`. + """ + with mock.patch('rest_framework.authtoken.models.secrets.token_hex') as mock_token_hex: + mock_token_hex.return_value = 'a_mocked_key_of_proper_length_0123456789' + key = Token.generate_key() + + mock_token_hex.assert_called_once_with(20) + self.assertEqual(key, 'a_mocked_key_of_proper_length_0123456789') + class AuthTokenCommandTests(TestCase): From 06b04419691661de286fdeb3279eb272d7fc4387 Mon Sep 17 00:00:00 2001 From: Mahdi Date: Thu, 7 Aug 2025 22:42:52 +0330 Subject: [PATCH 2/9] Fix code style: remove trailing whitespace and unused imports --- rest_framework/authtoken/models.py | 6 +++--- tests/test_authtoken.py | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/rest_framework/authtoken/models.py b/rest_framework/authtoken/models.py index 2e4040833e..1465f048e0 100644 --- a/rest_framework/authtoken/models.py +++ b/rest_framework/authtoken/models.py @@ -29,7 +29,7 @@ class Meta: def save(self, *args, **kwargs): """ Save the token instance. - + If no key is provided, generates a cryptographically secure key. For existing tokens with cleared keys, regenerates the key. For new tokens, ensures they are inserted as new (not updated). @@ -45,10 +45,10 @@ def save(self, *args, **kwargs): def generate_key(cls): """ Generate a cryptographically secure token key. - + Uses secrets.token_hex(20) which provides 40 hexadecimal characters (160 bits of entropy) suitable for authentication tokens. - + Returns: str: A 40-character hexadecimal string """ diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 14f3414f64..f2c591fd57 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -1,5 +1,4 @@ import importlib -import secrets from io import StringIO from unittest import mock @@ -75,7 +74,7 @@ def test_token_creation_collision_raises_integrity_error(self): """ user2 = User.objects.create_user('user2', 'user2@example.com', 'p') existing_token = Token.objects.create(user=user2) - + # Try to create another token with the same key with self.assertRaises(IntegrityError): Token.objects.create(key=existing_token.key, user=self.user) @@ -90,11 +89,11 @@ def test_key_regeneration_on_save_is_not_a_breaking_change(self): token = Token(user=self.user) token.key = "" # Explicitly clear the key token.save() - + # Verify the key was generated self.assertEqual(len(token.key), 40) self.assertEqual(token.user, self.user) - + # Verify it's saved in the database token.refresh_from_db() self.assertEqual(len(token.key), 40) From 705fc2b465bdc96a81abf2cc4cde59e2b37825d4 Mon Sep 17 00:00:00 2001 From: Mahdi Date: Thu, 7 Aug 2025 23:43:51 +0330 Subject: [PATCH 3/9] Fix #9250: Prevent token overwrite with minimal changes - Add force_insert=True to Token.save() for new objects to prevent overwriting existing tokens - Revert generate_key method to original implementation (os.urandom + binascii) - Update tests to work with original setUp() approach - Remove verbose comments and unrelated changes per reviewer feedback --- rest_framework/authtoken/models.py | 14 ++------ tests/test_authtoken.py | 52 +++++++++++------------------- 2 files changed, 22 insertions(+), 44 deletions(-) diff --git a/rest_framework/authtoken/models.py b/rest_framework/authtoken/models.py index 1465f048e0..bf38bc6078 100644 --- a/rest_framework/authtoken/models.py +++ b/rest_framework/authtoken/models.py @@ -1,4 +1,5 @@ -import secrets +import binascii +import os from django.conf import settings from django.db import models @@ -43,16 +44,7 @@ def save(self, *args, **kwargs): @classmethod def generate_key(cls): - """ - Generate a cryptographically secure token key. - - Uses secrets.token_hex(20) which provides 40 hexadecimal characters - (160 bits of entropy) suitable for authentication tokens. - - Returns: - str: A 40-character hexadecimal string - """ - return secrets.token_hex(20) + return binascii.hexlify(os.urandom(20)).decode() def __str__(self): return self.key diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index f2c591fd57..90091e2094 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -21,13 +21,8 @@ class AuthTokenTests(TestCase): def setUp(self): self.site = site - # CORRECTED: Only create the user. Each test will now create its own - # token(s) to ensure proper test isolation. - self.user = User.objects.create_user( - username='test_user', - email='test@example.com', - password='password' - ) + self.user = User.objects.create_user(username='test_user') + self.token = Token.objects.create(key='test token', user=self.user) def test_authtoken_can_be_imported_when_not_included_in_installed_apps(self): import rest_framework.authtoken.models @@ -38,16 +33,12 @@ def test_authtoken_can_be_imported_when_not_included_in_installed_apps(self): importlib.reload(rest_framework.authtoken.models) def test_model_admin_displayed_fields(self): - # Create a token specifically for this test. - token = Token.objects.create(user=self.user) mock_request = object() - token_admin = TokenAdmin(token, self.site) + token_admin = TokenAdmin(self.token, self.site) assert token_admin.get_fields(mock_request) == ('user',) def test_token_string_representation(self): - # Create a token with a known key specifically for this test. - token = Token.objects.create(key='test token', user=self.user) - assert str(token) == 'test token' + assert str(self.token) == 'test token' def test_validate_raise_error_if_no_credentials_provided(self): with pytest.raises(ValidationError): @@ -59,14 +50,7 @@ def test_whitespace_in_password(self): self.user.save() assert AuthTokenSerializer(data=data).is_valid() - # --- Tests for Issue #9250 and secrets module refactor --- - def test_token_string_representation_is_randomly_generated_key(self): - """ - Ensure the string representation of a token is its key when auto-generated. - """ - token = Token.objects.create(user=self.user) - self.assertEqual(str(token), token.key) def test_token_creation_collision_raises_integrity_error(self): """ @@ -85,41 +69,43 @@ def test_key_regeneration_on_save_is_not_a_breaking_change(self): This tests the backward compatibility scenario where existing code might create tokens without explicitly setting a key. """ + # Create a new user for this test to avoid conflicts with setUp token + user2 = User.objects.create_user('test_user2', 'test2@example.com', 'password') + # Create a token without a key - it should generate one automatically - token = Token(user=self.user) + token = Token(user=user2) token.key = "" # Explicitly clear the key token.save() # Verify the key was generated self.assertEqual(len(token.key), 40) - self.assertEqual(token.user, self.user) + self.assertEqual(token.user, user2) # Verify it's saved in the database token.refresh_from_db() self.assertEqual(len(token.key), 40) - self.assertEqual(token.user, self.user) + self.assertEqual(token.user, user2) def test_saving_existing_token_without_changes_does_not_alter_key(self): """ Ensure that calling save() on an existing token without modifications does not change its key. """ - token = Token.objects.create(user=self.user) - original_key = token.key + original_key = self.token.key - token.save() - self.assertEqual(token.key, original_key) + self.token.save() + self.assertEqual(self.token.key, original_key) - def test_generate_key_uses_secrets_module(self): + def test_generate_key_uses_os_urandom(self): """ - Verify that `generate_key` correctly calls `secrets.token_hex`. + Verify that `generate_key` correctly calls `os.urandom`. """ - with mock.patch('rest_framework.authtoken.models.secrets.token_hex') as mock_token_hex: - mock_token_hex.return_value = 'a_mocked_key_of_proper_length_0123456789' + with mock.patch('rest_framework.authtoken.models.os.urandom') as mock_urandom: + mock_urandom.return_value = b'a_mocked_key_of_proper_length_0123456789' key = Token.generate_key() - mock_token_hex.assert_called_once_with(20) - self.assertEqual(key, 'a_mocked_key_of_proper_length_0123456789') + mock_urandom.assert_called_once_with(20) + self.assertEqual(key, '615f6d6f636b65645f6b65795f6f665f70726f7065725f6c656e6774685f30313233343536373839') class AuthTokenCommandTests(TestCase): From 15a21641b07a21aa9dba37bd3f40140289aee64b Mon Sep 17 00:00:00 2001 From: Mahdi Date: Thu, 7 Aug 2025 23:50:57 +0330 Subject: [PATCH 4/9] Fix flake8 violations: remove extra blank lines and trailing whitespace --- tests/test_authtoken.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 90091e2094..33ac3db70f 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -50,8 +50,6 @@ def test_whitespace_in_password(self): self.user.save() assert AuthTokenSerializer(data=data).is_valid() - - def test_token_creation_collision_raises_integrity_error(self): """ Verify that creating a token with an existing key raises IntegrityError. @@ -71,7 +69,7 @@ def test_key_regeneration_on_save_is_not_a_breaking_change(self): """ # Create a new user for this test to avoid conflicts with setUp token user2 = User.objects.create_user('test_user2', 'test2@example.com', 'password') - + # Create a token without a key - it should generate one automatically token = Token(user=user2) token.key = "" # Explicitly clear the key From 621968d3f8b8f8e430e23a328f433dbff08adfbc Mon Sep 17 00:00:00 2001 From: Mahdi Rahimi <31624047+mahdirahimi1999@users.noreply.github.com> Date: Thu, 7 Aug 2025 23:59:02 +0330 Subject: [PATCH 5/9] Update tests/test_authtoken.py Co-authored-by: Bruno Alla --- tests/test_authtoken.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 33ac3db70f..fb4689dda4 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -51,9 +51,6 @@ def test_whitespace_in_password(self): assert AuthTokenSerializer(data=data).is_valid() def test_token_creation_collision_raises_integrity_error(self): - """ - Verify that creating a token with an existing key raises IntegrityError. - """ user2 = User.objects.create_user('user2', 'user2@example.com', 'p') existing_token = Token.objects.create(user=user2) From a720d715321d9ad9c9ee64c3fb76c0e46d7193cd Mon Sep 17 00:00:00 2001 From: Mahdi Rahimi <31624047+mahdirahimi1999@users.noreply.github.com> Date: Thu, 7 Aug 2025 23:59:39 +0330 Subject: [PATCH 6/9] Update tests/test_authtoken.py Co-authored-by: Bruno Alla --- tests/test_authtoken.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index fb4689dda4..478777a9fb 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -82,10 +82,6 @@ def test_key_regeneration_on_save_is_not_a_breaking_change(self): self.assertEqual(token.user, user2) def test_saving_existing_token_without_changes_does_not_alter_key(self): - """ - Ensure that calling save() on an existing token without modifications - does not change its key. - """ original_key = self.token.key self.token.save() From bf3a782396b14f55daf4705ec59a6bb779354e1b Mon Sep 17 00:00:00 2001 From: Mahdi Rahimi <31624047+mahdirahimi1999@users.noreply.github.com> Date: Fri, 8 Aug 2025 00:00:35 +0330 Subject: [PATCH 7/9] Update tests/test_authtoken.py Co-authored-by: Bruno Alla --- tests/test_authtoken.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 478777a9fb..58b256c84e 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -58,12 +58,7 @@ def test_token_creation_collision_raises_integrity_error(self): with self.assertRaises(IntegrityError): Token.objects.create(key=existing_token.key, user=self.user) - def test_key_regeneration_on_save_is_not_a_breaking_change(self): - """ - Verify that when a token is created without a key, it generates one correctly. - This tests the backward compatibility scenario where existing code might - create tokens without explicitly setting a key. - """ + def test_key_regeneration_on_save_when_cleared(self): # Create a new user for this test to avoid conflicts with setUp token user2 = User.objects.create_user('test_user2', 'test2@example.com', 'password') From e515e23a8b020b245bc43b651b8c44801e684eff Mon Sep 17 00:00:00 2001 From: Mahdi Date: Fri, 8 Aug 2025 12:10:02 +0330 Subject: [PATCH 8/9] Fix token key regeneration behavior and add test --- rest_framework/authtoken/models.py | 1 - tests/test_authtoken.py | 27 +++++++++++---------------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/rest_framework/authtoken/models.py b/rest_framework/authtoken/models.py index bf38bc6078..5bb235a808 100644 --- a/rest_framework/authtoken/models.py +++ b/rest_framework/authtoken/models.py @@ -32,7 +32,6 @@ def save(self, *args, **kwargs): Save the token instance. If no key is provided, generates a cryptographically secure key. - For existing tokens with cleared keys, regenerates the key. For new tokens, ensures they are inserted as new (not updated). """ if not self.key: diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index 58b256c84e..aaf1eb587c 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -1,6 +1,5 @@ import importlib from io import StringIO -from unittest import mock import pytest from django.contrib.admin import site @@ -71,10 +70,17 @@ def test_key_regeneration_on_save_when_cleared(self): self.assertEqual(len(token.key), 40) self.assertEqual(token.user, user2) - # Verify it's saved in the database - token.refresh_from_db() - self.assertEqual(len(token.key), 40) - self.assertEqual(token.user, user2) + def test_clearing_key_on_existing_token_raises_integrity_error(self): + """Test that clearing the key on an existing token raises IntegrityError.""" + user = User.objects.create_user('test_user3', 'test3@example.com', 'password') + token = Token.objects.create(user=user) + token.key = "" + + # This should raise IntegrityError because: + # 1. We're trying to update a record with an empty primary key + # 2. The OneToOneField constraint would be violated + with self.assertRaises(Exception): # Could be IntegrityError or DatabaseError + token.save() def test_saving_existing_token_without_changes_does_not_alter_key(self): original_key = self.token.key @@ -82,17 +88,6 @@ def test_saving_existing_token_without_changes_does_not_alter_key(self): self.token.save() self.assertEqual(self.token.key, original_key) - def test_generate_key_uses_os_urandom(self): - """ - Verify that `generate_key` correctly calls `os.urandom`. - """ - with mock.patch('rest_framework.authtoken.models.os.urandom') as mock_urandom: - mock_urandom.return_value = b'a_mocked_key_of_proper_length_0123456789' - key = Token.generate_key() - - mock_urandom.assert_called_once_with(20) - self.assertEqual(key, '615f6d6f636b65645f6b65795f6f665f70726f7065725f6c656e6774685f30313233343536373839') - class AuthTokenCommandTests(TestCase): From ae6161a52ebf14ba679a770c7c2a590c903df2d9 Mon Sep 17 00:00:00 2001 From: Mahdi Rahimi <31624047+mahdirahimi1999@users.noreply.github.com> Date: Sat, 9 Aug 2025 16:26:21 +0330 Subject: [PATCH 9/9] Update tests/test_authtoken.py Co-authored-by: Bruno Alla --- tests/test_authtoken.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_authtoken.py b/tests/test_authtoken.py index aaf1eb587c..3cfcbb3944 100644 --- a/tests/test_authtoken.py +++ b/tests/test_authtoken.py @@ -57,7 +57,7 @@ def test_token_creation_collision_raises_integrity_error(self): with self.assertRaises(IntegrityError): Token.objects.create(key=existing_token.key, user=self.user) - def test_key_regeneration_on_save_when_cleared(self): + def test_key_generated_on_save_when_cleared(self): # Create a new user for this test to avoid conflicts with setUp token user2 = User.objects.create_user('test_user2', 'test2@example.com', 'password')