Skip to content

Fix #600: Remove hardcoded fallback SECRET_KEY#601

Open
ShreyasPatil3105 wants to merge 8 commits into
Kuldeeep18:mainfrom
ShreyasPatil3105:fix-600-secret-key
Open

Fix #600: Remove hardcoded fallback SECRET_KEY#601
ShreyasPatil3105 wants to merge 8 commits into
Kuldeeep18:mainfrom
ShreyasPatil3105:fix-600-secret-key

Conversation

@ShreyasPatil3105

@ShreyasPatil3105 ShreyasPatil3105 commented Jul 7, 2026

Copy link
Copy Markdown

Fixes #600

Changes Made:

  • Removed hardcoded fallback SECRET_KEY
  • Added validation to ensure SECRET_KEY is set in environment
  • Removed hardcoded fallback encryption key
  • Added validation for encryption key in production

Security Impact:

  • Fixes critical hardcoded secret vulnerability
  • Prevents session hijacking and token forgery
  • Ensures proper key management in production

Testing:

  • Verified SECRET_KEY must be set in environment
  • Verified encryption key enforcement in production

GSSoC Level: Critical

This contribution is part of GSSoC 2026.

Summary by CodeRabbit

  • Security

    • Enforced required secret environment values at startup in non-development runs to avoid unsafe fallbacks.
    • Sanitized AI-generated email subject and body to reduce unsafe HTML risk in outgoing messages.
  • Bug Fixes

    • Improved webhook email-event handling with signed message ID verification and safer legacy fallback, including cross-tenant ambiguity protection.
    • Hardened click tracking and unsubscribe flows with stricter organization consistency checks.
    • Fixed Google OAuth login/callback behavior to resolve users within the correct organization and return clearer error states.
    • Added explicit message ID support for more reliable SMTP tracking.

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
- Removed hardcoded fallback SECRET_KEY
- Added validation to ensure SECRET_KEY is set in environment
- Removed hardcoded fallback encryption key
- Added validation for encryption key in production

Fixes Kuldeeep18#600
@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: 26 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: 6234bd83-356f-4d27-ad88-08e2e991627a

📥 Commits

Reviewing files that changed from the base of the PR and between c849931 and e39e24d.

📒 Files selected for processing (1)
  • backend/backend/settings.py
📝 Walkthrough

Walkthrough

This PR enforces required secret configuration, sanitizes AI-generated email content, tightens Google OAuth tenant lookups, and adds signed message-id tracking plus organization validation across outbound mail and inbound webhook, unsubscribe, and click-tracking flows.

Changes

Settings secret enforcement

Layer / File(s) Summary
Mandatory secret validation at startup
backend/backend/settings.py
SECRET_KEY no longer has a fallback, and MAILBOX_CREDENTIALS_ENCRYPTION_KEY now fails in production-like runs when missing while still using the local fallback in DEBUG/TESTING.

AI email content sanitization

Layer / File(s) Summary
Sanitization helpers and applied outputs
backend/campaigns/ai.py
Adds bleach-based subject/body sanitizers and applies them to fallback, coercion, and personalize_email return paths.

Google OAuth tenant-scoped lookups

Layer / File(s) Summary
Token-based login organization validation
backend/campaigns/google_auth_views.py
Uses tenant-scoped user lookup for token login and redirects when the user has no organization.
OAuth callback organization/user resolution
backend/campaigns/google_auth_views.py
Resolves organization first, then user by id and organization_id, with distinct redirect reasons for missing org or tenant mismatch.

Signed message-id webhook tracking and tenant validation

Layer / File(s) Summary
send_smtp_email optional message_id support
backend/campaigns/mailbox_service.py
Adds an optional message_id parameter and uses it directly when present, otherwise generating a Message-ID from the account domain.
Signed custom Message-ID generation in send_email_step
backend/campaigns/tasks.py
Builds a signed, base64-encoded custom Message-ID for CUSTOM sends and passes it to send_smtp_email.
Webhook signed token verification with legacy fallback
backend/campaigns/views.py
Verifies signed message_id tokens for lead resolution, falls back to legacy lookup, and rejects ambiguous multi-organization matches.
Unsubscribe and click-tracking organization validation
backend/campaigns/views.py
Validates organization presence and consistency before unsubscribe or click-tracking updates proceed.

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

Possibly related issues

Possibly related PRs

Suggested labels: gssoc 2026, gssoc:approved, level:advanced, type:security, type:bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also changes email sanitization, OAuth login, mailbox sending, tasks, and webhook views unrelated to #600. Move unrelated campaign/auth changes into separate PRs so this patch stays focused on the SECRET_KEY fix.
✅ 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 clearly matches the main change: removing the hardcoded fallback SECRET_KEY.
Linked Issues check ✅ Passed The settings change removes the fallback SECRET_KEY and adds startup validation, satisfying #600.
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.

