Skip to content

✨(inbound) add prototype Brevo inbound hook for email delivery#598

Open
sylvinus wants to merge 1 commit intomainfrom
brevo
Open

✨(inbound) add prototype Brevo inbound hook for email delivery#598
sylvinus wants to merge 1 commit intomainfrom
brevo

Conversation

@sylvinus
Copy link
Copy Markdown
Member

@sylvinus sylvinus commented Mar 30, 2026

Based on https://developers.brevo.com/reference/create-webhook

Summary by CodeRabbit

Release Notes

  • New Features

    • Added Brevo email service integration with webhook support for processing inbound messages
    • Implemented dual authentication methods: channel-based authorization and HMAC signature verification
    • Added new endpoint to validate email address deliverability against local system configuration
  • Tests

    • Added comprehensive test coverage for Brevo integration, including authentication validation, webhook processing, and end-to-end message delivery verification

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

New Brevo inbound API implementation added with two authentication modes (channel-scoped and HMAC signature validation), a viewset handling webhook delivery and address validation, payload conversion to internal email format, and comprehensive test coverage including fixtures and end-to-end verification.

Changes

Cohort / File(s) Summary
Brevo Inbound Implementation
src/backend/core/api/viewsets/inbound/brevo.py
Added BrevoAuthentication supporting channel-scoped requests (via X-Channel-ID) and HMAC-based validation (via X-Brevo-Signature with sha256(secret + request.body)). Added InboundBrevoViewSet with webhook action processing item lists and returning appropriate status codes (400 for invalid input, 500 if all fail, 207 for partial success, 200 if all succeed) and check action validating local deliverability of email addresses. Added convert_brevo_payload_to_parsed_email helper to transform Brevo payloads into internal parsed email structures with fallback from HTML to markdown content.
Brevo Tests
src/backend/core/tests/api/test_inbound_brevo.py
Added fixtures for API client and Brevo channel/payload test objects. Added unit tests for payload conversion, authentication (channel ID, HMAC signature, missing credentials), webhook endpoint (validation, authentication, item processing, delivery), address checking endpoint, and end-to-end tests verifying thread association, sender/subject extraction, label propagation, and reply threading.
URL Routing
src/backend/core/urls.py
Registered new brevo inbound route under /api/{API_VERSION}/inbound/ router with basename="inbound-brevo".

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant Webhook as Brevo Webhook
    participant Auth as BrevoAuthentication
    participant ViewSet as InboundBrevoViewSet
    participant DB as Channel DB
    participant Delivery as Local Delivery

    Client->>Webhook: POST /webhook with signature/channel-id
    Webhook->>Auth: authenticate(request)
    
    alt Channel-scoped (X-Channel-ID present)
        Auth->>DB: Load Brevo Channel by ID
        DB-->>Auth: Channel object
        Auth-->>Webhook: Authenticated with channel
    else HMAC-based
        Auth->>Auth: Validate X-Brevo-Signature<br/>sha256(secret + body)
        Auth-->>Webhook: Authenticated
    end
    
    Webhook->>ViewSet: webhook(request) with items
    ViewSet->>ViewSet: Validate items list
    
    loop For each item
        ViewSet->>ViewSet: _process_brevo_item()
        ViewSet->>ViewSet: Extract From/To addresses
        ViewSet->>ViewSet: Check local deliverability
        ViewSet->>ViewSet: Convert to parsed email
        ViewSet->>Delivery: deliver_inbound_message()
        Delivery-->>ViewSet: Delivery result
    end
    
    alt All items failed
        ViewSet-->>Client: 500 with details
    else Some items succeeded
        ViewSet-->>Client: 207 Multi-Status
    else All items succeeded
        ViewSet-->>Client: 200 OK
    end
