Connect generic mail securely - #86

apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift (3)

86-88: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Parse the tagged IMAP completion line, not the full buffer. readIMAPResponse(tag:) already waits for the tagged response, but contains("A1 OK") / contains("A2 OK") can still match an earlier untagged line and treat a final NO or BAD as success. Check the tagged line itself and require an exact 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 86 - 88, Update readIMAPResponse(tag:) and the isPositiveGreeting
validation to parse the returned tagged completion line rather than searching
the full response buffer. Extract the line matching the requested tag and
require its completion status to be exactly OK, rejecting tagged NO or BAD
responses while preserving the existing authenticationFailed behavior.

---

175-182: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Parse IMAP LIST responses structurally. listedMailbox(in:) treats the last quoted pair as the mailbox name, so a valid response like * LIST (\Sent) "/" Sent is parsed as / instead of Sent. Literals and escaped quoted names are also unsupported. This can persist an incorrect SPECIAL-USE mapping; parse the LIST fields per RFC 3501 instead.

🤖 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 175 - 182, Update listedMailbox(in:) to parse IMAP LIST responses
structurally according to RFC 3501 rather than selecting the last quoted pair.
Correctly handle unquoted mailbox names, quoted names such as "* LIST (\Sent)
"/" Sent", escaped characters, and literal mailbox fields, returning the decoded
mailbox name used for SPECIAL-USE mapping.

---

184-192: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject CR/LF in auth inputs before building POP3/IMAP commands. USER/PASS are interpolated raw, and quoted(_:) only escapes \ and ", so a newline in username or credential can break out of the command and inject another 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 184 - 192, Validate username, credential, and any other
authentication inputs for CR or LF before constructing POP3 or IMAP commands in
the authentication flows, including authenticatePOP3 and the quoted(_) path.
Reject invalid inputs with the existing authentication/setup error before
calling write, while preserving normal OAuth and non-newline authentication
behavior.

apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift (5)

191-204: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

POP3 USER/PASS are built with zero escaping — same command-injection class as quoted(), and worse since POP3 has no quoting mechanism at all.

username/credential are interpolated directly (lines 195, 199). A \r\n in either value lets an attacker (e.g. via a maliciously crafted "app password") inject additional POP3 commands using the victim's own authenticated session.