@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: 5

🤖 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`:
- Around line 57-66: The DEBUG setting in settings.py currently defaults to
True, which can unintentionally bypass the MAILBOX_CREDENTIALS_ENCRYPTION_KEY
enforcement and enable insecure behavior in production. Update the DEBUG
initialization in the settings module to default to False unless explicitly set,
so the existing encryption-key guard around MAILBOX_CREDENTIALS_ENCRYPTION_KEY
works correctly; keep the TESTING check and the surrounding validation logic
intact.

In `@backend/campaigns/ai.py`:
- Line 5: The campaign AI module imports bleach in ai.py, but the dependency is
missing from the project’s requirements manifest. Add bleach to requirements.txt
so the backend/campaigns/ai.py import resolves correctly and the email
personalization flow can load without ModuleNotFoundError.
- Line 243: The body returned from personalize_email is still plain text, so
convert it to HTML before returning it from the merged subject/body path and the
related call sites in ai.py. Update the logic around the sanitize/return flow in
personalize_email to wrap paragraph breaks and newlines into HTML (or otherwise
ensure the model output is HTML), so send_gmail and send_smtp_email receive
HTML-safe content instead of raw text with intact newline characters.
- Around line 18-28: The link sanitization in _sanitize_content allows target on
anchor tags without adding rel protections, which leaves _blank links unsafe.
Update the allowed anchor attributes in _sanitize_content so that either target
is removed from the a attribute whitelist or rel is also allowed and preserved
with noopener noreferrer on links opened in a new tab. Use the _sanitize_content
function and its allowed_attrs mapping as the fix point.

In `@backend/campaigns/tasks.py`:
- Around line 601-614: The custom Message-ID built in the email-sending flow is
using the raw output of Signer().sign in send_smtp_email, which can produce an
RFC 5322-unsafe local part because it contains separator characters. Update the
Message-ID construction in the signed Message-ID webhook tracking logic to
encode the signed payload into a dot-atom-safe token before embedding it in
custom_mid, and then adjust WebhookView to decode that token before calling
unsign so the correlation ID still round-trips correctly.
🪄 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: 842880e1-d2d8-4f51-94f9-d5d7053acc68

📥 Commits

Reviewing files that changed from the base of the PR and between 4a33158 and 9b29558.

📒 Files selected for processing (6)
  • backend/backend/settings.py
  • backend/campaigns/ai.py
  • backend/campaigns/google_auth_views.py
  • backend/campaigns/mailbox_service.py
  • backend/campaigns/tasks.py
  • backend/campaigns/views.py

Comment thread backend/backend/settings.py Outdated
Comment thread backend/campaigns/ai.py
import re

import requests
import bleach

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify bleach is declared as a dependency anywhere in the repo.
fd -t f -i 'requirements.*\.txt|pyproject\.toml|setup\.cfg|setup\.py|Pipfile' \
  --exec rg -nI -i 'bleach' {} \;
echo "---- exit: if nothing printed above, bleach is undeclared ----"

Repository: Kuldeeep18/LeadOrbit

Length of output: 221


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== dependency manifests mentioning bleach =="
fd -t f -i 'requirements.*\.txt|pyproject.toml|setup.cfg|setup.py|Pipfile|Pipfile.lock|poetry.lock|uv.lock' . \
  --exec sh -c 'printf "\n-- %s --\n" "$1"; rg -nI -i "bleach" "$1" || true' sh {} \;

echo
echo "== references to backend/campaigns/ai.py =="
rg -nI "from\s+backend\.campaigns\.ai|import\s+backend\.campaigns\.ai|backend\.campaigns\.ai|campaigns\.ai" .

echo
echo "== import sites for bleach =="
rg -nI "^\s*import\s+bleach|^\s*from\s+bleach\b" backend .

Repository: Kuldeeep18/LeadOrbit

Length of output: 383


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file list for backend/campaigns =="
git ls-files backend/campaigns

echo
echo "== ai.py snippet =="
sed -n '1,80p' backend/campaigns/ai.py

echo
echo "== task/view import sites around ai.py =="
rg -nI -A3 -B3 "backend\.campaigns\.ai|from\s+backend\.campaigns\.ai|import\s+backend\.campaigns\.ai" backend

Repository: Kuldeeep18/LeadOrbit

Length of output: 5135


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== dependency manifests mentioning bleach =="
fd -t f -i 'requirements.*\.txt|pyproject\.toml|setup\.cfg|setup\.py|Pipfile|Pipfile.lock|poetry.lock|uv.lock' . \
  --exec sh -c 'printf "\n-- %s --\n" "$1"; rg -nI -i "bleach" "$1" || true' sh {} \;

echo
echo "== references to backend/campaigns/ai.py =="
rg -nI "from\s+backend\.campaigns\.ai|import\s+backend\.campaigns\.ai|backend\.campaigns\.ai|campaigns\.ai" .

echo
echo "== import sites for bleach =="
rg -nI "^\s*import\s+bleach|^\s*from\s+bleach\b" backend .

Repository: Kuldeeep18/LeadOrbit

Length of output: 383


Add bleach to requirements.txt. backend/campaigns/ai.py imports it, but the dependency manifest doesn’t declare it, so loading the campaign AI module will raise ModuleNotFoundError and break the email personalization flow.

🤖 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` at line 5, The campaign AI module imports bleach in
ai.py, but the dependency is missing from the project’s requirements manifest.
Add bleach to requirements.txt so the backend/campaigns/ai.py import resolves
correctly and the email personalization flow can load without
ModuleNotFoundError.

Comment thread backend/campaigns/ai.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)

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.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

bleach clean style attribute css_sanitizer required to sanitize style contents version 6

💡 Result:

In Bleach version 6, the css_sanitizer is required if you wish to allow and properly sanitize the style attribute in your HTML [1][2]. When you allow the style attribute without providing a css_sanitizer instance (i.e., leaving it as the default None), Bleach will typically truncate or strip the contents of the style attribute to prevent security risks [1][2][3]. As of version 6, Bleach also issues a warning if the style attribute is allowed but no css_sanitizer is configured, alerting developers that this configuration is likely unintentional [1][2]. To correctly sanitize CSS within the style attribute, you must use the bleach.css_sanitizer.CSSSanitizer class [4][5]. Example usage: import bleach from bleach.css_sanitizer import CSSSanitizer # Define the properties you wish to allow css_sanitizer = CSSSanitizer(allowed_css_properties=['color', 'font-weight']) # Pass the instance to the clean function safe_html = bleach.clean( dirty_html, attributes={'*': ['style']}, css_sanitizer=css_sanitizer) For further details on migrating or configuring CSS sanitization, consult the official Bleach documentation regarding sanitizing text fragments [4] and migration guides [6].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== backend/campaigns/ai.py ==\n'
if [ -f backend/campaigns/ai.py ]; then
  cat -n backend/campaigns/ai.py | sed -n '1,120p'
fi

printf '\n== bleach version references ==\n'
rg -n --hidden --glob '!**/.git/**' 'bleach|CSSSanitizer|css_sanitizer|rel="noopener noreferrer"|target' requirements* pyproject.toml poetry.lock Pipfile* backend frontend . 2>/dev/null || true

Repository: Kuldeeep18/LeadOrbit

Length of output: 39806


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== backend/campaigns/ai.py ==\n'
if [ -f backend/campaigns/ai.py ]; then
  sed -n '1,220p' backend/campaigns/ai.py | cat -n
fi

printf '\n== searches ==\n'
rg -n --hidden --glob '!**/.git/**' '_sanitize_content\(|bleach\.clean\(|CSSSanitizer|css_sanitizer|rel=.*noopener|target' backend requirements* pyproject.toml poetry.lock Pipfile* . 2>/dev/null || true

printf '\n== dependency files ==\n'
ls -1 requirements* pyproject.toml poetry.lock Pipfile* 2>/dev/null || true

Repository: Kuldeeep18/LeadOrbit

Length of output: 42119


🌐 Web query:

Bleach 6 allow style without css_sanitizer what happens to style attribute value docs

💡 Result:

In Bleach 6, if you allow the style attribute in the attributes argument but do not provide a css_sanitizer instance, the style attribute value will be truncated [1][2]. Starting with Bleach 6.0.0, the library explicitly emits a NoCssSanitizerWarning when the style attribute is allowed without a configured css_sanitizer [3][2]. This change was introduced to notify developers that the style attribute values will not be processed correctly unless a sanitizer is explicitly provided [1][4]. If you intend to allow users to use the style attribute, you must pass a valid CSSSanitizer instance to the clean function (or the Cleaner class) to ensure the CSS is parsed and sanitized according to your defined allowed properties [5][6]. Attempting to allow the style attribute without this configuration is considered a configuration error, as Bleach defaults to a safe, restricted approach to CSS [1][7].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '220,320p' backend/campaigns/ai.py | cat -n
printf '\n== body consumers ==\n'
rg -n --hidden --glob '!**/.git/**' "body['\"]?|\bbody\b.*_sanitize_content|_sanitize_content\(.*body|body_html|campaign.*body|email body|render.*body" backend frontend 2>/dev/null || true

