feat: improve error handling and lifecycle management for tabCapture - #669
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds email verification, password complexity checks, MFA enrollment and login verification, login throttling, JWT revocation support, restricted CORS, secure cookies, CSP headers, and configurable admin routing. ChangesAuthentication Security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthViewSet
participant CustomTokenObtainSerializer
participant User
Client->>AuthViewSet: register with email and password
AuthViewSet->>User: create unverified account
AuthViewSet-->>Client: send verification message
Client->>AuthViewSet: verify email token
AuthViewSet->>User: mark email as verified
Client->>CustomTokenObtainSerializer: submit login credentials
CustomTokenObtainSerializer->>User: check verification and MFA state
CustomTokenObtainSerializer-->>Client: return MFA challenge or JWT tokens
Client->>AuthViewSet: submit MFA token and TOTP code
AuthViewSet->>User: validate MFA secret
AuthViewSet-->>Client: return refresh and access tokens
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/backend/settings.py (1)
195-201: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winJWT signing should not fall back to the hardcoded default, and refresh-token revocation is still disabled
backend/backend/settings.pycurrently signs JWTs withSECRET_KEYwhenJWT_SIGNING_KEYis unset, andSECRET_KEYitself defaults to a source-controlled fallback. Also,rest_framework_simplejwt.token_blacklistis installed but rotation/blacklisting isn’t enabled, so refresh tokens won’t be revoked automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/backend/settings.py` around lines 195 - 201, Update the SIMPLE_JWT configuration so SIGNING_KEY requires JWT_SIGNING_KEY and never falls back to SECRET_KEY or its source-controlled default. Enable refresh-token rotation and blacklist-after-rotation in SIMPLE_JWT, reusing the existing rest_framework_simplejwt.token_blacklist installation so rotated refresh tokens are revoked automatically.
🧹 Nitpick comments (6)
requirements.txt (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider bounding the
pyotpversion.
pyotp>=2.9.0has no upper bound, unlike the neighboringgunicorn>=20.1,<21.0pin. This package backs the MFA/TOTP flow, so an unexpected future major bump pulled in on a fresh install could silently change auth behavior. PyOTP's own docs recommend it: it is recommended that application developers pin the package version and manage it using pip-tools or similar.-pyotp>=2.9.0 +pyotp>=2.9.0,<3.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` at line 17, Update the pyotp dependency specification in requirements.txt to add an appropriate upper version bound, following the neighboring gunicorn constraint style and preserving the existing minimum version.backend/users/views.py (2)
51-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReusing
default_token_generator(password-reset generator) for email verification.
default_token_generatorhashesuser.pk + user.password + last_login + timestampunder a fixedkey_salttied to password resets. It works today since there's no separate password-reset flow, but it means a token isn't cryptographically scoped to "email verification" specifically. Consider subclassingPasswordResetTokenGeneratorwith a distinct hash/salt (a common Django pattern for account-activation tokens) to avoid any future cross-purpose token confusion if a password-reset feature is added later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 51 - 70, Update verify_email to validate tokens with a dedicated email-verification PasswordResetTokenGenerator subclass rather than the shared default_token_generator. Define a distinct key_salt for that generator and use the same dedicated generator when creating verification tokens, preserving the existing verification response behavior.
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent localhost fallback for
FRONTEND_BASE_URLin production.If the env var is unset in a deployed environment, verification emails link to
http://localhost:3000, silently breaking the entire verification flow instead of failing loudly. This directly relates to the PR's goal of improving env-var validation/defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 33 - 34, The verification-link construction in the relevant user email flow should not silently default FRONTEND_BASE_URL to localhost. Require a configured FRONTEND_BASE_URL in deployed environments and fail loudly when it is missing, while preserving an explicit development fallback only if the existing environment configuration supports it.backend/users/validators.py (1)
6-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly the first failed rule is reported; aggregate all violations instead.
If a password fails multiple checks (e.g. no uppercase and no digit), the caller only ever sees the first error, forcing repeated submit-and-fix cycles. Django's
ValidationErrorsupports a list of errors — collect and raise them together.♻️ Proposed refactor
def validate(self, password, user=None): - if not re.search(r'[A-Z]', password): - raise ValidationError( - _("The password must contain at least one uppercase letter."), - code='password_no_upper', - ) - if not re.search(r'[a-z]', password): - raise ValidationError( - _("The password must contain at least one lowercase letter."), - code='password_no_lower', - ) - if not re.search(r'\d', password): - raise ValidationError( - _("The password must contain at least one digit."), - code='password_no_digit', - ) - if not re.search(r'[!@#$%^&*(),.?":{}|<>\-_=+\[\];:\']', password): - raise ValidationError( - _("The password must contain at least one special character."), - code='password_no_special', - ) + errors = [] + if not re.search(r'[A-Z]', password): + errors.append(ValidationError( + _("The password must contain at least one uppercase letter."), + code='password_no_upper', + )) + if not re.search(r'[a-z]', password): + errors.append(ValidationError( + _("The password must contain at least one lowercase letter."), + code='password_no_lower', + )) + if not re.search(r'\d', password): + errors.append(ValidationError( + _("The password must contain at least one digit."), + code='password_no_digit', + )) + if not re.search(r'[!@#$%^&*(),.?":{}|<>\-_=+\[\];:\']', password): + errors.append(ValidationError( + _("The password must contain at least one special character."), + code='password_no_special', + )) + if errors: + raise ValidationError(errors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/validators.py` around lines 6 - 26, Update validate to collect every failed password rule in a list rather than raising immediately for each check, then raise one Django ValidationError containing all collected errors after all checks run. Preserve each existing message and error code, and leave valid passwords without raising.backend/users/serializers.py (2)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original exception (
raise ... from exc).Ruff (B904) flags this: re-raising inside an
exceptblock withoutfrom exc/from Nonediscards the original traceback context, making production debugging of password-validation failures harder.🔧 Proposed fix
- except DjangoValidationError as exc: - raise serializers.ValidationError({'password': list(exc.messages)}) + except DjangoValidationError as exc: + raise serializers.ValidationError({'password': list(exc.messages)}) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/serializers.py` around lines 28 - 35, Update the exception handling in the serializer’s validate method to chain the DjangoValidationError when raising serializers.ValidationError, using the caught exc as the explicit cause while preserving the existing password error payload.Source: Linters/SAST tools
31-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
UserAttributeSimilarityValidatoronly seesfirst_name/last_nameare silently skipped.
attrsalready containsfirst_name/last_name, but the temporaryUser(email=...)passed tovalidate_passworddoesn't set them, soUserAttributeSimilarityValidatorcan't reject passwords similar to the user's actual name — weakening the complexity requirement this PR is adding.🔧 Proposed fix
- user = User(email=attrs.get('email')) + user = User( + email=attrs.get('email'), + first_name=attrs.get('first_name', ''), + last_name=attrs.get('last_name', ''), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/serializers.py` around lines 31 - 33, Update the temporary User instance in the password-validation flow to populate first_name and last_name from attrs alongside email before calling validate_password. Keep the existing validation and error handling unchanged so UserAttributeSimilarityValidator evaluates all available user attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db.sqlite3`:
- Line 272: Remove the tracked backend/db.sqlite3 database from source control
and purge it from repository history. Revoke and rotate all exposed OAuth
tokens, password hashes, SMTP credentials, and IMAP credentials, then replace
the committed snapshot workflow with migrations and sanitized fixtures that
contain no real PII or credential-bearing values.
In `@backend/tenants/security.py`:
- Line 61: Update the CSP assignment in the security middleware to remove
script-src 'unsafe-inline'; use nonces or hashes for any required inline
scripts, or move them to external files while preserving legitimate script
execution. Also add object-src 'none' and base-uri 'self' to the policy.
In `@backend/users/jwt.py`:
- Around line 18-33: Update the MFA token flow in the JWT token-generation
method and the corresponding decode logic in users/views.py to use the
configured JWT signing key, such as JWT_SIGNING_KEY, instead of
settings.SECRET_KEY. Ensure both encoding and decoding use the same key
consistently, or introduce and apply a dedicated MFA key in both paths.
In
`@backend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.py`:
- Around line 13-17: Update the migration containing the is_email_verified
AddField to backfill existing users as verified before enforcing the new JWT
behavior. Add a data-migration step targeting the User model that sets
is_email_verified=True for all pre-existing users, while retaining the field’s
default=False for newly created users.
In `@backend/users/models.py`:
- Around line 36-38: The mfa_secret field in the user model stores TOTP secrets
in plaintext; replace it with the project’s established encrypted-field
mechanism or add Fernet encryption/decryption around persistence while
preserving the existing plaintext value returned to verify_mfa and enable_mfa
for pyotp.TOTP. Include the required key/configuration and migration so existing
data and field access remain compatible.
In `@backend/users/throttling.py`:
- Around line 13-14: Update the request-data handling around the visible request
throttling logic to catch DRF ParseError when evaluating request.data; treat
malformed input as unavailable data so the throttle flow continues and returns
its normal throttled 429 response instead of propagating the exception.
- Around line 6-24: Update get_cache_key so the existing throttle always uses
the client identifier (typically IP) without appending request.data email,
ensuring all login attempts from one IP share a bucket. If per-account
protection is required, add a separate LoginPerEmailThrottle with its own scope
and register both throttles in throttle_classes.
In `@backend/users/views.py`:
- Around line 72-88: Protect the TOTP secret throughout the MFA flow by
encrypting it before assigning `user.mfa_secret` in `enable_mfa`, then
decrypting it only when constructing `pyotp.TOTP` in `confirm_mfa` and
`verify_mfa`. Use a dedicated Fernet key from configuration and a shared
encryption/decryption helper, while preserving the existing provisioning URI and
verification behavior.
- Around line 26-43: Update the registration flow around the persisted user and
send_mail call so email-delivery failures cannot turn a successful account
creation into an unhandled 500. Add appropriate error handling around send_mail,
preserving the saved user response while recording the failure through the
project’s established logging or notification mechanism; do not change token or
verification-link generation.
- Around line 58-64: Update the exception tuple in the uid decoding and
User.objects.get lookup to also catch DjangoValidationError, ensuring malformed
UUID primary keys set user to None instead of propagating an error.
---
Outside diff comments:
In `@backend/backend/settings.py`:
- Around line 195-201: Update the SIMPLE_JWT configuration so SIGNING_KEY
requires JWT_SIGNING_KEY and never falls back to SECRET_KEY or its
source-controlled default. Enable refresh-token rotation and
blacklist-after-rotation in SIMPLE_JWT, reusing the existing
rest_framework_simplejwt.token_blacklist installation so rotated refresh tokens
are revoked automatically.
---
Nitpick comments:
In `@backend/users/serializers.py`:
- Around line 28-35: Update the exception handling in the serializer’s validate
method to chain the DjangoValidationError when raising
serializers.ValidationError, using the caught exc as the explicit cause while
preserving the existing password error payload.
- Around line 31-33: Update the temporary User instance in the
password-validation flow to populate first_name and last_name from attrs
alongside email before calling validate_password. Keep the existing validation
and error handling unchanged so UserAttributeSimilarityValidator evaluates all
available user attributes.
In `@backend/users/validators.py`:
- Around line 6-26: Update validate to collect every failed password rule in a
list rather than raising immediately for each check, then raise one Django
ValidationError containing all collected errors after all checks run. Preserve
each existing message and error code, and leave valid passwords without raising.
In `@backend/users/views.py`:
- Around line 51-70: Update verify_email to validate tokens with a dedicated
email-verification PasswordResetTokenGenerator subclass rather than the shared
default_token_generator. Define a distinct key_salt for that generator and use
the same dedicated generator when creating verification tokens, preserving the
existing verification response behavior.
- Around line 33-34: The verification-link construction in the relevant user
email flow should not silently default FRONTEND_BASE_URL to localhost. Require a
configured FRONTEND_BASE_URL in deployed environments and fail loudly when it is
missing, while preserving an explicit development fallback only if the existing
environment configuration supports it.
In `@requirements.txt`:
- Line 17: Update the pyotp dependency specification in requirements.txt to add
an appropriate upper version bound, following the neighboring gunicorn
constraint style and preserving the existing minimum version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c733a666-e063-4be6-a166-b1bcaf58de29
📒 Files selected for processing (13)
backend/backend/settings.pybackend/backend/urls.pybackend/db.sqlite3backend/tenants/security.pybackend/users/jwt.pybackend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.pybackend/users/models.pybackend/users/serializers.pybackend/users/tests.pybackend/users/throttling.pybackend/users/validators.pybackend/users/views.pyrequirements.txt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/backend/settings.py (1)
195-201: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winJWT signing should not fall back to the hardcoded default, and refresh-token revocation is still disabled
backend/backend/settings.pycurrently signs JWTs withSECRET_KEYwhenJWT_SIGNING_KEYis unset, andSECRET_KEYitself defaults to a source-controlled fallback. Also,rest_framework_simplejwt.token_blacklistis installed but rotation/blacklisting isn’t enabled, so refresh tokens won’t be revoked automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/backend/settings.py` around lines 195 - 201, Update the SIMPLE_JWT configuration so SIGNING_KEY requires JWT_SIGNING_KEY and never falls back to SECRET_KEY or its source-controlled default. Enable refresh-token rotation and blacklist-after-rotation in SIMPLE_JWT, reusing the existing rest_framework_simplejwt.token_blacklist installation so rotated refresh tokens are revoked automatically.
🧹 Nitpick comments (6)
requirements.txt (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider bounding the
pyotpversion.
pyotp>=2.9.0has no upper bound, unlike the neighboringgunicorn>=20.1,<21.0pin. This package backs the MFA/TOTP flow, so an unexpected future major bump pulled in on a fresh install could silently change auth behavior. PyOTP's own docs recommend it: it is recommended that application developers pin the package version and manage it using pip-tools or similar.-pyotp>=2.9.0 +pyotp>=2.9.0,<3.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` at line 17, Update the pyotp dependency specification in requirements.txt to add an appropriate upper version bound, following the neighboring gunicorn constraint style and preserving the existing minimum version.backend/users/views.py (2)
51-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReusing
default_token_generator(password-reset generator) for email verification.
default_token_generatorhashesuser.pk + user.password + last_login + timestampunder a fixedkey_salttied to password resets. It works today since there's no separate password-reset flow, but it means a token isn't cryptographically scoped to "email verification" specifically. Consider subclassingPasswordResetTokenGeneratorwith a distinct hash/salt (a common Django pattern for account-activation tokens) to avoid any future cross-purpose token confusion if a password-reset feature is added later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 51 - 70, Update verify_email to validate tokens with a dedicated email-verification PasswordResetTokenGenerator subclass rather than the shared default_token_generator. Define a distinct key_salt for that generator and use the same dedicated generator when creating verification tokens, preserving the existing verification response behavior.
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent localhost fallback for
FRONTEND_BASE_URLin production.If the env var is unset in a deployed environment, verification emails link to
http://localhost:3000, silently breaking the entire verification flow instead of failing loudly. This directly relates to the PR's goal of improving env-var validation/defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 33 - 34, The verification-link construction in the relevant user email flow should not silently default FRONTEND_BASE_URL to localhost. Require a configured FRONTEND_BASE_URL in deployed environments and fail loudly when it is missing, while preserving an explicit development fallback only if the existing environment configuration supports it.backend/users/validators.py (1)
6-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly the first failed rule is reported; aggregate all violations instead.
If a password fails multiple checks (e.g. no uppercase and no digit), the caller only ever sees the first error, forcing repeated submit-and-fix cycles. Django's
ValidationErrorsupports a list of errors — collect and raise them together.♻️ Proposed refactor
def validate(self, password, user=None): - if not re.search(r'[A-Z]', password): - raise ValidationError( - _("The password must contain at least one uppercase letter."), - code='password_no_upper', - ) - if not re.search(r'[a-z]', password): - raise ValidationError( - _("The password must contain at least one lowercase letter."), - code='password_no_lower', - ) - if not re.search(r'\d', password): - raise ValidationError( - _("The password must contain at least one digit."), - code='password_no_digit', - ) - if not re.search(r'[!@#$%^&*(),.?":{}|<>\-_=+\[\];:\']', password): - raise ValidationError( - _("The password must contain at least one special character."), - code='password_no_special', - ) + errors = [] + if not re.search(r'[A-Z]', password): + errors.append(ValidationError( + _("The password must contain at least one uppercase letter."), + code='password_no_upper', + )) + if not re.search(r'[a-z]', password): + errors.append(ValidationError( + _("The password must contain at least one lowercase letter."), + code='password_no_lower', + )) + if not re.search(r'\d', password): + errors.append(ValidationError( + _("The password must contain at least one digit."), + code='password_no_digit', + )) + if not re.search(r'[!@#$%^&*(),.?":{}|<>\-_=+\[\];:\']', password): + errors.append(ValidationError( + _("The password must contain at least one special character."), + code='password_no_special', + )) + if errors: + raise ValidationError(errors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/validators.py` around lines 6 - 26, Update validate to collect every failed password rule in a list rather than raising immediately for each check, then raise one Django ValidationError containing all collected errors after all checks run. Preserve each existing message and error code, and leave valid passwords without raising.backend/users/serializers.py (2)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original exception (
raise ... from exc).Ruff (B904) flags this: re-raising inside an
exceptblock withoutfrom exc/from Nonediscards the original traceback context, making production debugging of password-validation failures harder.🔧 Proposed fix
- except DjangoValidationError as exc: - raise serializers.ValidationError({'password': list(exc.messages)}) + except DjangoValidationError as exc: + raise serializers.ValidationError({'password': list(exc.messages)}) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/serializers.py` around lines 28 - 35, Update the exception handling in the serializer’s validate method to chain the DjangoValidationError when raising serializers.ValidationError, using the caught exc as the explicit cause while preserving the existing password error payload.Source: Linters/SAST tools
31-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
UserAttributeSimilarityValidatoronly seesfirst_name/last_nameare silently skipped.
attrsalready containsfirst_name/last_name, but the temporaryUser(email=...)passed tovalidate_passworddoesn't set them, soUserAttributeSimilarityValidatorcan't reject passwords similar to the user's actual name — weakening the complexity requirement this PR is adding.🔧 Proposed fix
- user = User(email=attrs.get('email')) + user = User( + email=attrs.get('email'), + first_name=attrs.get('first_name', ''), + last_name=attrs.get('last_name', ''), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/serializers.py` around lines 31 - 33, Update the temporary User instance in the password-validation flow to populate first_name and last_name from attrs alongside email before calling validate_password. Keep the existing validation and error handling unchanged so UserAttributeSimilarityValidator evaluates all available user attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db.sqlite3`:
- Line 272: Remove the tracked backend/db.sqlite3 database from source control
and purge it from repository history. Revoke and rotate all exposed OAuth
tokens, password hashes, SMTP credentials, and IMAP credentials, then replace
the committed snapshot workflow with migrations and sanitized fixtures that
contain no real PII or credential-bearing values.
In `@backend/tenants/security.py`:
- Line 61: Update the CSP assignment in the security middleware to remove
script-src 'unsafe-inline'; use nonces or hashes for any required inline
scripts, or move them to external files while preserving legitimate script
execution. Also add object-src 'none' and base-uri 'self' to the policy.
In `@backend/users/jwt.py`:
- Around line 18-33: Update the MFA token flow in the JWT token-generation
method and the corresponding decode logic in users/views.py to use the
configured JWT signing key, such as JWT_SIGNING_KEY, instead of
settings.SECRET_KEY. Ensure both encoding and decoding use the same key
consistently, or introduce and apply a dedicated MFA key in both paths.
In
`@backend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.py`:
- Around line 13-17: Update the migration containing the is_email_verified
AddField to backfill existing users as verified before enforcing the new JWT
behavior. Add a data-migration step targeting the User model that sets
is_email_verified=True for all pre-existing users, while retaining the field’s
default=False for newly created users.
In `@backend/users/models.py`:
- Around line 36-38: The mfa_secret field in the user model stores TOTP secrets
in plaintext; replace it with the project’s established encrypted-field
mechanism or add Fernet encryption/decryption around persistence while
preserving the existing plaintext value returned to verify_mfa and enable_mfa
for pyotp.TOTP. Include the required key/configuration and migration so existing
data and field access remain compatible.
In `@backend/users/throttling.py`:
- Around line 13-14: Update the request-data handling around the visible request
throttling logic to catch DRF ParseError when evaluating request.data; treat
malformed input as unavailable data so the throttle flow continues and returns
its normal throttled 429 response instead of propagating the exception.
- Around line 6-24: Update get_cache_key so the existing throttle always uses
the client identifier (typically IP) without appending request.data email,
ensuring all login attempts from one IP share a bucket. If per-account
protection is required, add a separate LoginPerEmailThrottle with its own scope
and register both throttles in throttle_classes.
In `@backend/users/views.py`:
- Around line 72-88: Protect the TOTP secret throughout the MFA flow by
encrypting it before assigning `user.mfa_secret` in `enable_mfa`, then
decrypting it only when constructing `pyotp.TOTP` in `confirm_mfa` and
`verify_mfa`. Use a dedicated Fernet key from configuration and a shared
encryption/decryption helper, while preserving the existing provisioning URI and
verification behavior.
- Around line 26-43: Update the registration flow around the persisted user and
send_mail call so email-delivery failures cannot turn a successful account
creation into an unhandled 500. Add appropriate error handling around send_mail,
preserving the saved user response while recording the failure through the
project’s established logging or notification mechanism; do not change token or
verification-link generation.
- Around line 58-64: Update the exception tuple in the uid decoding and
User.objects.get lookup to also catch DjangoValidationError, ensuring malformed
UUID primary keys set user to None instead of propagating an error.
---
Outside diff comments:
In `@backend/backend/settings.py`:
- Around line 195-201: Update the SIMPLE_JWT configuration so SIGNING_KEY
requires JWT_SIGNING_KEY and never falls back to SECRET_KEY or its
source-controlled default. Enable refresh-token rotation and
blacklist-after-rotation in SIMPLE_JWT, reusing the existing
rest_framework_simplejwt.token_blacklist installation so rotated refresh tokens
are revoked automatically.
---
Nitpick comments:
In `@backend/users/serializers.py`:
- Around line 28-35: Update the exception handling in the serializer’s validate
method to chain the DjangoValidationError when raising
serializers.ValidationError, using the caught exc as the explicit cause while
preserving the existing password error payload.
- Around line 31-33: Update the temporary User instance in the
password-validation flow to populate first_name and last_name from attrs
alongside email before calling validate_password. Keep the existing validation
and error handling unchanged so UserAttributeSimilarityValidator evaluates all
available user attributes.
In `@backend/users/validators.py`:
- Around line 6-26: Update validate to collect every failed password rule in a
list rather than raising immediately for each check, then raise one Django
ValidationError containing all collected errors after all checks run. Preserve
each existing message and error code, and leave valid passwords without raising.
In `@backend/users/views.py`:
- Around line 51-70: Update verify_email to validate tokens with a dedicated
email-verification PasswordResetTokenGenerator subclass rather than the shared
default_token_generator. Define a distinct key_salt for that generator and use
the same dedicated generator when creating verification tokens, preserving the
existing verification response behavior.
- Around line 33-34: The verification-link construction in the relevant user
email flow should not silently default FRONTEND_BASE_URL to localhost. Require a
configured FRONTEND_BASE_URL in deployed environments and fail loudly when it is
missing, while preserving an explicit development fallback only if the existing
environment configuration supports it.
In `@requirements.txt`:
- Line 17: Update the pyotp dependency specification in requirements.txt to add
an appropriate upper version bound, following the neighboring gunicorn
constraint style and preserving the existing minimum version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c733a666-e063-4be6-a166-b1bcaf58de29
📒 Files selected for processing (13)
backend/backend/settings.pybackend/backend/urls.pybackend/db.sqlite3backend/tenants/security.pybackend/users/jwt.pybackend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.pybackend/users/models.pybackend/users/serializers.pybackend/users/tests.pybackend/users/throttling.pybackend/users/validators.pybackend/users/views.pyrequirements.txt
🛑 Comments failed to post (10)
backend/db.sqlite3 (1)
272-272: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Remove this database from source control and rotate exposed credentials.
This snapshot contains OAuth bearer tokens (Line [272]), password hashes and user PII (Line [522]), plus credential-bearing fields such as
access_token,refresh_token,smtp_password, andimap_password(Line [480]). Even if these are test accounts, any still-valid tokens can enable account access or unauthorized mail activity.Remove
backend/db.sqlite3from the repository and its history, revoke/rotate all exposed OAuth and mail credentials, and use migrations with sanitized fixtures instead.Also applies to: 480-480, 522-522
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/db.sqlite3` at line 272, Remove the tracked backend/db.sqlite3 database from source control and purge it from repository history. Revoke and rotate all exposed OAuth tokens, password hashes, SMTP credentials, and IMAP credentials, then replace the committed snapshot workflow with migrations and sanitized fixtures that contain no real PII or credential-bearing values.backend/tenants/security.py (1)
61-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
script-src 'unsafe-inline'significantly weakens CSP's XSS protection.The PR's goal is security hardening, but
unsafe-inlineinscript-srceffectively disables CSP's primary defense against injected inline scripts. Whilestyle-src 'unsafe-inline'is often necessary for Django's admin and form media,script-srcshould avoid it. Consider using nonces or hashes for any legitimate inline scripts instead.Also consider adding
object-src 'none'andbase-uri 'self'to further harden the policy.🔒 Proposed CSP hardening
- response['Content-Security-Policy'] = "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; frame-ancestors 'none';" + response['Content-Security-Policy'] = "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; object-src 'none'; base-uri 'self'; frame-ancestors 'none';"If inline scripts are currently required, migrate to CSP nonces or move scripts to external files. Django's
django-csppackage can help manage nonces.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.response['Content-Security-Policy'] = "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; object-src 'none'; base-uri 'self'; frame-ancestors 'none';"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tenants/security.py` at line 61, Update the CSP assignment in the security middleware to remove script-src 'unsafe-inline'; use nonces or hashes for any required inline scripts, or move them to external files while preserving legitimate script execution. Also add object-src 'none' and base-uri 'self' to the policy.backend/users/jwt.py (1)
18-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n "SIMPLE_JWT|SIGNING_KEY|SECRET_KEY" backend/backend/settings.py -A3 -B1Repository: Kuldeeep18/LeadOrbit
Length of output: 742
🏁 Script executed:
#!/bin/bash sed -n '1,120p' backend/users/jwt.pyRepository: Kuldeeep18/LeadOrbit
Length of output: 1372
🏁 Script executed:
#!/bin/bash rg -n "mfa_token|mfa_pending|mfa_required" backendRepository: Kuldeeep18/LeadOrbit
Length of output: 1208
Use the configured JWT signing key for MFA tokens
backend/users/jwt.pyhard-codessettings.SECRET_KEY, whilebackend/backend/settings.pyalready exposesSIMPLE_JWT['SIGNING_KEY']viaJWT_SIGNING_KEY.backend/users/views.pyalso decodes the MFA token withsettings.SECRET_KEY, so this flow bypasses the configurable JWT secret entirely. Use a dedicated MFA key, or the shared JWT signing key, in both encode and decode paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/jwt.py` around lines 18 - 33, Update the MFA token flow in the JWT token-generation method and the corresponding decode logic in users/views.py to use the configured JWT signing key, such as JWT_SIGNING_KEY, instead of settings.SECRET_KEY. Ensure both encoding and decoding use the same key consistently, or introduce and apply a dedicated MFA key in both paths.backend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.py (1)
13-17: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill existing users when adding
is_email_verified. Without a data migration, pre-existing accounts stayfalseand JWT issuance will reject them, blocking logins after deploy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/migrations/0003_user_is_email_verified_user_mfa_enabled_and_more.py` around lines 13 - 17, Update the migration containing the is_email_verified AddField to backfill existing users as verified before enforcing the new JWT behavior. Add a data-migration step targeting the User model that sets is_email_verified=True for all pre-existing users, while retaining the field’s default=False for newly created users.backend/users/models.py (1)
36-38: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
mfa_secretis stored in plaintext.The TOTP shared secret is persisted as a plain
CharFieldwith no encryption at rest. Per theverify_mfa/enable_mfacontext (views.py), this raw value is read directly intopyotp.TOTP(user.mfa_secret). A DB dump or read-only SQL injection would let an attacker generate valid TOTP codes indefinitely for any user, defeating the purpose of MFA. Consider an encrypted field (e.g.django-cryptography'sEncryptedCharField, or Fernet-encrypt/decrypt around get/set) so the secret is unreadable from a raw DB dump.🧰 Tools
🪛 ast-grep (0.44.1)
[info] 37-37: use help_text to document model columns
Context: models.CharField(max_length=32, blank=True, null=True)
Note: [CWE-710] Improper Adherence to Coding Standards.(model-help-text)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/models.py` around lines 36 - 38, The mfa_secret field in the user model stores TOTP secrets in plaintext; replace it with the project’s established encrypted-field mechanism or add Fernet encryption/decryption around persistence while preserving the existing plaintext value returned to verify_mfa and enable_mfa for pyotp.TOTP. Include the required key/configuration and migration so existing data and field access remain compatible.backend/users/throttling.py (2)
6-24: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Composite cache key allows brute-force bypass by rotating email addresses.
The cache key
f"{ident}_{username}"creates a separate rate-limit bucket for each unique (IP, email) combination. An attacker from a single IP performing credential stuffing (trying many email/password pairs) gets a fresh bucket for each email, effectively bypassing the throttle entirely. This defeats the PR's stated goal of "login throttling and brute-force protection."The throttle should enforce a per-IP limit regardless of the submitted email, and optionally a separate per-email limit to protect individual accounts.
🔒 Proposed fix: dual-key throttling
from rest_framework.throttling import SimpleRateThrottle -class LoginRateThrottle(SimpleRateThrottle): +class LoginRateThrottle(SimpleRateThrottle): scope = 'login' def get_cache_key(self, request, view): - if request.user and request.user.is_authenticated: - ident = request.user.pk - else: - ident = self.get_ident(request) - - username = None - if hasattr(request, 'data') and isinstance(request.data, dict): - username = request.data.get('email') - - if username: - return self.cache_format % { - 'scope': self.scope, - 'ident': f"{ident}_{username}" - } - return self.cache_format % { - 'scope': self.scope, - 'ident': ident - } + # Always throttle by IP to prevent credential stuffing + return self.cache_format % { + 'scope': self.scope, + 'ident': self.get_ident(request), + }If per-account protection is also needed, create a second throttle class (e.g.,
LoginPerEmailThrottle) with its own scope and add both tothrottle_classes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def get_cache_key(self, request, view): # Always throttle by IP to prevent credential stuffing return self.cache_format % { 'scope': self.scope, 'ident': self.get_ident(request), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/throttling.py` around lines 6 - 24, Update get_cache_key so the existing throttle always uses the client identifier (typically IP) without appending request.data email, ensuring all login attempts from one IP share a bucket. If per-account protection is required, add a separate LoginPerEmailThrottle with its own scope and register both throttles in throttle_classes.
13-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Accessing
request.datacan raiseParseErroron malformed input.
request.datatriggers DRF's parser, which raisesParseError(HTTP 400) for invalid JSON bodies. In a throttle class, this unhandled exception would surface as a 500 error instead of a throttled 429 response. Wrap the access in a try/except.🛡️ Proposed fix
- if hasattr(request, 'data') and isinstance(request.data, dict): - username = request.data.get('email') + try: + if hasattr(request, 'data') and isinstance(request.data, dict): + username = request.data.get('email') + except Exception: + username = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: if hasattr(request, 'data') and isinstance(request.data, dict): username = request.data.get('email') except Exception: username = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/throttling.py` around lines 13 - 14, Update the request-data handling around the visible request throttling logic to catch DRF ParseError when evaluating request.data; treat malformed input as unavailable data so the throttle flow continues and returns its normal throttled 429 response instead of propagating the exception.backend/users/views.py (3)
26-43: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unhandled, blocking
send_mailcall can 500 a successful registration.
user = serializer.save()already committed the user+org to the DB.send_mail(..., fail_silently=False)then runs synchronously on the request thread with no timeout and no error handling — if the mail backend is slow or errors, the client gets an unhandled 500 despite the account existing, and they can't re-register (email now taken) or know to check their inbox.🔧 Proposed fix
- send_mail( - 'Verify Your LeadOrbit Email', - f'Please verify your email by clicking the following link: {verification_link}', - 'noreply@leadorbit.com', - [user.email], - fail_silently=False, - ) + try: + send_mail( + 'Verify Your LeadOrbit Email', + f'Please verify your email by clicking the following link: {verification_link}', + 'noreply@leadorbit.com', + [user.email], + fail_silently=False, + ) + except Exception: + logger.exception('Failed to send verification email to %s', user.pk)Consider moving email delivery to a background task/queue longer-term so registration latency doesn't depend on the mail server.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: send_mail( 'Verify Your LeadOrbit Email', f'Please verify your email by clicking the following link: {verification_link}', 'noreply@leadorbit.com', [user.email], fail_silently=False, ) except Exception: logger.exception('Failed to send verification email to %s', user.pk)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 26 - 43, Update the registration flow around the persisted user and send_mail call so email-delivery failures cannot turn a successful account creation into an unhandled 500. Add appropriate error handling around send_mail, preserving the saved user response while recording the failure through the project’s established logging or notification mechanism; do not change token or verification-link generation.
58-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n "class User\(|class TenantModel|id = models\.|UUIDField|AutoField" backend/users/models.py backend/tenants/models.py -A3Repository: Kuldeeep18/LeadOrbit
Length of output: 1219
🏁 Script executed:
python3 - <<'PY' # Read-only probe: inspect the exact pk type and the lookup path around the user retrieval. from pathlib import Path for p in ["backend/tenants/models.py", "backend/users/models.py", "backend/users/views.py"]: print(f"\n## {p}") text = Path(p).read_text() for i, line in enumerate(text.splitlines(), 1): if any(k in line for k in ["UUIDField", "class User", "get(pk=uid)", "ValidationError", "DoesNotExist"]): print(f"{i}: {line}") PYRepository: Kuldeeep18/LeadOrbit
Length of output: 868
Catch
DjangoValidationErrorfor UUID PK lookups
Useruses a UUID primary key, so a malformed decodeduidcan raiseDjangoValidationErrorinUser.objects.get(pk=uid)beforeDoesNotExist. Add it to the except tuple to avoid a 500 on this endpoint.🔧 Proposed defensive fix
- except (TypeError, ValueError, OverflowError, User.DoesNotExist): + except (TypeError, ValueError, OverflowError, User.DoesNotExist, DjangoValidationError): user = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 58 - 64, Update the exception tuple in the uid decoding and User.objects.get lookup to also catch DjangoValidationError, ensuring malformed UUID primary keys set user to None instead of propagating an error.
72-88: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
MFA secrets are persisted in plaintext.
user.mfa_secret(a plainCharFieldpermodels.py) stores the raw TOTP secret frompyotp.random_base32().confirm_mfa(Lines 89-104 / Line 98) andverify_mfa(Line 121) later read it back in the clear too. A database compromise would expose every user's TOTP seed, letting an attacker fully bypass MFA for every enrolled account — undermining the very control this PR adds. Consider encrypting the secret at rest (e.g. Fernet with a dedicated key) beforesave(), decrypting only when constructingpyotp.TOTP(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/users/views.py` around lines 72 - 88, Protect the TOTP secret throughout the MFA flow by encrypting it before assigning `user.mfa_secret` in `enable_mfa`, then decrypting it only when constructing `pyotp.TOTP` in `confirm_mfa` and `verify_mfa`. Use a dedicated Fernet key from configuration and a shared encryption/decryption helper, while preserving the existing provisioning URI and verification behavior.
1 participant
Pull Request
🔗 Related Issue
Closes #643
📝 Summary of Changes
This PR introduces critical security hardening and safeguards to the backend:
Authentication Enhancements:
LoginRateThrottleclass.Application Safeguards:
CORS_ALLOW_ALL_ORIGINS.HttpOnly,Secure, andSameSiteattributes./admin/panel to a configurable path via theADMIN_PATHsetting.User Verification:
/verify-email/view endpoint.🏷️ Type of Change
🧪 Testing
The changes were verified using Django's test framework. Comprehensive test suites were added to
users/tests.pyto validate:Steps to test:
backenddirectory:cd backendpython manage.py test✅ Checklist
Summary by CodeRabbit