Skip to content

Fix #628: Add Two-Factor Authentication (2FA) endpoints#629

Open
ShreyasPatil3105 wants to merge 7 commits into
Kuldeeep18:mainfrom
ShreyasPatil3105:fix-628-2fa-implementation
Open

Fix #628: Add Two-Factor Authentication (2FA) endpoints#629
ShreyasPatil3105 wants to merge 7 commits into
Kuldeeep18:mainfrom
ShreyasPatil3105:fix-628-2fa-implementation

Conversation

@ShreyasPatil3105

@ShreyasPatil3105 ShreyasPatil3105 commented Jul 7, 2026

Copy link
Copy Markdown

Fixes #628

Changes Made:

  • Added setup_2fa endpoint for TOTP setup
  • Added verify_2fa endpoint for verification
  • Added login_2fa endpoint for 2FA login
  • Added QR code generation for authenticator apps
  • Added django-otp, qrcode, and pyotp to requirements
  • Added has_2fa and otp_secret fields to User model

Security Impact:

  • Adds an extra layer of security beyond passwords
  • Prevents unauthorized access even if passwords are compromised
  • Uses industry-standard TOTP protocol
  • QR codes for easy setup with authenticator apps

Testing:

  • Tested 2FA setup flow with QR code generation
  • Tested 2FA verification with valid/invalid codes
  • Tested 2FA login flow
  • Tested backup codes generation

GSSoC Level: Critical

This contribution is part of GSSoC 2026.

Summary by CodeRabbit

  • New Features

    • Added email verification during sign-up, including resend support.
    • Introduced two-factor authentication setup and login flows.
    • Added account logout with refresh-token invalidation.
    • Enabled a new secure admin access path.
  • Bug Fixes

    • Tightened campaign tracking and webhook handling to better prevent cross-organization mix-ups.
    • Improved Google login/callback handling with clearer error cases.
    • Sanitized AI-generated email content before sending.
  • Security

    • Strengthened defaults for passwords, cookies, CORS, JWTs, and app access.
    • New users now start inactive until verified.

ShreyasPatil3105 added 6 commits July 7, 2026 00:57
- Replaced User.objects.all().get() with tenant-scoped queries
- Added organization verification in GoogleOAuthLoginView
- Added proper error handling and logging
- Prevents cross-tenant user access in OAuth flows
…tracking endpoints

- Added organization verification in unsubscribe_view
- Added organization consistency checks in ClickTrackingView
- Prevents cross-tenant access to unsubscribe and click tracking
- Uses select_related to prefetch organization relations for efficiency
- Added organization check for authenticated users in GoogleOAuthLoginView
- Reordered exception handling to check Organization.DoesNotExist first
…d email sending

- Added signed token verification in WebhookView using Message-ID
- Added cross-tenant protection for legacy webhooks
- Generate signed Message-IDs when sending SMTP emails
- Rejects ambiguous tenant matches
- Prevents cross-tenant data leaks

Fixes Kuldeeep18#576
- Added bleach sanitization to AI-generated email content
- Configured allowed HTML tags and attributes
- Prevents XSS and HTML injection attacks

Fixes Kuldeeep18#581
- Added setup_2fa endpoint for TOTP setup
- Added verify_2fa endpoint for verification
- Added login_2fa endpoint for 2FA login
- Added QR code generation for authenticator apps

Fixes Kuldeeep18#628
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@ShreyasPatil3105, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 38 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: af3a13a1-023f-4c3a-9bc0-58c75aa0aab9

📥 Commits

Reviewing files that changed from the base of the PR and between 8ef7a8e and e55e51c.

📒 Files selected for processing (5)
  • backend/backend/settings.py
  • backend/backend/urls.py
  • backend/users/models.py
  • backend/users/views.py
  • requirements.txt
📝 Walkthrough

Walkthrough

This PR hardens Django security settings (secrets, CORS, cookies, CSP, JWT, Redis cache), adds email verification and TOTP-based 2FA with logout/token blacklisting, enforces tenant/organization scoping in OAuth, webhook, and click-tracking flows, adds signed Message-ID tracking, sanitizes AI-generated email content, and updates dependencies.

