Add IMAP/SMTP email account support ("bring your own server")#3919
Open
jbecke wants to merge 2 commits into
Open
Add IMAP/SMTP email account support ("bring your own server")#3919jbecke wants to merge 2 commits into
jbecke wants to merge 2 commits into
Conversation
Adds a second email provider alongside Gmail so users can connect any mailbox that speaks IMAP (receive) and SMTP (send). Backend: - Migration: IMAP_SMTP provider enum value, email_imap_smtp_credentials (passwords encrypted at rest with AES-256-GCM via the new email_utils::credential_crypto module, keyed by EMAIL_CREDENTIALS_ENCRYPTION_KEY), and email_imap_folder_states (per-folder UIDVALIDITY / last-seen-UID sync state) - New imap_smtp_client crate: rustls-based IMAP client (implicit TLS and STARTTLS, EXAMINE, incremental UID fetches with BODY.PEEK[], special-use sent-folder detection, APPEND, \Seen flag updates) and SMTP submission via mail-send - POST /email/links/imap verifies both server connections before persisting anything, stores encrypted credentials, and enqueues an initial sync - New ImapPoll inbox-sync operation: seeds recent messages per folder, then polls incrementally; threads by References/In-Reply-To, maps folders/flags onto Gmail-compatible system labels, and reuses the existing search/notification/contacts/CRM fan-out. Polls are scheduled through the link refresh cycle and on-demand resync - Scheduled sends dispatch on provider: SMTP submit plus best-effort APPEND of the sent copy into the IMAP sent folder - Read/unread changes are mirrored to the server as \Seen flags via the ops worker; link deletion skips Gmail-only teardown for IMAP links - Gmail-only surfaces (attachment re-download, signature fetch) fail gracefully with clear messages for IMAP links Frontend: - "Connect IMAP/SMTP account" dialog in settings -> Account with per-server host/port/security fields and backend error surfacing - Regenerated email-service client (UserProvider.IMAP_SMTP, ConnectionSecurity, create-link request/response types) Infra: - Optional email_credentials_encryption_key_secret_name stack config provisioning EMAIL_CREDENTIALS_ENCRYPTION_KEY for the service and worker containers; IMAP/SMTP linking stays disabled when unset Known v1 limitations: no historical backfill beyond the seed window, forwarded attachments are dropped (with a warning) on SMTP sends, and attachment bodies aren't re-downloadable from IMAP yet. https://claude.ai/code/session_01MPjkey3yUVNtiPYuk8LKkT
Collapse nested ifs into let-chains and allow the extra context arg on the gmail_ops/scheduled workers, matching the inbox_sync worker. https://claude.ai/code/session_01MPjkey3yUVNtiPYuk8LKkT
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds comprehensive support for IMAP/SMTP email servers as a new provider alongside Gmail. It introduces credential encryption infrastructure, a new IMAP/SMTP client, message mapping and incremental syncing, label mirroring, provider-aware send operations, and a frontend UI for connecting arbitrary email servers. The implementation includes database schema changes, API endpoints, background workers for polling, and wiring of encryption keys throughout the service architecture. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
rust/cloud-storage/email_db_client/src/imap/test.rs (1)
102-124: ⚡ Quick winAdd a regression assertion for stale cursor writes (same UIDVALIDITY).
Please add a case where a second upsert writes a lower
last_seen_uidwith unchangeduid_validity, and assert the stored value remains the higher one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/email_db_client/src/imap/test.rs` around lines 102 - 124, Add a regression assertion in the folder_state_upsert_and_fetch test to ensure stale cursor writes (same uid_validity but lower last_seen_uid) are ignored: after the existing UIDVALIDITY-change check, call upsert_folder_state(&pool, link.id, "INBOX", 101, 1).await? (same uid_validity 101 but last_seen_uid lower than current 0->or if current is 0 use a higher-first then lower-second—ensure you create the scenario by first upserting a higher last_seen_uid for "INBOX" with uid_validity 101), then call fetch_folder_states(&pool, link.id).await? and assert that the stored last_seen_uid for the INBOX remains the higher value (i.e., unchanged) to prevent regression; reference upsert_folder_state and fetch_folder_states to locate where to add this assertion.rust/cloud-storage/imap_smtp_client/src/imap.rs (1)
298-301: ⚡ Quick winPrefer
.inspect_errfor logging onResult.As per coding guidelines, use
.inspect_err(...)instead ofif let Err(e)for log-side effects on errors.
As per coding guidelines, "Prefer using.inspect_errinstead ofif let Err(e)to perform logging on errors".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/imap_smtp_client/src/imap.rs` around lines 298 - 301, The logout method currently uses an if-let to log errors from self.inner.logout().await; change this to call .inspect_err(...) on the Result returned by self.inner.logout().await so the logging is a side-effect and the method can await the future cleanly (e.g., await the call and chain .inspect_err(|e| tracing::debug!(error = ?e, "IMAP logout failed"))); update the pub async fn logout(&mut self) implementation to perform the await and use .inspect_err for error logging instead of the if let Err(e) pattern.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app/packages/service-clients/service-email/openapi.json`:
- Around line 4712-4717: The OpenAPI schema for ServerSettingsInput.port
currently permits 0 and values >65535 which the backend (u16) can't represent;
update the "port" property in service-email openapi.json
(ServerSettingsInput.port) to restrict the valid TCP port range by setting
"minimum": 1 and adding "maximum": 65535 (keeping "type":"integer" and
"format":"int32") so the client contract matches the backend.
In `@rust/cloud-storage/email_db_client/src/imap/mod.rs`:
- Around line 180-184: The ON CONFLICT upsert currently overwrites last_seen_uid
and can move the cursor backward when out-of-order writes arrive; modify the DO
UPDATE to only advance last_seen_uid when the uid_validity is unchanged by using
a conditional/GREATEST expression. Concretely, in the INSERT ... ON CONFLICT
(link_id, folder) DO UPDATE SET block update uid_validity =
EXCLUDED.uid_validity, last_seen_uid = CASE WHEN EXCLUDED.uid_validity =
uid_validity THEN GREATEST(last_seen_uid, EXCLUDED.last_seen_uid) ELSE
EXCLUDED.last_seen_uid END, updated_at = NOW() so last_seen_uid never regresses
for the same uid_validity but is replaced when uid_validity changes (use the
same column names uid_validity, last_seen_uid, EXCLUDED.* shown in the diff).
In `@rust/cloud-storage/email_service/src/api/email/attachments/get.rs`:
- Around line 131-142: The OpenAPI annotations for the attachment GET handler
need to include a 501 response because the handler in email/attachments/get.rs
returns StatusCode::NOT_IMPLEMENTED for IMAP links; update the
#[utoipa::path(... responses = [...])] on the same function (the get attachment
handler) to add a response entry for 501 Not Implemented with a matching
schema/description (e.g., same ErrorResponse type/description used for other
error responses) so the generated spec reflects the new branch that returns
NOT_IMPLEMENTED for UserProvider::ImapSmtp.
In `@rust/cloud-storage/email_service/src/api/email/links/imap.rs`:
- Around line 225-252: After tx.commit() succeeds, don't propagate errors from
ctx.sqs_client.enqueue_gmail_inbox_sync_notification back to the API; instead
treat enqueue failures as best-effort (log the full error and link id) and
return success to the client. Locate the code that calls
enqueue_gmail_inbox_sync_notification (uses InboxSyncPubsubMessage,
InboxSyncOperation::ImapPoll, ImapPollPayload) and change the await error
handling to inspect_err/log the error with context (including inserted.id) and
swallow it (e.g., .ok() or similar) so the endpoint returns the committed link
response rather than a 500 when enqueue fails.
- Around line 51-63: CreateImapLinkError::into_response should return an
ErrorResponse JSON body (not a plain string) to match the API contract: keep the
existing status_code logic in into_response, but construct the documented
ErrorResponse (e.g., ErrorResponse { message: self.to_string(), /* other fields
if any */ }) and return (status_code, Json(error_response)).into_response() or
otherwise serialize the ErrorResponse before converting to a Response; update
the import/use to reference the ErrorResponse type and remove the raw string
return.
In
`@rust/cloud-storage/email_service/src/pubsub/gmail_ops/operations/imap_modify_labels.rs`:
- Around line 61-75: The loop over FLAG_TARGET_FOLDERS calls
session.set_seen_by_message_id(...) but currently only logs Err cases and still
returns Ok(()), which acks the message even if all folder updates failed; change
the logic so the function only returns Ok(()) when at least one call succeeded
(the existing found = true / break branch), and if no folder succeeded (found is
false) return an Err that preserves or wraps the last encountered error (or
constructs a meaningful error including payload.provider_message_id and
marks_read) so the message is not acknowledged and can be retried; apply the
same fix to the analogous block handling labels (the 79-88 section) so failures
across all targets are propagated rather than silently logged.
In
`@rust/cloud-storage/email_service/src/pubsub/inbox_sync/operations/imap_poll.rs`:
- Around line 256-266: Change the thread-resolution order to try resolving by
provider_thread_id first, then fall back to ancestor/global-id lookup: after
computing thread_provider_id and setting message.provider_thread_id, call the DB
lookup that resolves a thread by provider_thread_id (use the same ctx.db and
link.id context) and if it returns a thread id use it; only if that lookup
returns none, call email_db_client::messages::get::get_thread_id_by_global_ids
with lookup_ids (the ancestor_global_ids plus global_id) as currently
implemented. Ensure you reference/keep the existing variables
thread_provider_id, message.provider_thread_id, ancestor_global_ids, lookup_ids
and the function get_thread_id_by_global_ids when adding the new provider-thread
lookup.
- Around line 164-223: The code currently always upserts new_last_seen_uid even
if ingest_message failed, which can skip failed UIDs; change the loop so it
tracks the highest successfully ingested UID (e.g., last_success_uid) and stop
advancing past a failure: when ingest_message returns Err for a fetched message,
do not continue advancing or include subsequent messages (either break or set a
had_error flag and stop updating last_success_uid), and then call
email_db_client::imap::upsert_folder_state with last_success_uid (or the
previous last_seen UID) instead of the precomputed new_last_seen_uid; update
references in the loop around ingest_message, map_imap_message_to_service, and
the upsert_folder_state call to use this guarded last_success_uid.
In `@rust/cloud-storage/imap_smtp_client/src/imap.rs`:
- Around line 63-103: Wrap all network/handshake awaits in connect with explicit
timeouts: use tokio::time::timeout (or a configurable timeout value) around
connect_tls, read_greeting(&mut client, ...), TcpStream::connect,
plain_client.run_command_and_check_ok("STARTTLS", ...),
crate::tls::upgrade_tls(...), and the client.login(...) future so each can fail
fast; when a timeout elapses convert the timeout error into an anyhow::Error
with context mentioning the operation and server (e.g., "IMAP connect to {}:{}
timed out", "IMAP STARTTLS negotiation timed out", "IMAP login to {}:{} timed
out") so callers can distinguish timeout vs other failures and avoid unbounded
awaits.
- Around line 175-176: Guard against count == 0 before doing count - 1 to avoid
underflow: check if count == 0 and return/skip the fetch (or otherwise produce
an empty/no-op range) instead of computing start; otherwise compute start using
status.exists.saturating_sub(count - 1).max(1) and build range =
format!("{}:{}", start, status.exists). Make the change where start and range
are computed (variables status.exists, count, start, range) so no subtraction
occurs when count is zero.
In `@rust/cloud-storage/imap_smtp_client/src/smtp.rs`:
- Around line 116-120: Wrap the unbounded network calls connect(...) and
client.send(envelope).await in timeouts (e.g., tokio::time::timeout) so a
slow/blocked SMTP server cannot stall processing; use a configured Duration
(from settings or a sensible constant) for both the connect and send operations,
map a timeout result into an error with context (preserving the existing
with_context for send and adding context for connect) and ensure the timeout
error is returned/propagated the same way as other failures so callers of
connect(...) and client.send(...) (symbols: connect and client.send) receive a
clear timeout error.
- Around line 92-96: The code currently calls message.attachments.take() which
mutates the Message before the SMTP send; change this to iterate over the
attachments without consuming them (e.g., use
message.attachments.as_ref()/iter() and clone the attachment data or the minimal
fields needed) and call builder = builder.attachment(...) for each cloned
attachment so the original message.attachments remains intact if the build/send
fails; update the loop near the builder usage in smtp.rs to use non-consuming
iteration instead of message.attachments.take().
In
`@rust/cloud-storage/macro_db_client/migrations/20260609173003_imap_smtp_support.sql`:
- Around line 20-45: Add DB-level CHECK constraints to validate port and UID
fields: in the email_imap_smtp_credentials table add CHECKs on imap_port and
smtp_port to ensure values are between 1 and 65535; in the
email_imap_folder_states table add CHECKs on uid_validity and last_seen_uid to
ensure they are non-negative (>= 0) (or an appropriate upper bound for bigint if
desired). You can implement these by adding CHECK clauses to the CREATE TABLE
statements for email_imap_smtp_credentials and email_imap_folder_states or by
issuing ALTER TABLE ... ADD CONSTRAINT statements referencing the columns
imap_port, smtp_port, uid_validity, and last_seen_uid and giving each constraint
a clear name.
In `@rust/cloud-storage/models_email/src/email/service/imap.rs`:
- Around line 31-38: ServerSettings derives Debug but contains plaintext
password; implement a custom Debug (or remove the derived Debug) for
ServerSettings to redact the password field (e.g., show "<redacted>") and do the
same for ImapSmtpCredentials so no credentials are printed in logs; locate the
ServerSettings struct and ImapSmtpCredentials type and replace the derived Debug
with a manual impl that omits or masks password/secret fields.
---
Nitpick comments:
In `@rust/cloud-storage/email_db_client/src/imap/test.rs`:
- Around line 102-124: Add a regression assertion in the
folder_state_upsert_and_fetch test to ensure stale cursor writes (same
uid_validity but lower last_seen_uid) are ignored: after the existing
UIDVALIDITY-change check, call upsert_folder_state(&pool, link.id, "INBOX", 101,
1).await? (same uid_validity 101 but last_seen_uid lower than current 0->or if
current is 0 use a higher-first then lower-second—ensure you create the scenario
by first upserting a higher last_seen_uid for "INBOX" with uid_validity 101),
then call fetch_folder_states(&pool, link.id).await? and assert that the stored
last_seen_uid for the INBOX remains the higher value (i.e., unchanged) to
prevent regression; reference upsert_folder_state and fetch_folder_states to
locate where to add this assertion.
In `@rust/cloud-storage/imap_smtp_client/src/imap.rs`:
- Around line 298-301: The logout method currently uses an if-let to log errors
from self.inner.logout().await; change this to call .inspect_err(...) on the
Result returned by self.inner.logout().await so the logging is a side-effect and
the method can await the future cleanly (e.g., await the call and chain
.inspect_err(|e| tracing::debug!(error = ?e, "IMAP logout failed"))); update the
pub async fn logout(&mut self) implementation to perform the await and use
.inspect_err for error logging instead of the if let Err(e) pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5783d87-c541-4521-b689-2a5faf7f7aca
⛔ Files ignored due to path filters (30)
js/app/packages/service-clients/service-email/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/connectionSecurity.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/createImapLinkRequest.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/createImapLinkResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/resyncResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/resyncResponseBackfillJobId.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/serverSettingsInput.tsis excluded by!**/generated/**js/app/packages/service-clients/service-email/generated/schemas/userProvider.tsis excluded by!**/generated/**rust/cloud-storage/.sqlx/query-15b00cf9b57c2c077f7e55bd25c267a6cf83c9efdc10ba23981f7b85847881c3.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-2ba42e1b8b6e432c5cbe966076ca05523a4b1f4be0798ae362a325c172b19a43.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-2c1df8ba2035cc1f63f9d566e8ac763f4d0b5e28370570971935d29237a0211a.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-45fced9bee3cf06b7f54821c6c96e3b483654160636aab0f6c5ab18f64addc99.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-4857541516ce135939f34754fde00c114cdf9ed7f203553020cf032732892e80.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-4a8b04d2fa922cd2f007241f83d6ad7e4dbcb5e7f6b0560316ca474878971e63.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-50318d6d13fb931307f2115b33d277b5298c45ed798719e792e722bd30615a95.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-6b5a41c0c33c20537ef484ad312d80da7dfa7251e2333f8981e2a76f40e623c2.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-7630425280ebc21d6c451df7474e42b3ae2d0a78219ea44d4831cbbc6ecbd2ca.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-7bdcbf27b9edb2a2efc0e30c83230f6400385b295a50b48776985089d6ae3c84.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-7f9dea3f16070c9e5c67107643cef03d9c7e1704353496d79faf02dc998bf6fa.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-8942100b50a073f4f95b37246d355dc8b17c5a105492901ad72da89438ec6601.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-8a1debc8a93f83a63f33dfd12a1eb8f707e99f0a207a9b34f27848ab362a7bff.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-94058ba40f828251432cad4a2514e4ec0dcd8d9f6517694f3c00d7c9aa8ccc82.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-95c5d4f79e729713642efcda97838eddd466c0ec2ec9f96ae2be185ef9ff1547.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-a81b373188c98bdfa52a878a659909f73796aef0302323f7930fe4d17ccfe6af.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-c588fc5c8adc1e58d7127b11242eb9f9a29d35648304934dc750040aad9e8f1c.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-c8569a737734eab039a3ca9759478b203596330442305a6f3925edf67a8b52a3.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-f4dd3bd30a64a7fd82d9a254a0fb0ea36e8607ebd4c272509afba882a3e0b572.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-f634430e82099c69b9aead94a2870bcf8a7052dce3ea4b07da7ee1586945726f.jsonis excluded by!**/.sqlx/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (62)
infra/stacks/email-service/index.tsjs/app/packages/app/component/settings/Account.tsxjs/app/packages/queries/email/link.tsjs/app/packages/service-clients/service-email/client.tsjs/app/packages/service-clients/service-email/openapi.jsonrust/cloud-storage/email/src/domain/models/link.rsrust/cloud-storage/email/src/inbound/axum/thread_labels_router.rsrust/cloud-storage/email/src/outbound/email_pg_repo/db_types.rsrust/cloud-storage/email/src/outbound/email_pg_repo/link.rsrust/cloud-storage/email_db_client/src/imap/mod.rsrust/cloud-storage/email_db_client/src/imap/test.rsrust/cloud-storage/email_db_client/src/lib.rsrust/cloud-storage/email_db_client/src/links/types.rsrust/cloud-storage/email_db_client/src/messages/get.rsrust/cloud-storage/email_refresh_handler/src/handler.rsrust/cloud-storage/email_service/Cargo.tomlrust/cloud-storage/email_service/src/api/context.rsrust/cloud-storage/email_service/src/api/email/attachments/get.rsrust/cloud-storage/email_service/src/api/email/links/imap.rsrust/cloud-storage/email_service/src/api/email/links/list.rsrust/cloud-storage/email_service/src/api/email/links/mod.rsrust/cloud-storage/email_service/src/api/email/links/resync.rsrust/cloud-storage/email_service/src/api/middleware/link.rsrust/cloud-storage/email_service/src/api/swagger.rsrust/cloud-storage/email_service/src/bin/pubsub_workers/pubsub_workers.rsrust/cloud-storage/email_service/src/config.rsrust/cloud-storage/email_service/src/convert/imap.rsrust/cloud-storage/email_service/src/convert/imap/test.rsrust/cloud-storage/email_service/src/convert/mod.rsrust/cloud-storage/email_service/src/main.rsrust/cloud-storage/email_service/src/pubsub/backfill/worker.rsrust/cloud-storage/email_service/src/pubsub/context.rsrust/cloud-storage/email_service/src/pubsub/gmail_ops/operations/imap_modify_labels.rsrust/cloud-storage/email_service/src/pubsub/gmail_ops/operations/mod.rsrust/cloud-storage/email_service/src/pubsub/gmail_ops/process.rsrust/cloud-storage/email_service/src/pubsub/gmail_ops/worker.rsrust/cloud-storage/email_service/src/pubsub/inbox_sync/operations/imap_poll.rsrust/cloud-storage/email_service/src/pubsub/inbox_sync/operations/mod.rsrust/cloud-storage/email_service/src/pubsub/inbox_sync/operations/upsert_message.rsrust/cloud-storage/email_service/src/pubsub/inbox_sync/process.rsrust/cloud-storage/email_service/src/pubsub/inbox_sync/worker.rsrust/cloud-storage/email_service/src/pubsub/link_manager/process.rsrust/cloud-storage/email_service/src/pubsub/scheduled/context.rsrust/cloud-storage/email_service/src/pubsub/scheduled/process.rsrust/cloud-storage/email_service/src/pubsub/scheduled/worker.rsrust/cloud-storage/email_service/src/util/imap/mod.rsrust/cloud-storage/email_service/src/util/mod.rsrust/cloud-storage/email_utils/Cargo.tomlrust/cloud-storage/email_utils/src/credential_crypto.rsrust/cloud-storage/email_utils/src/credential_crypto/test.rsrust/cloud-storage/email_utils/src/lib.rsrust/cloud-storage/imap_smtp_client/Cargo.tomlrust/cloud-storage/imap_smtp_client/src/imap.rsrust/cloud-storage/imap_smtp_client/src/lib.rsrust/cloud-storage/imap_smtp_client/src/smtp.rsrust/cloud-storage/imap_smtp_client/src/tls.rsrust/cloud-storage/macro_db_client/migrations/20260609173003_imap_smtp_support.sqlrust/cloud-storage/models_email/src/email/api/link.rsrust/cloud-storage/models_email/src/email/service/imap.rsrust/cloud-storage/models_email/src/email/service/link.rsrust/cloud-storage/models_email/src/email/service/mod.rsrust/cloud-storage/models_email/src/gmail/inbox_sync.rs
Comment on lines
+4712
to
+4717
| "port": { | ||
| "type": "integer", | ||
| "format": "int32", | ||
| "description": "Server port, e.g. 993 for IMAP over TLS, 465/587 for SMTP.", | ||
| "minimum": 0 | ||
| }, |
Contributor
There was a problem hiding this comment.
Constrain ServerSettingsInput.port to the backend-supported TCP range.
This schema currently allows values (0 and >65535) that the backend model cannot represent (u16), creating a client/server contract break.
💡 Proposed fix
"port": {
"type": "integer",
"format": "int32",
"description": "Server port, e.g. 993 for IMAP over TLS, 465/587 for SMTP.",
- "minimum": 0
+ "minimum": 1,
+ "maximum": 65535
},📝 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
| "port": { | |
| "type": "integer", | |
| "format": "int32", | |
| "description": "Server port, e.g. 993 for IMAP over TLS, 465/587 for SMTP.", | |
| "minimum": 0 | |
| }, | |
| "port": { | |
| "type": "integer", | |
| "format": "int32", | |
| "description": "Server port, e.g. 993 for IMAP over TLS, 465/587 for SMTP.", | |
| "minimum": 1, | |
| "maximum": 65535 | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/service-clients/service-email/openapi.json` around lines 4712
- 4717, The OpenAPI schema for ServerSettingsInput.port currently permits 0 and
values >65535 which the backend (u16) can't represent; update the "port"
property in service-email openapi.json (ServerSettingsInput.port) to restrict
the valid TCP port range by setting "minimum": 1 and adding "maximum": 65535
(keeping "type":"integer" and "format":"int32") so the client contract matches
the backend.
Comment on lines
+180
to
+184
| ON CONFLICT (link_id, folder) | ||
| DO UPDATE SET | ||
| uid_validity = EXCLUDED.uid_validity, | ||
| last_seen_uid = EXCLUDED.last_seen_uid, | ||
| updated_at = NOW() |
Contributor
There was a problem hiding this comment.
Prevent last_seen_uid from regressing on conflict updates.
Current upserts can move the folder cursor backward when writes arrive out of order for the same uid_validity, which causes avoidable reprocessing.
Suggested conflict update logic
ON CONFLICT (link_id, folder)
DO UPDATE SET
uid_validity = EXCLUDED.uid_validity,
- last_seen_uid = EXCLUDED.last_seen_uid,
+ last_seen_uid = CASE
+ WHEN email_imap_folder_states.uid_validity = EXCLUDED.uid_validity
+ THEN GREATEST(email_imap_folder_states.last_seen_uid, EXCLUDED.last_seen_uid)
+ ELSE EXCLUDED.last_seen_uid
+ END,
updated_at = NOW()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/email_db_client/src/imap/mod.rs` around lines 180 - 184,
The ON CONFLICT upsert currently overwrites last_seen_uid and can move the
cursor backward when out-of-order writes arrive; modify the DO UPDATE to only
advance last_seen_uid when the uid_validity is unchanged by using a
conditional/GREATEST expression. Concretely, in the INSERT ... ON CONFLICT
(link_id, folder) DO UPDATE SET block update uid_validity =
EXCLUDED.uid_validity, last_seen_uid = CASE WHEN EXCLUDED.uid_validity =
uid_validity THEN GREATEST(last_seen_uid, EXCLUDED.last_seen_uid) ELSE
EXCLUDED.last_seen_uid END, updated_at = NOW() so last_seen_uid never regresses
for the same uid_validity but is replaced when uid_validity changes (use the
same column names uid_validity, last_seen_uid, EXCLUDED.* shown in the diff).
Comment on lines
+131
to
+142
| // On-demand fetch is Gmail-only for now: IMAP attachments aren't | ||
| // re-downloadable yet (no per-part provider id is stored), so fail | ||
| // with a clear message instead of a token error. | ||
| if link.provider == models_email::service::link::UserProvider::ImapSmtp { | ||
| return Err(( | ||
| StatusCode::NOT_IMPLEMENTED, | ||
| Json(ErrorResponse { | ||
| message: "attachment downloads are not yet supported for IMAP inboxes".into(), | ||
| }), | ||
| ) | ||
| .into_response()); | ||
| } |
Contributor
There was a problem hiding this comment.
Add the new 501 response to this endpoint’s OpenAPI annotation.
This handler now returns StatusCode::NOT_IMPLEMENTED for IMAP inboxes, but the #[utoipa::path(...responses...)] list for this route doesn’t declare 501.
Proposed fix
responses(
(status = 200, body=GetAttachmentResponse),
(status = 400, body=ErrorResponse),
(status = 401, body=ErrorResponse),
(status = 404, body=ErrorResponse),
(status = 500, body=ErrorResponse),
+ (status = 501, body=ErrorResponse),
)
)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/email_service/src/api/email/attachments/get.rs` around
lines 131 - 142, The OpenAPI annotations for the attachment GET handler need to
include a 501 response because the handler in email/attachments/get.rs returns
StatusCode::NOT_IMPLEMENTED for IMAP links; update the #[utoipa::path(...
responses = [...])] on the same function (the get attachment handler) to add a
response entry for 501 Not Implemented with a matching schema/description (e.g.,
same ErrorResponse type/description used for other error responses) so the
generated spec reflects the new branch that returns NOT_IMPLEMENTED for
UserProvider::ImapSmtp.
Comment on lines
+51
to
+63
| impl IntoResponse for CreateImapLinkError { | ||
| fn into_response(self) -> Response { | ||
| let status_code = match &self { | ||
| CreateImapLinkError::AlreadyInitialized | ||
| | CreateImapLinkError::BadRequest(_) | ||
| | CreateImapLinkError::ImapVerificationFailed(_) | ||
| | CreateImapLinkError::SmtpVerificationFailed(_) => StatusCode::BAD_REQUEST, | ||
| CreateImapLinkError::NotConfigured => StatusCode::NOT_IMPLEMENTED, | ||
| CreateImapLinkError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, | ||
| }; | ||
|
|
||
| (status_code, self.to_string()).into_response() | ||
| } |
Contributor
There was a problem hiding this comment.
Return ErrorResponse JSON to match the documented API contract.
CreateImapLinkError::into_response currently emits a plain string, but this endpoint is declared with ErrorResponse bodies. This breaks runtime/spec consistency for generated clients.
Proposed fix
impl IntoResponse for CreateImapLinkError {
fn into_response(self) -> Response {
let status_code = match &self {
CreateImapLinkError::AlreadyInitialized
| CreateImapLinkError::BadRequest(_)
| CreateImapLinkError::ImapVerificationFailed(_)
| CreateImapLinkError::SmtpVerificationFailed(_) => StatusCode::BAD_REQUEST,
CreateImapLinkError::NotConfigured => StatusCode::NOT_IMPLEMENTED,
CreateImapLinkError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
};
-
- (status_code, self.to_string()).into_response()
+ (
+ status_code,
+ Json(ErrorResponse {
+ message: self.to_string().into(),
+ }),
+ )
+ .into_response()
}
}📝 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
| impl IntoResponse for CreateImapLinkError { | |
| fn into_response(self) -> Response { | |
| let status_code = match &self { | |
| CreateImapLinkError::AlreadyInitialized | |
| | CreateImapLinkError::BadRequest(_) | |
| | CreateImapLinkError::ImapVerificationFailed(_) | |
| | CreateImapLinkError::SmtpVerificationFailed(_) => StatusCode::BAD_REQUEST, | |
| CreateImapLinkError::NotConfigured => StatusCode::NOT_IMPLEMENTED, | |
| CreateImapLinkError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, | |
| }; | |
| (status_code, self.to_string()).into_response() | |
| } | |
| impl IntoResponse for CreateImapLinkError { | |
| fn into_response(self) -> Response { | |
| let status_code = match &self { | |
| CreateImapLinkError::AlreadyInitialized | |
| | CreateImapLinkError::BadRequest(_) | |
| | CreateImapLinkError::ImapVerificationFailed(_) | |
| | CreateImapLinkError::SmtpVerificationFailed(_) => StatusCode::BAD_REQUEST, | |
| CreateImapLinkError::NotConfigured => StatusCode::NOT_IMPLEMENTED, | |
| CreateImapLinkError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, | |
| }; | |
| ( | |
| status_code, | |
| Json(ErrorResponse { | |
| message: self.to_string().into(), | |
| }), | |
| ) | |
| .into_response() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/email_service/src/api/email/links/imap.rs` around lines 51
- 63, CreateImapLinkError::into_response should return an ErrorResponse JSON
body (not a plain string) to match the API contract: keep the existing
status_code logic in into_response, but construct the documented ErrorResponse
(e.g., ErrorResponse { message: self.to_string(), /* other fields if any */ })
and return (status_code, Json(error_response)).into_response() or otherwise
serialize the ErrorResponse before converting to a Response; update the
import/use to reference the ErrorResponse type and remove the raw string return.
Comment on lines
+225
to
+252
| tx.commit() | ||
| .await | ||
| .context("failed to commit link transaction")?; | ||
|
|
||
| // Record link creation in history table for tracking (best-effort) | ||
| email_db_client::links_history::insert::insert_email_link_history( | ||
| &ctx.db, | ||
| inserted.id, | ||
| &inserted.fusionauth_user_id, | ||
| inserted.email_address.0.as_ref(), | ||
| inserted.provider, | ||
| ) | ||
| .await | ||
| .inspect_err(|e| { | ||
| tracing::error!(error=?e, link_id=?inserted.id, "Failed to insert email link history"); | ||
| }) | ||
| .ok(); | ||
|
|
||
| // Seed the inbox with recent mail; subsequent polls are scheduled by the | ||
| // link refresh cycle. | ||
| ctx.sqs_client | ||
| .enqueue_gmail_inbox_sync_notification(InboxSyncPubsubMessage { | ||
| link_id: inserted.id, | ||
| operation: InboxSyncOperation::ImapPoll(ImapPollPayload { initial: true }), | ||
| }) | ||
| .await | ||
| .context("failed to enqueue initial IMAP sync")?; | ||
|
|
Contributor
There was a problem hiding this comment.
Don’t return 500 after the link is already committed.
After commit succeeds, enqueue failure returns an error response. That leaves a saved link with a failed API result, which creates confusing retries and “already connected” follow-ups.
Proposed fix
- ctx.sqs_client
- .enqueue_gmail_inbox_sync_notification(InboxSyncPubsubMessage {
- link_id: inserted.id,
- operation: InboxSyncOperation::ImapPoll(ImapPollPayload { initial: true }),
- })
- .await
- .context("failed to enqueue initial IMAP sync")?;
+ if let Err(e) = ctx
+ .sqs_client
+ .enqueue_gmail_inbox_sync_notification(InboxSyncPubsubMessage {
+ link_id: inserted.id,
+ operation: InboxSyncOperation::ImapPoll(ImapPollPayload { initial: true }),
+ })
+ .await
+ {
+ tracing::error!(error=?e, link_id=?inserted.id, "failed to enqueue initial IMAP sync");
+ }📝 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
| tx.commit() | |
| .await | |
| .context("failed to commit link transaction")?; | |
| // Record link creation in history table for tracking (best-effort) | |
| email_db_client::links_history::insert::insert_email_link_history( | |
| &ctx.db, | |
| inserted.id, | |
| &inserted.fusionauth_user_id, | |
| inserted.email_address.0.as_ref(), | |
| inserted.provider, | |
| ) | |
| .await | |
| .inspect_err(|e| { | |
| tracing::error!(error=?e, link_id=?inserted.id, "Failed to insert email link history"); | |
| }) | |
| .ok(); | |
| // Seed the inbox with recent mail; subsequent polls are scheduled by the | |
| // link refresh cycle. | |
| ctx.sqs_client | |
| .enqueue_gmail_inbox_sync_notification(InboxSyncPubsubMessage { | |
| link_id: inserted.id, | |
| operation: InboxSyncOperation::ImapPoll(ImapPollPayload { initial: true }), | |
| }) | |
| .await | |
| .context("failed to enqueue initial IMAP sync")?; | |
| tx.commit() | |
| .await | |
| .context("failed to commit link transaction")?; | |
| // Record link creation in history table for tracking (best-effort) | |
| email_db_client::links_history::insert::insert_email_link_history( | |
| &ctx.db, | |
| inserted.id, | |
| &inserted.fusionauth_user_id, | |
| inserted.email_address.0.as_ref(), | |
| inserted.provider, | |
| ) | |
| .await | |
| .inspect_err(|e| { | |
| tracing::error!(error=?e, link_id=?inserted.id, "Failed to insert email link history"); | |
| }) | |
| .ok(); | |
| // Seed the inbox with recent mail; subsequent polls are scheduled by the | |
| // link refresh cycle. | |
| ctx.sqs_client | |
| .enqueue_gmail_inbox_sync_notification(InboxSyncPubsubMessage { | |
| link_id: inserted.id, | |
| operation: InboxSyncOperation::ImapPoll(ImapPollPayload { initial: true }), | |
| }) | |
| .await | |
| .inspect_err(|e| { | |
| tracing::error!(error=?e, link_id=?inserted.id, "failed to enqueue initial IMAP sync"); | |
| }) | |
| .ok(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/email_service/src/api/email/links/imap.rs` around lines
225 - 252, After tx.commit() succeeds, don't propagate errors from
ctx.sqs_client.enqueue_gmail_inbox_sync_notification back to the API; instead
treat enqueue failures as best-effort (log the full error and link id) and
return success to the client. Locate the code that calls
enqueue_gmail_inbox_sync_notification (uses InboxSyncPubsubMessage,
InboxSyncOperation::ImapPoll, ImapPollPayload) and change the await error
handling to inspect_err/log the error with context (including inserted.id) and
swallow it (e.g., .ok() or similar) so the endpoint returns the committed link
response rather than a 500 when enqueue fails.
Comment on lines
+175
to
+176
| let start = status.exists.saturating_sub(count - 1).max(1); | ||
| let range = format!("{}:{}", start, status.exists); |
Contributor
There was a problem hiding this comment.
Guard zero-count fetches before subtracting.
count - 1 underflows when count == 0, which can panic in debug builds and produce incorrect range math otherwise.
Suggested fix
pub async fn fetch_recent_messages(
&mut self,
status: &FolderStatus,
count: u32,
) -> anyhow::Result<Vec<FetchedMessage>> {
+ if count == 0 {
+ return Ok(Vec::new());
+ }
if status.exists == 0 {
return Ok(Vec::new());
}
// Sequence numbers are contiguous (1..=exists), so the last `count`
// sequence numbers are the most recent messages.
let start = status.exists.saturating_sub(count - 1).max(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let start = status.exists.saturating_sub(count - 1).max(1); | |
| let range = format!("{}:{}", start, status.exists); | |
| pub async fn fetch_recent_messages( | |
| &mut self, | |
| status: &FolderStatus, | |
| count: u32, | |
| ) -> anyhow::Result<Vec<FetchedMessage>> { | |
| if count == 0 { | |
| return Ok(Vec::new()); | |
| } | |
| if status.exists == 0 { | |
| return Ok(Vec::new()); | |
| } | |
| // Sequence numbers are contiguous (1..=exists), so the last `count` | |
| // sequence numbers are the most recent messages. | |
| let start = status.exists.saturating_sub(count - 1).max(1); | |
| let range = format!("{}:{}", start, status.exists); | |
| // ... rest of function |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/imap_smtp_client/src/imap.rs` around lines 175 - 176,
Guard against count == 0 before doing count - 1 to avoid underflow: check if
count == 0 and return/skip the fetch (or otherwise produce an empty/no-op range)
instead of computing start; otherwise compute start using
status.exists.saturating_sub(count - 1).max(1) and build range =
format!("{}:{}", start, status.exists). Make the change where start and range
are computed (variables status.exists, count, start, range) so no subtraction
occurs when count is zero.
Comment on lines
+92
to
+96
| if let Some(attachments) = message.attachments.take() { | ||
| for att in attachments { | ||
| builder = builder.attachment(att.content_type, att.file_name, att.data); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do not clear attachments before SMTP send succeeds.
message.attachments.take() mutates the input before the network call. If build/send fails, retries can send without attachments.
Suggested fix
- if let Some(attachments) = message.attachments.take() {
- for att in attachments {
- builder = builder.attachment(att.content_type, att.file_name, att.data);
- }
- }
+ if let Some(attachments) = message.attachments.as_ref() {
+ for att in attachments {
+ builder = builder.attachment(
+ att.content_type.clone(),
+ att.file_name.clone(),
+ att.data.clone(),
+ );
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/imap_smtp_client/src/smtp.rs` around lines 92 - 96, The
code currently calls message.attachments.take() which mutates the Message before
the SMTP send; change this to iterate over the attachments without consuming
them (e.g., use message.attachments.as_ref()/iter() and clone the attachment
data or the minimal fields needed) and call builder = builder.attachment(...)
for each cloned attachment so the original message.attachments remains intact if
the build/send fails; update the loop near the builder usage in smtp.rs to use
non-consuming iteration instead of message.attachments.take().
Comment on lines
+116
to
+120
| let mut client = connect(settings).await?; | ||
| client | ||
| .send(envelope) | ||
| .await | ||
| .with_context(|| format!("SMTP send failed for link_id: {}", message.link_id))?; |
Contributor
There was a problem hiding this comment.
Bound SMTP connect/send with timeouts.
These are unbounded network operations right now; one unhealthy remote server can stall scheduled-send processing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/imap_smtp_client/src/smtp.rs` around lines 116 - 120, Wrap
the unbounded network calls connect(...) and client.send(envelope).await in
timeouts (e.g., tokio::time::timeout) so a slow/blocked SMTP server cannot stall
processing; use a configured Duration (from settings or a sensible constant) for
both the connect and send operations, map a timeout result into an error with
context (preserving the existing with_context for send and adding context for
connect) and ensure the timeout error is returned/propagated the same way as
other failures so callers of connect(...) and client.send(...) (symbols: connect
and client.send) receive a clear timeout error.
Comment on lines
+20
to
+45
| CREATE TABLE public.email_imap_smtp_credentials ( | ||
| link_id uuid PRIMARY KEY REFERENCES public.email_links (id) ON DELETE CASCADE, | ||
| imap_host character varying(255) NOT NULL, | ||
| imap_port integer NOT NULL, | ||
| imap_security public.email_connection_security_enum NOT NULL, | ||
| imap_username character varying(320) NOT NULL, | ||
| imap_password_ciphertext bytea NOT NULL, | ||
| smtp_host character varying(255) NOT NULL, | ||
| smtp_port integer NOT NULL, | ||
| smtp_security public.email_connection_security_enum NOT NULL, | ||
| smtp_username character varying(320) NOT NULL, | ||
| smtp_password_ciphertext bytea NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL | ||
| ); | ||
|
|
||
| -- Incremental sync state per IMAP folder, the IMAP analogue of | ||
| -- email_gmail_histories. last_seen_uid tracks the highest UID we've ingested; | ||
| -- a UIDVALIDITY change invalidates stored UIDs and forces a folder re-sync. | ||
| CREATE TABLE public.email_imap_folder_states ( | ||
| link_id uuid NOT NULL REFERENCES public.email_links (id) ON DELETE CASCADE, | ||
| folder text NOT NULL, | ||
| uid_validity bigint NOT NULL, | ||
| last_seen_uid bigint DEFAULT 0 NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| PRIMARY KEY (link_id, folder) |
Contributor
There was a problem hiding this comment.
Add DB-level bounds checks for ports and IMAP UID fields.
imap_port, smtp_port, uid_validity, and last_seen_uid are currently unconstrained. Invalid values can be persisted and later cause runtime sync/connect failures.
Suggested schema hardening
CREATE TABLE public.email_imap_smtp_credentials (
link_id uuid PRIMARY KEY REFERENCES public.email_links (id) ON DELETE CASCADE,
imap_host character varying(255) NOT NULL,
- imap_port integer NOT NULL,
+ imap_port integer NOT NULL CHECK (imap_port BETWEEN 1 AND 65535),
imap_security public.email_connection_security_enum NOT NULL,
imap_username character varying(320) NOT NULL,
imap_password_ciphertext bytea NOT NULL,
smtp_host character varying(255) NOT NULL,
- smtp_port integer NOT NULL,
+ smtp_port integer NOT NULL CHECK (smtp_port BETWEEN 1 AND 65535),
smtp_security public.email_connection_security_enum NOT NULL,
smtp_username character varying(320) NOT NULL,
smtp_password_ciphertext bytea NOT NULL,
created_at timestamp with time zone DEFAULT now() NOT NULL,
updated_at timestamp with time zone DEFAULT now() NOT NULL
);
CREATE TABLE public.email_imap_folder_states (
link_id uuid NOT NULL REFERENCES public.email_links (id) ON DELETE CASCADE,
folder text NOT NULL,
- uid_validity bigint NOT NULL,
- last_seen_uid bigint DEFAULT 0 NOT NULL,
+ uid_validity bigint NOT NULL CHECK (uid_validity > 0),
+ last_seen_uid bigint DEFAULT 0 NOT NULL CHECK (last_seen_uid >= 0),
updated_at timestamp with time zone DEFAULT now() NOT NULL,
PRIMARY KEY (link_id, folder)
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rust/cloud-storage/macro_db_client/migrations/20260609173003_imap_smtp_support.sql`
around lines 20 - 45, Add DB-level CHECK constraints to validate port and UID
fields: in the email_imap_smtp_credentials table add CHECKs on imap_port and
smtp_port to ensure values are between 1 and 65535; in the
email_imap_folder_states table add CHECKs on uid_validity and last_seen_uid to
ensure they are non-negative (>= 0) (or an appropriate upper bound for bigint if
desired). You can implement these by adding CHECK clauses to the CREATE TABLE
statements for email_imap_smtp_credentials and email_imap_folder_states or by
issuing ALTER TABLE ... ADD CONSTRAINT statements referencing the columns
imap_port, smtp_port, uid_validity, and last_seen_uid and giving each constraint
a clear name.
Comment on lines
+31
to
+38
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct ServerSettings { | ||
| pub host: String, | ||
| pub port: u16, | ||
| pub security: ConnectionSecurity, | ||
| pub username: String, | ||
| pub password: String, | ||
| } |
Contributor
There was a problem hiding this comment.
Redact credentials from Debug output for IMAP/SMTP settings.
ServerSettings includes a plaintext password but derives Debug; any debug logging can expose credentials (including via ImapSmtpCredentials).
💡 Proposed fix
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq)]
pub struct ServerSettings {
pub host: String,
pub port: u16,
pub security: ConnectionSecurity,
pub username: String,
pub password: String,
}
+
+impl std::fmt::Debug for ServerSettings {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("ServerSettings")
+ .field("host", &self.host)
+ .field("port", &self.port)
+ .field("security", &self.security)
+ .field("username", &self.username)
+ .field("password", &"<redacted>")
+ .finish()
+ }
+}Also applies to: 41-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/models_email/src/email/service/imap.rs` around lines 31 -
38, ServerSettings derives Debug but contains plaintext password; implement a
custom Debug (or remove the derived Debug) for ServerSettings to redact the
password field (e.g., show "<redacted>") and do the same for ImapSmtpCredentials
so no credentials are printed in logs; locate the ServerSettings struct and
ImapSmtpCredentials type and replace the derived Debug with a manual impl that
omits or masks password/secret fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for connecting arbitrary IMAP/SMTP email servers as inboxes, complementing the existing Gmail OAuth integration. Users can now add non-Gmail email accounts by providing server credentials.
Summary
Implements a complete IMAP/SMTP integration layer including:
Key Changes
Backend Infrastructure:
imap_smtp_clientcrate for IMAP/SMTP protocol handling with TLS supportemail_utils::credential_crypto) using AES-256-GCMemail_db_client::imapmodule for credential and folder state persistenceAPI & Link Management:
POST /email/links/imapendpoint to create IMAP/SMTP links with server verificationMessage Synchronization:
imap_polloperation for incremental inbox/sent folder syncing using UIDVALIDITY and high-water marksmailparsecrateLabel/Flag Operations:
imap_modify_labelsoperation to push read state changes to IMAP\SeenflagScheduled Message Sending:
Frontend:
Configuration:
EMAIL_CREDENTIALS_ENCRYPTION_KEYfor credential encryptionImplementation Details
provider_idandglobal_id\Sentspecial-use mailboxImapPolloperations on configured refresh schedulehttps://claude.ai/code/session_01MPjkey3yUVNtiPYuk8LKkT