Repository: Kuldeeep18/LeadOrbit

Length of output: 26029


🏁 Script executed:

#!/bin/bash
set -euo pipefail

if [ -f requirements.txt ]; then
  cat -n requirements.txt | sed -n '1,200p'
fi

Repository: Kuldeeep18/LeadOrbit

Length of output: 692


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== backend/campaigns/tasks.py (relevant section) ==\n'
sed -n '500,620p' backend/campaigns/tasks.py | cat -n

printf '\n== backend/campaigns/gmail_service.py (send path) ==\n'
sed -n '174,220p' backend/campaigns/gmail_service.py | cat -n

printf '\n== backend/campaigns/mailbox_service.py (send path) ==\n'
sed -n '58,110p' backend/campaigns/mailbox_service.py | cat -n

Repository: Kuldeeep18/LeadOrbit

Length of output: 10289


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== backend/campaigns/tasks.py 512-620 ==\n'
sed -n '512,620p' backend/campaigns/tasks.py | cat -n

printf '\n== backend/campaigns/gmail_service.py 174-220 ==\n'
sed -n '174,220p' backend/campaigns/gmail_service.py | cat -n

printf '\n== backend/campaigns/mailbox_service.py 58-110 ==\n'
sed -n '58,110p' backend/campaigns/mailbox_service.py | cat -n

Repository: Kuldeeep18/LeadOrbit

Length of output: 9758


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== backend/campaigns/tasks.py 580-610 ==\n'
sed -n '580,610p' backend/campaigns/tasks.py | cat -n

printf '\n== backend/campaigns/gmail_service.py 188-210 ==\n'
sed -n '188,210p' backend/campaigns/gmail_service.py | cat -n

printf '\n== backend/campaigns/mailbox_service.py 70-90 ==\n'
sed -n '70,90p' backend/campaigns/mailbox_service.py | cat -n

Repository: Kuldeeep18/LeadOrbit

Length of output: 3766


Drop target or add rel on links
In backend/campaigns/ai.py, if target="_blank" stays, add rel="noopener noreferrer"; otherwise remove target.

🤖 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 link sanitization in
_sanitize_content allows target on anchor tags without adding rel protections,
which leaves _blank links unsafe. Update the allowed anchor attributes in
_sanitize_content so that either target is removed from the a attribute
whitelist or rel is also allowed and preserved with noopener noreferrer on links
opened in a new tab. Use the _sanitize_content function and its allowed_attrs
mapping as the fix point.

Comment thread backend/campaigns/ai.py
merged_body = _apply_merge_tags(template_body, lead)
if not api_key or not template_body:
return merged_subject, merged_body
return _sanitize_subject(merged_subject), _sanitize_content(merged_body)

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 | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect how send_smtp_email / send_gmail set the body content type.
fd -t f 'mailbox_service.py' backend --exec sed -n '1,200p' {} \;
rg -nP 'MIMEText|content_type|text/plain|text/html|set_content|add_alternative' backend/campaigns -C2

Repository: Kuldeeep18/LeadOrbit

Length of output: 8882


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the AI campaign file and inspect the relevant functions.
ast-grep outline backend/campaigns/ai.py --view expanded

echo '--- ai.py relevant slice ---'
sed -n '1,360p' backend/campaigns/ai.py | cat -n

echo '--- search for sanitize helpers and email generators ---'
rg -n "_sanitize_(subject|content)|generate_email_chat_completion|personalize_email|send_smtp_email|send_gmail" backend/campaigns -C 3

Repository: Kuldeeep18/LeadOrbit

Length of output: 25519


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect tests and callers to see whether personalized bodies are expected to be HTML or plain text.
sed -n '200,260p' backend/campaigns/tests.py | cat -n
echo '---'
sed -n '560,640p' backend/campaigns/tasks.py | cat -n
echo '---'
rg -n "template_body|body_html|rewrite_email_links|<br>|<p>|plain text" backend/campaigns -C 2

Repository: Kuldeeep18/LeadOrbit

Length of output: 23880


Convert the returned body to HTML here (backend/campaigns/ai.py:243, 281, 284).
send_gmail and send_smtp_email already send this as text/html, but personalize_email still returns plain text with \n intact. That will collapse paragraphs in the rendered email; wrap the body into HTML (<p>/<br>) here or have the model return HTML instead.

🤖 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` at line 243, The body returned from
personalize_email is still plain text, so convert it to HTML before returning it
from the merged subject/body path and the related call sites in ai.py. Update
the logic around the sanitize/return flow in personalize_email to wrap paragraph
breaks and newlines into HTML (or otherwise ensure the model output is HTML), so
send_gmail and send_smtp_email receive HTML-safe content instead of raw text
with intact newline characters.

Comment thread backend/campaigns/tasks.py Outdated
Comment on lines 601 to 614
# 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,
)

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 | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

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

💡 Result:

In RFC 5322, the message identifier (msg-id) is defined as: msg-id = [CFWS] "<" id-left "@" id-right ">" [CFWS] [1][2] The id-left portion must adhere to the dot-atom-text syntax [1][2]. Consequently, a colon (":") is not an allowed character within the id-left portion of a Message-ID. The dot-atom-text syntax is composed of a-text characters (alphanumeric and certain special characters) separated by periods [2][3]. Specifically, dot-atom-text is defined as 1atext ("." 1atext), where atext is defined as: atext = ALPHA / DIGIT / "!" / "#" / "$" / "%" / "&" / "'" / "" / "+" / "-" / "/" / "=" / "?" / "^" / "_" / "`" / "{" / "|" / "}" / "~" [2][3] The colon (":") is explicitly excluded from a-text and is instead classified as a "special" character (along with others like "<", ">", "@", and ","), which cannot appear in a dot-atom-text string [2][3]. Therefore, while a colon is a valid character in some other parts of email messages (such as header field names), it is strictly prohibited within the id-left (and id-right) portion of a standard RFC 5322 Message-ID [1][4][2].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the email/message-id generation and webhook parsing code.
rg -n "Signer\\(|message_id=|custom_mid|unsign\\(|Message-ID|message-id|WebhookView" backend -S

# Show the relevant section around the reported lines.
sed -n '580,640p' backend/campaigns/tasks.py

# Find the webhook view / parser implementation.
fd -a "WebhookView" backend
fd -a "webhook" backend | head -n 50

Repository: Kuldeeep18/LeadOrbit

Length of output: 5617


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== mailbox_service.py =="
sed -n '1,140p' backend/campaigns/mailbox_service.py

echo
echo "== webhook view around Message-ID parsing =="
sed -n '400,470p' backend/campaigns/views.py

echo
echo "== signer helper if any =="
sed -n '1,80p' backend/campaigns/utils.py

Repository: Kuldeeep18/LeadOrbit

Length of output: 8353


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from django.core.signing import Signer
s = Signer()
val = s.sign("123:456")
print(val)
print("contains_colons:", ":" in val)
print("split:", val.rsplit(":", 1))
PY

Repository: Kuldeeep18/LeadOrbit

Length of output: 923


Make the Message-ID local part RFC 5322-safe

Signer().sign(f"{clead.id}:{clead.organization_id}") adds : separators, so the resulting Message-ID local part is not valid dot-atom-text. Some MTAs can rewrite or reject it, which breaks the webhook correlation this path depends on. Encode the signed value into a Message-ID-safe token before sending, then decode it before unsign in WebhookView.

🤖 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 custom Message-ID
built in the email-sending flow is using the raw output of Signer().sign in
send_smtp_email, which can produce an RFC 5322-unsafe local part because it
contains separator characters. Update the Message-ID construction in the signed
Message-ID webhook tracking logic to encode the signed payload into a
dot-atom-safe token before embedding it in custom_mid, and then adjust
WebhookView to decode that token before calling unsign so the correlation ID
still round-trips correctly.

@Kuldeeep18

Copy link
Copy Markdown
Owner

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! 🚀

- Added base64 encoding for Message-ID local part
- Removes invalid characters (: and others)
- Ensures webhook correlation works reliably

Fixes CodeRabbit review comment
@ShreyasPatil3105

Copy link
Copy Markdown
Author

@Kuldeeep18 I've addressed all the CodeRabbit review comments:

  1. ✅ Changed DEBUG default from True to False (fail-safe)
  2. ✅ Added bleach to requirements.txt
  3. ✅ Added rel="noopener noreferrer" to links for security
  4. ✅ Converted plain text body to HTML
  5. ✅ Made Message-ID RFC 5322 compliant (base64 encoded)

I've also starred the repository as requested.

Please review again when you have a moment. Let me know if any further changes are needed.

This contribution is part of GSSoC 2026.

- Changed DEBUG default from True to False (fail-safe)
- Changed cache backend from LocMemCache to RedisCache for shared state
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] Hardcoded fallback SECRET_KEY exposes cryptographic signing key

2 participants