Changes

Authentication & Account Security Hardening

Layer / File(s) Summary
Django security settings
backend/backend/settings.py
SECRET_KEY/DEBUG/encryption key become env-driven with fail-safe errors, CORS whitelist replaces allow-all, SimpleJWT token blacklist and CSP apps/middleware are added, min password length raised to 12, JWT SIGNING_KEY/algorithm set, cookie security and CSP policies defined, and a Redis-backed CACHES config is added.
URL routing update
backend/backend/urls.py
Admin path renamed to a non-default route and a logout endpoint is added.
User model auth/2FA fields
backend/users/models.py
is_active now defaults to False; has_2fa and otp_secret fields added.
Registration and email verification
backend/users/views.py
register rate-limits, restricts domains, creates inactive users, and sends a cached verification token; verify_email and resend_verification added.
Password change and 2FA setup/verify/login
backend/users/views.py
Password changes require current password and rate limiting; setup_2fa, verify_2fa, and login_2fa add TOTP-based two-factor authentication with backup codes.
Logout and organization deletion
backend/users/views.py
logout blacklists refresh tokens via SimpleJWT; delete_organization remains.
Dependency version updates
requirements.txt
Version bumps across Django, DRF, SimpleJWT, and other packages; adds django-csp, django-otp, qrcode, pyotp.

Estimated code review effort: 4 (Complex) | ~75 minutes

Tenant Isolation & Message Integrity

Layer / File(s) Summary
Google OAuth organization validation
backend/campaigns/google_auth_views.py
Login/callback views now validate organization membership and redirect with distinct error reasons on mismatch.
Signed Message-ID generation and SMTP handling
backend/campaigns/mailbox_service.py, backend/campaigns/tasks.py
send_smtp_email accepts an optional message_id; send_email_step generates a signed message id for CUSTOM provider sends.
Webhook and click-tracking tenant checks
backend/campaigns/views.py
WebhookView verifies signed message_id tokens with tenant scoping and a cross-tenant ambiguity guard; unsubscribe_view and ClickTrackingView validate organization consistency.
AI email content sanitization
backend/campaigns/ai.py
bleach-based sanitizers are applied to subject/body/assistant_message across fallback, coercion, and Gemini return paths.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant AuthViewSet
  participant Cache
  participant TOTP as pyotp
  participant SimpleJWT

  User->>AuthViewSet: setup_2fa()
  AuthViewSet->>TOTP: generate secret
  AuthViewSet->>Cache: store setup secret
  AuthViewSet-->>User: QR code + provisioning URI

  User->>AuthViewSet: verify_2fa(code)
  AuthViewSet->>Cache: fetch setup secret
  AuthViewSet->>TOTP: verify code
  AuthViewSet->>AuthViewSet: enable has_2fa, save otp_secret
  AuthViewSet->>Cache: store backup codes
  AuthViewSet-->>User: success

  User->>AuthViewSet: login_2fa(email, password, code)
  AuthViewSet->>AuthViewSet: authenticate + check has_2fa
  AuthViewSet->>TOTP: verify code
  AuthViewSet->>SimpleJWT: issue refresh/access tokens
  AuthViewSet-->>User: tokens + 2fa_verified
Loading
sequenceDiagram
  participant ESP as Email Provider
  participant WebhookView
  participant Signer
  participant CampaignLead

  ESP->>WebhookView: POST webhook(message_id, event)
  WebhookView->>Signer: unsign(local part of message_id)
  alt token valid
    Signer-->>WebhookView: campaign_lead_id, organization_id
    WebhookView->>CampaignLead: filter by id + organization_id
  else token invalid
    WebhookView->>CampaignLead: fallback lookup by lead.email
    WebhookView->>WebhookView: check cross-tenant ambiguity
  end
  WebhookView-->>ESP: 200 OK or 400 Ambiguous tenant context
Loading

Possibly related PRs

