✨(client-bridge) add new IMAP/SMTP proxy for legacy clients (!)#583
Open
✨(client-bridge) add new IMAP/SMTP proxy for legacy clients (!)#583
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:
📝 WalkthroughWalkthroughAdds a Client Bridge service and integrates it across backend, client-bridge runtime, frontend, tests, CI, and dev tooling: new API endpoints and auth, per-channel encrypted settings and password rotation, IMAP/SMTP bridge runtime and backend client, DB migration, OpenAPI updates, many tests, Makefile/compose/CI changes, and a pylint plugin. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220, 240, 255, 0.5)
participant Client as Email Client
participant Bridge as Client-Bridge
participant API as Messages API
participant Backend as Messages Backend
end
Client->>Bridge: IMAP/SMTP connect + PLAIN auth (username,password)
Bridge->>API: POST /client-bridge/auth/ (username,password + X-Service-Auth)
API->>Backend: lookup mailbox & channel, verify password
Backend-->>API: generate JWT (channel_id, role, exp)
API-->>Bridge: 200 {token}
Bridge-->>Client: AUTH OK (session established)
Client->>Bridge: IMAP SELECT / FETCH
Bridge->>API: GET /threads/?folder=inbox (X-Channel-Token)
API->>Backend: validate JWT & role
Backend-->>API: return threads/messages
API-->>Bridge: thread list
Bridge-->>Client: IMAP response (EXISTS, FETCH, ...)
sequenceDiagram
rect rgba(220, 240, 255, 0.5)
participant Client as Email Client
participant Bridge as Client-Bridge
participant API as Messages API
participant Backend as Messages Backend
participant Queue as Task Queue
end
Client->>Bridge: SMTP MAIL FROM / RCPT TO / DATA (raw RFC822)
Bridge->>API: POST /client-bridge/submit/ (raw email, X-Channel-Token, X-Mail-From, X-Rcpt-To)
API->>Backend: validate JWT & role, parse/accept message
Backend->>Backend: create outbound message (is_draft)
Backend->>Backend: prepare_outbound_message (DKIM sign, store)
Backend->>Queue: send_message_task.delay(message.id)
Queue-->>Backend: ACK
Backend-->>API: 202 {message_id}
API-->>Bridge: 202 Accepted
Bridge-->>Client: 250 OK {message_id}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
src/client-bridge/README.md-11-16 (1)
11-16:⚠️ Potential issue | 🟡 MinorAdd language tags to the fenced code blocks.
Markdownlint is already flagging these fences. Label them as
text/plaintextandhaproxy(orcfg) so the docs stay CI-clean.📝 Suggested fix
-``` +```text ┌──────────────┐ IMAP/SMTP ┌───────────────────┐ HTTP ┌──────────────┐ │ Email client │ ◄────────────────────► │ Client Bridge │ ◄──────────────────► │ Messages │ │ (Thunderbird)│ │ (pymap/aiosmtpd) │ │ Backend │ └──────────────┘ └───────────────────┘ └──────────────┘-
+haproxy
frontend imap-tls
bind *:993 ssl crt /etc/ssl/certs/mail.pem
default_backend client-bridge-imap
@@
backend client-bridge-smtp
server bridge1 client-bridge:587Also applies to: 96-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/README.md` around lines 11 - 16, Update the fenced code blocks in src/client-bridge/README.md to include language labels so markdownlint passes: add ```text (or ```plaintext) to the ASCII diagram block and label the HAProxy config blocks with ```haproxy or ```cfg; specifically modify the block containing the box diagram and the blocks that include the haproxy stanzas such as the one starting with "frontend imap-tls" and the backend block containing "backend client-bridge-smtp" to use the appropriate language tag.src/backend/messages/settings.py-267-273 (1)
267-273:⚠️ Potential issue | 🟡 MinorRequire a positive client-bridge session TTL.
values.IntegerValueaccepts0and negatives, which can produce immediately expired or otherwise nonsensical session lifetimes.⏱️ Suggested fix
- CLIENTBRIDGE_SESSION_TIMEOUT = values.IntegerValue( + CLIENTBRIDGE_SESSION_TIMEOUT = values.PositiveIntegerValue( 3600, environ_name="CLIENTBRIDGE_SESSION_TIMEOUT", environ_prefix=None, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/messages/settings.py` around lines 267 - 273, CLIENTBRIDGE_SESSION_TIMEOUT may be zero or negative because values.IntegerValue doesn't enforce positivity; after the existing CLIENTBRIDGE_SESSION_TIMEOUT = values.IntegerValue(...) declaration add a validation check that reads the resolved value and raises django.core.exceptions.ImproperlyConfigured if CLIENTBRIDGE_SESSION_TIMEOUT < 1 (ensuring a minimum of 1 second), so callers and startup will fail fast with a clear error instead of producing an immediately expired session TTL.README.md-63-63 (1)
63-63:⚠️ Potential issue | 🟡 MinorFix typo: "compilant" → "compliant".
📝 Proposed fix
-* ✅ JMAP-inspired data model. JMAP-compilant endpoint [in progress](https://github.com/suitenumerique/messages/pull/479). +* ✅ JMAP-inspired data model. JMAP-compliant endpoint [in progress](https://github.com/suitenumerique/messages/pull/479).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 63, Update the README text fragment "JMAP-inspired data model. JMAP-compilant endpoint" by replacing the misspelled word "compilant" with "compliant" so the line reads "JMAP-inspired data model. JMAP-compliant endpoint" (locate the exact string in README.md to change).src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scss-255-262 (1)
255-262:⚠️ Potential issue | 🟡 MinorRemove quotes around single-word font name
Consolas.Stylelint flagged
"Consolas"as having unnecessary quotes. Single-word font family names that are not CSS keywords don't require quotes.🔧 Proposed fix
code { - font-family: "Fira Code", "Consolas", monospace; + font-family: "Fira Code", Consolas, monospace; font-size: 0.8rem; background: var(--c--contextuals--background--surface--primary); padding: 2px 6px; border-radius: 3px; border: 1px solid var(--c--contextuals--border--semantic--neutral--default); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scss` around lines 255 - 262, The font-family declaration in the code selector uses unnecessary quotes around the single-word font name "Consolas"; edit the font-family value in the `code` rule (font-family: "Fira Code", "Consolas", monospace;) to remove the quotes around Consolas so it becomes font-family: "Fira Code", Consolas, monospace;, preserving the existing order and other tokens.src/client-bridge/src/settings.py-21-21 (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd error handling for environment variable parsing.
The
int()casts on lines 21, 25, 27, and 29 will raiseValueErrorif the environment variable contains a non-numeric value, causing the service to crash on startup with an unclear error message.🛡️ Proposed fix with error handling
+def _env_int(name: str, default: int) -> int: + """Read an integer from an environment variable.""" + val = os.environ.get(name, "").strip() + if not val: + return default + try: + return int(val) + except ValueError: + raise ValueError(f"Environment variable {name} must be an integer, got: {val!r}") + ENABLE_IMAP = _env_bool("ENABLE_IMAP", default=True) IMAP_HOST = os.environ.get("IMAP_HOST", "0.0.0.0") -IMAP_PORT = int(os.environ.get("IMAP_PORT", "143")) +IMAP_PORT = _env_int("IMAP_PORT", 143) ENABLE_SMTP = _env_bool("ENABLE_SMTP", default=True) SMTP_HOST = os.environ.get("SMTP_HOST", "0.0.0.0") -SMTP_PORT = int(os.environ.get("SMTP_PORT", "587")) -SMTP_SESSION_TIMEOUT = int(os.environ.get("SMTP_SESSION_TIMEOUT", "300")) -SMTP_MAX_MESSAGES_PER_SESSION = int(os.environ.get("SMTP_MAX_MESSAGES_PER_SESSION", "50")) +SMTP_PORT = _env_int("SMTP_PORT", 587) +SMTP_SESSION_TIMEOUT = _env_int("SMTP_SESSION_TIMEOUT", 300) +SMTP_MAX_MESSAGES_PER_SESSION = _env_int("SMTP_MAX_MESSAGES_PER_SESSION", 50)Also applies to: 25-25, 27-27, 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/settings.py` at line 21, The environment variable parsing for IMAP_PORT (and the other integer constants defined similarly) uses int() directly and can raise ValueError for non-numeric values; update settings.py to parse these variables safely by wrapping each int() conversion for IMAP_PORT and the other port constants in a try/except ValueError (or a small helper function like parse_int_env), log or warn when parsing fails, and fall back to the existing default value instead of allowing the process to crash. Ensure you reference and update the exact symbols IMAP_PORT and the other port constant names present in the file so the behavior is consistent across all four locations.src/backend/core/api/viewsets/client_bridge.py-245-251 (1)
245-251:⚠️ Potential issue | 🟡 MinorEmail addresses logged in submission info log.
The log statement includes
mail_fromandrcpt_towhich are email addresses (PII). Consider removing these from the log or redacting them.🛡️ Proposed fix
logger.info( - "Message submitted via client-bridge: channel=%s, message=%s, from=%s, to=%s", + "Message submitted via client-bridge: channel=%s, message=%s", channel.id, message.id, - mail_from, - rcpt_to, )As per coding guidelines: "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/client_bridge.py` around lines 245 - 251, The info log in logger.info currently includes PII via mail_from and rcpt_to; update the log in the client-bridge submission flow (the logger.info call that references channel.id, message.id, mail_from, rcpt_to) to stop emitting raw email addresses—either remove mail_from/rcpt_to from the message entirely or replace them with redacted values (e.g., mask local-part or log only domain/hashed value) before passing to logger.info; ensure only non-PII fields like channel.id and message.id remain in the log and, if you add a redaction helper, call it where mail_from/rcpt_to are prepared for logging (keep the logger.info signature unchanged except for the sanitized values).src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx-267-274 (1)
267-274:⚠️ Potential issue | 🟡 MinorAdd error handling for clipboard operations.
navigator.clipboard.writeText()can fail in insecure contexts (non-HTTPS) or when clipboard permissions are denied. The current implementation will show a success toast even if the copy fails.🛡️ Proposed fix with error handling
const CopyButton = ({ value }: { value: string }) => { const { t } = useTranslation(); return ( <Button type="button" variant="tertiary" size="small" icon={<Icon name="content_copy" type={IconType.OUTLINED} size={IconSize.SMALL} />} - onClick={() => { - navigator.clipboard.writeText(value); - addToast( - <ToasterItem type="info"> - <span>{t("Copied to clipboard")}</span> - </ToasterItem> - ); + onClick={async () => { + try { + await navigator.clipboard.writeText(value); + addToast( + <ToasterItem type="info"> + <span>{t("Copied to clipboard")}</span> + </ToasterItem> + ); + } catch { + addToast( + <ToasterItem type="error"> + <span>{t("Failed to copy to clipboard")}</span> + </ToasterItem> + ); + } }} aria-label={t("Copy")} /> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 267 - 274, The onClick handler uses navigator.clipboard.writeText(value) but doesn't handle failures; update the handler (the onClick that calls navigator.clipboard.writeText, referencing value and addToast/ToasterItem) to first guard for navigator.clipboard, then perform the writeText call with async/await or a .then/.catch and show a success toast only on success, and on failure catch the error and call addToast with an error ToasterItem containing a helpful message and (optionally) the error message so users know the copy failed.src/backend/core/api/viewsets/client_bridge.py-46-50 (1)
46-50:⚠️ Potential issue | 🟡 MinorAvoid logging email addresses in authentication logs.
Lines 46 and 50 log the username (email address), which is PII. Consider logging only non-identifying information or using a hashed identifier for correlation.
🛡️ Proposed fix
except httpx.HTTPError: - logger.exception("SMTP auth HTTP error for %s", username) + logger.exception("SMTP auth HTTP error (see request context)") return AuthResult(success=False, handled=False) if resp.status_code != 200: - logger.warning("SMTP auth failed for %s", username) + logger.warning("SMTP auth failed (invalid credentials)") return AuthResult(success=False, handled=False)As per coding guidelines: "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/client_bridge.py` around lines 46 - 50, The auth view ClientBridgeAuthView is logging the username (an email / PII); replace those logs so they do not contain raw PII: remove direct logging of the username variable and instead log a non-identifying correlation value (e.g., a deterministic hash of username.lower()) or only generic state messages. Update any processLogger / logger calls in ClientBridgeAuthView that reference username to use the hashed identifier or omit the field entirely, and ensure comments reference the username variable and the class name so reviewers can find the change.src/client-bridge/src/submission.py-72-72 (1)
72-72:⚠️ Potential issue | 🟡 MinorAvoid logging user email addresses (PII).
Line 72 logs the username, which is an email address. Per coding guidelines, sensitive information including PII should not be logged. Consider logging only the channel_id or a hashed/truncated identifier.
🛡️ Proposed fix
- logger.info("SMTP auth success for %s (role=%s)", username, role) + logger.info("SMTP auth success for channel (role=%s)", role)Or if identification is needed:
- logger.info("SMTP auth success for %s (role=%s)", username, role) + logger.info("SMTP auth success (channel=%s, role=%s)", payload.get("channel_id"), role)As per coding guidelines: "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/client-bridge/src/submission.py` at line 72, The logger.info call currently logs the username (an email/PII) in logger.info("SMTP auth success for %s (role=%s)", username, role); change this to avoid PII by removing username from the log and instead log a non-sensitive identifier (e.g., channel_id) or a deterministic truncated/hashed identifier derived from username; update the logger.info invocation to include only the safe identifier and role, and if you add a hash routine create it near the authentication logic and reference that hashed_id when calling logger.info.src/client-bridge/tests/conftest.py-210-218 (1)
210-218:⚠️ Potential issue | 🟡 MinorFail fast if the mock API never becomes ready.
This loop falls through silently when the port never opens, so later tests fail with generic connection errors instead of pointing at startup. Mirror the IMAP/SMTP helpers and raise after the retry budget is exhausted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/conftest.py` around lines 210 - 218, The readiness loop that polls for the mock API port (the for _ in range(50): try: with socket.socket(...) s.connect(("localhost", MOCK_API_PORT)) ... except ...) should fail fast: after the retry budget is exhausted (i.e. the loop completes without breaking) raise a clear exception (e.g. RuntimeError) with a descriptive message including MOCK_API_PORT so tests immediately report the mock API startup failure; mirror the IMAP/SMTP helpers' behavior by adding the raise after the loop and ensure the exception message identifies the fixture and port.src/client-bridge/tests/conftest.py-68-167 (1)
68-167:⚠️ Potential issue | 🟡 MinorMake the mock API enforce the real authentication model.
These handlers currently succeed without checking
X-Service-AuthorX-Channel-Token, so the IMAP/SMTP integration tests can stay green even if the bridge stops sending required auth headers or token handling regresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/conftest.py` around lines 68 - 167, The mock API currently skips auth checks; update the handlers to enforce the real auth model by (1) validating the X-Service-Auth header (except for client_bridge_auth and health_check) and returning 401 if missing/incorrect, and (2) for endpoints that act on channel/mailbox resources (client_bridge_submit, list_threads, list_messages, get_eml, update_message, update_thread) require the x-channel-token header, decode it with pyjwt.decode(..., TEST_API_SECRET, algorithms=["HS256"]) and return 401 on decode errors or mismatched channel/mailbox IDs or insufficient role; implement these checks at the start of each handler so tests fail when the bridge stops sending required headers/tokens.src/backend/core/tests/api/test_client_bridge.py-880-891 (1)
880-891:⚠️ Potential issue | 🟡 MinorAssert the concrete post-auth status instead of
!= 403.These checks still pass on unrelated 401/404/500 failures, so the role suite can stay green even when the “allowed” path is broken. Pin each case to the exact status you expect once authorization succeeds.
As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."
Also applies to: 906-917, 933-941, 955-965, 1089-1095
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_client_bridge.py` around lines 880 - 891, The test uses a loose check "assert response.status_code != status.HTTP_403_FORBIDDEN" which masks other failures; update test_post_allowed (and the similar cases referenced) to assert the exact expected post-auth status (since the POST is expected to fail validation) by replacing the !=403 assertion with an equality check against the concrete validation response, e.g. assert response.status_code == status.HTTP_400_BAD_REQUEST (using status.HTTP_400_BAD_REQUEST) in test_post_allowed and the analogous assertions in the other test functions at the indicated ranges (906-917, 933-941, 955-965, 1089-1095) so each case verifies the expected outcome rather than just "not forbidden".
🧹 Nitpick comments (12)
src/backend/core/mda/inbound_create.py (1)
375-378: Complex ternary may benefit from restructuring for clarity.The nested ternary on lines 375-377 is correct but harder to read:
sent_at=parsed_email.get("date") or timezone.now() if not is_outbound else None,Consider restructuring for readability if this is touched again in the future.
♻️ Optional: clearer structure
+ sent_at=( + None if is_outbound + else (parsed_email.get("date") or timezone.now()) + ), - sent_at=parsed_email.get("date") or timezone.now() - if not is_outbound - else None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/mda/inbound_create.py` around lines 375 - 378, The inline conditional for sent_at is correct but hard to read; change the assignment to compute sent_at with a simple if/else block (use parsed_email.get("date") or timezone.now() when not is_outbound, otherwise set sent_at to None) rather than the nested inline ternary, and keep the is_draft=is_outbound comment and behavior intact; update the block around sent_at and is_draft in inbound_create.py (referencing sent_at, parsed_email.get, timezone.now, is_outbound, is_draft, and prepare_outbound_message) for clarity.src/client-bridge/Dockerfile (1)
66-70: Consider adding a non-root user for production.The container runs as root, which was flagged by static analysis (Trivy DS-0002). For production deployments, it's a security best practice to run as a non-root user.
🔒 Proposed fix to add non-root user
# ---- Base runtime image for production ---- FROM runtime-base AS runtime-prod +RUN useradd --create-home --uid 1000 appuser + COPY --from=base-with-deps /venv /venv COPY ./src /app/src + +RUN chown -R appuser:appuser /app +USER appuser🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/Dockerfile` around lines 66 - 70, The Dockerfile currently leaves the container running as root in the runtime-prod stage (stages: runtime-base, runtime-prod, base-with-deps); add a non-root user and switch to it in the runtime-prod stage: create a user/group (e.g., appuser), chown the application directories copied from /venv and /app (the COPY --from=base-with-deps /venv /venv and COPY ./src /app/src lines) to that user, and add a USER instruction to run the image as that non-root user; ensure the created user's home and permissions allow the existing entrypoint/commands to run.src/backend/core/models.py (1)
464-471: Consider addingdb_index=Truefor theuserfield.If channels will be queried by user (e.g., "get all channels created by this user"), adding an index will improve query performance. Per coding guidelines, database indexing should be considered for foreign keys used in filters.
♻️ Proposed fix
user = models.ForeignKey( "User", on_delete=models.CASCADE, null=True, blank=True, related_name="channels", help_text="User who created this channel (used for permissions and auditing)", + db_index=True, )As per coding guidelines: "Implement database indexing and query optimization (Model Meta indexes, constraints)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/models.py` around lines 464 - 471, The ForeignKey field user (models.ForeignKey(..., related_name="channels")) lacks a database index which will slow queries that filter by user; update the user field definition to include db_index=True on the models.ForeignKey declaration (keep null=True, blank=True, related_name="channels", help_text intact), then create and apply a Django migration to add the index so queries like "get all channels for a user" are optimized.src/client-bridge/tests/test_imap_auth.py (1)
3-10: Remove unusedloggerimport.The
loggeris defined but never used in this test module.Suggested fix
"""Tests for IMAP authentication using channel app-specific passwords.""" import imaplib -import logging import pytest from .conftest import IMAP_HOST, IMAP_PORT - -logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/test_imap_auth.py` around lines 3 - 10, Remove the unused logger variable declared at module scope in the test file (the identifier "logger") since it is defined but never referenced; locate the top-level assignment "logger = logging.getLogger(__name__)" in src/client-bridge/tests/test_imap_auth.py and delete that line and the unused import if left unused (remove the "logging" import if not used elsewhere) so the module has no unused symbols.src/backend/core/api/viewsets/channel.py (1)
115-139: Add 400 response to OpenAPI schema for consistency.The
rotate_passwordaction returns HTTP 400 for non-client-bridge channels (line 132), but this response is not documented in theextend_schemadecorator.Suggested fix
`@extend_schema`( request=None, responses={ + 400: OpenApiResponse(description="Channel type not supported"), }, )🤖 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 115 - 139, The OpenAPI schema for rotate_password (the extend_schema decorator on the rotate_password action) is missing the 400 response returned when instance.type != "client-bridge"; update the responses mapping passed to extend_schema to include a 400 OpenApiResponse (e.g. description "Bad request" or "Password rotation not available for this channel type") so the documented responses include 200, 400, 403, and 404 and reflect the actual behavior of the rotate_password view.src/client-bridge/tests/test_imap_messages.py (1)
3-6: Remove unusedloggerimport.The
loggeris defined but never used in this test module.Suggested fix
"""Tests for IMAP message operations using Python's imaplib.""" import email -import logging - -logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/test_imap_messages.py` around lines 3 - 6, Remove the unused logger by deleting the logging import and the logger = logging.getLogger(__name__) assignment in the test_imap_messages.py module; keep the import email line intact so only the unused logging-related symbols are removed.src/client-bridge/tests/test_smtp_submission.py (1)
76-90: Consider strengthening the assertion for multiple recipients.The test only verifies that the submission count increased, but doesn't confirm both recipients are present in the payload. This makes the test less precise.
Suggested improvement
initial_count = len(mock_api.submitted_messages) smtp_client.sendmail( "test@example.com", ["user1@example.com", "user2@example.com"], msg.as_string(), ) assert len(mock_api.submitted_messages) > initial_count + submitted = mock_api.submitted_messages[-1] + assert "user1@example.com" in submitted["rcpt_to"] + assert "user2@example.com" in submitted["rcpt_to"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/test_smtp_submission.py` around lines 76 - 90, The test_send_multiple_recipients test currently only checks that mock_api.submitted_messages length increased; update it to assert that the actual submitted payload contains both recipients: retrieve the last submission from mock_api.submitted_messages (e.g., mock_api.submitted_messages[-1]) and assert that both "user1@example.com" and "user2@example.com" appear in the submission's recipient/envelope fields (or in the parsed message headers like "To"), and optionally verify the subject or from fields to ensure the correct message was sent.src/client-bridge/tests/test_imap_folders.py (1)
3-6: Remove unusedloggerimport.The
loggeris defined but never used in this test module.Suggested fix
"""Tests for IMAP folder operations.""" -import logging import re -logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/test_imap_folders.py` around lines 3 - 6, Remove the unused logger: delete the "import logging" statement and the "logger = logging.getLogger(__name__)" line from the test_imap_folders.py module (they are unused), leaving the existing "import re" intact; alternatively, if logging is intended, replace usages accordingly, but the immediate fix is to remove the unused logger variable and its import.src/client-bridge/src/submission.py (1)
58-64: Clarification on JWT verification bypass.The
verify_signature=Falseoption is intentional here since the token was just received from the trusted auth endpoint over an internal HTTP call. The comment at line 58 documents this, but a more explicit note about the trust boundary would help future maintainers.📝 Enhanced documentation
- # Decode JWT without verification — we just need the payload fields + # Decode JWT without verification — the token was just received from our + # trusted auth endpoint over internal HTTP. We extract payload fields + # (channel_id, role, exp) to store in the session. try: payload = jwt.decode(token, options={"verify_signature": False})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/submission.py` around lines 58 - 64, The JWT is being decoded with jwt.decode(token, options={"verify_signature": False}) without verifying the signature; update the inline comment near this decode (around the jwt.decode call and the payload["token"] assignment) to explicitly document the trust boundary—state that the token originates from a trusted internal auth endpoint over an authenticated/secure channel, why signature verification is intentionally skipped here, and add a note describing when to remove or change this behavior (e.g., if the token source or transport changes) so future maintainers understand the rationale and risks.src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx (2)
98-105: Type safety gap withanycast for response data.The eslint-disable comments and
as anycasts suggest the generated API types don't include thepasswordfield in the response. Consider extending the generated types or creating a typed response interface to avoid runtime surprises.♻️ Suggested type-safe approach
// Define the expected response shape type ChannelCreateResponse = Channel & { password?: string }; // Then use it without any cast: const response = await createMutation.mutateAsync({...}); if (response.status === 201) { const password = (response.data as ChannelCreateResponse).password; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 98 - 105, The code is using an any cast to read password from the API response; define a typed response interface (e.g., ChannelCreateResponse = Channel & { password?: string }) and use that type instead of casting to any when handling the result of the create mutation (the response returned by createMutation.mutateAsync). Update the handler around the response.status === 201 block to read password from (response.data as ChannelCreateResponse).password, remove the eslint-disable and any casts, and keep calling setGeneratedPassword(password) and onSuccess(response.data) as before so the flow and side-effects remain unchanged.
29-32: Use Zod 4 error parameter syntax instead of deprecated message parameter.Zod 4.3.6 is in use, and the
messageparameter has been deprecated in favor oferrorfor validation error customization. Line 30 should be updated to align with the current Zod 4 API:♻️ Suggested Zod 4 syntax
const formSchema = (t: (key: string) => string) => z.object({ - name: z.string().min(1, { message: t("Name is required.") }), + name: z.string().min(1, { error: t("Name is required.") }), role: z.enum(["reader", "editor", "sender", "sender_only"]), });Note: Similar deprecated patterns exist in
widget-integration-form.tsxin the same directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 29 - 32, The zod validation uses the deprecated { message: ... } option; update the formSchema's name validator to use the Zod 4 error option (e.g., replace the min(1, { message: t("Name is required.") }) call with min(1, { error: t("Name is required.") })) so the schema function formSchema (t: (key: string) => string) conforms to Zod 4 syntax; also search for the same pattern in widget-integration-form.tsx and update those validators from message to error as well.src/backend/core/api/serializers.py (1)
1519-1527: Side effect:pop()mutates thesettings_datadictionary.The dictionary comprehension at lines 1519-1523 uses
pop()which mutatessettings_datain place. While this works correctly here, it could cause subtle bugs ifvalidated_data["settings"]is used elsewhere after this call. Consider using a non-mutating approach.♻️ Non-mutating alternative
- extracted = { - key: settings_data.pop(key) - for key in keys_to_encrypt - if key in settings_data - } + extracted = { + key: settings_data[key] + for key in keys_to_encrypt + if key in settings_data + } + for key in extracted: + del settings_data[key]🤖 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 1519 - 1527, The current dict comprehension uses settings_data.pop(...) which mutates settings_data; replace it with a non-mutating extraction such as building extracted = {key: settings_data[key] for key in keys_to_encrypt if key in settings_data} so settings_data remains unchanged, then merge as before into validated_data["encrypted_settings"] (using existing = (self.instance.encrypted_settings or {}) if self.instance else {} and validated_data["encrypted_settings"] = {**existing, **extracted}).
🤖 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 2849-2891: The 200 response for operationId
mailboxes_channels_rotate_password_create currently has no response schema;
update the OpenAPI entry for the POST path
"/api/v1.0/mailboxes/{mailbox_id}/channels/{id}/rotate-password/" so the 200
response includes a JSON response body (e.g. content: application/json) with an
explicit schema describing the rotated secret (at minimum a property like
"password" of type "string" and required), so generated clients and docs can see
and surface the one‑time password.
- Around line 127-139: The OpenAPI entry for operationId
client_bridge_auth_create (path "/api/v1.0/client-bridge/auth/") is incomplete:
add a requestBody with application/json schema containing required properties
"username" and "password"; declare the required header X-Service-Auth either as
a header parameter or, better, as a security scheme (e.g., apiKey in header
named X-Service-Auth) and reference it in the operation's security array; and
replace the "200" response description with a JSON response schema that includes
"token", "channel_id", "mailbox_id", "mailbox_email", "role", and "expires_at"
(with appropriate types/formats). Ensure content types are application/json and
mark required fields so generated clients/docs can call and validate the
endpoint.
- Around line 141-153: The OpenAPI operation "/api/v1.0/client-bridge/submit/"
(operationId: client_bridge_submit_create) lacks the documented wire format,
required headers, security reference, and response schema; update that operation
to include a requestBody with content type "message/rfc822" (raw string/binary
as appropriate), add three required header parameters "X-Channel-Token",
"X-Mail-From", and "X-Rcpt-To" (with appropriate descriptions and string types),
reference the existing ClientBridgeChannelAuthentication security scheme in the
operation's "security" array, and replace the empty 200 response description
with a JSON schema for the response object containing "message_id" and "status"
fields to match the implementation.
In `@src/backend/core/mda/outbound.py`:
- Around line 89-92: The client-bridge path returns early when raw_mime is
present and therefore never sets message.sender_user, causing _sign_and_store to
persist a missing/stale sender because its update_fields includes sender_user;
before returning from the raw_mime branch, set message.sender_user = user (the
authenticated user variable available in the scope) so that
_sign_and_store(mailbox_sender, message, raw_mime) saves the correct sender_user
value.
In `@src/backend/messages/settings.py`:
- Around line 261-266: Remove the checked-in fallback for
CLIENTBRIDGE_API_SECRET and enforce it at runtime: change the
CLIENTBRIDGE_API_SECRET declaration so it does not supply a default (so
empty/unset if not provided) and add a guard in Base.post_setup() that raises a
ValueError when cls.FEATURE_CLIENTBRIDGE is true but cls.CLIENTBRIDGE_API_SECRET
is empty; also update dev env files so the backend and client-bridge use the
same secret for local development (keep references to CLIENTBRIDGE_API_SECRET
and FEATURE_CLIENTBRIDGE to locate where to change).
In `@src/client-bridge/src/api/client.py`:
- Around line 23-33: The client currently sets a global "X-Service-Auth" header
in Client.__init__ which is reused by with_token() because it returns the same
httpx.AsyncClient, causing channel-scoped methods (list_threads, list_messages,
get_message_eml, update_message_flags, update_thread_flags) to send the bridge
secret; change with_token() to ensure per-request or per-session headers do not
include the global service secret by either creating a new AsyncClient copy
without the "X-Service-Auth" header or by using request-scoped headers (e.g.,
pass user token in the request call) so that the global headers set in __init__
(headers / X-Service-Auth) are not applied to channel-scoped calls and auth
precedence is preserved.
- Around line 52-57: The JWT claims are being read from unverified decodes in
client.py (the jwt.decode call that sets clone._token_exp and the similar decode
at lines 86-90), which lets tampered tokens influence local
expiry/authorization; change both decode calls to verify the signature using the
bridge's shared secret and explicit algorithms (pass the secret and algorithms
to jwt.decode) and only read exp, mailbox_id, and role from the payload when
verification succeeds; if verification fails, clear those fields (set to None)
or treat the token as invalid rather than trusting unverified claims. Ensure you
reference the same verification logic for both places where jwt.decode is used
so behavior is consistent.
In `@src/client-bridge/src/backend.py`:
- Around line 138-141: The log statements around IMAP authentication expose PII
via the authcid value; update the two logging calls that reference authcid (the
logger.warning("IMAP auth rejected for %s: sender_only has no read access",
authcid) and logger.info("Authenticated channel %s (role=%s)", authcid, role))
to log a non-PII identifier instead (e.g., a channel id, redacted form, or a
stable hash of authcid) while keeping the role and context; locate these within
the authentication routine handling InvalidAuth and replace authcid in both
logger.warning and logger.info with the chosen safe identifier provider (e.g.,
channel.id or redacted_hash(authcid)).
- Around line 75-76: The start method currently does nothing and never registers
the shared MessagesAPIClient/its underlying httpx.AsyncClient with the
AsyncExitStack, so the HTTP connections are not closed on shutdown; update
start(self, stack: AsyncExitStack) to create or access the shared
MessagesAPIClient/httpx.AsyncClient produced by init() and register its
close/async-close callback with the provided AsyncExitStack (e.g.,
stack.push_async_callback or stack.enter_async_context) so the client is
properly closed during teardown, referencing the MessagesAPIClient instance and
the start method to locate where to wire the exit stack registration.
- Around line 149-154: The authorize method currently rejects logins when
authzid is empty because it always compares authenticated.name to authzid;
change authorize(…) so it first ensures authenticated is an Identity (check
isinstance(authenticated, Identity) and raise InvalidAuth() if not) and then
only perform the name comparison if authzid is non-empty (e.g., if authzid: if
authenticated.name != authzid: raise InvalidAuth("Authorization as a different
user is not supported.")). This preserves the original error message for
mismatched names while treating an empty authzid as “self”.
In `@src/client-bridge/src/mailbox.py`:
- Around line 262-309: copy, move (and similarly update and delete) currently
only mutate in-memory state and never persist changes to the Messages API, so
operations are lost on reload; fix by invoking the appropriate Messages API
calls inside these functions (e.g., call the API to create a copied/moved
message in destination and to delete/remove or update flags on the source) after
acquiring the same locks (messages_lock / destination.messages_lock) and before
returning, update local state only after a successful API response, and handle
API errors (retry or propagate error and avoid committing local changes) so that
functions like copy, move, update, and delete persist to the backend and keep
_max_uid, _messages, and _updated consistent with the remote state.
- Around line 240-260: The append method currently fabricates a Message
(api_message_id="local-{uid}") only in memory (_messages) and ACKs success,
causing clients to lose messages on reconnect; modify append in Mailbox.append
to ensure the message is persisted to the authoritative backend before returning
(or fail the operation): call the mailbox/API persistence function used
elsewhere (or implement/save via the same persistence path that creates
Message/email_id/thread_id), only add to self._messages and return the Message
after the backend confirms persistence, and if persistence is unsupported, raise
an error/IMAP-specific failure instead of returning a synthetic local-* message;
refer to append, Message, _messages, api_message_id, and _load_messages to
locate and reuse the authoritative persistence logic.
- Around line 165-168: The code calls list_threads() once (in mailbox.py,
variables threads_data/threads) which returns paginated results so folders with
>1 page get truncated; modify the logic that prepares the thread list (where
threads_data = await self._api_client.list_threads(...)) to page through all
results: repeatedly call list_threads (or use the API client's pagination
helper) until no more pages, accumulate all thread items into threads (instead
of relying on a single page), and only then proceed to mark the folder ready.
---
Minor comments:
In `@README.md`:
- Line 63: Update the README text fragment "JMAP-inspired data model.
JMAP-compilant endpoint" by replacing the misspelled word "compilant" with
"compliant" so the line reads "JMAP-inspired data model. JMAP-compliant
endpoint" (locate the exact string in README.md to change).
In `@src/backend/core/api/viewsets/client_bridge.py`:
- Around line 245-251: The info log in logger.info currently includes PII via
mail_from and rcpt_to; update the log in the client-bridge submission flow (the
logger.info call that references channel.id, message.id, mail_from, rcpt_to) to
stop emitting raw email addresses—either remove mail_from/rcpt_to from the
message entirely or replace them with redacted values (e.g., mask local-part or
log only domain/hashed value) before passing to logger.info; ensure only non-PII
fields like channel.id and message.id remain in the log and, if you add a
redaction helper, call it where mail_from/rcpt_to are prepared for logging (keep
the logger.info signature unchanged except for the sanitized values).
- Around line 46-50: The auth view ClientBridgeAuthView is logging the username
(an email / PII); replace those logs so they do not contain raw PII: remove
direct logging of the username variable and instead log a non-identifying
correlation value (e.g., a deterministic hash of username.lower()) or only
generic state messages. Update any processLogger / logger calls in
ClientBridgeAuthView that reference username to use the hashed identifier or
omit the field entirely, and ensure comments reference the username variable and
the class name so reviewers can find the change.
In `@src/backend/core/tests/api/test_client_bridge.py`:
- Around line 880-891: The test uses a loose check "assert response.status_code
!= status.HTTP_403_FORBIDDEN" which masks other failures; update
test_post_allowed (and the similar cases referenced) to assert the exact
expected post-auth status (since the POST is expected to fail validation) by
replacing the !=403 assertion with an equality check against the concrete
validation response, e.g. assert response.status_code ==
status.HTTP_400_BAD_REQUEST (using status.HTTP_400_BAD_REQUEST) in
test_post_allowed and the analogous assertions in the other test functions at
the indicated ranges (906-917, 933-941, 955-965, 1089-1095) so each case
verifies the expected outcome rather than just "not forbidden".
In `@src/backend/messages/settings.py`:
- Around line 267-273: CLIENTBRIDGE_SESSION_TIMEOUT may be zero or negative
because values.IntegerValue doesn't enforce positivity; after the existing
CLIENTBRIDGE_SESSION_TIMEOUT = values.IntegerValue(...) declaration add a
validation check that reads the resolved value and raises
django.core.exceptions.ImproperlyConfigured if CLIENTBRIDGE_SESSION_TIMEOUT < 1
(ensuring a minimum of 1 second), so callers and startup will fail fast with a
clear error instead of producing an immediately expired session TTL.
In `@src/client-bridge/README.md`:
- Around line 11-16: Update the fenced code blocks in
src/client-bridge/README.md to include language labels so markdownlint passes:
add ```text (or ```plaintext) to the ASCII diagram block and label the HAProxy
config blocks with ```haproxy or ```cfg; specifically modify the block
containing the box diagram and the blocks that include the haproxy stanzas such
as the one starting with "frontend imap-tls" and the backend block containing
"backend client-bridge-smtp" to use the appropriate language tag.
In `@src/client-bridge/src/settings.py`:
- Line 21: The environment variable parsing for IMAP_PORT (and the other integer
constants defined similarly) uses int() directly and can raise ValueError for
non-numeric values; update settings.py to parse these variables safely by
wrapping each int() conversion for IMAP_PORT and the other port constants in a
try/except ValueError (or a small helper function like parse_int_env), log or
warn when parsing fails, and fall back to the existing default value instead of
allowing the process to crash. Ensure you reference and update the exact symbols
IMAP_PORT and the other port constant names present in the file so the behavior
is consistent across all four locations.
In `@src/client-bridge/src/submission.py`:
- Line 72: The logger.info call currently logs the username (an email/PII) in
logger.info("SMTP auth success for %s (role=%s)", username, role); change this
to avoid PII by removing username from the log and instead log a non-sensitive
identifier (e.g., channel_id) or a deterministic truncated/hashed identifier
derived from username; update the logger.info invocation to include only the
safe identifier and role, and if you add a hash routine create it near the
authentication logic and reference that hashed_id when calling logger.info.
In `@src/client-bridge/tests/conftest.py`:
- Around line 210-218: The readiness loop that polls for the mock API port (the
for _ in range(50): try: with socket.socket(...) s.connect(("localhost",
MOCK_API_PORT)) ... except ...) should fail fast: after the retry budget is
exhausted (i.e. the loop completes without breaking) raise a clear exception
(e.g. RuntimeError) with a descriptive message including MOCK_API_PORT so tests
immediately report the mock API startup failure; mirror the IMAP/SMTP helpers'
behavior by adding the raise after the loop and ensure the exception message
identifies the fixture and port.
- Around line 68-167: The mock API currently skips auth checks; update the
handlers to enforce the real auth model by (1) validating the X-Service-Auth
header (except for client_bridge_auth and health_check) and returning 401 if
missing/incorrect, and (2) for endpoints that act on channel/mailbox resources
(client_bridge_submit, list_threads, list_messages, get_eml, update_message,
update_thread) require the x-channel-token header, decode it with
pyjwt.decode(..., TEST_API_SECRET, algorithms=["HS256"]) and return 401 on
decode errors or mismatched channel/mailbox IDs or insufficient role; implement
these checks at the start of each handler so tests fail when the bridge stops
sending required headers/tokens.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scss`:
- Around line 255-262: The font-family declaration in the code selector uses
unnecessary quotes around the single-word font name "Consolas"; edit the
font-family value in the `code` rule (font-family: "Fira Code", "Consolas",
monospace;) to remove the quotes around Consolas so it becomes font-family:
"Fira Code", Consolas, monospace;, preserving the existing order and other
tokens.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 267-274: The onClick handler uses
navigator.clipboard.writeText(value) but doesn't handle failures; update the
handler (the onClick that calls navigator.clipboard.writeText, referencing value
and addToast/ToasterItem) to first guard for navigator.clipboard, then perform
the writeText call with async/await or a .then/.catch and show a success toast
only on success, and on failure catch the error and call addToast with an error
ToasterItem containing a helpful message and (optionally) the error message so
users know the copy failed.
---
Nitpick comments:
In `@src/backend/core/api/serializers.py`:
- Around line 1519-1527: The current dict comprehension uses
settings_data.pop(...) which mutates settings_data; replace it with a
non-mutating extraction such as building extracted = {key: settings_data[key]
for key in keys_to_encrypt if key in settings_data} so settings_data remains
unchanged, then merge as before into validated_data["encrypted_settings"] (using
existing = (self.instance.encrypted_settings or {}) if self.instance else {} and
validated_data["encrypted_settings"] = {**existing, **extracted}).
In `@src/backend/core/api/viewsets/channel.py`:
- Around line 115-139: The OpenAPI schema for rotate_password (the extend_schema
decorator on the rotate_password action) is missing the 400 response returned
when instance.type != "client-bridge"; update the responses mapping passed to
extend_schema to include a 400 OpenApiResponse (e.g. description "Bad request"
or "Password rotation not available for this channel type") so the documented
responses include 200, 400, 403, and 404 and reflect the actual behavior of the
rotate_password view.
In `@src/backend/core/mda/inbound_create.py`:
- Around line 375-378: The inline conditional for sent_at is correct but hard to
read; change the assignment to compute sent_at with a simple if/else block (use
parsed_email.get("date") or timezone.now() when not is_outbound, otherwise set
sent_at to None) rather than the nested inline ternary, and keep the
is_draft=is_outbound comment and behavior intact; update the block around
sent_at and is_draft in inbound_create.py (referencing sent_at,
parsed_email.get, timezone.now, is_outbound, is_draft, and
prepare_outbound_message) for clarity.
In `@src/backend/core/models.py`:
- Around line 464-471: The ForeignKey field user (models.ForeignKey(...,
related_name="channels")) lacks a database index which will slow queries that
filter by user; update the user field definition to include db_index=True on the
models.ForeignKey declaration (keep null=True, blank=True,
related_name="channels", help_text intact), then create and apply a Django
migration to add the index so queries like "get all channels for a user" are
optimized.
In `@src/client-bridge/Dockerfile`:
- Around line 66-70: The Dockerfile currently leaves the container running as
root in the runtime-prod stage (stages: runtime-base, runtime-prod,
base-with-deps); add a non-root user and switch to it in the runtime-prod stage:
create a user/group (e.g., appuser), chown the application directories copied
from /venv and /app (the COPY --from=base-with-deps /venv /venv and COPY ./src
/app/src lines) to that user, and add a USER instruction to run the image as
that non-root user; ensure the created user's home and permissions allow the
existing entrypoint/commands to run.
In `@src/client-bridge/src/submission.py`:
- Around line 58-64: The JWT is being decoded with jwt.decode(token,
options={"verify_signature": False}) without verifying the signature; update the
inline comment near this decode (around the jwt.decode call and the
payload["token"] assignment) to explicitly document the trust boundary—state
that the token originates from a trusted internal auth endpoint over an
authenticated/secure channel, why signature verification is intentionally
skipped here, and add a note describing when to remove or change this behavior
(e.g., if the token source or transport changes) so future maintainers
understand the rationale and risks.
In `@src/client-bridge/tests/test_imap_auth.py`:
- Around line 3-10: Remove the unused logger variable declared at module scope
in the test file (the identifier "logger") since it is defined but never
referenced; locate the top-level assignment "logger =
logging.getLogger(__name__)" in src/client-bridge/tests/test_imap_auth.py and
delete that line and the unused import if left unused (remove the "logging"
import if not used elsewhere) so the module has no unused symbols.
In `@src/client-bridge/tests/test_imap_folders.py`:
- Around line 3-6: Remove the unused logger: delete the "import logging"
statement and the "logger = logging.getLogger(__name__)" line from the
test_imap_folders.py module (they are unused), leaving the existing "import re"
intact; alternatively, if logging is intended, replace usages accordingly, but
the immediate fix is to remove the unused logger variable and its import.
In `@src/client-bridge/tests/test_imap_messages.py`:
- Around line 3-6: Remove the unused logger by deleting the logging import and
the logger = logging.getLogger(__name__) assignment in the test_imap_messages.py
module; keep the import email line intact so only the unused logging-related
symbols are removed.
In `@src/client-bridge/tests/test_smtp_submission.py`:
- Around line 76-90: The test_send_multiple_recipients test currently only
checks that mock_api.submitted_messages length increased; update it to assert
that the actual submitted payload contains both recipients: retrieve the last
submission from mock_api.submitted_messages (e.g.,
mock_api.submitted_messages[-1]) and assert that both "user1@example.com" and
"user2@example.com" appear in the submission's recipient/envelope fields (or in
the parsed message headers like "To"), and optionally verify the subject or from
fields to ensure the correct message was sent.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 98-105: The code is using an any cast to read password from the
API response; define a typed response interface (e.g., ChannelCreateResponse =
Channel & { password?: string }) and use that type instead of casting to any
when handling the result of the create mutation (the response returned by
createMutation.mutateAsync). Update the handler around the response.status ===
201 block to read password from (response.data as
ChannelCreateResponse).password, remove the eslint-disable and any casts, and
keep calling setGeneratedPassword(password) and onSuccess(response.data) as
before so the flow and side-effects remain unchanged.
- Around line 29-32: The zod validation uses the deprecated { message: ... }
option; update the formSchema's name validator to use the Zod 4 error option
(e.g., replace the min(1, { message: t("Name is required.") }) call with min(1,
{ error: t("Name is required.") })) so the schema function formSchema (t: (key:
string) => string) conforms to Zod 4 syntax; also search for the same pattern in
widget-integration-form.tsx and update those validators from message to error as
well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b03e905-06a9-41fb-8f05-173a214f83eb
⛔ Files ignored due to path filters (4)
src/client-bridge/uv.lockis excluded by!**/*.locksrc/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/client-bridge/client-bridge.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**
📒 Files selected for processing (53)
MakefileREADME.mdcompose.yamlenv.d/development/backend.defaultsenv.d/development/client-bridge.defaultssrc/backend/.pylintrcsrc/backend/README.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/client_bridge.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/api/viewsets/send.pysrc/backend/core/authentication/client_bridge.pysrc/backend/core/enums.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0020_channel_encrypted_settings_and_user.pysrc/backend/core/models.pysrc/backend/core/services/search/search.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_client_bridge.pysrc/backend/core/urls.pysrc/backend/messages/settings.pysrc/backend/pylint_custom.pysrc/client-bridge/Dockerfilesrc/client-bridge/README.mdsrc/client-bridge/entrypoint.shsrc/client-bridge/pyproject.tomlsrc/client-bridge/src/__init__.pysrc/client-bridge/src/api/__init__.pysrc/client-bridge/src/api/client.pysrc/client-bridge/src/backend.pysrc/client-bridge/src/mailbox.pysrc/client-bridge/src/server.pysrc/client-bridge/src/session.pysrc/client-bridge/src/settings.pysrc/client-bridge/src/submission.pysrc/client-bridge/tests/__init__.pysrc/client-bridge/tests/conftest.pysrc/client-bridge/tests/test_imap_auth.pysrc/client-bridge/tests/test_imap_folders.pysrc/client-bridge/tests/test_imap_messages.pysrc/client-bridge/tests/test_imap_operations.pysrc/client-bridge/tests/test_session_expiry.pysrc/client-bridge/tests/test_smtp_submission.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scsssrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsx
💤 Files with no reviewable changes (1)
- src/backend/README.md
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/mda/inbound_create.py (1)
274-299:⚠️ Potential issue | 🔴 CriticalBind client-bridge outbound submissions to the authenticated sender.
On the outbound path,
sender_contactcomes from the rawFromheader andis_senderis forced toTrue, but nothing verifies that address belongs tomailboxor one of its allowed identities. A valid bridge credential can therefore submit mail as an arbitrary sender address.🛡️ Suggested fix
if not sender_email: logger.warning( "Inbound message for %s missing 'From' email, using fallback.", recipient_email, ) sender_email = f"unknown-sender@{mailbox.domain.name}" # Use recipient's domain sender_name = sender_name or "Unknown Sender" + + if is_outbound: + allowed_senders = {str(mailbox)} + if sender_email not in allowed_senders: + raise ValidationError("From address does not match the authenticated mailbox")Also applies to: 361-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/mda/inbound_create.py` around lines 274 - 299, Verify that the parsed sender_email actually belongs to the recipient mailbox before treating it as the authenticated sender: add a check in the block around models.Contact.objects.get_or_create that confirms sender_email is one of mailbox's authorized identities (e.g., mailbox.identities/emails, mailbox.allowed_senders, or mailbox.users' emails); if it is not authorized, log a warning and do not mark or use the resulting sender_contact as the authenticated bridge sender (i.e., do not set is_sender=True or bind bridge credentials to it)—instead reject the outbound submission or create the contact only as an external/non-sender entry; apply the same validation to the corresponding outbound handling code referenced around lines 361-385.src/backend/core/api/viewsets/channel.py (1)
57-81:⚠️ Potential issue | 🟡 MinorExpose the one-time password in the documented 201 schema.
create()now returnspasswordforclient-bridgechannels, but the declared 201 response is stillChannelSerializer. Schema-driven clients and generated types will miss the only copy of that secret.🤖 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 57 - 81, The 201 OpenApi response still references ChannelSerializer but create() may add a one-time "password" field (from instance._generated_password), so update the decorator on create to declare a response schema that includes that password; e.g. create a small response serializer (or a modified OpenApiResponse) that wraps serializers.ChannelSerializer plus an optional "password" field and replace the current 201 OpenApiResponse with it so generated clients see the one-time secret; adjust the decorator on the create method accordingly (referencing create, serializers.ChannelSerializer, instance._generated_password, and the OpenApiResponse for 201).
♻️ Duplicate comments (1)
src/client-bridge/src/backend.py (1)
140-143:⚠️ Potential issue | 🟠 MajorDon't log mailbox identifiers during auth.
authcidis the mailbox email, so both log lines emit PII on normal and rejected login attempts. Log a channel ID or a redacted/hashed identifier instead.As per coding guidelines: "Do not log sensitive information (tokens, passwords, financial/health data, PII)".
🛡️ Proposed fix to avoid logging PII
if role == "sender_only": - logger.warning("IMAP auth rejected for %s: sender_only has no read access", authcid) + logger.warning("IMAP auth rejected: sender_only has no read access") raise InvalidAuth() - logger.info("Authenticated channel %s (role=%s)", authcid, role) + logger.info("Authenticated channel (role=%s)", role)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/backend.py` around lines 140 - 143, The current logs emit PII by including authcid; update both logging calls around the auth check (the logger.warning("IMAP auth rejected for %s: sender_only has no read access", authcid) and logger.info("Authenticated channel %s (role=%s)", authcid, role)) to avoid logging the raw mailbox identifier — instead log a non-PII channel identifier (e.g., an existing channel_id variable) or a deterministic redacted/hashed token derived from authcid (e.g., a short sha256/hexdigest or masked string) so that InvalidAuth behavior remains unchanged but no PII is written to logs.
🧹 Nitpick comments (4)
src/frontend/public/locales/common/en-US.json (1)
349-349: Minor: JSON keys are not in alphabetical order.A few new keys break the alphabetical ordering:
- Line 349:
"My email client"should come after"Monthly"(Mo... < My...)- Lines 411-413:
"Role","Rotate password","Rotating..."should come after"Reply all"(Rep... < Rol...)If the project enforces sorted keys (e.g., via a linter), these would need correction. Otherwise, feel free to defer.
Suggested key placement
- "My email client": "My email client", "Monthly": "Monthly", + "My email client": "My email client","Reply all": "Reply all", + "Role": "Role", + "Rotate password": "Rotate password", + "Rotating...": "Rotating...", "Report {{count}} threads as spam_one": "Report {{count}} thread as spam", - "Role": "Role", - "Rotate password": "Rotate password", - "Rotating...": "Rotating...",Also applies to: 411-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/public/locales/common/en-US.json` at line 349, The JSON locale keys are out of alphabetical order; move the key "My email client" so it appears after the "Monthly" key, and move "Role", "Rotate password", and "Rotating..." so they appear after the "Reply all" key to restore alphabetical ordering; update the entries in the same file (preserve their values and surrounding commas) and run the linter/formatter to verify ordering.src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx (1)
31-34: Use Zod 4'serroroption here.
{ message: ... }is the deprecated validation style in Zod 4. Switching this schema to{ error: ... }keeps it aligned with the rest of the frontend validation code.♻️ Proposed fix
const formSchema = (t: (key: string) => string) => z.object({ - name: z.string().min(1, { message: t("Name is required.") }), + name: z.string().min(1, { error: t("Name is required.") }), role: z.enum(["reader", "editor", "sender", "sender_only"]), });Based on learnings, "In Zod 4, replace the deprecated message option with the error parameter for error customization."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 31 - 34, Replace the deprecated Zod 4 validation option for the name field in formSchema: change z.string().min(1, { message: t("Name is required.") }) to use the new error option (z.string().min(1, { error: t("Name is required.") })); keep the role enum unchanged. Locate the formSchema function and update the min option object for the name validator to use error instead of message so it aligns with the rest of the frontend validation code.src/client-bridge/tests/conftest.py (1)
548-594: Consider silencing exceptions in fixture teardown.The bare
except Exception: passpatterns are appropriate here to ensure cleanup doesn't fail tests, but logging the exception would help debugging test infrastructure issues.♻️ Optional improvement for debugging
`@pytest.fixture` def imap_client(imap_server, test_channel): """An authenticated IMAP client connected to the test server.""" client = imaplib.IMAP4(IMAP_HOST, IMAP_PORT) client.login(test_channel["mailbox_email"], test_channel["password"]) yield client try: client.logout() - except Exception: - pass + except Exception as e: + logger.debug("IMAP client logout failed: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/conftest.py` around lines 548 - 594, Update the fixture teardowns (imap_client, imap_connection, smtp_client, smtp_connection) to log any exceptions instead of silently passing: add a module-level logger (logging.getLogger(__name__)) and in each except Exception block catch the exception as e and call logger.exception or logger.warning with context (e.g., "imap_client.logout failed") referencing the failing method names logout() and quit() so teardown failures are recorded for debugging.src/client-bridge/src/api/client.py (1)
100-104: Handle JWT verification failure gracefully.If
self._api_secretis empty (e.g., misconfiguration),jwt.decode()will raiseInvalidSignatureErrorand the exception will propagate, potentially causing a 500 error instead of a clean auth failure.♻️ Suggested improvement
token = resp.json()["token"] # Verify the JWT signature using the shared secret before trusting claims. - payload = jwt.decode(token, key=self._api_secret, algorithms=["HS256"]) - payload["token"] = token - return payload + try: + payload = jwt.decode(token, key=self._api_secret, algorithms=["HS256"]) + payload["token"] = token + return payload + except jwt.InvalidTokenError: + logger.warning("Failed to verify JWT from auth response") + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/api/client.py` around lines 100 - 104, Check that self._api_secret is present and wrap the jwt.decode(token, key=self._api_secret, algorithms=["HS256"]) call in a try/except that catches JWT verification errors (e.g., jwt.InvalidSignatureError, jwt.DecodeError, jwt.ExpiredSignatureError); on failure, do not let the exception bubble as a 500 — log the verification failure including the token or error details and return/raise a clear authentication failure (e.g., return None or raise an AuthenticationError) so callers can respond with a 401/403 instead of an internal server error. Ensure you reference the same variables/functions (token, self._api_secret, jwt.decode) when implementing the checks and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/messages.yml:
- Around line 98-137: Add an explicit permissions block to the workflow or to
each job (e.g., lint-client-bridge, test-client-bridge, test-mta-in,
test-socks-proxy) to restrict the GITHUB_TOKEN scope (at minimum use contents:
read); update either the top-level workflow permissions or add a job-level
permissions: { contents: read } entry for each job to avoid inheriting
repository write access.
In `@src/backend/core/api/serializers.py`:
- Around line 1506-1529: The current _move_sensitive_settings moves sensitive
keys (e.g., "password") from settings into encrypted_settings and update()
persists them, allowing regular updates to set client secrets; modify
_move_sensitive_settings to refuse or ignore incoming password updates on
existing instances: when self.instance is present (update path) do not include
the "password" key in extracted (or remove it before merging) so only creates or
explicit rotate-password flows can set/replace that value; keep existing merging
behavior for other keys and preserve existing encrypted_settings when skipping
password.
In `@src/backend/core/api/viewsets/client_bridge.py`:
- Around line 172-179: The handler currently validates HTTP_X_MAIL_FROM into
mail_from and HTTP_X_RCPT_TO into rcpt_to but then ignores them and reconstructs
recipients using recipient_email/mailbox_email from the MIME payload, which
drops envelope-only recipients and the MAIL FROM; update the outbound pipeline
to accept and use the validated envelope values: pass mail_from and rcpt_to
(parsed into a list) into whatever function(s) build/send the outbound message
(search for recipient_email, mailbox_email, and the send/build helper used later
in this viewset), ensure Bcc/other envelope-only addresses from rcpt_to are
included, and replace any use of recipient_email/mailbox_email as the sole
recipient source (also apply the same change to the similar block around lines
216–239).
- Around line 132-145: The issued JWTs lack any password version/timestamp or
issued-at claim, so rotated passwords don't invalidate existing tokens; update
the token creation in client_bridge.py (where channel_token is encoded) to
include "iat" (issued-at) and a channel-specific revocation marker such as
"password_version" or "password_rotated_at" (e.g., include
channel.password_version or channel.password_rotated_at). Then update the token
verification logic (the function/method that decodes client bridge tokens) to
decode the JWT, read the "iat"/revocation field and compare it against the
current Channel state (e.g., Channel.password_version or
Channel.password_rotated_at or a rotation timestamp from rotate_password()) and
reject the token if it predates rotation or the versions mismatch. Ensure the
new claims are signed and tests cover token acceptance before rotation and
rejection after rotate_password() runs.
- Around line 41-48: The get_cache_key method accesses request.data and calls
.get() and .lower() without validating payload shape; update get_cache_key to
first ensure request.data is a dict and that username is a str (e.g., if not
isinstance(request.data, dict): return None; username =
request.data.get("username"); if not isinstance(username, str): return None)
before using .lower() or returning a cache key, and apply the same defensive
checks to the other places in this module that access request.data.get() (the
two other blocks noted in the review) so malformed JSON (arrays/numbers) cannot
cause attribute/type errors.
- Around line 31-39: Remove the hardcoded rate on the ClientBridgeAuthThrottle
class so Django REST Framework will use the configured throttle rate;
specifically delete the line setting the rate attribute (rate = "5/minute") in
class ClientBridgeAuthThrottle and keep scope = "client_bridge_auth" so the
value is read from DEFAULT_THROTTLE_RATES / API_CLIENT_BRIDGE_AUTH_THROTTLE_RATE
instead.
In `@src/backend/core/mda/outbound.py`:
- Around line 330-338: The draft blob is read after calling _sign_and_store so
message.draft_blob has been cleared; capture it before calling _sign_and_store
and then delete the captured blob afterward. Specifically, assign draft_blob =
message.draft_blob before invoking _sign_and_store(mailbox_sender, message,
composed_mime, ...), call _sign_and_store, then if the previously captured
draft_blob is truthy call draft_blob.delete(); ensure the same change is applied
to the other occurrence around lines 372–375 where draft deletion is attempted
after _sign_and_store.
- Around line 339-342: The loop that unconditionally deletes attachment.blob and
attachment (for attachment in message.attachments.all()) can remove blobs still
referenced by other messages/drafts; update it to mirror the ownership/reference
checks used in the draft cleanup signals: before deleting attachment.blob,
confirm no other Attachment rows reference the same blob (e.g., check for other
Attachment objects with the same blob), and before deleting the Attachment row
ensure it isn’t referenced/owned by any other message/draft; only call
attachment.blob.delete() when the blob is unreferenced elsewhere and only call
attachment.delete() when the attachment is not shared. Use the same helper/logic
from the draft cleanup signals to locate and perform these checks so behavior is
consistent.
- Around line 89-93: The client-bridge branch that handles raw_mime currently
returns immediately and bypasses the outbound size guard; before calling
_sign_and_store(mailbox_sender, message, raw_mime) you must enforce the same
MIME size limit as the SMTP path by checking the length/byte-size of raw_mime
and rejecting/raising an appropriate error (or returning the same
oversized-response) if it exceeds the configured cap; set message.sender_user =
user only after the size check passes so oversized raw_mime inputs are rejected
consistently.
In `@src/client-bridge/src/mailbox.py`:
- Around line 254-257: In the except blocks that catch SessionExpired and then
raise CloseConnection (the blocks referencing SessionExpired, self._folder and
raising CloseConnection()), preserve the original traceback by using exception
chaining (i.e., raise CloseConnection() from e where e is the caught exception);
apply the same change to the other similar except block later (the one around
lines handling generic exceptions at 270-273 that raises CloseConnection) so
both re-raises use "from" to chain the original exception.
In `@src/client-bridge/src/settings.py`:
- Around line 6-13: In _env_bool, change the logic so that an explicitly set but
unrecognized non-empty value raises an error instead of falling back to default:
first get and normalize val, return default only when val == "", map known
true/false tokens to True/False, and if val is non-empty and not in those sets
raise a ValueError (include the env var name and value in the message) — update
the _env_bool function accordingly to mirror the fail-fast behavior used by
_env_int.
In `@src/client-bridge/tests/test_imap_folders.py`:
- Around line 23-24: Replace the lone INBOX membership check with an assertion
that the returned folder_names contain the full expected virtual-folder set;
locate the test using the folder_names variable in test_imap_folders.py and
assert that the set of folder_names contains (or equals) the expected names:
INBOX, Sent, Drafts, Trash, Archive, Spam so a partial listing (e.g., only
INBOX) will fail.
In `@src/client-bridge/tests/test_imap_messages.py`:
- Around line 57-60: The test currently only checks that at least one of the
requested headers is present; update the assertion in test_imap_messages.py
(around the imap_client.fetch call and variables status, data, raw) to assert
each requested header is present individually: confirm status == "OK", extract
raw = data[0][1], and assert b"Subject:" in raw, b"From:" in raw, and b"To:" in
raw so partial regressions are caught when any single header is missing.
In `@src/client-bridge/tests/test_imap_operations.py`:
- Around line 184-205: The tests (e.g., test_expunge_deleted_message) mutate the
session-scoped fixtures imap_server and test_channel which makes later tests
flaky; modify these destructive tests to use an isolated mailbox or restore
state after mutation: create/select a unique temporary mailbox for the test (or
clone/backup the seeded messages) before marking messages \\Deleted and running
EXPUNGE/COPY/CLOSE, or after the operation recreate/replicate the original
messages so the session-scoped imap_server/test_channel remain unchanged; update
the other affected tests (the blocks around lines 227-255, 257-265, 340-369)
similarly to use a per-test mailbox or explicit cleanup.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scss`:
- Around line 234-262: The .client-bridge-form__detail-item layout prevents long
credential strings from wrapping and causes overflow; update the CSS so the
detail value can shrink and wrap by adding min-width: 0 to the dd selector (to
allow the flex child to shrink) and make the code block wrap by setting
white-space to normal and enabling overflow-wrap/word-break (e.g.,
overflow-wrap: anywhere or word-break: break-word) on the code selector; this
lets long emails, hostnames, or passwords wrap instead of overflowing and
preserves the copy button visibility.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 346-347: The UI is incorrectly deriving the IMAP/SMTP host from
window.location.hostname (seen where <code>{window.location.hostname}</code> and
CopyButton use it); replace that with a deploy-time or backend-provided value
instead of the web origin. Update the client-bridge-integration-form component
to read the host from a prop or from a config/service (e.g., a
CLIENT_BRIDGE_HOST constant injected at build time or a backend endpoint like
getClientBridgeConfig) and render that value where window.location.hostname is
currently used (also update the other occurrence around lines 366-367). Ensure
the component falls back to a clear “not configured” UI state if the host value
is absent and avoid using window.location.* anywhere for IMAP/SMTP host
settings.
- Around line 99-106: The code currently gates success behavior on
response.status === 201 inside the mutate handler; remove that exact-status
check so successful responses from mutateAsync (which already throws on non-ok)
always run the success path. Specifically, after await invalidateChannels(),
always extract the password from (response.data as ChannelCreateResponse) and
call setGeneratedPassword(password) if present, then call
onSuccess(response.data) instead of wrapping these calls in the if
(response.status === 201) block; also remove any references to response.status
checks around this logic (the mutateAsync error handling is sufficient).
---
Outside diff comments:
In `@src/backend/core/api/viewsets/channel.py`:
- Around line 57-81: The 201 OpenApi response still references ChannelSerializer
but create() may add a one-time "password" field (from
instance._generated_password), so update the decorator on create to declare a
response schema that includes that password; e.g. create a small response
serializer (or a modified OpenApiResponse) that wraps
serializers.ChannelSerializer plus an optional "password" field and replace the
current 201 OpenApiResponse with it so generated clients see the one-time
secret; adjust the decorator on the create method accordingly (referencing
create, serializers.ChannelSerializer, instance._generated_password, and the
OpenApiResponse for 201).
In `@src/backend/core/mda/inbound_create.py`:
- Around line 274-299: Verify that the parsed sender_email actually belongs to
the recipient mailbox before treating it as the authenticated sender: add a
check in the block around models.Contact.objects.get_or_create that confirms
sender_email is one of mailbox's authorized identities (e.g.,
mailbox.identities/emails, mailbox.allowed_senders, or mailbox.users' emails);
if it is not authorized, log a warning and do not mark or use the resulting
sender_contact as the authenticated bridge sender (i.e., do not set
is_sender=True or bind bridge credentials to it)—instead reject the outbound
submission or create the contact only as an external/non-sender entry; apply the
same validation to the corresponding outbound handling code referenced around
lines 361-385.
---
Duplicate comments:
In `@src/client-bridge/src/backend.py`:
- Around line 140-143: The current logs emit PII by including authcid; update
both logging calls around the auth check (the logger.warning("IMAP auth rejected
for %s: sender_only has no read access", authcid) and logger.info("Authenticated
channel %s (role=%s)", authcid, role)) to avoid logging the raw mailbox
identifier — instead log a non-PII channel identifier (e.g., an existing
channel_id variable) or a deterministic redacted/hashed token derived from
authcid (e.g., a short sha256/hexdigest or masked string) so that InvalidAuth
behavior remains unchanged but no PII is written to logs.
---
Nitpick comments:
In `@src/client-bridge/src/api/client.py`:
- Around line 100-104: Check that self._api_secret is present and wrap the
jwt.decode(token, key=self._api_secret, algorithms=["HS256"]) call in a
try/except that catches JWT verification errors (e.g.,
jwt.InvalidSignatureError, jwt.DecodeError, jwt.ExpiredSignatureError); on
failure, do not let the exception bubble as a 500 — log the verification failure
including the token or error details and return/raise a clear authentication
failure (e.g., return None or raise an AuthenticationError) so callers can
respond with a 401/403 instead of an internal server error. Ensure you reference
the same variables/functions (token, self._api_secret, jwt.decode) when
implementing the checks and error handling.
In `@src/client-bridge/tests/conftest.py`:
- Around line 548-594: Update the fixture teardowns (imap_client,
imap_connection, smtp_client, smtp_connection) to log any exceptions instead of
silently passing: add a module-level logger (logging.getLogger(__name__)) and in
each except Exception block catch the exception as e and call logger.exception
or logger.warning with context (e.g., "imap_client.logout failed") referencing
the failing method names logout() and quit() so teardown failures are recorded
for debugging.
In `@src/frontend/public/locales/common/en-US.json`:
- Line 349: The JSON locale keys are out of alphabetical order; move the key "My
email client" so it appears after the "Monthly" key, and move "Role", "Rotate
password", and "Rotating..." so they appear after the "Reply all" key to restore
alphabetical ordering; update the entries in the same file (preserve their
values and surrounding commas) and run the linter/formatter to verify ordering.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 31-34: Replace the deprecated Zod 4 validation option for the name
field in formSchema: change z.string().min(1, { message: t("Name is required.")
}) to use the new error option (z.string().min(1, { error: t("Name is
required.") })); keep the role enum unchanged. Locate the formSchema function
and update the min option object for the name validator to use error instead of
message so it aligns with the rest of the frontend validation code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7773cc-63c1-46ce-9572-4cab75cbfd09
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/rotate_password_response.tsis excluded by!**/gen/**
📒 Files selected for processing (27)
.github/workflows/messages.ymlREADME.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/client_bridge.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/tests/api/test_client_bridge.pysrc/backend/messages/settings.pysrc/client-bridge/Dockerfilesrc/client-bridge/README.mdsrc/client-bridge/src/api/client.pysrc/client-bridge/src/backend.pysrc/client-bridge/src/mailbox.pysrc/client-bridge/src/settings.pysrc/client-bridge/src/submission.pysrc/client-bridge/tests/conftest.pysrc/client-bridge/tests/test_imap_auth.pysrc/client-bridge/tests/test_imap_folders.pysrc/client-bridge/tests/test_imap_messages.pysrc/client-bridge/tests/test_imap_operations.pysrc/client-bridge/tests/test_smtp_submission.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scsssrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/core/api/openapi.json
- README.md
- src/client-bridge/tests/test_smtp_submission.py
Comment on lines
+31
to
+39
| class ClientBridgeAuthThrottle(SimpleRateThrottle): | ||
| """Throttle auth attempts by username to prevent brute-force attacks. | ||
|
|
||
| All requests come from the client-bridge service so IP-based throttling | ||
| is useless. We key on the username (email) from the request body instead. | ||
| """ | ||
|
|
||
| scope = "client_bridge_auth" | ||
| rate = "5/minute" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Django REST Framework 3.16.1, if a SimpleRateThrottlesubclass defines bothscope = "client_bridge_auth"andrate = "5/minute", will DEFAULT_THROTTLE_RATES["client_bridge_auth"] still be used, or does the class attribute override it? Please cite the relevant docs or source.
💡 Result:
If you set rate = "5/minute" on your SimpleRateThrottle subclass, that class attribute wins and DRF will not consult DEFAULT_THROTTLE_RATES["client_bridge_auth"], even if you also set scope = "client_bridge_auth".
- Source (DRF 3.16.1):
SimpleRateThrottle.__init__only callsself.get_rate()(which looks upDEFAULT_THROTTLE_RATES[self.scope]) whenself.rateis not already set (if not getattr(self, 'rate', None): ...). So an explicitrateprevents the settings lookup. [1] - Docs: DRF explicitly states the rate is determined (in order) by (1) the
rateproperty on the class, then (2) the correspondingDEFAULT_THROTTLE_RATES[...]setting (shown forAnonRateThrottle/UserRateThrottle, which inherit the sameSimpleRateThrottlebehavior). [2]
Sources: [1] https://raw.githubusercontent.com/encode/django-rest-framework/3.16.1/rest_framework/throttling.py, [2] https://www.django-rest-framework.org/api-guide/throttling/
Remove the hardcoded rate to allow API_CLIENT_BRIDGE_AUTH_THROTTLE_RATE setting to take effect.
With rate = "5/minute" on the class, Django REST Framework ignores DEFAULT_THROTTLE_RATES["client_bridge_auth"]. Operators cannot tune this throttle control without code changes. Delete the rate attribute and let the setting (scope = "client_bridge_auth") resolve the rate from configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/client_bridge.py` around lines 31 - 39, Remove
the hardcoded rate on the ClientBridgeAuthThrottle class so Django REST
Framework will use the configured throttle rate; specifically delete the line
setting the rate attribute (rate = "5/minute") in class ClientBridgeAuthThrottle
and keep scope = "client_bridge_auth" so the value is read from
DEFAULT_THROTTLE_RATES / API_CLIENT_BRIDGE_AUTH_THROTTLE_RATE instead.
Comment on lines
+41
to
+48
| def get_cache_key(self, request, view): | ||
| username = request.data.get("username") | ||
| if not username: | ||
| return None | ||
| return self.cache_format % { | ||
| "scope": self.scope, | ||
| "ident": username.lower(), | ||
| } |
There was a problem hiding this comment.
Validate the auth payload shape before touching request.data.
request.data is not guaranteed to be a dict of strings. A JSON array/number here will make the throttle's .get()/.lower() path or compare_digest() raise before you return 400/401, turning malformed input into a 500 on a public endpoint.
Also applies to: 95-101, 127-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/client_bridge.py` around lines 41 - 48, The
get_cache_key method accesses request.data and calls .get() and .lower() without
validating payload shape; update get_cache_key to first ensure request.data is a
dict and that username is a str (e.g., if not isinstance(request.data, dict):
return None; username = request.data.get("username"); if not
isinstance(username, str): return None) before using .lower() or returning a
cache key, and apply the same defensive checks to the other places in this
module that access request.data.get() (the two other blocks noted in the review)
so malformed JSON (arrays/numbers) cannot cause attribute/type errors.
Comment on lines
+132
to
+145
| channel_token = jwt.encode( | ||
| { | ||
| "channel_id": str(channel.id), | ||
| "mailbox_id": str(mailbox.id), | ||
| "mailbox_email": username, | ||
| "role": role, | ||
| "exp": expires_at, | ||
| }, | ||
| settings.CLIENTBRIDGE_API_SECRET, | ||
| algorithm="HS256", | ||
| ) | ||
| return Response( | ||
| {"token": channel_token}, | ||
| status=status.HTTP_200_OK, |
There was a problem hiding this comment.
Rotation won't revoke already-issued bridge sessions.
These JWTs carry no password version, rotation timestamp, or iat that can be compared to channel state later. Once a client authenticates, rotate_password() only blocks new logins; existing tokens keep working until CLIENTBRIDGE_SESSION_TIMEOUT expires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/client_bridge.py` around lines 132 - 145, The
issued JWTs lack any password version/timestamp or issued-at claim, so rotated
passwords don't invalidate existing tokens; update the token creation in
client_bridge.py (where channel_token is encoded) to include "iat" (issued-at)
and a channel-specific revocation marker such as "password_version" or
"password_rotated_at" (e.g., include channel.password_version or
channel.password_rotated_at). Then update the token verification logic (the
function/method that decodes client bridge tokens) to decode the JWT, read the
"iat"/revocation field and compare it against the current Channel state (e.g.,
Channel.password_version or Channel.password_rotated_at or a rotation timestamp
from rotate_password()) and reject the token if it predates rotation or the
versions mismatch. Ensure the new claims are signed and tests cover token
acceptance before rotation and rejection after rotate_password() runs.
Comment on lines
+57
to
+60
| status, data = imap_client.fetch("1", "(BODY[HEADER.FIELDS (SUBJECT FROM TO)])") | ||
| assert status == "OK" | ||
| raw = data[0][1] | ||
| assert b"Subject:" in raw or b"From:" in raw |
There was a problem hiding this comment.
The header assertion is too weak for this request shape.
Line 57 asks for SUBJECT FROM TO, but the current or check passes even if two of those headers disappear. Assert each requested header so the test catches partial regressions.
💡 Suggested assertion
- assert b"Subject:" in raw or b"From:" in raw
+ assert b"Subject:" in raw
+ assert b"From:" in raw
+ assert b"To:" in raw📝 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
| status, data = imap_client.fetch("1", "(BODY[HEADER.FIELDS (SUBJECT FROM TO)])") | |
| assert status == "OK" | |
| raw = data[0][1] | |
| assert b"Subject:" in raw or b"From:" in raw | |
| status, data = imap_client.fetch("1", "(BODY[HEADER.FIELDS (SUBJECT FROM TO)])") | |
| assert status == "OK" | |
| raw = data[0][1] | |
| assert b"Subject:" in raw | |
| assert b"From:" in raw | |
| assert b"To:" in raw |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client-bridge/tests/test_imap_messages.py` around lines 57 - 60, The test
currently only checks that at least one of the requested headers is present;
update the assertion in test_imap_messages.py (around the imap_client.fetch call
and variables status, data, raw) to assert each requested header is present
individually: confirm status == "OK", extract raw = data[0][1], and assert
b"Subject:" in raw, b"From:" in raw, and b"To:" in raw so partial regressions
are caught when any single header is missing.
Comment on lines
+184
to
+205
| def test_expunge_deleted_message(imap_client): | ||
| """Test EXPUNGE removes messages marked with \\Deleted.""" | ||
| imap_client.select("INBOX") | ||
|
|
||
| # Get initial count | ||
| status, data = imap_client.search(None, "ALL") | ||
| assert status == "OK" | ||
| initial_count = len(data[0].split()) | ||
|
|
||
| # Mark message 1 as deleted | ||
| status, data = imap_client.store("1", "+FLAGS", "(\\Deleted)") | ||
| assert status == "OK" | ||
|
|
||
| # Expunge | ||
| status, data = imap_client.expunge() | ||
| assert status == "OK" | ||
|
|
||
| # Message count should be reduced | ||
| status, data = imap_client.search(None, "ALL") | ||
| assert status == "OK" | ||
| new_count = len(data[0].split()) if data[0] else 0 | ||
| assert new_count == initial_count - 1 |
There was a problem hiding this comment.
These tests permanently mutate a session-scoped mailbox.
imap_server and test_channel are session-scoped in src/client-bridge/tests/conftest.py, so the EXPUNGE/COPY/CLOSE cases here change the shared fixture for every later test. That makes count-based assertions elsewhere order-dependent and flaky; use an isolated mailbox fixture or restore the seeded state after each destructive case.
Also applies to: 227-255, 257-265, 340-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client-bridge/tests/test_imap_operations.py` around lines 184 - 205, The
tests (e.g., test_expunge_deleted_message) mutate the session-scoped fixtures
imap_server and test_channel which makes later tests flaky; modify these
destructive tests to use an isolated mailbox or restore state after mutation:
create/select a unique temporary mailbox for the test (or clone/backup the
seeded messages) before marking messages \\Deleted and running
EXPUNGE/COPY/CLOSE, or after the operation recreate/replicate the original
messages so the session-scoped imap_server/test_channel remain unchanged; update
the other affected tests (the blocks around lines 227-255, 257-265, 340-369)
similarly to use a per-test mailbox or explicit cleanup.
Comment on lines
+234
to
+262
| .client-bridge-form__detail-item { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: var(--c--globals--spacings--sm); | ||
|
|
||
| dt { | ||
| font-weight: 600; | ||
| font-size: 0.85rem; | ||
| color: var(--c--contextuals--content--semantic--neutral--secondary); | ||
| min-width: 100px; | ||
| flex-shrink: 0; | ||
| } | ||
|
|
||
| dd { | ||
| margin: 0; | ||
| font-size: 0.85rem; | ||
| display: flex; | ||
| align-items: center; | ||
| gap: var(--c--globals--spacings--2xs); | ||
| } | ||
|
|
||
| code { | ||
| font-family: "Fira Code", Consolas, monospace; | ||
| font-size: 0.8rem; | ||
| background: var(--c--contextuals--background--surface--primary); | ||
| padding: 2px 6px; | ||
| border-radius: 3px; | ||
| border: 1px solid var(--c--contextuals--border--semantic--neutral--default); | ||
| } |
There was a problem hiding this comment.
Allow long credential values to wrap.
These rows are locked to a single flex line, and the <code> values are effectively unbreakable. Long email addresses, bridge hostnames, or generated passwords will overflow the modal on narrower widths and can push the copy button out of view.
💡 Proposed fix
.client-bridge-form__detail-item {
display: flex;
- align-items: center;
+ align-items: flex-start;
gap: var(--c--globals--spacings--sm);
dt {
font-weight: 600;
font-size: 0.85rem;
@@
dd {
margin: 0;
font-size: 0.85rem;
display: flex;
align-items: center;
+ flex-wrap: wrap;
+ min-width: 0;
gap: var(--c--globals--spacings--2xs);
}
code {
font-family: "Fira Code", Consolas, monospace;
font-size: 0.8rem;
background: var(--c--contextuals--background--surface--primary);
padding: 2px 6px;
border-radius: 3px;
border: 1px solid var(--c--contextuals--border--semantic--neutral--default);
+ overflow-wrap: anywhere;
}
}📝 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
| .client-bridge-form__detail-item { | |
| display: flex; | |
| align-items: center; | |
| gap: var(--c--globals--spacings--sm); | |
| dt { | |
| font-weight: 600; | |
| font-size: 0.85rem; | |
| color: var(--c--contextuals--content--semantic--neutral--secondary); | |
| min-width: 100px; | |
| flex-shrink: 0; | |
| } | |
| dd { | |
| margin: 0; | |
| font-size: 0.85rem; | |
| display: flex; | |
| align-items: center; | |
| gap: var(--c--globals--spacings--2xs); | |
| } | |
| code { | |
| font-family: "Fira Code", Consolas, monospace; | |
| font-size: 0.8rem; | |
| background: var(--c--contextuals--background--surface--primary); | |
| padding: 2px 6px; | |
| border-radius: 3px; | |
| border: 1px solid var(--c--contextuals--border--semantic--neutral--default); | |
| } | |
| .client-bridge-form__detail-item { | |
| display: flex; | |
| align-items: flex-start; | |
| gap: var(--c--globals--spacings--sm); | |
| dt { | |
| font-weight: 600; | |
| font-size: 0.85rem; | |
| color: var(--c--contextuals--content--semantic--neutral--secondary); | |
| min-width: 100px; | |
| flex-shrink: 0; | |
| } | |
| dd { | |
| margin: 0; | |
| font-size: 0.85rem; | |
| display: flex; | |
| align-items: center; | |
| flex-wrap: wrap; | |
| min-width: 0; | |
| gap: var(--c--globals--spacings--2xs); | |
| } | |
| code { | |
| font-family: "Fira Code", Consolas, monospace; | |
| font-size: 0.8rem; | |
| background: var(--c--contextuals--background--surface--primary); | |
| padding: 2px 6px; | |
| border-radius: 3px; | |
| border: 1px solid var(--c--contextuals--border--semantic--neutral--default); | |
| overflow-wrap: anywhere; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scss`
around lines 234 - 262, The .client-bridge-form__detail-item layout prevents
long credential strings from wrapping and causes overflow; update the CSS so the
detail value can shrink and wrap by adding min-width: 0 to the dd selector (to
allow the flex child to shrink) and make the code block wrap by setting
white-space to normal and enabling overflow-wrap/word-break (e.g.,
overflow-wrap: anywhere or word-break: break-word) on the code selector; this
lets long emails, hostnames, or passwords wrap instead of overflowing and
preserves the copy button visibility.
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 @.github/workflows/messages.yml:
- Around line 101-109: The lint-client-bridge job currently runs the Makefile
target lint-client-bridge which runs `ruff format .` and can silently modify
files; update the workflow to enforce formatting by adding a guard step after
running the target that fails on uncommitted changes (e.g., run a git diff
--exit-code check) or replace the Makefile target with `ruff format --check .`;
target the job name `lint-client-bridge` and the Makefile target
`lint-client-bridge`/`ruff format` in your change so the CI fails when sources
need formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88e3e256-973d-4ee4-b2ff-135d0dd5e79c
📒 Files selected for processing (5)
.github/workflows/crowdin_download.yml.github/workflows/crowdin_upload.yml.github/workflows/docker-publish.yml.github/workflows/messages-ghcr.yml.github/workflows/messages.yml
jbpenrath
reviewed
Mar 9, 2026
| "DEFAULT_AUTHENTICATION_CLASSES": ( | ||
| "mozilla_django_oidc.contrib.drf.OIDCAuthentication", | ||
| "rest_framework.authentication.SessionAuthentication", | ||
| "core.authentication.client_bridge.ClientBridgeChannelAuthentication", |
Contributor
There was a problem hiding this comment.
Do we really want to expose this authentication class all the time even if client bridge feature is disabled?
Comment on lines
+251
to
+261
| # Client-bridge endpoints (IMAP/SMTP auth and message submission) | ||
| path( | ||
| f"api/{settings.API_VERSION}/client-bridge/auth/", | ||
| ClientBridgeAuthView.as_view(), | ||
| name="client-bridge-auth", | ||
| ), | ||
| path( | ||
| f"api/{settings.API_VERSION}/client-bridge/submit/", | ||
| ClientBridgeSubmitView.as_view(), | ||
| name="client-bridge-submit", | ||
| ), |
Contributor
There was a problem hiding this comment.
Ditto should we expose those endpoints when client bridge is disabled ?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx (1)
32-35: Useerrorinstead ofmessagefor Zod 4 validation.Zod 4 replaced the
messageparameter with a unifiederrorparameter for error customization.♻️ Proposed fix
const formSchema = (t: (key: string) => string) => z.object({ - name: z.string().min(1, { message: t("Name is required.") }), + name: z.string().min(1, { error: t("Name is required.") }), role: z.enum(["reader", "editor", "sender", "sender_only"]), });Based on learnings: "In Zod 4, replace the deprecated message option with the error parameter for error customization. Use syntax like z.string().min(1, { error: "message" }) instead of { message: "message" }."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 32 - 35, The Zod schema in formSchema uses the deprecated { message: ... } option; update the validators to use the Zod 4 { error: ... } option instead—replace z.string().min(1, { message: t("Name is required.") }) with a version that supplies { error: t("Name is required.") } and ensure any other occurrences (e.g., within formSchema or other validators around role/enums) follow the same { error: ... } shape so the schema is Zod 4 compatible.src/backend/core/api/openapi.json (1)
261-290: Mark the client-bridge connection tuple as complete when present.If
CLIENTBRIDGE_PUBLIC_CONFIGis returned, clients need all six values to configure IMAP/SMTP. Leaving every nested field optional makes the contract looser than the adjacentDRIVEobject and weakens generated types.♻️ Proposed schema tightening
"CLIENTBRIDGE_PUBLIC_CONFIG": { "type": "object", "description": "Client-bridge IMAP/SMTP connection settings for email clients.", "readOnly": true, "properties": { "imap_host": { "type": "string", "readOnly": true }, "imap_port": { "type": "integer", "readOnly": true }, "imap_security": { "type": "string", "readOnly": true }, "smtp_host": { "type": "string", "readOnly": true }, "smtp_port": { "type": "integer", "readOnly": true }, "smtp_security": { "type": "string", "readOnly": true } - } + }, + "required": [ + "imap_host", + "imap_port", + "imap_security", + "smtp_host", + "smtp_port", + "smtp_security" + ] }🤖 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 261 - 290, The CLIENTBRIDGE_PUBLIC_CONFIG schema currently marks all six nested fields as optional; change it so that when CLIENTBRIDGE_PUBLIC_CONFIG is present the full IMAP/SMTP tuple is required by adding a "required" array referencing "imap_host","imap_port","imap_security","smtp_host","smtp_port","smtp_security" in the CLIENTBRIDGE_PUBLIC_CONFIG object schema (mirror how DRIVE is tightened) so generated types require all six values when the object exists.
🤖 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 2894-2896: The OpenAPI spec's 400 response for the channel
endpoint only has a description but should document the JSON body returned by
src/backend/core/api/viewsets/channel.py (around the logic at lines 136-144)
which returns {"detail": "..."}; update the 400 response schema in openapi.json
for that endpoint to an object schema with a string property "detail" (required)
and appropriate example, so the documented contract matches the actual response
from the Channel view (ChannelViewSet / the channel rotation/password
endpoints).
In `@src/backend/messages/settings.py`:
- Around line 1086-1090: When FEATURE_CLIENTBRIDGE is enabled (the same spot
that checks FEATURE_CLIENTBRIDGE and CLIENTBRIDGE_API_SECRET), validate that
CLIENTBRIDGE_PUBLIC_CONFIG is a dict and contains the required public keys for
clientbridge (at minimum non-empty 'imap' and 'smtp' entries and that each entry
includes required nested fields such as 'host' and 'port'); if any required keys
are missing or empty raise a ValueError with which keys are missing so startup
fails fast. Implement this check alongside the existing
FEATURE_CLIENTBRIDGE/CLIENTBRIDGE_API_SECRET validation (referencing
FEATURE_CLIENTBRIDGE, CLIENTBRIDGE_API_SECRET, and CLIENTBRIDGE_PUBLIC_CONFIG)
and mirror the exact required key contract in the API view that forwards the
dict so the schema and runtime validation stay consistent.
- Around line 567-571: The global DRF DEFAULT_AUTHENTICATION_CLASSES currently
includes ClientBridgeChannelAuthentication which unintentionally lets any view
use X-Channel-Token; remove
"core.authentication.client_bridge.ClientBridgeChannelAuthentication" from
DEFAULT_AUTHENTICATION_CLASSES in settings and instead add that authenticator
only to bridge-facing endpoints (e.g., set authentication_classes =
[ClientBridgeChannelAuthentication] on the viewsets or APIViews that implement
the bridge submission flow referenced in client_bridge.py and
reverse("client-bridge-submit")). Ensure other views keep the existing
OIDC/Session authentication defaults and audit viewsets under core/api/viewsets
for places to add the bridge auth locally.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 261-290: The CLIENTBRIDGE_PUBLIC_CONFIG schema currently marks all
six nested fields as optional; change it so that when CLIENTBRIDGE_PUBLIC_CONFIG
is present the full IMAP/SMTP tuple is required by adding a "required" array
referencing
"imap_host","imap_port","imap_security","smtp_host","smtp_port","smtp_security"
in the CLIENTBRIDGE_PUBLIC_CONFIG object schema (mirror how DRIVE is tightened)
so generated types require all six values when the object exists.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 32-35: The Zod schema in formSchema uses the deprecated { message:
... } option; update the validators to use the Zod 4 { error: ... } option
instead—replace z.string().min(1, { message: t("Name is required.") }) with a
version that supplies { error: t("Name is required.") } and ensure any other
occurrences (e.g., within formSchema or other validators around role/enums)
follow the same { error: ... } shape so the schema is Zod 4 compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ce219d1-04cd-47ce-bdb3-4a96d2e9cf0d
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200_clientbridge_public_confi_g.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**
📒 Files selected for processing (10)
.github/workflows/messages-ghcr.yml.github/workflows/messages.ymlMakefiledocs/env.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/viewsets/config.pysrc/backend/messages/settings.pysrc/client-bridge/README.mdsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsxsrc/frontend/src/features/providers/config.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/env.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/messages.yml
Comment on lines
+2894
to
+2896
| "400": { | ||
| "description": "Channel type does not support password rotation" | ||
| }, |
There was a problem hiding this comment.
Expose the custom 400 payload for unsupported channels.
The backend returns a JSON body with detail here, but the schema only has a description. That leaves the documented contract out of sync with src/backend/core/api/viewsets/channel.py:136-144.
🛠️ Proposed response schema
"400": {
+ "content": {
+ "application/json": {
+ "schema": {
+ "type": "object",
+ "properties": {
+ "detail": {
+ "type": "string"
+ }
+ },
+ "required": [
+ "detail"
+ ]
+ }
+ }
+ },
"description": "Channel type does not support password rotation"
},📝 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
| "400": { | |
| "description": "Channel type does not support password rotation" | |
| }, | |
| "400": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "detail": { | |
| "type": "string" | |
| } | |
| }, | |
| "required": [ | |
| "detail" | |
| ] | |
| } | |
| } | |
| }, | |
| "description": "Channel type does not support password rotation" | |
| }, |
🤖 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 2894 - 2896, The OpenAPI
spec's 400 response for the channel endpoint only has a description but should
document the JSON body returned by src/backend/core/api/viewsets/channel.py
(around the logic at lines 136-144) which returns {"detail": "..."}; update the
400 response schema in openapi.json for that endpoint to an object schema with a
string property "detail" (required) and appropriate example, so the documented
contract matches the actual response from the Channel view (ChannelViewSet / the
channel rotation/password endpoints).
Comment on lines
567
to
571
| "DEFAULT_AUTHENTICATION_CLASSES": ( | ||
| "mozilla_django_oidc.contrib.drf.OIDCAuthentication", | ||
| "rest_framework.authentication.SessionAuthentication", | ||
| "core.authentication.client_bridge.ClientBridgeChannelAuthentication", | ||
| ), |
There was a problem hiding this comment.
Keep bridge auth out of the global DRF defaults.
Unless bridge tokens are meant to authorize the full REST API, this is too broad. src/backend/core/authentication/client_bridge.py:47-110 mostly authorizes by HTTP method, so any view inheriting DEFAULT_AUTHENTICATION_CLASSES will accept X-Channel-Token as the owning user, including non-bridge views.
🔐 Suggested scoping
"DEFAULT_AUTHENTICATION_CLASSES": (
"mozilla_django_oidc.contrib.drf.OIDCAuthentication",
"rest_framework.authentication.SessionAuthentication",
- "core.authentication.client_bridge.ClientBridgeChannelAuthentication",
),# Add the bridge auth class only on bridge-facing views/routes.
from core.authentication.client_bridge import ClientBridgeChannelAuthentication
authentication_classes = [ClientBridgeChannelAuthentication]Use this read-only check to confirm the current exposure. You should see the global registration, the method-based checks in the authenticator, and the local auth overrides under the API viewsets:
#!/bin/bash
set -euo pipefail
echo "Global DRF authentication defaults:"
rg -n -C2 'DEFAULT_AUTHENTICATION_CLASSES|ClientBridgeChannelAuthentication' \
src/backend/messages/settings.py
echo
echo "Client-bridge authenticator checks:"
rg -n -C3 'X-Channel-Token|reverse\("client-bridge-submit"\)|return \(channel\.user, channel\)|_SAFE_METHODS|CLIENT_BRIDGE_ROLES_CAN_' \
src/backend/core/authentication/client_bridge.py
echo
echo "APIView/ViewSet authentication overrides:"
rg -n -C2 'authentication_classes\s*=' --type=py src/backend/core/api/viewsets🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/messages/settings.py` around lines 567 - 571, The global DRF
DEFAULT_AUTHENTICATION_CLASSES currently includes
ClientBridgeChannelAuthentication which unintentionally lets any view use
X-Channel-Token; remove
"core.authentication.client_bridge.ClientBridgeChannelAuthentication" from
DEFAULT_AUTHENTICATION_CLASSES in settings and instead add that authenticator
only to bridge-facing endpoints (e.g., set authentication_classes =
[ClientBridgeChannelAuthentication] on the viewsets or APIViews that implement
the bridge submission flow referenced in client_bridge.py and
reverse("client-bridge-submit")). Ensure other views keep the existing
OIDC/Session authentication defaults and audit viewsets under core/api/viewsets
for places to add the bridge auth locally.
jbpenrath
reviewed
Mar 9, 2026
|
|
||
| const formSchema = (t: (key: string) => string) => z.object({ | ||
| name: z.string().min(1, { message: t("Name is required.") }), | ||
| role: z.enum(["reader", "editor", "sender", "sender_only"]), |
Contributor
There was a problem hiding this comment.
It would be great if those values comes from the OpenAPI Schema.
Surely need to decorate serializer method to type channel.settings property.
Comment on lines
+258
to
+284
| const CopyButton = ({ value }: { value: string }) => { | ||
| const { t } = useTranslation(); | ||
| return ( | ||
| <Button | ||
| type="button" | ||
| variant="tertiary" | ||
| size="small" | ||
| icon={<Icon name="content_copy" type={IconType.OUTLINED} size={IconSize.SMALL} />} | ||
| onClick={() => { | ||
| navigator.clipboard.writeText(value).then(() => { | ||
| addToast( | ||
| <ToasterItem type="info"> | ||
| <span>{t("Copied to clipboard")}</span> | ||
| </ToasterItem> | ||
| ); | ||
| }).catch(() => { | ||
| addToast( | ||
| <ToasterItem type="error"> | ||
| <span>{t("Failed to copy to clipboard")}</span> | ||
| </ToasterItem> | ||
| ); | ||
| }); | ||
| }} | ||
| aria-label={t("Copy")} | ||
| /> | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
This component could be put in src/features/ui/components has it has a high reusability potential.
jbpenrath
reviewed
Mar 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx (1)
296-303:⚠️ Potential issue | 🟡 MinorDisplay a warning or disable the connection details section when
bridgeConfigis undefined.The
CLIENTBRIDGE_PUBLIC_CONFIGfield is optional in the API schema. When the backend doesn't provide it, the code silently falls back towindow.location.hostnamefor IMAP/SMTP hosts (lines 298, 301), which will produce incorrect connection details. Users will not understand why their email client configuration fails. Either show a clear warning when the config is missing or disable this section entirely whenbridgeConfigis undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 296 - 303, The code currently uses config.CLIENTBRIDGE_PUBLIC_CONFIG (bridgeConfig) directly and silently falls back to window.location.hostname for imapHost/smtpHost, which yields wrong connection details when bridgeConfig is undefined; update the component that defines bridgeConfig, imapHost, imapPort, imapSecurity, smtpHost, smtpPort, smtpSecurity to check whether bridgeConfig exists and if not either render the connection details section disabled (make inputs readOnly/disabled and prevent submission) or render a visible warning banner informing the user that CLIENTBRIDGE_PUBLIC_CONFIG is missing and connection details cannot be auto-filled; ensure this check gating is applied wherever imapHost/smtpHost are used so the UI does not display or rely on the hostname fallback when bridgeConfig is undefined.env.d/development/backend.defaults (1)
103-106:⚠️ Potential issue | 🟠 MajorThis default env now trips the backend startup guard.
FEATURE_CLIENTBRIDGE=Trueis checked in here, but this file never setsCLIENTBRIDGE_API_SECRET. With the newpost_setup()validation, the default development backend will fail to boot unless every developer adds a local override. Either add the same secret asenv.d/development/client-bridge.defaultshere, or leave the feature disabled by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@env.d/development/backend.defaults` around lines 103 - 106, The development defaults enable FEATURE_CLIENTBRIDGE but do not set CLIENTBRIDGE_API_SECRET, which causes post_setup() validation to fail; fix by either disabling the feature by setting FEATURE_CLIENTBRIDGE=False in this file or adding the same CLIENTBRIDGE_API_SECRET value used in env.d/development/client-bridge.defaults so post_setup() sees a valid secret; update the CLIENTBRIDGE_PUBLIC_CONFIG if needed to match the chosen change and ensure FEATURE_MAILBOX_ADMIN_CHANNELS remains consistent with the new default.src/backend/messages/settings.py (2)
1087-1091:⚠️ Potential issue | 🟠 MajorValidate
CLIENTBRIDGE_PUBLIC_CONFIGin the same startup guard.
FEATURE_CLIENTBRIDGEcan still boot with an empty or partial public config, and the UI will only fail later when it tries to render or copy the IMAP/SMTP settings. Fail fast here on the required keys.🧪 Suggested startup check
if cls.FEATURE_CLIENTBRIDGE and not cls.CLIENTBRIDGE_API_SECRET: raise ValueError( "CLIENTBRIDGE_API_SECRET must be set when " "FEATURE_CLIENTBRIDGE is enabled" ) + + if cls.FEATURE_CLIENTBRIDGE: + required_clientbridge_public_config = { + "imap_host", + "imap_port", + "imap_security", + "smtp_host", + "smtp_port", + "smtp_security", + } + missing = required_clientbridge_public_config.difference( + cls.CLIENTBRIDGE_PUBLIC_CONFIG.keys() + ) + if missing: + raise ValueError( + "CLIENTBRIDGE_PUBLIC_CONFIG must define: " + + ", ".join(sorted(missing)) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/messages/settings.py` around lines 1087 - 1091, When FEATURE_CLIENTBRIDGE is enabled, extend the existing startup guard (the block referencing cls.FEATURE_CLIENTBRIDGE and cls.CLIENTBRIDGE_API_SECRET) to also validate cls.CLIENTBRIDGE_PUBLIC_CONFIG: ensure it is present, is a mapping/dict, and contains the required public keys (e.g., the IMAP/SMTP entries the UI expects) and that those entries include required subkeys before allowing startup; if validation fails, raise a ValueError with a clear message (similar to the CLIENTBRIDGE_API_SECRET check) so the app fails fast rather than letting the UI error later.
568-572:⚠️ Potential issue | 🟠 MajorKeep
ClientBridgeChannelAuthenticationoff the global DRF defaults.Registering the bridge authenticator here lets any DRF view that uses the default auth stack accept
X-Channel-Tokenas the channel owner. Scope it to the bridge-facing endpoints instead.🔐 Minimal hardening
"DEFAULT_AUTHENTICATION_CLASSES": ( "mozilla_django_oidc.contrib.drf.OIDCAuthentication", "rest_framework.authentication.SessionAuthentication", - "core.authentication.client_bridge.ClientBridgeChannelAuthentication", ),Then add
ClientBridgeChannelAuthenticationexplicitly on the bridge API views/viewsets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/messages/settings.py` around lines 568 - 572, Remove "core.authentication.client_bridge.ClientBridgeChannelAuthentication" from the global DEFAULT_AUTHENTICATION_CLASSES tuple in settings (leave OIDCAuthentication and SessionAuthentication) and instead add ClientBridgeChannelAuthentication only to the bridge-facing DRF views/viewsets by setting authentication_classes on those specific View or ViewSet classes (e.g., the bridge API view handlers) so only bridge endpoints accept X-Channel-Token; ensure the global DEFAULT_AUTHENTICATION_CLASSES symbol remains unchanged except for that removal and that bridge views explicitly import and include core.authentication.client_bridge.ClientBridgeChannelAuthentication in their authentication_classes.
🧹 Nitpick comments (8)
src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx (2)
68-73: Guard against missingselectedMailboxinstead of using non-null assertions.The repeated
selectedMailbox!.idassertions (lines 70, 81, 93, 119) will throw at runtime ifselectedMailboxis unexpectedly null. Consider adding an early guard.♻️ Proposed fix
Add an early return at the component level:
// After line 50 if (!selectedMailbox) { return null; }Then remove all
!assertions and useselectedMailbox.iddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 68 - 73, The code uses non-null assertions on selectedMailbox (seen in invalidateChannels and other places) which can crash at runtime; add an early guard in the component to return null (or a fallback) when selectedMailbox is falsy, then remove all `!` assertions and use selectedMailbox.id directly (update functions like invalidateChannels and calls to getMailboxesChannelsListUrl to reference selectedMailbox.id safely).
178-183: Extract duplicated role options to a constant.The role options array is defined identically in both the edit form (lines 178-183) and create form (lines 228-233). Extract to a shared constant to avoid duplication.
♻️ Proposed fix
Add near the top of the file (after line 37):
const getRoleOptions = (t: (key: string) => string) => [ { value: "reader", label: t("Reader — read-only IMAP access") }, { value: "editor", label: t("Editor — IMAP read and edit flags") }, { value: "sender", label: t("Sender — full IMAP and SMTP access") }, { value: "sender_only", label: t("Sender only — SMTP send, no IMAP") }, ];Then use
options={getRoleOptions(t)}in bothRhfSelectcomponents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx` around lines 178 - 183, Extract the duplicated role options array into a single helper and use it in both places: create a constant function like getRoleOptions(t) that returns the four option objects, place it near the top of the file, and replace the inline arrays in the two RhfSelect usages (the ones in the edit form and the create form in client-bridge-integration-form.tsx) with options={getRoleOptions(t)} so both forms reuse the same source of truth.src/client-bridge/src/mailbox.py (2)
595-595: Minor: Simplify dict initialization.Static analysis suggests using
dict.fromkeys()instead of dict comprehension for this initialization.♻️ Proposed simplification
- self._subscribed: dict[str, bool] = {name: True for name in VIRTUAL_FOLDERS} + self._subscribed: dict[str, bool] = dict.fromkeys(VIRTUAL_FOLDERS, True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/mailbox.py` at line 595, Replace the dict comprehension used to initialize subscriptions with a fromkeys call: update the initialization of self._subscribed (currently using {name: True for name in VIRTUAL_FOLDERS}) to use dict.fromkeys(VIRTUAL_FOLDERS, True) so the mapping of VIRTUAL_FOLDERS to True is created more succinctly.
397-425: Consider API call order for consistency.The
move()method pops the source message from local cache (line 401) before the API flag changes succeed (lines 414-425). If API calls fail, the message disappears locally until the next refresh, which could be confusing.A safer order would be:
- Call API to update flags
- Only remove from source cache on success
This is a minor concern since the message isn't lost (just temporarily misplaced locally), and the current behavior may be acceptable given the "best effort" approach indicated by the warning logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/src/mailbox.py` around lines 397 - 425, In move(), don't remove the message from the local cache before the API calls succeed: first compute clear_flags and set_flags using self._folder_flags_for(self._folder) and self._folder_flags_for(destination._folder), then perform the await self._api_client.change_flag(...) loop for all_flag_changes; only after those calls succeed acquire async with self.messages_lock.write_lock(): and pop the message from self._messages and call self._updated.set(); ensure you still reference source.api_message_id for the API calls and handle exceptions as before (log warnings) but only remove the message from the cache on successful persistence.src/client-bridge/tests/conftest.py (2)
59-66: Session-scoped mutable state may cause test pollution.The
MockMessagesAPIinstance stores mutable state (channels,mailbox_threads,thread_messages,message_emls,submitted_messages,message_flag_updates) and is used as a session-scoped fixture. Tests that mutate this state (e.g., adding messages, clearingmessage_flag_updates) can affect subsequent tests. Consider either:
- Adding reset methods called in fixtures, or
- Using function-scoped fixtures for tests that mutate state
This is especially relevant given the
mock_api.message_flag_updates.clear()calls intest_imap_operations.py.♻️ Optional: Add reset method
def add_message(self, thread_id: str, message: dict, eml: bytes | None = None): """Add a message to a thread with optional EML content.""" self.thread_messages.setdefault(thread_id, []).append(message) if eml is not None: self.message_emls[message["id"]] = eml + def reset_tracking(self): + """Reset mutable tracking state between tests.""" + self.submitted_messages.clear() + self.message_flag_updates.clear() + def start(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/conftest.py` around lines 59 - 66, The MockMessagesAPI holds session-scoped mutable state (channels, mailbox_threads, thread_messages, message_emls, submitted_messages, message_flag_updates) which can leak between tests; add a reset method on MockMessagesAPI (e.g., def reset(self): ...) that clears/reinitializes those attributes and call it from fixtures or tests that need a clean state, or change the fixture providing MockMessagesAPI to function scope so each test gets a fresh instance; update tests that currently clear state manually (like test_imap_operations.py) to call mock_api.reset() or rely on the function-scoped fixture.
304-310: Potential resource leak inIMAPServer.stop()if loop tasks raise during cancellation.The
stop()method cancels tasks and stops the loop, but iftask.cancel()or_loop.stop()raises, the thread join may hang or leave resources dangling. Consider wrapping in try/except for robustness.♻️ Defensive cleanup
def stop(self): if self._loop: - for task in asyncio.all_tasks(self._loop): - self._loop.call_soon_threadsafe(task.cancel) - self._loop.call_soon_threadsafe(self._loop.stop) + try: + for task in asyncio.all_tasks(self._loop): + self._loop.call_soon_threadsafe(task.cancel) + self._loop.call_soon_threadsafe(self._loop.stop) + except Exception: + pass # Best-effort cleanup if self._thread: self._thread.join(timeout=5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/conftest.py` around lines 304 - 310, IMAPServer.stop currently calls asyncio.all_tasks(self._loop), cancels tasks and stops the loop but may fail to join the thread if task cancellation or loop.stop raises; wrap the calls to self._loop.call_soon_threadsafe(task.cancel) and self._loop.call_soon_threadsafe(self._loop.stop) in try/except to catch and log/suppress exceptions, and ensure the thread join (self._thread.join(timeout=5)) runs from a finally block so it always executes even if cancellation/stop raises; reference the stop method, self._loop, self._thread, and asyncio.all_tasks to locate and update the cleanup control flow.Makefile (1)
251-257: Consider addingruff checkto client-bridge linting for consistency.The
lint-client-bridgetarget only runsruff format, whilelint-backruns bothruff format,ruff check, andpylint. This means static analysis for client-bridge is less comprehensive. Consider aligning with backend linting for better coverage.♻️ Optional enhancement
lint-client-bridge: ## lint client-bridge python sources (with auto-fix) $(COMPOSE_RUN) --rm -e EXEC_CMD_ONLY=true client-bridge-test ruff format . + $(COMPOSE_RUN) --rm -e EXEC_CMD_ONLY=true client-bridge-test ruff check . --fix .PHONY: lint-client-bridge lint-check-client-bridge: ## lint client-bridge python sources in check mode (no auto-fix) $(COMPOSE_RUN) --rm -e EXEC_CMD_ONLY=true client-bridge-test ruff format --check . + $(COMPOSE_RUN) --rm -e EXEC_CMD_ONLY=true client-bridge-test ruff check . .PHONY: lint-check-client-bridge🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 251 - 257, The client-bridge Makefile targets currently only run ruff format; update the targets to include ruff check for parity with the backend: modify the lint-client-bridge target (and/or lint-check-client-bridge) so it runs both ruff format . and ruff check . (and for the check-mode target run ruff format --check . followed by ruff check .) Refer to the existing lint-back behavior as the pattern to match and ensure the target names lint-client-bridge and lint-check-client-bridge are updated accordingly.src/client-bridge/tests/test_imap_operations.py (1)
368-402: Good isolation pattern: uses fresh connection for destructive test.This test correctly creates a fresh IMAP connection instead of using the shared
imap_clientfixture. However, it still mutates the Trash folder in the session-scopedtest_channel. Consider documenting test execution order dependencies or using isolated fixtures for destructive operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client-bridge/tests/test_imap_operations.py` around lines 368 - 402, test_close_expunges_deleted mutates the shared session-scoped test_channel Trash and can affect other tests; change the test to operate on an isolated mailbox/folder created for this test (use imap_server or a helper to create a temporary mailbox/folder or clone test_channel data), perform the delete/close on that per-test mailbox using the local client variable, and ensure proper cleanup in the finally block (delete the temporary mailbox or restore messages) so no shared state in test_channel is modified.
🤖 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 1559-1581: In update(), prevent changing a channel's type on PATCH
by checking if instance is present and "type" is in validated_data and
validated_data["type"] != instance.type, and if so raise
serializers.ValidationError (e.g., {"type": "Cannot change channel type on
existing channels"}); modify the update method (the same one that currently
handles channel_type, settings, and encrypted_settings) to perform this check
before any provisioning logic so converting a non-client-bridge to client-bridge
is rejected and encrypted_settings/password provisioning is not bypassed.
- Around line 1639-1657: The validation only enforces the FEATURE_CLIENTBRIDGE
flag inside the nested mailbox branch, so calls that pass mailbox/maildomain
directly can still create a 'client-bridge' channel when the feature is
disabled; modify the serializer validation to always check attrs.get("type") ==
"client-bridge" against settings.FEATURE_CLIENTBRIDGE regardless of
self.context.get("mailbox") branch (i.e., move or duplicate the client-bridge
guard so any code path that processes attrs and reads attrs.get("type") will
raise serializers.ValidationError({"type": "Client bridge feature is not
enabled."}) when FEATURE_CLIENTBRIDGE is false), while keeping the existing
allowed-types logic that uses settings.FEATURE_MAILBOX_ADMIN_CHANNELS.
In `@src/backend/e2e/management/commands/e2e_demo.py`:
- Around line 422-441: The get_or_create call for models.Channel (type
"client-bridge") only sets encrypted_settings on creation; update the existing
channel's encrypted_settings to the known E2E credential when created is False:
after the get_or_create that returns _channel and created, if not created ensure
_channel.encrypted_settings is a dict, set
_channel.encrypted_settings["password"] = CLIENTBRIDGE_APP_PASSWORD and call
_channel.save() (or use update) so rerunning the command resets rotated/old
credentials for the given mailbox.
- Around line 453-525: The helper _create_imap_test_messages currently always
creates two new threads ("IMAP unread test" and "IMAP read test"), causing
duplicates on repeated runs; make it idempotent by first checking for existing
threads for this mailbox with those subjects (e.g. via
models.Thread.objects.filter(subject="IMAP unread test",
threadaccess__mailbox=mailbox) and similarly for "IMAP read test") and skip
creation if present (or update existing ThreadAccess.read_at, Message
blob/sent_at as needed) instead of blindly creating new Thread, ThreadAccess,
Message and blobs; ensure you still call thread.update_stats() for any created
or updated threads.
In `@src/e2e/compose.yaml`:
- Around line 103-112: Add a proper healthcheck for the IMAP listener and gate
the e2e-runner on service health: in the compose service `client-bridge` add a
`healthcheck` block that probes the IMAP port (e.g., curl/nc or a docker
healthcheck script that checks port 143) so the container reports "healthy" only
when IMAP is accepting connections, then change any `depends_on` entries that
reference `client-bridge` (including the `e2e-runner` dependency occurrences)
from the current startup condition to use `condition: service_healthy` (or the
Compose v3 `depends_on: { client-bridge: condition: service_healthy }`
equivalent for your compose version); apply the same change to the other
`client-bridge` dependency instance noted in the review.
In `@src/e2e/src/__tests__/client-bridge.spec.ts`:
- Around line 128-147: The test currently only verifies the thread exists
(targetThread) after messageFlagsAdd and never asserts the read/unread state;
update the test to fetch the thread with the mailbox context (use
getMailboxEmail("user","chromium") as the mailbox_id when calling the threads
API via page.request.get or by loading the page URL with that mailbox context)
and assert the thread's read marker (e.g., targetThread.is_unread === false or
equivalent read_at presence) instead of only checking targetThread truthiness so
the IMAP→API flag sync is actually validated.
- Around line 207-217: The fetch loop on client2 may never find targetSubject so
the test can pass silently; modify the code around client2.getMailboxLock and
client2.fetch("1:*", { flags: true, envelope: true }) to track a boolean (e.g.,
foundTarget) set to true when msg.envelope.subject === targetSubject, then after
the for-await loop assert that foundTarget is true and that msg.flags (or the
recorded flags) does not contain "\\Seen" (i.e., expect(foundTarget).toBe(true)
and expect(seenFlag).toBe(false) or similar); ensure you reference client2,
getMailboxLock, fetch and targetSubject when adding the flag and post-loop
assertions so the test fails if the message was never returned.
- Around line 20-34: The IMAP user and Keycloak sign-in must be parameterized by
browserName to isolate mailbox state: update createImapClient to accept a
browserName parameter and use getMailboxEmail("user", browserName) to build the
IMAP_USER inside createImapClient, then update all callers (the Keycloak sign-in
call and the test function signature that invokes createImapClient) to pass
browserName through; also modify the test signature to accept browserName.
Additionally, add the assertion expect(targetThread.is_unread).toBe(false) after
verifying the thread to confirm read state synced, and add
expect(messageFound).toBe(true) after the message loop to ensure the target
message was found.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 32-35: The formSchema uses Zod's deprecated "message" option;
update the schema to use the Zod 4 "error" parameter instead: replace the name
field's z.string().min(..., { message: ... }) with z.string().min(1, { error:
t("Name is required.") }) and likewise ensure any other validation options
(e.g., enum or other refinements) use { error: ... } rather than { message: ...
} so that the formSchema and its validations (referenced in formSchema and the
name validation) are compatible with Zod 4.
---
Duplicate comments:
In `@env.d/development/backend.defaults`:
- Around line 103-106: The development defaults enable FEATURE_CLIENTBRIDGE but
do not set CLIENTBRIDGE_API_SECRET, which causes post_setup() validation to
fail; fix by either disabling the feature by setting FEATURE_CLIENTBRIDGE=False
in this file or adding the same CLIENTBRIDGE_API_SECRET value used in
env.d/development/client-bridge.defaults so post_setup() sees a valid secret;
update the CLIENTBRIDGE_PUBLIC_CONFIG if needed to match the chosen change and
ensure FEATURE_MAILBOX_ADMIN_CHANNELS remains consistent with the new default.
In `@src/backend/messages/settings.py`:
- Around line 1087-1091: When FEATURE_CLIENTBRIDGE is enabled, extend the
existing startup guard (the block referencing cls.FEATURE_CLIENTBRIDGE and
cls.CLIENTBRIDGE_API_SECRET) to also validate cls.CLIENTBRIDGE_PUBLIC_CONFIG:
ensure it is present, is a mapping/dict, and contains the required public keys
(e.g., the IMAP/SMTP entries the UI expects) and that those entries include
required subkeys before allowing startup; if validation fails, raise a
ValueError with a clear message (similar to the CLIENTBRIDGE_API_SECRET check)
so the app fails fast rather than letting the UI error later.
- Around line 568-572: Remove
"core.authentication.client_bridge.ClientBridgeChannelAuthentication" from the
global DEFAULT_AUTHENTICATION_CLASSES tuple in settings (leave
OIDCAuthentication and SessionAuthentication) and instead add
ClientBridgeChannelAuthentication only to the bridge-facing DRF views/viewsets
by setting authentication_classes on those specific View or ViewSet classes
(e.g., the bridge API view handlers) so only bridge endpoints accept
X-Channel-Token; ensure the global DEFAULT_AUTHENTICATION_CLASSES symbol remains
unchanged except for that removal and that bridge views explicitly import and
include core.authentication.client_bridge.ClientBridgeChannelAuthentication in
their authentication_classes.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 296-303: The code currently uses config.CLIENTBRIDGE_PUBLIC_CONFIG
(bridgeConfig) directly and silently falls back to window.location.hostname for
imapHost/smtpHost, which yields wrong connection details when bridgeConfig is
undefined; update the component that defines bridgeConfig, imapHost, imapPort,
imapSecurity, smtpHost, smtpPort, smtpSecurity to check whether bridgeConfig
exists and if not either render the connection details section disabled (make
inputs readOnly/disabled and prevent submission) or render a visible warning
banner informing the user that CLIENTBRIDGE_PUBLIC_CONFIG is missing and
connection details cannot be auto-filled; ensure this check gating is applied
wherever imapHost/smtpHost are used so the UI does not display or rely on the
hostname fallback when bridgeConfig is undefined.
---
Nitpick comments:
In `@Makefile`:
- Around line 251-257: The client-bridge Makefile targets currently only run
ruff format; update the targets to include ruff check for parity with the
backend: modify the lint-client-bridge target (and/or lint-check-client-bridge)
so it runs both ruff format . and ruff check . (and for the check-mode target
run ruff format --check . followed by ruff check .) Refer to the existing
lint-back behavior as the pattern to match and ensure the target names
lint-client-bridge and lint-check-client-bridge are updated accordingly.
In `@src/client-bridge/src/mailbox.py`:
- Line 595: Replace the dict comprehension used to initialize subscriptions with
a fromkeys call: update the initialization of self._subscribed (currently using
{name: True for name in VIRTUAL_FOLDERS}) to use dict.fromkeys(VIRTUAL_FOLDERS,
True) so the mapping of VIRTUAL_FOLDERS to True is created more succinctly.
- Around line 397-425: In move(), don't remove the message from the local cache
before the API calls succeed: first compute clear_flags and set_flags using
self._folder_flags_for(self._folder) and
self._folder_flags_for(destination._folder), then perform the await
self._api_client.change_flag(...) loop for all_flag_changes; only after those
calls succeed acquire async with self.messages_lock.write_lock(): and pop the
message from self._messages and call self._updated.set(); ensure you still
reference source.api_message_id for the API calls and handle exceptions as
before (log warnings) but only remove the message from the cache on successful
persistence.
In `@src/client-bridge/tests/conftest.py`:
- Around line 59-66: The MockMessagesAPI holds session-scoped mutable state
(channels, mailbox_threads, thread_messages, message_emls, submitted_messages,
message_flag_updates) which can leak between tests; add a reset method on
MockMessagesAPI (e.g., def reset(self): ...) that clears/reinitializes those
attributes and call it from fixtures or tests that need a clean state, or change
the fixture providing MockMessagesAPI to function scope so each test gets a
fresh instance; update tests that currently clear state manually (like
test_imap_operations.py) to call mock_api.reset() or rely on the function-scoped
fixture.
- Around line 304-310: IMAPServer.stop currently calls
asyncio.all_tasks(self._loop), cancels tasks and stops the loop but may fail to
join the thread if task cancellation or loop.stop raises; wrap the calls to
self._loop.call_soon_threadsafe(task.cancel) and
self._loop.call_soon_threadsafe(self._loop.stop) in try/except to catch and
log/suppress exceptions, and ensure the thread join
(self._thread.join(timeout=5)) runs from a finally block so it always executes
even if cancellation/stop raises; reference the stop method, self._loop,
self._thread, and asyncio.all_tasks to locate and update the cleanup control
flow.
In `@src/client-bridge/tests/test_imap_operations.py`:
- Around line 368-402: test_close_expunges_deleted mutates the shared
session-scoped test_channel Trash and can affect other tests; change the test to
operate on an isolated mailbox/folder created for this test (use imap_server or
a helper to create a temporary mailbox/folder or clone test_channel data),
perform the delete/close on that per-test mailbox using the local client
variable, and ensure proper cleanup in the finally block (delete the temporary
mailbox or restore messages) so no shared state in test_channel is modified.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`:
- Around line 68-73: The code uses non-null assertions on selectedMailbox (seen
in invalidateChannels and other places) which can crash at runtime; add an early
guard in the component to return null (or a fallback) when selectedMailbox is
falsy, then remove all `!` assertions and use selectedMailbox.id directly
(update functions like invalidateChannels and calls to
getMailboxesChannelsListUrl to reference selectedMailbox.id safely).
- Around line 178-183: Extract the duplicated role options array into a single
helper and use it in both places: create a constant function like
getRoleOptions(t) that returns the four option objects, place it near the top of
the file, and replace the inline arrays in the two RhfSelect usages (the ones in
the edit form and the create form in client-bridge-integration-form.tsx) with
options={getRoleOptions(t)} so both forms reuse the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62ef5744-1229-4249-a5dd-ef9a0302e19d
📒 Files selected for processing (24)
Makefilecompose.yamlenv.d/development/backend.defaultsenv.d/development/backend.e2eenv.d/development/client-bridge.e2esrc/backend/.pylintrcsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/urls.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/settings.pysrc/client-bridge/src/api/client.pysrc/client-bridge/src/mailbox.pysrc/client-bridge/tests/conftest.pysrc/client-bridge/tests/test_imap_messages.pysrc/client-bridge/tests/test_imap_operations.pysrc/e2e/bin/backend-manage.shsrc/e2e/compose.yamlsrc/e2e/package.jsonsrc/e2e/src/__tests__/client-bridge.spec.tssrc/e2e/src/constants.tssrc/e2e/src/utils.tssrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/_index.scsssrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx
✅ Files skipped from review due to trivial changes (2)
- src/e2e/src/utils.ts
- src/e2e/bin/backend-manage.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/core/api/viewsets/config.py
- src/backend/.pylintrc
- src/backend/core/urls.py
Comment on lines
+1559
to
+1581
| def update(self, instance, validated_data): | ||
| channel_type = validated_data.get("type") or ( | ||
| instance.type if instance else None | ||
| ) | ||
| if channel_type == "client-bridge": | ||
| # Strip any user-supplied password — passwords can only be | ||
| # changed via the dedicated rotate-password endpoint. | ||
| settings_data = validated_data.get("settings") or {} | ||
| settings_data.pop("password", None) | ||
| if "role" in settings_data: | ||
| if settings_data["role"] not in enums.CLIENT_BRIDGE_ROLES: | ||
| raise serializers.ValidationError( | ||
| { | ||
| "settings": { | ||
| "role": f"Invalid role. Must be one of: {', '.join(enums.CLIENT_BRIDGE_ROLES)}" | ||
| } | ||
| } | ||
| ) | ||
| # Prevent encrypted_settings from being overwritten | ||
| validated_data.pop("encrypted_settings", None) | ||
| else: | ||
| validated_data = self._move_sensitive_settings(validated_data) | ||
| return super().update(instance, validated_data) |
There was a problem hiding this comment.
Reject type changes on existing channels.
A PATCH can currently turn a non-client-bridge channel into client-bridge, but this path never provisions encrypted_settings["password"]. That leaves the channel in a broken state with no app password for IMAP/SMTP auth.
Proposed fix
def update(self, instance, validated_data):
+ if "type" in validated_data and validated_data["type"] != instance.type:
+ raise serializers.ValidationError(
+ {"type": "Channel type cannot be changed after creation."}
+ )
channel_type = validated_data.get("type") or (
instance.type if instance else None
)
if channel_type == "client-bridge":🤖 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 1559 - 1581, In update(),
prevent changing a channel's type on PATCH by checking if instance is present
and "type" is in validated_data and validated_data["type"] != instance.type, and
if so raise serializers.ValidationError (e.g., {"type": "Cannot change channel
type on existing channels"}); modify the update method (the same one that
currently handles channel_type, settings, and encrypted_settings) to perform
this check before any provisioning logic so converting a non-client-bridge to
client-bridge is rejected and encrypted_settings/password provisioning is not
bypassed.
Comment on lines
1639
to
1657
| if self.context.get("mailbox"): | ||
| channel_type = attrs.get("type") | ||
| if channel_type: | ||
| allowed_types = settings.FEATURE_MAILBOX_ADMIN_CHANNELS | ||
| allowed_types = list(settings.FEATURE_MAILBOX_ADMIN_CHANNELS) | ||
| if channel_type not in allowed_types: | ||
| raise serializers.ValidationError( | ||
| { | ||
| "type": f"Channel type '{channel_type}' is not authorized. " | ||
| f"Allowed types: {', '.join(allowed_types)}" | ||
| } | ||
| ) | ||
| if ( | ||
| channel_type == "client-bridge" | ||
| and not settings.FEATURE_CLIENTBRIDGE | ||
| ): | ||
| raise serializers.ValidationError( | ||
| {"type": "Client bridge feature is not enabled."} | ||
| ) | ||
| return attrs |
There was a problem hiding this comment.
Apply the client-bridge feature flag to every serializer entry point.
The FEATURE_CLIENTBRIDGE check only runs in the nested mailbox branch. Any non-nested create/update path that supplies mailbox or maildomain directly can still persist a client-bridge channel while the feature is disabled.
Proposed fix
def validate(self, attrs):
"""Validate channel data.
@@
+ channel_type = attrs.get("type") or (
+ self.instance.type if self.instance else None
+ )
+ if channel_type == "client-bridge" and not settings.FEATURE_CLIENTBRIDGE:
+ raise serializers.ValidationError(
+ {"type": "Client bridge feature is not enabled."}
+ )
+
# If we have a mailbox in context (from ChannelViewSet), validate channel type
# and skip mailbox/maildomain validation.
# This allows Django admin to create any channel type.
if self.context.get("mailbox"):
- channel_type = attrs.get("type")
if channel_type:
allowed_types = list(settings.FEATURE_MAILBOX_ADMIN_CHANNELS)
if channel_type not in allowed_types:
raise serializers.ValidationError(
{
@@
- if (
- channel_type == "client-bridge"
- and not settings.FEATURE_CLIENTBRIDGE
- ):
- raise serializers.ValidationError(
- {"type": "Client bridge feature is not enabled."}
- )
return attrs🤖 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 1639 - 1657, The validation
only enforces the FEATURE_CLIENTBRIDGE flag inside the nested mailbox branch, so
calls that pass mailbox/maildomain directly can still create a 'client-bridge'
channel when the feature is disabled; modify the serializer validation to always
check attrs.get("type") == "client-bridge" against settings.FEATURE_CLIENTBRIDGE
regardless of self.context.get("mailbox") branch (i.e., move or duplicate the
client-bridge guard so any code path that processes attrs and reads
attrs.get("type") will raise serializers.ValidationError({"type": "Client bridge
feature is not enabled."}) when FEATURE_CLIENTBRIDGE is false), while keeping
the existing allowed-types logic that uses
settings.FEATURE_MAILBOX_ADMIN_CHANNELS.
Comment on lines
+422
to
+441
| _channel, created = models.Channel.objects.get_or_create( | ||
| mailbox=mailbox, | ||
| type="client-bridge", | ||
| defaults={ | ||
| "name": f"E2E client-bridge ({mailbox})", | ||
| "user": access.user, | ||
| "settings": {"role": "sender"}, | ||
| "encrypted_settings": {"password": CLIENTBRIDGE_APP_PASSWORD}, | ||
| }, | ||
| ) | ||
| if created: | ||
| self.stdout.write( | ||
| self.style.SUCCESS(f" Created client-bridge channel for {mailbox}") | ||
| ) | ||
| else: | ||
| self.stdout.write( | ||
| self.style.SUCCESS( | ||
| f" Client-bridge channel already exists for {mailbox}" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Reset existing client-bridge channels to the known E2E credential.
On Line 422, get_or_create() only applies the E2E password on first creation. If this command is rerun after a channel already exists or its password was rotated, the helper reports success but leaves the old credential in place, so the advertised E2E login stops working.
Proposed fix
- _channel, created = models.Channel.objects.get_or_create(
- mailbox=mailbox,
- type="client-bridge",
- defaults={
- "name": f"E2E client-bridge ({mailbox})",
- "user": access.user,
- "settings": {"role": "sender"},
- "encrypted_settings": {"password": CLIENTBRIDGE_APP_PASSWORD},
- },
- )
+ _channel, created = models.Channel.objects.update_or_create(
+ mailbox=mailbox,
+ type="client-bridge",
+ defaults={
+ "name": f"E2E client-bridge ({mailbox})",
+ "user": access.user,
+ "settings": {"role": "sender"},
+ "encrypted_settings": {"password": CLIENTBRIDGE_APP_PASSWORD},
+ },
+ )📝 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
| _channel, created = models.Channel.objects.get_or_create( | |
| mailbox=mailbox, | |
| type="client-bridge", | |
| defaults={ | |
| "name": f"E2E client-bridge ({mailbox})", | |
| "user": access.user, | |
| "settings": {"role": "sender"}, | |
| "encrypted_settings": {"password": CLIENTBRIDGE_APP_PASSWORD}, | |
| }, | |
| ) | |
| if created: | |
| self.stdout.write( | |
| self.style.SUCCESS(f" Created client-bridge channel for {mailbox}") | |
| ) | |
| else: | |
| self.stdout.write( | |
| self.style.SUCCESS( | |
| f" Client-bridge channel already exists for {mailbox}" | |
| ) | |
| ) | |
| _channel, created = models.Channel.objects.update_or_create( | |
| mailbox=mailbox, | |
| type="client-bridge", | |
| defaults={ | |
| "name": f"E2E client-bridge ({mailbox})", | |
| "user": access.user, | |
| "settings": {"role": "sender"}, | |
| "encrypted_settings": {"password": CLIENTBRIDGE_APP_PASSWORD}, | |
| }, | |
| ) | |
| if created: | |
| self.stdout.write( | |
| self.style.SUCCESS(f" Created client-bridge channel for {mailbox}") | |
| ) | |
| else: | |
| self.stdout.write( | |
| self.style.SUCCESS( | |
| f" Client-bridge channel already exists for {mailbox}" | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/e2e/management/commands/e2e_demo.py` around lines 422 - 441, The
get_or_create call for models.Channel (type "client-bridge") only sets
encrypted_settings on creation; update the existing channel's encrypted_settings
to the known E2E credential when created is False: after the get_or_create that
returns _channel and created, if not created ensure _channel.encrypted_settings
is a dict, set _channel.encrypted_settings["password"] =
CLIENTBRIDGE_APP_PASSWORD and call _channel.save() (or use update) so rerunning
the command resets rotated/old credentials for the given mailbox.
Comment on lines
+453
to
+525
| def _create_imap_test_messages(self, mailbox, domain): | ||
| """Create messages for IMAP read/unread e2e testing. | ||
|
|
||
| Creates two threads with proper EML blobs so the client-bridge | ||
| can serve envelope data (subject, from, date) over IMAP. | ||
| """ | ||
| sender_email = f"imap-sender@{domain.name}" | ||
| recipient_email = str(mailbox) | ||
| sender_contact, _ = models.Contact.objects.get_or_create( | ||
| email=sender_email, | ||
| mailbox=mailbox, | ||
| defaults={"name": "IMAP Test Sender"}, | ||
| ) | ||
|
|
||
| # Thread 1: an unread message | ||
| now = timezone.now() | ||
| thread1 = models.Thread.objects.create(subject="IMAP unread test") | ||
| models.ThreadAccess.objects.create( | ||
| thread=thread1, | ||
| mailbox=mailbox, | ||
| role=ThreadAccessRoleChoices.EDITOR, | ||
| read_at=None, # No read_at → all messages unread | ||
| ) | ||
| eml1 = self._make_eml( | ||
| "IMAP unread test", | ||
| sender_email, | ||
| recipient_email, | ||
| "This message should appear as unread in IMAP.", | ||
| now, | ||
| ) | ||
| blob1 = mailbox.create_blob(content=eml1, content_type="message/rfc822") | ||
| models.Message.objects.create( | ||
| thread=thread1, | ||
| sender=sender_contact, | ||
| subject="IMAP unread test", | ||
| is_sender=False, | ||
| is_draft=False, | ||
| sent_at=now, | ||
| blob=blob1, | ||
| ) | ||
| thread1.update_stats() | ||
|
|
||
| # Thread 2: a read message | ||
| sent_at2 = now - timezone.timedelta(minutes=5) | ||
| thread2 = models.Thread.objects.create(subject="IMAP read test") | ||
| models.ThreadAccess.objects.create( | ||
| thread=thread2, | ||
| mailbox=mailbox, | ||
| role=ThreadAccessRoleChoices.EDITOR, | ||
| read_at=now, # read_at set → messages before this are read | ||
| ) | ||
| eml2 = self._make_eml( | ||
| "IMAP read test", | ||
| sender_email, | ||
| recipient_email, | ||
| "This message should appear as read in IMAP.", | ||
| sent_at2, | ||
| ) | ||
| blob2 = mailbox.create_blob(content=eml2, content_type="message/rfc822") | ||
| models.Message.objects.create( | ||
| thread=thread2, | ||
| sender=sender_contact, | ||
| subject="IMAP read test", | ||
| is_sender=False, | ||
| is_draft=False, | ||
| sent_at=sent_at2, | ||
| blob=blob2, | ||
| ) | ||
| thread2.update_stats() | ||
|
|
||
| self.stdout.write( | ||
| self.style.SUCCESS(f" Created IMAP test messages for {mailbox}") | ||
| ) |
There was a problem hiding this comment.
Make the IMAP fixture creation idempotent.
This helper always inserts two more threads. A second run of the bootstrap command will duplicate the IMAP unread test / IMAP read test fixtures in the same mailbox, which is likely to make E2E assertions on message counts or unread state flaky.
Proposed fix
def _create_imap_test_messages(self, mailbox, domain):
"""Create messages for IMAP read/unread e2e testing.
Creates two threads with proper EML blobs so the client-bridge
can serve envelope data (subject, from, date) over IMAP.
"""
+ imap_subjects = ["IMAP unread test", "IMAP read test"]
+ existing_threads = models.Thread.objects.filter(
+ subject__in=imap_subjects,
+ accesses__mailbox=mailbox,
+ )
+ deleted_count = existing_threads.count()
+ if deleted_count > 0:
+ existing_threads.delete()
+ self.stdout.write(
+ self.style.WARNING(
+ f" ⚠ Deleted {deleted_count} existing IMAP test thread(s)"
+ )
+ )
+
sender_email = f"imap-sender@{domain.name}"
recipient_email = str(mailbox)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/e2e/management/commands/e2e_demo.py` around lines 453 - 525, The
helper _create_imap_test_messages currently always creates two new threads
("IMAP unread test" and "IMAP read test"), causing duplicates on repeated runs;
make it idempotent by first checking for existing threads for this mailbox with
those subjects (e.g. via models.Thread.objects.filter(subject="IMAP unread
test", threadaccess__mailbox=mailbox) and similarly for "IMAP read test") and
skip creation if present (or update existing ThreadAccess.read_at, Message
blob/sent_at as needed) instead of blindly creating new Thread, ThreadAccess,
Message and blobs; ensure you still call thread.update_stats() for any created
or updated threads.
Comment on lines
+103
to
+112
| client-bridge: | ||
| extends: | ||
| file: ../../compose.yaml | ||
| service: client-bridge | ||
| env_file: | ||
| - ../../env.d/development/client-bridge.defaults | ||
| - ../../env.d/development/client-bridge.e2e | ||
| ports: !reset [] | ||
| depends_on: !override | ||
| - backend |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/e2e && head -n 200 compose.yaml | cat -nRepository: suitenumerique/messages
Length of output: 6319
🏁 Script executed:
cat -n ../../compose.yaml | grep -A 20 "client-bridge:"Repository: suitenumerique/messages
Length of output: 117
🏁 Script executed:
find . -name "compose.yaml" -type fRepository: suitenumerique/messages
Length of output: 104
🏁 Script executed:
cat -n compose.yaml | grep -A 20 "client-bridge:"Repository: suitenumerique/messages
Length of output: 1388
🏁 Script executed:
find src/e2e/src -name "*.spec.ts" -o -name "*.test.ts" | head -10Repository: suitenumerique/messages
Length of output: 363
🏁 Script executed:
head -100 src/e2e/src/__tests__/client-bridge.spec.tsRepository: suitenumerique/messages
Length of output: 3202
🏁 Script executed:
cat -n src/e2e/playwright.config.ts | head -100Repository: suitenumerique/messages
Length of output: 2696
🏁 Script executed:
grep -r "retries\|retry\|wait\|timeout" src/e2e/src/utils-test.ts src/e2e/src/__tests__/client-bridge.spec.ts | head -20Repository: suitenumerique/messages
Length of output: 1832
🏁 Script executed:
cat -n src/e2e/src/__tests__/client-bridge.spec.ts | grep -A 5 "async function createImapClient"Repository: suitenumerique/messages
Length of output: 294
Add healthcheck to client-bridge and gate e2e-runner on service_healthy.
The IMAP tests connect immediately via await client.connect() without retry logic. service_started only guarantees the container has started, not that port 143 is listening. This creates a race condition that will cause intermittent failures. Add a healthcheck on the IMAP port to client-bridge and change the e2e-runner dependency to service_healthy.
Also applies to lines 190-191.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e/compose.yaml` around lines 103 - 112, Add a proper healthcheck for
the IMAP listener and gate the e2e-runner on service health: in the compose
service `client-bridge` add a `healthcheck` block that probes the IMAP port
(e.g., curl/nc or a docker healthcheck script that checks port 143) so the
container reports "healthy" only when IMAP is accepting connections, then change
any `depends_on` entries that reference `client-bridge` (including the
`e2e-runner` dependency occurrences) from the current startup condition to use
`condition: service_healthy` (or the Compose v3 `depends_on: { client-bridge:
condition: service_healthy }` equivalent for your compose version); apply the
same change to the other `client-bridge` dependency instance noted in the
review.
Comment on lines
+20
to
+34
| const IMAP_USER = "user.e2e.chromium@example.local"; | ||
|
|
||
| async function createImapClient(): Promise<ImapFlow> { | ||
| const client = new ImapFlow({ | ||
| host: CLIENTBRIDGE_IMAP_HOST, | ||
| port: CLIENTBRIDGE_IMAP_PORT, | ||
| secure: false, | ||
| auth: { | ||
| user: IMAP_USER, | ||
| pass: CLIENTBRIDGE_APP_PASSWORD, | ||
| }, | ||
| logger: false, | ||
| }); | ||
| await client.connect(); | ||
| return client; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/e2e/src/__tests__/client-bridge.spec.ts | head -200Repository: suitenumerique/messages
Length of output: 8317
🏁 Script executed:
cat -n src/e2e/src/__tests__/client-bridge.spec.ts | sed -n '150,250p'Repository: suitenumerique/messages
Length of output: 3106
🏁 Script executed:
rg -n "getMailboxEmail" src/e2e/src/ -A 5Repository: suitenumerique/messages
Length of output: 9551
🏁 Script executed:
rg -n "should sync read state from IMAP to webmail API" src/e2e/src/__tests__/client-bridge.spec.ts -A 50 | head -60Repository: suitenumerique/messages
Length of output: 2311
🏁 Script executed:
cat -n src/e2e/src/__tests__/client-bridge.spec.ts | sed -n '200,224p'Repository: suitenumerique/messages
Length of output: 937
Parameterize test account from browserName to isolate mailbox state across parallel runs.
Both IMAP and Keycloak logins are hardcoded to user.e2e.chromium. When tests run in parallel or retry against existing state, they mutate the same mailbox, making read/unread assertions nondeterministic.
Three instances need updating:
-
Line 20–34: Pass
browserNametocreateImapClient()and usegetMailboxEmail("user", browserName)for the IMAP user.-const IMAP_USER = "user.e2e.chromium@example.local"; - -async function createImapClient(): Promise<ImapFlow> { +async function createImapClient(browserName: string): Promise<ImapFlow> { const client = new ImapFlow({ host: CLIENTBRIDGE_IMAP_HOST, port: CLIENTBRIDGE_IMAP_PORT, secure: false, auth: { - user: IMAP_USER, + user: getMailboxEmail("user", browserName), pass: CLIENTBRIDGE_APP_PASSWORD, }, logger: false, }); await client.connect(); return client; }
-
Line 100–104: Use
getMailboxEmail("user", browserName)in Keycloak sign-in and passbrowserNametocreateImapClient().-await signInKeycloakIfNeeded({ page, username: `user.e2e.chromium` }); -const client = await createImapClient(); +await signInKeycloakIfNeeded({ page, username: getMailboxEmail("user", browserName) }); +const client = await createImapClient(browserName);
-
Line 150: Add
browserNameparameter to test signature and update both Keycloak and IMAP calls.-test("should sync read state from webmail API to IMAP", async ({ page }) => { - await signInKeycloakIfNeeded({ page, username: `user.e2e.chromium` }); +test("should sync read state from webmail API to IMAP", async ({ page, browserName }) => { + await signInKeycloakIfNeeded({ page, username: getMailboxEmail("user", browserName) }); - const client1 = await createImapClient(); + const client1 = await createImapClient(browserName); - const client2 = await createImapClient(); + const client2 = await createImapClient(browserName);
Bonus issues:
- Line 147: Test only verifies thread existence; add
expect(targetThread.is_unread).toBe(false)to confirm read state was synced. - Lines 212–217: Add
expect(messageFound).toBe(true)after the loop to ensure the target message was located and verified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e/src/__tests__/client-bridge.spec.ts` around lines 20 - 34, The IMAP
user and Keycloak sign-in must be parameterized by browserName to isolate
mailbox state: update createImapClient to accept a browserName parameter and use
getMailboxEmail("user", browserName) to build the IMAP_USER inside
createImapClient, then update all callers (the Keycloak sign-in call and the
test function signature that invokes createImapClient) to pass browserName
through; also modify the test signature to accept browserName. Additionally, add
the assertion expect(targetThread.is_unread).toBe(false) after verifying the
thread to confirm read state synced, and add expect(messageFound).toBe(true)
after the message loop to ensure the target message was found.
Comment on lines
+128
to
+147
| // Verify via the API that the thread is now read | ||
| // The API returns is_unread on messages when mailbox_id is provided | ||
| const mailboxEmail = getMailboxEmail("user", "chromium"); | ||
|
|
||
| // Get threads to find the one with "IMAP unread test" | ||
| const threadsResp = await page.request.get(`${API_URL}/api/v1.0/threads/`, { | ||
| params: { mailbox_id: "" }, // We'll search by subject | ||
| }); | ||
|
|
||
| // Use the page context to make API requests (authenticated via session) | ||
| // Navigate to inbox first to ensure we have the right mailbox context | ||
| await page.goto(`${API_URL}/api/v1.0/threads/?page_size=100`); | ||
| const threadsData = JSON.parse(await page.locator("body").innerText()); | ||
| const targetThread = threadsData.results?.find( | ||
| (t: any) => t.subject === "IMAP unread test" | ||
| ); | ||
|
|
||
| // If we found the thread, the read state should have been synced | ||
| // (the IMAP STORE +FLAGS \Seen should have set read_at on the thread access) | ||
| expect(targetThread).toBeTruthy(); |
There was a problem hiding this comment.
Assert the IMAP→API state change, not just thread existence.
After messageFlagsAdd(..., ["\\Seen"]), this only proves that a thread with that subject exists. It never checks the unread/read field, and the dedicated API response above is ignored, so the test stays green even if the bridge never syncs the flag. Fetch the thread with its mailbox context and assert is_unread === false or the equivalent read marker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e/src/__tests__/client-bridge.spec.ts` around lines 128 - 147, The test
currently only verifies the thread exists (targetThread) after messageFlagsAdd
and never asserts the read/unread state; update the test to fetch the thread
with the mailbox context (use getMailboxEmail("user","chromium") as the
mailbox_id when calling the threads API via page.request.get or by loading the
page URL with that mailbox context) and assert the thread's read marker (e.g.,
targetThread.is_unread === false or equivalent read_at presence) instead of only
checking targetThread truthiness so the IMAP→API flag sync is actually
validated.
Comment on lines
+207
to
+217
| // Reconnect via IMAP and verify the message no longer has \Seen | ||
| const client2 = await createImapClient(); | ||
| try { | ||
| const lock = await client2.getMailboxLock("INBOX"); | ||
| try { | ||
| for await (const msg of client2.fetch("1:*", { flags: true, envelope: true })) { | ||
| if (msg.envelope.subject === targetSubject) { | ||
| expect(msg.flags.has("\\Seen")).toBe(false); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail if the target message is not returned after reconnect.
If fetch("1:*") never yields targetSubject, the loop falls through and the test passes without asserting anything about the IMAP state. Track whether the message was seen and assert after the loop.
✅ Minimal assertion
+ let foundTarget = false;
for await (const msg of client2.fetch("1:*", { flags: true, envelope: true })) {
if (msg.envelope.subject === targetSubject) {
+ foundTarget = true;
expect(msg.flags.has("\\Seen")).toBe(false);
break;
}
}
+ expect(foundTarget).toBe(true);📝 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
| // Reconnect via IMAP and verify the message no longer has \Seen | |
| const client2 = await createImapClient(); | |
| try { | |
| const lock = await client2.getMailboxLock("INBOX"); | |
| try { | |
| for await (const msg of client2.fetch("1:*", { flags: true, envelope: true })) { | |
| if (msg.envelope.subject === targetSubject) { | |
| expect(msg.flags.has("\\Seen")).toBe(false); | |
| break; | |
| } | |
| } | |
| // Reconnect via IMAP and verify the message no longer has \Seen | |
| const client2 = await createImapClient(); | |
| try { | |
| const lock = await client2.getMailboxLock("INBOX"); | |
| try { | |
| let foundTarget = false; | |
| for await (const msg of client2.fetch("1:*", { flags: true, envelope: true })) { | |
| if (msg.envelope.subject === targetSubject) { | |
| foundTarget = true; | |
| expect(msg.flags.has("\\Seen")).toBe(false); | |
| break; | |
| } | |
| } | |
| expect(foundTarget).toBe(true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/e2e/src/__tests__/client-bridge.spec.ts` around lines 207 - 217, The
fetch loop on client2 may never find targetSubject so the test can pass
silently; modify the code around client2.getMailboxLock and client2.fetch("1:*",
{ flags: true, envelope: true }) to track a boolean (e.g., foundTarget) set to
true when msg.envelope.subject === targetSubject, then after the for-await loop
assert that foundTarget is true and that msg.flags (or the recorded flags) does
not contain "\\Seen" (i.e., expect(foundTarget).toBe(true) and
expect(seenFlag).toBe(false) or similar); ensure you reference client2,
getMailboxLock, fetch and targetSubject when adding the flag and post-loop
assertions so the test fails if the message was never returned.
Comment on lines
+32
to
+35
| const formSchema = (t: (key: string) => string) => z.object({ | ||
| name: z.string().min(1, { message: t("Name is required.") }), | ||
| role: z.enum(["reader", "editor", "sender", "sender_only"]), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Zod 4's error parameter instead of message.
In Zod 4, the message option is deprecated in favor of the unified error parameter for error customization.
♻️ Proposed fix
const formSchema = (t: (key: string) => string) => z.object({
- name: z.string().min(1, { message: t("Name is required.") }),
+ name: z.string().min(1, { error: t("Name is required.") }),
role: z.enum(["reader", "editor", "sender", "sender_only"]),
});Based on learnings: "In Zod 4, replace the deprecated message option with the error parameter for error customization."
📝 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
| const formSchema = (t: (key: string) => string) => z.object({ | |
| name: z.string().min(1, { message: t("Name is required.") }), | |
| role: z.enum(["reader", "editor", "sender", "sender_only"]), | |
| }); | |
| const formSchema = (t: (key: string) => string) => z.object({ | |
| name: z.string().min(1, { error: t("Name is required.") }), | |
| role: z.enum(["reader", "editor", "sender", "sender_only"]), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/client-bridge-integration-form.tsx`
around lines 32 - 35, The formSchema uses Zod's deprecated "message" option;
update the schema to use the Zod 4 "error" parameter instead: replace the name
field's z.string().min(..., { message: ... }) with z.string().min(1, { error:
t("Name is required.") }) and likewise ensure any other validation options
(e.g., enum or other refinements) use { error: ... } rather than { message: ...
} so that the formSchema and its validations (referenced in formSchema and the
name validation) are compatible with Zod 4.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With the ability to create API keys, people were going to build this anyway so we did it first. Hah !
This lets users connect Thunderbird, mobile mail apps, or any standard email client to their Messages mailbox.
This won't be activated in ANCT's instance but anyone is free to activate it on theirs. It has been reasonably well tested and we'll support contributions to it with best effort.
This PR introduces a new
client-bridgemicroservice that bridges IMAP (read) and SMTP (send) protocols to the Messages API. Some Messages features (labels, real-time collaboration, rich threads) won't be available through IMAP because, as we have explained over and over, it's a legacy protocol we. do. not. recommend. Messages will continue to innovate regardless. OIDC will continue being the primary auth method.How it works:
password
What's included:
password storage
Summary by CodeRabbit
New Features
API
Frontend
Documentation
Tests & CI