Fix #618: Explicitly set JWT algorithm and signing key#619
Fix #618: Explicitly set JWT algorithm and signing key#619ShreyasPatil3105 wants to merge 5 commits into
Conversation
- 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
|
@Kuldeeep18 This PR fixes a critical issue where JWT uses default HS256 algorithm with SECRET_KEY. Please review and add the appropriate GSSoC labels. This contribution is part of GSSoC 2026. |
📝 WalkthroughWalkthroughThis PR sanitizes AI-generated email content with bleach, and hardens multi-tenant security across OAuth flows, outbound email sending, and webhook/tracking endpoints by adding organization-scoped lookups, signed Message-ID tracking, and tenant-mismatch validation. ChangesAI Content Sanitization
Multi-Tenant Security Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 2
🤖 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/tasks.py`:
- Around line 601-614: The Message-ID construction in the signed webhook
tracking block uses Signer().sign with a payload that can produce a local part
containing “:”, which is not RFC-safe for the id-left in custom_mid. Update the
logic around send_smtp_email in the clead/organization_id Message-ID flow to
encode or transform the signed payload into a dot-atom-safe token before
interpolating it into the Message-ID, while keeping the domain selection and
custom_mid usage intact.
In `@backend/campaigns/views.py`:
- Around line 841-855: In the recipient click flow in CampaignLead handling, do
not return HttpResponseBadRequest for missing org or organization mismatches
after the signed token is verified. Instead, in the block that uses
CampaignLead.objects.select_related(...).get and checks organization_id on lead,
campaign, and lead.lead, log the inconsistency, skip the analytics/write path
for that record, and continue to the existing redirect to dest so the endpoint
still honors the failsafe redirect behavior.
🪄 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: ab691190-03dd-4e0e-b08a-32c723ff0124
📒 Files selected for processing (5)
backend/campaigns/ai.pybackend/campaigns/google_auth_views.pybackend/campaigns/mailbox_service.pybackend/campaigns/tasks.pybackend/campaigns/views.py
| # 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}>" | ||
|
|
||
| message_id = send_smtp_email( | ||
| account, | ||
| clead.lead.email, | ||
| subject, | ||
| body, | ||
| unsubscribe_url=build_unsubscribe_url(clead.lead), | ||
| message_id=custom_mid, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
RFC 5322 Message-ID id-left dot-atom allowed characters colon
💡 Result:
Under RFC 5322, the id-left portion of a Message-ID must adhere to the dot-atom-text syntax [1][2][3]. The dot-atom-text construct consists of one or more atext characters, optionally separated by dots [3]. The atext characters are defined as: - Letters (a-z, A-Z) - Digits (0-9) - The following special characters:! # $ % & ' * + - / =? ^ _ ` { | } ~ The colon (:) character is not included in the set of allowed atext characters [3]. Therefore, a colon is not permitted in the id-left portion of a Message-ID unless it is part of an obsolete syntax (obs-id-left), which is generally discouraged [1][2]. As the id-left is constrained to dot-atom-text, implementing systems should not include colons in this field [1][3].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc5322.txt
- 2: https://datatracker.ietf.org/doc/rfc5322/
- 3: https://smtpedia.com/message-id-header/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target code and related webhook/message-id handling.
printf '\n== backend/campaigns/tasks.py around target lines ==\n'
sed -n '560,640p' backend/campaigns/tasks.py
printf '\n== search for message_id / Signer / unsign usage ==\n'
rg -n "Signer\(|sign\(|unsign\(|message_id|last_sent_message_id|custom_mid|Message-ID|msg-id" backend -S
printf '\n== likely webhook handler files (outline first, then relevant slices) ==\n'
fd -a "webhook" backendRepository: Kuldeeep18/LeadOrbit
Length of output: 1963
Message-ID local part contains an invalid :
Signer().sign(f"{clead.id}:{clead.organization_id}") puts colons into the id-left of custom_mid, which is outside RFC 5322 dot-atom-text. Some relays can reject or rewrite it, so encode the token into an RFC-safe local part before using it as 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 601 - 614, The Message-ID
construction in the signed webhook tracking block uses Signer().sign with a
payload that can produce a local part containing “:”, which is not RFC-safe for
the id-left in custom_mid. Update the logic around send_smtp_email in the
clead/organization_id Message-ID flow to encode or transform the signed payload
into a dot-atom-safe token before interpolating it into the Message-ID, while
keeping the domain selection and custom_mid usage intact.
| # Prefetch relations to verify organization consistency | ||
| lead = CampaignLead.objects.select_related('organization', 'campaign', 'lead').get(id=campaign_lead_id) | ||
|
|
||
| # Verify organization presence and consistency | ||
| if not lead.organization_id: | ||
| return HttpResponseBadRequest("Invalid organization context.") | ||
|
|
||
| # Verify campaign organization matches | ||
| if lead.campaign and lead.campaign.organization_id != lead.organization_id: | ||
| return HttpResponseBadRequest("Campaign organization mismatch.") | ||
|
|
||
| # Verify lead organization matches | ||
| if lead.lead and lead.lead.organization_id != lead.organization_id: | ||
| return HttpResponseBadRequest("Lead organization mismatch.") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Org-consistency failures abort the recipient redirect; skip analytics but still redirect.
This is a public click endpoint reached by legitimate recipients after the signed t token is already verified. Returning HttpResponseBadRequest on missing org or campaign/lead org mismatch shows the recipient an error page instead of sending them to dest. An org mismatch is an internal data-consistency problem, not something the recipient caused — the surrounding code already signals intent to always redirect (except CampaignLead.DoesNotExist: pass # Failsafe: Continue to redirect). These new guards break that guarantee. Log the anomaly and skip the analytics write, but continue to the redirect.
🔧 Proposed fix: skip analytics on mismatch, still redirect
- # Verify organization presence and consistency
- if not lead.organization_id:
- return HttpResponseBadRequest("Invalid organization context.")
-
- # Verify campaign organization matches
- if lead.campaign and lead.campaign.organization_id != lead.organization_id:
- return HttpResponseBadRequest("Campaign organization mismatch.")
-
- # Verify lead organization matches
- if lead.lead and lead.lead.organization_id != lead.organization_id:
- return HttpResponseBadRequest("Lead organization mismatch.")
-
- lead.last_clicked_at = timezone.now()
- lead.save(update_fields=['last_clicked_at'])
-
- # Optional: Agar conditionally aage badhana hai sequence ko
- if lead.current_step and lead.current_step.channel_type == 'CONDITION_CLICK':
- from .tasks import _execute_condition_click_step
- _execute_condition_click_step(lead, lead.current_step, now=timezone.now())
+ org_ok = (
+ lead.organization_id
+ and (not lead.campaign or lead.campaign.organization_id == lead.organization_id)
+ and (not lead.lead or lead.lead.organization_id == lead.organization_id)
+ )
+ if org_ok:
+ lead.last_clicked_at = timezone.now()
+ lead.save(update_fields=['last_clicked_at'])
+
+ # Optional: Agar conditionally aage badhana hai sequence ko
+ if lead.current_step and lead.current_step.channel_type == 'CONDITION_CLICK':
+ from .tasks import _execute_condition_click_step
+ _execute_condition_click_step(lead, lead.current_step, now=timezone.now())
+ else:
+ logger.warning(
+ "Skipping click analytics for lead %s due to organization mismatch.",
+ campaign_lead_id,
+ )📝 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.
| # Prefetch relations to verify organization consistency | |
| lead = CampaignLead.objects.select_related('organization', 'campaign', 'lead').get(id=campaign_lead_id) | |
| # Verify organization presence and consistency | |
| if not lead.organization_id: | |
| return HttpResponseBadRequest("Invalid organization context.") | |
| # Verify campaign organization matches | |
| if lead.campaign and lead.campaign.organization_id != lead.organization_id: | |
| return HttpResponseBadRequest("Campaign organization mismatch.") | |
| # Verify lead organization matches | |
| if lead.lead and lead.lead.organization_id != lead.organization_id: | |
| return HttpResponseBadRequest("Lead organization mismatch.") | |
| # Prefetch relations to verify organization consistency | |
| lead = CampaignLead.objects.select_related('organization', 'campaign', 'lead').get(id=campaign_lead_id) | |
| org_ok = ( | |
| lead.organization_id | |
| and (not lead.campaign or lead.campaign.organization_id == lead.organization_id) | |
| and (not lead.lead or lead.lead.organization_id == lead.organization_id) | |
| ) | |
| if org_ok: | |
| lead.last_clicked_at = timezone.now() | |
| lead.save(update_fields=['last_clicked_at']) | |
| # Optional: Agar conditionally aage badhana hai sequence ko | |
| if lead.current_step and lead.current_step.channel_type == 'CONDITION_CLICK': | |
| from .tasks import _execute_condition_click_step | |
| _execute_condition_click_step(lead, lead.current_step, now=timezone.now()) | |
| else: | |
| logger.warning( | |
| "Skipping click analytics for lead %s due to organization mismatch.", | |
| campaign_lead_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/views.py` around lines 841 - 855, In the recipient click
flow in CampaignLead handling, do not return HttpResponseBadRequest for missing
org or organization mismatches after the signed token is verified. Instead, in
the block that uses CampaignLead.objects.select_related(...).get and checks
organization_id on lead, campaign, and lead.lead, log the inconsistency, skip
the analytics/write path for that record, and continue to the existing redirect
to dest so the endpoint still honors the failsafe redirect behavior.
1 participant
Fixes #618
Changes Made:
ALGORITHMtoHS256SIGNING_KEYenvironment variable supportSecurity Impact:
Testing:
GSSoC Level: Critical
This contribution is part of GSSoC 2026.
Summary by CodeRabbit
New Features
Bug Fixes