Loading
sequenceDiagram
    participant ViewSet as InboundBrevoViewSet
    participant Validator as Address Validator
    participant Converter as Payload Converter
    participant Delivery as Delivery Engine
    participant Logger as Logger

    ViewSet->>Validator: check_local_recipients(addresses)
    Validator-->>ViewSet: Deliverability map
    
    ViewSet->>Converter: convert_brevo_payload_to_parsed_email(item)
    Note over Converter: Extract subject, from, to<br/>Fallback HTML→markdown
    Converter-->>ViewSet: Parsed email dict
    
    ViewSet->>ViewSet: Build RFC raw email<br/>with prepend headers
    
    loop For each local recipient
        ViewSet->>Delivery: deliver_inbound_message()<br/>skip_inbound_queue=True
        Delivery->>Logger: Log delivery attempt
        Logger-->>Delivery: ✓
        Delivery-->>ViewSet: Delivery result
    end
    
    ViewSet->>ViewSet: Accumulate success/failure counts
    ViewSet->>ViewSet: Compose response<br/>(status code + results)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 Brevo hops into the warren bright,
With channels verified and signatures tight,
Items processed at lightning speed,
Delivering joy to all we feed! 📧✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly indicates the main change: adding a Brevo inbound webhook implementation for email delivery, which is fully supported by the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch brevo

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines +196 to +200
{
"status": "error",
"detail": "Failed to process all messages",
"results": results,
},

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.

Copilot Autofix

AI about 1 month ago

In general, the fix is to avoid returning raw exception messages to the client while still logging full details on the server. The server should send a generic error description (for example, "Internal processing error") and keep logger.exception(...) so that developers can diagnose issues from logs.

For this specific code, the best fix with minimal behavior change is:

  • Keep the logger.exception("Error processing Brevo item: %s", e) call as-is so stack traces are logged server-side.
  • Change the returned dict in the except block of _process_brevo_item to use a generic message instead of str(e). For example:
    • return {"success": False, "error": "Failed to process Brevo item"}.
  • No changes are needed in the webhook method; it will still aggregate results, but those results will now contain only non-sensitive error strings.
  • No new imports or helper methods are strictly necessary.

All changes are confined to src/backend/core/api/viewsets/inbound/brevo.py, within the shown _process_brevo_item method, specifically lines 274–276.

Suggested changeset 1
src/backend/core/api/viewsets/inbound/brevo.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/backend/core/api/viewsets/inbound/brevo.py b/src/backend/core/api/viewsets/inbound/brevo.py
--- a/src/backend/core/api/viewsets/inbound/brevo.py
+++ b/src/backend/core/api/viewsets/inbound/brevo.py
@@ -273,7 +273,7 @@
 
         except Exception as e:
             logger.exception("Error processing Brevo item: %s", e)
