Fix #604: Add current password validation and rate limiting to password change#605
Open
ShreyasPatil3105 wants to merge 6 commits into
Open
Fix #604: Add current password validation and rate limiting to password change#605ShreyasPatil3105 wants to merge 6 commits into
ShreyasPatil3105 wants to merge 6 commits into
Conversation
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
… to password change - Added current password requirement for changing password - Added rate limiting (3 attempts per hour) for password changes - Prevents account takeover via stolen session tokens Fixes Kuldeeep18#604
Author
|
@Kuldeeep18 This PR fixes a critical security vulnerability where password changes could be made without current password verification. Please review and add the appropriate GSSoC labels. This contribution is part of GSSoC 2026. |
Contributor
📝 WalkthroughWalkthroughThis PR adds bleach-based sanitization to AI-generated email content, enforces organization scoping in Google OAuth login/callback flows, adds an optional signed message_id used to track and validate webhook/unsubscribe/click-tracking requests against the correct organization, and requires current-password verification plus rate limiting for password changes. ChangesEmail Content Sanitization
Google OAuth Organization Enforcement
Signed Message-ID Tracking and Tenant Validation
Password Change Security Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Task as send_email_step
participant Mailbox as send_smtp_email
participant Provider as SMTP Provider
participant Webhook as WebhookView
participant DB as CampaignLead
Task->>Task: Sign payload (Signer)
Task->>Mailbox: send_smtp_email(message_id=signed_id)
Mailbox->>Provider: Send email with Message-ID header
Provider-->>Webhook: Delivery event with message_id
Webhook->>Webhook: Verify signed token (Signer)
alt token valid
Webhook->>DB: Query CampaignLead(id, organization_id, status)
else token invalid
Webhook->>DB: Fallback lookup by lead email
Webhook->>Webhook: Check for cross-tenant ambiguity
end
DB-->>Webhook: CampaignLead rows
Webhook-->>Provider: 200 OK or 400 Bad Request
sequenceDiagram
participant Client
participant AuthView as AuthViewSet.me
participant Cache
participant User as request.user
Client->>AuthView: PATCH /me (new_password, current_password)
AuthView->>Cache: Check rate limit (user+IP)
alt rate limit exceeded
AuthView-->>Client: 429 Too Many Requests
else within limit
AuthView->>User: check_password(current_password)
alt invalid current password
AuthView-->>Client: 400 Bad Request
else valid
AuthView->>AuthView: validate_password(new_password)
AuthView->>User: set_password(new_password) + save
AuthView->>Cache: Increment attempt count
AuthView-->>Client: 200 OK
end
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/campaigns/ai.py`:
- Around line 24-27: The HTML sanitization in ai.py currently allows <a> target
without enforcing safe rel attributes, which can leave tabnabbing risk. Update
the sanitizer configuration around allowed_attrs so that the link handling in
the relevant HTML-cleaning path also forces rel="noopener noreferrer" whenever
target is permitted, using the existing attribute callback/cleaning logic tied
to the <a> rule.
- Around line 18-28: The _sanitize_content helper currently permits inline style
attributes without any CSS sanitization, so sanitize the HTML path more
strictly. Update the bleach.clean call in _sanitize_content to either remove the
wildcard style allowance from allowed_attrs or supply a CSS sanitizer when
keeping style support, and keep the fix localized to this function so lead
content cannot pass unsanitized CSS through.
In `@backend/campaigns/tasks.py`:
- Around line 603-605: The Message-ID generation in the campaign task flow uses
a colon-delimited signer payload that is not RFC-safe for strict SMTP receivers.
Update the logic around the custom_mid construction in the task that builds the
outbound Message-ID, and align the webhook decode path that currently
unsigns/splits the token so both use a dot-atom-safe delimiter scheme instead of
the current colon-based format. Ensure the sender and receiver stay in sync by
changing the token format in both the generator and the corresponding decoder
methods that handle the signed identifier.
In `@backend/campaigns/views.py`:
- Around line 452-453: Normalize the incoming message ID before the legacy
fallback filter so bracketed webhook values can match stored IDs. In the legacy
lookup path around the `message_id` check in `views.py`, strip angle brackets
once before calling `base_qs.filter(last_sent_message_id=...)`, and reuse that
normalized value consistently so the `last_sent_message_id` comparison aligns
with what `send_smtp_email` stores.
- Around line 423-467: The signed Message-ID fast path in the webhook handler is
still blocked by the outer `event_type and lead_email` check, so message-ID-only
payloads never reach the `Signer().unsign` / `CampaignLead.objects.filter(...)`
logic. Update the branching in `views.py` so the secure token verification path
runs whenever `message_id` is present, independent of `lead_email`, and only
fall back to the legacy `lead_email` lookup when `lead_email` exists; keep the
existing `campaign_lead_id` and `organization_id` handling intact.
In `@backend/users/views.py`:
- Around line 88-114: The password-change limiter in the user password update
flow is bypassed for incorrect current passwords because check_password()
happens before the rate-limit logic. Update the password-change handler in
users/views.py to apply the rate-limit check before verifying current_password,
and increment the counter for every attempt (successful or not) using an atomic
cache add/incr approach or DRF throttling. Keep the existing password validation
and save logic in the same password-change branch, but ensure the limiter is
enforced first in the relevant view/method.
🪄 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: bb6c20aa-a8dd-4ab2-9cd1-f54e30066892
📒 Files selected for processing (6)
backend/campaigns/ai.pybackend/campaigns/google_auth_views.pybackend/campaigns/mailbox_service.pybackend/campaigns/tasks.pybackend/campaigns/views.pybackend/users/views.py
Comment on lines
+18
to
+28
| def _sanitize_content(text): | ||
| if not text: | ||
| return "" | ||
| allowed_tags = [ | ||
| 'a', 'b', 'i', 'strong', 'em', 'p', 'br', 'ul', 'ol', 'li', 'span', 'div' | ||
| ] | ||
| allowed_attrs = { | ||
| 'a': ['href', 'target', 'title'], | ||
| '*': ['style'] | ||
| } | ||
| return bleach.clean(str(text), tags=allowed_tags, attributes=allowed_attrs, strip=True) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
style is allowed without CSS sanitization. Bleach leaves inline CSS untouched unless a css_sanitizer is provided, so lead-supplied HTML can still carry arbitrary CSS through this path. Drop style or add a CSS sanitizer.
🤖 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 18 - 28, The _sanitize_content helper
currently permits inline style attributes without any CSS sanitization, so
sanitize the HTML path more strictly. Update the bleach.clean call in
_sanitize_content to either remove the wildcard style allowance from
allowed_attrs or supply a CSS sanitizer when keeping style support, and keep the
fix localized to this function so lead content cannot pass unsanitized CSS
through.
Comment on lines
+24
to
+27
| allowed_attrs = { | ||
| 'a': ['href', 'target', 'title'], | ||
| '*': ['style'] | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
target allowed on <a> without enforcing rel="noopener noreferrer".
Allowing target without forcing rel on links opens a minor tabnabbing risk if body content is rendered as HTML with target="_blank" links.
🛡️ Force rel via attribute callback
allowed_attrs = {
- 'a': ['href', 'target', 'title'],
+ 'a': ['href', 'target', 'rel', 'title'],
'*': ['style']
}
- return bleach.clean(str(text), tags=allowed_tags, attributes=allowed_attrs, strip=True)
+ cleaned = bleach.clean(str(text), tags=allowed_tags, attributes=allowed_attrs, strip=True)
+ return re.sub(r'<a\s+([^>]*target="_blank"[^>]*)>', lambda m: f'<a {m.group(1)} rel="noopener noreferrer">', cleaned)🤖 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 24 - 27, The HTML sanitization in ai.py
currently allows <a> target without enforcing safe rel attributes, which can
leave tabnabbing risk. Update the sanitizer configuration around allowed_attrs
so that the link handling in the relevant HTML-cleaning path also forces
rel="noopener noreferrer" whenever target is permitted, using the existing
attribute callback/cleaning logic tied to the <a> rule.
Comment on lines
+603
to
+605
| 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}>" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Message-ID token generation/parsing sites and check current colon-bearing local parts.
rg -n "custom_mid|Signer\\(|signer\\.sign|signer\\.unsign|Message-ID" backend/campaigns
python - <<'PY'
import re
dot_atom = re.compile(r"[A-Za-z0-9!#$%&'*+\-/=?^_`{|}~]+(?:\.[A-Za-z0-9!#$%&'*+\-/=?^_`{|}~]+)*\Z")
current = "00000000-0000-0000-0000-000000000000:11111111-1111-1111-1111-111111111111:signature"
proposed = "00000000-0000-0000-0000-000000000000~11111111-1111-1111-1111-111111111111.signature"
print("current_valid_dot_atom=", bool(dot_atom.fullmatch(current)))
print("proposed_valid_dot_atom=", bool(dot_atom.fullmatch(proposed)))
PYRepository: Kuldeeep18/LeadOrbit
Length of output: 1893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant encode/decode paths in small slices.
sed -n '500,540p' backend/campaigns/tasks.py
printf '\n---\n'
sed -n '416,442p' backend/campaigns/views.py
printf '\n---\n'
sed -n '820,842p' backend/campaigns/views.py
printf '\n---\n'
sed -n '1,80p' backend/campaigns/utils.py
# Check whether Message-ID local-part rules exclude ':' but allow '.' and '~'.
python3 - <<'PY'
import re
dot_atom = re.compile(r"[A-Za-z0-9!#$%&'*+\-/=?^_`{|}~]+(?:\.[A-Za-z0-9!#$%&'*+\-/=?^_`{|}~]+)*\Z")
samples = {
"current": "00000000-0000-0000-0000-000000000000:11111111-1111-1111-1111-111111111111:signature",
"proposed": "00000000-0000-0000-0000-000000000000~11111111-1111-1111-1111-111111111111.signature",
"related_view_token": "00000000-0000-0000-0000-000000000000:signature",
}
for name, s in samples.items():
print(name, bool(dot_atom.fullmatch(s)))
PYRepository: Kuldeeep18/LeadOrbit
Length of output: 4179
Use an RFC-safe Message-ID token format. The current custom_mid embeds a colon-delimited Signer.sign() value in the unquoted local part, which strict SMTP receivers can reject or rewrite. Switch the Message-ID generator and the webhook decoder to a dot-atom-safe delimiter scheme so the unsign()/split logic stays aligned.
🤖 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 603 - 605, The Message-ID generation
in the campaign task flow uses a colon-delimited signer payload that is not
RFC-safe for strict SMTP receivers. Update the logic around the custom_mid
construction in the task that builds the outbound Message-ID, and align the
webhook decode path that currently unsigns/splits the token so both use a
dot-atom-safe delimiter scheme instead of the current colon-based format. Ensure
the sender and receiver stay in sync by changing the token format in both the
generator and the corresponding decoder methods that handle the signed
identifier.
Comment on lines
+423
to
+467
| # ── Secure token verification (Fast Path) ── | ||
| campaign_lead_id = None | ||
| organization_id = None | ||
|
|
||
| if message_id: | ||
| base_qs = base_qs.filter(last_sent_message_id=message_id) | ||
| cleads = list(base_qs) | ||
| signer = Signer() | ||
| try: | ||
| # Clean delimiters and extract local part | ||
| local_part = message_id.strip('<>').split('@')[0] | ||
| unsigned_payload = signer.unsign(local_part) | ||
| campaign_lead_id, organization_id = unsigned_payload.split(':') | ||
| except (BadSignature, ValueError): | ||
| # Invalid signature - will fall back to email lookup | ||
| pass | ||
|
|
||
| # Layer 1: Secure token verification | ||
| if campaign_lead_id and organization_id: | ||
| cleads = CampaignLead.objects.filter( | ||
| id=campaign_lead_id, | ||
| organization_id=organization_id, | ||
| status__in=tracked_statuses | ||
| ) | ||
| cleads = list(cleads) | ||
| else: | ||
| # Layer 2: Legacy fallback with cross-tenant protection | ||
| base_qs = CampaignLead.objects.filter( | ||
| lead__email=lead_email, | ||
| status__in=tracked_statuses, | ||
| ) | ||
| if message_id: | ||
| base_qs = base_qs.filter(last_sent_message_id=message_id) | ||
| cleads = list(base_qs) | ||
|
|
||
| # Security Guard: Reject cross-tenant matches | ||
| if len(cleads) > 1: | ||
| org_ids = {str(cl.organization_id) for cl in cleads} | ||
| if len(org_ids) > 1: | ||
| logger.warning( | ||
| f"Aborted webhook update for email {lead_email}: " | ||
| f"ambiguous tenant context across orgs {org_ids}." | ||
| ) | ||
| return Response( | ||
| {"error": "Ambiguous tenant context"}, | ||
| status=status.HTTP_400_BAD_REQUEST | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Let signed Message-ID webhooks process without requiring email.
The new fast path can derive campaign_lead_id and organization_id from message_id, but it is still nested under the event_type and lead_email gate, so message-ID-only provider payloads return 200 without updating anything.
Proposed fix
- if event_type and lead_email:
+ if event_type and (lead_email or message_id):Then only run the legacy email fallback when lead_email is present.
🤖 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/views.py` around lines 423 - 467, The signed Message-ID
fast path in the webhook handler is still blocked by the outer `event_type and
lead_email` check, so message-ID-only payloads never reach the `Signer().unsign`
/ `CampaignLead.objects.filter(...)` logic. Update the branching in `views.py`
so the secure token verification path runs whenever `message_id` is present,
independent of `lead_email`, and only fall back to the legacy `lead_email`
lookup when `lead_email` exists; keep the existing `campaign_lead_id` and
`organization_id` handling intact.
Comment on lines
+452
to
+453
| if message_id: | ||
| base_qs = base_qs.filter(last_sent_message_id=message_id) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize message_id before legacy lookup.
send_smtp_email stores IDs without angle brackets, but webhooks commonly send the header value with brackets. Normalize once before filtering so the legacy fallback can actually match stored last_sent_message_id.
Proposed fix
- if message_id:
- base_qs = base_qs.filter(last_sent_message_id=message_id)
+ if message_id:
+ base_qs = base_qs.filter(last_sent_message_id=message_id.strip('<>'))📝 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.
Suggested change
| if message_id: | |
| base_qs = base_qs.filter(last_sent_message_id=message_id) | |
| if message_id: | |
| base_qs = base_qs.filter(last_sent_message_id=message_id.strip('<>')) |
🤖 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/views.py` around lines 452 - 453, Normalize the incoming
message ID before the legacy fallback filter so bracketed webhook values can
match stored IDs. In the legacy lookup path around the `message_id` check in
`views.py`, strip angle brackets once before calling
`base_qs.filter(last_sent_message_id=...)`, and reuse that normalized value
consistently so the `last_sent_message_id` comparison aligns with what
`send_smtp_email` stores.
Comment on lines
+88
to
+114
| if not request.user.check_password(current_password): | ||
| return Response( | ||
| {'error': 'Current password is incorrect.'}, | ||
| status=status.HTTP_400_BAD_REQUEST | ||
| ) | ||
|
|
||
| # Rate limiting for password changes (3 attempts per hour) | ||
| ip = request.META.get('REMOTE_ADDR') | ||
| rate_limit_key = f"password_change_{ip}_{request.user.id}" | ||
| attempt_count = cache.get(rate_limit_key, 0) | ||
| if attempt_count >= 3: | ||
| return Response( | ||
| {'error': 'Too many password change attempts. Please try again later.'}, | ||
| status=status.HTTP_429_TOO_MANY_REQUESTS | ||
| ) | ||
|
|
||
| try: | ||
| validate_password(new_password, request.user) | ||
| except DjangoValidationError as exc: | ||
| return Response({'new_password': list(exc.messages)}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| request.user.set_password(new_password) | ||
| request.user.save(update_fields=['password']) | ||
| updates_made = True | ||
|
|
||
| # Increment rate limit counter | ||
| cache.set(rate_limit_key, attempt_count + 1, timeout=3600) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and surrounding context.
git ls-files backend/users/views.py
wc -l backend/users/views.py
cat -n backend/users/views.py | sed -n '1,180p'Repository: Kuldeeep18/LeadOrbit
Length of output: 7336
Count failed current-password attempts in the password-change rate limit.
check_password() runs before the limiter, so incorrect current passwords return without incrementing it. Move the rate-limit check ahead of password verification and count every password-change attempt; use an atomic cache increment/add or DRF throttle to avoid races.
🤖 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 88 - 114, The password-change limiter in
the user password update flow is bypassed for incorrect current passwords
because check_password() happens before the rate-limit logic. Update the
password-change handler in users/views.py to apply the rate-limit check before
verifying current_password, and increment the counter for every attempt
(successful or not) using an atomic cache add/incr approach or DRF throttling.
Keep the existing password validation and save logic in the same password-change
branch, but ensure the limiter is enforced first in the relevant view/method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #604
Changes Made:
Security Impact:
Testing:
GSSoC Level: Critical
This contribution is part of GSSoC 2026.
Summary by CodeRabbit
New Features
Bug Fixes