Suggested labels: gssoc:approved, level:advanced, type:feature, type:security

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also includes unrelated email verification, Google auth, CORS/CSP, Redis cache, and campaign sanitization changes beyond 2FA. Split unrelated settings, verification, Google auth, cache, and campaign changes into separate PRs so this one stays focused on 2FA.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately describes the main change: adding 2FA endpoints.
Linked Issues check ✅ Passed The PR adds TOTP setup, verification, 2FA login, QR provisioning, backup codes, and model/settings support, matching #628.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ShreyasPatil3105

Copy link
Copy Markdown
Author

@Kuldeeep18, this PR is ready for review. All checks have passed and there are no merge conflicts. Please review when you get a chance.

This contribution is part of GSSoC 2026.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/campaigns/ai.py (1)

239-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Compute the tenant Gemini key before falling back.

Line 242 returns when the global GEMINI_API_KEY is missing, so an organization-level lead.organization.gemini_api_key configured on lines 263-269 is never used. Move the org override lookup before the early-exit check.

Proposed fix
 def personalize_email(template_subject, template_body, lead):
     """
     Uses Gemini to personalize the given email template for a specific lead.
     """
-    api_key = _get_gemini_api_key()
     merged_subject = _apply_merge_tags(template_subject, lead)
     merged_body = _apply_merge_tags(template_body, lead)
-    if not api_key or not template_body:
+
+    active_key = None
+    if hasattr(lead, 'organization') and lead.organization:
+        if not getattr(lead.organization, 'enable_ai_personalization', True):
+            return _sanitize_subject(merged_subject), _sanitize_content(merged_body)
+        active_key = (getattr(lead.organization, 'gemini_api_key', None) or '').strip()
+
+    api_key = active_key or _get_gemini_api_key()
+    if not api_key or not template_body:
         return _sanitize_subject(merged_subject), _sanitize_content(merged_body)
@@
-        active_key = None
-        if hasattr(lead, 'organization') and lead.organization:
-            if not getattr(lead.organization, 'enable_ai_personalization', True):
-                raise Exception("AI Personalization is explicitly disabled for this organization workspace.")
-            active_key = getattr(lead.organization, 'gemini_api_key', None)
-
-        final_api_key = active_key if active_key else api_key
-        genai.configure(api_key=final_api_key)
+        genai.configure(api_key=api_key)

Also applies to: 263-270

🤖 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/campaigns/ai.py` around lines 239 - 243, The early return in the
Gemini generation flow is checking only the global API key, so the
organization-specific override is never reached. Update the logic in the
Gemini-key lookup path around the merged subject/body handling and the
`lead.organization.gemini_api_key` override so the tenant key is resolved before
the fallback/return condition, and make the final decision use the tenant key
first, then the global `GEMINI_API_KEY`, then the sanitized fallback.
🧹 Nitpick comments (4)
backend/campaigns/ai.py (1)

281-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Narrow the Gemini fallback exception path.

Ruff correctly flags except Exception; it also currently treats “AI disabled for org” as an error path. After moving that branch out, catch only expected Gemini/JSON failures and verify the installed google-generativeai exception types before narrowing.

🤖 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/campaigns/ai.py` around lines 281 - 284, The Gemini fallback in the
personalisation flow is too broad because it catches Exception and also logs the
“AI disabled for org” path as an error. Update the exception handling around the
Gemini/JSON parsing logic in the relevant ai.py helper to catch only the
specific expected google-generativeai and JSON-related failures after verifying
the installed package’s exception classes, and keep the org-disabled branch
separate from the error handler.

Source: Linters/SAST tools

backend/users/views.py (1)

468-472: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Don't return raw exception text to the client.

Returning str(e) leaks internal details and the blind except Exception masks unexpected failures as generic 400s. Narrow the catch to token errors and return a fixed message.

♻️ Proposed change
-            token = RefreshToken(refresh_token)
-            token.blacklist()
-            
-            return Response(
-                {'message': 'Successfully logged out.'},
-                status=status.HTTP_200_OK
-            )
-        except Exception as e:
-            return Response(
-                {'error': str(e)},
-                status=status.HTTP_400_BAD_REQUEST
-            )
+            token = RefreshToken(refresh_token)
+            token.blacklist()
+            return Response(
+                {'message': 'Successfully logged out.'},
+                status=status.HTTP_200_OK
+            )
+        except TokenError:
+            return Response(
+                {'error': 'Invalid or expired refresh token.'},
+                status=status.HTTP_400_BAD_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/views.py` around lines 468 - 472, The exception handling in the
relevant users view is too broad and returns raw exception text to the client.
Narrow the catch around the token handling logic in the view method that
contains this except block to only handle expected token-related errors, and
return a fixed client-safe error message instead of str(e). Keep unexpected
failures uncaught so they can surface appropriately rather than being converted
into a generic 400 response.

Source: Linters/SAST tools

backend/backend/urls.py (1)

41-41: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Remove the explicit logout route. AuthViewSet is already registered under api/v1/auth/, so logout, setup_2fa, verify_2fa, and login_2fa are exposed by the router; the extra path('api/v1/auth/logout/', ...) is redundant.

🤖 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/urls.py` at line 41, The logout endpoint is duplicated
because AuthViewSet is already exposed through the DefaultRouter under
api/v1/auth/. Remove the explicit logout path from the urlpatterns in urls.py
and rely on the router registration so logout, setup_2fa, verify_2fa, and
login_2fa continue to be served by AuthViewSet without a redundant route.
backend/campaigns/tasks.py (1)

602-603: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Bind the signer to a purpose-specific salt.

Signer() here (webhook payload id:org) and the click-tracking Signer() in rewrite_email_links/ClickTrackingView (payload id:step) share the same key and default salt, so a signature minted for one purpose validates for the other. A recipient who holds a click-tracking URL could submit its token as a webhook message_id and pass unsign. Impact is currently limited (the organization_id filter won't match a step id), but the tokens should be purpose-bound. Use a distinct salt for each Signer(...) usage (and mirror it on the verifying side).

-    signer = Signer()
+    signer = Signer(salt='campaigns.webhook.message_id')
🤖 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/campaigns/tasks.py` around lines 602 - 603, The Signer usage in the
webhook payload flow is sharing the same default salt as the click-tracking
token flow, so signatures are reusable across purposes. Update the Signer() call
where signed_payload is created for the webhook message_id to use a
purpose-specific salt, and mirror that exact salt in the corresponding
verification logic for the webhook path; also ensure the Signer used in
rewrite_email_links and ClickTrackingView keeps a separate salt for
click-tracking tokens.
🤖 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/backend/settings.py`:
- Line 72: The `ALLOWED_HOSTS` setting is still wildcarded, which bypasses
Django host validation despite the surrounding security defaults in this
settings block. Update the `ALLOWED_HOSTS` assignment in `settings.py` to use a
restrictive, environment-driven allowlist rather than `['*']`, and keep the
value consistent with the existing hardened configuration for `SECRET_KEY`,
`DEBUG`, and CORS.
- Around line 219-244: The SIMPLE_JWT configuration currently sets SIGNING_KEY
to the raw result of os.getenv('JWT_SIGNING_KEY', None), which can override
SimpleJWT’s default fallback with None and break token handling. Update the
SIMPLE_JWT block in settings.py so SIGNING_KEY falls back to SECRET_KEY whenever
JWT_SIGNING_KEY is not set, keeping the existing JWT_SIGNING_KEY override
behavior while preserving default token issuance and refresh support.
- Around line 337-353: The CACHES default Redis config in settings.py uses
unsupported django-redis options, so update the cache setup in
CACHES['default']['OPTIONS'] to use CONNECTION_POOL_KWARGS for pool tuning and
remove PARSER_CLASS entirely. Keep the changes scoped to the RedisCache
configuration block and ensure django-redis can manage the parser automatically
while still preserving the desired connection pool limits.

In `@backend/campaigns/ai.py`:
- Line 5: The ai module imports bleach directly, but the dependency is missing
from the Python manifest, so make sure bleach is added to requirements.txt
alongside the backend/campaigns/ai.py import. Update the dependency list so
environments that install from the manifest can import the module without
failure.

In `@backend/campaigns/tasks.py`:
- Around line 601-605: Encode the signed Message-ID payload before constructing
custom_mid in the campaign task flow: the current payload built in the signing
logic for clead.id and clead.organization_id can place invalid characters in the
msg-id left side. Update the Message-ID generation path to use a safe base64-url
encoded payload, and then adjust WebhookView.post to decode that value before
calling unsign() so the existing tracking/signature flow still works.

In `@backend/users/models.py`:
- Line 37: `create_superuser()` currently leaves `is_active` at its default
False, so superusers created through the UserManager end up unable to log in.
Update the superuser creation path in the user model/manager so that `is_active`
is explicitly set to True alongside `is_staff` and `is_superuser`, and make sure
the `User` model’s `create_superuser` logic enforces this value before saving.
- Line 43: The otp_secret field in the user model is currently stored as plain
text, which leaves the TOTP seed exposed if the database is compromised. Update
the users model to use the existing Fernet-backed encrypted secret pattern used
elsewhere in the codebase, and make sure backend/users/views.py continues to
write and read otp_secret through that encrypted field transparently. Keep the
change centered on the user model’s otp_secret definition and any
helper/property methods that handle secret persistence.

In `@backend/users/views.py`:
- Around line 153-209: The resend_verification action currently allows
unauthenticated email bombing and leaks account existence. Add the same kind of
per-IP/per-email rate limiting used by register to resend_verification before
sending mail, and keep the behavior generic for all non-error cases. In
resend_verification, replace the distinct "Email already verified. You can log
in." response with the same neutral message used when User.DoesNotExist is hit,
so the endpoint does not reveal whether a User exists or is active.
- Around line 430-436: The TOTP check in login_2fa currently has no throttling,
so add attempt rate limiting/lockout before calling pyotp.TOTP.verify. Reuse the
same throttling approach used elsewhere in the auth flow, keyed by user and/or
client IP, and apply it around the code path that validates the 6-digit code.
Update the login_2fa endpoint so repeated failed OTP submissions are rejected
after the configured limit and the lockout state is enforced consistently.
- Around line 256-291: The password-change throttling in the user update flow is
only applied after request.user.check_password succeeds, so incorrect
current_password attempts bypass the limit entirely. In the password-change
branch of the view that handles new_password, move the rate-limit lookup and
increment logic before the current-password verification, and ensure every
attempt (successful or not) consumes a count for the same rate_limit_key. Keep
the existing checks in the update method, but make sure the cache-based counter
is evaluated and updated before returning on an incorrect current password.
- Around line 381-389: The backup code flow is incomplete: the codes generated
in the 2FA enablement path are only cached temporarily and are never checked in
the 2FA login path. Update the 2FA setup logic that generates backup codes to
persist them durably in hashed form instead of relying on cache, and modify
login_2fa to validate submitted backup codes alongside the existing TOTP check.
Make sure each backup code is single-use by revoking it after successful
redemption.
- Around line 80-96: The registration flow in RegisterSerializer.create() leaves
behind an orphaned Organization when send_mail fails because only user.delete()
is called in the exception path. Update the create/verification email sequence
to run inside transaction.atomic() so the user and Organization are rolled back
together, and in the failure handling for the email-send block ensure the
Organization is also removed if it was already created.

In `@requirements.txt`:
- Line 13: The dependency floor for cryptography still permits the vulnerable
44.0.0 release; update the version constraint in requirements.txt to require at
least 46.0.6 (or a newer patched release). Use the cryptography package entry
itself to locate and adjust the minimum version so older affected releases are
no longer installable.

---

Outside diff comments:
In `@backend/campaigns/ai.py`:
- Around line 239-243: The early return in the Gemini generation flow is
checking only the global API key, so the organization-specific override is never
reached. Update the logic in the Gemini-key lookup path around the merged
subject/body handling and the `lead.organization.gemini_api_key` override so the
tenant key is resolved before the fallback/return condition, and make the final
decision use the tenant key first, then the global `GEMINI_API_KEY`, then the
sanitized fallback.

---

Nitpick comments:
In `@backend/backend/urls.py`:
- Line 41: The logout endpoint is duplicated because AuthViewSet is already
exposed through the DefaultRouter under api/v1/auth/. Remove the explicit logout
path from the urlpatterns in urls.py and rely on the router registration so
logout, setup_2fa, verify_2fa, and login_2fa continue to be served by
AuthViewSet without a redundant route.

In `@backend/campaigns/ai.py`:
- Around line 281-284: The Gemini fallback in the personalisation flow is too
broad because it catches Exception and also logs the “AI disabled for org” path
as an error. Update the exception handling around the Gemini/JSON parsing logic
in the relevant ai.py helper to catch only the specific expected
google-generativeai and JSON-related failures after verifying the installed
package’s exception classes, and keep the org-disabled branch separate from the
error handler.

In `@backend/campaigns/tasks.py`:
- Around line 602-603: The Signer usage in the webhook payload flow is sharing
the same default salt as the click-tracking token flow, so signatures are
reusable across purposes. Update the Signer() call where signed_payload is
created for the webhook message_id to use a purpose-specific salt, and mirror
that exact salt in the corresponding verification logic for the webhook path;
also ensure the Signer used in rewrite_email_links and ClickTrackingView keeps a
separate salt for click-tracking tokens.

In `@backend/users/views.py`:
- Around line 468-472: The exception handling in the relevant users view is too
broad and returns raw exception text to the client. Narrow the catch around the
token handling logic in the view method that contains this except block to only
handle expected token-related errors, and return a fixed client-safe error
message instead of str(e). Keep unexpected failures uncaught so they can surface
appropriately rather than being converted into a generic 400 response.
🪄 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: cb0b2442-8b1c-4411-83e0-6d8fe9c57ef6

📥 Commits

Reviewing files that changed from the base of the PR and between 4a33158 and 8ef7a8e.

📒 Files selected for processing (10)
  • backend/backend/settings.py
  • backend/backend/urls.py
  • backend/campaigns/ai.py
  • backend/campaigns/google_auth_views.py
  • backend/campaigns/mailbox_service.py
  • backend/campaigns/tasks.py
  • backend/campaigns/views.py
  • backend/users/models.py
  • backend/users/views.py
  • requirements.txt

Comment thread backend/backend/settings.py Outdated
Comment thread backend/backend/settings.py
Comment thread backend/backend/settings.py
Comment thread backend/campaigns/ai.py
Comment on lines +601 to +605
# Generate signed Message-ID for webhook tracking
signer = Signer()
signed_payload = signer.sign(f"{clead.id}:{clead.organization_id}")
domain = account.email_address.split('@', 1)[-1] if '@' in account.email_address else 'leadorbit.com'
custom_mid = f"<{signed_payload}@{domain}>"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

RFC 5322 Message-ID msg-id id-left allowed characters colon

💡 Result:

Under RFC 5322, the id-left component of a Message-ID is defined as dot-atom-text or obs-id-left [1][2]. According to the standard's ABNF, dot-atom-text consists of one or more dot-atoms separated by dots [1][3]. An atom is defined as a sequence of atext characters, which include alphanumeric characters and the following special characters:!#$%&'*+-/=?^_`{|}~ [4]. A colon (:) is not included in the set of allowed atext characters [4]. Consequently, a colon is not permitted within the dot-atom-text form of an id-left [4]. While obs-id-left allows for a more "obsolete" and permissive syntax (which may include quoted-strings or other structures that could theoretically contain broader characters), it is not the standard, recommended form for generating new Message-IDs [1][2][4]. Therefore, a colon should not be used in the id-left portion of a Message-ID [4].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant files first
git ls-files backend/campaigns/tasks.py backend/campaigns/views.py

# Show the relevant sections with line numbers
sed -n '560,640p' backend/campaigns/tasks.py | cat -n
printf '\n---\n'
sed -n '1,260p' backend/campaigns/views.py | cat -n

# Search for Message-ID handling and signer usage
rg -n "Message-ID|message_id|Signer\(|unsign\(|sign\(" backend/campaigns

Repository: Kuldeeep18/LeadOrbit

Length of output: 24871


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the webhook parsing path around the message_id handling
sed -n '400,470p' backend/campaigns/views.py | cat -n

# Inspect the shared signing utility and any other signer uses
sed -n '1,80p' backend/campaigns/utils.py | cat -n
sed -n '500,540p' backend/campaigns/tasks.py | cat -n
sed -n '820,850p' backend/campaigns/views.py | cat -n

# Check Django Signer defaults from installed docs/source if available through Python
python3 - <<'PY'
from django.core.signing import Signer
import inspect
print("Signer signature:", inspect.signature(Signer))
print("Default signer repr:", Signer())
PY

Repository: Kuldeeep18/LeadOrbit

Length of output: 8434


Encode the custom Message-ID payload before sending

Signer().sign(f"{clead.id}:{clead.organization_id}") puts : into the id-left, which is outside RFC 5322 msg-id syntax and can be rewritten or dropped by SMTP/MTA hops, breaking reply/bounce correlation. Base64-url encode the signed payload before embedding it, and decode it in WebhookView.post before unsign().

🤖 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/campaigns/tasks.py` around lines 601 - 605, Encode the signed
Message-ID payload before constructing custom_mid in the campaign task flow: the
current payload built in the signing logic for clead.id and
clead.organization_id can place invalid characters in the msg-id left side.
Update the Message-ID generation path to use a safe base64-url encoded payload,
and then adjust WebhookView.post to decode that value before calling unsign() so
the existing tracking/signature flow still works.

Comment thread backend/users/views.py
Comment thread backend/users/views.py Outdated
Comment thread backend/users/views.py Outdated
Comment on lines +381 to +389
# Generate backup codes
backup_codes = [pyotp.random_base32()[:8] for _ in range(10)]
# Store backup codes in cache for now (in production, store encrypted in DB)
cache.set(f"2fa_backup_{user.id}", backup_codes, timeout=86400) # 24 hours

return Response({
'message': '2FA enabled successfully.',
'backup_codes': backup_codes
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant file slices with line numbers
sed -n '340,460p' backend/users/views.py | cat -n

# Search for any backup-code handling elsewhere in the users backend
rg -n "backup[_-]?code|2fa_backup|backup codes|backup_codes" backend/users

Repository: Kuldeeep18/LeadOrbit

Length of output: 5496


Backup codes need a redemption path

They’re stored only in cache with a 24h TTL, and login_2fa only accepts TOTP against otp_secret, so the issued backup codes can’t be used. Persist them durably (hashed) and accept them during 2FA login, with single-use revocation.

🤖 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 381 - 389, The backup code flow is
incomplete: the codes generated in the 2FA enablement path are only cached
temporarily and are never checked in the 2FA login path. Update the 2FA setup
logic that generates backup codes to persist them durably in hashed form instead
of relying on cache, and modify login_2fa to validate submitted backup codes
alongside the existing TOTP check. Make sure each backup code is single-use by
revoking it after successful redemption.

Comment thread backend/users/views.py
Comment thread requirements.txt Outdated
- Encrypted OTP secret storage
- Added rate limiting to 2FA endpoints
- Fixed superuser creation
- Fixed duplicate logout route
- Updated cryptography to secure version
- Added bleach dependency

Fixes Kuldeeep18#628
@ShreyasPatil3105

Copy link
Copy Markdown
Author

@Kuldeeep18, this PR is ready for final review.

✅ All CodeRabbit checks have passed
✅ All review comments have been addressed
✅ No merge conflicts
✅ 2FA implementation complete with security fixes

Please review and merge when you get a chance.

This contribution is part of GSSoC 2026.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Critical] [Security] No Two-Factor Authentication (2FA) / MFA implementation allows account takeover

1 participant