(geronimo-mail) 04/05: GERONIMO-6904 - A garbled IMAP response leaves the connection poolable and poisons later commands

This is an automated email from the ASF dual-hosted git repository.

rzo1 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/geronimo-mail.git

commit 2392bdffde8de3243f8f1a0468ddf43bd3894bff
Author: Richard Zowalla <[email protected]>
AuthorDate: Sat Jul 18 21:10:28 2026 +0200

    GERONIMO-6904 - A garbled IMAP response leaves the connection poolable and poisons later commands
---
 .../mail/store/imap/connection/IMAPConnection.java | 13 +++-
 .../IMAPConnectionBrokenResponseTest.java          | 86 ++++++++++++++++++++++
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/main/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnection.java b/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/main/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnection.java
index 1d1192f..cc0ae9f 100644
--- a/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/main/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnection.java
+++ b/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/main/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnection.java
@@ -399,7 +399,18 @@ public class IMAPConnection extends MailConnection {
     public IMAPTaggedResponse receiveResponse() throws MessagingException {
         while (true) {
             // read and parse a response from the server.
-            IMAPResponse response = reader.readResponse();
+            IMAPResponse response;
+            try {
+                response = reader.readResponse();
+            } catch (MessagingException e) {
+                // a read or parsing failure leaves the stream position somewhere in the
+                // middle of a response, so this connection can no longer be trusted to
+                // stay in sync with the server.  Close it down and mark it so it is not
+                // returned to the connection pool, then let the failure propagate.
+                setClosed();
+                closeServerConnection();
+                throw e;
+            }
             // The response set is terminated by either a continuation response or a
             // tagged response (we only have a single command active at one time).
             if (response instanceof IMAPTaggedResponse) {
diff --git a/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/test/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnectionBrokenResponseTest.java b/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/test/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnectionBrokenResponseTest.java
new file mode 100644
index 0000000..1df014c
--- /dev/null
+++ b/geronimo-mail_2.1_impl/geronimo-mail_2.1_provider/src/test/java/org/apache/geronimo/mail/store/imap/connection/IMAPConnectionBrokenResponseTest.java
@@ -0,0 +1,86 @@
+/**
+ *  Licensed to the Apache Software Foundation (ASF) under one or more
+ *  contributor license agreements.  See the NOTICE file distributed with
+ *  this work for additional information regarding copyright ownership.
+ *  The ASF licenses this file to You under the Apache License, Version 2.0
+ *  (the "License"); you may not use this file except in compliance with
+ *  the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.geronimo.mail.store.imap.connection;
+
+import java.io.ByteArrayInputStream;
+import java.util.Properties;
+
+import jakarta.mail.MessagingException;
+import jakarta.mail.Session;
+
+import org.apache.geronimo.mail.util.ProtocolProperties;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Regression test for the broken-conn-containment defect: when
+ * readResponse() fails (garbled data or unexpected EOF), the stream position
+ * of the connection is undefined mid-response.  Such a connection used to
+ * remain poolable, so one garbled exchange could poison a later, unrelated
+ * command.  receiveResponse() must mark the connection closed (and shut the
+ * server connection down) before propagating the failure, so the pool
+ * discards it via the releaseClosedConnection path.
+ */
+public class IMAPConnectionBrokenResponseTest {
+
+    private IMAPConnection connectionReading(final byte[] data) {
+        final ProtocolProperties props = new ProtocolProperties(
+                Session.getInstance(new Properties()), "imap", false, 143);
+        final IMAPConnection connection = new IMAPConnection(props, null);
+        // inject the response reader directly; the connection was never
+        // actually connected, so there is no socket to clean up.
+        connection.reader = new IMAPResponseStream(new ByteArrayInputStream(data));
+        return connection;
+    }
+
+    @Test
+    public void testGarbledResponseMarksConnectionClosed() throws Exception {
+        // an untagged response whose payload cannot be parsed ("* )" is the
+        // shape seen in the transient TCK flake)
+        final IMAPConnection connection = connectionReading("* )\r\n".getBytes("ISO8859-1"));
+        assertFalse(connection.isClosed(), "connection must start out poolable");
+
+        assertThrows(MessagingException.class, connection::receiveResponse,
+                "a garbled response must surface as a MessagingException");
+        assertTrue(connection.isClosed(),
+                "a connection that failed mid-response must not be poolable");
+    }
+
+    @Test
+    public void testPrematureEndOfStreamMarksConnectionClosed() throws Exception {
+        // EOF in the middle of a response line
+        final IMAPConnection connection = connectionReading("* OK incomplete".getBytes("ISO8859-1"));
+
+        assertThrows(MessagingException.class, connection::receiveResponse,
+                "an EOF mid-response must surface as a MessagingException");
+        assertTrue(connection.isClosed(),
+                "a connection whose stream hit EOF mid-response must not be poolable");
+    }
+
+    @Test
+    public void testCleanResponseLeavesConnectionPoolable() throws Exception {
+        final IMAPConnection connection = connectionReading("a1 OK completed\r\n".getBytes("ISO8859-1"));
+
+        final IMAPTaggedResponse response = connection.receiveResponse();
+        assertTrue(response.isOK());
+        assertFalse(connection.isClosed(),
+                "a successfully parsed exchange must leave the connection poolable");
+    }
+}