diff --git a/apps/accounts/serializers.py b/apps/accounts/serializers.py index 591616b57f..4c68f6fa9a 100644 --- a/apps/accounts/serializers.py +++ b/apps/accounts/serializers.py @@ -1,16 +1,21 @@ from allauth.account.models import EmailAddress from base.utils import get_user_by_email +from django.conf import settings from django.contrib.auth import get_user_model from django.contrib.auth.models import User from participants.utils import ( has_participated_in_require_complete_profile_challenge, ) +from rest_auth.registration.serializers import RegisterSerializer from rest_auth.serializers import PasswordResetSerializer from rest_framework import serializers +from rest_framework.authtoken.serializers import AuthTokenSerializer from rest_framework.exceptions import ValidationError from .models import JwtToken, Profile +PASSWORD_MAX_LENGTH = getattr(settings, "PASSWORD_MAX_LENGTH", 128) + LOCKED_PROFILE_FIELDS = [ "first_name", "last_name", @@ -337,6 +342,29 @@ def create(self, validated_data): return email_address +class CustomRegisterSerializer(RegisterSerializer): + """Registration serializer with bounded password field lengths.""" + + password1 = serializers.CharField( + write_only=True, max_length=PASSWORD_MAX_LENGTH + ) + password2 = serializers.CharField( + write_only=True, max_length=PASSWORD_MAX_LENGTH + ) + + +class BoundedAuthTokenSerializer(AuthTokenSerializer): + """Login serializer with bounded password field length.""" + + password = serializers.CharField( + label="Password", + style={"input_type": "password"}, + write_only=True, + trim_whitespace=False, + max_length=PASSWORD_MAX_LENGTH, + ) + + class CustomPasswordResetSerializer(PasswordResetSerializer): """ Send password reset emails only for eligible accounts while always diff --git a/apps/accounts/throttles.py b/apps/accounts/throttles.py index 5842fb2739..c2fafdc420 100644 --- a/apps/accounts/throttles.py +++ b/apps/accounts/throttles.py @@ -41,6 +41,23 @@ def get_cache_key(self, request, view): return self.cache_format % {"scope": self.scope, "ident": email} +class RegistrationIPThrottle(_ThrottlingCacheMixin, SimpleRateThrottle): + """ + Limit registration requests to 10/hour per client IP address. + + Requires REST_FRAMEWORK["NUM_PROXIES"] to be set when deployed behind + trusted reverse proxies so client IPs are derived correctly. + """ + + scope = "registration_ip" + + def get_cache_key(self, request, view): + return self.cache_format % { + "scope": self.scope, + "ident": self.get_ident(request), + } + + class PasswordResetIPThrottle(_ThrottlingCacheMixin, SimpleRateThrottle): """ Limit password reset requests to 10/hour per client IP address. diff --git a/apps/accounts/validators.py b/apps/accounts/validators.py new file mode 100644 index 0000000000..52a92e500d --- /dev/null +++ b/apps/accounts/validators.py @@ -0,0 +1,31 @@ +from django.conf import settings +from django.core.exceptions import ValidationError +from django.utils.translation import gettext as _ + + +class MaximumLengthValidator: + """ + Reject passwords longer than PASSWORD_MAX_LENGTH to prevent resource + exhaustion from oversized password payloads. + """ + + def __init__(self, max_length=None): + self.max_length = max_length or getattr( + settings, "PASSWORD_MAX_LENGTH", 128 + ) + + def validate(self, password, user=None): + if len(password) > self.max_length: + raise ValidationError( + _( + "This password is longer than the maximum of " + "%(max_length)d characters." + ), + code="password_too_long", + params={"max_length": self.max_length}, + ) + + def get_help_text(self): + return _( + "Your password must be at most %(max_length)d characters." + ) % {"max_length": self.max_length} diff --git a/apps/accounts/views.py b/apps/accounts/views.py index 0cd63c26a7..28a7e3783d 100644 --- a/apps/accounts/views.py +++ b/apps/accounts/views.py @@ -15,7 +15,10 @@ throttle_classes, ) from rest_framework.response import Response +from rest_framework.status import HTTP_400_BAD_REQUEST from rest_framework.throttling import UserRateThrottle +from rest_framework_expiring_authtoken.models import ExpiringToken +from rest_framework_expiring_authtoken.views import ObtainExpiringAuthToken from rest_framework_simplejwt.authentication import JWTAuthentication from rest_framework_simplejwt.exceptions import TokenError from rest_framework_simplejwt.token_blacklist.models import OutstandingToken @@ -24,10 +27,15 @@ from .authentication import ExpiringTokenAuthentication from .models import JwtToken from .permissions import HasVerifiedEmail -from .serializers import JwtTokenSerializer, UpdateEmailSerializer +from .serializers import ( + BoundedAuthTokenSerializer, + JwtTokenSerializer, + UpdateEmailSerializer, +) from .throttles import ( PasswordResetEmailThrottle, PasswordResetIPThrottle, + RegistrationIPThrottle, ResendEmailThrottle, ) @@ -40,6 +48,31 @@ ) +class SafeObtainExpiringAuthToken(ObtainExpiringAuthToken): + """Login view with bounded password field length.""" + + def post(self, request): + serializer = BoundedAuthTokenSerializer(data=request.data) + + if serializer.is_valid(): + token, _ = ExpiringToken.objects.get_or_create( + user=serializer.validated_data["user"] + ) + + if token.expired(): + token.delete() + token = ExpiringToken.objects.create( + user=serializer.validated_data["user"] + ) + + return Response({"token": token.key}) + + return Response(serializer.errors, status=HTTP_400_BAD_REQUEST) + + +safe_obtain_expiring_auth_token = SafeObtainExpiringAuthToken.as_view() + + class SafePasswordResetView(PasswordResetView): """Password reset view that returns a generic response to prevent email enumeration.""" @@ -65,6 +98,8 @@ class SafeRegisterView(RegisterView): slip past the check. This view acts as a safety net for that case. """ + throttle_classes = (RegistrationIPThrottle,) + def create(self, request, *args, **kwargs): try: return super().create(request, *args, **kwargs) diff --git a/apps/challenges/serializers.py b/apps/challenges/serializers.py index 90fbf3bcba..d20cd61d92 100644 --- a/apps/challenges/serializers.py +++ b/apps/challenges/serializers.py @@ -5,6 +5,7 @@ DEFAULT_WORKER_PYTHON_VERSION, SUPPORTED_WORKER_PYTHON_VERSIONS, ) +from django.conf import settings from django.contrib.auth.password_validation import validate_password from django.core.exceptions import ValidationError as DjangoValidationError from hosts.serializers import ChallengeHostTeamSerializer @@ -615,7 +616,10 @@ class ChallengeInvitationRegisterSerializer(serializers.Serializer): last_name = serializers.CharField( max_length=150, required=False, allow_blank=True ) - password = serializers.CharField(write_only=True) + password = serializers.CharField( + write_only=True, + max_length=getattr(settings, "PASSWORD_MAX_LENGTH", 128), + ) def validate(self, attrs): user = self.context.get("user") diff --git a/evalai/urls.py b/evalai/urls.py index 431e390455..42f993667f 100644 --- a/evalai/urls.py +++ b/evalai/urls.py @@ -14,7 +14,11 @@ 2. Add a URL to urlpatterns: url(r'^blog/', include('blog.urls')) """ -from accounts.views import SafePasswordResetView, SafeRegisterView +from accounts.views import ( + SafePasswordResetView, + SafeRegisterView, + safe_obtain_expiring_auth_token, +) from allauth.account.views import ConfirmEmailView from django.conf import settings from django.conf.urls import include, url @@ -26,7 +30,6 @@ SpectacularRedocView, SpectacularSwaggerView, ) -from rest_framework_expiring_authtoken.views import obtain_expiring_auth_token from web import views handler404 = "web.views.page_not_found" @@ -43,7 +46,7 @@ url(r"^api/admin/", admin.site.urls), url( r"^api/auth/login", - obtain_expiring_auth_token, + safe_obtain_expiring_auth_token, name="obtain_expiring_auth_token", ), url( diff --git a/frontend/src/views/web/auth/login.html b/frontend/src/views/web/auth/login.html index c800bbeee9..ef622bfdcf 100644 --- a/frontend/src/views/web/auth/login.html +++ b/frontend/src/views/web/auth/login.html @@ -20,7 +20,7 @@
- + diff --git a/frontend/src/views/web/auth/reset-password-confirm.html b/frontend/src/views/web/auth/reset-password-confirm.html index 176036e920..8119aa43c2 100644 --- a/frontend/src/views/web/auth/reset-password-confirm.html +++ b/frontend/src/views/web/auth/reset-password-confirm.html @@ -19,7 +19,7 @@
- + @@ -31,7 +31,7 @@
- + diff --git a/frontend/src/views/web/auth/signup.html b/frontend/src/views/web/auth/signup.html index 4891400a93..48f711f135 100644 --- a/frontend/src/views/web/auth/signup.html +++ b/frontend/src/views/web/auth/signup.html @@ -30,7 +30,7 @@
- + @@ -41,12 +41,13 @@

Password is less than 8 characters.

+

Password must be at most 128 characters.

Password is required.

- + @@ -55,6 +56,7 @@

Passwords do not match

Password is less than 8 characters.

+

Password must be at most 128 characters.

Password confirmation is required.

diff --git a/frontend/src/views/web/challenge-invite.html b/frontend/src/views/web/challenge-invite.html index 40514b0241..fa9fb2d2dd 100644 --- a/frontend/src/views/web/challenge-invite.html +++ b/frontend/src/views/web/challenge-invite.html @@ -70,7 +70,7 @@
+ ng-model="challenge_invitation.password" ng-minlength="8" ng-maxlength="128" required>
@@ -88,13 +89,14 @@
+ ng-model="challenge_invitation.confirm_password" ng-minlength="8" ng-maxlength="128" compare-to="challenge_invitation.password" required>

Passwords do not match

Password is less than 8 characters.

+

Password must be at most 128 characters.

Password confirmation is required.

diff --git a/frontend/src/views/web/change-password.html b/frontend/src/views/web/change-password.html index b9bf3c75b2..917e750f2d 100644 --- a/frontend/src/views/web/change-password.html +++ b/frontend/src/views/web/change-password.html @@ -7,7 +7,7 @@
Change Password
- + @@ -19,7 +19,7 @@
- + @@ -32,7 +32,7 @@
- + diff --git a/settings/common.py b/settings/common.py index 62ea1987b3..77d7593e64 100755 --- a/settings/common.py +++ b/settings/common.py @@ -121,6 +121,8 @@ # Password validation # https://docs.djangoproject.com/en/1.10/ref/settings/#auth-password-validators +PASSWORD_MAX_LENGTH = 128 + AUTH_PASSWORD_VALIDATORS = [ { "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator" # noqa @@ -128,6 +130,10 @@ { "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator" # noqa }, + { + "NAME": "accounts.validators.MaximumLengthValidator", + "OPTIONS": {"max_length": PASSWORD_MAX_LENGTH}, + }, { "NAME": "django.contrib.auth.password_validation.CommonPasswordValidator" # noqa }, @@ -187,6 +193,7 @@ "resend_email": "3/hour", "password_reset_email": "3/hour", "password_reset_ip": "10/hour", + "registration_ip": "10/hour", }, "DEFAULT_RENDERER_CLASSES": ("rest_framework.renderers.JSONRenderer",), "DEFAULT_SCHEMA_CLASS": "drf_spectacular.openapi.AutoSchema", @@ -348,6 +355,10 @@ "PASSWORD_RESET_SERIALIZER": "accounts.serializers.CustomPasswordResetSerializer", } +REST_AUTH_REGISTER_SERIALIZERS = { + "REGISTER_SERIALIZER": "accounts.serializers.CustomRegisterSerializer", +} + # Default email for sending emails.. # Format "Display Name
" so inboxes show "EvalAI Team" as sender. DEFAULT_FROM_EMAIL = os.environ.get( diff --git a/settings/prod.py b/settings/prod.py index a01b391c23..bffd661816 100755 --- a/settings/prod.py +++ b/settings/prod.py @@ -91,6 +91,9 @@ def _sentry_before_send(event, hint): # Hide API Docs on production environment REST_FRAMEWORK_DOCS = {"HIDE_DOCS": True} +# One trusted reverse proxy (nginx) sets X-Forwarded-For in production. +REST_FRAMEWORK["NUM_PROXIES"] = 1 # noqa: F405 + # Port number for the python-memcached cache backend. CACHES["default"]["LOCATION"] = os.environ.get( # noqa: ignore=F405 "MEMCACHED_LOCATION" diff --git a/tests/unit/accounts/test_serializers.py b/tests/unit/accounts/test_serializers.py index 009a6835e7..055cc34927 100644 --- a/tests/unit/accounts/test_serializers.py +++ b/tests/unit/accounts/test_serializers.py @@ -3,11 +3,14 @@ from accounts.models import Profile from accounts.serializers import ( + BoundedAuthTokenSerializer, CustomPasswordResetSerializer, + CustomRegisterSerializer, ProfileSerializer, ) from allauth.account.models import EmailAddress from challenges.models import Challenge +from django.conf import settings from django.contrib.auth import get_user_model from django.test import TestCase from django.utils import timezone @@ -402,3 +405,48 @@ def test_read_creates_no_profile_when_missing(self): Profile.objects.filter(user=self.user).delete() serializer = ProfileSerializer(self.user) self.assertFalse(serializer.data["is_profile_complete"]) + + +class TestBoundedAuthTokenSerializer(TestCase): + def setUp(self): + self.user_model = get_user_model() + self.user = self.user_model.objects.create_user( + username="bounded_login", + email="bounded@example.com", + password=" spacedpass ", + ) + + def test_rejects_password_over_max_length(self): + serializer = BoundedAuthTokenSerializer( + data={ + "username": "bounded_login", + "password": "a" * (settings.PASSWORD_MAX_LENGTH + 1), + } + ) + self.assertFalse(serializer.is_valid()) + self.assertIn("password", serializer.errors) + + def test_accepts_password_with_leading_and_trailing_whitespace(self): + serializer = BoundedAuthTokenSerializer( + data={ + "username": "bounded_login", + "password": " spacedpass ", + } + ) + self.assertTrue(serializer.is_valid(), serializer.errors) + self.assertEqual(serializer.validated_data["user"], self.user) + + +class TestCustomRegisterSerializer(TestCase): + def test_rejects_password_over_max_length(self): + long_password = "a" * (settings.PASSWORD_MAX_LENGTH + 1) + serializer = CustomRegisterSerializer( + data={ + "username": "registeruser", + "email": "register@example.com", + "password1": long_password, + "password2": long_password, + } + ) + self.assertFalse(serializer.is_valid()) + self.assertIn("password1", serializer.errors) diff --git a/tests/unit/accounts/test_throttles.py b/tests/unit/accounts/test_throttles.py index 6b8db4ff21..d8adc4fbc0 100644 --- a/tests/unit/accounts/test_throttles.py +++ b/tests/unit/accounts/test_throttles.py @@ -3,6 +3,7 @@ from accounts.throttles import ( PasswordResetEmailThrottle, PasswordResetIPThrottle, + RegistrationIPThrottle, ResendEmailThrottle, ) from django.test import TestCase @@ -65,3 +66,15 @@ def test_get_cache_key_uses_client_ip(self): self.assertIn("throttle_password_reset_ip", cache_key) self.assertIn("10.0.0.1", cache_key) + + +class TestRegistrationIPThrottle(TestCase): + def test_get_cache_key_uses_client_ip(self): + throttle = RegistrationIPThrottle() + request = MagicMock() + request.META = {"REMOTE_ADDR": "203.0.113.5"} + + cache_key = throttle.get_cache_key(request, MagicMock()) + + self.assertIn("throttle_registration_ip", cache_key) + self.assertIn("203.0.113.5", cache_key) diff --git a/tests/unit/accounts/test_validators.py b/tests/unit/accounts/test_validators.py new file mode 100644 index 0000000000..bdb130e85e --- /dev/null +++ b/tests/unit/accounts/test_validators.py @@ -0,0 +1,28 @@ +from accounts.validators import MaximumLengthValidator +from django.conf import settings +from django.core.exceptions import ValidationError +from django.test import SimpleTestCase + + +class MaximumLengthValidatorTest(SimpleTestCase): + def test_accepts_password_within_limit(self): + validator = MaximumLengthValidator( + max_length=settings.PASSWORD_MAX_LENGTH + ) + validator.validate("a" * settings.PASSWORD_MAX_LENGTH) + + def test_rejects_password_over_limit(self): + validator = MaximumLengthValidator( + max_length=settings.PASSWORD_MAX_LENGTH + ) + with self.assertRaises(ValidationError) as context: + validator.validate("a" * (settings.PASSWORD_MAX_LENGTH + 1)) + self.assertEqual(context.exception.code, "password_too_long") + + def test_default_max_length_uses_settings(self): + validator = MaximumLengthValidator() + self.assertEqual(validator.max_length, settings.PASSWORD_MAX_LENGTH) + + def test_get_help_text_includes_max_length(self): + validator = MaximumLengthValidator(max_length=128) + self.assertIn("128", validator.get_help_text()) diff --git a/tests/unit/accounts/test_views.py b/tests/unit/accounts/test_views.py index bedd0a3e60..e04c6c2eb9 100644 --- a/tests/unit/accounts/test_views.py +++ b/tests/unit/accounts/test_views.py @@ -4,12 +4,14 @@ from accounts.models import JwtToken from accounts.views import PASSWORD_RESET_GENERIC_MESSAGE from allauth.account.models import EmailAddress +from django.conf import settings from django.contrib.auth.models import User from django.core.cache import caches from django.db import IntegrityError from django.urls import reverse_lazy from rest_framework import status from rest_framework.test import APIClient, APITestCase +from rest_framework_expiring_authtoken.models import ExpiringToken from rest_framework_simplejwt.exceptions import TokenError from rest_framework_simplejwt.token_blacklist.models import OutstandingToken from rest_framework_simplejwt.tokens import RefreshToken @@ -324,6 +326,78 @@ def test_integrity_error_non_email_reraised(self): format="json", ) + @patch("accounts.adapter.dns.resolver.resolve") + def test_register_rejects_password_over_max_length(self, mock_resolve): + """Oversized passwords are rejected before hashing.""" + mock_resolve.return_value = [MagicMock()] + long_password = "a" * (settings.PASSWORD_MAX_LENGTH + 1) + response = self.client.post( + self.url, + { + "username": "longpassuser", + "email": "longpass@example.com", + "password1": long_password, + "password2": long_password, + }, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("password1", response.data) + + +class LoginPasswordLengthTest(APITestCase): + url = reverse_lazy("obtain_expiring_auth_token") + + def test_login_rejects_password_over_max_length(self): + long_password = "a" * (settings.PASSWORD_MAX_LENGTH + 1) + response = self.client.post( + self.url, + { + "username": "someuser", + "password": long_password, + }, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("password", response.data) + + +class SafeObtainExpiringAuthTokenTest(APITestCase): + url = reverse_lazy("obtain_expiring_auth_token") + + def setUp(self): + self.client = APIClient(enforce_csrf_checks=True) + self.user = User.objects.create_user( + username="tokenuser", + email="token@example.com", + password="validpass1", + ) + + def test_login_returns_token_for_valid_credentials(self): + response = self.client.post( + self.url, + {"username": "tokenuser", "password": "validpass1"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn("token", response.data) + self.assertTrue(ExpiringToken.objects.filter(user=self.user).exists()) + + @patch.object(ExpiringToken, "expired", return_value=True) + def test_login_refreshes_expired_token(self, mock_expired): + old_token = ExpiringToken.objects.create(user=self.user) + old_key = old_token.key + + response = self.client.post( + self.url, + {"username": "tokenuser", "password": "validpass1"}, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertNotEqual(response.data["token"], old_key) + mock_expired.assert_called() + class PasswordResetViewTest(APITestCase): def setUp(self): diff --git a/tests/unit/challenges/test_serializers.py b/tests/unit/challenges/test_serializers.py index 0b30b747ce..deebe9d2ae 100644 --- a/tests/unit/challenges/test_serializers.py +++ b/tests/unit/challenges/test_serializers.py @@ -746,6 +746,16 @@ def test_validate_rejects_password_similar_to_submitted_first_name(self): self.assertFalse(serializer.is_valid()) self.assertIn("password", serializer.errors) + def test_validate_rejects_password_over_max_length(self): + serializer = ChallengeInvitationRegisterSerializer( + data={ + "password": "a" * 129, + }, + context={"user": self.user}, + ) + self.assertFalse(serializer.is_valid()) + self.assertIn("password", serializer.errors) + @pytest.mark.django_db class AddSponsorsToChallengeTests(TestCase):