-            return {"success": False, "error": str(e)}
+            return {"success": False, "error": "Failed to process Brevo item"}
 
     @extend_schema(exclude=True)
     @action(
EOF
@@ -273,7 +273,7 @@

except Exception as e:
logger.exception("Error processing Brevo item: %s", e)
return {"success": False, "error": str(e)}
return {"success": False, "error": "Failed to process Brevo item"}

@extend_schema(exclude=True)
@action(
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines +206 to +211
{
"status": "partial_success",
"processed": success_count,
"failed": failure_count,
"results": results,
},

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.

Copilot Autofix

AI about 1 month ago

In general, to fix information exposure through exceptions, stop returning raw exception objects or messages in API responses. Instead, log detailed errors on the server and send a generic, user‑safe message (optionally with a non‑sensitive error code) in the response.

Here, the best fix is to change _process_brevo_item so that the except Exception as e block no longer includes str(e) in the returned dictionary. We already call logger.exception("Error processing Brevo item: %s", e), which captures the stack trace and message server‑side. The webhook response should instead return a generic error description, e.g. "Internal error processing message" and perhaps a stable error code like "internal_error". This keeps the public API behavior (indicating which items failed and that they failed) while avoiding leaking implementation details.

Concretely, in src/backend/core/api/viewsets/inbound/brevo.py, around lines 274–276, replace:

        except Exception as e:
            logger.exception("Error processing Brevo item: %s", e)
            return {"success": False, "error": str(e)}

with something like:

        except Exception as e:
            logger.exception("Error processing Brevo item: %s", e)
            return {
                "success": False,
                "error": "Internal error processing message",
                "error_code": "internal_error",
            }

No new imports or helper methods are needed.

Suggested changeset 1
src/backend/core/api/viewsets/inbound/brevo.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/backend/core/api/viewsets/inbound/brevo.py b/src/backend/core/api/viewsets/inbound/brevo.py
--- a/src/backend/core/api/viewsets/inbound/brevo.py
+++ b/src/backend/core/api/viewsets/inbound/brevo.py
@@ -273,7 +273,11 @@
 
         except Exception as e:
             logger.exception("Error processing Brevo item: %s", e)
-            return {"success": False, "error": str(e)}
+            return {
+                "success": False,
+                "error": "Internal error processing message",
+                "error_code": "internal_error",
+            }
 
     @extend_schema(exclude=True)
     @action(
EOF
@@ -273,7 +273,11 @@

except Exception as e:
logger.exception("Error processing Brevo item: %s", e)
return {"success": False, "error": str(e)}
return {
"success": False,
"error": "Internal error processing message",
"error_code": "internal_error",
}

@extend_schema(exclude=True)
@action(
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines +220 to +224
{
"status": "ok",
"processed": success_count,
"results": results,
}

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.

Copilot Autofix

AI about 1 month ago

In general, to fix information exposure through exceptions, you should avoid including raw exception messages or stack traces in HTTP responses. Instead, log the full details on the server (for operators and developers) and return a generic, non-sensitive error description to the client, optionally with a stable error code.

In this file, the minimal, non‑breaking fix is to change _process_brevo_item so that it no longer returns str(e) as the error value. We should keep the logger.exception(...) call so the full stack trace and exception details are available in logs, but the dictionary returned to the caller should contain only a generic error message, such as "Internal error processing message" and possibly a stable error code like "processing_error". The rest of the logic in webhook that aggregates results and counts successes/failures can remain unchanged; it will still indicate which items failed without leaking internal details.

Concretely:

  • Edit src/backend/core/api/viewsets/inbound/brevo.py in _process_brevo_item, lines 274–276.
  • Keep the logger.exception("Error processing Brevo item: %s", e) line.
  • Replace return {"success": False, "error": str(e)} with a generic structure like:
    • {"success": False, "error": "Internal error processing message", "error_code": "processing_error"} (or simply a generic error string if you prefer).
      No new imports or helper methods are required; we just alter the returned payload.

Suggested changeset 1
src/backend/core/api/viewsets/inbound/brevo.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/backend/core/api/viewsets/inbound/brevo.py b/src/backend/core/api/viewsets/inbound/brevo.py
--- a/src/backend/core/api/viewsets/inbound/brevo.py
+++ b/src/backend/core/api/viewsets/inbound/brevo.py
@@ -273,7 +273,11 @@
 
         except Exception as e:
             logger.exception("Error processing Brevo item: %s", e)
-            return {"success": False, "error": str(e)}
+            return {
+                "success": False,
+                "error": "Internal error processing message",
+                "error_code": "processing_error",
+            }
 
     @extend_schema(exclude=True)
     @action(
EOF
@@ -273,7 +273,11 @@

except Exception as e:
logger.exception("Error processing Brevo item: %s", e)
return {"success": False, "error": str(e)}
return {
"success": False,
"error": "Internal error processing message",
"error_code": "processing_error",
}

@extend_schema(exclude=True)
@action(
Copilot is powered by AI and may make mistakes. Always verify output.
@sylvinus
Copy link
Copy Markdown
Member Author

sylvinus commented Apr 6, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 6, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
src/backend/core/api/viewsets/inbound/brevo.py (1)

274-276: Narrow the broad exception catch.

Catching bare Exception is flagged by pylint. While the try-except structure is appropriate for this processing loop, consider catching more specific exceptions or at minimum excluding system-exiting exceptions.

♻️ Proposed fix to narrow exception scope
-        except Exception as e:
+        except (ValueError, KeyError, TypeError, ValidationError) as e:
             logger.exception("Error processing Brevo item: %s", e)
-            return {"success": False, "error": str(e)}
+            return {"success": False, "error": "Internal processing error"}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/viewsets/inbound/brevo.py` around lines 274 - 276, The
current broad "except Exception as e" in the Brevo processing loop (the block
that calls logger.exception("Error processing Brevo item: %s", e) and returns
{"success": False, "error": str(e)}) should be narrowed: replace it with
specific exception handlers for the expected error types (for example
requests.RequestException, json.JSONDecodeError, KeyError, or any Brevo-specific
API exception your code uses) and handle each with appropriate logging and the
same error return; if you still include a generic catch-all, first re-raise
system-exiting exceptions by checking isinstance(e, (KeyboardInterrupt,
SystemExit)) before logging/returning to avoid swallowing them.
🤖 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/inbound/brevo.py`:
- Around line 194-224: The response `results` currently includes raw exception
strings (str(e)) which may leak internal details; update the exception handlers
that append to the `results` list (where you currently append str(e)) to instead
append a sanitized, generic error object or message (e.g., {"error": "internal
processing error"} or {"error": "failed to process message"}) and move the
detailed exception into server-side logs using logger.exception(...) or
logger.error(..., exc_info=True); keep `failure_count`/`success_count` logic
intact and ensure only the sanitized messages are returned in the
error/partial_success Responses while full exception details remain in the logs.
- Around line 145-152: InboundBrevoViewSet is using IsAuthenticated while
BrevoAuthentication.authenticate() returns (None, auth_data), causing
request.user to be None and IsAuthenticated to reject requests; change
permission_classes to [AllowAny] and enforce BrevoAuthentication manually in the
viewset (e.g., override initial or a before-action method on InboundBrevoViewSet
to call BrevoAuthentication().authenticate(request) and raise PermissionDenied
when auth fails), so authentication logic lives in
BrevoAuthentication.authenticate and permission gating is handled explicitly in
the viewset.

In `@src/backend/core/tests/api/test_inbound_brevo.py`:
- Line 315: The test currently asserts response.status_code ==
status.HTTP_403_FORBIDDEN for an unauthenticated request; update the assertion
to expect status.HTTP_401_UNAUTHORIZED instead (i.e., assert
response.status_code == status.HTTP_401_UNAUTHORIZED) so it matches DRF's
AuthenticationFailed behavior, using the same response and status symbols
already in the test.
- Around line 407-413: The test mock data references a non-existent
Mailbox.address attribute; replace all occurrences of mailbox.address with
str(mailbox) so the mocked "Name", "Address" and "Recipients" fields use the
Mailbox.__str__() output (local_part@domain); update the two occurrences in the
test_inbound_brevo.py mock objects where "Name": mailbox.address, "Address":
mailbox.address and {"Address": mailbox.address} / "Recipients":
[mailbox.address] appear to use str(mailbox) instead.
- Around line 453-456: The test incorrectly uses thread__mailbox when creating
existing_message; instead create a Thread via ThreadFactory, link it to the
mailbox using ThreadAccessFactory(thread=thread, mailbox=mailbox), then create
existing_message with MessageFactory(thread=thread,
mime_id="original-message-id@example.com"). Update references in the test to use
the new thread variable and ThreadAccessFactory to ensure the Thread->Mailbox
relationship exists.

---

Nitpick comments:
In `@src/backend/core/api/viewsets/inbound/brevo.py`:
- Around line 274-276: The current broad "except Exception as e" in the Brevo
processing loop (the block that calls logger.exception("Error processing Brevo
item: %s", e) and returns {"success": False, "error": str(e)}) should be
narrowed: replace it with specific exception handlers for the expected error
types (for example requests.RequestException, json.JSONDecodeError, KeyError, or
any Brevo-specific API exception your code uses) and handle each with
appropriate logging and the same error return; if you still include a generic
catch-all, first re-raise system-exiting exceptions by checking isinstance(e,
(KeyboardInterrupt, SystemExit)) before logging/returning to avoid swallowing
them.
🪄 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: d94356c6-1a75-496b-92d6-232c6027a539

📥 Commits

Reviewing files that changed from the base of the PR and between 3461981 and 6026821.

📒 Files selected for processing (3)
  • src/backend/core/api/viewsets/inbound/brevo.py
  • src/backend/core/tests/api/test_inbound_brevo.py
  • src/backend/core/urls.py

Comment on lines +145 to +152
class InboundBrevoViewSet(viewsets.GenericViewSet):
"""Handles incoming email messages from Brevo inbound parse webhooks."""

CHANNEL_TYPE = "brevo"
CHANNEL_DESCRIPTION = "Brevo inbound email parsing"

permission_classes = [IsAuthenticated]
authentication_classes = [BrevoAuthentication]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

IsAuthenticated permission incompatible with user=None from BrevoAuthentication.

BrevoAuthentication.authenticate() returns (None, auth_data) with user=None. DRF's IsAuthenticated permission checks request.user.is_authenticated, which will fail when user is None, causing all requests to return 403 Forbidden. This explains the pipeline failures where tests receive 403 instead of expected status codes.

Either return an anonymous user object that passes is_authenticated, or use AllowAny permissions with manual auth validation in the viewset.

🐛 Proposed fix using AllowAny with manual validation
-from rest_framework.permissions import IsAuthenticated
+from rest_framework.permissions import AllowAny

 class InboundBrevoViewSet(viewsets.GenericViewSet):
     """Handles incoming email messages from Brevo inbound parse webhooks."""

     CHANNEL_TYPE = "brevo"
     CHANNEL_DESCRIPTION = "Brevo inbound email parsing"

-    permission_classes = [IsAuthenticated]
+    permission_classes = [AllowAny]
     authentication_classes = [BrevoAuthentication]

Alternatively, create an anonymous user wrapper:

🔧 Alternative: Return authenticated anonymous user
+from django.contrib.auth.models import AnonymousUser
+
+class BrevoWebhookUser(AnonymousUser):
+    """Anonymous user that passes is_authenticated for webhook auth."""
+    `@property`
+    def is_authenticated(self):
+        return True

 class BrevoAuthentication(BaseAuthentication):
     # ...
     def _authenticate_by_channel_id(self, channel_id: str):
         try:
             channel = models.Channel.objects.get(id=channel_id, type="brevo")
         except (models.Channel.DoesNotExist, ValidationError) as e:
             raise AuthenticationFailed("Invalid channel_id") from e
-        return (None, {"channel": channel, "auth_type": "channel_id"})
+        return (BrevoWebhookUser(), {"channel": channel, "auth_type": "channel_id"})

     def _authenticate_by_signature(self, request, signature: str):
         # ...
-        return (None, {"auth_type": "hmac"})
+        return (BrevoWebhookUser(), {"auth_type": "hmac"})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/viewsets/inbound/brevo.py` around lines 145 - 152,
InboundBrevoViewSet is using IsAuthenticated while
BrevoAuthentication.authenticate() returns (None, auth_data), causing
request.user to be None and IsAuthenticated to reject requests; change
permission_classes to [AllowAny] and enforce BrevoAuthentication manually in the
viewset (e.g., override initial or a before-action method on InboundBrevoViewSet
to call BrevoAuthentication().authenticate(request) and raise PermissionDenied
when auth fails), so authentication logic lives in
BrevoAuthentication.authenticate and permission gating is handled explicitly in
the viewset.

Comment on lines +194 to +224
if failure_count > 0 and success_count == 0:
return Response(
{
"status": "error",
"detail": "Failed to process all messages",
"results": results,
},
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
)

if failure_count > 0:
return Response(
{
"status": "partial_success",
"processed": success_count,
"failed": failure_count,
"results": results,
},
status=status.HTTP_207_MULTI_STATUS,
)

logger.info(
"Successfully processed %d Brevo inbound messages",
success_count,
)
return Response(
{
"status": "ok",
"processed": success_count,
"results": results,
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid exposing internal error details in response results.

The results list returned in error/partial responses includes str(e) from caught exceptions (line 276), which can leak stack traces or internal implementation details to external callers. This was flagged by static analysis.

Sanitize error messages before including them in responses.

🛡️ Proposed fix to sanitize error messages
         except Exception as e:
             logger.exception("Error processing Brevo item: %s", e)
-            return {"success": False, "error": str(e)}
+            return {"success": False, "error": "Internal processing error"}
🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 196-200: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.


[warning] 206-211: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.


[warning] 220-224: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/viewsets/inbound/brevo.py` around lines 194 - 224, The
response `results` currently includes raw exception strings (str(e)) which may
leak internal details; update the exception handlers that append to the
`results` list (where you currently append str(e)) to instead append a
sanitized, generic error object or message (e.g., {"error": "internal processing
error"} or {"error": "failed to process message"}) and move the detailed
exception into server-side logs using logger.exception(...) or logger.error(...,
exc_info=True); keep `failure_count`/`success_count` logic intact and ensure
only the sanitized messages are returned in the error/partial_success Responses
while full exception details remain in the logs.

format="json",
)

assert response.status_code == status.HTTP_403_FORBIDDEN
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Incorrect expected status code for unauthenticated requests.

DRF's AuthenticationFailed exception returns HTTP 401 (Unauthorized) by default, not 403 (Forbidden). The test assertion should expect 401.

💚 Proposed fix
-        assert response.status_code == status.HTTP_403_FORBIDDEN
+        assert response.status_code == status.HTTP_401_UNAUTHORIZED
📝 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
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_401_UNAUTHORIZED
🧰 Tools
🪛 GitHub Actions: Lint and tests

[error] 315-315: pytest assertion failed in TestInboundBrevoWebhook.test_webhook_without_authentication: expected HTTP 403 but got 401 (Unauthorized) for POST /api/v1.0/inbound/brevo/webhook/.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/tests/api/test_inbound_brevo.py` at line 315, The test
currently asserts response.status_code == status.HTTP_403_FORBIDDEN for an
unauthenticated request; update the assertion to expect
status.HTTP_401_UNAUTHORIZED instead (i.e., assert response.status_code ==
status.HTTP_401_UNAUTHORIZED) so it matches DRF's AuthenticationFailed behavior,
using the same response and status symbols already in the test.

Comment on lines +407 to +413
"To": [
{
"Name": mailbox.address,
"Address": mailbox.address,
}
],
"Recipients": [mailbox.address],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the Mailbox model definition and its address-related fields/properties
ast-grep --pattern $'class Mailbox($_):
    $$$
'
rg -n "class Mailbox" -A 50 --type=py | head -80
rg -n "def address|@property" src/backend/core/models.py -A 3

Repository: suitenumerique/messages

Length of output: 21867


🏁 Script executed:

# Check the exact lines in the test file where mailbox.address is used
sed -n '400,420p' src/backend/core/tests/api/test_inbound_brevo.py
sed -n '460,470p' src/backend/core/tests/api/test_inbound_brevo.py

Repository: suitenumerique/messages

Length of output: 1540


🏁 Script executed:

# Search for all usages of mailbox.address
rg "mailbox\.address" --type=py

Repository: suitenumerique/messages

Length of output: 584


Replace mailbox.address with str(mailbox) in test mock data.

The Mailbox model does not have an address attribute. Use the __str__() method output instead, which returns the formatted email address (local_part@domain).

Affected locations
  • Line 407-413: "Name": mailbox.address and "Address": mailbox.address
  • Line 466-467: {"Address": mailbox.address} and "Recipients": [mailbox.address]
🧰 Tools
🪛 GitHub Actions: Lint and tests

[error] 409-410: pytest error in TestInboundBrevoE2E.test_webhook_message_e2e: AttributeError 'Mailbox' object has no attribute 'address' while building the Brevo payload.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/tests/api/test_inbound_brevo.py` around lines 407 - 413, The
test mock data references a non-existent Mailbox.address attribute; replace all
occurrences of mailbox.address with str(mailbox) so the mocked "Name", "Address"
and "Recipients" fields use the Mailbox.__str__() output (local_part@domain);
update the two occurrences in the test_inbound_brevo.py mock objects where
"Name": mailbox.address, "Address": mailbox.address and {"Address":
mailbox.address} / "Recipients": [mailbox.address] appear to use str(mailbox)
instead.

Comment on lines +453 to +456
existing_message = factories.MessageFactory(
thread__mailbox=mailbox,
mime_id="original-message-id@example.com",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find MessageFactory and ThreadFactory definitions to understand correct usage
rg -n "class MessageFactory|class ThreadFactory" -A 20 --type=py
# Check Thread model for mailbox relationship
rg -n "class Thread" -A 30 src/backend/core/models.py

Repository: suitenumerique/messages

Length of output: 5624


🏁 Script executed:

sed -n '440,470p' src/backend/core/tests/api/test_inbound_brevo.py

Repository: suitenumerique/messages

Length of output: 1271


🏁 Script executed:

rg -n "MessageFactory" src/backend/core/tests/api/test_inbound_brevo.py -B 2 -A 2 | head -50

Repository: suitenumerique/messages

Length of output: 242


🏁 Script executed:

rg -n "thread.*mailbox|mailbox.*thread" src/backend/core/tests/ --type=py | head -20

Repository: suitenumerique/messages

Length of output: 2338


🏁 Script executed:

sed -n '14,30p' src/backend/core/tests/tasks/test_task_send_message.py

Repository: suitenumerique/messages

Length of output: 625


🏁 Script executed:

sed -n '155,160p' src/backend/core/tests/services/test_throttle.py

Repository: suitenumerique/messages

Length of output: 312


Fix the thread-mailbox setup by creating a ThreadAccess relationship.

The thread__mailbox syntax is invalid because the Thread model has no direct mailbox field. The relationship is indirect through ThreadAccess. Create the thread separately and link it to the mailbox via ThreadAccessFactory:

Corrected code
thread = factories.ThreadFactory()
factories.ThreadAccessFactory(thread=thread, mailbox=mailbox)
existing_message = factories.MessageFactory(
    thread=thread,
    mime_id="original-message-id@example.com",
)
🧰 Tools
🪛 GitHub Actions: Lint and tests

[error] 453-454: pytest error in TestInboundBrevoE2E.test_webhook_with_reply: TypeError Thread() got unexpected keyword arguments: 'mailbox' when creating existing_message via factory.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/tests/api/test_inbound_brevo.py` around lines 453 - 456, The
test incorrectly uses thread__mailbox when creating existing_message; instead
create a Thread via ThreadFactory, link it to the mailbox using
ThreadAccessFactory(thread=thread, mailbox=mailbox), then create
existing_message with MessageFactory(thread=thread,
mime_id="original-message-id@example.com"). Update references in the test to use
the new thread variable and ThreadAccessFactory to ensure the Thread->Mailbox
relationship exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants