make SMTP socket I/O more robust against overlong or multi-line replies and timeouts

src/Mailer.php

@@ -67,6 +67,18 @@ public function setServer($host, $port, $secure=null)
         return $this;
     }
 
+    /**
+     * set the socket timeout in seconds, applied to both connecting and
+     * to each read/write while talking to the server
+     * @param int $timeout
+     * @return $this
+     */
+    public function setTimeout($timeout)
+    {
+        $this->smtp->setTimeout($timeout);
+        return $this;
+    }
+
     /**
      * auth with server
      * @param string $username

src/Mailer/SMTP.php

@@ -41,6 +41,11 @@ class SMTP
      */
     protected $port;
 
+    /**
+     * socket timeout in seconds (connect + read/write)
+     */
+    protected $timeout;
+
     /**
      * smtp secure ssl tls tlsv1.0 tlsv1.1 tlsv1.2
      */
@@ -102,6 +107,7 @@ class SMTP
     public function __construct(LoggerInterface $logger=null)
     {
         $this->logger = $logger;
+        $this->timeout = (int) ini_get('default_socket_timeout');
     }
 
     /**
@@ -160,6 +166,18 @@ public function setEhlo($ehlo)
         return $this;
     }
 
+    /**
+     * set the socket timeout in seconds, applied to both connecting and
+     * to each read/write while talking to the server
+     * @param int $timeout
+     * @return $this
+     */
+    public function setTimeout($timeout)
+    {
+        $this->timeout = (int) $timeout;
+        return $this;
+    }
+
     /**
      * Send the message
      *
@@ -219,13 +237,14 @@ protected function connect()
             $host.':'.$this->port,
             $error_code,
             $error_message,
-            ini_get('default_socket_timeout'),
+            $this->timeout,
             STREAM_CLIENT_CONNECT,
             $context
         );
         if (!$this->smtp){
             throw new SMTPException("Could not open SMTP Port to $host:{$this->port}");
         }
+        stream_set_timeout($this->smtp, $this->timeout);
         $code = $this->getCode();
         if ($code !== '220'){
             throw new CodeException('220', $code, array_pop($this->resultStack));
@@ -449,28 +468,81 @@ protected function quit()
     protected function pushStack($string)
     {
         $this->commandStack[] = $string;
-        fputs($this->smtp, $string, strlen($string));
+        if (fputs($this->smtp, $string, strlen($string)) === false) {
+            throw new SMTPException("Failed to write to SMTP socket");
+        }
         $this->logger && $this->logger->debug('Sent: '. $string);
         return $this->getCode();
     }
 
     /**
-     * get smtp response code
-     * once time has three digital and a space
-     * @return string
-     * @throws SMTPException
+     * Read one complete SMTP reply and return its 3-digit status code.
+     *
+     * An SMTP reply may span several lines (RFC 5321 §4.2.1): continuation
+     * lines have the form "NNN-text", the final line "NNN text" (or "NNN").
+     * All lines are read and reassembled into a single reply, which is pushed
+     * as one entry onto $this->resultStack -- so it stays aligned with
+     * $this->commandStack (one result per command) and CodeException can
+     * surface the whole reply, not just its last line.
+     *
+     * @return string the 3-digit reply code
+     * @throws SMTPException on a closed/timed-out connection or an unparseable reply
      */
     protected function getCode()
     {
-        while ($str = fgets($this->smtp, 515)) {
-            $this->logger && $this->logger->debug("Got: ". $str);
-            $this->resultStack[] = $str;
-            if(substr($str,3,1) == " ") {
-                $code = substr($str,0,3);
-                return $code;
+        $code = null;
+        $reply = '';
+
+        do {
+            $line = $this->readLine();
+            if ($line === null) {
+                $meta = stream_get_meta_data($this->smtp);
+                if (!empty($meta['timed_out'])) {
+                    throw new SMTPException("SMTP read timed out after {$this->timeout} seconds");
+                }
+                throw new SMTPException("SMTP Server did not respond with anything I recognized");
+            }
+
+            $this->logger && $this->logger->debug("Got: " . $line);
+            $reply .= $line;
+
+            // RFC 5321 §4.2.1: "NNN " (or bare "NNN") is the final line,
+            // "NNN-" marks a continuation line that must be followed by more.
+            if (!preg_match('/^(\d{3})([ -]?)/', $line, $m)) {
+                $this->resultStack[] = $reply;
+                throw new SMTPException("SMTP Server did not respond with anything I recognized");
+            }
+
+            $code      = $m[1];
+            $separator = $m[2];
+        } while ($separator === '-');
+
+        $this->resultStack[] = $reply;
+
+        return $code;
+    }
+
+    /**
+     * Read a single complete line (terminated by LF) from the socket,
+     * regardless of how long it is or how the OS chunks it. Reassembling the
+     * whole line here is what keeps a long reply from leaving a tail in the
+     * buffer that would desync the next getCode() call.
+     *
+     * @return string|null the line (including its trailing CRLF), or null on EOF/timeout
+     */
+    protected function readLine()
+    {
+        $line = '';
+        while (true) {
+            $chunk = fgets($this->smtp, 1024);
+            if ($chunk === false) {
+                return ($line === '') ? null : $line;   // EOF / timeout
+            }
+            $line .= $chunk;
+            if (substr($line, -1) === "\n") {
+                return $line;                            // got the full line
             }
         }
-        throw new SMTPException("SMTP Server did not respond with anything I recognized");
     }
 
 }

tests/SMTPParserTest.php

@@ -0,0 +1,168 @@
+<?php
+
+namespace Tx\Mailer\Tests;
+
+use PHPUnit\Framework\TestCase as BaseTestCase;
+use Tx\Mailer\SMTP;
+use Tx\Mailer\Exceptions\SMTPException;
+
+/**
+ * Unit tests for SMTP::getCode() / readLine() reply parsing (no network).
+ *
+ * The parser reads from the protected $smtp socket, so each test wires a real
+ * SMTP instance to an in-memory stream and invokes the protected getCode() via
+ * reflection — that keeps us testing the real implementation without a helper
+ * subclass and without stubbing anything away.
+ */
+class SMTPParserTest extends BaseTestCase
+{
+    /**
+     * Build a real SMTP whose socket is an in-memory stream preloaded with the
+     * given reply, and return: a callable that invokes the protected getCode(),
+     * the stream itself (to assert how much was consumed), and a callable that
+     * returns the protected resultStack (the reassembled replies CodeException
+     * reports on failure).
+     *
+     * @return array{0: callable, 1: resource, 2: callable}
+     */
+    private function smtpReading($reply)
+    {
+        $fp = fopen('php://memory', 'r+');
+        fwrite($fp, $reply);
+        rewind($fp);
+
+        $smtp = new SMTP();
+
+        $prop = new \ReflectionProperty(SMTP::class, 'smtp');
+        $getCode = new \ReflectionMethod(SMTP::class, 'getCode');
+        $stack = new \ReflectionProperty(SMTP::class, 'resultStack');
+
+        // setAccessible() is a no-op since PHP 8.1 and a deprecation as of 8.5,
+        // so only call it on the older PHP versions where it is still required.
+        if (PHP_VERSION_ID < 80100) {
+            $prop->setAccessible(true);
+            $getCode->setAccessible(true);
+            $stack->setAccessible(true);
+        }
+
+        $prop->setValue($smtp, $fp);
+
+        $invoke = function () use ($getCode, $smtp) {
+            return $getCode->invoke($smtp);
+        };
+        $resultStack = function () use ($stack, $smtp) {
+            return $stack->getValue($smtp);
+        };
+
+        return array($invoke, $fp, $resultStack);
+    }
+
+    public function testSingleLineReply()
+    {
+        list($getCode) = $this->smtpReading("250 OK\r\n");
+        $this->assertSame('250', $getCode());
+    }
+
+    /**
+     * Every continuation line is consumed, leaving the stream at EOF so the
+     * next command reads its own reply.
+     */
+    public function testMultiLineReplyIsFullyConsumed()
+    {
+        list($getCode, $fp) = $this->smtpReading(
+            "250-mx.example.com\r\n" .
+            "250-PIPELINING\r\n" .
+            "250-SIZE 1000000\r\n" .
+            "250 8BITMIME\r\n"
+        );
+        $this->assertSame('250', $getCode());
+        $this->assertFalse(fgets($fp));
+    }
+
+    /**
+     * A multi-line reply is recorded as a single reassembled resultStack entry,
+     * keeping resultStack aligned with commandStack (one result per command).
+     */
+    public function testMultiLineReplyIsReassembledIntoOneStackEntry()
+    {
+        $reply =
+            "250-mx.example.com\r\n" .
+            "250-PIPELINING\r\n" .
+            "250-SIZE 1000000\r\n" .
+            "250 8BITMIME\r\n";
+        list($getCode, , $resultStack) = $this->smtpReading($reply);
+        $this->assertSame('250', $getCode());
+
+        $stack = $resultStack();
+        $this->assertCount(1, $stack);
+        $this->assertSame($reply, $stack[0]);
+    }
+
+    /**
+     * The whole multi-line reply is preserved in the single resultStack entry
+     * callers pop for CodeException, so an explanatory line survives alongside
+     * the final one.
+     */
+    public function testMultiLineErrorReplyKeepsExplanatoryText()
+    {
+        $reply =
+            "535-5.7.8 Username and Password not accepted.\r\n" .
+            "535 5.7.8 https://support.google.com/mail/?p=BadCredentials\r\n";
+        list($getCode, , $resultStack) = $this->smtpReading($reply);
+        $this->assertSame('535', $getCode());
+
+        $stack = $resultStack();
+        $popped = array_pop($stack);
+        $this->assertSame($reply, $popped);
+        $this->assertStringContainsString('Username and Password not accepted', $popped);
+    }
+
+    /**
+     * A reply line longer than readLine()'s fgets() chunk is reassembled
+     * byte-for-byte instead of being truncated, leaving nothing buffered.
+     */
+    public function testLongLineIsNotTruncated()
+    {
+        // > the 1024-byte fgets() chunk, so reassembly must stitch >1 chunk
+        $line = "250 " . str_repeat("X", 2000) . "\r\n";
+        list($getCode, $fp, $resultStack) = $this->smtpReading($line);
+        $this->assertSame('250', $getCode());
+        $captured = $resultStack();
+        $this->assertSame($line, $captured[0]);
+        $this->assertFalse(fgets($fp));
+    }
+
+    /**
+     * After a line longer than the fgets() chunk is reassembled, the stream is
+     * left at the start of the next reply, so the following command reads its
+     * own code.
+     */
+    public function testTwoRepliesStayInSyncAfterLongLine()
+    {
+        // first line > the 1024-byte fgets() chunk, forcing reassembly
+        $first = "250 " . str_repeat("X", 2000) . "\r\n";
+        list($getCode) = $this->smtpReading($first . "354 go ahead\r\n");
+        $this->assertSame('250', $getCode());
+        $this->assertSame('354', $getCode());
+    }
+
+    public function testBareCodeLine()
+    {
+        list($getCode) = $this->smtpReading("220\r\n");
+        $this->assertSame('220', $getCode());
+    }
+
+    public function testEmptyReplyThrows()
+    {
+        list($getCode) = $this->smtpReading("");
+        $this->expectException(SMTPException::class);
+        $getCode();
+    }
+
+    public function testGarbageReplyThrows()
+    {
+        list($getCode) = $this->smtpReading("not an smtp reply\r\n");
+        $this->expectException(SMTPException::class);
+        $getCode();
+    }
+}