🔒️ Proposed fix

   private func authenticatePOP3() async throws {
+    guard !username.contains(where: { $0 == "\r" || $0 == "\n" }),
+      !credential.contains(where: { $0 == "\r" || $0 == "\n" })
+    else {
+      throw GenericMailSetupError.authenticationFailed(.pop3)
+    }
     if authorizationMethod == .oauth {

🤖 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 191 - 204, Update authenticatePOP3 to reject or safely handle any
username or credential containing POP3 command delimiters, especially CR or LF,
before interpolating them into the USER and PASS commands. Preserve normal
authentication behavior for valid values and ensure malicious inputs cannot
inject additional commands.

---

257-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant/dead first clause makes this read like it filters on code, but it doesn't.

$0.hasPrefix("\(code) ") is always a subset of the regex match ^\d{3} , since code is itself a 3-digit string — the loop actually terminates on any terminal 3-digit response line, and the code parameter has no filtering effect. That's the intended (and previously confirmed) behavior, but the dead clause is misleading to future readers.

♻️ Proposed cleanup

   private func readSMTPResponse(code: String) async throws -> String {
     var response = ""
     repeat {
       response += try await readResponse()
     } while !response.components(separatedBy: "\r\n").contains(where: {
-      $0.hasPrefix("\(code) ") || $0.range(of: #"^\d{3} "#, options: .regularExpression) != nil
+      $0.range(of: #"^\d{3} "#, options: .regularExpression) != nil
     })
     return 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 257 - 265, In readSMTPResponse, remove the redundant code-specific
hasPrefix condition and retain only the regular-expression check for any
three-digit SMTP response line. Keep the existing loop termination behavior
unchanged and leave the code parameter intact unless the surrounding API permits
removing it safely.

---

291-299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

No upper bound on unreadResponse growth — a slow/misbehaving mail server can grow this string unboundedly.

The loop only exits once \r\n appears; a server that streams data without ever sending a line terminator (or discoverIMAPRoleMappings's huge LIST output at lines 157-171 combined with readIMAPResponse's tag-wait loop at lines 240-255) can grow this buffer indefinitely, risking memory exhaustion. Since this feature explicitly connects to arbitrary, user-supplied servers, a size cap is a reasonable defensive measure. (The ! at line 295 is safe given the preceding loop invariant, so no separate concern there.)

🛡️ Proposed fix

   private func readResponse() async throws -> String {
+    let maxBufferedBytes = 64 * 1024
     while !unreadResponse.contains("\r\n") {
       unreadResponse += try await task.read()
+      guard unreadResponse.utf8.count <= maxBufferedBytes else {
+        throw GenericMailSetupError.authenticationFailed(endpoint.mailProtocol)
+      }
     }

🤖 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 291 - 299, Update readResponse() to enforce a reasonable maximum
size for unreadResponse while waiting for "\r\n"; reject or terminate the read
when the cap would be exceeded, preserving normal response parsing below the
limit. Ensure the limit also protects callers such as readIMAPResponse and
discoverIMAPRoleMappings from unbounded server responses.

---

267-274: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

quoted() escapes \ and " but not CR/LF — IMAP LOGIN command injection.

RFC 3501 quoted strings cannot contain CR/LF, but nothing here strips or rejects them. If username/credential (e.g. a pasted app-specific password) contains an embedded \r\n, it breaks out of the quoted argument and injects arbitrary IMAP commands into the connection at line 146 (a2 LOGIN \(quoted(username)) \(quoted(credential))\r\n), running with the user's own authenticated session.

🔒️ Proposed fix

   private func quoted(_ value: String) -> String {
-    "\"\(value.replacingOccurrences(of: "\\", with: "\\\\").replacingOccurrences(of: "\"", with: "\\\""))\""
+    let sanitized = value
+      .replacingOccurrences(of: "\r", with: "")
+      .replacingOccurrences(of: "\n", with: "")
+      .replacingOccurrences(of: "\\", with: "\\\\")
+      .replacingOccurrences(of: "\"", with: "\\\"")
+    return "\"\(sanitized)\""
   }

🤖 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 267 - 274, Update the quoted(_:) helper to reject or safely handle
embedded CR and LF characters before escaping backslashes and quotes, ensuring
values used by the LOGIN command cannot inject IMAP command delimiters. Preserve
the existing escaping behavior for valid values and ensure the login path using
quoted(username) and quoted(credential) cannot emit raw CR/LF.

---

85-98: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Use an exhaustive switch for endpoint.security
This keeps any future MailTransportSecurity case from slipping through without explicit TLS handling.

🤖 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
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemGenericMailEndpointVerifier.swift`
around lines 85 - 98, Replace the separate security conditionals in the
connection flow with an exhaustive switch over endpoint.security, explicitly
handling implicitTLS and startTLS while preserving their existing timing and
operations, and add the required handling for any other MailTransportSecurity
case.

apps/unwired-mail/unwired-mail/Features/Account/GenericMailSetupPanel.swift (1)

106-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clear stale setup when loading fails.

A storage error leaves the previously loaded definition and endpoints visible; after changing the email address, this can present stale settings as saved. Clear state before reporting the error.

Proposed fix

     } catch {
+      clearLoadedSetup()
       errorMessage = error.localizedDescription
     }

🤖 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 `@apps/unwired-mail/unwired-mail/Features/Account/GenericMailSetupPanel.swift`
around lines 106 - 108, Update the error path in the setup-loading method
containing the catch block to clear the previously loaded definition and
endpoints before assigning error.localizedDescription to errorMessage. Preserve
the existing error reporting while ensuring failed loads cannot leave stale
settings visible.