✨(channels) add encryption, custom scopes and levels, auditing#599
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces legacy bearer-token service keys with channel-scoped API-key auth; adds Channel scope/user/encrypted-settings model fields and validations; introduces channel-scoped permissions/mixins; adds raw-submit and provisioning-mailbox endpoints; updates inbound/outbound message flows, serializers, admin, migrations, routing, OpenAPI, factories, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API_Server
participant Auth as ChannelApiKeyAuthentication
participant Perm as channel_scope Permission
participant DB as Database
participant View as ViewHandler
Client->>API: HTTP request (X-Channel-Id, X-API-Key, ... )
API->>Auth: authenticate(request)
Auth->>DB: SELECT Channel WHERE id=... AND type=API_KEY
DB-->>Auth: Channel{api_key_hashes, scopes, scope_level, expires_at}
Auth->>Auth: SHA256(provided_key) -> compare_digest hashes
alt auth fails or expired
Auth-->>API: AuthenticationFailed (401)
else
Auth->>DB: mark_used() (conditional update)
Auth-->>API: return (AnonymousUser, Channel)
API->>Perm: has_permission(request, view)
Perm->>Perm: check required_scope ∈ channel.settings["scopes"] and global-only rules
alt permission denied
Perm-->>API: PermissionDenied (403)
else
API->>View: dispatch()
View->>DB: perform action (create/lookup/send/rotate)
View-->>API: Response (2xx/4xx/5xx)
API-->>Client: Response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sylvinus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/mda/outbound.py (1)
355-373:⚠️ Potential issue | 🟠 MajorThis validation now leaks a persisted blob on rejection.
compose_and_store_mime()has already created and storedmessage.blobby the timevalidate_mime_size()runs here. If the size check raises, the request fails but that new blob row remains orphaned in storage. The final MIME should be validated beforecreate_blob(), or explicitly cleaned up on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/mda/outbound.py` around lines 355 - 373, compose_and_store_mime currently creates and persists message.blob before validate_mime_size runs, which can leave an orphaned blob if validation fails; update the flow in the outbound send path so that MIME size is validated before any persistent blob is created (i.e. validate the composed MIME bytes/size prior to calling create_blob/create_and_store_blob inside compose_and_store_mime), or if you prefer minimal change, catch validation failures around validate_mime_size and explicitly delete the persisted blob (use the same blob deletion routine used elsewhere) in the except block to ensure cleanup; reference compose_and_store_mime, validate_mime_size, create_blob/create_and_store_blob and message.blob to locate the spots to move the check or add deletion logic.
🧹 Nitpick comments (1)
src/backend/core/api/viewsets/channel.py (1)
68-76: Consider a more explicit contract for passing the generated password.Using a private attribute
_generated_passwordon the model instance creates implicit coupling between the viewset and serializer. The context snippet showsChannelSerializer.create()doesn't currently set this attribute.When implementing the password generation feature, consider alternatives:
- Return a tuple or dict from the serializer's
create()method via a custom mixin- Use a response serializer that includes the password field explicitly
- Add a documented public attribute or method on the Channel model
For now, the
getattr(..., None)fallback makes this forward-compatible and safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/channel.py` around lines 68 - 76, The view currently reads a private attribute _generated_password from the created Channel instance which couples the view to serializer internals; change the contract so ChannelSerializer.create/save returns the generated password explicitly (e.g., return a dict or tuple like {"instance": instance, "generated_password": pwd} or (instance, generated_password)) or have the serializer set a documented public attribute (e.g., instance.generated_password) before returning the instance; then update the viewset code that calls serializer.save(mailbox=self.mailbox) to unpack the returned value or read the public attribute (prefer unpacking a dict/tuple from ChannelSerializer.save/create to find the created instance and password) and include that password in the Response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1524-1555: The _move_sensitive_settings helper only migrates
incoming payloads, leaving existing plaintext secrets in settings and preventing
clearing of previously encrypted keys; update the serializer and model access so
reads never expose plaintext by filtering settings keys listed in
ENCRYPTED_SETTINGS_KEYS for the channel type and returning values only from
encrypted_settings, implement a backfill/migration to move legacy plaintext
values from settings into encrypted_settings, and adjust create/update semantics
so that a full update can remove an encrypted setting when the key is explicitly
set to null (and the merge-path respects explicit nulls vs omitted keys) to
ensure secrets are either encrypted, cleared, or never returned in plaintext.
In `@src/backend/core/api/viewsets/submit.py`:
- Around line 67-70: recipient_emails parsed from rcpt_to_header is validated
but never used, so envelope-only recipients (e.g. Bcc) are not created and
send_message_task never delivers to them; update the message creation flow in
_create_message_from_inbound to merge the parsed recipient_emails into the set
of recipients used to create MessageRecipient rows (ensuring duplicates are
deduped and that Bcc/envelope-only addresses are included even if absent from
MIME headers), and apply the same fix to the analogous logic referenced around
lines 107-116 so all envelope recipients are persisted for send_message_task to
process.
In `@src/backend/core/mda/outbound.py`:
- Around line 309-313: The code validates len(raw_mime) before calling
_sign_and_store, but _sign_and_store prepends a DKIM header which can push the
final payload over the size limit; update the raw-MIME path(s) to re-check the
total size after signing by either (a) having _sign_and_store return the signed
bytes and then call validate_mime_size(len(signed_bytes), message.id) before
persisting, or (b) add a validate_mime_size call inside _sign_and_store
immediately after prepending the DKIM header; apply the same change for the
similar branch around lines 430-438 so messages are size-validated post-signing.
- Around line 375-380: The line overwriting message.has_attachments with
len(attachments) > 0 discards attachments added during compose_and_store_mime()
(like inline signature images); remove that override (or set
message.has_attachments based on the post-composition list such as
all_attachments) so that compose_and_store_mime()’s result is preserved; look
for message.has_attachments, compose_and_store_mime(), attachments and any
all_attachments variable and either delete the reset or replace it with a check
against the final attachment list.
In `@src/backend/core/models.py`:
- Around line 467-474: The ForeignKey field "user" on the Channel model
currently uses on_delete=models.CASCADE which will remove channels if the
creator account is deleted; change its on_delete behavior to models.SET_NULL so
the creator link becomes NULL instead of deleting the Channel (update the user
ForeignKey definition in the Channel model and run a migration to apply the
change). Ensure the field remains null=True, blank=True and that any references
to the creator handle a None value safely.
---
Outside diff comments:
In `@src/backend/core/mda/outbound.py`:
- Around line 355-373: compose_and_store_mime currently creates and persists
message.blob before validate_mime_size runs, which can leave an orphaned blob if
validation fails; update the flow in the outbound send path so that MIME size is
validated before any persistent blob is created (i.e. validate the composed MIME
bytes/size prior to calling create_blob/create_and_store_blob inside
compose_and_store_mime), or if you prefer minimal change, catch validation
failures around validate_mime_size and explicitly delete the persisted blob (use
the same blob deletion routine used elsewhere) in the except block to ensure
cleanup; reference compose_and_store_mime, validate_mime_size,
create_blob/create_and_store_blob and message.blob to locate the spots to move
the check or add deletion logic.
---
Nitpick comments:
In `@src/backend/core/api/viewsets/channel.py`:
- Around line 68-76: The view currently reads a private attribute
_generated_password from the created Channel instance which couples the view to
serializer internals; change the contract so ChannelSerializer.create/save
returns the generated password explicitly (e.g., return a dict or tuple like
{"instance": instance, "generated_password": pwd} or (instance,
generated_password)) or have the serializer set a documented public attribute
(e.g., instance.generated_password) before returning the instance; then update
the viewset code that calls serializer.save(mailbox=self.mailbox) to unpack the
returned value or read the public attribute (prefer unpacking a dict/tuple from
ChannelSerializer.save/create to find the created instance and password) and
include that password in the Response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4081d7f8-4228-46a3-a993-eb1f4fb88b4e
📒 Files selected for processing (17)
src/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/provisioning.pysrc/backend/core/api/viewsets/submit.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0023_channel_encrypted_settings_and_user.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_submit.pysrc/backend/core/tests/mda/test_autoreply.pysrc/backend/core/urls.pysrc/backend/messages/settings.py
💤 Files with no reviewable changes (1)
- src/backend/core/tests/mda/test_autoreply.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/backend/core/tests/api/test_submit.py (2)
150-159: Tighten the status code assertion.The assertion
response.status_code in (400, 404)is imprecise. Looking at the implementation insubmit.pylines 59-65, an invalid UUID raisesValueErrorwhich is caught alongsideDoesNotExist, returning 404. The test should assert the specific expected behavior.♻️ Suggested fix
def test_invalid_uuid_in_x_mail_from_returns_error(self, client, auth_header): response = client.post( SUBMIT_URL, data=MINIMAL_MIME, content_type="message/rfc822", HTTP_X_MAIL_FROM="not-a-uuid", HTTP_X_RCPT_TO="attendee@example.com", **auth_header, ) - assert response.status_code in (400, 404) + assert response.status_code == 404 + assert "not found" in response.json()["detail"].lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_submit.py` around lines 150 - 159, The test test_invalid_uuid_in_x_mail_from_returns_error currently allows 400 or 404 but the handler in submit.py catches ValueError alongside DoesNotExist and returns 404; update the assertion to expect a single specific status (response.status_code == 404) so the test matches the submit.py behavior for invalid UUIDs (leave SUBMIT_URL and MINIMAL_MIME usage unchanged).
208-212: Consider usingMockfor clearer mock object creation.The
type("Message", (), {"id": uuid.uuid4()})()pattern works but is unconventional. Usingunittest.mock.Mockwould be more idiomatic and self-documenting.♻️ Alternative approach
+from unittest.mock import Mock, patch -from unittest.mock import patch ... def test_accepted( self, mock_create, mock_prepare, mock_task, client, auth_header, mailbox ): - fake_message = type("Message", (), {"id": uuid.uuid4()})() + fake_message = Mock(id=uuid.uuid4()) mock_create.return_value = fake_message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_submit.py` around lines 208 - 212, In test_accepted, replace the unconventional dynamic type used for fake_message with an idiomatic unittest.mock.Mock: create a Mock (e.g., fake_message = Mock()) and set its id attribute (fake_message.id = uuid.uuid4()) or use Mock(spec=MessageClass) if a real Message class exists; update imports to include Mock if needed and ensure mock_create.return_value = fake_message remains the same so the test behavior is unchanged.src/backend/core/api/viewsets/submit.py (1)
119-137: Consider logging error conditions for observability.The
loggeris imported but unused. When message creation or preparation fails (returning 500), logging these failures would aid debugging and align with the coding guideline to "log expected and unexpected actions with appropriate log levels."📝 Suggested logging additions
if not message: + logger.error( + "Failed to create message for mailbox %s from raw submission", + mailbox_id, + ) return Response( {"detail": "Failed to create message."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) ... if not prepared: + logger.error( + "Failed to prepare outbound message %s for mailbox %s", + message.id, + mailbox_id, + ) return Response( {"detail": "Failed to prepare message for sending."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/submit.py` around lines 119 - 137, The code currently returns 500 responses when "message" is falsy or "prepared" is falsy without logging; use the imported logger to record these error conditions before returning so failures are observable. Specifically, in the block that checks "if not message:" add a logger.error (or logger.warning) that includes context such as mailbox id/identifier and any relevant exception or details available about the failed create message operation, and likewise before the "if not prepared:" return add a logger.error referencing prepare_outbound_message and include mailbox, message id (or a digest), and raw_mime presence; keep messages concise and consistent with existing logging conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/core/api/viewsets/submit.py`:
- Around line 119-137: The code currently returns 500 responses when "message"
is falsy or "prepared" is falsy without logging; use the imported logger to
record these error conditions before returning so failures are observable.
Specifically, in the block that checks "if not message:" add a logger.error (or
logger.warning) that includes context such as mailbox id/identifier and any
relevant exception or details available about the failed create message
operation, and likewise before the "if not prepared:" return add a logger.error
referencing prepare_outbound_message and include mailbox, message id (or a
digest), and raw_mime presence; keep messages concise and consistent with
existing logging conventions.
In `@src/backend/core/tests/api/test_submit.py`:
- Around line 150-159: The test test_invalid_uuid_in_x_mail_from_returns_error
currently allows 400 or 404 but the handler in submit.py catches ValueError
alongside DoesNotExist and returns 404; update the assertion to expect a single
specific status (response.status_code == 404) so the test matches the submit.py
behavior for invalid UUIDs (leave SUBMIT_URL and MINIMAL_MIME usage unchanged).
- Around line 208-212: In test_accepted, replace the unconventional dynamic type
used for fake_message with an idiomatic unittest.mock.Mock: create a Mock (e.g.,
fake_message = Mock()) and set its id attribute (fake_message.id = uuid.uuid4())
or use Mock(spec=MessageClass) if a real Message class exists; update imports to
include Mock if needed and ensure mock_create.return_value = fake_message
remains the same so the test behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fff35496-b21e-45ae-83d2-c31b5e91ddf6
📒 Files selected for processing (5)
src/backend/core/api/viewsets/submit.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0023_channel_encrypted_settings_and_user.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_submit.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/core/models.py
- src/backend/core/migrations/0023_channel_encrypted_settings_and_user.py
- src/backend/core/mda/outbound.py
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/backend/core/api/viewsets/submit.py (4)
129-146: Pre-compute lowercase recipient set to avoid O(n×m) complexity.Line 133 rebuilds
{e.lower() for e in mime_recipients}on every loop iteration, resulting in O(n×m) time complexity where n = envelope recipients and m = MIME recipients. Pre-compute the lowercase set once before the loop.♻️ Proposed fix
mime_recipients = set( message.recipients.values_list("contact__email", flat=True) ) + mime_recipients_lower = {e.lower() for e in mime_recipients} for addr in recipient_emails: - if addr.lower() not in {e.lower() for e in mime_recipients}: + if addr.lower() not in mime_recipients_lower: try: contact, _ = models.Contact.objects.get_or_create(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/submit.py` around lines 129 - 146, The loop rebuilds {e.lower() for e in mime_recipients} on every iteration causing O(n×m) complexity; precompute a lowercase set (e.g., mime_recipients_lower = {e.lower() for e in mime_recipients}) before iterating recipient_emails and use that set in the membership test, leaving the try/except that creates models.Contact and models.MessageRecipient (and uses models.MessageRecipientTypeChoices.BCC and logger.warning) unchanged.
156-160: Outbound preparation failure lacks diagnostic logging.Similar to message creation,
prepare_outbound_messagereturningFalseproduces a 500 response without server-side logging, hindering troubleshooting.♻️ Proposed fix
if not prepared: + logger.error( + "Failed to prepare outbound message %s for mailbox %s", + message.id, + mailbox.id, + ) return Response( {"detail": "Failed to prepare message for sending."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/submit.py` around lines 156 - 160, The prepare_outbound_message failure branch currently returns a 500 response without any server-side logging; update the branch where prepare_outbound_message is called (the code that checks "if not prepared") to log a detailed error via the existing logger (or process logger) before returning the Response—include identifying context such as the message id, conversation_id or payload summary and any exception or error info available from prepare_outbound_message so operators can diagnose why preparation failed (reference the prepare_outbound_message call and the Response-returning block to make the change).
145-146: Broad exception catch may hide database or integrity errors.Swallowing all exceptions with only a warning log could mask serious issues like database connection failures or constraint violations. Consider catching more specific exceptions (e.g.,
IntegrityError,DatabaseError) or at minimum logging atexceptionlevel to capture the traceback.♻️ Suggested improvement
- except Exception: # pylint: disable=broad-exception-caught - logger.warning("Failed to add BCC recipient %s", addr) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Failed to add BCC recipient %s", addr)Using
logger.exception()will capture the full traceback, making debugging easier while still allowing the submission to proceed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/submit.py` around lines 145 - 146, Replace the broad "except Exception" that currently logs via logger.warning("Failed to add BCC recipient %s", addr) with a narrower catch for database-related errors (e.g., except (IntegrityError, DatabaseError):) and log with logger.exception to capture the traceback; if other non-DB errors should still be tolerated, add a second generic except that logs at exception level before continuing. Make sure to import the specific exceptions (for example from django.db import IntegrityError, DatabaseError) and keep the existing context (addr) in the log message so the failure point in the BCC-addition code is clear.
109-123: Message creation failure lacks diagnostic logging.When
_create_message_from_inboundreturnsNone, a 500 response is returned without any server-side logging. This could make production debugging difficult.♻️ Proposed fix
if not message: + logger.error( + "Failed to create message for mailbox %s from raw MIME submission", + mailbox.id, + ) return Response( {"detail": "Failed to create message."}, status=status.HTTP_500_INTERNAL_SERVER_ERROR, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/submit.py` around lines 109 - 123, The code returns a 500 when _create_message_from_inbound(...) yields None but does not log any diagnostic details; update the submit view to log a clear error before returning by calling the existing logger (e.g., logger or process_logger) with context including the mailbox identifier (mailbox or mailbox.email), recipient_email (mailbox_email), and a short summary of parsed_email (e.g., subject or message-id) so operators can diagnose why _create_message_from_inbound failed; keep sensitive raw_mime out of logs or truncate it if absolutely necessary.src/backend/core/tests/api/test_submit.py (1)
205-210: Mock message factory could be more realistic.The
values_listmock returns a plain list. While this works because the production code wraps it inset(), consider returning a mock QuerySet or at least documenting this coupling in a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_submit.py` around lines 205 - 210, The test factory _fake_message returns msg.recipients.values_list as a plain list which hides the fact production code expects an iterable/QuerySet; update _fake_message so msg.recipients.values_list.return_value is a QuerySet-like mock (e.g., a MagicMock with an __iter__ implementation or a Mock(spec=['__iter__']) that yields "attendee@example.com") or add a short comment in the _fake_message docstring noting the coupling to the production code that wraps values_list in set(); this ensures tests better mirror real behavior used by the submit logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/core/api/viewsets/submit.py`:
- Around line 129-146: The loop rebuilds {e.lower() for e in mime_recipients} on
every iteration causing O(n×m) complexity; precompute a lowercase set (e.g.,
mime_recipients_lower = {e.lower() for e in mime_recipients}) before iterating
recipient_emails and use that set in the membership test, leaving the try/except
that creates models.Contact and models.MessageRecipient (and uses
models.MessageRecipientTypeChoices.BCC and logger.warning) unchanged.
- Around line 156-160: The prepare_outbound_message failure branch currently
returns a 500 response without any server-side logging; update the branch where
prepare_outbound_message is called (the code that checks "if not prepared") to
log a detailed error via the existing logger (or process logger) before
returning the Response—include identifying context such as the message id,
conversation_id or payload summary and any exception or error info available
from prepare_outbound_message so operators can diagnose why preparation failed
(reference the prepare_outbound_message call and the Response-returning block to
make the change).
- Around line 145-146: Replace the broad "except Exception" that currently logs
via logger.warning("Failed to add BCC recipient %s", addr) with a narrower catch
for database-related errors (e.g., except (IntegrityError, DatabaseError):) and
log with logger.exception to capture the traceback; if other non-DB errors
should still be tolerated, add a second generic except that logs at exception
level before continuing. Make sure to import the specific exceptions (for
example from django.db import IntegrityError, DatabaseError) and keep the
existing context (addr) in the log message so the failure point in the
BCC-addition code is clear.
- Around line 109-123: The code returns a 500 when
_create_message_from_inbound(...) yields None but does not log any diagnostic
details; update the submit view to log a clear error before returning by calling
the existing logger (e.g., logger or process_logger) with context including the
mailbox identifier (mailbox or mailbox.email), recipient_email (mailbox_email),
and a short summary of parsed_email (e.g., subject or message-id) so operators
can diagnose why _create_message_from_inbound failed; keep sensitive raw_mime
out of logs or truncate it if absolutely necessary.
In `@src/backend/core/tests/api/test_submit.py`:
- Around line 205-210: The test factory _fake_message returns
msg.recipients.values_list as a plain list which hides the fact production code
expects an iterable/QuerySet; update _fake_message so
msg.recipients.values_list.return_value is a QuerySet-like mock (e.g., a
MagicMock with an __iter__ implementation or a Mock(spec=['__iter__']) that
yields "attendee@example.com") or add a short comment in the _fake_message
docstring noting the coupling to the production code that wraps values_list in
set(); this ensures tests better mirror real behavior used by the submit logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee05df2f-34ca-4279-b6e0-b915a000ed65
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/message.tsis excluded by!**/gen/**
📒 Files selected for processing (2)
src/backend/core/api/viewsets/submit.pysrc/backend/core/tests/api/test_submit.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets/submit.py`:
- Around line 147-148: The warning currently logs PII by including the raw email
variable addr in logger.warning("Failed to add BCC recipient %s", addr); update
this to avoid logging full email — either log a masked representation (e.g., use
a small helper like mask_email(addr) that preserves only non-PII parts such as
the domain or first character) or log a non-PII message (e.g., "Failed to add
BCC recipient (masked)" or "Failed to add BCC recipient; masked address
omitted"); change the logger call in the except block that references addr to
use the masked value or a generic message and add a small helper function
(mask_email) if you choose masking, keeping the variable name addr as the input
to that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeeaeb70-6f1b-4ed8-9c08-3d908ca841f5
📒 Files selected for processing (1)
src/backend/core/api/viewsets/submit.py
sylvinus
force-pushed
the
encrypt_channels
branch
from
df1e105 to
ec2be08
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/api/serializers.py (1)
1545-1579:⚠️ Potential issue | 🟠 MajorMake
typerequired on create and immutable on update.If POST omits
type, thevalidate()method skips theFEATURE_MAILBOX_ADMIN_CHANNELSallow-list check (theif channel_type:condition on line 1797 evaluates toFalse), and the channel defaults to"mta". If PATCH changes an existing channel'stypetoapi_key, theupdate()method never calls_generate_api_key_material(), leavingapi_key_hashesunset and rendering the channel unusable until a separate rotation call.Using
CreateOnlyFieldsMixinwithcreate_only_fields = ["type"]andextra_kwargs = {"type": {"required": True}}closes both holes by enforcing the type at creation and preventing mutation.Suggested fix
-class ChannelSerializer(serializers.ModelSerializer): +class ChannelSerializer(CreateOnlyFieldsMixin, serializers.ModelSerializer): """Serialize Channel model.""" # Explicitly mark nullable fields to fix OpenAPI schema @@ -1567,6 +1567,8 @@ class ChannelSerializer(serializers.ModelSerializer): read_only_fields = [ "id", "mailbox", "maildomain", "user", "scope_level", "last_used_at", "created_at", "updated_at", ] + create_only_fields = ["type"] + extra_kwargs = {"type": {"required": True}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 1545 - 1579, ChannelSerializer currently allows POST to omit type and allows PATCH to mutate type which skips allow-list validation in validate() and can leave api_key_hashes unset because update() may not call _generate_api_key_material(); fix by mixing in CreateOnlyFieldsMixin on ChannelSerializer, add create_only_fields = ["type"] to prevent updates to type, and set extra_kwargs = {"type": {"required": True}} to require type on create so validate() runs properly and update paths that need to generate API key material are preserved.
🧹 Nitpick comments (2)
src/backend/core/tests/api/test_mailbox_usage_metrics.py (1)
37-53: Consider usingfull_clean()beforesave()for model validation.The fixture directly instantiates and saves a
Channelmodel. If model validation (viaclean()) is important for catching invalid configurations, consider callingchannel.full_clean()beforesave(), or using the factory which may handle this.However, since this is test code creating a known-valid channel configuration, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_mailbox_usage_metrics.py` around lines 37 - 53, The fixture correctly_configured_header creates and saves a Channel instance directly without running model validation; call channel.full_clean() right before channel.save() in the correctly_configured_header fixture so Django model validation (clean/field validation) runs and will raise on invalid configuration before persisting the Channel object.src/backend/core/mda/outbound.py (1)
49-53: Clarify the 1.4x multiplier rationale.The formula applies a 1.4x factor to
MAX_OUTGOING_ATTACHMENT_SIZE, but the settings comments (line 969 insettings.py) state: "Raw binary upload, no encoding" for outgoing attachments.For composed MIME (the web/API path), base64 encoding does add ~33% overhead to attachment bytes, so the multiplier makes sense there. However, the raw MIME path (line 311) validates
len(raw_mime)which is already MIME-encoded by the caller—applying the 1.4x factor there may over-permit.Consider documenting the intent or splitting the limit:
# Composed MIME: body + attachments (with base64 overhead) max_total_size = settings.MAX_OUTGOING_BODY_SIZE + int( settings.MAX_OUTGOING_ATTACHMENT_SIZE * 1.4 # ~33% base64 encoding overhead )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/mda/outbound.py` around lines 49 - 53, The 1.4x multiplier in validate_mime_size is ambiguous and may over-permit raw MIME (since raw_mime is already encoded); update validate_mime_size to clearly apply the 1.4 multiplier only for the composed/API path (where attachments are base64-encoded) and not for the raw MIME path, or split the checks into two explicit limits: one for composed MIME (use max_total_size = settings.MAX_OUTGOING_BODY_SIZE + int(settings.MAX_OUTGOING_ATTACHMENT_SIZE * 1.4) with a comment “~33% base64 overhead for composed MIME”) and one strict limit for raw MIME (use settings.MAX_OUTGOING_BODY_SIZE + settings.MAX_OUTGOING_ATTACHMENT_SIZE) so that callers validating len(raw_mime) do not get an inflated allowance; keep references to validate_mime_size and the raw_mime validation call so the change is applied where those are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1586-1608: The _move_sensitive_settings function currently copies
any client-supplied keys (including "api_key_hashes") from settings into
encrypted_settings; change it so "api_key_hashes" is treated as server-managed
and never copied from client input. Specifically, in _move_sensitive_settings
(and the similar logic around the same code region noted), filter out
"api_key_hashes" from keys_to_encrypt or from the extracted dict before
deleting/merging, and when building validated_data["encrypted_settings"] ensure
any existing instance.encrypted_settings["api_key_hashes"] is preserved while
ignoring client-provided api_key_hashes values.
- Around line 1702-1781: Both _validate_api_key_scopes and
_validate_webhook_settings return early when attrs lacks "type", which lets
settings-only PATCHes bypass validation; change each to resolve channel_type =
attrs.get("type") or (self.instance.type if self.instance else None) and then
only skip validation if channel_type is None and "settings" not in attrs (i.e.,
when neither type nor settings are being changed). Update
_validate_api_key_scopes and _validate_webhook_settings to use this fallback and
to perform their existing checks when channel_type indicates API_KEY or WEBHOOK
respectively.
In `@src/backend/core/models.py`:
- Around line 647-663: The throttling check in mark_used uses the stale
in-memory self.last_used_at and can allow concurrent requests to all run the
UPDATE; change the logic to perform the cutoff in the DB update: compute now and
cutoff (timedelta seconds=only_if_older_than_seconds), call
Channel.objects.filter(pk=self.pk).filter(Q(last_used_at__lt=cutoff) |
Q(last_used_at__isnull=True)).update(last_used_at=now) (or equivalent), then
only assign self.last_used_at = now if the update returned a positive
rows_affected; keep the method name mark_used and the intent to avoid
full_clean/save.
In `@src/backend/core/tests/api/test_channel_api_key_auth.py`:
- Around line 51-54: The test docstring in test_missing_headers_returns_401 is
incorrect—update the docstring to reflect a 401 response (e.g., "No headers at
all → DRF returns 401 (unauthorized)") so it matches the assertion asserting
response.status_code == 401; locate the test function
test_missing_headers_returns_401 and change the text accordingly.
---
Outside diff comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1545-1579: ChannelSerializer currently allows POST to omit type
and allows PATCH to mutate type which skips allow-list validation in validate()
and can leave api_key_hashes unset because update() may not call
_generate_api_key_material(); fix by mixing in CreateOnlyFieldsMixin on
ChannelSerializer, add create_only_fields = ["type"] to prevent updates to type,
and set extra_kwargs = {"type": {"required": True}} to require type on create so
validate() runs properly and update paths that need to generate API key material
are preserved.
---
Nitpick comments:
In `@src/backend/core/mda/outbound.py`:
- Around line 49-53: The 1.4x multiplier in validate_mime_size is ambiguous and
may over-permit raw MIME (since raw_mime is already encoded); update
validate_mime_size to clearly apply the 1.4 multiplier only for the composed/API
path (where attachments are base64-encoded) and not for the raw MIME path, or
split the checks into two explicit limits: one for composed MIME (use
max_total_size = settings.MAX_OUTGOING_BODY_SIZE +
int(settings.MAX_OUTGOING_ATTACHMENT_SIZE * 1.4) with a comment “~33% base64
overhead for composed MIME”) and one strict limit for raw MIME (use
settings.MAX_OUTGOING_BODY_SIZE + settings.MAX_OUTGOING_ATTACHMENT_SIZE) so that
callers validating len(raw_mime) do not get an inflated allowance; keep
references to validate_mime_size and the raw_mime validation call so the change
is applied where those are used.
In `@src/backend/core/tests/api/test_mailbox_usage_metrics.py`:
- Around line 37-53: The fixture correctly_configured_header creates and saves a
Channel instance directly without running model validation; call
channel.full_clean() right before channel.save() in the
correctly_configured_header fixture so Django model validation (clean/field
validation) runs and will raise on invalid configuration before persisting the
Channel object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c80d2d0-a684-44ac-b87c-2a79d7beaed9
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/message.tsis excluded by!**/gen/**
📒 Files selected for processing (29)
env.d/development/backend.defaultssrc/backend/core/api/authentication.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/metrics.pysrc/backend/core/api/viewsets/provisioning.pysrc/backend/core/api/viewsets/submit.pysrc/backend/core/apps.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0024_channel_encrypted_settings_scope_level.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_channel_api_key_auth.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_inbound_widget.pysrc/backend/core/tests/api/test_mailbox_usage_metrics.pysrc/backend/core/tests/api/test_maildomain_users_metrics.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_provisioning_maildomains.pysrc/backend/core/tests/api/test_submit.pysrc/backend/core/urls.pysrc/backend/messages/settings.py
💤 Files with no reviewable changes (1)
- env.d/development/backend.defaults
🚧 Files skipped from review as they are similar to previous changes (6)
- src/backend/core/signals.py
- src/backend/core/mda/inbound.py
- src/backend/core/mda/inbound_create.py
- src/backend/core/tests/api/test_channels.py
- src/backend/core/api/openapi.json
- src/backend/core/api/viewsets/channel.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/backend/core/api/serializers.py (1)
1545-1569:⚠️ Potential issue | 🟠 Major
settingsare still serialized back unchanged.Hiding
encrypted_settingsis not enough here. Line 1562 still exposes rawsettings, so any plaintext secret already stored there continues to leak from list/detail responses even thoughencrypted_settingsitself is hidden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 1545 - 1569, ChannelSerializer currently still exposes the plaintext "settings" attribute in responses (despite hiding encrypted_settings); change the serializer to stop returning raw settings from list/detail endpoints by replacing the settings field with a read-only computed field (e.g., use a SerializerMethodField or override to_representation/get_settings) that returns a masked/omitted value (null or sanitized keys) instead of the raw dict, or remove "settings" from the Meta.fields if it must not be returned at all; update ChannelSerializer (the settings field and any to_representation/get_settings helpers) accordingly so stored plaintext secrets are not sent in API responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/permissions.py`:
- Around line 480-492: has_permission assumes channel.settings is a dict and
settings["scopes"] is a list; validate shapes and fail closed: in
has_permission, first ensure request.auth is models.Channel (already done), then
verify channel.settings is a dict and that scopes is a list where every item is
a str (e.g., isinstance(channel.settings, dict) and isinstance(scopes, list) and
all(isinstance(s, str) for s in scopes)); if any check fails, return False. Keep
the subsequent check against enums.CHANNEL_API_KEY_SCOPES_GLOBAL_ONLY and
channel.scope_level unchanged but only run it after the validated scopes list is
obtained.
In `@src/backend/core/api/serializers.py`:
- Around line 1745-1796: The validator currently unconditionally rejects
global-only scopes and non-nested creates, preventing legitimate global API
keys; update _validate_api_key_scopes to compute the effective scope level
(using the same attrs/instance resolution you already use in
_resolve_type_and_settings or by reading attrs.get("scope_level") /
instance.scope_level) and only reject CHANNEL_API_KEY_SCOPES_GLOBAL_ONLY when
that effective scope_level is not GLOBAL, and likewise only enforce the
mailbox/maildomain presence check when the effective scope_level is
MAILBOX/USER; this preserves the existing global save-time path in channel
viewset that sets scope_level=global.
In `@src/backend/core/tests/api/test_channels.py`:
- Around line 499-533: Enable/pin the "api_key" feature flag for this test class
so the two tests (test_post_with_reserved_key_in_settings_is_rejected and
test_unrelated_settings_keys_pass_through) exercise the api_key code paths
rather than relying on ambient FEATURE_MAILBOX_ADMIN_CHANNELS state; do this by
setting the feature flag to True at the class level (e.g., class-level
decorator/fixture or setup that enables the "api_key" flag for the duration of
the tests) and ensure it is cleaned up after the tests.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1545-1569: ChannelSerializer currently still exposes the plaintext
"settings" attribute in responses (despite hiding encrypted_settings); change
the serializer to stop returning raw settings from list/detail endpoints by
replacing the settings field with a read-only computed field (e.g., use a
SerializerMethodField or override to_representation/get_settings) that returns a
masked/omitted value (null or sanitized keys) instead of the raw dict, or remove
"settings" from the Meta.fields if it must not be returned at all; update
ChannelSerializer (the settings field and any to_representation/get_settings
helpers) accordingly so stored plaintext secrets are not sent in API responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f6a9b4-1d1b-4c90-81f6-97fbad5a94f7
📒 Files selected for processing (6)
src/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_channel_api_key_auth.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.py
✅ Files skipped from review due to trivial changes (1)
- src/backend/core/tests/api/test_channel_api_key_auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/tests/api/test_channel_scope_level.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/backend/core/api/permissions.py (1)
527-537: Use DRF'sPermissionDeniedfor consistency in API views.The mixin raises
django.core.exceptions.PermissionDenied, which DRF does handle and convert. However, for DRF views it's more idiomatic and direct to userest_framework.exceptions.PermissionDenied.♻️ Suggested refactor to use DRF exception
from django.core import exceptions from rest_framework import permissions +from rest_framework import exceptions as drf_exceptions from core import enums, modelsThen in the mixin:
# Generic message — do not leak the scope_level requirement # to the caller. - raise exceptions.PermissionDenied() + raise drf_exceptions.PermissionDenied()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/permissions.py` around lines 527 - 537, The initial method in the mixin currently raises the wrong PermissionDenied type; replace the django.core PermissionDenied with DRF's rest_framework.exceptions.PermissionDenied so API views use the DRF exception class consistently. Update the import to reference rest_framework.exceptions.PermissionDenied (or import it as PermissionDenied) and keep the same conditional logic in initial (method name: initial; symbols: request.auth/channel, models.Channel, enums.ChannelScopeLevel.GLOBAL) but raise the DRF PermissionDenied instead of the Django one.src/backend/core/api/openapi.json (1)
6456-6459: Consider constrainingscope_levelwith an enum.Using a free-form string for
scope_levelweakens the contract. Declaring known values (e.g., via enum) will improve client-side validation and reduce integration ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 6456 - 6459, The schema property "scope_level" currently allows any string; constrain it by adding an enum of the known values used by the backend (replace or augment the "scope_level" schema with "type": "string", "enum": [...], and keep "readOnly": true) so OpenAPI clients can validate correctly; update the enum entries to the exact tokens your server emits (e.g., "global", "workspace", "user" or whatever your implementation uses) and regenerate any client SDKs/docs that rely on this component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/openapi.json`:
- Around line 2853-2855: The 200 response for the regenerate API key endpoints
lacks a response schema so clients have no typed contract; update the OpenAPI
entries that currently only include a "200" description (the two occurrences
around the plaintext api_key note) to add a concrete JSON response schema
(either an inline schema or a reusable component like RegeneratedApiKeyResponse)
whose content type is application/json and which defines a single property
api_key: string (with a description stating it is returned once and shown only
once). Ensure the schema is referenced from both response objects (the ones
mentioning plaintext api_key) so SDKs can generate proper types.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6456-6459: The schema property "scope_level" currently allows any
string; constrain it by adding an enum of the known values used by the backend
(replace or augment the "scope_level" schema with "type": "string", "enum":
[...], and keep "readOnly": true) so OpenAPI clients can validate correctly;
update the enum entries to the exact tokens your server emits (e.g., "global",
"workspace", "user" or whatever your implementation uses) and regenerate any
client SDKs/docs that rely on this component.
In `@src/backend/core/api/permissions.py`:
- Around line 527-537: The initial method in the mixin currently raises the
wrong PermissionDenied type; replace the django.core PermissionDenied with DRF's
rest_framework.exceptions.PermissionDenied so API views use the DRF exception
class consistently. Update the import to reference
rest_framework.exceptions.PermissionDenied (or import it as PermissionDenied)
and keep the same conditional logic in initial (method name: initial; symbols:
request.auth/channel, models.Channel, enums.ChannelScopeLevel.GLOBAL) but raise
the DRF PermissionDenied instead of the Django one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e656a13d-c994-414d-96c3-507a1fa6b31d
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel.tsis excluded by!**/gen/**
📒 Files selected for processing (3)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/tests/api/test_channels.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/backend/core/api/viewsets/channel.py (2)
67-78:@extend_schemais attached to the wrong method.Line 67 decorates
get_save_kwargs(helper), socreate()at Line 94 won’t carry the intended explicit request/response schema metadata.🔧 Proposed fix
- `@extend_schema`( - request=serializers.ChannelSerializer, - responses={ - 201: OpenApiResponse( - response=serializers.ChannelSerializer, - description="Channel created successfully", - ), - 400: OpenApiResponse(description="Invalid input data"), - 403: OpenApiResponse(description="Permission denied"), - }, - ) def get_save_kwargs(self): """Hook for subclasses to inject the scope-level + target FKs. @@ return { "mailbox": self.mailbox, "scope_level": ChannelScopeLevel.MAILBOX, "user": self.request.user, } + `@extend_schema`( + request=serializers.ChannelSerializer, + responses={ + 201: OpenApiResponse( + response=serializers.ChannelSerializer, + description="Channel created successfully", + ), + 400: OpenApiResponse(description="Invalid input data"), + 403: OpenApiResponse(description="Permission denied"), + }, + ) def create(self, request, *args, **kwargs):Also applies to: 94-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/channel.py` around lines 67 - 78, The `@extend_schema` decorator is applied to the helper method get_save_kwargs instead of the actual endpoint method create, so move the decorator from get_save_kwargs to the create method on the same viewset (ChannelViewSet) and ensure it uses the same request=serializers.ChannelSerializer and responses mapping; remove the decorator from get_save_kwargs, attach it directly above def create(self, request, *args, **kwargs) so the create endpoint carries the explicit OpenAPI metadata.
187-216: Add audit logging for API key regeneration (without secrets).This is a security-sensitive action; add structured logs for success/failure using channel/user IDs only, and never log plaintext
api_key.As per coding guidelines "Log expected and unexpected actions with appropriate log levels" and "Do not log sensitive information (tokens, passwords, financial/health data, PII)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/channel.py` around lines 187 - 216, The regenerate_api_key method lacks audit logging; add structured logs (no secrets) that record the action, actor and target IDs and outcome: on successful rotation (after instance.save in regenerate_api_key) emit an info-level audit log including the channel id (instance.id), the requesting user id (request.user.id or request.user.pk) and a descriptive message but do NOT include the plaintext api_key; also add an error-level audit log in the exception path if saving or validation fails (include channel id, user id and the exception message/stack) and ensure logging uses existing project logger/audit facility rather than printing secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/openapi.json`:
- Around line 8650-8659: The OpenAPI spec currently documents channel auth only
in prose and mismatches types: update the security definitions to model channel
auth as a first-class security scheme (add a new securitySchemes entry, e.g.,
"channelApiKey" with type: apiKey, in: header, name: "X-Channel-Id") and apply
that scheme to machine-facing paths/operations that consume channel credentials;
also make RegeneratedApiKeyResponse.id use format: uuid to match Channel.id
(change RegeneratedApiKeyResponse -> properties -> id to include "format":
"uuid") so generated clients get a typed authentication path and consistent UUID
typing.
- Around line 8653-8655: RegeneratedApiKeyResponse.id is missing the UUID format
metadata—make it consistent with Channel.id by adding "format": "uuid" to the
RegeneratedApiKeyResponse.id schema in the OpenAPI document; locate the
RegeneratedApiKeyResponse definition and add the format property to the id field
so SDK generators will treat it as a UUID.
---
Nitpick comments:
In `@src/backend/core/api/viewsets/channel.py`:
- Around line 67-78: The `@extend_schema` decorator is applied to the helper
method get_save_kwargs instead of the actual endpoint method create, so move the
decorator from get_save_kwargs to the create method on the same viewset
(ChannelViewSet) and ensure it uses the same
request=serializers.ChannelSerializer and responses mapping; remove the
decorator from get_save_kwargs, attach it directly above def create(self,
request, *args, **kwargs) so the create endpoint carries the explicit OpenAPI
metadata.
- Around line 187-216: The regenerate_api_key method lacks audit logging; add
structured logs (no secrets) that record the action, actor and target IDs and
outcome: on successful rotation (after instance.save in regenerate_api_key) emit
an info-level audit log including the channel id (instance.id), the requesting
user id (request.user.id or request.user.pk) and a descriptive message but do
NOT include the plaintext api_key; also add an error-level audit log in the
exception path if saving or validation fails (include channel id, user id and
the exception message/stack) and ensure logging uses existing project
logger/audit facility rather than printing secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b458de4c-3b26-47c5-b3a2-60fc2b0f5376
⛔ Files ignored due to path filters (5)
src/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel.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/scope_level_enum.tsis excluded by!**/gen/**
📒 Files selected for processing (4)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.py
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/backend/core/admin.py (1)
481-517: Consider adding exception handling forrotate_api_key()call.If
rotate_api_key()raises an exception (e.g., database error, encryption failure), the view will return a 500 error. Adding a try-except block would allow displaying a user-friendly error message in the admin interface.Proposed improvement
if channel.type != ChannelTypes.API_KEY: messages.error( request, "Only api_key channels can have their secret regenerated." ) return redirect("..") - plaintext = channel.rotate_api_key() + try: + plaintext = channel.rotate_api_key() + except Exception: + logging.exception("Failed to rotate API key for channel %s", channel.id) + messages.error(request, "Failed to regenerate API key. Please try again.") + return redirect("..") context = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/admin.py` around lines 481 - 517, Wrap the call to channel.rotate_api_key() inside a try/except in regenerate_api_key_view so any exception (e.g., DB/encryption failure) is caught; on exception log the error (use logger.exception) and call messages.error(request, "Failed to regenerate API key.") then redirect back (e.g., return redirect("..")) instead of letting a 500 bubble up; keep successful flow returning the TemplateResponse with the plaintext when no exception occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1828-1842: The code currently only validates channel type when
attrs includes "type", allowing nested mailbox/user creations to default to the
model default ("mta") and bypass FEATURE_MAILBOX_ADMIN_CHANNELS; update the
serializer branch (the method containing the shown block) to require that attrs
contains "type" when self.context.get("mailbox") or
self.context.get("user_channel") is true: if no type present, raise
serializers.ValidationError with a clear message like "Channel type is required
for mailbox/user channel creation"; otherwise proceed to check allowed_types (as
currently done) and then call _reject_caller_supplied_encrypted_keys,
_validate_api_key_scopes, and _validate_webhook_settings before returning attrs.
In `@src/backend/core/tests/api/test_channel_api_key_auth.py`:
- Around line 104-111: The test currently allows a 500 response which hides
server errors; update the assertion on response.status_code in
test_channel_api_key_auth.py to only accept 400, 403, or 202 (remove 500) and
keep the rest of the test flow the same (leave the channel.refresh_from_db() and
last_used_at check intact); also update the inline comment above the assertion
to reflect the tightened set of expected status codes so the intent matches the
assertion.
- Around line 57-65: In test_expired_key_returns_401 replace the incorrect use
of timezone.timedelta with datetime.timedelta: add an import for timedelta from
the datetime module and change the expression timezone.timedelta(days=1) to just
timedelta(days=1) in the _make_channel call; update the import section and the
test function (test_expired_key_returns_401) accordingly so the expires_at value
is computed using the datetime.timedelta class.
In `@src/backend/core/tests/api/test_channels.py`:
- Around line 442-470: The tests test_encrypted_settings_not_in_api_response and
test_encrypted_settings_not_in_list_response only assert top-level absence of
"encrypted_settings" but must also ensure no secrets are nested inside the
"settings" payload; update each test to additionally inspect
response.data.get("settings") (for the detail view) and each
item.get("settings") (for the list view) and assert those dicts do not contain
secret keys (e.g., "password", "token") or any value copied from
Channel.encrypted_settings so that any serializer bug copying encrypted_settings
into settings is caught.
In `@src/backend/core/tests/api/test_mailbox_usage_metrics.py`:
- Around line 67-82: The test test_requires_auth currently exercises a
non-existent channel (HTTP_X_CHANNEL_ID=str(uuid.uuid4())) which only covers the
Channel.DoesNotExist path; change the invalid-auth case to reuse the real
channel id from correctly_configured_header (extract HTTP_X_CHANNEL_ID or the
channel object's id provided by that fixture) and send a bad HTTP_X_API_KEY
value so the hash/secret-check branch is exercised and still asserts a 401
response.
In `@src/backend/core/tests/api/test_provisioning_mailbox.py`:
- Around line 59-61: The class docstring for TestServiceAuthSecurity incorrectly
references HasCalendarsApiKey; update it to state that the provisioning endpoint
requires ChannelApiKeyScope.MAILBOXES_READ (or similarly worded, e.g., "requires
ChannelApiKeyScope.MAILBOXES_READ") so it accurately reflects the mailbox
provisioning tests and the scope enforced by the code under test.
---
Nitpick comments:
In `@src/backend/core/admin.py`:
- Around line 481-517: Wrap the call to channel.rotate_api_key() inside a
try/except in regenerate_api_key_view so any exception (e.g., DB/encryption
failure) is caught; on exception log the error (use logger.exception) and call
messages.error(request, "Failed to regenerate API key.") then redirect back
(e.g., return redirect("..")) instead of letting a 500 bubble up; keep
successful flow returning the TemplateResponse with the plaintext when no
exception occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 755bf472-2a74-4967-93be-54c59e8717d1
📒 Files selected for processing (18)
src/backend/core/admin.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/models.pysrc/backend/core/templates/admin/_object_tools_button_styles.htmlsrc/backend/core/templates/admin/core/channel/change_form.htmlsrc/backend/core/templates/admin/core/channel/regenerated_api_key.htmlsrc/backend/core/templates/admin/core/maildomain/change_form.htmlsrc/backend/core/tests/api/test_channel_api_key_auth.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_mailbox_usage_metrics.pysrc/backend/core/tests/api/test_maildomain_users_metrics.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_provisioning_maildomains.pysrc/backend/core/tests/api/test_submit.py
✅ Files skipped from review due to trivial changes (2)
- src/backend/core/templates/admin/core/maildomain/change_form.html
- src/backend/core/enums.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/backend/core/tests/api/test_maildomain_users_metrics.py
- src/backend/core/factories.py
- src/backend/core/tests/api/test_submit.py
- src/backend/core/tests/api/test_provisioning_maildomains.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/backend/core/migrations/0024_channel_encrypted_settings_scope_level.py (1)
68-77:⚠️ Potential issue | 🟠 MajorBackfill legacy secrets before shipping this schema.
This migration adds
encrypted_settings, but the only data step here backfillsscope_level. Any pre-existing secret-bearing keys already stored inChannel.settingsstay in plaintext after deploy, andsrc/backend/core/api/serializers.py:1558-1580still returnssettingsverbatim, so legacy rows remain exposed. Please add a data migration that moves the old sensitive keys intoencrypted_settingsand removes them fromsettings(or temporarily scrub them on read until the backfill is complete).Also applies to: 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/migrations/0024_channel_encrypted_settings_scope_level.py` around lines 68 - 77, Add a data migration after the AddField that iterates Channel rows and moves legacy secret keys from Channel.settings into the new Channel.encrypted_settings field (e.g., copy keys like api_key/app_password/secret into encrypted_settings, delete them from settings, and save), performing the operation in batches (iterator/bulk_update) to avoid memory/N+1 issues; also temporarily update the serializer at src/backend/core/api/serializers.py (around the code that reads settings) to prefer values from encrypted_settings for those sensitive keys (or scrub those keys on read) until the backfill completes so secrets are not exposed during deployment. Ensure you reference the Channel model, encrypted_settings EncryptedJSONField, and the serializer code paths when implementing the migration and serializer change.src/backend/core/api/serializers.py (1)
1548-1550:⚠️ Potential issue | 🟠 MajorThe root/global channel path is still blocked.
mailboxandmaildomainare read-only here, so the non-nested branch can never receive them inattrs. That makes theEither mailbox or maildomain must be specifiedcheck reject every top-level create/update, and Lines 1774-1778 still reject the global-only scopes thatsrc/backend/core/api/viewsets/channel.py:95-121needs when it savesscope_level=global. The new provisioning/metrics API-key channels still can't be managed through DRF until validation resolves the effective target/scope from context/save kwargs instead of rawattrs.Also applies to: 1769-1778, 1828-1877
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 1548 - 1550, The serializer currently treats mailbox and maildomain as read-only, so validation that checks attrs for those fields always fails for non-nested/top-level operations; update the serializer's validation logic (the serializer class that defines the PrimaryKeyRelatedField fields mailbox, maildomain, user and its validate/create/update methods) to derive the effective target/scope from the request/context/save kwargs/instance when attrs lack those keys instead of only inspecting attrs. Concretely, change the validation that enforces "Either mailbox or maildomain must be specified" to check: 1) attrs, 2) self.instance (for updates), and 3) self.context (view.kwargs or explicitly passed save kwargs) to determine mailbox/maildomain, and only reject when none of these sources provide a target; apply the same approach to the other validation branches referenced around the serializer's create/update/validate methods so global-only scope_level operations succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/tests/api/test_provisioning_mailbox.py`:
- Around line 77-84: Add a new unit test alongside
test_user_email_unknown_channel_returns_401 that sends a GET to MAILBOX_URL with
{"user_email": "a@b.com"} but with HTTP_X_CHANNEL_ID set to a malformed string
like "not-a-uuid" (and HTTP_X_API_KEY="anything"), and assert
response.status_code == 401 to exercise the invalid-UUID authentication branch;
name the test something like test_user_email_malformed_channel_returns_401 so
it’s clear and focused.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1548-1550: The serializer currently treats mailbox and maildomain
as read-only, so validation that checks attrs for those fields always fails for
non-nested/top-level operations; update the serializer's validation logic (the
serializer class that defines the PrimaryKeyRelatedField fields mailbox,
maildomain, user and its validate/create/update methods) to derive the effective
target/scope from the request/context/save kwargs/instance when attrs lack those
keys instead of only inspecting attrs. Concretely, change the validation that
enforces "Either mailbox or maildomain must be specified" to check: 1) attrs, 2)
self.instance (for updates), and 3) self.context (view.kwargs or explicitly
passed save kwargs) to determine mailbox/maildomain, and only reject when none
of these sources provide a target; apply the same approach to the other
validation branches referenced around the serializer's create/update/validate
methods so global-only scope_level operations succeed.
In `@src/backend/core/migrations/0024_channel_encrypted_settings_scope_level.py`:
- Around line 68-77: Add a data migration after the AddField that iterates
Channel rows and moves legacy secret keys from Channel.settings into the new
Channel.encrypted_settings field (e.g., copy keys like
api_key/app_password/secret into encrypted_settings, delete them from settings,
and save), performing the operation in batches (iterator/bulk_update) to avoid
memory/N+1 issues; also temporarily update the serializer at
src/backend/core/api/serializers.py (around the code that reads settings) to
prefer values from encrypted_settings for those sensitive keys (or scrub those
keys on read) until the backfill completes so secrets are not exposed during
deployment. Ensure you reference the Channel model, encrypted_settings
EncryptedJSONField, and the serializer code paths when implementing the
migration and serializer change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a28d730-658c-4f5d-969a-24fc00136640
📒 Files selected for processing (8)
src/backend/core/api/serializers.pysrc/backend/core/migrations/0024_channel_encrypted_settings_scope_level.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_channel_api_key_auth.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_mailbox_usage_metrics.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_provisioning_maildomains.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/tests/api/test_provisioning_maildomains.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/backend/core/tests/api/test_mailbox_usage_metrics.py (1)
65-86: Consider splittingtest_requires_authinto focused test cases.This method currently validates three behaviors at once (no headers, invalid key, valid key). Splitting into dedicated tests will improve failure localization and keep each unit test narrower.
♻️ Suggested refactor
- def test_requires_auth(self, api_client, url, correctly_configured_header): - """Requires a valid api_key Channel to access. - - No headers → 401 (NotAuthenticated). Invalid → 401. Valid → 200. - """ + def test_requires_auth_without_headers(self, api_client, url): + """No auth headers returns 401.""" response = api_client.get(url) assert response.status_code == 401 - response = api_client.get( + def test_requires_auth_with_invalid_key(self, api_client, url, correctly_configured_header): + """Wrong API key for an existing channel returns 401.""" + response = api_client.get( url, HTTP_X_CHANNEL_ID=correctly_configured_header["HTTP_X_CHANNEL_ID"], HTTP_X_API_KEY="invalid_token", ) assert response.status_code == 401 - response = api_client.get(url, **correctly_configured_header) + def test_requires_auth_with_valid_key(self, api_client, url, correctly_configured_header): + """Valid channel/key returns 200.""" + response = api_client.get(url, **correctly_configured_header) assert response.status_code == 200As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_mailbox_usage_metrics.py` around lines 65 - 86, Split the combined test_requires_auth into three focused tests to isolate behaviors: create test_no_auth_returns_401 that calls api_client.get(url) and asserts 401, test_invalid_auth_returns_401 that calls api_client.get(url, HTTP_X_CHANNEL_ID=correctly_configured_header["HTTP_X_CHANNEL_ID"], HTTP_X_API_KEY="invalid_token") and asserts 401, and test_valid_auth_returns_200 that calls api_client.get(url, **correctly_configured_header) and asserts 200; keep the same fixtures (api_client, url, correctly_configured_header) and move each assertion into its own test function so failures pinpoint the exact case.src/backend/core/api/openapi.json (1)
6495-6501: Clarify the semantics ofChannel.user.This field is new, but its current description only says “UUID”. In the viewset logic it is the owning user for
scope_level=userchannels and the creator/audit user for other scopes, so the schema text should call that out to prevent client-side misinterpretation.✏️ Proposed wording
"user": { "type": "string", "format": "uuid", - "description": "primary key for the record as UUID", + "description": "Owning user for scope_level=user channels; creator/audit user for other channel scopes.", "readOnly": true, "nullable": true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 6495 - 6501, Update the OpenAPI schema description for the Channel.user property to precisely describe its semantics: state that Channel.user is a UUID referencing the owning user for channels with scope_level="user", and for channels with other scope levels it represents the creator/audit user; keep readOnly and nullable behavior unchanged and ensure the wording clarifies it is the authoritative user identifier used by the viewset logic (reference symbol: "user" property under the Channel schema in openapi.json).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/openapi.json`:
- Around line 2825-2843: The path parameter definitions for channel routes
currently declare the "id" path params as plain strings; update each path
parameter object that refers to Channel.id (the "id" path parameter) to include
"format": "uuid" so they match the Channel.id schema (e.g., change the "id"
parameter schema from { "type": "string" } to { "type": "string", "format":
"uuid" }); do this for every occurrence mentioned (the parameter blocks around
the listed ranges) and ensure "mailbox_id" already using "format":"uuid" remains
unchanged.
In `@src/backend/core/models.py`:
- Around line 594-636: api_key_covers currently allows ambiguous inputs (both
mailbox and maildomain) to be resolved by branch order; add an explicit early
check in api_key_covers (in src/backend/core/models.py) that if both mailbox and
maildomain are provided the function fails closed (return False) to
deterministically reject conflicting resources; keep existing ChannelScopeLevel
handling (GLOBAL, MAILDOMAIN, MAILBOX, USER) unchanged otherwise and adjust
tests that passed both arguments to expect a rejection.
In `@src/backend/core/tests/api/test_provisioning_maildomains.py`:
- Around line 31-33: The docstring for the auth_header helper is inconsistent
with the actual scope used in tests; update the auth_header docstring to
reference ChannelApiKeyScope.MAILDOMAINS_CREATE (or change the enum used to
match "maildomains:write" if that label is preferred) so the text and the enum
(ChannelApiKeyScope.MAILDOMAINS_CREATE) are aligned and unambiguous.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6495-6501: Update the OpenAPI schema description for the
Channel.user property to precisely describe its semantics: state that
Channel.user is a UUID referencing the owning user for channels with
scope_level="user", and for channels with other scope levels it represents the
creator/audit user; keep readOnly and nullable behavior unchanged and ensure the
wording clarifies it is the authoritative user identifier used by the viewset
logic (reference symbol: "user" property under the Channel schema in
openapi.json).
In `@src/backend/core/tests/api/test_mailbox_usage_metrics.py`:
- Around line 65-86: Split the combined test_requires_auth into three focused
tests to isolate behaviors: create test_no_auth_returns_401 that calls
api_client.get(url) and asserts 401, test_invalid_auth_returns_401 that calls
api_client.get(url,
HTTP_X_CHANNEL_ID=correctly_configured_header["HTTP_X_CHANNEL_ID"],
HTTP_X_API_KEY="invalid_token") and asserts 401, and test_valid_auth_returns_200
that calls api_client.get(url, **correctly_configured_header) and asserts 200;
keep the same fixtures (api_client, url, correctly_configured_header) and move
each assertion into its own test function so failures pinpoint the exact case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bf5ae83-31c1-40f6-a24a-5983c53ec8bb
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/channel.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel_request.tsis excluded by!**/gen/**
📒 Files selected for processing (10)
src/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/migrations/0024_channel_encrypted_settings_scope_level.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_channel_api_key_auth.pysrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_mailbox_usage_metrics.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_provisioning_maildomains.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/core/tests/api/test_channel_scope_level.py
- src/backend/core/api/serializers.py
sylvinus
changed the title
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes
Tests