✨(integrations) add outgoing webhooks for every incoming email - #691
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds webhook channel support end to end, including secret rotation, API and admin changes, a new outbound webhook dispatch pipeline, inbound message storage and delivery updates for internal mail, and matching frontend, localization, and test coverage. ChangesWebhook channels, inbound pipeline, and frontend integration
Sequence DiagramsequenceDiagram
participant deliver_inbound_message
participant process_inbound_message_task
participant build_inbound_pipeline
participant UserWebhookStep
participant SSRFSafeSession
participant WebhookReceiver
deliver_inbound_message->>process_inbound_message_task: queue InboundMessage (raw_data or blob)
process_inbound_message_task->>build_inbound_pipeline: build steps from InboundContext
build_inbound_pipeline-->>process_inbound_message_task: ordered steps
process_inbound_message_task->>UserWebhookStep: execute phase step
UserWebhookStep->>SSRFSafeSession: POST signed payload
SSRFSafeSession->>WebhookReceiver: X-StMsg-* + eml/jmap body
WebhookReceiver-->>SSRFSafeSession: response
SSRFSafeSession-->>UserWebhookStep: classified result
UserWebhookStep-->>process_inbound_message_task: Decision + side effects
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/backend/core/admin.py (1)
583-619:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWebhook secret rotation is still blocked in Django admin.
This view now delegates to
Channel.rotate_secret(), but Line 602 still rejects every non-api_keychannel and the response stays API-key-specific. That leaves webhook channels with no admin path to rotate their shared secret even though this PR exposes them as mailbox-admin integrations.Suggested direction
- def regenerate_api_key_view(self, request, object_id): - """Regenerate the api_key secret on an api_key channel. + def regenerate_secret_view(self, request, object_id): + """Regenerate the shared secret on an api_key or webhook channel. @@ - if channel.type != ChannelTypes.API_KEY: + if channel.type not in {ChannelTypes.API_KEY, ChannelTypes.WEBHOOK}: messages.error( - request, "Only api_key channels can have their secret regenerated." + request, "Only api_key and webhook channels can have their secret regenerated." ) return redirect("..") @@ - "title": "New api_key generated", - "api_key": plaintext, + "title": "New secret generated", + "secret": plaintext, } - return TemplateResponse( - request, "admin/core/channel/regenerated_api_key.html", context - ) + return TemplateResponse(request, "admin/core/channel/regenerated_secret.html", context)🤖 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 `@src/backend/core/admin.py` around lines 583 - 619, The regenerate_api_key_view currently blocks non-API_KEY channels — update regenerate_api_key_view to allow rotating secrets for webhook channels as well by permitting ChannelTypes.WEBHOOK in the type check (use ChannelTypes.API_KEY or ChannelTypes.WEBHOOK), call channel.rotate_secret() for either type, and build a context/title that is generic or conditional (e.g., use a type-aware title like "New secret generated" and include channel.type in the context). Also update the error message when the type is unsupported and ensure the TemplateResponse/template ("admin/core/channel/regenerated_api_key.html") or its context keys (like "api_key") are renamed/kept consistent (e.g., use "secret" instead of "api_key") so webhook secrets display correctly. Ensure messages and redirects remain unchanged for missing channel.src/backend/core/api/serializers.py (1)
2196-2204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-HTTPS webhook targets outside local development.
Allowing
http://here means the dispatcher can send full inbound message payloads and the webhook credential over cleartext transport. Please require HTTPS, with an explicit localhost/DEBUG escape hatch if dev still needs plain HTTP.🤖 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 `@src/backend/core/api/serializers.py` around lines 2196 - 2204, Change the webhook URL validation to reject plain HTTP except for a local-dev escape hatch: in the serializer validation where settings_data and url are checked (the block validating settings.url), require url.startswith("https://") instead of allowing "http://", but allow "http://" when Django settings.DEBUG is True or the host is localhost/127.0.0.1 (and optionally port), and keep raising serializers.ValidationError with a clear message when non-HTTPS targets are provided; use Django's settings.DEBUG and parse the URL host to detect localhost to implement the exception.src/backend/core/api/viewsets/channel.py (1)
127-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the one-time credential fields on
create().
create()now returnsapi_key/webhook_secret/webhook_api_key/password, but the 201 schema still resolves toChannelSerializer. That drops the new fields from generated clients, which is a contract break for the webhook/API-key creation flows.🤖 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 `@src/backend/core/api/viewsets/channel.py` around lines 127 - 149, The create() endpoint surfaces one-time fields (api_key, webhook_secret, webhook_api_key, password) but the response schema still uses ChannelSerializer so generated clients miss those fields; update the view to return a creation-aware serializer/response schema: either add a CreateChannelSerializer that includes these one-time fields (or mark them appropriately on ChannelSerializer as create-only/write_only and included in .data), and make get_serializer_class() return that serializer when action == "create", or annotate the create() method with an explicit response schema (e.g., via your OpenAPI helper like `@extend_schema`) that includes api_key, webhook_secret, webhook_api_key, and password so the 201 response contract matches what create() actually returns. Ensure you reference create(), get_serializer_class()/get_serializer(), and ChannelSerializer when applying the change.
🤖 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 `@docs/webhooks.md`:
- Line 69: The docs entry for the `blocking` webhook field is inaccurate: update
the description for the `blocking` column in docs/webhooks.md to replace "If
true, non-200 (or any failure) drops the inbound message." with a concise
statement like "If true, the webhook response determines delivery (see Response
contract below)." Also ensure the Response contract section (referenced as the
webhook response decision matrix) clearly lists the behavior for 2xx with
{"action":"drop"} → DROP, 2xx without drop → CONTINUE, 4xx → DROP, 408/429/5xx
and timeouts/connection errors → RETRY so readers can find the nuanced behavior.
- Around line 312-330: The fenced code block that begins with "POST /inbox-hook
HTTP/1.1" is missing a language identifier; update that opening triple-backtick
to include the "http" language (i.e., change ``` to ```http) so the HTTP request
example is rendered with proper syntax highlighting and rendering.
- Line 152: Remove the parenthetical narrative from the table row that reads
"Connection error, timeout, DNS, etc. | RETRY | Transient. (This was the
original bug: timeouts used to silently lose mail.)" — edit the third column for
that row to simply "Transient." (keep the first column "Connection error,
timeout, DNS, etc." and the second column "RETRY" unchanged) so the API docs
contain only neutral, permanent documentation text.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 155-183: The function _classify_response_body ignores an
"action":"retry" directive from webhook JSON; update _classify_response_body to
recognize action strings case-insensitively and map "retry" to result.decision =
Decision.RETRY (similar to how "drop" is handled), returning the _HttpResult
with the RETRY decision so downstream UserWebhookStep/dispatch logic re-queues
the inbound task; keep using _HttpResult and Decision enums already referenced
in the function.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 149-176: The code currently only handles created credentials when
newChannel.status === 201, which misses other successful 2xx responses; update
the guard to accept any successful status (e.g., newChannel.status < 300) or
otherwise check the resolved success instead of exact 201 so the branches that
call setCreatedCredential(...) or onSuccess(...) run for all 2xx responses;
locate the block referencing newChannel.status, newChannel.data,
setCreatedCredential, and onSuccess in webhook-integration-form.tsx and replace
the strict equality check with a generalized success check (status < 300) so
credentials are surfaced and onSuccess is invoked for any successful create
response.
- Around line 45-58: The Zod validators in createFormSchema are using the
deprecated `{ message: ... }` option; update those calls to use Zod 4's `{
error: ... }` shape (e.g., replace min(..., { message: t("...") }) with min(...,
{ error: t("...") }) and regex(..., { message: t("...") }) with regex(..., {
error: t("...") })). Search the createFormSchema function for any other
validators using `{ message: ... }` and replace them similarly so all Zod error
objects use `error` instead of `message`.
---
Outside diff comments:
In `@src/backend/core/admin.py`:
- Around line 583-619: The regenerate_api_key_view currently blocks non-API_KEY
channels — update regenerate_api_key_view to allow rotating secrets for webhook
channels as well by permitting ChannelTypes.WEBHOOK in the type check (use
ChannelTypes.API_KEY or ChannelTypes.WEBHOOK), call channel.rotate_secret() for
either type, and build a context/title that is generic or conditional (e.g., use
a type-aware title like "New secret generated" and include channel.type in the
context). Also update the error message when the type is unsupported and ensure
the TemplateResponse/template ("admin/core/channel/regenerated_api_key.html") or
its context keys (like "api_key") are renamed/kept consistent (e.g., use
"secret" instead of "api_key") so webhook secrets display correctly. Ensure
messages and redirects remain unchanged for missing channel.
In `@src/backend/core/api/serializers.py`:
- Around line 2196-2204: Change the webhook URL validation to reject plain HTTP
except for a local-dev escape hatch: in the serializer validation where
settings_data and url are checked (the block validating settings.url), require
url.startswith("https://") instead of allowing "http://", but allow "http://"
when Django settings.DEBUG is True or the host is localhost/127.0.0.1 (and
optionally port), and keep raising serializers.ValidationError with a clear
message when non-HTTPS targets are provided; use Django's settings.DEBUG and
parse the URL host to detect localhost to implement the exception.
In `@src/backend/core/api/viewsets/channel.py`:
- Around line 127-149: The create() endpoint surfaces one-time fields (api_key,
webhook_secret, webhook_api_key, password) but the response schema still uses
ChannelSerializer so generated clients miss those fields; update the view to
return a creation-aware serializer/response schema: either add a
CreateChannelSerializer that includes these one-time fields (or mark them
appropriately on ChannelSerializer as create-only/write_only and included in
.data), and make get_serializer_class() return that serializer when action ==
"create", or annotate the create() method with an explicit response schema
(e.g., via your OpenAPI helper like `@extend_schema`) that includes api_key,
webhook_secret, webhook_api_key, and password so the 201 response contract
matches what create() actually returns. Ensure you reference create(),
get_serializer_class()/get_serializer(), and ChannelSerializer when applying the
change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c8c113f-8e55-44d1-a991-a81cbbe7ad31
📒 Files selected for processing (24)
docs/webhooks.mdenv.d/development/backend.defaultssrc/backend/core/admin.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/factories.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/models.pysrc/backend/core/services/ssrf.pysrc/backend/core/services/thread_events.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound_auth.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/messages/settings.pysrc/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx
Comment on lines
+149
to
+176
| if (newChannel.status === 201) { | ||
| // Surface the freshly minted credential exactly | ||
| // once — the receiver needs this value to verify | ||
| // every webhook we send. The backend returns | ||
| // ``webhook_secret`` for auth_method=jwt and | ||
| // ``webhook_api_key`` for auth_method=api_key. | ||
| const payload = newChannel.data as Record<string, unknown>; | ||
| const secret = payload.webhook_secret as | ||
| | string | ||
| | undefined; | ||
| const apiKey = payload.webhook_api_key as | ||
| | string | ||
| | undefined; | ||
| if (secret) { | ||
| setCreatedCredential({ | ||
| label: t("Webhook signing secret"), | ||
| value: secret, | ||
| }); | ||
| } else if (apiKey) { | ||
| setCreatedCredential({ | ||
| label: t("Webhook API key"), | ||
| value: apiKey, | ||
| }); | ||
| } else { | ||
| onSuccess(newChannel.data); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
status === 201 guard can strand a successful create.
If the create endpoint returns a 2xx other than 201 (e.g. 200), the success toast at Lines 143-147 still fires but none of the credential branches run and onSuccess is never called — the modal neither surfaces the credential nor advances to edit mode. Consider gating on status < 300 (or relying on the resolved promise) rather than an exact 201.
🤖 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
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`
around lines 149 - 176, The code currently only handles created credentials when
newChannel.status === 201, which misses other successful 2xx responses; update
the guard to accept any successful status (e.g., newChannel.status < 300) or
otherwise check the resolved success instead of exact 201 so the branches that
call setCreatedCredential(...) or onSuccess(...) run for all 2xx responses;
locate the block referencing newChannel.status, newChannel.data,
setCreatedCredential, and onSuccess in webhook-integration-form.tsx and replace
the strict equality check with a generalized success check (status < 300) so
credentials are surfaced and onSuccess is invoked for any successful create
response.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sylvinus
force-pushed
the
outbound_webhooks
branch
from
June 16, 2026 09:07
f23003a to
9c775e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/models.py (1)
3262-3267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor the documented user fallback when the contact name is blank.
Line 3262 picks
mailbox.contact.namewhenever a contact exists, so an empty contact name suppressesuser.full_nameeven though the docstring says the user is the fallback forname.Proposed fix
context = {} + mailbox_name = ( + mailbox.contact.name if mailbox and mailbox.contact else None + ) + user_name = getattr(user, "full_name", None) if user else "" context["name"] = ( - mailbox.contact.name - if mailbox and mailbox.contact - else (getattr(user, "full_name", None) if user else "") - ) or "" - context["user_name"] = (getattr(user, "full_name", None) or "") if user else "" + mailbox_name or user_name or "" + ) + context["user_name"] = user_name or ""🤖 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 `@src/backend/core/models.py` around lines 3262 - 3267, The context["name"] assignment currently checks only if mailbox and mailbox.contact exist, but doesn't verify that mailbox.contact.name is actually non-empty. This means an empty or falsy contact name prevents the fallback to user.full_name. Fix this by also checking that mailbox.contact.name is truthy (not just that the contact exists) before using it; if the contact name is empty, None, or falsy, fall back to user.full_name as documented.src/backend/core/api/serializers.py (1)
2224-2235:⚠️ Potential issue | 🟠 MajorValidate the parsed webhook URL, including hostname.
https:///missing-hostpasses the current validation because the scheme ishttpsand thehostnameis empty, allowing a malformed URL to be saved and later fail at dispatch time. Additionally, the case-sensitivestartswithcheck rejects valid uppercase schemes likeHTTPS://beforeurlparsenormalizes them.Replace the scheme validation with a check against the parsed URL's normalized scheme and add explicit hostname validation:
Proposed fix
- if not url.startswith(("http://", "https://")): - raise serializers.ValidationError( - {"settings": "webhook settings.url must be http:// or https://"} - ) # Each POST carries the HMAC/JWT signing material in its headers, # so plaintext http would leak credentials in transit. Require # https except for a local-dev escape hatch: a loopback host, or # DEBUG (where operators routinely point at a local receiver). parsed_url = urlparse(url) host = (parsed_url.hostname or "").lower() + if parsed_url.scheme not in ("http", "https") or not host: + raise serializers.ValidationError( + { + "settings": ( + "webhook settings.url must be an absolute http:// " + "or https:// URL with a hostname." + ) + } + ) is_loopback = host in ("localhost", "127.0.0.1", "::1")🤖 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 `@src/backend/core/api/serializers.py` around lines 2224 - 2235, The webhook URL validation has two issues: the case-sensitive startswith check rejects valid uppercase schemes like HTTPS:// that urlparse would normalize, and malformed URLs like https:///missing-host pass validation because the hostname check only occurs later and doesn't reject empty hostnames. Replace the startswith scheme validation with a check against parsed_url.scheme (which is normalized by urlparse), and add explicit validation that parsed_url.hostname is not empty or None before proceeding with the loopback and DEBUG checks. This ensures the hostname exists and the scheme is properly normalized.
🤖 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 `@src/backend/core/mda/autoreply.py`:
- Around line 331-333: The code at line 332 assumes raw_body is always a string
before encoding to UTF-8, but template.raw_body may already be a dict or other
JSON-like object, which causes an error. Before calling .encode("utf-8") on
raw_body, add type checking to serialize it appropriately: if raw_body is a dict
or non-string type, convert it to a JSON string using json.dumps() first; if
it's already a string, use it directly. Then encode the resulting string to
UTF-8 bytes.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 651-692: The code logs full webhook URLs in multiple logger calls
(logger.warning, logger.exception, and logger.info) which can expose sensitive
receiver tokens embedded in the path, query string, or userinfo. Create a helper
function to sanitize the URL by extracting only the scheme, host, and port
components, then replace all instances where url is logged as url=%s with the
sanitized version throughout the exception handlers and response processing
logic to ensure no secrets are leaked in logs.
- Around line 475-482: The jmap_without_body payload stripping logic is
currently removing multiple body-related fields (textBody, htmlBody, bodyValues,
bodyStructure, attachments) but is missing the preview field, which is derived
from the message body and should also be stripped to maintain the
notification-only privacy contract. Add "preview" to the tuple of keys in the
for loop that is popping body-bearing fields from the email object to ensure
this derived content field is also removed.
In `@src/backend/core/mda/inbound_create.py`:
- Around line 278-280: The logger.error call is logging raw email addresses in
the recipient_email variable, which exposes personally identifiable information
(PII). Replace the direct logging of recipient_email and sender_email variables
with either mailbox IDs, message IDs, or hashed/redacted versions of the email
addresses instead. This same issue appears in multiple locations within the file
where similar logging statements reference raw email variables, so update all
occurrences to remove the sensitive email address data from the logs.
- Around line 81-84: The regex pattern in the subject canonicalization function
requires whitespace after the colon (using \s+), which prevents matching common
subject patterns like "Re:Subject" without a space. Fix this by changing the
`\s+` quantifier that follows the colon in the regex pattern to `\s*` (zero or
more whitespace characters instead of one or more) so that both "Re: Subject"
and "Re:Subject" formats are properly normalized and thread matching works
correctly.
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 440-463: The set comprehension used to create target_emails loses
the ordering of input emails, resulting in non-deterministic assignment payload
ordering. Replace the set comprehension that creates target_emails with a
deduplication approach that preserves insertion order (such as using
dict.fromkeys()), ensuring that the order of emails in the input is maintained
when target_emails is used in subsequent filter operations and iterations
through the for loop that processes each email in target_emails.
In `@src/backend/core/tests/api/test_channels.py`:
- Around line 589-599: The test_create_minimal_webhook method does not verify
that webhook credentials are returned in the response, making it possible for
the API contract to break without test detection. Add an assertion to verify
that webhook_secret is present in the response for the JWT auth method case.
Additionally, create a separate test case for auth_method="api_key" that asserts
webhook_api_key is returned in the response. Finally, add test coverage for the
regenerate-secret endpoint (likely via a separate test method) that covers both
auth_method variants (JWT and api_key) to ensure credentials are properly
regenerated for each authentication type.
In `@src/backend/core/tests/mda/test_dispatch_webhooks.py`:
- Around line 1652-1688: The test_response_body_is_capped test does not enforce
that reading stops at the MAX_RESPONSE_BODY limit. Currently, the iter_content
mock can return all three oversize_chunk items without consequence, as the test
only verifies the final decision outcome and that close() was called. Replace
the iter_content mock with a generator that tracks consumed bytes and raises an
exception (such as AssertionError or a custom exception) once the cumulative
bytes read exceed MAX_RESPONSE_BODY, ensuring that any attempt by
dispatch_webhooks to read beyond the cap will cause the test to fail if the
capping logic is not working correctly.
---
Outside diff comments:
In `@src/backend/core/api/serializers.py`:
- Around line 2224-2235: The webhook URL validation has two issues: the
case-sensitive startswith check rejects valid uppercase schemes like HTTPS://
that urlparse would normalize, and malformed URLs like https:///missing-host
pass validation because the hostname check only occurs later and doesn't reject
empty hostnames. Replace the startswith scheme validation with a check against
parsed_url.scheme (which is normalized by urlparse), and add explicit validation
that parsed_url.hostname is not empty or None before proceeding with the
loopback and DEBUG checks. This ensures the hostname exists and the scheme is
properly normalized.
In `@src/backend/core/models.py`:
- Around line 3262-3267: The context["name"] assignment currently checks only if
mailbox and mailbox.contact exist, but doesn't verify that mailbox.contact.name
is actually non-empty. This means an empty or falsy contact name prevents the
fallback to user.full_name. Fix this by also checking that mailbox.contact.name
is truthy (not just that the contact exists) before using it; if the contact
name is empty, None, or falsy, fall back to user.full_name as documented.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64be5c53-0b29-4f46-96d7-e43edd25d08d
📒 Files selected for processing (25)
docs/webhooks.mdenv.d/development/backend.defaultssrc/backend/core/admin.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/factories.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/models.pysrc/backend/core/services/ssrf.pysrc/backend/core/services/thread_events.pysrc/backend/core/templates/admin/core/channel/regenerated_api_key.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound_auth.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/messages/settings.pysrc/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx
💤 Files with no reviewable changes (5)
- src/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsx
- src/backend/messages/settings.py
- src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsx
- src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx
- src/backend/core/tests/mda/test_spam_processing.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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 `@src/backend/core/api/openapi.json`:
- Around line 7602-7617: The required fields array in the ChannelCreateResponse
schema includes all credential fields unconditionally, but the create endpoints
only return the credential matching the specific channel type and authentication
method. Remove the type-specific credential fields (such as webhook_api_key,
webhook_secret, password, api_key, and any others that are mutually exclusive
based on credential type) from the required array in the ChannelCreateResponse
schema definition so that only universally present fields like id, created_at,
updated_at, user, mailbox, maildomain, name, scope_level, and type remain
required. This allows the schema to accurately represent that some credential
fields will intentionally be absent depending on which authentication method was
used for that specific credential.
In `@src/backend/core/tests/mda/test_dispatch_webhooks.py`:
- Around line 265-266: The assertions on lines 265 and 266 for
email["inReplyTo"] and email["references"] use `assert not` which accepts any
falsey value (None, "", [], etc.) instead of explicitly checking for the
JMAP-compliant empty list. Replace `assert not email["inReplyTo"]` with `assert
email["inReplyTo"] == []` and `assert not email["references"]` with `assert
email["references"] == []` to enforce the strict JMAP Id[] empty-list contract.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fd187ea-53f6-4dfd-b290-48edbb07461b
⛔ Files ignored due to path filters (5)
src/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_api_key_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**
📒 Files selected for processing (8)
src/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/mda/test_dispatch_webhooks.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/mda/inbound_tasks.py (1)
165-172:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse failures lack expiration — row retries indefinitely.
When
parse_emailfails, theInboundMessagerow is preserved for retry but never quarantine-expires. UnlikeDecision.RETRY(bounded byQUARANTINE_AFTER), a permanently unparseable message will retry every 5-minute sweep indefinitely, accumulating failed attempts.Consider either:
- Applying similar
QUARANTINE_AFTERlogic to parse failures (delete after 48h with a log)- Incrementing a retry counter and giving up after N attempts
🤖 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 `@src/backend/core/mda/inbound_tasks.py` around lines 165 - 172, The parse_email function failure handling in the InboundMessage processing block does not implement any expiration or retry limiting logic, causing permanently unparseable messages to retry indefinitely every 5 minutes. Add expiration handling by either implementing a QUARANTINE_AFTER timeout similar to Decision.RETRY logic that removes the row after a set duration (e.g., 48 hours) with appropriate logging, or add a retry counter field to InboundMessage that increments on each parse failure and stops retrying after reaching a maximum attempt threshold. Make sure this logic is applied in the same code block where parsed_email is None and before saving the InboundMessage with the error_message.src/backend/core/enums.py (1)
224-232:⚠️ Potential issue | 🟠 MajorBreaking change: webhook event names renamed without data migration.
The enum change from
MESSAGE_RECEIVED/MESSAGE_SENTtoMESSAGE_INBOUND/MESSAGE_OUTBOUNDbreaks existing webhook channels. The dispatcher indispatch_webhooks.pychecks whether"message.inbound"is in the channel'ssettings["events"]; channels with the old"message.received"or"message.sent"values will be silently skipped and no longer fire.If webhook channels with the old event names exist in the database, a data migration should update
Channel.settings["events"]to use the new values, or verify this is pre-production with no existing data.🤖 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 `@src/backend/core/enums.py` around lines 224 - 232, The WebhookEvents enum in this change renames event identifiers from MESSAGE_RECEIVED/MESSAGE_SENT to MESSAGE_INBOUND/MESSAGE_OUTBOUND, but existing webhook Channel records in the database still reference the old event names in their settings["events"] field. This will cause the dispatcher in dispatch_webhooks.py to silently skip channels with the old values when checking for message.inbound or message.outbound events. Create a Django data migration that updates all existing Channel.settings["events"] to replace the old event name strings (message.received, message.sent) with the new ones (message.inbound, message.outbound), or if this is a pre-production environment, verify and document that no production data with the old event names exists.
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx (1)
150-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
status === 201still creates a success-path gap.At Line 150, create success handling is gated to exactly
201, so any other successful 2xx response skips both credential display andonSuccess, leaving the modal flow incomplete.♻️ Proposed fix
- if (newChannel.status === 201) { + if (newChannel.status < 300) { // Surface the freshly minted credential exactly // once — the receiver needs this value to verify // every webhook we send. The backend returns // ``secret`` for auth_method=jwt and ``api_key`` for // auth_method=api_key. These one-time credentials are // create-only response fields, not part of the // generated ``Channel`` type — read them off an // index-signature view. const payload = newChannel.data as unknown as Record< string, unknown >; const secret = payload.secret as string | undefined; const apiKey = payload.api_key as string | undefined; if (secret) { setCreatedCredential({ label: t("Webhook signing secret"), value: secret, }); } else if (apiKey) { setCreatedCredential({ label: t("Webhook API key"), value: apiKey, }); } else { onSuccess(newChannel.data); } + } else { + onSuccess(newChannel.data); }Based on learnings: in TSX files using the centralized fetchAPI wrapper, avoid explicit status-based branching for success/error control flow after API calls and rely on thrown APIError for non-ok responses.
🤖 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 `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx` around lines 150 - 178, Remove the explicit `status === 201` check on the newChannel response in the webhook integration form's success handler. Since the centralized fetchAPI wrapper throws APIError for non-ok responses, any response that reaches this code path is already successful. Execute the credential extraction and credential/success handling logic unconditionally for all successful responses, eliminating the gap where other 2xx status codes would bypass both credential display and the onSuccess callback.Source: Learnings
🤖 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 `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 558-560: The comments in the webhook error handling section
contain outdated references to a 7-day retry budget, but the system has been
updated to use a 48-hour quarantine window model. Update the comment blocks
around line 560 and line 678 by replacing any mention of "7-day budget" with
"48-hour quarantine window" to ensure the documentation accurately reflects the
current retry and quarantine behavior of the pipeline.
- Around line 783-785: The current code at line 783 uses a fallback with `or`
that allows malformed non-list values (like strings) to pass the subsequent `in`
check on line 784, which could cause substring matching instead of list
membership checking and unintentionally enable webhook dispatch. Add an explicit
type validation after retrieving the events value to ensure it is a list, and
skip processing the current row with `continue` if events is not a list type,
ensuring the fail-closed behavior for invalid settings.
---
Outside diff comments:
In `@src/backend/core/enums.py`:
- Around line 224-232: The WebhookEvents enum in this change renames event
identifiers from MESSAGE_RECEIVED/MESSAGE_SENT to
MESSAGE_INBOUND/MESSAGE_OUTBOUND, but existing webhook Channel records in the
database still reference the old event names in their settings["events"] field.
This will cause the dispatcher in dispatch_webhooks.py to silently skip channels
with the old values when checking for message.inbound or message.outbound
events. Create a Django data migration that updates all existing
Channel.settings["events"] to replace the old event name strings
(message.received, message.sent) with the new ones (message.inbound,
message.outbound), or if this is a pre-production environment, verify and
document that no production data with the old event names exists.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 165-172: The parse_email function failure handling in the
InboundMessage processing block does not implement any expiration or retry
limiting logic, causing permanently unparseable messages to retry indefinitely
every 5 minutes. Add expiration handling by either implementing a
QUARANTINE_AFTER timeout similar to Decision.RETRY logic that removes the row
after a set duration (e.g., 48 hours) with appropriate logging, or add a retry
counter field to InboundMessage that increments on each parse failure and stops
retrying after reaching a maximum attempt threshold. Make sure this logic is
applied in the same code block where parsed_email is None and before saving the
InboundMessage with the error_message.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 150-178: Remove the explicit `status === 201` check on the
newChannel response in the webhook integration form's success handler. Since the
centralized fetchAPI wrapper throws APIError for non-ok responses, any response
that reaches this code path is already successful. Execute the credential
extraction and credential/success handling logic unconditionally for all
successful responses, eliminating the gap where other 2xx status codes would
bypass both credential display and the onSuccess callback.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b94a3b55-68da-4f0e-ba22-b6434e22bf3b
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**
📒 Files selected for processing (20)
docs/webhooks.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/enums.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
sylvinus
force-pushed
the
outbound_webhooks
branch
from
June 19, 2026 16:34
16604f8 to
154e7ab
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
221-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
author_displayfor the avatar color too.Webhook-authored IM events have
author=nullbut a populatedauthor_display, so line 221 hashes""while lines 258-259 show the webhook name.Proposed fix
- const authorName = event.author?.full_name || event.author?.email || ""; - const avatarColor = getAvatarColor(authorName); + const authorDisplayName = + event.author_display || + event.author?.full_name || + event.author?.email || + t("Unknown"); + const avatarColor = getAvatarColor(authorDisplayName); ... - <UserAvatar fullName={event.author_display || t("Unknown")} size="xsmall" /> - {event.author_display || t("Unknown")} + <UserAvatar fullName={authorDisplayName} size="xsmall" /> + {authorDisplayName}🤖 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 `@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx` around lines 221 - 259, The IM thread event avatar color is being derived from author.full_name/email, which falls back to an empty string for webhook-authored events; update the ThreadEvent rendering logic to use author_display for the color input as well. In the thread-view component, adjust the authorName calculation (or equivalent helper used for getAvatarColor) so it prefers event.author_display before falling back to author fields, keeping the avatar color consistent with the displayed webhook name.src/backend/core/mda/inbound_pipeline.py (1)
728-731: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid logging assignment exception text.
This expected race path can include user-identifying details from assignment validation. Log the exception type instead. As per coding guidelines, “Do not log sensitive information (tokens, passwords, financial/health data, PII)”.
Proposed fix
logger.warning( - "assign_users skipped %d assignee(s) due to race: %s", + "assign_users skipped %d assignee(s) due to race (%s)", len(assignees_data), - exc, + type(exc).__name__, )🤖 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 `@src/backend/core/mda/inbound_pipeline.py` around lines 728 - 731, The warning in the assign_users race-handling path is logging the full exception text, which can leak user-identifying details. Update the logger.warning call in the assign_users flow of inbound_pipeline to log only the exception type name (or class) instead of the exception message/object, while keeping the existing count/context in the message.Source: Coding guidelines
src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit webhook-actor fixture path.
authorId === nullstill forcesauthor_displaytonull, so this helper never constructs the newauthor === null && author_displayshape thatbuildAssignmentMessagenow special-cases for webhook/channel events.🤖 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 `@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts` around lines 18 - 30, The makeEvent helper in assignment-message.test.ts does not cover the new webhook actor shape because authorId === null always forces author_display to null. Update this fixture helper or add a dedicated case so tests can construct the author === null and author_display set path that buildAssignmentMessage special-cases for webhook/channel events, and verify the assignment-message behavior through the existing AssignmentEvent setup.
🤖 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 `@docs/webhooks.md`:
- Around line 65-66: Update the webhooks documentation entry for the `url` field
to state the real protocol requirement enforced by the serializer: `https://` is
required in normal deployments, and `http://` is only accepted when Django
`DEBUG` is enabled. Keep the wording aligned with the `url` and `events` table
in `docs/webhooks.md` so readers don’t assume plain HTTP is generally valid.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 610-618: The webhook dispatch task is sending the full email
payload through Celery via the dispatch_webhook_task.delay call in the
dispatch_webhooks flow, which should be replaced with a reference-based handoff.
Update the code around the body_b64/base64 encoding path to persist the snapshot
in encrypted/blob-backed storage first, then pass only the stored object
reference (and any minimal metadata needed) into dispatch_webhook_task instead
of the raw body content.
- Around line 157-158: The warning in dispatch_webhooks around the streamed
response read failure is logging the raw exception message, which can leak
request or URL details. Update the exception handling in the truncated response
body read path to avoid interpolating exc into logger.warning, and use a generic
sanitized message instead. Keep the fix localized to the streamed response read
exception block in dispatch_webhooks and preserve the existing control flow.
- Around line 943-954: The webhook event selection logic in the channel
processing path is treating an explicitly empty events list as missing, so
channels configured with no events still default to MESSAGE_INBOUND. Update the
events handling in dispatch_webhooks so the code distinguishes between a
missing/None value and an empty list, preserving cfg["events"] = [] as-is while
still falling back to the default only when the key is absent or null. Keep the
existing non-list guard and the MESSAGE_INBOUND membership check in place.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 249-279: The quarantine-delivery RETRY path in inbound_tasks.py
leaves ctx.is_spam unchanged, so later logic still treats the message as spam
even though _create_message_from_inbound stores it in the inbox. In the
RETRY/quarantine branch around _handle_retry and _stamp_processing_failed,
normalize ctx.is_spam to False when quarantined=True so downstream task results
and try_send_autoreply use the corrected spam state; apply the same change in
the duplicate RETRY handling block referenced by the comment.
In `@src/backend/core/mda/inbound.py`:
- Around line 214-215: `deliver_inbound_message()` currently raises a
`ValueError` for missing `blob` on internal deliveries, which bypasses its
documented bool-return flow and normal logging. Update the
`deliver_inbound_message` path to catch this validation case with try-except,
log the failure through the existing helper logging, and return `False` instead
of throwing so callers always get a boolean result. Use the `is_internal`/`blob`
check in this function as the place to preserve the standard delivery-failure
behavior.
In `@src/backend/core/tasks.py`:
- Around line 1-4: The task-registration import in tasks.py is intentionally
unused, but only flake8 is being silenced right now; add the proper pylint
suppression so the import of dispatch_webhook_task does not trigger W0611.
Update the module-level disable comment near the top of the file to cover the
unused-import warning for this registration pattern, while keeping the import
itself for Celery autodiscovery.
In `@src/backend/core/tests/mda/test_dispatch_webhooks.py`:
- Around line 2135-2140: The backdating logic in test_dispatch_webhooks.py is
using dj_timezone.timedelta, but django.utils.timezone does not provide
timedelta, so the test will break when updating created_at. Update the
quarantine-window backdate in the test that uses
models.InboundMessage.objects.filter(...).update(...) to use timedelta from
datetime instead, and make sure the timedelta import is available alongside
dj_timezone and QUARANTINE_AFTER.
In `@src/backend/core/tests/mda/test_inbound_spoofed_sender.py`:
- Around line 179-182: The dedupe test is currently using the import path, so it
no longer covers the internal queued-delivery behavior described in the
docstring. Update the test around deliver_inbound_message in
test_inbound_spoofed_sender to exercise the internal-delivery path instead,
using is_internal=True so the duplicate MIME-id dedupe is validated through the
production queueing flow. Keep the assertion focused on the same
duplicate-message behavior, but make sure the setup matches the internal
redelivery scenario.
In `@src/backend/core/tests/mda/test_inbound.py`:
- Around line 260-262: The affected tests are now exercising the import flow by
passing is_import=True to deliver_inbound_message, which hides regressions in
the real inbound-delivery path. Update these test cases to go through the
non-import queue/task entrypoint instead, or explicitly rename/scope them as
import-path tests and adjust the assertions to reflect import behavior in
_create_message_from_inbound and its thread/read-state handling.
In `@src/frontend/public/locales/common/en-US.json`:
- Line 442: The locale copy in the auth setup text uses “secret” in a way that
is specific to one method, so update the affected entries in common/en-US.json
to use auth-method-neutral wording like “credential” or branch the text by
auth_method. Check the translation keys around the receiver authentication and
rotation/setup messages, including the related entries noted by the comment, and
keep the copy accurate for both API key and secret-based flows.
- Line 428: The webhook description text is too specific about JSON and should
be made format-neutral. Update the relevant localization strings in the common
locale entries, including the “Forward every incoming message…” copy and the
other referenced occurrence, so they describe forwarding messages to a URL
without promising a JSON POST or JSON payload; keep the wording compatible with
raw .eml and the other supported formats exposed by the webhook channel.
---
Outside diff comments:
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 728-731: The warning in the assign_users race-handling path is
logging the full exception text, which can leak user-identifying details. Update
the logger.warning call in the assign_users flow of inbound_pipeline to log only
the exception type name (or class) instead of the exception message/object,
while keeping the existing count/context in the message.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts`:
- Around line 18-30: The makeEvent helper in assignment-message.test.ts does not
cover the new webhook actor shape because authorId === null always forces
author_display to null. Update this fixture helper or add a dedicated case so
tests can construct the author === null and author_display set path that
buildAssignmentMessage special-cases for webhook/channel events, and verify the
assignment-message behavior through the existing AssignmentEvent setup.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 221-259: The IM thread event avatar color is being derived from
author.full_name/email, which falls back to an empty string for webhook-authored
events; update the ThreadEvent rendering logic to use author_display for the
color input as well. In the thread-view component, adjust the authorName
calculation (or equivalent helper used for getAvatarColor) so it prefers
event.author_display before falling back to author fields, keeping the avatar
color consistent with the displayed webhook name.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba3d67de-37b5-4e37-b072-3b8aa0fad14c
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**
📒 Files selected for processing (40)
docs/tiered-storage.mddocs/webhooks.mdenv.d/development/backend.defaultssrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/enums.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0032_inboundmessage_blob_inboundmessage_is_internal_and_more.pysrc/backend/core/models.pysrc/backend/core/services/exporter/tasks.pysrc/backend/core/signals.pysrc/backend/core/tasks.pysrc/backend/core/templates/admin/core/channel/change_form.htmlsrc/backend/core/templates/admin/core/channel/regenerated_secret.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/core/tests/models/test_blob.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
💤 Files with no reviewable changes (1)
- src/backend/core/templates/admin/core/channel/regenerated_secret.html
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/backend/core/mda/dispatch_webhooks.py (1)
953-987: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRun
message.deliveringhooks beforemessage.deliveredhooks.After-spam steps are emitted in queryset order, but
message.deliveredcapturesctx.is_spamimmediately. If a latermessage.deliveringhook overrides the verdict, the queued webhook goes out with staleX-StMsg-Is-Spamdata.Suggested fix
- steps: List[Step] = [] + steps: List[Step] = [] + deferred_non_blocking: List[Step] = [] @@ - steps.append(UserWebhookStep(channel, phase=phase)) - return steps + step = UserWebhookStep(channel, phase=phase) + if ( + phase == PHASE_AFTER_SPAM + and trigger == enums.WebhookTrigger.MESSAGE_DELIVERED + ): + deferred_non_blocking.append(step) + else: + steps.append(step) + return steps + deferred_non_blocking🤖 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 `@src/backend/core/mda/dispatch_webhooks.py` around lines 953 - 987, In the webhook step assembly logic that builds the `steps` list for `UserWebhookStep`, ensure `message.delivering` is queued and executed before any `message.delivered` hooks so the final spam verdict is settled first. Update the ordering in the phase-based selection around `runs_at` / `phase` so after-spam hooks do not capture a stale `ctx.is_spam` value, while keeping the existing trigger and format validation behavior unchanged.src/backend/core/tests/api/test_channel_scope_level.py (1)
1021-1037: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the one-time webhook credential on personal-channel creation.
With
auth_method: "jwt", this path should now surfacesecretexactly once, just like the mailbox-scoped webhook create flow. The current test only checks that legacyhmac_secretis absent, so a regression in the/users/me/channels/response contract would still pass.Suggested assertion
assert response.status_code == 201, response.content - # No HMAC secret generation yet — that scaffolding lands with the - # delivery pipeline. The response carries the row id only. assert "hmac_secret" not in response.data + assert response.data.get("secret") created = models.Channel.objects.get(pk=response.data["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 `@src/backend/core/tests/api/test_channel_scope_level.py` around lines 1021 - 1037, The personal webhook creation test in test_channel_scope_level.py only checks that legacy hmac_secret is absent, so it can miss regressions in the /users/me/channels/ response contract. Update the assertion in the personal-channel create flow to verify the one-time webhook credential is surfaced as secret exactly once when auth_method is jwt, matching the mailbox-scoped webhook behavior, and keep the existing response checks around the created webhook payload.src/backend/core/tests/api/test_messages_delivery_statuses.py (1)
576-600: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCover
SENT_INTERNALin these transition tests as well.After splitting delivered into
SENT_INTERNALandSENT_EXTERNAL, these cases now exercise only the external branch. That leaves a hole where the endpoint could reject updates forSENT_EXTERNALbut still allow the same invalid transition for internally delivered recipients.Suggested pattern
+ `@pytest.mark.parametrize`( + "delivered_status", + [ + enums.MessageDeliveryStatusChoices.SENT_INTERNAL, + enums.MessageDeliveryStatusChoices.SENT_EXTERNAL, + ], + ) - def test_api_messages_delivery_statuses_invalid_transition_sent_to_cancelled(self): + def test_api_messages_delivery_statuses_invalid_transition_sent_to_cancelled( + self, delivered_status + ): ... recipient = factories.MessageRecipientFactory( message=message, - delivery_status=enums.MessageDeliveryStatusChoices.SENT_EXTERNAL, + delivery_status=delivered_status, )As per coding guidelines, “Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases.”
Also applies to: 613-692, 698-752, 754-836
🤖 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 `@src/backend/core/tests/api/test_messages_delivery_statuses.py` around lines 576 - 600, Add SENT_INTERNAL coverage to the delivery-status transition tests that currently only exercise SENT_EXTERNAL, since the split left an untested branch in the same invalid-transition scenarios. Update the affected test cases in test_api_messages_delivery_statuses_* to parameterize or duplicate the setup for both MessageDeliveryStatusChoices.SENT_EXTERNAL and MessageDeliveryStatusChoices.SENT_INTERNAL, keeping the same transition assertions for each recipient status. Use the existing test method names and the MessageRecipientFactory setup as the anchor points when expanding coverage across the listed cases.Source: Coding guidelines
src/backend/core/services/ssrf.py (1)
266-279: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't replay POST on
303 See Other. The redirect loop preserves the original verb for every status inREDIRECT_STATUS_CODES, so a POST that receives303is sent again to theLocationURL instead of switching to GET. That can duplicate side effects at the redirected endpoint; special-case303before retrying.🤖 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 `@src/backend/core/services/ssrf.py` around lines 266 - 279, The redirect handling in the SSRF helper is replaying the original request verb for every status in REDIRECT_STATUS_CODES, so a POST followed by 303 See Other is retried as POST instead of switching to GET. Update the redirect loop in the SSRF redirect-following logic to special-case 303 before the retry, preserving the existing behavior for other redirect codes while converting the next request to GET for 303. Use the redirect flow around current_url, response.status_code, and REDIRECT_STATUS_CODES to locate the change.src/backend/core/mda/inbound_tasks.py (1)
203-210: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound parse failures with the same retry/abandon policy.
A malformed RFC-822 payload will keep returning
success=False, so the retry sweep can pin theInboundMessageindefinitely. Route this through_retry_or_abandon(...)so deterministic parse failures stop looping after the quarantine window.Proposed fix
if parsed_email is None: error_msg = "Failed to parse email message" logger.error(error_msg) - inbound_message.error_message = error_msg - inbound_message.save(update_fields=["error_message"]) - return {"success": False, "error": error_msg} + return _retry_or_abandon(inbound_message, error_msg)🤖 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 `@src/backend/core/mda/inbound_tasks.py` around lines 203 - 210, The parse failure path in inbound task handling currently returns a plain failure, which leaves malformed RFC-822 messages subject to repeated retries. Update the `parse_email`/`parsed_email is None` branch in `inbound_tasks.py` to route the failure through `_retry_or_abandon(...)` using the same error message and `InboundMessage` handling, so deterministic parse errors are abandoned after the quarantine window instead of looping indefinitely.
♻️ Duplicate comments (1)
src/backend/core/tests/mda/test_inbound.py (1)
506-507: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThese assertions still hit the import-only branch.
is_import=Trueis not a sync replacement for the inbound path:_create_message_from_inbound()switches thread matching, import flags, and read-state handling when that flag is set. These tests can now pass while real inbound delivery regresses. Please drive the non-import entrypoint here, or explicitly scope/rename them as import-path tests.Also applies to: 535-536, 564-565, 820-824
🤖 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 `@src/backend/core/tests/mda/test_inbound.py` around lines 506 - 507, The inbound delivery tests are still exercising the import-only path because deliver_inbound_message is called with is_import=True, which bypasses the real inbound behavior in _create_message_from_inbound. Update these cases to use the non-import inbound entrypoint so thread matching, flags, and read-state handling follow the normal path, or rename/scope the tests explicitly as import-path coverage; apply the same fix in the other deliver_inbound_message call sites in this test module.
🤖 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 `@src/backend/core/admin.py`:
- Around line 612-619: The credential rotation in the admin flow is accepting
unsupported webhook auth methods and then falling back to the newly rotated
root, which can invalidate the old secret. Update the logic around
rotate_secret() and get_webhook_surfaced_credential() to reject webhook channels
with missing/unknown auth_method before rotating, mirroring the DRF pre-rotation
validation. Only keep the root fallback for ChannelTypes.API_KEY channels, and
preserve the existing webhook credential display behavior for supported methods.
In `@src/backend/core/mda/webhook_payload.py`:
- Around line 65-74: build_jmap_email currently strips parser-only fields from
textBody, htmlBody, and attachments, but it leaves bodyStructure untouched, so
multipart emails can still contain non-JMAP keys or raw bytes. Update the
include_body path in build_jmap_email to recursively sanitize
parsed_email["bodyStructure"] using the same _strip_body_part logic (including
nested parts), and ensure the cleaned structure is assigned back to
email["bodyStructure"] before returning.
In `@src/backend/core/templates/admin/core/channel/change_form.html`:
- Line 15: The confirmation prompt in the channel change form is hard-coded
English, creating a mixed-language admin UI. Update the inline confirm text used
by the onsubmit handler in the channel secret regeneration form to use the same
translation approach as the button label, so the message is localized. Keep the
existing confirmation behavior, but move the prompt text into the template’s
i18n flow so it can be translated consistently.
In `@src/backend/core/tests/mda/test_autoreply.py`:
- Around line 821-828: The final gate assertion in the autoreply test is using
an outbound-shaped parsed message with the original sender mailbox, so it can
pass for the wrong reason. Update the assertion around should_send_autoreply()
in test_autoreply to use an inbound-shaped message/mailbox pair that matches the
delivery path being tested, so the check specifically verifies loop prevention
rather than self-sender/address-shape filtering.
In `@src/frontend/public/locales/common/nl-NL.json`:
- Around line 16-17: The Dutch translation for the webhook auth help text
currently hard-codes a secret-only term, which is inaccurate for both
secret-based and API-key webhooks. Update the affected entries in the common
locale JSON to use a neutral term such as credential, inloggegeven, or
toegangsgegeven instead of geheim, and make sure the matching regenerate CTA
copy uses the same neutral wording so both messages stay consistent across
webhook auth types.
In `@src/frontend/public/locales/common/ru-RU.json`:
- Line 847: The Russian locale strings in the credential description are too
specific because they hard-code “секрет” even though this flow can use either a
signing secret or an API key. Update the affected translation entries in the
common ru-RU locale to use credential-neutral wording, and make the same wording
change for the related strings referenced by the comment so the text stays
accurate regardless of credential type.
In `@src/frontend/public/locales/common/uk-UA.json`:
- Around line 846-847: The Ukrainian locale text for the webhook authentication
help and regenerate label is too specific by translating “credential” as
“secret,” which is incorrect for API-key auth. Update the affected strings in
common/uk-UA.json so the wording uses a neutral term for the one-time value
(covering both secret and api_key), and apply the same fix to the matching
entries referenced by the other locale keys.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 220-224: The IM event header now uses a different fallback than
the color hash, so stale payloads without author_display can render as Unknown
for real users. In thread-event/index.tsx, update the IM event handling in the
isIMEvent branch to derive one shared author label from event.author_display,
event.author?.full_name, event.author?.email, then reuse that same value for
both the hash input and the rendered header text. Apply the same fix anywhere
else in this file where the IM header label is built separately from the color
fallback, including the other affected block.
---
Outside diff comments:
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 953-987: In the webhook step assembly logic that builds the
`steps` list for `UserWebhookStep`, ensure `message.delivering` is queued and
executed before any `message.delivered` hooks so the final spam verdict is
settled first. Update the ordering in the phase-based selection around `runs_at`
/ `phase` so after-spam hooks do not capture a stale `ctx.is_spam` value, while
keeping the existing trigger and format validation behavior unchanged.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 203-210: The parse failure path in inbound task handling currently
returns a plain failure, which leaves malformed RFC-822 messages subject to
repeated retries. Update the `parse_email`/`parsed_email is None` branch in
`inbound_tasks.py` to route the failure through `_retry_or_abandon(...)` using
the same error message and `InboundMessage` handling, so deterministic parse
errors are abandoned after the quarantine window instead of looping
indefinitely.
In `@src/backend/core/services/ssrf.py`:
- Around line 266-279: The redirect handling in the SSRF helper is replaying the
original request verb for every status in REDIRECT_STATUS_CODES, so a POST
followed by 303 See Other is retried as POST instead of switching to GET. Update
the redirect loop in the SSRF redirect-following logic to special-case 303
before the retry, preserving the existing behavior for other redirect codes
while converting the next request to GET for 303. Use the redirect flow around
current_url, response.status_code, and REDIRECT_STATUS_CODES to locate the
change.
In `@src/backend/core/tests/api/test_channel_scope_level.py`:
- Around line 1021-1037: The personal webhook creation test in
test_channel_scope_level.py only checks that legacy hmac_secret is absent, so it
can miss regressions in the /users/me/channels/ response contract. Update the
assertion in the personal-channel create flow to verify the one-time webhook
credential is surfaced as secret exactly once when auth_method is jwt, matching
the mailbox-scoped webhook behavior, and keep the existing response checks
around the created webhook payload.
In `@src/backend/core/tests/api/test_messages_delivery_statuses.py`:
- Around line 576-600: Add SENT_INTERNAL coverage to the delivery-status
transition tests that currently only exercise SENT_EXTERNAL, since the split
left an untested branch in the same invalid-transition scenarios. Update the
affected test cases in test_api_messages_delivery_statuses_* to parameterize or
duplicate the setup for both MessageDeliveryStatusChoices.SENT_EXTERNAL and
MessageDeliveryStatusChoices.SENT_INTERNAL, keeping the same transition
assertions for each recipient status. Use the existing test method names and the
MessageRecipientFactory setup as the anchor points when expanding coverage
across the listed cases.
---
Duplicate comments:
In `@src/backend/core/tests/mda/test_inbound.py`:
- Around line 506-507: The inbound delivery tests are still exercising the
import-only path because deliver_inbound_message is called with is_import=True,
which bypasses the real inbound behavior in _create_message_from_inbound. Update
these cases to use the non-import inbound entrypoint so thread matching, flags,
and read-state handling follow the normal path, or rename/scope the tests
explicitly as import-path coverage; apply the same fix in the other
deliver_inbound_message call sites in this test module.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48845f12-ba30-42ba-b29c-92c44f02fa21
📒 Files selected for processing (46)
docs/webhooks.mdsrc/backend/core/admin.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/outbound.pysrc/backend/core/mda/selfcheck.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/models.pysrc/backend/core/services/ssrf.pysrc/backend/core/tasks.pysrc/backend/core/templates/admin/core/channel/change_form.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_messages_delivery_statuses.pysrc/backend/core/tests/api/test_messages_import.pysrc/backend/core/tests/api/test_prometheus_metrics.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/mda/test_autoreply.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_outbound.pysrc/backend/core/tests/mda/test_outbound_e2e.pysrc/backend/core/tests/mda/test_retry.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/core/tests/test_signals.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
Comment on lines
+65
to
+74
| if include_body: | ||
| email["textBody"] = [ | ||
| _strip_body_part(p) for p in parsed_email.get("textBody") or [] | ||
| ] | ||
| email["htmlBody"] = [ | ||
| _strip_body_part(p) for p in parsed_email.get("htmlBody") or [] | ||
| ] | ||
| email["attachments"] = [ | ||
| _strip_body_part(p) for p in parsed_email.get("attachments") or [] | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Sanitize bodyStructure recursively as well.
build_jmap_email() only cleans textBody / htmlBody / attachments. The copied bodyStructure still carries parser-only fields, so multipart payloads can leak non-JMAP keys or raw attachment bytes and break JSON serialization on send.
Suggested fix
def _strip_body_part(part: Dict[str, Any]) -> Dict[str, Any]:
@@
- return {k: v for k, v in part.items() if k not in ("content", "sha256")}
+ clean = {
+ k: v for k, v in part.items() if k not in ("_ext", "content", "sha256")
+ }
+ if isinstance(clean.get("subParts"), list):
+ clean["subParts"] = [_strip_body_part(p) for p in clean["subParts"]]
+ return clean
@@
if include_body:
email["textBody"] = [
_strip_body_part(p) for p in parsed_email.get("textBody") or []
]
email["htmlBody"] = [
_strip_body_part(p) for p in parsed_email.get("htmlBody") or []
]
email["attachments"] = [
_strip_body_part(p) for p in parsed_email.get("attachments") or []
]
+ if parsed_email.get("bodyStructure"):
+ email["bodyStructure"] = _strip_body_part(parsed_email["bodyStructure"])📝 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 include_body: | |
| email["textBody"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("textBody") or [] | |
| ] | |
| email["htmlBody"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("htmlBody") or [] | |
| ] | |
| email["attachments"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("attachments") or [] | |
| ] | |
| if include_body: | |
| email["textBody"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("textBody") or [] | |
| ] | |
| email["htmlBody"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("htmlBody") or [] | |
| ] | |
| email["attachments"] = [ | |
| _strip_body_part(p) for p in parsed_email.get("attachments") or [] | |
| ] | |
| if parsed_email.get("bodyStructure"): | |
| email["bodyStructure"] = _strip_body_part(parsed_email["bodyStructure"]) |
🤖 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 `@src/backend/core/mda/webhook_payload.py` around lines 65 - 74,
build_jmap_email currently strips parser-only fields from textBody, htmlBody,
and attachments, but it leaves bodyStructure untouched, so multipart emails can
still contain non-JMAP keys or raw bytes. Update the include_body path in
build_jmap_email to recursively sanitize parsed_email["bodyStructure"] using the
same _strip_body_part logic (including nested parts), and ensure the cleaned
structure is assigned back to email["bodyStructure"] before returning.
sylvinus
force-pushed
the
outbound_webhooks
branch
from
June 29, 2026 12:22
9331775 to
27222a7
Compare
There was a problem hiding this comment.
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 (4)
src/backend/core/tests/mda/test_retry.py (1)
77-84: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a
SENT_INTERNALnon-retry case.These updates prove
SENT_EXTERNALis excluded from retry, but the renamed enum now has two terminal delivered states. A sibling assertion forSENT_INTERNALwould guard the retry filter against the exact==/!= SENT_EXTERNALfootgun documented onMessageDeliveryStatusChoices.🧪 Suggested coverage
+ `@patch`("core.mda.outbound_tasks.send_message") + def test_retry_message_with_sent_internal_recipient_not_retryable( + self, mock_send_message, mailbox_sender, thread + ): + sender_contact = factories.ContactFactory(mailbox=mailbox_sender) + message = factories.MessageFactory( + thread=thread, + sender=sender_contact, + is_draft=False, + is_sender=True, + subject="Internal delivery already completed", + ) + internal_contact = factories.ContactFactory(mailbox=mailbox_sender) + factories.MessageRecipientFactory( + message=message, + contact=internal_contact, + type=models.MessageRecipientTypeChoices.TO, + delivery_status=enums.MessageDeliveryStatusChoices.SENT_INTERNAL, + delivered_at=timezone.now(), + ) + + result = retry_messages_task.apply(args=[[str(message.id)]]).get() + + assert result["processed_messages"] == 0 + mock_send_message.assert_not_called()As per coding guidelines,
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases.Also applies to: 408-415, 463-470
🤖 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 `@src/backend/core/tests/mda/test_retry.py` around lines 77 - 84, Add a parallel non-retry assertion for MessageDeliveryStatusChoices.SENT_INTERNAL in the retry-related tests, alongside the existing SENT_EXTERNAL case, so the retry filter is validated against both delivered terminal states. Update the relevant test setup in test_retry.py and any other matching retry coverage blocks to create a recipient with SENT_INTERNAL and assert it is excluded from retry, using the existing test helpers and MessageRecipientFactory for consistency.Source: Coding guidelines
src/backend/core/services/ssrf.py (1)
240-280: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject HTTPS→HTTP redirect downgrades.
The manual redirect loop will replay the same POST body and caller headers to a plaintext
http://Location. That leaks webhook payloads or auth material in transit even when the configured endpoint started as HTTPS. If plaintext delivery is intended, require it in the initial URL instead of silently downgrading mid-chain.Proposed fix
next_url = urljoin(current_url, location) + if ( + urlparse(current_url).scheme == "https" + and urlparse(next_url).scheme != "https" + ): + response.close() + raise SSRFValidationError( + "Refusing redirect from HTTPS to insecure HTTP" + ) response.close() current_url = next_url🤖 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 `@src/backend/core/services/ssrf.py` around lines 240 - 280, The manual redirect handling in _request_with_redirects currently follows Location headers from an HTTPS request to an HTTP target, which can silently downgrade transport security. Update the redirect validation logic in _request_with_redirects to reject any HTTPS-to-HTTP redirect before calling _pinned_session or replaying the request, while still allowing same-scheme or stronger redirects. Make sure the check is applied per hop using current_url and next_url so redirects are blocked even when the chain starts on HTTPS.src/backend/core/models.py (1)
2291-2327: 🚀 Performance & Scalability | 🔵 TrivialAdd an index for the new abandonment cutoff.
abandoned_atis now the retry/purge discriminator, butMeta.indexesstill only coverscreated_at. Once abandoned rows accumulate, the periodic sweeps behind this field will degrade to full scans. Add the supporting index(es) in the model/migration alongside the field. As per coding guidelines,src/backend/**/{models.py,migrations/**/*.py}:Implement database indexing and query optimization (Model Meta indexes, constraints).🤖 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 `@src/backend/core/models.py` around lines 2291 - 2327, `abandoned_at` is now used as the cutoff for retry/purge sweeps, but `Meta.indexes` on `InboundMessage` still only indexes `created_at`, so the cleanup queries will degrade as abandoned rows grow. Add the appropriate model index in `InboundMessage.Meta.indexes` for `abandoned_at`, and mirror it in the matching migration so `purge_abandoned_inbound_messages_task` and related queries can filter efficiently.Source: Coding guidelines
src/backend/core/tests/mda/test_spam_processing.py (1)
192-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale rspamd error comment.
The helper returns
Falseplus an error sentinel, but the pipeline now uses that error to hold for retry rather than deliver unchecked.Proposed wording fix
- # On error, treat as not spam to avoid blocking legitimate messages + # Low-level helper returns a false verdict plus an error sentinel; + # the rspamd pipeline step uses the error to hold the message for retry. assert is_spam is False🤖 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 `@src/backend/core/tests/mda/test_spam_processing.py` around lines 192 - 195, Update the stale inline comment in the spam-processing test to match the current rspamd error handling: the helper now returns False with an error sentinel that the pipeline uses to hold for retry, not to immediately treat the message as safe. Keep the assertion block in test_spam_processing aligned with the behavior of the related helper and pipeline logic, and revise the comment text near is_spam, error, and rspamd_result accordingly.
♻️ Duplicate comments (2)
src/backend/core/mda/inbound_pipeline.py (1)
528-556: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove assignee emails from warning logs.
These webhook-supplied addresses are PII. Keep the skip reason, but don’t emit the raw email value.
Proposed fix
logger.warning( - "Webhook assignee email %s does not resolve to any user — skipping", - email, + "Webhook assignee email does not resolve to any user — skipping", ) @@ logger.warning( - "Webhook assignee email %s is ambiguous (multiple matches) — skipping", - email, + "Webhook assignee email is ambiguous (multiple matches) — skipping", ) @@ logger.warning( - "Webhook assignee %s lacks an assignable role on the thread — skipping", - candidate_users[uid].email, + "Webhook assignee lacks an assignable role on the thread — skipping", )As per coding guidelines, “Do not log sensitive information (tokens, passwords, financial/health data, PII)”.
🤖 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 `@src/backend/core/mda/inbound_pipeline.py` around lines 528 - 556, The warning logs in the assignee resolution flow still emit webhook-supplied email addresses, which is sensitive PII. Update the logging in the inbound pipeline’s assignee lookup/assignment path so the messages in this section keep the skip reason but do not include the raw email value; use the existing logger.warning calls around candidate resolution and ThreadAccess checks to reference a non-sensitive identifier or omit the address entirely.Source: Coding guidelines
src/backend/core/tests/mda/test_autoreply.py (1)
813-850: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the final gate assertion inbound-shaped.
parsedis still addressed to the original sender, soshould_send_autoreply(other_mailbox, parsed)can returnNoneeven if_is_auto_reply_message()regresses because_is_recipient_explicit()rejects it first. Point the parsedto/ccatother_mailbox(or build an inbound-shaped fixture) before the last assertion so this actually proves loop prevention.Possible fix
- assert should_send_autoreply(other_mailbox, parsed) is None + inbound_shaped = { + **parsed, + "to": [{"email": str(other_mailbox)}], + "cc": [], + } + assert should_send_autoreply(other_mailbox, inbound_shaped) is None🤖 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 `@src/backend/core/tests/mda/test_autoreply.py` around lines 813 - 850, The final autoreply gate assertion is not using an inbound-shaped message, so it can fail for recipient-shape reasons instead of the auto-reply detector. Update the test around send_autoreply_for_message, _is_auto_reply_message, and should_send_autoreply so the parsed message is addressed to other_mailbox (or use an inbound-shaped fixture with matching to/cc) before the last assertion. Keep the second mailbox/template setup, but ensure the final should_send_autoreply(other_mailbox, parsed) check can only pass if loop prevention works.
🤖 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 `@docs/env.md`:
- Around line 247-249: The ALLOWED_HOSTS documentation entry is inaccurate
because it conflates the Django setting with the separate DJANGO_ALLOWED_HOSTS
variable. Update the wording in the env docs to describe ALLOWED_HOSTS as the
Django allow-list setting from settings.py, and clarify that
DJANGO_ALLOWED_HOSTS is only used by development defaults/tooling rather than
saying the Development class sets ALLOWED_HOSTS from it.
In `@docs/tiered-storage.md`:
- Line 55: Update the fast-path GC section so the blob deletion paragraph
matches the liveness graph change for InboundMessage.blob. In the tiered-storage
docs near the liveness graph and deletion description, either add InboundMessage
to the list of entities whose deletions enqueue blob candidates, or explicitly
state that only mode="full" can reclaim InboundMessage blobs. Reference the
InboundMessage.blob and mode="full" wording consistently with the existing
Message, Attachment, and MessageTemplate descriptions.
In `@docs/webhooks.md`:
- Around line 106-108: The regenerate-secret example is scope-specific and
currently implies the unscoped channel route, which is misleading for
mailbox-scoped webhooks. Update the webhook docs near the root secret
description to explain that the regenerate-secret path prefix depends on scope,
and for mailbox-scoped channels it should be reached through the mailbox route
using both mailbox_id and pk. Keep the generic `regenerate-secret` wording tied
to the relevant channel scope so readers are directed to the correct endpoint.
In `@src/backend/core/admin.py`:
- Around line 1603-1608: The replay loop in the admin action mutates each
inbound message before publishing the Celery task, so a broker failure can erase
the abandoned/error state without re-queueing. Update the logic around the
queryset iteration in the admin handler to call
process_inbound_message_task.delay inside a try-except, and only clear
abandoned_at and error_message after a successful publish; if delay fails, leave
the row unchanged and avoid incrementing replayed for that item.
In `@src/backend/core/mda/autoreply.py`:
- Around line 290-291: The async send enqueue in the autoreply flow is still
happening inside the inner savepoint, so it can race an uncommitted message when
an outer transaction exists. Update the send trigger in the autoreply logic
around the `send_message_task.delay(str(message.id))` call to register the
enqueue with `transaction.on_commit()` instead, so the task is queued only after
the message row is fully committed.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 302-319: _make `_sanitize_url()` resilient to malformed ports_:
`urlparse()` can succeed while accessing `parsed.port` raises `ValueError`, so
update the helper to safely handle that case and still return a fallback value
instead of failing during logging. Use the existing `_sanitize_url` function and
keep the current `<unparseable-url>` / `<no-host>` behavior, but wrap port
access so invalid port strings do not propagate an exception.
- Around line 1055-1068: Revalidate the channel before dispatching the queued
webhook payload in the webhook task around the channel/message lookup logic in
dispatch_webhooks; after re-fetching the Channel and Mailbox, ensure the channel
is still a webhook, still has message.delivered enabled, and still belongs to
the same mailbox as the Message being posted. If any of those checks fail, skip
sending and return early, using the existing channel_id/mailbox_id/message_id
context in the task.
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 265-275: The rspamd error path in inbound_pipeline should stop
returning raw exception text because it can expose endpoint details and gets
logged again downstream. Update the exception handling around the rspamd call in
the rspamd-check helper so both the RequestException/ValueError and generic
Exception branches return a sanitized error token instead of str(exc), and make
the same change in the other rspamd error path noted in the review so callers
only receive non-sensitive, stable error identifiers.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 95-103: The broad exception handlers in the inbound task flow are
swallowing Celery’s soft-timeout exception before the task-level timeout
handling can run. Update the `except Exception` blocks in the finalize-step path
and the task-level handler in `inbound_tasks.py` to detect
`SoftTimeLimitExceeded` and re-raise it immediately, while keeping the existing
logging for other exceptions. Use the surrounding `fn()` execution and the
inbound-message task handler as the places to make this change.
In `@src/backend/core/mda/inbound.py`:
- Around line 228-232: The info log in the inbound message queueing path is
exposing recipient email PII; update the logger call in the inbound message
handling flow to avoid printing recipient_email and use a non-user identifier
such as mailbox.id or inbound_message.id instead. Locate the log statement in
the inbound message queueing code and keep the message useful for debugging by
reporting internal/non-sensitive identifiers only.
In `@src/backend/core/templates/admin/core/channel/change_form.html`:
- Line 15: The inline confirm prompt in the channel change form is using
translated text directly inside JavaScript, which can break when a locale
includes quotes or backslashes. Update the `onsubmit` handler in the
`change_form.html` template so the `{% blocktranslate %}` output is escaped for
JavaScript before being passed to `confirm(...)`, keeping the prompt safe across
translations and preserving the secret regeneration flow.
In `@src/backend/core/tests/api/test_threads_list.py`:
- Around line 1521-1524: The flag-preservation test in test_threads_list
currently only covers the SENT_EXTERNAL terminal delivery case, so it can miss
regressions in the same-instance path. Update the test around
MessageRecipientFactory and the related status assertions to include
SENT_INTERNAL alongside SENT_EXTERNAL, ensuring both delivered-class statuses
are treated identically by the thread-list flag logic.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 67-74: The URL validation in webhook-integration-form.tsx only
checks the scheme prefix, so host-less values like https:///no-host slip through
client-side validation and fail later on save. Update the zod schema for the url
field to also require a valid host/authority (not just http(s)://), keeping the
existing allowInsecureUrl behavior and error messages in the same validation
block so the user gets an inline field error before submit.
---
Outside diff comments:
In `@src/backend/core/models.py`:
- Around line 2291-2327: `abandoned_at` is now used as the cutoff for
retry/purge sweeps, but `Meta.indexes` on `InboundMessage` still only indexes
`created_at`, so the cleanup queries will degrade as abandoned rows grow. Add
the appropriate model index in `InboundMessage.Meta.indexes` for `abandoned_at`,
and mirror it in the matching migration so
`purge_abandoned_inbound_messages_task` and related queries can filter
efficiently.
In `@src/backend/core/services/ssrf.py`:
- Around line 240-280: The manual redirect handling in _request_with_redirects
currently follows Location headers from an HTTPS request to an HTTP target,
which can silently downgrade transport security. Update the redirect validation
logic in _request_with_redirects to reject any HTTPS-to-HTTP redirect before
calling _pinned_session or replaying the request, while still allowing
same-scheme or stronger redirects. Make sure the check is applied per hop using
current_url and next_url so redirects are blocked even when the chain starts on
HTTPS.
In `@src/backend/core/tests/mda/test_retry.py`:
- Around line 77-84: Add a parallel non-retry assertion for
MessageDeliveryStatusChoices.SENT_INTERNAL in the retry-related tests, alongside
the existing SENT_EXTERNAL case, so the retry filter is validated against both
delivered terminal states. Update the relevant test setup in test_retry.py and
any other matching retry coverage blocks to create a recipient with
SENT_INTERNAL and assert it is excluded from retry, using the existing test
helpers and MessageRecipientFactory for consistency.
In `@src/backend/core/tests/mda/test_spam_processing.py`:
- Around line 192-195: Update the stale inline comment in the spam-processing
test to match the current rspamd error handling: the helper now returns False
with an error sentinel that the pipeline uses to hold for retry, not to
immediately treat the message as safe. Keep the assertion block in
test_spam_processing aligned with the behavior of the related helper and
pipeline logic, and revise the comment text near is_spam, error, and
rspamd_result accordingly.
---
Duplicate comments:
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 528-556: The warning logs in the assignee resolution flow still
emit webhook-supplied email addresses, which is sensitive PII. Update the
logging in the inbound pipeline’s assignee lookup/assignment path so the
messages in this section keep the skip reason but do not include the raw email
value; use the existing logger.warning calls around candidate resolution and
ThreadAccess checks to reference a non-sensitive identifier or omit the address
entirely.
In `@src/backend/core/tests/mda/test_autoreply.py`:
- Around line 813-850: The final autoreply gate assertion is not using an
inbound-shaped message, so it can fail for recipient-shape reasons instead of
the auto-reply detector. Update the test around send_autoreply_for_message,
_is_auto_reply_message, and should_send_autoreply so the parsed message is
addressed to other_mailbox (or use an inbound-shaped fixture with matching
to/cc) before the last assertion. Keep the second mailbox/template setup, but
ensure the final should_send_autoreply(other_mailbox, parsed) check can only
pass if loop prevention works.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 368ad8b0-573b-4500-8ca7-9f4f521a7dc0
⛔ Files ignored due to path filters (7)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_api_key_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**
📒 Files selected for processing (64)
docs/env.mddocs/tiered-storage.mddocs/webhooks.mdenv.d/development/backend.defaultssrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/outbound.pysrc/backend/core/mda/selfcheck.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/migrations/0032_inboundmessage_blob_inboundmessage_is_internal_and_more.pysrc/backend/core/models.pysrc/backend/core/services/exporter/tasks.pysrc/backend/core/services/ssrf.pysrc/backend/core/services/thread_events.pysrc/backend/core/signals.pysrc/backend/core/tasks.pysrc/backend/core/templates/admin/core/channel/change_form.htmlsrc/backend/core/templates/admin/core/channel/regenerated_secret.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_messages_delivery_statuses.pysrc/backend/core/tests/api/test_messages_import.pysrc/backend/core/tests/api/test_prometheus_metrics.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/mda/test_autoreply.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/mda/test_inbound_auth.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_outbound.pysrc/backend/core/tests/mda/test_outbound_e2e.pysrc/backend/core/tests/mda/test_retry.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/core/tests/models/test_blob.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/core/tests/test_signals.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/celery_app.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scsssrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
Comment on lines
+290
to
291
| # Trigger async send (outside transaction to avoid sending before commit) | ||
| send_message_task.delay(str(message.id)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use transaction.on_commit() for the send enqueue.
Leaving this inner atomic() only releases the savepoint. If a caller already has an outer transaction open, delay() can still run before the message row commits and the worker can race a missing message. Register the enqueue on commit instead.
Possible fix
- send_message_task.delay(str(message.id))
+ transaction.on_commit(lambda: send_message_task.delay(str(message.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.
Suggested change
| # Trigger async send (outside transaction to avoid sending before commit) | |
| send_message_task.delay(str(message.id)) | |
| # Trigger async send (outside transaction to avoid sending before commit) | |
| transaction.on_commit(lambda: send_message_task.delay(str(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 `@src/backend/core/mda/autoreply.py` around lines 290 - 291, The async send
enqueue in the autoreply flow is still happening inside the inner savepoint, so
it can race an uncommitted message when an outer transaction exists. Update the
send trigger in the autoreply logic around the
`send_message_task.delay(str(message.id))` call to register the enqueue with
`transaction.on_commit()` instead, so the task is queued only after the message
row is fully committed.
Comment on lines
95
to
103
| try: | ||
| headers = {"Content-Type": "message/rfc822"} | ||
| spam_auth = spam_config.get("rspamd_auth") | ||
| if spam_auth: | ||
| headers["Authorization"] = spam_auth | ||
|
|
||
| response = requests.post( | ||
| f"{spam_url}/checkv2", | ||
| data=raw_data, | ||
| headers=headers, | ||
| timeout=10, | ||
| fn() | ||
| except Exception as exc: # pylint: disable=broad-exception-caught | ||
| logger.exception( | ||
| "Finalize step %r failed on inbound message %s: %s", | ||
| label, | ||
| inbound_message_id, | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python - <<'PY'
from celery.exceptions import SoftTimeLimitExceeded
print(issubclass(SoftTimeLimitExceeded, Exception))
PYRepository: suitenumerique/messages
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect its structure.
git ls-files 'src/backend/core/mda/inbound_tasks.py'
echo '--- outline ---'
ast-grep outline src/backend/core/mda/inbound_tasks.py --view expanded || true
echo '--- soft timelimit search ---'
rg -n 'SoftTimeLimitExceeded|soft time|soft_time_limit|time limit' src/backend/core/mda/inbound_tasks.py src/backend -g '*.py' || true
echo '--- relevant slices ---'
sed -n '1,180p' src/backend/core/mda/inbound_tasks.py
echo '--- later slice ---'
sed -n '430,540p' src/backend/core/mda/inbound_tasks.pyRepository: suitenumerique/messages
Length of output: 14814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/backend/core/mda/inbound_tasks.py')
print('exists', p.exists())
if p.exists():
txt = p.read_text()
for needle in ['SoftTimeLimitExceeded', 'except Exception', 'Finalize step', 'Autoreply failed', 'soft_time_limit']:
print('---', needle, '---')
for i, line in enumerate(txt.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: suitenumerique/messages
Length of output: 982
🌐 Web query:
Celery SoftTimeLimitExceeded inherits Exception documentation
💡 Result:
The SoftTimeLimitExceeded exception in Celery inherits directly from the Python built-in Exception class [1][2]. As defined in the source code of billiard (the multiprocessing pool implementation used by Celery), the class definition is: class SoftTimeLimitExceeded(Exception): """The soft time limit has been exceeded. This exception is raised to give the task a chance to clean up.""" Because it inherits from Exception rather than BaseException, it can be inadvertently caught by broad "except Exception:" blocks in user code [3]. This behavior has been noted in the Celery community as a potential issue, as it may prevent the exception from propagating correctly and effectively terminating or cleaning up a task as intended [3]. Users are generally advised to ensure their exception handling logic explicitly re-raises SoftTimeLimitExceeded or excludes it from generic catch-all blocks [3].
Citations:
- 1: https://docs.celeryq.dev/en/stable/_modules/billiard/exceptions.html
- 2: https://docs.celeryq.dev/en/latest/_modules/billiard/exceptions.html
- 3: SoftTimeLimitExceeded inherits from Exception celery/billiard#240
Re-raise SoftTimeLimitExceeded in these broad catches.
Both except Exception blocks here can swallow Celery’s soft-timeout exception before the task-level handler runs, so the inbound message won’t be parked cleanly on timeout. src/backend/core/mda/inbound_tasks.py:95-103, 468-471
🤖 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 `@src/backend/core/mda/inbound_tasks.py` around lines 95 - 103, The broad
exception handlers in the inbound task flow are swallowing Celery’s soft-timeout
exception before the task-level timeout handling can run. Update the `except
Exception` blocks in the finalize-step path and the task-level handler in
`inbound_tasks.py` to detect `SoftTimeLimitExceeded` and re-raise it
immediately, while keeping the existing logging for other exceptions. Use the
surrounding `fn()` execution and the inbound-message task handler as the places
to make this change.
Comment on lines
+67
to
+74
| url: z | ||
| .string() | ||
| .min(1, { error: t("URL is required.") }) | ||
| .regex(allowInsecureUrl ? /^https?:\/\//i : /^https:\/\//i, { | ||
| error: allowInsecureUrl | ||
| ? t("URL must start with http:// or https://") | ||
| : t("URL must start with https://"), | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject host-less URLs in the form too.
This schema only checks the scheme prefix, so values like https:///no-host pass client-side validation even though the backend rejects them. Users then fall through to the generic save error path instead of getting an inline field error.
Suggested fix
url: z
.string()
.min(1, { error: t("URL is required.") })
- .regex(allowInsecureUrl ? /^https?:\/\//i : /^https:\/\//i, {
+ .refine((value) => {
+ try {
+ const parsed = new URL(value);
+ const protocolOk = allowInsecureUrl
+ ? ["http:", "https:"].includes(parsed.protocol)
+ : parsed.protocol === "https:";
+ return protocolOk && !!parsed.hostname;
+ } catch {
+ return false;
+ }
+ }, {
error: allowInsecureUrl
? t("URL must start with http:// or https://")
: t("URL must start with https://"),
}),📝 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
| url: z | |
| .string() | |
| .min(1, { error: t("URL is required.") }) | |
| .regex(allowInsecureUrl ? /^https?:\/\//i : /^https:\/\//i, { | |
| error: allowInsecureUrl | |
| ? t("URL must start with http:// or https://") | |
| : t("URL must start with https://"), | |
| }), | |
| url: z | |
| .string() | |
| .min(1, { error: t("URL is required.") }) | |
| .refine((value) => { | |
| try { | |
| const parsed = new URL(value); | |
| const protocolOk = allowInsecureUrl | |
| ? ["http:", "https:"].includes(parsed.protocol) | |
| : parsed.protocol === "https:"; | |
| return protocolOk && !!parsed.hostname; | |
| } catch { | |
| return false; | |
| } | |
| }, { | |
| error: allowInsecureUrl | |
| ? t("URL must start with http:// or https://") | |
| : t("URL must start with https://"), | |
| }), |
🤖 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
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`
around lines 67 - 74, The URL validation in webhook-integration-form.tsx only
checks the scheme prefix, so host-less values like https:///no-host slip through
client-side validation and fail later on save. Update the zod schema for the url
field to also require a valid host/authority (not just http(s)://), keeping the
existing allowInsecureUrl behavior and error messages in the same validation
block so the user gets an inline field error before submit.
jbpenrath
reviewed
Jul 1, 2026
Contributor
There was a problem hiding this comment.
Message model has a post_save signal which trigger a thread reindexation. That means that at each save call here, one opensearch request will be done... furthermore to update data that opensearch doesn't care.
Maybe we could use bulk_update ?
| take = min(batch_size, limit - scanned) | ||
| # Processed rows leave the ``postmark__isnull=True`` set, so the next | ||
| # slice is always fresh work — no cursor/offset needed. | ||
| batch = list(qs.order_by("created_at")[:take]) |
Contributor
There was a problem hiding this comment.
In dry mode this will not work as the qs content does not change as we don't populate postmark field. We never reach if not batch statement so if there is only one message to update, we will loop on the same message until scanned equals to limit.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/backend/core/tests/mda/test_dispatch_webhooks.py (1)
1118-1129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
auth_methodfrom this misconfiguration test.The fixture still sets
auth_method="jwt", so this never exercises the missing-auth-method fail-closed path the test name describes. Drop the field (or make it invalid if you want the unknown-method branch) so the assertion matches the scenario.Suggested fix
models.Channel.objects.create( name="no-auth-method", type=enums.ChannelTypes.WEBHOOK, scope_level=enums.ChannelScopeLevel.MAILBOX, mailbox=mailbox, settings={ "url": "https://hook.example.com", "trigger": "message.delivered", - "auth_method": "jwt", }, encrypted_settings={"secret": "whsec_test"}, )🤖 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 `@src/backend/core/tests/mda/test_dispatch_webhooks.py` around lines 1118 - 1129, The misconfiguration test for the webhook channel is still setting auth_method in the Channel.objects.create fixture, so it does not cover the missing-auth-method fail-closed path it is meant to verify. Update the test setup in test_dispatch_webhooks to remove auth_method from settings for this case, or change it to an invalid value only if you intend to exercise the unknown-method branch, and keep the assertion aligned with the scenario being tested.src/backend/core/mda/dispatch_webhooks.py (1)
1129-1138: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the message still belongs to the mailbox before posting it.
The task revalidates the channel scope, but
message_idis fetched independently. If task args are stale or mismatched, this can send another mailbox’s message blob to a still-valid webhook formailbox_id.Suggested fix
- message = models.Message.objects.filter(id=message_id).first() + message = models.Message.objects.filter( + id=message_id, + thread__accesses__mailbox=mailbox, + ).first() if message is None or message.blob_id is None: logger.warning( "Webhook source message %s missing — skipping dispatch (channel=%s)",🤖 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 `@src/backend/core/mda/dispatch_webhooks.py` around lines 1129 - 1138, The webhook dispatch path in dispatch_webhooks.py re-fetches Message by message_id but never confirms it still belongs to the current mailbox before building the body. Update the logic around the message lookup in the dispatch flow to validate message.mailbox_id matches the mailbox_id for this task, and skip with a warning if it does not. Keep the check near the existing message/blob_id guard and use the same symbols (message_id, mailbox_id, channel_id, _resolve_body_from_message) so stale or mismatched task args cannot post another mailbox’s content.src/backend/core/mda/inbound_pipeline.py (1)
415-417: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the user lookup case-insensitive.
email__in=target_emailscan miss users whose storedUser.emailcasing differs from the webhook payload, so valid assignees get skipped. Use a case-insensitive lookup here 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 `@src/backend/core/mda/inbound_pipeline.py` around lines 415 - 417, The user matching logic in inbound_pipeline should use a case-insensitive email lookup instead of a direct email__in match, since webhook payload casing may differ from stored User.email values and cause valid assignees to be skipped. Update the lookup in the user-fetching path that builds target_emails so it compares emails case-insensitively, while keeping the existing ambiguity handling and skip behavior in place.src/backend/core/signals.py (1)
168-176: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSchedule blob GC after the delete commits.
post_deletecan fire inside a transaction. Queueing GC immediately can race with uncommitted state or survive a rollback; mirror the index handlers and defer the side effect withtransaction.on_commit.🐛 Proposed fix
- schedule_for_gc(instance.blob_id) + blob_id = instance.blob_id + if blob_id is not None: + transaction.on_commit(lambda: schedule_for_gc(blob_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 `@src/backend/core/signals.py` around lines 168 - 176, The blob GC scheduling in schedule_inbound_message_blob_for_gc is happening directly in post_delete, which can run before the surrounding transaction commits. Update this receiver to defer the schedule_for_gc(instance.blob_id) call with transaction.on_commit, matching the existing index handler pattern so the side effect only runs after a successful commit and is skipped on rollback.
♻️ Duplicate comments (1)
docs/webhooks.md (1)
203-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSplit the blocking failure matrix into DROP vs RETRY.
This still says every non-2xx retries, but the dispatcher drops
4xxresponses and only retries transient408/429/5xx, timeouts, and connection errors. Please update the prose/table to match that contract.Suggested fix
- * A **transient** failure — a `4xx`/`5xx`, a timeout, a connection error, a receiver that is temporarily down — is held for **RETRY** and re-fired by the sweep until it recovers (bounded by the deferral window). + * A **client** failure (`4xx`) is **DROP**. + * A **transient** failure — `408`/`429`/`5xx`, a timeout, or a connection error — is held for **RETRY** and re-fired by the sweep until it recovers (bounded by the deferral window). @@ -| Any non-2xx (`4xx`/`5xx`/`3xx`), timeout, conn. | RETRY | Transient — held, re-fired by the sweep, bounded by the deferral window. | +| HTTP `4xx` | DROP | Client error — the receiver rejected the message. | +| HTTP `408`/`429`/`5xx`, timeout, conn. | RETRY | Transient — held, re-fired by the sweep, bounded by the deferral window. |🤖 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 `@docs/webhooks.md` around lines 203 - 229, Update the webhook failure matrix in docs/webhooks.md so the dispatcher contract is split correctly: keep the existing DROP path tied to HTTP 2xx with {"action":"drop"}, but change the non-2xx handling in the prose and table to distinguish DROP-worthy 4xx responses from RETRY-only transient cases. Reference the webhook outcome table and surrounding retry description, and revise the entries to say only 408/429/5xx, timeouts, and connection errors are retried while 4xx responses are dropped per the dispatcher behavior.
🤖 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 `@src/backend/core/admin.py`:
- Around line 1603-1617: The re-queue flow in the abandoned-message loop leaves
abandoned_at set until after process_inbound_message_task.delay() returns, which
lets a fast worker observe the stale abandoned state and skip processing. Update
the admin re-queue logic around the abandoned_message loop in admin.py to clear
abandoned_at and error_message before publishing the task, or otherwise ensure
the worker cannot see the old marker when process_inbound_message_task runs,
while keeping the failure path logged and leaving the row abandoned if publish
fails.
In `@src/backend/core/api/viewsets/inbound/mta.py`:
- Around line 210-217: The Received header construction in the mta metadata
branch can still raise KeyError because it only checks client_helo before
indexing client_hostname and client_address. Update the logic in the
Received-header path to use safe access via mta_metadata.get(...) for
client_helo, client_hostname, and client_address, and only append the header
when all required values are present; otherwise skip adding that header.
In `@src/backend/core/management/commands/backfill_postmark.py`:
- Around line 96-121: The current batching logic in
backfill_postmark.Command.run relies on rows leaving the postmark__isnull
queryset only after a save, so dry-run and exception paths can reprocess the
same records forever. Update the loop in the backfill_postmark management
command to page with a stable cursor based on created_at plus a tie-breaker
(instead of repeatedly slicing qs.order_by("created_at")[:take]), and advance
that cursor after every scanned message regardless of whether dry_run is set or
_postmark_from_stmsg fails. Keep the existing logging and postmark save
behavior, but make forward progress independent of writes so the command remains
bounded and resumable.
In `@src/backend/core/mda/inbound_create.py`:
- Around line 457-462: The queued inbound flow is passing the canonical mailbox
address into _create_message_from_inbound instead of the actual SMTP RCPT TO,
which breaks alias/catch-all handling. Update the task caller that builds the
inbound message context to prefer (inbound_message.envelope or
{}).get("rcpt_to") over ctx.recipient_email, and keep the fallback behavior only
if the envelope value is missing. Use the relevant inbound task / message
construction code path and _create_message_from_inbound as the anchors when
making the change.
In `@src/backend/core/mda/spam.py`:
- Around line 44-47: In the spam rule matching logic around
header_match_regex/header_match, stop lowercasing the regex pattern itself
because it changes regex semantics; only normalize the input header text or do
case-insensitive matching via the regex flags/search call. Also wrap regex
compilation/matching in the spam processing path so malformed configured rules
are caught and skipped with a warning instead of raising and aborting
processing, including the duplicated logic in the later block noted in the
review.
In `@src/backend/core/migrations/0032_inboundmessage_blob_envelope.py`:
- Around line 17-26: The migration currently imports core.models.Blob and calls
Blob.objects.create_blob, which is not migration-safe because BlobManager is not
guaranteed to be available or stable during migrations. Update the
0032_inboundmessage_blob_envelope migration to avoid live model imports and
instead use a migration-safe approach in the same function that processes
InboundMessage rows, either by inlining the minimal blob creation/dedup logic or
by using a helper that is safe to run in migrations. Ensure the logic still
creates the blob content for each inbound message without depending on
Blob.objects.
In `@src/backend/core/models.py`:
- Around line 2196-2209: The STMsg header projection in get_stmsg_headers() is
missing postmark["rcpt_to"], so divergent RCPT/BCC/alias data recorded by
_record_divergent_rcpt() gets dropped. Update the existing postmark overlay in
src/backend/core/models.py to include rcpt_to alongside sender-auth,
processing-failed, and spam, using the same postmark dict handling pattern and
preserving the structured value surfaced by the model.
In `@src/backend/core/services/ssrf.py`:
- Around line 253-260: The redirect handling in ssrf.py currently forwards
Authorization headers and POST bodies verbatim across hosts in the webhook
dispatch path, which should not happen by default. Update the redirect logic
around the SSRF/session flow to strip sensitive headers and avoid resending the
original body when the redirect target changes host, while preserving same-host
behavior only if explicitly intended. Use the existing SSRF validation and
redirect/session methods in the relevant webhook request path to locate and
adjust the cross-host redirect forwarding behavior.
In `@src/backend/core/tests/commands/test_backfill_postmark.py`:
- Around line 44-51: The dry-run test in test_dry_run_writes_nothing only
verifies that Message.postmark stays unset, so it misses the batching bug in
backfill_postmark.handle() where the same row can be reprocessed repeatedly.
Update this test to capture stdout and assert the reported scanned count is 1
for a single MessageFactory input, using call_command("backfill_postmark",
"--dry-run", stdout=...). Also add a separate case around the unreadable-blob
path to assert the errors counter is incremented as expected.
---
Outside diff comments:
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 1129-1138: The webhook dispatch path in dispatch_webhooks.py
re-fetches Message by message_id but never confirms it still belongs to the
current mailbox before building the body. Update the logic around the message
lookup in the dispatch flow to validate message.mailbox_id matches the
mailbox_id for this task, and skip with a warning if it does not. Keep the check
near the existing message/blob_id guard and use the same symbols (message_id,
mailbox_id, channel_id, _resolve_body_from_message) so stale or mismatched task
args cannot post another mailbox’s content.
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 415-417: The user matching logic in inbound_pipeline should use a
case-insensitive email lookup instead of a direct email__in match, since webhook
payload casing may differ from stored User.email values and cause valid
assignees to be skipped. Update the lookup in the user-fetching path that builds
target_emails so it compares emails case-insensitively, while keeping the
existing ambiguity handling and skip behavior in place.
In `@src/backend/core/signals.py`:
- Around line 168-176: The blob GC scheduling in
schedule_inbound_message_blob_for_gc is happening directly in post_delete, which
can run before the surrounding transaction commits. Update this receiver to
defer the schedule_for_gc(instance.blob_id) call with transaction.on_commit,
matching the existing index handler pattern so the side effect only runs after a
successful commit and is skipped on rollback.
In `@src/backend/core/tests/mda/test_dispatch_webhooks.py`:
- Around line 1118-1129: The misconfiguration test for the webhook channel is
still setting auth_method in the Channel.objects.create fixture, so it does not
cover the missing-auth-method fail-closed path it is meant to verify. Update the
test setup in test_dispatch_webhooks to remove auth_method from settings for
this case, or change it to an invalid value only if you intend to exercise the
unknown-method branch, and keep the assertion aligned with the scenario being
tested.
---
Duplicate comments:
In `@docs/webhooks.md`:
- Around line 203-229: Update the webhook failure matrix in docs/webhooks.md so
the dispatcher contract is split correctly: keep the existing DROP path tied to
HTTP 2xx with {"action":"drop"}, but change the non-2xx handling in the prose
and table to distinguish DROP-worthy 4xx responses from RETRY-only transient
cases. Reference the webhook outcome table and surrounding retry description,
and revise the entries to say only 408/429/5xx, timeouts, and connection errors
are retried while 4xx responses are dropped per the dispatcher 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00b6ae79-1515-4954-8fe3-fd66beced24d
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**
📒 Files selected for processing (52)
docs/env.mddocs/tiered-storage.mddocs/webhooks.mdsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/inbound/mta.pysrc/backend/core/api/viewsets/inbound/widget.pysrc/backend/core/enums.pysrc/backend/core/management/commands/backfill_postmark.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_auth.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/outbound.pysrc/backend/core/mda/selfcheck.pysrc/backend/core/mda/spam.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/migrations/0032_inboundmessage_blob_envelope.pysrc/backend/core/models.pysrc/backend/core/services/ssrf.pysrc/backend/core/signals.pysrc/backend/core/templates/admin/core/channel/change_form.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_inbound_mta.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/commands/test_backfill_postmark.pysrc/backend/core/tests/mda/test_autoreply.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/mda/test_inbound_auth.pysrc/backend/core/tests/mda/test_inbound_e2e.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_retry.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/core/tests/models/test_blob.pysrc/backend/core/tests/models/test_message_get_parsed_data.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
Comment on lines
+1603
to
+1617
| for inbound_message in queryset.filter(abandoned_at__isnull=False): | ||
| # Publish to the broker *before* clearing the terminal markers: if | ||
| # the publish fails, leave the row abandoned (with its error) so the | ||
| # operator can retry, rather than erasing the state without a re-queue. | ||
| try: | ||
| process_inbound_message_task.delay(str(inbound_message.id)) | ||
| except Exception: # pylint: disable=broad-except | ||
| logging.exception( | ||
| "Failed to re-queue abandoned inbound message %s", | ||
| inbound_message.id, | ||
| ) | ||
| continue | ||
| inbound_message.abandoned_at = None | ||
| inbound_message.error_message = "" | ||
| inbound_message.save(update_fields=["abandoned_at", "error_message"]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the abandoned marker before the worker can observe the row.
Line 1608 publishes the Celery task while abandoned_at is still set. A fast worker can hit process_inbound_message_task’s abandoned guard and exit before Lines 1615-1617 clear the marker, leaving the row live but not actually replayed.
Proposed fix
replayed = 0
for inbound_message in queryset.filter(abandoned_at__isnull=False):
- # Publish to the broker *before* clearing the terminal markers: if
- # the publish fails, leave the row abandoned (with its error) so the
- # operator can retry, rather than erasing the state without a re-queue.
+ previous_abandoned_at = inbound_message.abandoned_at
+ previous_error_message = inbound_message.error_message
+ inbound_message.abandoned_at = None
+ inbound_message.error_message = ""
+ inbound_message.save(update_fields=["abandoned_at", "error_message"])
try:
process_inbound_message_task.delay(str(inbound_message.id))
except Exception: # pylint: disable=broad-except
+ inbound_message.abandoned_at = previous_abandoned_at
+ inbound_message.error_message = previous_error_message
+ inbound_message.save(update_fields=["abandoned_at", "error_message"])
logging.exception(
"Failed to re-queue abandoned inbound message %s",
inbound_message.id,
)
continue
- inbound_message.abandoned_at = None
- inbound_message.error_message = ""
- inbound_message.save(update_fields=["abandoned_at", "error_message"])
replayed += 1📝 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
| for inbound_message in queryset.filter(abandoned_at__isnull=False): | |
| # Publish to the broker *before* clearing the terminal markers: if | |
| # the publish fails, leave the row abandoned (with its error) so the | |
| # operator can retry, rather than erasing the state without a re-queue. | |
| try: | |
| process_inbound_message_task.delay(str(inbound_message.id)) | |
| except Exception: # pylint: disable=broad-except | |
| logging.exception( | |
| "Failed to re-queue abandoned inbound message %s", | |
| inbound_message.id, | |
| ) | |
| continue | |
| inbound_message.abandoned_at = None | |
| inbound_message.error_message = "" | |
| inbound_message.save(update_fields=["abandoned_at", "error_message"]) | |
| for inbound_message in queryset.filter(abandoned_at__isnull=False): | |
| previous_abandoned_at = inbound_message.abandoned_at | |
| previous_error_message = inbound_message.error_message | |
| inbound_message.abandoned_at = None | |
| inbound_message.error_message = "" | |
| inbound_message.save(update_fields=["abandoned_at", "error_message"]) | |
| try: | |
| process_inbound_message_task.delay(str(inbound_message.id)) | |
| except Exception: # pylint: disable=broad-except | |
| inbound_message.abandoned_at = previous_abandoned_at | |
| inbound_message.error_message = previous_error_message | |
| inbound_message.save(update_fields=["abandoned_at", "error_message"]) | |
| logging.exception( | |
| "Failed to re-queue abandoned inbound message %s", | |
| inbound_message.id, | |
| ) | |
| continue | |
| replayed += 1 |
🤖 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 `@src/backend/core/admin.py` around lines 1603 - 1617, The re-queue flow in the
abandoned-message loop leaves abandoned_at set until after
process_inbound_message_task.delay() returns, which lets a fast worker observe
the stale abandoned state and skip processing. Update the admin re-queue logic
around the abandoned_message loop in admin.py to clear abandoned_at and
error_message before publishing the task, or otherwise ensure the worker cannot
see the old marker when process_inbound_message_task runs, while keeping the
failure path logged and leaving the row abandoned if publish fails.
Comment on lines
+210
to
+217
| if "client_helo" in mta_metadata: | ||
| prepend_headers = [ | ||
| prepend_headers.append( | ||
| ( | ||
| "Received", | ||
| f"from {mta_metadata['client_helo']} (" | ||
| + f"{mta_metadata['client_hostname']} [{mta_metadata['client_address']}]);", | ||
| ), | ||
| ] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid KeyError on partial MTA metadata.
This branch only checks client_helo, but then indexes client_hostname and client_address. Use .get() and omit the Received header unless the required parts are present.
🐛 Proposed fix
- if "client_helo" in mta_metadata:
+ client_helo = mta_metadata.get("client_helo")
+ client_hostname = mta_metadata.get("client_hostname")
+ client_address = mta_metadata.get("client_address")
+ if client_helo and client_address:
prepend_headers.append(
(
"Received",
- f"from {mta_metadata['client_helo']} ("
- + f"{mta_metadata['client_hostname']} [{mta_metadata['client_address']}]);",
+ f"from {client_helo} ({client_hostname or 'unknown'} [{client_address}]);",
)
)📝 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 "client_helo" in mta_metadata: | |
| prepend_headers = [ | |
| prepend_headers.append( | |
| ( | |
| "Received", | |
| f"from {mta_metadata['client_helo']} (" | |
| + f"{mta_metadata['client_hostname']} [{mta_metadata['client_address']}]);", | |
| ), | |
| ] | |
| ) | |
| ) | |
| client_helo = mta_metadata.get("client_helo") | |
| client_hostname = mta_metadata.get("client_hostname") | |
| client_address = mta_metadata.get("client_address") | |
| if client_helo and client_address: | |
| prepend_headers.append( | |
| ( | |
| "Received", | |
| f"from {client_helo} ({client_hostname or 'unknown'} [{client_address}]);", | |
| ) | |
| ) |
🤖 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 `@src/backend/core/api/viewsets/inbound/mta.py` around lines 210 - 217, The
Received header construction in the mta metadata branch can still raise KeyError
because it only checks client_helo before indexing client_hostname and
client_address. Update the logic in the Received-header path to use safe access
via mta_metadata.get(...) for client_helo, client_hostname, and client_address,
and only append the header when all required values are present; otherwise skip
adding that header.
Comment on lines
+96
to
+121
| while scanned < limit: | ||
| take = min(batch_size, limit - scanned) | ||
| # Processed rows leave the ``postmark__isnull=True`` set, so the next | ||
| # slice is always fresh work — no cursor/offset needed. | ||
| batch = list(qs.order_by("created_at")[:take]) | ||
| if not batch: | ||
| break | ||
|
|
||
| for message in batch: | ||
| scanned += 1 | ||
| try: | ||
| postmark = _postmark_from_stmsg(message.get_stmsg_headers()) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| # A single unreadable/corrupt blob must not abort the run. | ||
| errors += 1 | ||
| logger.exception("backfill_postmark: failed to read %s", message.id) | ||
| continue | ||
|
|
||
| if postmark: | ||
| populated += 1 | ||
| if dry_run: | ||
| continue | ||
| # ``{}`` marks the row scanned (won't be re-read) and reads back | ||
| # the same as NULL through ``get_stmsg_headers``. | ||
| message.postmark = postmark | ||
| message.save(update_fields=["postmark"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Batching invariant breaks for --dry-run and error rows — no real forward progress.
The loop assumes every processed row leaves the postmark__isnull=True set (per the comment on lines 98-99), but that's only true when a write actually happens:
--dry-run: line 116-117 never persists, soqs.order_by("created_at")[:take]returns the identical topbatch_sizerows every iteration. The command spins up to--limittimes reprocessing the same small window instead of surveying the backlog, and reports ascannedcount that vastly overstates what was actually examined. This is exactly what happens intest_dry_run_writes_nothing(seesrc/backend/core/tests/commands/test_backfill_postmark.pylines 44-51): with one message and defaultlimit=10000, the same row gets read and parsed 10000 times.- Errors: line 108-112 also skips the write on exception, so a message whose blob is corrupt/unreadable will remain the oldest
postmark IS NULLrow forever — it will keep occupying one batch slot and re-triggeringlogger.exceptionon every future run until someone manually intervenes.
Both defeat the "bounded, resumable, run-from-cron" design goal stated in the module docstring.
🐛 Proposed fix: paginate with a stable cursor instead of relying on writes to shrink the queryset
+from django.db.models import Q
+
...
- while scanned < limit:
- take = min(batch_size, limit - scanned)
- # Processed rows leave the ``postmark__isnull=True`` set, so the next
- # slice is always fresh work — no cursor/offset needed.
- batch = list(qs.order_by("created_at")[:take])
- if not batch:
- break
-
- for message in batch:
+ cursor = None # (created_at, id) of the last row examined this run
+ while scanned < limit:
+ take = min(batch_size, limit - scanned)
+ batch_qs = qs.order_by("created_at", "id")
+ if cursor is not None:
+ batch_qs = batch_qs.filter(
+ Q(created_at__gt=cursor[0])
+ | Q(created_at=cursor[0], id__gt=cursor[1])
+ )
+ batch = list(batch_qs[:take])
+ if not batch:
+ break
+ cursor = (batch[-1].created_at, batch[-1].id)
+
+ for message in batch:This makes forward progress independent of whether a write happened, and also fixes tie-breaking for rows sharing the same created_at.
📝 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
| while scanned < limit: | |
| take = min(batch_size, limit - scanned) | |
| # Processed rows leave the ``postmark__isnull=True`` set, so the next | |
| # slice is always fresh work — no cursor/offset needed. | |
| batch = list(qs.order_by("created_at")[:take]) | |
| if not batch: | |
| break | |
| for message in batch: | |
| scanned += 1 | |
| try: | |
| postmark = _postmark_from_stmsg(message.get_stmsg_headers()) | |
| except Exception: # pylint: disable=broad-exception-caught | |
| # A single unreadable/corrupt blob must not abort the run. | |
| errors += 1 | |
| logger.exception("backfill_postmark: failed to read %s", message.id) | |
| continue | |
| if postmark: | |
| populated += 1 | |
| if dry_run: | |
| continue | |
| # ``{}`` marks the row scanned (won't be re-read) and reads back | |
| # the same as NULL through ``get_stmsg_headers``. | |
| message.postmark = postmark | |
| message.save(update_fields=["postmark"]) | |
| cursor = None # (created_at, id) of the last row examined this run | |
| while scanned < limit: | |
| take = min(batch_size, limit - scanned) | |
| batch_qs = qs.order_by("created_at", "id") | |
| if cursor is not None: | |
| batch_qs = batch_qs.filter( | |
| Q(created_at__gt=cursor[0]) | |
| | Q(created_at=cursor[0], id__gt=cursor[1]) | |
| ) | |
| batch = list(batch_qs[:take]) | |
| if not batch: | |
| break | |
| cursor = (batch[-1].created_at, batch[-1].id) | |
| for message in batch: | |
| scanned += 1 | |
| try: | |
| postmark = _postmark_from_stmsg(message.get_stmsg_headers()) | |
| except Exception: # pylint: disable=broad-exception-caught | |
| # A single unreadable/corrupt blob must not abort the run. | |
| errors += 1 | |
| logger.exception("backfill_postmark: failed to read %s", message.id) | |
| continue | |
| if postmark: | |
| populated += 1 | |
| if dry_run: | |
| continue | |
| # ``{}`` marks the row scanned (won't be re-read) and reads back | |
| # the same as NULL through ``get_stmsg_headers``. | |
| message.postmark = postmark | |
| message.save(update_fields=["postmark"]) |
🤖 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 `@src/backend/core/management/commands/backfill_postmark.py` around lines 96 -
121, The current batching logic in backfill_postmark.Command.run relies on rows
leaving the postmark__isnull queryset only after a save, so dry-run and
exception paths can reprocess the same records forever. Update the loop in the
backfill_postmark management command to page with a stable cursor based on
created_at plus a tie-breaker (instead of repeatedly slicing
qs.order_by("created_at")[:take]), and advance that cursor after every scanned
message regardless of whether dry_run is set or _postmark_from_stmsg fails. Keep
the existing logging and postmark save behavior, but make forward progress
independent of writes so the command remains bounded and resumable.
Comment on lines
+457
to
+462
| # Per-recipient postmark: record the envelope RCPT TO only when it | ||
| # diverges from the MIME To/Cc (BCC / alias / catch-all) — the | ||
| # "you were BCC'd / reached via <alias>" signal. Structured, never | ||
| # in the bytes, so it can't break blob dedup across recipients. | ||
| postmark = dict(postmark or {}) | ||
| _record_divergent_rcpt(postmark, recipient_email, parsed_email) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the queued envelope RCPT, not the canonical mailbox address.
The queued inbound path passes ctx.recipient_email from str(mailbox) into _create_message_from_inbound, so alias/catch-all deliveries can record the mailbox address instead of the real SMTP rcpt_to. Prefer (inbound_message.envelope or {}).get("rcpt_to") when calling this function from the task.
🐛 Proposed fix in the task caller
+actual_recipient = (inbound_message.envelope or {}).get("rcpt_to") or ctx.recipient_email
inbound_msg = _create_message_from_inbound(
- recipient_email=ctx.recipient_email,
+ recipient_email=actual_recipient,
parsed_email=ctx.parsed_email,
raw_data=ctx.raw_data,🤖 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 `@src/backend/core/mda/inbound_create.py` around lines 457 - 462, The queued
inbound flow is passing the canonical mailbox address into
_create_message_from_inbound instead of the actual SMTP RCPT TO, which breaks
alias/catch-all handling. Update the task caller that builds the inbound message
context to prefer (inbound_message.envelope or {}).get("rcpt_to") over
ctx.recipient_email, and keep the fallback behavior only if the envelope value
is missing. Use the relevant inbound task / message construction code path and
_create_message_from_inbound as the anchors when making the change.
Comment on lines
+44
to
+47
| key, value = header_match.split(":", 1) | ||
| key = key.lower().strip() | ||
| value = value.lower().strip() | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve regex semantics and handle invalid regex rules.
Lowercasing header_match_regex changes regex tokens and character classes before matching. Also, a malformed configured regex currently raises and can abort spam processing instead of skipping the bad rule.
Suggested fix
key, value = header_match.split(":", 1)
key = key.lower().strip()
- value = value.lower().strip()
+ value = value.strip()
@@
header_value = (
found_value.lower().strip()
if isinstance(found_value, str)
else str(found_value).lower().strip()
)
if rule.get("header_match"):
- is_match = header_value == value
+ is_match = header_value == value.lower()
else: # header_match_regex
- is_match = re.fullmatch(value, header_value) is not None
+ try:
+ is_match = re.fullmatch(value, header_value, re.IGNORECASE) is not None
+ except re.error:
+ logger.warning("Invalid header_match_regex in spam rule #%d", idx)
+ continueAlso applies to: 88-91
🤖 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 `@src/backend/core/mda/spam.py` around lines 44 - 47, In the spam rule matching
logic around header_match_regex/header_match, stop lowercasing the regex pattern
itself because it changes regex semantics; only normalize the input header text
or do case-insensitive matching via the regex flags/search call. Also wrap regex
compilation/matching in the spam processing path so malformed configured rules
are caught and skipped with a warning instead of raising and aborting
processing, including the duplicated logic in the later block noted in the
review.
Comment on lines
+17
to
+26
| # pylint: disable=import-outside-toplevel | ||
| from core.models import Blob | ||
|
|
||
| InboundMessage = apps.get_model("core", "inboundmessage") | ||
| rows = InboundMessage.objects.filter(blob__isnull=True).exclude(raw_data=None) | ||
| for inbound in rows.iterator(): | ||
| raw = bytes(inbound.raw_data) if inbound.raw_data else b"" | ||
| if not raw: | ||
| continue | ||
| blob = Blob.objects.create_blob(content=raw, content_type="message/rfc822") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Blob's manager is migration-safe and find current-model imports in migrations.
rg -n "class .*Blob.*Manager|use_in_migrations|def create_blob|from core.models import Blob|core.models import" src/backend/coreRepository: suitenumerique/messages
Length of output: 5962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect BlobManager, Blob, and the migration in question.
sed -n '2360,2465p' src/backend/core/models.py
printf '\n---- MIGRATION ----\n'
sed -n '1,120p' src/backend/core/migrations/0032_inboundmessage_blob_envelope.py
printf '\n---- SEARCH use_in_migrations / migrations-safe manager ----\n'
rg -n "use_in_migrations|class BlobManager|objects = BlobManager|create_blob\(" src/backend/core/models.py src/backend/core/migrationsRepository: suitenumerique/messages
Length of output: 7964
Avoid importing core.models.Blob in this migration
BlobManager isn’t marked use_in_migrations, so Blob.objects.create_blob() still depends on the live model/manager shape. Use a migration-safe helper or inline the minimal blob insert/dedup logic here.
🤖 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 `@src/backend/core/migrations/0032_inboundmessage_blob_envelope.py` around
lines 17 - 26, The migration currently imports core.models.Blob and calls
Blob.objects.create_blob, which is not migration-safe because BlobManager is not
guaranteed to be available or stable during migrations. Update the
0032_inboundmessage_blob_envelope migration to avoid live model imports and
instead use a migration-safe approach in the same function that processes
InboundMessage rows, either by inlining the minimal blob creation/dedup logic or
by using a helper that is safe to run in migrations. Ensure the logic still
creates the blob content for each inbound message without depending on
Blob.objects.
Comment on lines
+2196
to
+2209
| postmark = self.postmark or {} | ||
| if postmark.get("auth"): | ||
| # ``sender-auth`` carries the verdict value verbatim ("none"/"fail"), | ||
| # matching the legacy header. | ||
| result["sender-auth"] = postmark["auth"] | ||
| if postmark.get("processing"): | ||
| # ``processing-failed`` is a boolean flag; normalise to the legacy | ||
| # header value ("true") regardless of the structured reason. | ||
| result["processing-failed"] = "true" | ||
| if postmark.get("spam"): | ||
| # rspamd flagged probable spam but below the Junk threshold — the | ||
| # UI renders a "suspected spam" marker while the message stays in | ||
| # the inbox. | ||
| result["spam"] = postmark["spam"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Project postmark["rcpt_to"] into STMsg headers.
Line 2094 documents rcpt_to as structured postmark data surfaced through get_stmsg_headers(), but this overlay only emits sender-auth, processing-failed, and spam. Divergent RCPT/BCC/alias information recorded by _record_divergent_rcpt() is therefore dropped at the model boundary.
Proposed fix
if postmark.get("spam"):
# rspamd flagged probable spam but below the Junk threshold — the
# UI renders a "suspected spam" marker while the message stays in
# the inbox.
result["spam"] = postmark["spam"]
+ if postmark.get("rcpt_to"):
+ result["rcpt-to"] = postmark["rcpt_to"]
return result📝 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
| postmark = self.postmark or {} | |
| if postmark.get("auth"): | |
| # ``sender-auth`` carries the verdict value verbatim ("none"/"fail"), | |
| # matching the legacy header. | |
| result["sender-auth"] = postmark["auth"] | |
| if postmark.get("processing"): | |
| # ``processing-failed`` is a boolean flag; normalise to the legacy | |
| # header value ("true") regardless of the structured reason. | |
| result["processing-failed"] = "true" | |
| if postmark.get("spam"): | |
| # rspamd flagged probable spam but below the Junk threshold — the | |
| # UI renders a "suspected spam" marker while the message stays in | |
| # the inbox. | |
| result["spam"] = postmark["spam"] | |
| postmark = self.postmark or {} | |
| if postmark.get("auth"): | |
| # ``sender-auth`` carries the verdict value verbatim ("none"/"fail"), | |
| # matching the legacy header. | |
| result["sender-auth"] = postmark["auth"] | |
| if postmark.get("processing"): | |
| # ``processing-failed`` is a boolean flag; normalise to the legacy | |
| # header value ("true") regardless of the structured reason. | |
| result["processing-failed"] = "true" | |
| if postmark.get("spam"): | |
| # rspamd flagged probable spam but below the Junk threshold — the | |
| # UI renders a "suspected spam" marker while the message stays in | |
| # the inbox. | |
| result["spam"] = postmark["spam"] | |
| if postmark.get("rcpt_to"): | |
| result["rcpt-to"] = postmark["rcpt_to"] |
🤖 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 `@src/backend/core/models.py` around lines 2196 - 2209, The STMsg header
projection in get_stmsg_headers() is missing postmark["rcpt_to"], so divergent
RCPT/BCC/alias data recorded by _record_divergent_rcpt() gets dropped. Update
the existing postmark overlay in src/backend/core/models.py to include rcpt_to
alongside sender-auth, processing-failed, and spam, using the same postmark dict
handling pattern and preserving the structured value surfaced by the model.
Comment on lines
+253
to
+260
| ``kwargs`` (including any ``Authorization`` header and the POST body) | ||
| are re-sent verbatim on every hop, so a cross-host redirect forwards | ||
| the caller's credentials + payload to the redirect target. This is | ||
| intentional for webhook delivery — a receiver that 3xx-redirects (LB / | ||
| canonicaliser) must still get the signed body and its auth — and safe | ||
| because every hop is SSRF-validated to a public host and an HTTPS→HTTP | ||
| downgrade is refused. Callers that must not leak credentials across | ||
| hosts should not send a bearer credential through this session. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not forward webhook credentials and email bodies across hosts by default.
Line 253 documents that Authorization and the POST body are re-sent verbatim on every redirect. For webhook dispatch, that means a receiver-side open redirect can exfiltrate the bearer token plus full email payload to any public host; SSRF validation does not mitigate that confidentiality leak.
Suggested fix
next_url = urljoin(current_url, location)
+ current_parsed = urlparse(current_url)
+ next_parsed = urlparse(next_url)
# Refuse an HTTPS→HTTP downgrade: a redirect must not silently drop
# the connection from TLS to cleartext. Same-scheme or an HTTP→HTTPS
# upgrade is fine.
if (
- urlparse(current_url).scheme == "https"
- and urlparse(next_url).scheme == "http"
+ current_parsed.scheme == "https"
+ and next_parsed.scheme == "http"
):
response.close()
raise SSRFValidationError(
"Refusing to follow HTTPS→HTTP redirect downgrade"
)
+ if current_parsed.hostname != next_parsed.hostname and (
+ kwargs.get("data") is not None
+ or kwargs.get("json") is not None
+ or (kwargs.get("headers") or {}).get("Authorization")
+ ):
+ response.close()
+ raise SSRFValidationError(
+ "Refusing to forward credentials/body across hosts"
+ )
response.close()
current_url = next_urlAlso applies to: 275-277
🤖 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 `@src/backend/core/services/ssrf.py` around lines 253 - 260, The redirect
handling in ssrf.py currently forwards Authorization headers and POST bodies
verbatim across hosts in the webhook dispatch path, which should not happen by
default. Update the redirect logic around the SSRF/session flow to strip
sensitive headers and avoid resending the original body when the redirect target
changes host, while preserving same-host behavior only if explicitly intended.
Use the existing SSRF validation and redirect/session methods in the relevant
webhook request path to locate and adjust the cross-host redirect forwarding
behavior.
Comment on lines
+44
to
+51
| def test_dry_run_writes_nothing(self): | ||
| """--dry-run reports but does not persist.""" | ||
| message = factories.MessageFactory(raw_mime=_WITH_VERDICTS) | ||
|
|
||
| call_command("backfill_postmark", "--dry-run", stdout=StringIO()) | ||
|
|
||
| message.refresh_from_db() | ||
| assert message.postmark is None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Coverage gap masks the dry-run batching bug.
This test only checks message.postmark is None; it doesn't assert on the command's reported scanned count or iteration behavior, so it passes even though (per the root-cause issue flagged on backfill_postmark.py's handle()) the command actually reprocesses this single row up to --limit (10000) times in dry-run mode. Once the batching fix is applied, consider also capturing stdout and asserting scanned == 1 here to guard against regressions, and adding a case for the errors counter (a message whose blob is unreadable).
🤖 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 `@src/backend/core/tests/commands/test_backfill_postmark.py` around lines 44 -
51, The dry-run test in test_dry_run_writes_nothing only verifies that
Message.postmark stays unset, so it misses the batching bug in
backfill_postmark.handle() where the same row can be reprocessed repeatedly.
Update this test to capture stdout and assert the reported scanned count is 1
for a single MessageFactory input, using call_command("backfill_postmark",
"--dry-run", stdout=...). Also add a separate case around the unreadable-blob
path to assert the errors counter is incremented as expected.
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
3 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation