Development#728
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (125)
📝 WalkthroughWalkthroughThis PR adds a step-based inbound message pipeline with blob-backed storage and structured envelope/postmark metadata, introduces outbound webhook dispatch, replaces channel API-key rotation with generic secret rotation, and adds a pure-Python inbound MTA implementation with matching docs, settings, UI, and tests. ChangesInbound Pipeline, Webhooks, and Channel Secrets
Estimated code review effort: 5 (Critical) | ~150 minutes Pure-Python Inbound MTA (pymta)
Estimated code review effort: 5 (Critical) | ~120 minutes Frontend Webhook Integration UI & Email Exporter
Estimated code review effort: 3 (Moderate) | ~30 minutes Documentation and Environment Configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Unrelated Test Formatting and Tooling Cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jbpenrath
marked this pull request as ready for review
July 6, 2026 07:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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/tests/commands/test_backfill_postmark.py (1)
20-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
--before.No test exercises the
--beforecutoff path. This is the exact code path with the naive/aware datetime issue flagged inbackfill_postmark.py; a test passing a full ISO datetime without an offset would have caught it.🤖 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 20 - 113, Add a test in TestBackfillPostmark that exercises the backfill_postmark command’s --before cutoff path, since no current case covers it. Create a MessageFactory row that should be excluded by the cutoff, invoke call_command("backfill_postmark", ...) with a full ISO datetime string lacking an offset, and assert the row is not backfilled. Use the backfill_postmark command path and the existing call_command/message.refresh_from_db pattern so the naive-vs-aware datetime handling in the cutoff logic is covered.src/backend/core/admin.py (1)
583-645: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a change-permission check before rotating the secret.
admin_viewonly gates staff access, so a staff user withoutChannelchange rights can still hit this URL directly and rotate an arbitrary channel’s secret.🔒 Suggested fix
channel = self.get_object(request, object_id) if channel is None: messages.error(request, "Channel not found.") return redirect("..") + if not self.has_change_permission(request, channel): + messages.error(request, "You do not have permission to do that.") + return redirect("..") if channel.type not in (ChannelTypes.API_KEY, ChannelTypes.WEBHOOK):🤖 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 - 645, The regenerate_secret_view flow is missing a permission gate, so a staff user without Channel change rights can still rotate secrets by calling it directly. Add an explicit change-permission check in regenerate_secret_view, before get_object/rotate_secret, using the admin’s existing permission helpers for the Channel model, and return a denied response or redirect if the user lacks change access. Keep the fix localized to regenerate_secret_view and preserve the existing POST/type/auth_method validations.docs/env.md (1)
341-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing doc entry for
MESSAGES_INBOUND_DEFERRAL_MAX_AGE.This PR introduces
MESSAGES_INBOUND_DEFERRAL_MAX_AGEinsettings.py(48h default, governs the RETRY/deferral window heavily documented indocs/webhooks.md), but it isn't added to this Business Logic table alongside the other new rows (MESSAGES_ALLOW_INTERNAL_DELIVERY,MESSAGES_MAILBOX_LOCALPART_DENYLIST_PERSONAL). Given how central this setting is to the new webhook retry/deferral semantics, operators should be able to find it here.📝 Suggested addition
| `MESSAGES_ALLOW_INTERNAL_DELIVERY` | `True` | Deliver mailbox-to-mailbox mail through the internal inbound pipeline (fast path). Set `False` to force same-instance mail out through the external MTA so it passes the same scanning/archiving as outbound. | Optional | +| `MESSAGES_INBOUND_DEFERRAL_MAX_AGE` | `172800` | Max time (seconds) an inbound message is held/retried when a processing step (spam check, blocking webhook) keeps failing before it's delivered anyway, flagged `X-StMsg-Processing-Failed` (48h) | Optional | | `MESSAGES_MAILBOX_LOCALPART_DENYLIST_PERSONAL` | `[]` | Local parts rejected for personal mailboxes (case-insensitive exact match). | Optional |🤖 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/env.md` around lines 341 - 356, The Business Logic settings table is missing the new MESSAGES_INBOUND_DEFERRAL_MAX_AGE entry, so add a row for it alongside the other MESSAGES_* options in docs/env.md. Use the same style as the existing rows, and ensure the description matches its purpose in settings.py and docs/webhooks.md: the inbound deferral/retry window with a 48-hour default. Keep the naming consistent with nearby symbols like MESSAGES_ALLOW_INTERNAL_DELIVERY and MESSAGES_MAILBOX_LOCALPART_DENYLIST_PERSONAL so operators can find the setting in one place.
🤖 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 `@compose.yaml`:
- Around line 288-346: The pymta services are still configured to bind SMTP on
port 25 while running as UID 65532 with all capabilities dropped, so the
listener can fail to start under the hardened setup. Update the mta-in-py and
mta-in-py-test service definitions to either restore NET_BIND_SERVICE in their
capability settings or change the pymta and test port configuration to a
non-privileged port, and make sure the relevant environment values and port
mappings stay consistent with the chosen approach.
In `@src/backend/core/management/commands/backfill_postmark.py`:
- Around line 78-91: The `handle` flow in `backfill_postmark` can crash with an
unhandled `ValueError` when `options["before"]` is malformed and both
`parse_datetime()` and `datetime.fromisoformat()` fail. Update the `before`
parsing branch in `handle` to catch invalid input and raise a `CommandError`
instead, using the existing `cutoff` parsing logic and the `options["before"]`
value to report the bad argument cleanly.
In `@src/backend/core/templates/admin/core/channel/change_form.html`:
- Line 10: The conditional in the channel change form template is relying on
implicit and/or precedence, so make it explicit for readability. Update the
condition around the `original.type` check in `change_form.html` to group the
`api_key` and `webhook` cases clearly, keeping the same behavior but making the
logic easier to scan and verify.
In `@src/backend/messages/settings.py`:
- Around line 381-384: Make MDA_HEADER_XMAILER env-configurable like the other
settings in the settings class by replacing the bare string with a
values.Value-backed setting so deployments can override the outbound X-Mailer
product name; update the Messages settings definition alongside MDA_API_SECRET,
INSTANCE_URL, and DJANGO_EMAIL_BRAND_NAME, and ensure compose_and_sign_mime
continues to read MDA_HEADER_XMAILER from the configurable setting.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 168-182: The onRegenerate handler in webhook-integration-form.tsx
triggers secret regeneration immediately, which can invalidate the current
webhook credential on accidental clicks. Add a confirmation gate before calling
regenerateMutation.mutateAsync, using the same destructive-action pattern as
handleDelete with modals.deleteConfirmationModal. Keep the existing success flow
intact by only calling showCredential after the user confirms and the mutation
succeeds, and preserve the current error handling in the catch block.
In `@src/mta-in/Dockerfile.pymta`:
- Around line 101-119: The runtime setup in runtime-base drops privileges with
USER ${DOCKER_USER} but still relies on the default PYMTA_SMTP_PORT of 25, which
a non-root process cannot bind. Update the Dockerfile.pymta runtime path so the
pymta entrypoint runs on a non-privileged SMTP port or explicitly grants
CAP_NET_BIND_SERVICE to the runtime image, making sure the fix applies to the
runtime-base and any production stages that inherit it.
In `@src/mta-in/src/pymta/address.py`:
- Around line 162-184: The domain validation in address parsing is checking
label length on the raw UTF-8 text, which can incorrectly reject valid IDN
labels. Update the domain-label validation in address.py so the length check is
performed on each label’s IDNA A-label (punycode) form before enforcing the
63-octet limit, while keeping the existing malformed-domain and address-literal
checks in the same domain-shape logic.
In `@src/mta-in/src/pymta/handler.py`:
- Around line 206-215: The RCPT hard-error-limit path in handle_RCPT only
returns a 421 response and does not actually terminate the SMTP session, so
update that branch to explicitly close server.transport after sending the reply;
also apply the same transport փակ-down behavior in the unknown-recipient RCPT
rejection path so both 421 cases stop further hammering. Use handle_RCPT and the
unknown-recipient branch in the same handler as the anchors for the fix.
In `@src/mta-in/src/pymta/mda_async.py`:
- Around line 101-126: _add an observable metric for weak credentials and
plaintext MDA transport in _validate_credentials instead of relying only on
warning logs. When _validate_credentials detects a non-local http:// base URL or
a secret shorter than _MIN_SECRET_LENGTH, keep the existing logger.warning calls
but also increment a Prometheus counter/gauge using the module’s existing
metrics plumbing so alerting can detect the condition. Use the existing
_validate_credentials, _LOCAL_HOSTNAMES, and _MIN_SECRET_LENGTH checks to place
the metric emission alongside the warnings.
- Around line 101-142: The credential validation in _validate_credentials only
warns on short secrets and misses the empty or None MDA_API_SECRET case,
allowing the RuntimeError from _build_jwt to surface later during
deliver/check_recipient. Update _validate_credentials in MdaAsync to treat a
missing secret as an immediate startup misconfiguration, using the existing
validation flow near the base_url and secret checks so the problem is raised or
logged before start/_post are ever used.
In `@src/mta-in/tests/test_handler.py`:
- Line 110: Replace the literal setattr usage in the test setup with a direct
assignment on session for the _pymta_envelopes attribute. Update the code in the
test_handler flow that sets session state so it uses normal attribute assignment
instead of setattr, since Ruff B010 flags this pattern as unnecessary.
---
Outside diff comments:
In `@docs/env.md`:
- Around line 341-356: The Business Logic settings table is missing the new
MESSAGES_INBOUND_DEFERRAL_MAX_AGE entry, so add a row for it alongside the other
MESSAGES_* options in docs/env.md. Use the same style as the existing rows, and
ensure the description matches its purpose in settings.py and docs/webhooks.md:
the inbound deferral/retry window with a 48-hour default. Keep the naming
consistent with nearby symbols like MESSAGES_ALLOW_INTERNAL_DELIVERY and
MESSAGES_MAILBOX_LOCALPART_DENYLIST_PERSONAL so operators can find the setting
in one place.
In `@src/backend/core/admin.py`:
- Around line 583-645: The regenerate_secret_view flow is missing a permission
gate, so a staff user without Channel change rights can still rotate secrets by
calling it directly. Add an explicit change-permission check in
regenerate_secret_view, before get_object/rotate_secret, using the admin’s
existing permission helpers for the Channel model, and return a denied response
or redirect if the user lacks change access. Keep the fix localized to
regenerate_secret_view and preserve the existing POST/type/auth_method
validations.
In `@src/backend/core/tests/commands/test_backfill_postmark.py`:
- Around line 20-113: Add a test in TestBackfillPostmark that exercises the
backfill_postmark command’s --before cutoff path, since no current case covers
it. Create a MessageFactory row that should be excluded by the cutoff, invoke
call_command("backfill_postmark", ...) with a full ISO datetime string lacking
an offset, and assert the row is not backfilled. Use the backfill_postmark
command path and the existing call_command/message.refresh_from_db pattern so
the naive-vs-aware datetime handling in the cutoff logic is covered.
🪄 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: 13e94632-746a-4cb4-aa6e-5828555c6585
⛔ Files ignored due to path filters (8)
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/**src/mta-in/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (124)
Makefilecompose.yamldocs/env.mddocs/tiered-storage.mddocs/webhooks.mdenv.d/development/backend.defaultsenv.d/development/mta-in-py.defaultssrc/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/factories.pysrc/backend/core/management/commands/backfill_postmark.pysrc/backend/core/management/commands/send_mail.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/utils.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/migrations/0032_inboundmessage_blob_envelope.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_inbound_mta.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_submit.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_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/models/test_message_get_parsed_data.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/core/tests/test_admin_inbound_reprocess.pysrc/backend/core/tests/test_signals.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/celery_app.pysrc/backend/messages/settings.pysrc/frontend/package.jsonsrc/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/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsxsrc/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.tsxsrc/frontend/src/features/utils/mail-helper.test.tsxsrc/frontend/src/features/utils/mail-helper.tsxsrc/jmap-email/examples/inline_image_roundtrip.pysrc/jmap-email/pyproject.tomlsrc/keycloak/tests/test_bulk_role_membership.pysrc/mpa/tests/conftest.pysrc/mpa/tests/test_rspamd_api.pysrc/mta-in/Dockerfilesrc/mta-in/Dockerfile.pymtasrc/mta-in/README.mdsrc/mta-in/entrypoint.pymta.shsrc/mta-in/pyproject.tomlsrc/mta-in/src/api/mda.pysrc/mta-in/src/delivery_milter.pysrc/mta-in/src/pymta/__init__.pysrc/mta-in/src/pymta/address.pysrc/mta-in/src/pymta/controller.pysrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/limits.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/metrics.pysrc/mta-in/src/pymta/server.pysrc/mta-in/src/pymta/settings.pysrc/mta-in/src/pymta/smtp_protocol.pysrc/mta-in/tests/conftest.pysrc/mta-in/tests/test_address.pysrc/mta-in/tests/test_email_delivery.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_limits.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_metrics.pysrc/mta-in/tests/test_security.pysrc/mta-in/tests/test_smtp_protocol.pysrc/mta-out/tests/conftest.pysrc/mta-out/tests/test_attachments.pysrc/mta-out/tests/test_email_sending.pysrc/mta-out/tests/test_message_integrity.pysrc/mta-out/tests/test_smtp_auth.pysrc/socks-proxy/tests/conftest.pysrc/socks-proxy/tests/test_smtp.pysrc/socks-proxy/tests/test_socks_proxy.py
💤 Files with no reviewable changes (2)
- src/frontend/package.json
- src/frontend/src/features/utils/mail-helper.test.tsx
Comment on lines
+162
to
+184
| # ----- 5. domain shape --------------------------------------------------- | ||
| # Allow IDN/UTF-8 in the domain; reject empty labels, leading dot, label | ||
| # > 63 octets, and bare IP literals (the inbound path expects FQDNs from | ||
| # legitimate senders). | ||
| if domain.startswith("[") and domain.endswith("]"): | ||
| raise AddressError( | ||
| reason="address_literal", | ||
| smtp_code=501, | ||
| smtp_text="5.1.3 Address literals not accepted", | ||
| ) | ||
| if domain.startswith(".") or domain.endswith(".") or ".." in domain: | ||
| raise AddressError( | ||
| reason="bad_address", | ||
| smtp_code=501, | ||
| smtp_text="5.1.3 Malformed domain", | ||
| ) | ||
| for label in domain.split("."): | ||
| if not label or len(label.encode("utf-8")) > 63: | ||
| raise AddressError( | ||
| reason="bad_address", | ||
| smtp_code=501, | ||
| smtp_text="5.1.3 Malformed domain label", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
RFC 1035 domain label 63 octet limit IDNA punycode vs raw UTF-8 encoding
💡 Result:
Under the Domain Name System (DNS) architecture established by RFC 1035, all labels must be 63 octets or fewer [1][2][3]. This limit is a fundamental requirement of the DNS protocol, as the DNS wire format uses a one-octet length field with the high-order two bits reserved, restricting the remaining six bits to a maximum value of 63 [1][2][4]. Internationalized Domain Names for Applications (IDNA) maintain compatibility with this legacy restriction by distinguishing between two label forms: 1. A-labels: These are the standard, ASCII-compatible representations of domain labels that are actually transmitted and stored in the DNS [5][6][7]. Because they must be valid DNS labels, A-labels—which consist of the "xn--" prefix followed by the Punycode-encoded string—are strictly limited to a maximum of 63 octets [5][6]. Since the "xn--" prefix occupies 4 octets, the remaining Punycode-encoded data is limited to 59 octets [5]. 2. U-labels: These are the Unicode representations of the domain label (e.g., in UTF-8) [5][6]. U-labels are not directly stored in the DNS; they exist at the application layer [6][8]. Because Punycode is a highly efficient encoding for non-ASCII characters, a U-label can represent a significantly larger amount of data than an A-label can store [5][6]. Consequently, a U-label that maps to a maximum-length 63-octet A-label can be much longer than 63 octets—potentially reaching up to 236–252 octets in its UTF-8 form [5][6][9]. In summary, the 63-octet limit applies strictly to the encoded A-label used in the DNS protocol [5][8]. While raw UTF-8 labels are used by applications, they are always converted to the 63-octet-limited A-label format before being processed by the DNS [6][7][8].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc1035.html
- 2: https://www.rfc-editor.org/rfc/rfc1035.txt
- 3: https://datatracker.ietf.org/doc/html/rfc1035
- 4: https://www.ietf.org/rfc/rfc1035
- 5: https://www.ietf.org/rfc/rfc5890
- 6: https://www.rfc-editor.org/rfc/rfc5890.html
- 7: https://www.ietf.org/rfc/rfc5891.txt
- 8: https://www.psg.com/lists/idn/idn.2002/msg01859.html
- 9: https://www.rfc-editor.org/errata/eid4823
Domain label length should be checked on the IDNA A-label, not raw UTF-8. The 63-octet check can reject valid IDN labels whose punycode form is still DNS-safe; normalize the label with IDNA first, then enforce the limit.
🤖 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/mta-in/src/pymta/address.py` around lines 162 - 184, The domain
validation in address parsing is checking label length on the raw UTF-8 text,
which can incorrectly reject valid IDN labels. Update the domain-label
validation in address.py so the length check is performed on each label’s IDNA
A-label (punycode) form before enforcing the 63-octet limit, while keeping the
existing malformed-domain and address-literal checks in the same domain-shape
logic.
Comment on lines
+206
to
+215
| async def handle_RCPT(self, server, session, envelope, address, rcpt_options): # noqa: PLR0911 | ||
| # First gate: hard-error budget. Once the session has accumulated | ||
| # ``PYMTA_HARD_ERROR_LIMIT`` 4xx/5xx replies, send 421 and close so | ||
| # bulk address enumeration / dictionary attacks cannot keep hammering | ||
| # this single TCP session. | ||
| if getattr(session, _SOFT_ERRORS_ATTR, 0) >= settings.PYMTA_HARD_ERROR_LIMIT: | ||
| metrics.SECURITY_REJECTIONS.labels(reason="hard_error_limit").inc() | ||
| metrics.DISCONNECTS_421.labels(reason="hard_error_limit").inc() | ||
| metrics.RCPT_TOTAL.labels(result="rejected_temp").inc() | ||
| return "421 4.7.0 Too many errors, goodbye" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether HardenedSMTP intercepts 421 replies to force a disconnect
fd smtp_protocol.py src/mta-in/src/pymta --exec cat {}Repository: suitenumerique/messages
Length of output: 5965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler.py relevant lines =="
nl -ba src/mta-in/src/pymta/handler.py | sed -n '180,280p'
echo
echo "== search for 421 handling / transport.close / push =="
rg -n "421 4\.7\.0|transport\.close\(|writer\.drain\(|handle_RCPT|command_call_limit|disconnect" src/mta-in/src/pymta -S
echo
echo "== aiosmtpd source if available in environment =="
python3 - <<'PY'
import importlib.util, os, sys
spec = importlib.util.find_spec("aiosmtpd")
print("aiosmtpd_spec:", spec.origin if spec else None)
PYRepository: suitenumerique/messages
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sys, subprocess, textwrap, os, importlib.util, pathlib
spec = importlib.util.find_spec("aiosmtpd")
print("spec:", spec.origin if spec else None)
if spec:
p = pathlib.Path(spec.origin).resolve()
# show the SMTP class around handle_RCPT/push command handling if source exists
print("path:", p)
PYRepository: suitenumerique/messages
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import importlib.util, pathlib
spec = importlib.util.find_spec("aiosmtpd")
print(spec.origin if spec else "")
PYRepository: suitenumerique/messages
Length of output: 162
🌐 Web query:
aiosmtpd custom handler return "421" close transport handle_RCPT behavior
💡 Result:
In aiosmtpd, the handle_RCPT hook is designed to return an SMTP status string (e.g., "250 OK", "550 User unknown") to inform the client of the outcome of the RCPT TO command [1][2][3]. Returning a string beginning with "421" (a transient, system-closing error code) from your custom handler will send that response to the client, but it does not automatically close the underlying transport [1][2]. If your goal is to immediately close the connection after returning a 421 status, you must access the server object passed to the handle_RCPT method and explicitly close its transport [4]. You can achieve this by calling server.transport.close within your handler [4]: async def handle_RCPT(self, server, session, envelope, address, rcpt_options): # Logic to decide if you need to close if should_close_connection: # Schedule the transport to close server.transport.close # Return the 421 error code string return "421 Service not available, closing transmission channel" # Normal processing envelope.rcpt_tos.append(address) return "250 OK" Important considerations: 1. SMTP RFC Compliance: RFC 5321 generally expects the server to keep the connection open until a QUIT command is received [5]. Forcing a closure after a 4xx or 5xx error may cause issues with some SMTP clients that expect to continue sending commands or cleanly exit [5]. 2. Transport Access: The server object provided to handle_RCPT has a transport attribute that represents the underlying connection [4]. Calling.close on this object will trigger the cleanup of the session [4]. 3. Alternative: If you simply want to reject the recipient without closing the entire connection, you should return a standard 5xx (permanent failure) or 4xx (transient failure) response string [6][3]. This is the standard way to reject a specific RCPT TO command while allowing the client to continue the SMTP session [2][3].
Citations:
- 1: https://aiosmtpd.aio-libs.org/en/stable/handlers.html
- 2: https://aiosmtpd.aio-libs.org/en/1.4.3/controller.html
- 3: Send 550 in response to SMTP RCPT aio-libs/aiosmtpd#47
- 4: https://github.com/aio-libs/aiosmtpd/blob/master/aiosmtpd/smtp.py
- 5: CancelledError and misbehaving clients handling aio-libs/aiosmtpd#110
- 6: https://github.com/aio-libs/aiosmtpd/blob/master/aiosmtpd/tests/test_handlers.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/mta-in/src/pymta/handler.py | sed -n '200,270p'Repository: suitenumerique/messages
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler.py RCPT branches =="
nl -ba src/mta-in/src/pymta/handler.py | sed -n '200,265p'
echo
echo "== smtp_protocol.py close-on-421 paths =="
rg -n '421|transport\.close\(|push\("421|disconnect' src/mta-in/src/pymta/smtp_protocol.py -n -S
echo
echo "== aiosmtpd source presence =="
python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("aiosmtpd")
print(spec.origin if spec else "NO_AIOSMTPD")
PYRepository: suitenumerique/messages
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler.py RCPT branches =="
awk 'NR>=200 && NR<=260 { printf "%4d %s\n", NR, $0 }' src/mta-in/src/pymta/handler.py
echo
echo "== explicit transport close / 421 in pymta =="
rg -n 'transport\.close\(|return "421|push\("421|DISCONNECTS_421|handle_RCPT|handle_PROXY' src/mta-in/src/pymta -S
echo
echo "== smtp_protocol.py slice =="
awk 'NR>=1 && NR<=220 { printf "%4d %s\n", NR, $0 }' src/mta-in/src/pymta/smtp_protocol.pyRepository: suitenumerique/messages
Length of output: 11694
Close the transport on RCPT 421s. Returning 421 here only sends the reply; it doesn’t end the SMTP session. The hard-error-limit and unknown-recipient paths should explicitly close server.transport if the goal is to stop further hammering. src/mta-in/src/pymta/handler.py:206-215,247-255
🤖 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/mta-in/src/pymta/handler.py` around lines 206 - 215, The RCPT
hard-error-limit path in handle_RCPT only returns a 421 response and does not
actually terminate the SMTP session, so update that branch to explicitly close
server.transport after sending the reply; also apply the same transport փակ-down
behavior in the unknown-recipient RCPT rejection path so both 421 cases stop
further hammering. Use handle_RCPT and the unknown-recipient branch in the same
handler as the anchors for the fix.
Comment on lines
+101
to
+126
| def _validate_credentials(self) -> None: | ||
| """Warn loudly at startup about weak secret or plaintext non-local MDA URL. | ||
|
|
||
| Warnings rather than hard failures because the shared dev secret | ||
| ``my-shared-secret-mda`` (20 chars) is intentionally short, and dev | ||
| deployments talk to the MDA over the docker bridge without TLS. The | ||
| log line gives a prod operator clear feedback to fix; promote to | ||
| ``RuntimeError`` here once prod has migrated to a stronger secret. | ||
| """ | ||
| parsed = urlparse(self.base_url) | ||
| host = (parsed.hostname or "").lower() | ||
| if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES: | ||
| logger.warning( | ||
| "MDA_API_BASE_URL uses plaintext http:// for non-local host %r " | ||
| "(%r). The JWT bearer token will traverse the network in clear. " | ||
| "Configure https:// in production.", | ||
| host, | ||
| self.base_url, | ||
| ) | ||
| if self.secret and len(self.secret) < _MIN_SECRET_LENGTH: | ||
| logger.warning( | ||
| "MDA_API_SECRET is %d bytes; recommended minimum is %d. " | ||
| "Short HS256 secrets are brute-forceable from a captured JWT.", | ||
| len(self.secret), | ||
| _MIN_SECRET_LENGTH, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Weak-secret / plaintext-HTTP posture is warn-only.
_validate_credentials logs a warning for a short HS256 secret or plaintext http:// to a non-local MDA host but still proceeds to sign and send requests. The comment acknowledges this is intentional ("promote to RuntimeError... once prod has migrated"), but a log line alone is easy to miss operationally. Consider also emitting a Prometheus counter/gauge (this module already wires metrics elsewhere) so a weak/plaintext deployment is visible to alerting, not just logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mta-in/src/pymta/mda_async.py` around lines 101 - 126, _add an observable
metric for weak credentials and plaintext MDA transport in _validate_credentials
instead of relying only on warning logs. When _validate_credentials detects a
non-local http:// base URL or a secret shorter than _MIN_SECRET_LENGTH, keep the
existing logger.warning calls but also increment a Prometheus counter/gauge
using the module’s existing metrics plumbing so alerting can detect the
condition. Use the existing _validate_credentials, _LOCAL_HOSTNAMES, and
_MIN_SECRET_LENGTH checks to place the metric emission alongside the warnings.
Comment on lines
+101
to
+142
| def _validate_credentials(self) -> None: | ||
| """Warn loudly at startup about weak secret or plaintext non-local MDA URL. | ||
|
|
||
| Warnings rather than hard failures because the shared dev secret | ||
| ``my-shared-secret-mda`` (20 chars) is intentionally short, and dev | ||
| deployments talk to the MDA over the docker bridge without TLS. The | ||
| log line gives a prod operator clear feedback to fix; promote to | ||
| ``RuntimeError`` here once prod has migrated to a stronger secret. | ||
| """ | ||
| parsed = urlparse(self.base_url) | ||
| host = (parsed.hostname or "").lower() | ||
| if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES: | ||
| logger.warning( | ||
| "MDA_API_BASE_URL uses plaintext http:// for non-local host %r " | ||
| "(%r). The JWT bearer token will traverse the network in clear. " | ||
| "Configure https:// in production.", | ||
| host, | ||
| self.base_url, | ||
| ) | ||
| if self.secret and len(self.secret) < _MIN_SECRET_LENGTH: | ||
| logger.warning( | ||
| "MDA_API_SECRET is %d bytes; recommended minimum is %d. " | ||
| "Short HS256 secrets are brute-forceable from a captured JWT.", | ||
| len(self.secret), | ||
| _MIN_SECRET_LENGTH, | ||
| ) | ||
|
|
||
| async def start(self) -> httpx.AsyncClient: | ||
| """Open the persistent HTTP client. Idempotent.""" | ||
| if self._client is None: | ||
| limits = httpx.Limits(max_keepalive_connections=20, max_connections=100) | ||
| self._client = httpx.AsyncClient(timeout=self.timeout, limits=limits) | ||
| return self._client | ||
|
|
||
| async def close(self) -> None: | ||
| if self._client is not None: | ||
| await self._client.aclose() | ||
| self._client = None | ||
|
|
||
| def _build_jwt(self, body: bytes, metadata: dict) -> str: | ||
| if not self.secret: | ||
| raise RuntimeError("MDA_API_SECRET is required to sign MDA API requests") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail fast on missing MDA_API_SECRET instead of deferring to first request.
_validate_credentials checks secret length but never checks for an empty/None secret; that case only surfaces later as an uncaught RuntimeError from _build_jwt (Line 141-142) during the first check_recipient/deliver call. _post's except clauses (Lines 205-219) only catch httpx exceptions, so this RuntimeError propagates unhandled into the SMTP handler's RCPT/DATA path — turning a startup misconfiguration into a runtime crash mid-session instead of an immediate, obvious failure at process boot.
🛡️ Proposed fix: validate secret presence at construction
def _validate_credentials(self) -> None:
...
parsed = urlparse(self.base_url)
host = (parsed.hostname or "").lower()
+ if not self.secret:
+ raise RuntimeError("MDA_API_SECRET is required to sign MDA API requests")
if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES:📝 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
| def _validate_credentials(self) -> None: | |
| """Warn loudly at startup about weak secret or plaintext non-local MDA URL. | |
| Warnings rather than hard failures because the shared dev secret | |
| ``my-shared-secret-mda`` (20 chars) is intentionally short, and dev | |
| deployments talk to the MDA over the docker bridge without TLS. The | |
| log line gives a prod operator clear feedback to fix; promote to | |
| ``RuntimeError`` here once prod has migrated to a stronger secret. | |
| """ | |
| parsed = urlparse(self.base_url) | |
| host = (parsed.hostname or "").lower() | |
| if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES: | |
| logger.warning( | |
| "MDA_API_BASE_URL uses plaintext http:// for non-local host %r " | |
| "(%r). The JWT bearer token will traverse the network in clear. " | |
| "Configure https:// in production.", | |
| host, | |
| self.base_url, | |
| ) | |
| if self.secret and len(self.secret) < _MIN_SECRET_LENGTH: | |
| logger.warning( | |
| "MDA_API_SECRET is %d bytes; recommended minimum is %d. " | |
| "Short HS256 secrets are brute-forceable from a captured JWT.", | |
| len(self.secret), | |
| _MIN_SECRET_LENGTH, | |
| ) | |
| async def start(self) -> httpx.AsyncClient: | |
| """Open the persistent HTTP client. Idempotent.""" | |
| if self._client is None: | |
| limits = httpx.Limits(max_keepalive_connections=20, max_connections=100) | |
| self._client = httpx.AsyncClient(timeout=self.timeout, limits=limits) | |
| return self._client | |
| async def close(self) -> None: | |
| if self._client is not None: | |
| await self._client.aclose() | |
| self._client = None | |
| def _build_jwt(self, body: bytes, metadata: dict) -> str: | |
| if not self.secret: | |
| raise RuntimeError("MDA_API_SECRET is required to sign MDA API requests") | |
| def _validate_credentials(self) -> None: | |
| """Warn loudly at startup about weak secret or plaintext non-local MDA URL. | |
| Warnings rather than hard failures because the shared dev secret | |
| ``my-shared-secret-mda`` (20 chars) is intentionally short, and dev | |
| deployments talk to the MDA over the docker bridge without TLS. The | |
| log line gives a prod operator clear feedback to fix; promote to | |
| ``RuntimeError`` here once prod has migrated to a stronger secret. | |
| """ | |
| parsed = urlparse(self.base_url) | |
| host = (parsed.hostname or "").lower() | |
| if not self.secret: | |
| raise RuntimeError("MDA_API_SECRET is required to sign MDA API requests") | |
| if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES: | |
| logger.warning( | |
| "MDA_API_BASE_URL uses plaintext http:// for non-local host %r " | |
| "(%r). The JWT bearer token will traverse the network in clear. " | |
| "Configure https:// in production.", | |
| host, | |
| self.base_url, | |
| ) | |
| if self.secret and len(self.secret) < _MIN_SECRET_LENGTH: | |
| logger.warning( | |
| "MDA_API_SECRET is %d bytes; recommended minimum is %d. " | |
| "Short HS256 secrets are brute-forceable from a captured JWT.", | |
| len(self.secret), | |
| _MIN_SECRET_LENGTH, | |
| ) | |
| async def start(self) -> httpx.AsyncClient: | |
| """Open the persistent HTTP client. Idempotent.""" | |
| if self._client is None: | |
| limits = httpx.Limits(max_keepalive_connections=20, max_connections=100) | |
| self._client = httpx.AsyncClient(timeout=self.timeout, limits=limits) | |
| return self._client | |
| async def close(self) -> None: | |
| if self._client is not None: | |
| await self._client.aclose() | |
| self._client = None | |
| def _build_jwt(self, body: bytes, metadata: dict) -> str: | |
| if not self.secret: | |
| raise RuntimeError("MDA_API_SECRET is required to sign MDA API requests") |
🤖 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/mta-in/src/pymta/mda_async.py` around lines 101 - 142, The credential
validation in _validate_credentials only warns on short secrets and misses the
empty or None MDA_API_SECRET case, allowing the RuntimeError from _build_jwt to
surface later during deliver/check_recipient. Update _validate_credentials in
MdaAsync to treat a missing secret as an immediate startup misconfiguration,
using the existing validation flow near the base_url and secret checks so the
problem is raised or logged before start/_post are ever used.
| @pytest.mark.asyncio | ||
| async def test_data_max_envelopes_bumps_soft_errors(): | ||
| session, envelope = _session(), _envelope() | ||
| setattr(session, "_pymta_envelopes", settings.PYMTA_MAX_ENVELOPES_PER_CONNECTION) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace setattr with direct assignment.
Ruff (B010) flags setattr with a literal attribute name as no safer than direct assignment.
🔧 Proposed fix
- setattr(session, "_pymta_envelopes", settings.PYMTA_MAX_ENVELOPES_PER_CONNECTION)
+ session._pymta_envelopes = settings.PYMTA_MAX_ENVELOPES_PER_CONNECTION📝 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
| setattr(session, "_pymta_envelopes", settings.PYMTA_MAX_ENVELOPES_PER_CONNECTION) | |
| session._pymta_envelopes = settings.PYMTA_MAX_ENVELOPES_PER_CONNECTION |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🤖 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/mta-in/tests/test_handler.py` at line 110, Replace the literal setattr
usage in the test setup with a direct assignment on session for the
_pymta_envelopes attribute. Update the code in the test_handler flow that sets
session state so it uses normal attribute assignment instead of setattr, since
Ruff B010 flags this pattern as unnecessary.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/socks-proxy/tests/conftest.py (1)
26-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid silent fallbacks for proxy/env parsing failures.
parse_proxy_env()now collapses malformed input intoProxyConfig(), andget_container_ip()does the same forhostname -Ifailures. That can mask bad CI/env configuration and make the SOCKS fixtures exercise localhost/defaults instead of the intended proxy path.🤖 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/socks-proxy/tests/conftest.py` around lines 26 - 62, parse_proxy_env() and get_container_ip() currently swallow parse/command failures and silently return default values, which can hide bad test configuration. Update parse_proxy_env() to validate the SOCKS_PROXY1/SOCKS_PROXY2 input and surface malformed values instead of returning a blank ProxyConfig(), and make get_container_ip() report or raise on hostname -I errors rather than always falling back to 127.0.0.1. Keep the behavior explicit in the conftest.py fixtures so failures are visible when PROXY1_CONFIG, PROXY2_CONFIG, or get_container_ip() are used.
🤖 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 `@compose.yaml`:
- Around line 288-346: The mta-in-py and mta-in-py-test services still rely on
binding to privileged port 25 while running as uid 65532 with cap_drop: ALL, so
startup depends on host sysctl behavior. Update the Compose configuration for
these services by either adding NET_BIND_SERVICE to their capabilities or
changing the container-side SMTP listener to an unprivileged port and keeping
the host port mapping in place. Use the mta-in-py and mta-in-py-test service
definitions to make the change consistently.
In `@docs/env.md`:
- Around line 354-355: The Business Logic table is missing the new
MESSAGES_INBOUND_DEFERRAL_MAX_AGE setting, so add it alongside the related
messages settings in docs/env.md. Update the table entry to describe its
env-configurable 48-hour default and that it controls the inbound deferral/retry
window, using the existing MESSAGES_MANUAL_RETRY_MAX_AGE row as the nearby
reference point for placement and style.
In `@src/backend/core/admin.py`:
- Around line 616-619: The webhook auth_method validation in the admin action
uses membership against WebhookAuthMethod directly, which is not version-safe
for string values. Update the check in the admin logic around the channel.type
== ChannelTypes.WEBHOOK branch to compare (channel.settings or
{}).get("auth_method") against an explicit set of enum values, such as the
values from WebhookAuthMethod, so the membership test works consistently across
Python versions.
In `@src/backend/core/api/viewsets/inbound/mta.py`:
- Around line 244-250: Normalize the `base_envelope` fields in
`InboundMTAViewSet` so `mail_from`, `ip`, `helo`, and `hostname` are always
strings, matching the existing `Return-Path` handling. Update the
`base_envelope` construction to coerce missing or null `mta_metadata` values to
empty strings rather than preserving `None`, using the `mta_metadata.get(...)`
lookups in the MTA inbound flow as the place to fix it.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 150-205: The `_retry_or_abandon` flow is persisting and logging
raw `reason` values, which may include sensitive PII when they come from
`str(e)` in the parse/create failure paths. Update `_retry_or_abandon` so
`error_message` and the `logger.error` call use a sanitized, bounded message
instead of the raw exception text, and keep full exception details only in a
safe internal context if needed. Check the call sites that pass `str(e)` into
`_retry_or_abandon` to ensure they sanitize or normalize the message before it
reaches `error_message`, `abandoned_at`, or logs.
- Around line 396-527: The post-delete path in the inbound task can still fall
through to the outer retry/abandon handlers, but once `inbound_message.delete()`
has run the instance no longer has a valid pk and `_retry_or_abandon()` may try
to save a deleted row and raise `ValueError`, hiding the original failure. Add a
clear “deleted/already_removed” guard around the `inbound_message` flow in this
finalize section of `process_inbound_message_task` so any exception after
deletion bypasses `_retry_or_abandon()` and exits cleanly instead of retrying
the deleted object.
In `@src/backend/core/mda/spam.py`:
- Around line 79-82: Cast the trusted_relays value to int before using it in
spam rule evaluation; in the logic around spam_config.get,
headers_blocks(parsed_email), and blocks_to_check, normalize trusted_relays so
trusted_relays + 1 cannot fail when SPAM_CONFIG contains a string override.
Update the assignment in the spam check flow to ensure the hardcoded-rule scan
still runs safely with mixed-type config values.
In `@src/backend/core/models.py`:
- Around line 2388-2394: get_raw_bytes() assumes self.blob is always present,
but if blob is None it will fail later with a bare AttributeError from
get_content(). Update get_raw_bytes in the Blob-backed model to explicitly
validate blob before dereferencing it, and raise a clear, intentional error that
names the missing blob state so regressions in the ingest paths are caught at
the source.
In `@src/backend/core/tests/commands/test_backfill_postmark.py`:
- Around line 93-102: Add test coverage for the --before option in
test_backfill_postmark by exercising backfill_postmark with an offset-less
datetime string such as 2026-07-01T10:00:00. The current tests only cover
--limit, so extend the command test cases around
call_command("backfill_postmark", ...) to verify naive datetime input is handled
correctly and does not regress the naive/aware datetime logic in
backfill_postmark.
In `@src/backend/messages/settings.py`:
- Around line 546-581: The new MESSAGES_INBOUND_DEFERRAL_MAX_AGE setting in the
Settings config is missing from the environment documentation. Add this
env-configurable option to the Business Logic table in docs/env.md next to
MESSAGES_MANUAL_RETRY_MAX_AGE, and make sure the entry matches the existing
48-hour deferral behavior described by the MESSAGES_INBOUND_DEFERRAL_MAX_AGE
constant in settings.py.
- Around line 1152-1156: Update the docs for FEATURE_MAILBOX_ADMIN_CHANNELS to
match the new default in settings.py; the Base default now includes webhook via
the ListValue in FEATURE_MAILBOX_ADMIN_CHANNELS. Find the “Common Feature Flags”
entry in docs/env.md and change the documented default from empty to
api_key,webhook so the runtime behavior and docs stay in sync.
In `@src/frontend/src/features/blocknote/email-exporter/index.tsx`:
- Around line 164-172: sanitizeLinkHref currently validates the parsed scheme
but returns the original raw href for absolute URLs, so the exported HTML can
receive an unnormalized value that was never actually used for the allowlist
check. Update sanitizeLinkHref to keep the parsed URL object when new URL(href)
succeeds, validate its protocol against VALID_LINK_PROTOCOLS, and return the
normalized url.href for allowed links while still returning null for disallowed
schemes and the raw href only for parse failures if that behavior is intended.
In `@src/mta-in/entrypoint.pymta.sh`:
- Around line 24-30: The readiness loop in entrypoint.pymta.sh only checks the
socket and can wait the full timeout even when the pymta process has already
crashed. Update the polling logic around the existing for/sleep loop to also
monitor $PYMTA_PID (for example with a lightweight process liveness check) and
exit early with a failure when that process is no longer running. Keep the
existing port probe behavior, but make the loop fail fast by combining the PID
check with the current python socket connection test.
In `@src/mta-in/src/pymta/limits.py`:
- Around line 8-10: The per-IP new-session limit logic in limits.py is a
fixed/reset window, not a rolling window, so update the docstring/comment near
the rate-cap definition to match the actual behavior or switch the
implementation in the relevant limiter code (including the session tracking
around max_per_ip_per_minute) to a true sliding-window/token-bucket approach.
Use the identifiers for the per-IP session cap and its window-reset check to
locate the comment and decide whether to revise the wording to “fixed window” or
change the rate-limiting algorithm.
In `@src/mta-in/src/pymta/mda_async.py`:
- Around line 101-126: The _validate_credentials method in mda_async.py only
emits warnings for plaintext non-local MDA URLs and short MDA_API_SECRET values,
but the comment asks for enforcement in production. Update _validate_credentials
to keep the current warnings for dev, then add a production gate (for example
using PYMTA_ENV or a similar env flag) that raises a RuntimeError when base_url
is http:// on a non-local host or when secret length is below
_MIN_SECRET_LENGTH; use the existing _validate_credentials, _LOCAL_HOSTNAMES,
and _MIN_SECRET_LENGTH symbols to keep the behavior easy to locate and preserve
the current warning paths outside production.
In `@src/mta-in/src/pymta/server.py`:
- Around line 36-58: Use the controller’s public startup path in _serve instead
of calling the private _create_server() and manually assigning
controller.server. Since HardenedController inherits UnthreadedController,
switch startup to the public controller begin/lifecycle flow so server creation
and registration are handled by the controller itself, and keep the
MDAClient/InboundHandler/IPGate setup unchanged.
---
Outside diff comments:
In `@src/socks-proxy/tests/conftest.py`:
- Around line 26-62: parse_proxy_env() and get_container_ip() currently swallow
parse/command failures and silently return default values, which can hide bad
test configuration. Update parse_proxy_env() to validate the
SOCKS_PROXY1/SOCKS_PROXY2 input and surface malformed values instead of
returning a blank ProxyConfig(), and make get_container_ip() report or raise on
hostname -I errors rather than always falling back to 127.0.0.1. Keep the
behavior explicit in the conftest.py fixtures so failures are visible when
PROXY1_CONFIG, PROXY2_CONFIG, or get_container_ip() are used.
🪄 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: 13e94632-746a-4cb4-aa6e-5828555c6585
⛔ Files ignored due to path filters (8)
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/**src/mta-in/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (124)
Makefilecompose.yamldocs/env.mddocs/tiered-storage.mddocs/webhooks.mdenv.d/development/backend.defaultsenv.d/development/mta-in-py.defaultssrc/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/factories.pysrc/backend/core/management/commands/backfill_postmark.pysrc/backend/core/management/commands/send_mail.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/utils.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/migrations/0032_inboundmessage_blob_envelope.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_inbound_mta.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_submit.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_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/models/test_message_get_parsed_data.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/core/tests/test_admin_inbound_reprocess.pysrc/backend/core/tests/test_signals.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/celery_app.pysrc/backend/messages/settings.pysrc/frontend/package.jsonsrc/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/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsxsrc/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.tsxsrc/frontend/src/features/utils/mail-helper.test.tsxsrc/frontend/src/features/utils/mail-helper.tsxsrc/jmap-email/examples/inline_image_roundtrip.pysrc/jmap-email/pyproject.tomlsrc/keycloak/tests/test_bulk_role_membership.pysrc/mpa/tests/conftest.pysrc/mpa/tests/test_rspamd_api.pysrc/mta-in/Dockerfilesrc/mta-in/Dockerfile.pymtasrc/mta-in/README.mdsrc/mta-in/entrypoint.pymta.shsrc/mta-in/pyproject.tomlsrc/mta-in/src/api/mda.pysrc/mta-in/src/delivery_milter.pysrc/mta-in/src/pymta/__init__.pysrc/mta-in/src/pymta/address.pysrc/mta-in/src/pymta/controller.pysrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/limits.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/metrics.pysrc/mta-in/src/pymta/server.pysrc/mta-in/src/pymta/settings.pysrc/mta-in/src/pymta/smtp_protocol.pysrc/mta-in/tests/conftest.pysrc/mta-in/tests/test_address.pysrc/mta-in/tests/test_email_delivery.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_limits.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_metrics.pysrc/mta-in/tests/test_security.pysrc/mta-in/tests/test_smtp_protocol.pysrc/mta-out/tests/conftest.pysrc/mta-out/tests/test_attachments.pysrc/mta-out/tests/test_email_sending.pysrc/mta-out/tests/test_message_integrity.pysrc/mta-out/tests/test_smtp_auth.pysrc/socks-proxy/tests/conftest.pysrc/socks-proxy/tests/test_smtp.pysrc/socks-proxy/tests/test_socks_proxy.py
💤 Files with no reviewable changes (2)
- src/frontend/package.json
- src/frontend/src/features/utils/mail-helper.test.tsx
Comment on lines
+288
to
+346
| mta-in-py: | ||
| build: | ||
| context: src/mta-in | ||
| dockerfile: Dockerfile.pymta | ||
| target: runtime-distroless-prod | ||
| args: | ||
| DOCKER_USER: ${DOCKER_USER:-65532} | ||
| user: ${DOCKER_USER:-65532} | ||
| env_file: | ||
| - env.d/development/mta-in.defaults | ||
| - env.d/development/mta-in.local | ||
| - env.d/development/mta-in-py.defaults | ||
| - env.d/development/mta-in-py.local | ||
| ports: | ||
| - "8920:25" | ||
| - "9120:9100" # Prometheus metrics | ||
| # Defence-in-depth: pymta needs no on-disk writes at runtime. Read-only | ||
| # rootfs + dropped capabilities + no-new-privileges mirror the posture | ||
| # a production k8s pod-spec should run with. | ||
| read_only: true | ||
| cap_drop: | ||
| - ALL | ||
| security_opt: | ||
| - no-new-privileges:true | ||
| tmpfs: | ||
| - /tmp:rw,noexec,nosuid,size=16m | ||
| depends_on: | ||
| - backend-dev | ||
|
|
||
| mta-in-py-test: | ||
| profiles: | ||
| - tools | ||
| build: | ||
| context: src/mta-in | ||
| dockerfile: Dockerfile.pymta | ||
| target: runtime-dev | ||
| args: | ||
| DOCKER_USER: ${DOCKER_USER:-65532} | ||
| user: ${DOCKER_USER:-65532} | ||
| cap_drop: | ||
| - ALL | ||
| security_opt: | ||
| - no-new-privileges:true | ||
| env_file: | ||
| - env.d/development/mta-in.defaults | ||
| - env.d/development/mta-in.local | ||
| - env.d/development/mta-in-py.defaults | ||
| - env.d/development/mta-in-py.local | ||
| environment: | ||
| - EXEC_CMD=true | ||
| - MDA_API_BASE_URL=http://localhost:8000/api/mail/ | ||
| - MTA_HOST=localhost | ||
| - MTA_PORT=25 | ||
| - MTA_IMPL=pymta | ||
| - MTA_METRICS_URL=http://localhost:9100/metrics | ||
| command: pytest -vvs tests/ | ||
| volumes: | ||
| - ./src/mta-in:/app | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files of interest ==\n'
git ls-files | rg '^(compose\.yaml|src/mta-in/|env\.d/development/mta-in-py\.|env\.d/development/mta-in\.)'
printf '\n== compose.yaml around mta-in-py ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path('compose.yaml')
lines = p.read_text().splitlines()
for start, end in [(260, 360)]:
print(f'--- {p}:{start}-{end} ---')
for i, line in enumerate(lines[start-1:end], start):
print(f'{i:4d}: {line}')
PY
printf '\n== Dockerfile.pymta outline ==\n'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline src/mta-in/Dockerfile.pymta || true
else
sed -n '1,240p' src/mta-in/Dockerfile.pymta
fi
printf '\n== Search for port config / 25 / 2525 / PYMTA_SMTP_PORT ==\n'
rg -n 'PYMTA_SMTP_PORT|2525|:25\b|port\s*=?\s*25|SMTP_PORT|bind.*25|listen.*25' src/mta-in env.d/development compose.yamlRepository: suitenumerique/messages
Length of output: 7494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
'src/mta-in/Dockerfile.pymta',
'src/mta-in/entrypoint.pymta.sh',
'src/mta-in/src/pymta/settings.py',
'src/mta-in/src/pymta/server.py',
'src/mta-in/README.md',
'env.d/development/mta-in-py.defaults',
]
for file in files:
p = Path(file)
print(f'\n== {file} ({sum(1 for _ in p.open())} lines) ==')
lines = p.read_text().splitlines()
# print focused slices where useful
if file.endswith('Dockerfile.pymta'):
ranges = [(1, 240)]
elif file.endswith('entrypoint.pymta.sh'):
ranges = [(1, 220)]
elif file.endswith('settings.py'):
ranges = [(1, 160)]
elif file.endswith('server.py'):
ranges = [(1, 180)]
elif file.endswith('README.md'):
ranges = [(50, 90)]
else:
ranges = [(1, 80)]
for start, end in ranges:
print(f'--- {file}:{start}-{end} ---')
for i, line in enumerate(lines[start-1:end], start):
print(f'{i:4d}: {line}')
PYRepository: suitenumerique/messages
Length of output: 26369
Restore a portable bind path for mta-in-py
mta-in-py and mta-in-py-test run as uid 65532 with cap_drop: ALL, but they still default to SMTP port 25 and grant no NET_BIND_SERVICE. That makes startup depend on the runtime’s privileged-port sysctl. Add cap_add: NET_BIND_SERVICE or move the container listener to an unprivileged port and remap it from Compose.
🤖 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 `@compose.yaml` around lines 288 - 346, The mta-in-py and mta-in-py-test
services still rely on binding to privileged port 25 while running as uid 65532
with cap_drop: ALL, so startup depends on host sysctl behavior. Update the
Compose configuration for these services by either adding NET_BIND_SERVICE to
their capabilities or changing the container-side SMTP listener to an
unprivileged port and keeping the host port mapping in place. Use the mta-in-py
and mta-in-py-test service definitions to make the change consistently.
Comment on lines
+150
to
+205
| def _retry_or_abandon( | ||
| inbound_message: models.InboundMessage, | ||
| reason: str, | ||
| blocking_webhook_results: Optional[Dict[Any, Any]] = None, | ||
| ) -> Dict[str, Any]: | ||
| """Bounded handling for a message that failed to be created/processed. | ||
|
|
||
| Within ``DEFERRAL_MAX_AGE`` the row is kept so the 5-min sweep retries | ||
| it (a transient DB error or constraint hiccup clears on its own). Past | ||
| the window the attempt is abandoned: ``abandoned_at`` is stamped so the | ||
| sweep skips the row and stops re-running the whole pipeline — and | ||
| re-firing every user webhook — on it, but the row is NOT deleted. The | ||
| referenced ``blob`` is the only copy of the message, so deleting would | ||
| silently lose mail; instead an operator | ||
| can inspect and replay the row from the Django admin, and ``logger.error`` | ||
| raises a Sentry alert. ``error_message`` keeps the human-readable reason. | ||
|
|
||
| ``blocking_webhook_results`` (when the failure happened AFTER the pipeline | ||
| already ran the blocking webhooks) is persisted on the retry path so the | ||
| next sweep replays those successes from cache instead of re-POSTing them. | ||
| """ | ||
| age = timezone.now() - inbound_message.created_at | ||
| if age <= DEFERRAL_MAX_AGE: | ||
| if blocking_webhook_results: | ||
| persist_cached_webhook_results( | ||
| str(inbound_message.id), blocking_webhook_results | ||
| ) | ||
| inbound_message.error_message = reason | ||
| # See ``_handle_retry``: list ``updated_at`` so each retry bumps it | ||
| # even when ``error_message`` is identical to the previous attempt. | ||
| inbound_message.save(update_fields=["error_message", "updated_at"]) | ||
| return { | ||
| "success": False, | ||
| "inbound_message_id": str(inbound_message.id), | ||
| "error": "retry", | ||
| "reason": reason, | ||
| } | ||
| logger.error( | ||
| "Inbound message %s abandoned after persistent failure (age=%s): %s", | ||
| inbound_message.id, | ||
| age, | ||
| reason, | ||
| ) | ||
| # Keep the row (and its bytes) — stamp it terminally failed so the sweep | ||
| # skips it instead of deleting and losing the only copy of the mail. | ||
| inbound_message.error_message = reason | ||
| inbound_message.abandoned_at = timezone.now() | ||
| inbound_message.save( | ||
| update_fields=["error_message", "abandoned_at", "updated_at"] | ||
| ) | ||
| return { | ||
| "success": False, | ||
| "inbound_message_id": str(inbound_message.id), | ||
| "error": "abandoned", | ||
| "reason": reason, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
reason/str(e) stored and logged may carry sensitive content.
_retry_or_abandon writes reason verbatim into error_message (Lines 177, 195) and logger.error (Lines 187-192) logs it too. When reason originates from str(e) on a parse/creation failure (Lines 273, 559), the exception text can echo raw header/content fragments from the inbound email (addresses, subject snippets, etc.). As per coding guidelines, src/backend/**/*.py should "Do not log sensitive information (tokens, passwords, financial/health data, PII)" — worth confirming these error paths can't leak sender/recipient PII into logs or the admin-visible error_message field.
🤖 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 150 - 205, The
`_retry_or_abandon` flow is persisting and logging raw `reason` values, which
may include sensitive PII when they come from `str(e)` in the parse/create
failure paths. Update `_retry_or_abandon` so `error_message` and the
`logger.error` call use a sanitized, bounded message instead of the raw
exception text, and keep full exception details only in a safe internal context
if needed. Check the call sites that pass `str(e)` into `_retry_or_abandon` to
ensure they sanitize or normalize the message before it reaches `error_message`,
`abandoned_at`, or logs.
Source: Coding guidelines
Comment on lines
+24
to
+30
| for i in $(seq 1 30); do | ||
| if python -c "import socket, sys; s=socket.socket(); s.settimeout(0.5); s.connect(('127.0.0.1', int('$port'))); s.close()" 2>/dev/null; then | ||
| echo "pymta SMTP ready on port $port" | ||
| return 0 | ||
| fi | ||
| sleep 0.5 | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Detect early process death instead of waiting out the full timeout.
The polling loop doesn't check if $PYMTA_PID already exited (e.g., crash-on-bind), so a fast failure still takes the full ~15s before being reported.
♻️ Proposed fix: fail fast on process death
for i in $(seq 1 30); do
if python -c "import socket, sys; s=socket.socket(); s.settimeout(0.5); s.connect(('127.0.0.1', int('$port'))); s.close()" 2>/dev/null; then
echo "pymta SMTP ready on port $port"
return 0
fi
+ if ! kill -0 "$PYMTA_PID" 2>/dev/null; then
+ echo "ERROR: pymta exited before opening port $port" >&2
+ return 1
+ fi
sleep 0.5
done📝 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 i in $(seq 1 30); do | |
| if python -c "import socket, sys; s=socket.socket(); s.settimeout(0.5); s.connect(('127.0.0.1', int('$port'))); s.close()" 2>/dev/null; then | |
| echo "pymta SMTP ready on port $port" | |
| return 0 | |
| fi | |
| sleep 0.5 | |
| done | |
| for i in $(seq 1 30); do | |
| if python -c "import socket, sys; s=socket.socket(); s.settimeout(0.5); s.connect(('127.0.0.1', int('$port'))); s.close()" 2>/dev/null; then | |
| echo "pymta SMTP ready on port $port" | |
| return 0 | |
| fi | |
| if ! kill -0 "$PYMTA_PID" 2>/dev/null; then | |
| echo "ERROR: pymta exited before opening port $port" >&2 | |
| return 1 | |
| fi | |
| sleep 0.5 | |
| done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 24-24: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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/mta-in/entrypoint.pymta.sh` around lines 24 - 30, The readiness loop in
entrypoint.pymta.sh only checks the socket and can wait the full timeout even
when the pymta process has already crashed. Update the polling logic around the
existing for/sleep loop to also monitor $PYMTA_PID (for example with a
lightweight process liveness check) and exit early with a failure when that
process is no longer running. Keep the existing port probe behavior, but make
the loop fail fast by combining the PID check with the current python socket
connection test.
Comment on lines
+8
to
+10
| * a per-IP new-session rate cap (rolling 60s window), defending against fast | ||
| open/close churn from one IP that never exceeds the concurrent cap but | ||
| still costs CPU/TLS handshakes/MDA RCPT checks. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
"Rolling window" wording doesn't match the reset-based fixed-window implementation.
The docstring/comment describes a per-IP new-session rate cap (rolling 60s window), defending against fast open/close churn from one IP that never exceeds the concurrent cap but still costs CPU/TLS handshakes/MDA RCPT checks. but the actual logic resets the counter to 0 whenever the window has elapsed rather than sliding it, so a client can burst up to ~2x max_per_ip_per_minute around a window boundary (N requests just before reset, N more right after). Not a functional bug, but worth either fixing the comment to say "fixed/reset window" or switching to a proper sliding-window/token-bucket implementation if the 2x burst is a concern for abuse mitigation.
Also applies to: 81-92
🤖 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/mta-in/src/pymta/limits.py` around lines 8 - 10, The per-IP new-session
limit logic in limits.py is a fixed/reset window, not a rolling window, so
update the docstring/comment near the rate-cap definition to match the actual
behavior or switch the implementation in the relevant limiter code (including
the session tracking around max_per_ip_per_minute) to a true
sliding-window/token-bucket approach. Use the identifiers for the per-IP session
cap and its window-reset check to locate the comment and decide whether to
revise the wording to “fixed window” or change the rate-limiting algorithm.
Comment on lines
+101
to
+126
| def _validate_credentials(self) -> None: | ||
| """Warn loudly at startup about weak secret or plaintext non-local MDA URL. | ||
|
|
||
| Warnings rather than hard failures because the shared dev secret | ||
| ``my-shared-secret-mda`` (20 chars) is intentionally short, and dev | ||
| deployments talk to the MDA over the docker bridge without TLS. The | ||
| log line gives a prod operator clear feedback to fix; promote to | ||
| ``RuntimeError`` here once prod has migrated to a stronger secret. | ||
| """ | ||
| parsed = urlparse(self.base_url) | ||
| host = (parsed.hostname or "").lower() | ||
| if parsed.scheme == "http" and host not in _LOCAL_HOSTNAMES: | ||
| logger.warning( | ||
| "MDA_API_BASE_URL uses plaintext http:// for non-local host %r " | ||
| "(%r). The JWT bearer token will traverse the network in clear. " | ||
| "Configure https:// in production.", | ||
| host, | ||
| self.base_url, | ||
| ) | ||
| if self.secret and len(self.secret) < _MIN_SECRET_LENGTH: | ||
| logger.warning( | ||
| "MDA_API_SECRET is %d bytes; recommended minimum is %d. " | ||
| "Short HS256 secrets are brute-forceable from a captured JWT.", | ||
| len(self.secret), | ||
| _MIN_SECRET_LENGTH, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Weak-secret / plaintext-MDA-URL checks only warn, never fail.
_validate_credentials logs but doesn't enforce a minimum secret length or reject plaintext non-local MDA URLs, per the comment this is intentional for now, but a short MDA_API_SECRET (or http:// bearer-token leakage) is a real forgeable-JWT/credential-exposure risk. Consider gating a hard failure behind an env flag (e.g., PYMTA_ENV=production) so dev keeps its short shared secret while prod is enforced.
🤖 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/mta-in/src/pymta/mda_async.py` around lines 101 - 126, The
_validate_credentials method in mda_async.py only emits warnings for plaintext
non-local MDA URLs and short MDA_API_SECRET values, but the comment asks for
enforcement in production. Update _validate_credentials to keep the current
warnings for dev, then add a production gate (for example using PYMTA_ENV or a
similar env flag) that raises a RuntimeError when base_url is http:// on a
non-local host or when secret length is below _MIN_SECRET_LENGTH; use the
existing _validate_credentials, _LOCAL_HOSTNAMES, and _MIN_SECRET_LENGTH symbols
to keep the behavior easy to locate and preserve the current warning paths
outside production.
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/admin.py (1)
583-610: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winAdd a
has_change_permissioncheck before regenerating secrets.admin_site.admin_view()only gates active staff access, so any staff user can hit this custom URL, rotate the secret, and read the plaintext credential unless the view checksChannelchange permission first.🤖 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 - 610, The regenerate_secret_view flow currently allows any staff user to rotate and view a channel secret once they reach the custom admin URL. Add a has_change_permission check in regenerate_secret_view, using the existing Channel object from self.get_object before any secret regeneration logic, and reject unauthorized users the same way the admin change views do.
♻️ Duplicate comments (1)
src/backend/core/tests/commands/test_backfill_postmark.py (1)
93-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill missing
--beforetest coverage.The command supports a
--beforeoption per the earlier review note, but no test exercises it (e.g. with a naive/offset-less datetime like2026-07-01T10:00:00) to guard the naive/aware datetime handling path inbackfill_postmark.py.🤖 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 93 - 102, Add test coverage for the `--before` option in `test_backfill_postmark.py` by introducing a case that invokes `call_command("backfill_postmark", "--before", "2026-07-01T10:00:00", ...)` and verifies only messages older than that cutoff are processed. Make sure the test uses the `backfill_postmark` command path that parses the cutoff datetime so the naive/aware handling in `backfill_postmark.py` is exercised.
🤖 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`:
- Line 348: The `MESSAGES_INBOUND_DEFERRAL_MAX_AGE` row still references the old
`X-StMsg-Processing-Failed` header, but the current deferral-expiry behavior in
`inbound_pipeline.py` now stores structured data in `postmark["processing"]`.
Update the description in `docs/env.md` to match the new `InboundPipeline`
behavior and remove the stale header-specific wording so it reflects the
postmark-based failure record instead.
In `@src/backend/core/admin.py`:
- Around line 1610-1624: The per-row update in reprocess is not isolated from
database failures, so a single inbound_message.save(update_fields=[...]) error
can abort the whole admin action before later rows are processed or message_user
runs. Wrap the save/reset block in its own try/except alongside the existing
process_inbound_message_task.delay handling, log the failure with the
inbound_message.id, and continue to the next item so reprocess still queues the
remaining messages and reports the final queued count.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 1105-1109: The early return in dispatch_webhooks silently skips
when channel or mailbox is missing, which breaks the existing log-then-skip
pattern. Update the channel/mailbox lookup branch in dispatch_webhooks to emit a
warning before returning, using the same logging style as the other early-exit
checks in this flow. Reference the existing dispatch_webhooks path and the
channel/mailbox retrieval logic so the missing-record case is easy to locate.
- Around line 884-897: The initial SSRFSafeSession().post call in
dispatch_webhooks is using WEBHOOK_TIMEOUT as the read/header-wait timeout,
which can push the total webhook exchange past the documented budget. Adjust the
timeout setup so the request’s connect and header-wait phase is bounded by the
remaining total deadline computed from time.monotonic() + WEBHOOK_TIMEOUT, and
keep the streamed-body deadline check in place for the rest of the read loop.
Use the existing WEBHOOK_TIMEOUT, WEBHOOK_CONNECT_TIMEOUT, deadline, and
SSRFSafeSession().post flow to implement the fix.
In `@src/backend/core/mda/inbound_create.py`:
- Around line 288-299: The current `_created_now` flag in `inbound_create.py` is
a fragile implicit side-channel for deciding whether finalize side effects
should run. Update the create flow around the `Message` return path so the
created/existing state is explicit in the contract, ideally by returning a
`(message, created)` pair from the `inbound_create`/create helper logic instead
of mutating `existing_message._created_now`. Then adjust downstream consumers
that currently read `_created_now` to use the explicit boolean when gating
autoreply, webhook, and other non-idempotent finalize work.
- Around line 126-128: The Strategy 2 fallback in inbound_create should not
unconditionally return None; either restore the intended behavior in the
parent-selection logic so it returns the most recent matching parent thread (the
potential_parents.first().thread path) or clearly mark the fallback as
intentionally disabled with an explicit comment and handling. Update the
relevant fallback branch in the function that chooses the parent thread for
inbound messages so replies with matching references but non-matching subjects
still attach correctly.
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 288-309: The on-demand rspamd lookup in inbound_auth is ignoring
the error returned by spam.call_rspamd, so transient rspamd failures can be
treated as a normal unauthenticated result instead of being retried. Update the
inbound_auth flow to capture and inspect the err value from spam.call_rspamd,
and if it indicates an rspamd failure, propagate the same retry/hold behavior
used by _make_rspamd_step rather than continuing into
check_inbound_authentication with a missing ctx.rspamd_result. Keep the existing
verdict handling and widget-origin fallback unchanged.
In `@src/backend/core/mda/spam.py`:
- Around line 93-106: The header_match_regex path in the spam rule evaluation
can be vulnerable to ReDoS because user-provided patterns from
MailDomain.get_spam_config() are passed straight into re.fullmatch() for every
message. Update the spam rule parsing/matching flow in the spam check logic to
reject unsafe or overly complex regexes before they are used, or replace the
current regex handling with a timeout-capable engine so catastrophic patterns
cannot block the worker. Keep the existing malformed-regex handling and logging
in place, but add validation/safeguards around the regex rules identified by the
spam rule index.
In `@src/backend/core/services/ssrf.py`:
- Around line 250-301: _sessions created in _pinned_session are not being closed
during redirect handling, so _request_with_redirects should explicitly release
each per-hop requests.Session after use. Update the redirect loop in
_request_with_redirects to close the temporary session for intermediate hops
once the response is no longer needed, while preserving the session associated
with the final returned response so stream=True behavior is not broken. Use the
existing _pinned_session and _request_with_redirects symbols to locate the
cleanup point.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 209-224: The edit flow in webhook-integration-form’s save handler
completes after updateMutation.mutateAsync, but never signals completion to the
parent. Update the successful “isEditing && channel” branch so it mirrors the
create path by invoking onSuccess(...) and then onClose() after the
toast/invalidateChannels work, using the same callback handling already present
in the save handler.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts`:
- Around line 117-136: Add a test in assignment-message.test.ts for
ThreadEventTypeEnum.unassign with a webhook actor (null author, populated
author_display) to cover the non-system path. Use buildAssignmentMessage with a
matching event from makeEvent and assert the webhook-style actor is not treated
as the passive “was unassigned” case; this exercises the isSystem logic in
assignment-message.ts that checks event.author and event.author_display.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 221-227: The author label fallback logic is duplicated between the
thread event component and assignment message handling, which risks future
drift. Extract the shared name-resolution logic into a single helper and have
both `thread-view/components/thread-event/index.tsx` and `assignment-message.ts`
call it instead of maintaining separate priority orders and comments. Use the
existing `author_display`-first resolution as the source of truth, with the same
fallback behavior for `author.full_name`, `author.email`, and `Unknown`.
In `@src/mta-in/README.md`:
- Around line 34-35: Update the authorization contract in the MTA-in README so
it matches both JWT implementations. The current “exp: 60 s” text is only true
for pymta’s _build_jwt, while the Postfix/milter path in src/api/mda.py uses the
configurable MDA_API_JWT_TTL default. Revise the documentation near the
Authorization section to describe the shared HS256/MDA_API_SECRET requirement
and note that token lifetime is implementation-specific or configurable, rather
than fixed at 60 seconds.
- Line 38: Update the response contract in the MTA inbound README to separate
behavior by implementation: the current wording in the README claims 4xx is a
permanent reject, but delivery_milter.py actually maps any non-200 to
Milter.TEMPFAIL while pymta treats 4xx as permanent reject. Clarify the contract
by explicitly documenting the two paths, using the delivery_milter.py behavior
and pymta behavior as the distinguishing symbols, and avoid describing a single
shared rule for all implementations.
In `@src/mta-in/src/pymta/smtp_protocol.py`:
- Around line 92-96: `_acquire_gate` and `_release_gate` in `SMTPProtocol` are
reaching into `IPGate` private internals via `_try_acquire` and `_release`,
which forces the `SLF001` suppression. Add thin public wrappers on `IPGate` in
`limits.py` such as `acquire` and `release` that delegate to the existing
private methods, then update `SMTPProtocol` to call those public methods instead
of the private ones.
In `@src/mta-in/tests/test_security.py`:
- Around line 32-33: The test module is re-reading the MTA host and port
directly from environment variables, duplicating the same configuration now
provided by the mta_address fixture. Update the security tests to consume
mta_address instead of the module-level MTA_HOST/MTA_PORT constants, and remove
the redundant env parsing so there is a single source of truth for the address
in this test suite.
---
Outside diff comments:
In `@src/backend/core/admin.py`:
- Around line 583-610: The regenerate_secret_view flow currently allows any
staff user to rotate and view a channel secret once they reach the custom admin
URL. Add a has_change_permission check in regenerate_secret_view, using the
existing Channel object from self.get_object before any secret regeneration
logic, and reject unauthorized users the same way the admin change views do.
---
Duplicate comments:
In `@src/backend/core/tests/commands/test_backfill_postmark.py`:
- Around line 93-102: Add test coverage for the `--before` option in
`test_backfill_postmark.py` by introducing a case that invokes
`call_command("backfill_postmark", "--before", "2026-07-01T10:00:00", ...)` and
verifies only messages older than that cutoff are processed. Make sure the test
uses the `backfill_postmark` command path that parses the cutoff datetime so the
naive/aware handling in `backfill_postmark.py` is exercised.
🪄 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: 223488f5-0a47-49ab-bf8a-9157d9b79db6
⛔ Files ignored due to path filters (8)
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/**src/mta-in/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (125)
Makefilecompose.yamldocs/env.mddocs/tiered-storage.mddocs/webhooks.mdenv.d/development/backend.defaultsenv.d/development/mta-in-py.defaultssrc/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/factories.pysrc/backend/core/management/commands/backfill_postmark.pysrc/backend/core/management/commands/send_mail.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/utils.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/migrations/0032_inboundmessage_blob_envelope.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_inbound_mta.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_submit.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_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/models/test_inbound_message.pysrc/backend/core/tests/models/test_message_get_parsed_data.pysrc/backend/core/tests/services/test_ssrf.pysrc/backend/core/tests/test_admin_inbound_reprocess.pysrc/backend/core/tests/test_signals.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/celery_app.pysrc/backend/messages/settings.pysrc/frontend/package.jsonsrc/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/blocknote/email-exporter/index.test.tsxsrc/frontend/src/features/blocknote/email-exporter/index.tsxsrc/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.tsxsrc/frontend/src/features/utils/mail-helper.test.tsxsrc/frontend/src/features/utils/mail-helper.tsxsrc/jmap-email/examples/inline_image_roundtrip.pysrc/jmap-email/pyproject.tomlsrc/keycloak/tests/test_bulk_role_membership.pysrc/mpa/tests/conftest.pysrc/mpa/tests/test_rspamd_api.pysrc/mta-in/Dockerfilesrc/mta-in/Dockerfile.pymtasrc/mta-in/README.mdsrc/mta-in/entrypoint.pymta.shsrc/mta-in/pyproject.tomlsrc/mta-in/src/api/mda.pysrc/mta-in/src/delivery_milter.pysrc/mta-in/src/pymta/__init__.pysrc/mta-in/src/pymta/address.pysrc/mta-in/src/pymta/controller.pysrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/limits.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/metrics.pysrc/mta-in/src/pymta/server.pysrc/mta-in/src/pymta/settings.pysrc/mta-in/src/pymta/smtp_protocol.pysrc/mta-in/tests/conftest.pysrc/mta-in/tests/test_address.pysrc/mta-in/tests/test_email_delivery.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_limits.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_metrics.pysrc/mta-in/tests/test_security.pysrc/mta-in/tests/test_smtp_protocol.pysrc/mta-out/tests/conftest.pysrc/mta-out/tests/test_attachments.pysrc/mta-out/tests/test_email_sending.pysrc/mta-out/tests/test_message_integrity.pysrc/mta-out/tests/test_smtp_auth.pysrc/socks-proxy/tests/conftest.pysrc/socks-proxy/tests/test_smtp.pysrc/socks-proxy/tests/test_socks_proxy.py
💤 Files with no reviewable changes (2)
- src/frontend/package.json
- src/frontend/src/features/utils/mail-helper.test.tsx
Comment on lines
+884
to
+897
| # ``stream=True`` lets us cap the response body we actually read — a | ||
| # misconfigured receiver returning a multi-GB error page must not OOM | ||
| # the worker. The ``(connect, read)`` tuple bounds the connect phase | ||
| # tightly and each socket read; the ``deadline`` below bounds the | ||
| # *total* exchange against slow drip. | ||
| deadline = time.monotonic() + WEBHOOK_TIMEOUT | ||
| try: | ||
| response = SSRFSafeSession().post( | ||
| url, | ||
| timeout=(WEBHOOK_CONNECT_TIMEOUT, WEBHOOK_TIMEOUT), | ||
| stream=True, | ||
| headers=signed_headers, | ||
| data=body_bytes, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Header-wait timeout can exceed the documented 30s total budget.
WEBHOOK_TIMEOUT is documented as the "Total wall-clock budget ... (connect + send + read...)", but it's reused as the read timeout for the initial requests.post() call. Worst case, connect (5s) + waiting for response headers (30s) = 35s elapses before the streamed-body deadline check even starts, exceeding the stated total budget and pinning the inbound worker longer than intended for one blocking, in-pipeline webhook.
⏱️ Proposed fix to bound the header-wait phase within the total budget
deadline = time.monotonic() + WEBHOOK_TIMEOUT
try:
response = SSRFSafeSession().post(
url,
- timeout=(WEBHOOK_CONNECT_TIMEOUT, WEBHOOK_TIMEOUT),
+ timeout=(WEBHOOK_CONNECT_TIMEOUT, WEBHOOK_TIMEOUT - WEBHOOK_CONNECT_TIMEOUT),
stream=True,
headers=signed_headers,
data=body_bytes,
)📝 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
| # ``stream=True`` lets us cap the response body we actually read — a | |
| # misconfigured receiver returning a multi-GB error page must not OOM | |
| # the worker. The ``(connect, read)`` tuple bounds the connect phase | |
| # tightly and each socket read; the ``deadline`` below bounds the | |
| # *total* exchange against slow drip. | |
| deadline = time.monotonic() + WEBHOOK_TIMEOUT | |
| try: | |
| response = SSRFSafeSession().post( | |
| url, | |
| timeout=(WEBHOOK_CONNECT_TIMEOUT, WEBHOOK_TIMEOUT), | |
| stream=True, | |
| headers=signed_headers, | |
| data=body_bytes, | |
| ) | |
| # ``stream=True`` lets us cap the response body we actually read — a | |
| # misconfigured receiver returning a multi-GB error page must not OOM | |
| # the worker. The ``(connect, read)`` tuple bounds the connect phase | |
| # tightly and each socket read; the ``deadline`` below bounds the | |
| # *total* exchange against slow drip. | |
| deadline = time.monotonic() + WEBHOOK_TIMEOUT | |
| try: | |
| response = SSRFSafeSession().post( | |
| url, | |
| timeout=(WEBHOOK_CONNECT_TIMEOUT, WEBHOOK_TIMEOUT - WEBHOOK_CONNECT_TIMEOUT), | |
| stream=True, | |
| headers=signed_headers, | |
| data=body_bytes, | |
| ) |
🤖 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 884 - 897, The
initial SSRFSafeSession().post call in dispatch_webhooks is using
WEBHOOK_TIMEOUT as the read/header-wait timeout, which can push the total
webhook exchange past the documented budget. Adjust the timeout setup so the
request’s connect and header-wait phase is bounded by the remaining total
deadline computed from time.monotonic() + WEBHOOK_TIMEOUT, and keep the
streamed-body deadline check in place for the rest of the read loop. Use the
existing WEBHOOK_TIMEOUT, WEBHOOK_CONNECT_TIMEOUT, deadline, and
SSRFSafeSession().post flow to implement the fix.
Comment on lines
+1105
to
+1109
| try: | ||
| channel = models.Channel.objects.filter(id=channel_id).first() | ||
| mailbox = models.Mailbox.objects.filter(id=mailbox_id).first() | ||
| if channel is None or mailbox is None: | ||
| return |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Silent skip breaks the file's log-then-skip pattern.
Every other early-return in this task logs a warning before skipping (trigger mismatch, missing message, mailbox mismatch), but a missing channel/mailbox here returns silently — a deleted channel/mailbox between recording and dispatch vanishes without a trace.
📝 Proposed fix to log before skipping
channel = models.Channel.objects.filter(id=channel_id).first()
mailbox = models.Mailbox.objects.filter(id=mailbox_id).first()
if channel is None or mailbox is None:
+ logger.warning(
+ "Webhook dispatch skipped: channel=%s or mailbox=%s no longer exists",
+ channel_id,
+ mailbox_id,
+ )
return📝 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
| try: | |
| channel = models.Channel.objects.filter(id=channel_id).first() | |
| mailbox = models.Mailbox.objects.filter(id=mailbox_id).first() | |
| if channel is None or mailbox is None: | |
| return | |
| try: | |
| channel = models.Channel.objects.filter(id=channel_id).first() | |
| mailbox = models.Mailbox.objects.filter(id=mailbox_id).first() | |
| if channel is None or mailbox is None: | |
| logger.warning( | |
| "Webhook dispatch skipped: channel=%s or mailbox=%s no longer exists", | |
| channel_id, | |
| mailbox_id, | |
| ) | |
| return |
🤖 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 1105 - 1109, The
early return in dispatch_webhooks silently skips when channel or mailbox is
missing, which breaks the existing log-then-skip pattern. Update the
channel/mailbox lookup branch in dispatch_webhooks to emit a warning before
returning, using the same logging style as the other early-exit checks in this
flow. Reference the existing dispatch_webhooks path and the channel/mailbox
retrieval logic so the missing-record case is easy to locate.
Comment on lines
126
to
128
| # Strategy 2 (Fallback): If no subject match, return thread of the most recent parent message | ||
| # The list is ordered by -sent_at, so the first element is the latest match. | ||
| # The list is ordered by -created_at, so the first element is the latest match. | ||
| return None # potential_parents.first().thread |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant slice.
ast-grep outline src/backend/core/mda/inbound_create.py --view expanded || true
printf '\n--- file slice ---\n'
cat -n src/backend/core/mda/inbound_create.py | sed -n '100,160p'
printf '\n--- searches ---\n'
rg -n "potential_parents|created_at|thread|Strategy 2|subject match|fallback" src/backend/core/mda/inbound_create.py src/backend/core -SRepository: suitenumerique/messages
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect related tests to see whether this fallback path is covered or expected.
rg -n "potential_parents|subject match|thread|inbound_create|created_now|_created_now" src/backend -SRepository: suitenumerique/messages
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the code path is present, verify whether `return None # potential_parents.first().thread`
# is the only implementation of Strategy 2 or whether surrounding control flow makes it intentional.
python3 - <<'PY'
from pathlib import Path
path = Path("src/backend/core/mda/inbound_create.py")
text = path.read_text()
for needle in [
"potential_parents.first().thread",
"Strategy 2",
"return None",
]:
print(f"\n### {needle}")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-4)
end = min(len(text.splitlines()), i+4)
for j in range(start, end + 1):
print(f"{j:4}: {text.splitlines()[j-1]}")
break
PYRepository: suitenumerique/messages
Length of output: 1482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific function around the fallback to determine whether the comment
# mismatch is an intentional stub or a functional bug.
python3 - <<'PY'
from pathlib import Path
path = Path("src/backend/core/mda/inbound_create.py")
lines = path.read_text().splitlines()
for idx, line in enumerate(lines, 1):
if "Strategy 2" in line or "potential_parents.first().thread" in line or "return None" in line:
start = max(1, idx - 8)
end = min(len(lines), idx + 8)
print(f"\n--- around line {idx} ---")
for j in range(start, end + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: suitenumerique/messages
Length of output: 8639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the immediate control flow around the fallback and any nearby TODOs/FIXMEs
# that would indicate whether Strategy 2 is intentionally disabled.
python3 - <<'PY'
from pathlib import Path
path = Path("src/backend/core/mda/inbound_create.py")
lines = path.read_text().splitlines()
for idx, line in enumerate(lines, 1):
if "potential_parents" in line or "Strategy 2" in line or "TODO" in line or "FIXME" in line or "return None # potential_parents.first().thread" in line:
start = max(1, idx - 6)
end = min(len(lines), idx + 6)
print(f"\n--- around line {idx} ---")
for j in range(start, end + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: suitenumerique/messages
Length of output: 4162
Restore the Strategy 2 fallback or mark it as intentionally disabled — the comment says this path should return the most recent parent thread, but the code returns None instead. That leaves replies with matching references but non-matching subjects in new threads.
🤖 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 126 - 128, The Strategy
2 fallback in inbound_create should not unconditionally return None; either
restore the intended behavior in the parent-selection logic so it returns the
most recent matching parent thread (the potential_parents.first().thread path)
or clearly mark the fallback as intentionally disabled with an explicit comment
and handling. Update the relevant fallback branch in the function that chooses
the parent thread for inbound messages so replies with matching references but
non-matching subjects still attach correctly.
Comment on lines
+34
to
+35
| - **Authorization** — `Bearer <jwt>` where the JWT is signed HS256 with `env.MDA_API_SECRET` and carries: | ||
| - `exp`: 60 s from issuance, anchored in UTC. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Documented exp TTL doesn't match the Postfix/milter implementation.
The doc states the JWT exp is fixed at 60s, which is only true for pymta's _build_jwt (hardcoded timedelta(seconds=60)). The Postfix/milter path (src/mta-in/src/api/mda.py) uses a configurable MDA_API_JWT_TTL, defaulting to MDA_API_TIMEOUT * 10 + 60 — not a fixed 60s. Since this section is titled "same contract" for both implementations, this is misleading for anyone reasoning about token replay windows.
📝 Suggested wording fix
- - `exp`: 60 s from issuance, anchored in UTC.
+ - `exp`: 60 s from issuance for pymta (hardcoded); Postfix/milter uses
+ `env.MDA_API_JWT_TTL` (default `MDA_API_TIMEOUT * 10 + 60`).📝 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
| - **Authorization** — `Bearer <jwt>` where the JWT is signed HS256 with `env.MDA_API_SECRET` and carries: | |
| - `exp`: 60 s from issuance, anchored in UTC. | |
| - **Authorization** — `Bearer <jwt>` where the JWT is signed HS256 with `env.MDA_API_SECRET` and carries: | |
| - `exp`: 60 s from issuance for pymta (hardcoded); Postfix/milter uses | |
| `env.MDA_API_JWT_TTL` (default `MDA_API_TIMEOUT * 10 + 60`). |
🤖 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/mta-in/README.md` around lines 34 - 35, Update the authorization contract
in the MTA-in README so it matches both JWT implementations. The current “exp:
60 s” text is only true for pymta’s _build_jwt, while the Postfix/milter path in
src/api/mda.py uses the configurable MDA_API_JWT_TTL default. Revise the
documentation near the Authorization section to describe the shared
HS256/MDA_API_SECRET requirement and note that token lifetime is
implementation-specific or configurable, rather than fixed at 60 seconds.
| - `exp`: 60 s from issuance, anchored in UTC. | ||
| - `body_hash`: `sha256(body).hexdigest()` — binds the token to the exact bytes posted (replay-proof per-request). | ||
| - Plus, for `deliver/`, envelope metadata claims (`sender`, `original_recipients`, `client_address`, `client_port`, `client_hostname`, `client_helo`, `size`). | ||
| - **Response** — `200 OK` + JSON for success; `4xx` for permanent reject; `5xx` for tempfail. Timeouts and transport errors are tempfail too. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm envrcpt/eom in delivery_milter.py don't branch on 4xx vs 5xx.
rg -n -A3 'status_code != 200|status_code == 200' src/mta-in/src/delivery_milter.pyRepository: suitenumerique/messages
Length of output: 593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== README excerpt =="
sed -n '34,44p' src/mta-in/README.md
echo
echo "== delivery_milter.py relevant sections =="
sed -n '60,170p' src/mta-in/src/delivery_milter.py
echo
echo "== pymta.py response handling =="
rg -n -A8 -B4 'def _post|status >= 500|status_code|raise_for_status|TEMPFAIL|REJECT' src/mta-in/src/pymta.pyRepository: suitenumerique/messages
Length of output: 5507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files 'src/mta-in/**' | sed -n '1,200p'
echo
echo "== search for pymta and response handling =="
rg -n -A6 -B4 'def _post|status >= 500|TEMPFAIL|REJECT|status_code' src/mta-inRepository: suitenumerique/messages
Length of output: 34754
Split the response contract by implementation
src/mta-in/README.md:38 says 4xx means permanent reject, but src/mta-in/src/delivery_milter.py turns any non-200 into Milter.TEMPFAIL. pymta is the only path that treats 4xx as a permanent reject, so the README should distinguish the two behaviors instead of describing a single wire contract.
🤖 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/mta-in/README.md` at line 38, Update the response contract in the MTA
inbound README to separate behavior by implementation: the current wording in
the README claims 4xx is a permanent reject, but delivery_milter.py actually
maps any non-200 to Milter.TEMPFAIL while pymta treats 4xx as permanent reject.
Clarify the contract by explicitly documenting the two paths, using the
delivery_milter.py behavior and pymta behavior as the distinguishing symbols,
and avoid describing a single shared rule for all implementations.
Comment on lines
+92
to
+96
| async def _acquire_gate(self, ip: str) -> bool: | ||
| assert self._ip_gate is not None # noqa: S101 — narrowing only; checked above | ||
| try: | ||
| await self._ip_gate._try_acquire(ip) # noqa: SLF001 | ||
| except TooManyConnections as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cross-module private-member access to IPGate internals.
_acquire_gate/_release_gate call IPGate._try_acquire/_release directly (suppressed via # noqa: SLF001). Exposing thin public wrapper methods on IPGate (e.g. acquire/release) would remove the need for the lint suppression and reduce coupling to IPGate's implementation details.
♻️ Proposed refactor sketch
- try:
- await self._ip_gate._try_acquire(ip) # noqa: SLF001
+ try:
+ await self._ip_gate.acquire(ip)(and expose IPGate.acquire/IPGate.release as thin public wrappers around the existing private implementations in limits.py.)
Also applies to: 114-124
🤖 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/mta-in/src/pymta/smtp_protocol.py` around lines 92 - 96, `_acquire_gate`
and `_release_gate` in `SMTPProtocol` are reaching into `IPGate` private
internals via `_try_acquire` and `_release`, which forces the `SLF001`
suppression. Add thin public wrappers on `IPGate` in `limits.py` such as
`acquire` and `release` that delegate to the existing private methods, then
update `SMTPProtocol` to call those public methods instead of the private ones.
Comment on lines
+32
to
+33
| MTA_HOST = os.getenv("MTA_HOST") | ||
| MTA_PORT = int(os.getenv("MTA_PORT", "25")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate MTA_HOST/MTA_PORT reading — reuse the new mta_address fixture.
This module re-derives MTA_HOST/MTA_PORT from env vars independently of conftest.py, which now exposes the exact same pair via the mta_address fixture added in this PR. Two independent sources for the same config risk silent drift (e.g., if the default port changes in one place but not the other).
♻️ Suggested consolidation
-MTA_HOST = os.getenv("MTA_HOST")
-MTA_PORT = int(os.getenv("MTA_PORT", "25"))
-
-
-def _raw_session(timeout: float = 5):
+def _raw_session(host: str = None, port: int = None, timeout: float = 5):
"""Open a raw socket to the MTA, swallow the banner, and return it."""
+ from tests.conftest import MTA_HOST, MTA_PORT
+ host = host or MTA_HOST
+ port = port or MTA_PORT
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.settimeout(timeout)
- s.connect((MTA_HOST, MTA_PORT))
+ s.connect((host, port))🤖 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/mta-in/tests/test_security.py` around lines 32 - 33, The test module is
re-reading the MTA host and port directly from environment variables,
duplicating the same configuration now provided by the mta_address fixture.
Update the security tests to consume mta_address instead of the module-level
MTA_HOST/MTA_PORT constants, and remove the redundant env parsing so there is a
single source of truth for the address in this test suite.
We miss to add X-Mailer header to our outbound messages and this missing can increase the spam score of our messages.
We are currently using react-email to generate html bodies. This library aims to generate marketing email consistent in all mail providers. For personal message, it generates too much custom styles that can increase spam score of those messages.
Postfix was already removed as a mta-out dependency, this is the second step so we have a pure python, more auditable path for incoming emails. We plan to keep postfix as a compatible option for a while but it won't be the default once this is battle tested.
With some mail providers, your current message-id could cause rejection so we migrate to the standard email.utils.make_msgid method.
This refactors our inbound pipeline into a more future-proof, extensible system.
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.
Purpose
Intermediate branch to merge heavy features to be able to validate all of them before releasing.
Summary by CodeRabbit
X-Mailerheader and strengthened delivery/safety metadata.