Fix #581: Sanitize AI-generated content to prevent XSS#583
Fix #581: Sanitize AI-generated content to prevent XSS#583ShreyasPatil3105 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
📝 WalkthroughWalkthroughThis PR sanitizes AI-generated email subject/body content with bleach, enforces organization presence and tenant scoping in Google OAuth login/callback flows, and adds signed Message-ID correlation for webhook, unsubscribe, and click-tracking endpoints with cross-tenant ambiguity guards. ChangesEmail Content Sanitization
Google OAuth Tenant Scoping
Signed Message-ID Tracking & Secure Validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EmailProvider
participant WebhookView
participant Signer
participant CampaignLead
EmailProvider->>WebhookView: POST event with message_id
WebhookView->>Signer: unsign(local part of message_id)
Signer-->>WebhookView: campaign_lead_id, organization_id
WebhookView->>CampaignLead: query by id + organization_id, tracked statuses
alt secure verification fails
WebhookView->>CampaignLead: legacy lookup by email + message_id
CampaignLead-->>WebhookView: matches spanning multiple organizations
WebhookView-->>EmailProvider: 400 Ambiguous tenant context
else verified match found
WebhookView->>CampaignLead: update tracked status
WebhookView-->>EmailProvider: 200 OK
end
sequenceDiagram
participant User
participant GoogleOAuthCallbackView
participant Organization
participant UserModel
User->>GoogleOAuthCallbackView: OAuth callback with state
GoogleOAuthCallbackView->>Organization: get(id=org_id)
alt organization not found
Organization-->>GoogleOAuthCallbackView: DoesNotExist
GoogleOAuthCallbackView-->>User: redirect reason=org_not_found
else organization found
GoogleOAuthCallbackView->>UserModel: get(id=user_id, organization_id=org_id)
alt user not in org
UserModel-->>GoogleOAuthCallbackView: DoesNotExist
GoogleOAuthCallbackView-->>User: redirect reason=user_not_in_org
else user found
GoogleOAuthCallbackView-->>User: complete linking
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 |
|
@Kuldeeep18 I'd like to request this issue to be labeled as CRITICAL level for the following reasons: Why This is CRITICAL:
CVSS Score: 9.1 (CRITICAL)
Real-World Impact
Please consider labeling this as This contribution is part of GSSoC 2026. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/campaigns/ai.py (1)
264-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDisabled personalization is surfaced as an error via the broad
except.When
enable_ai_personalizationis off, raising a genericExceptionhere is caught at Line 282 and logged asGemini Personalization Error, conflating an intentional business rule with an actual failure (also flagged by Ruff BLE001). Behavior is correct (falls back to sanitized merged content), but consider handling the disabled case explicitly to avoid misleading error logs.🤖 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 264 - 266, The disabled-personalization branch in the AI personalization flow is being treated like a failure because it raises a generic Exception in the lead.organization check and is then caught by the broad except in the same function, causing a misleading Gemini Personalization Error log. Update the logic in the personalization method to handle the enable_ai_personalization=false case explicitly without using a generic exception, and keep the fallback to sanitized merged content while reserving the catch block for real unexpected errors.Source: Linters/SAST tools
🤖 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-605: The signed Message-ID generation in the webhook tracking
flow still exposes raw identifiers because Signer() only signs, not obscures,
the f"{clead.id}:{clead.organization_id}" payload. Update the logic in the
Message-ID construction path to use an opaque token generated from the lead
context instead of embedding clead.id and clead.organization_id directly, and
keep the custom_mid/domain assembly intact so the header remains unique without
revealing internal IDs.
---
Nitpick comments:
In `@backend/campaigns/ai.py`:
- Around line 264-266: The disabled-personalization branch in the AI
personalization flow is being treated like a failure because it raises a generic
Exception in the lead.organization check and is then caught by the broad except
in the same function, causing a misleading Gemini Personalization Error log.
Update the logic in the personalization method to handle the
enable_ai_personalization=false case explicitly without using a generic
exception, and keep the fallback to sanitized merged content while reserving the
catch block for real unexpected errors.
🪄 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: 1330bd7f-831e-433f-b037-70a7f1133674
📒 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}>" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Django django.core.signing.Signer encrypt the signed value or only sign it in plaintext?
💡 Result:
The Django django.core.signing.Signer class does not encrypt the signed value [1][2]. It only provides cryptographic signing [1][3]. The Signer class works by appending a signature—an HMAC (Hash-based Message Authentication Code)—to the plaintext value, separated by a specified separator (default is a colon :) [2][4]. This process allows you to verify that the value has not been tampered with by an untrusted party, but it does not hide or obscure the contents of the value itself [5][3]. Anyone with access to the signed string can decode or see the original plaintext value [2][4]. In contrast, if you need to protect the confidentiality of the data (i.e., hide its contents), you would need to use an encryption library rather than django.core.signing.Signer. { "key": "The Signer class appends an HMAC signature to the plaintext value, providing integrity and authenticity, but it does not provide encryption or confidentiality [1][2][4].", "citations": [1][2][5][4][3], "confidence": 1.0 }
Citations:
- 1: https://docs.djangoproject.com/en/6.1/topics/signing/
- 2: https://docs.djangoproject.com/en/4.2/_modules/django/core/signing/
- 3: https://docs.djangoproject.com/en/5.0/topics/signing/
- 4: https://github.com/django/django/blob/stable/6.0.x/django/core/signing.py
- 5: https://docs.djangoproject.com/en/6.0/topics/signing/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tasks.py excerpt ==\n'
sed -n '560,640p' backend/campaigns/tasks.py
printf '\n== search for Message-ID / custom_mid usages ==\n'
rg -n "custom_mid|Message-ID|signed_payload|Signer\(" backend -S
printf '\n== webhook view candidates ==\n'
rg -n "WebhookView|unsign|signing|CampaignLead" backend -SRepository: Kuldeeep18/LeadOrbit
Length of output: 2029
Message-ID should not embed raw IDs
Signer() only authenticates the value; it does not hide it. f"{clead.id}:{clead.organization_id}" is therefore exposed in plaintext inside the Message-ID header. If these identifiers shouldn’t be visible to recipients or mail relays, switch to an opaque token instead of signing the raw IDs directly.
🤖 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, The signed Message-ID
generation in the webhook tracking flow still exposes raw identifiers because
Signer() only signs, not obscures, the f"{clead.id}:{clead.organization_id}"
payload. Update the logic in the Message-ID construction path to use an opaque
token generated from the lead context instead of embedding clead.id and
clead.organization_id directly, and keep the custom_mid/domain assembly intact
so the header remains unique without revealing internal IDs.
There was a problem hiding this comment.
@coderabbitai The Message-ID improvement mentioned is already being addressed in PR #601. This PR focuses on the XSS sanitization fix.
There was a problem hiding this comment.
@ShreyasPatil3105 Understood — thanks for the clarification. Since the Message-ID opacity improvement is already tracked in #601, I'll leave it out of scope here. This PR's focus on the XSS sanitization fix in backend/campaigns/ai.py looks good on that front.
|
@Kuldeeep18 Please add GSSoC labels for this CRITICAL-level issue. |
|
Hi @ShreyasPatil3105 👋 LeadOrbit Bot here! Thanks for opening a pull request and contributing to the project. Before your PR is reviewed, please make sure you've: We noticed that you haven't starred the repository yet. If you enjoy the project and would like to support it, please consider giving it a ⭐. It helps the project grow and motivates the maintainers. Once you've starred the repository, your PR will continue through the review process. Thanks for contributing! 🚀 |
2 participants
Fixes #581
Issue
Critical XSS vulnerability in AI draft generation - user input was not sanitized before being returned.
Fix
Impact
This patch prevents:
Testing
<script>alert('XSS')</script>)Security Severity: CRITICAL
This contribution is part of GSSoC 2026.
Summary by CodeRabbit