feat(generic_imap): add a generic IMAP/SMTP mailbox provider - #242
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:
Summary by CodeRabbit
WalkthroughThis change adds a Node-only generic IMAP provider with custom credentials, shared mail actions, runtime executors, and credential validators. It validates and normalizes IMAP and SMTP settings, derives SMTP hosts, and applies deployment-controlled network policies. Mail connections resolve and pin permitted addresses while preserving the original TLS hostname. SMTP validation uses the configured port and enforces STARTTLS on non-465 ports. Tests cover provider validation and mailbox host enforcement. Documentation lists IMAP Mailbox for private-network access. Sequence Diagram(s)sequenceDiagram
participant Provider
participant genericImapRuntimeConfig
participant NetworkPolicy
participant MailProtocol
participant IMAPOrSMTPTransport
Provider->>genericImapRuntimeConfig: validate and normalize credentials
genericImapRuntimeConfig->>NetworkPolicy: check IMAP and SMTP hosts
NetworkPolicy-->>genericImapRuntimeConfig: allow or reject hosts
Provider->>MailProtocol: create protocol with host policy
MailProtocol->>NetworkPolicy: resolve and screen mailbox addresses
NetworkPolicy-->>MailProtocol: return safe address or reject
MailProtocol->>IMAPOrSMTPTransport: connect using pinned address and original TLS hostname
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/providers/generic_imap/config.ts`:
- Around line 58-82: Re-validate credential.imapHost and credential.smtpHost at
connection time in the IMAP/SMTP protocol flow before passing them to ImapFlow
or nodemailer.createTransport. Resolve each hostname and apply the shared SSRF
policy to every resolved IPv4/IPv6 address, including loopback, link-local,
metadata, and private-network ranges, while preserving the existing
allowPrivateNetwork behavior; do not rely solely on assertMailHost’s save-time
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 982839bd-350e-4c1b-99ce-f5c074158803
📒 Files selected for processing (6)
docs/configuration.mdsrc/providers/generic_imap/actions.tssrc/providers/generic_imap/config.tssrc/providers/generic_imap/definition.tssrc/providers/generic_imap/executors.tssrc/providers/generic_imap/network-access.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mail/imap-smtp/protocol.ts (1)
536-559: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winNest SMTP
servernameundertls.Nodemailer passes SNI through
transportOptions.tls.servername, butsrc/mail/imap-smtp/protocol.tssetsservernameas a top-levelSMTPTransport.Optionsproperty. With IP-based SMTP target pinning, certificate validation can run against the IP instead of the original host. Nest the value undertlsand update the object-shape assertion insrc/mail/imap-smtp/host-pinning.test.tsto checkconfig.tls.servername.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-smtp/protocol.ts` around lines 536 - 559, Update createSmtpTransport so target.servername is placed under the transportConfig.tls object rather than as a top-level property, preserving the original hostname for TLS SNI when the pinned target is an IP; update the corresponding assertion in host-pinning tests to validate config.tls.servername.
🧹 Nitpick comments (1)
src/mail/imap-smtp/protocol.ts (1)
493-523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers the "could not be resolved" branch.
pinMailHostthrowsMailProtocolError("network", ...)whenaddresses.lengthis0. None of the tests inhost-pinning.test.tsexercise this branch (all mockedresolveHostAddressesimplementations return at least one address). Add a test withresolveHostAddresses: async () => []to confirm the error and message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-smtp/protocol.ts` around lines 493 - 523, Update the tests for pinMailHost in host-pinning.test.ts to cover an empty resolution result by providing resolveHostAddresses: async () => []. Assert that the call throws a MailProtocolError with network classification and the message stating that the configured field and host could not be resolved.
🤖 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.
Outside diff comments:
In `@src/mail/imap-smtp/protocol.ts`:
- Around line 536-559: Update createSmtpTransport so target.servername is placed
under the transportConfig.tls object rather than as a top-level property,
preserving the original hostname for TLS SNI when the pinned target is an IP;
update the corresponding assertion in host-pinning tests to validate
config.tls.servername.
---
Nitpick comments:
In `@src/mail/imap-smtp/protocol.ts`:
- Around line 493-523: Update the tests for pinMailHost in host-pinning.test.ts
to cover an empty resolution result by providing resolveHostAddresses: async ()
=> []. Assert that the call throws a MailProtocolError with network
classification and the message stating that the configured field and host could
not be resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3004c2cc-e1c3-4134-bca8-1fcda0b8106c
📒 Files selected for processing (4)
src/mail/imap-smtp/host-pinning.test.tssrc/mail/imap-smtp/protocol.tssrc/mail/imap-smtp/runtime.tssrc/providers/generic_imap/config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/generic_imap/config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/providers/generic_imap/config.ts`:
- Around line 134-135: Update isBareHostname to accept valid IPv6 literals
alongside existing DNS hostnames, using IP-literal validation rather than
broadening the hostname regex. When constructing the synthetic URL passed to the
shared SSRF guard, wrap IPv6 literals in brackets while preserving existing
hostname and IPv4 handling so blocked IPv6 ranges remain screened.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90898237-8158-4fc5-a328-04aa15a96c8b
📒 Files selected for processing (7)
src/mail/imap-smtp/errors.tssrc/mail/imap-smtp/host-pinning.test.tssrc/mail/imap-smtp/protocol.tssrc/mail/imap-smtp/runtime.tssrc/providers/generic_imap/config.tssrc/providers/generic_imap/definition.tssrc/providers/generic_imap/network-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/providers/generic_imap/definition.ts
- src/mail/imap-smtp/runtime.ts
- src/mail/imap-smtp/protocol.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mail/imap-smtp/host-pinning.test.ts (1)
45-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a named interface for the test-case contract.
addressFamilyCasesuses an inline object-shaped contract. DefineAddressFamilyCaseas an interface and useAddressFamilyCase[].As per coding guidelines, prefer
interfacefor object-shaped contracts.Proposed refactor
-const addressFamilyCases: Array<{ +interface AddressFamilyCase { deployment: string; interfaces: Record<string, NetworkInterfaceInfo[]>; pinned: string; -}> = [ +} + +const addressFamilyCases: AddressFamilyCase[] = [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/imap-smtp/host-pinning.test.ts` around lines 45 - 49, Define a named AddressFamilyCase interface for the test-case object contract, then update addressFamilyCases to use AddressFamilyCase[] instead of its inline object type. Keep the existing deployment, interfaces, and pinned fields unchanged.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.
Nitpick comments:
In `@src/mail/imap-smtp/host-pinning.test.ts`:
- Around line 45-49: Define a named AddressFamilyCase interface for the
test-case object contract, then update addressFamilyCases to use
AddressFamilyCase[] instead of its inline object type. Keep the existing
deployment, interfaces, and pinned fields unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea86547f-0f52-4457-a7fb-155569be1368
📒 Files selected for processing (2)
src/mail/imap-smtp/host-pinning.test.tssrc/mail/imap-smtp/protocol.ts
|
PTAL @l1shen |
OpenConnector ships provider-specific mail integrations (qq_mail, netease_mail) whose hosts are hardcoded, but no generic IMAP provider, so a mailbox on any other host cannot be connected at all. Add a `generic_imap` provider built on the shared `MailRuntimeConfig` factory, taking the IMAP/SMTP hosts as user-supplied credential fields. It is the first mail provider whose host comes from user input, so the host is validated against the shared SSRF policy (`assertPublicHttpUrl` gated by `isPrivateNetworkAccessAllowed`) rather than a bespoke hostname check, per AGENTS.md. IMAP/SMTP use implicit-TLS sockets rather than providerFetch, so the credential path is the only place the policy can be applied. Marked `nodeOnly` like the other mail providers: IMAP/SMTP sockets are not reliable from Cloudflare Workers.
The credential-time hostname check cannot stand alone: the DNS record may point at an internal address by the time the socket is opened, and IMAP and SMTP connect over raw TLS, so the guarded fetch never sees these hosts. Resolve the host and screen every returned address with the shared isBlockedIpAddress guard, then connect to the resolved literal address and carry the hostname separately as servername. Both imapflow and nodemailer skip their own lookup when the host is already an IP, so no second, unchecked resolution can happen, while SNI and certificate verification still run against the name the user typed. A host answering with one public and one private address is rejected outright, since the connection library is free to pick either one. Gated behind enforceHostNetworkPolicy, set only by generic_imap: providers with hardcoded hosts keep their exact current behaviour.
…table Review follow-ups on the generic_imap provider and the connect-time host pinning it introduces. Connect-time pinning: - Pin an address the deployment can actually route instead of `addresses[0]`. Pinning takes address selection away from the transport, and both transports did more than take the first answer: nodemailer resolves A before AAAA, skips families with no external interface, and retries the rest on a connect failure, while imapflow leaves the choice to Node's Happy Eyeballs. A lone AAAA answer on an IPv4-only deployment therefore turned a working mailbox into ENETUNREACH. The screened list is now filtered to routable families and prefers IPv4, matching nodemailer's own ordering. - Stop echoing the rejected address back to the caller. `IMAP host x resolves to a blocked address (10.42.7.19)` made the connector an internal name-to-address oracle; the shared egress guard withholds the address for the same reason. - Report a refused host as 400 through a new `blocked_host` error kind, the same status the credential-time guard already returns for a literal address, instead of 502. Provider: - Add an optional SMTP port. iCloud and Outlook.com offer submission on 587 only, and since the connection test verifies SMTP as well as IMAP, 465 being hardcoded made those mailboxes unconnectable rather than merely unsendable. Ports other than 465 negotiate STARTTLS with `requireTLS`, so the password is never sent to a server that does not upgrade. - Name the endpoint in connect-phase transport failures, so a bare "Connection timeout" says which host and port refused the connection. - Derive the SMTP host from a leading `imap` label rather than an `imap.` prefix, so `imap-mail.outlook.com` pairs with `smtp-mail.outlook.com`. - Accept single-label hosts: a Compose service name or a MagicDNS short name is exactly the mailbox a private-network deployment connects. The shared guard still owns the decision and still blocks `localhost`. - Add `homepageUrl`, the only provider definition of 1211 without one, so the console renders an icon. Tests: - Source the guarded protocol config from `genericImapRuntimeConfig`, so dropping the provider's opt-in fails the suite instead of silently disarming the guard. - Cover dual-stack pinning, address non-disclosure, STARTTLS selection, the host derivation table, single-label hosts, and SMTP port parsing.
…anch Follow-ups from automated review of the previous commit. - Inject the local interface table through `MailProtocolDependencies`, like every other environment dependency in this module. `hasInterfaceForFamily` read `os.networkInterfaces()` directly, so it had no test at all: replacing its body with `return true` or `return false` left the whole suite green, and the one test that exercised the preference only passed because the machine running it happens to have an IPv4 interface. It now covers a dual-stack deployment, an IPv4-only container, an IPv6-only host, and a loopback-only container, and both mutations fail. - Cover the empty-resolution branch of `pinMailHost`. An answer with no addresses must fail closed: the screening loop would otherwise pass vacuously and pin an undefined host. - Record why `servername` sits at the top level of both client configs rather than under `tls`. Both libraries read it there (imap-flow.js:282, smtp-connection/index.js:84) and fall back to the host only when it is absent, so nesting is optional but dropping it makes a pinned literal address be checked against the certificate as an IP. Verified end to end against real local TLS servers on the implicit-TLS and STARTTLS paths.
7b534f7 to
945a1ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/core/guarded-fetch.ts`:
- Around line 365-370: Update the screening loop around isBlockedIpAddress to
discard entries whose address is not a valid IP literal, reusing the already
imported isIpAddress helper. Ensure GuardedEgressTarget.addresses returns only
entries with string addresses that pass IP validation and private/reserved
address screening, while preserving the existing error for blocked IPs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21b8d9f7-b23b-40eb-941b-20a4b104e55c
📒 Files selected for processing (12)
docs/configuration.mdsrc/core/guarded-fetch.test.tssrc/core/guarded-fetch.tssrc/mail/imap-smtp/errors.tssrc/mail/imap-smtp/host-pinning.test.tssrc/mail/imap-smtp/protocol.tssrc/mail/imap-smtp/runtime.tssrc/providers/generic_imap/actions.tssrc/providers/generic_imap/config.tssrc/providers/generic_imap/definition.tssrc/providers/generic_imap/executors.tssrc/providers/generic_imap/network-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/mail/imap-smtp/errors.ts
- docs/configuration.md
- src/providers/generic_imap/executors.ts
- src/mail/imap-smtp/host-pinning.test.ts
- src/providers/generic_imap/config.ts
- src/providers/generic_imap/actions.ts
- src/providers/generic_imap/network-access.test.ts
- src/providers/generic_imap/definition.ts
- src/mail/imap-smtp/runtime.ts
| for (const entry of results) { | ||
| if (entry && typeof entry.address === "string" && isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) { | ||
| throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`); | ||
| } | ||
| } | ||
| return results; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Return only verified IP entries from the screening loop.
The loop only screens entries whose address is a string. Any other entry passes through unchecked, and the whole results array is now returned to callers. pinMailHost in src/mail/imap-smtp/protocol.ts feeds these entries into createPinnedLookup, which hands them to node:net and to imapflow. A custom lookup (deps.lookup, or a workerd DoH lookup that maps CNAME answers into { address: "target.example.com.", family: 4 }) can therefore produce a "pinned" entry that is a hostname. The transport then resolves that name itself, which defeats the pin and the address screening.
The module default lookup already filters non-IP answers. Apply the same filter inside the shared screening so every consumer of GuardedEgressTarget.addresses receives only validated IP literals.
🛡️ Proposed fix to screen and filter non-address entries
- for (const entry of results) {
- if (entry && typeof entry.address === "string" && isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) {
- throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`);
- }
- }
- return results;
+ const screened: ResolvedAddress[] = [];
+ for (const entry of results) {
+ if (!entry || typeof entry.address !== "string" || !isIpAddress(entry.address)) {
+ continue;
+ }
+ if (isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) {
+ throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`);
+ }
+ screened.push(entry);
+ }
+ if (screened.length === 0) {
+ throw policy.createResolutionError(`${fieldName} could not be resolved for validation`);
+ }
+ return screened;isIpAddress is already imported in this module.
📝 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.
| for (const entry of results) { | |
| if (entry && typeof entry.address === "string" && isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) { | |
| throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`); | |
| } | |
| } | |
| return results; | |
| const screened: ResolvedAddress[] = []; | |
| for (const entry of results) { | |
| if (!entry || typeof entry.address !== "string" || !isIpAddress(entry.address)) { | |
| continue; | |
| } | |
| if (isBlockedIpAddress(entry.address, policy.allowPrivateNetwork)) { | |
| throw policy.createError(`${fieldName} must not resolve to private or reserved IP addresses`); | |
| } | |
| screened.push(entry); | |
| } | |
| if (screened.length === 0) { | |
| throw policy.createResolutionError(`${fieldName} could not be resolved for validation`); | |
| } | |
| return screened; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/guarded-fetch.ts` around lines 365 - 370, Update the screening loop
around isBlockedIpAddress to discard entries whose address is not a valid IP
literal, reusing the already imported isIpAddress helper. Ensure
GuardedEgressTarget.addresses returns only entries with string addresses that
pass IP validation and private/reserved address screening, while preserving the
existing error for blocked IPs.
What
Adds a
generic_imapprovider so that any mailbox exposing IMAP over implicit TLS (port 993) can be connected with an application password, regardless of the mail host.Open Connector currently ships provider-specific mail integrations (
qq_mail,netease_mail) whose IMAP/SMTP hosts are hardcoded. A mailbox on any other host (OVH, Fastmail, self-hosted Dovecot, corporate mail, ...) cannot be connected at all today.How
MailRuntimeConfigfactory (src/mail/imap-smtp/), so it exposes the same 12 actions as the existing mail providers (send_email,list_folders,search_emails,get_email,download_attachment,mark_email_read,mark_email_unread,move_email,delete_email,get_folder_status,reply_email,forward_email). No new mail plumbing.email,password(application password, secret),imapHost(required),smtpHost(optional, defaults to the IMAP host with theimap.prefix replaced bysmtp., port 465).nodeOnlylike the other mail providers: IMAP/SMTP sockets are not reliable from Cloudflare Workers, and the description says so. The generated Cloudflare registry excludes it.testActionislist_folders, so the connection test authenticates against the user-supplied host before the credential is saved.SSRF hardening
This is the first mail provider whose hosts come from user input, so the hosts are guarded in two places: when the credential is read, and again when the socket is opened.
At credential time
assertPublicHttpUrl(src/core/request.ts), so the provider inherits the existing cloud-metadata, loopback, link-local, reserved-range, and IPv6 blocklists rather than re-implementing them.100.64.0.0/10, private-suffix hostnames) stay blocked unless the deployment opts in throughOOMOL_CONNECT_ALLOW_PRIVATE_NETWORK, mirroring the existing Dokploy behaviour. Loopback and metadata targets stay blocked even then. The flag is read per call viaisPrivateNetworkAccessAllowed(), so it always reflects the current bootstrap configuration.docs/configuration.mdis updated to list the IMAP Mailbox host next to Dokploy as an opt-in consumer of the private-network flag.At connect time (DNS rebinding)
A hostname check alone is not sufficient: the DNS record can point at an internal address by the time the socket is opened, and IMAP/SMTP connect over raw TLS, so the guarded fetch never sees these hosts.
pinMailHostinsrc/mail/imap-smtp/protocol.tscloses that window:dns.lookupwithall: true), and every returned address is screened with the sharedisBlockedIpAddress. A host answering with one public and one private address is rejected outright, since the connection library is free to pick either one;servername. Bothimapflowandnodemailerskip their own lookup when the host is already an IP (net.isIP), so no second, unchecked resolution can happen, while SNI and certificate verification still run against the name the user typed;OOMOL_CONNECT_ALLOW_PRIVATE_NETWORKis honoured here too, and loopback/metadata stay blocked even when it is set.This is gated behind a new
enforceHostNetworkPolicyflag onMailRuntimeConfig, set only bygeneric_imap. Providers with hardcoded hosts (qq_mail,netease_mail) keep their current behaviour exactly: no resolution, no extra DNS round-trip.Tests
src/providers/generic_imap/network-access.test.ts(9 cases) covers the credential-time policy:169.254.169.254,100.100.100.200,0.0.0.0);OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK=true, while RFC 1918/CGNAT/private hostnames become allowed;host:port, userinfo, path, trailing dot, IPv6 literal, whitespace);src/mail/imap-smtp/host-pinning.test.ts(9 cases) covers the connect-time policy:hostwhile the hostname is preserved asservername, for IMAP and for SMTP;How to test
For a live check, connect an "IMAP Mailbox" credential from the catalog UI with any IMAP-enabled mailbox and an application password; the
list_folderstest action runs on save. Trying127.0.0.1or169.254.169.254as the IMAP host is rejected with a 400 naming the field, and a hostname that resolves to either is rejected when the connection is attempted.