Merge pull request #210 from phil-davis/patches-for-6.0.1

Patches for 6.0.1
sabre-io · Jun 26, 2023 · 8deee75 · 8deee75
2 parents 62fe6b2 + 99a7692
commit 8deee75
Show file tree

Hide file tree

Showing 11 changed files with 140 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,11 @@
 ChangeLog
 =========
 
+6.0.1 (2023-06-26)
+------------------
+
+* #207 fix: handle client disconnect properly with ignore_user_abort true (@kesselb)
+
 6.0.0 (2022-08-31)
 ------------------
 
@@ -9,6 +14,12 @@ ChangeLog
 * #192 Set min PHP to 7.4 and add type declarations (@phil-davis)
 * #194 Adjust type declarations a little bit (@phil-davis)
 
+5.1.7 (2023-06-26)
+------------------
+
+* #98 and #176 Add more tests (@peter279k)
+* #207 fix: handle client disconnect properly with ignore_user_abort true (@kesselb)
+
 5.1.6 (2022-07-15)
 ------------------
 

diff --git a/lib/Client.php b/lib/Client.php
@@ -301,8 +301,6 @@ public function setThrowExceptions(bool $throwExceptions): void
      * Adds a CURL setting.
      *
      * These settings will be included in every HTTP request.
-     *
-     * @param mixed $value
      */
     public function addCurlSetting(int $name, $value): void
     {
@@ -340,7 +338,7 @@ protected function doRequest(RequestInterface $request): ResponseInterface
      *
      * @var resource|null
      */
-    private $curlHandle = null;
+    private $curlHandle;
 
     /**
      * Handler for curl_multi requests.
@@ -349,7 +347,7 @@ protected function doRequest(RequestInterface $request): ResponseInterface
      *
      * @var resource|null
      */
-    private $curlMultiHandle = null;
+    private $curlMultiHandle;
 
     /**
      * Has a list of curl handles, as well as their associated success and

diff --git a/lib/Message.php b/lib/Message.php
@@ -22,7 +22,7 @@ abstract class Message implements MessageInterface
      *
      * @var resource|string|callable|null
      */
-    protected $body = null;
+    protected $body;
 
     /**
      * Contains the list of HTTP headers.

diff --git a/lib/Response.php b/lib/Response.php
@@ -101,7 +101,7 @@ class Response extends Message implements ResponseInterface
      * @param array<string, mixed>|null     $headers
      * @param resource|string|callable|null $body
      */
-    public function __construct($status = 500, ?array $headers = null, $body = null)
+    public function __construct($status = 500, array $headers = null, $body = null)
     {
         if (null !== $status) {
             $this->setStatus($status);

diff --git a/lib/Sapi.php b/lib/Sapi.php
@@ -4,8 +4,6 @@
 
 namespace Sabre\HTTP;
 
-use InvalidArgumentException;
-
 /**
  * PHP SAPI.
  *
@@ -115,6 +113,12 @@ public static function sendResponse(ResponseInterface $response): void
                     if ($copied <= 0) {
                         break;
                     }
+                    // Abort on client disconnect.
+                    // With ignore_user_abort(true), the script is not aborted on client disconnect.
+                    // To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there.
+                    if (1 === ignore_user_abort() && 1 === connection_aborted()) {
+                        break;
+                    }
                     $left -= $copied;
                 }
             } else {
@@ -222,11 +226,11 @@ public static function createFromServerArray(array $serverArray): Request
         }
 
         if (null === $url) {
-            throw new InvalidArgumentException('The _SERVER array must have a REQUEST_URI key');
+            throw new \InvalidArgumentException('The _SERVER array must have a REQUEST_URI key');
         }
 
         if (null === $method) {
-            throw new InvalidArgumentException('The _SERVER array must have a REQUEST_METHOD key');
+            throw new \InvalidArgumentException('The _SERVER array must have a REQUEST_METHOD key');
         }
         $r = new Request($method, $url, $headers);
         $r->setHttpVersion($httpVersion);

diff --git a/lib/Version.php b/lib/Version.php
@@ -16,5 +16,5 @@ class Version
     /**
      * Full version number.
      */
-    public const VERSION = '6.0.0';
+    public const VERSION = '6.0.1';
 }
diff --git a/lib/functions.php b/lib/functions.php
@@ -28,7 +28,7 @@
  * See:
  *   http://tools.ietf.org/html/rfc7231#section-7.1.1.1
  *
- * @return bool|DateTime
+ * @return bool|\DateTime
  */
 function parseDate(string $dateString)
 {
@@ -64,7 +64,7 @@ function parseDate(string $dateString)
     }
 
     try {
-        return new DateTime($dateString, new \DateTimeZone('UTC'));
+        return new \DateTime($dateString, new \DateTimeZone('UTC'));
     } catch (\Exception $e) {
         return false;
     }
@@ -73,7 +73,7 @@ function parseDate(string $dateString)
 /**
  * Transforms a DateTime object to a valid HTTP/1.1 Date header value.
  */
-function toDate(DateTime $dateTime): string
+function toDate(\DateTime $dateTime): string
 {
     // We need to clone it, as we don't want to affect the existing
     // DateTime.
@@ -168,9 +168,9 @@ function negotiateContentType(?string $acceptHeaderValue, array $availableOption
 
             // Does this entry win?
             if (
-                ($proposal['quality'] > $lastQuality) ||
-                ($proposal['quality'] === $lastQuality && $specificity > $lastSpecificity) ||
-                ($proposal['quality'] === $lastQuality && $specificity === $lastSpecificity && $optionIndex < $lastOptionIndex)
+                ($proposal['quality'] > $lastQuality)
+                || ($proposal['quality'] === $lastQuality && $specificity > $lastSpecificity)
+                || ($proposal['quality'] === $lastQuality && $specificity === $lastSpecificity && $optionIndex < $lastOptionIndex)
             ) {
                 $lastQuality = $proposal['quality'];
                 $lastSpecificity = $specificity;

diff --git a/phpstan.neon b/phpstan.neon
@@ -12,7 +12,7 @@ parameters:
     path: lib/Client.php
   -
     message: "#^Left side of || is always false.$#"
-    count: 1
+    count: 2
     path: lib/Client.php
   -
     message: "#^Strict comparison using === between null and array<string, mixed> will always evaluate to false.$#"

diff --git a/tests/HTTP/Auth/DigestTest.php b/tests/HTTP/Auth/DigestTest.php
@@ -42,7 +42,7 @@ public function testDigest(): void
             $nc.':'.
             $cnonce.':'.
             'auth:'.
-            md5('GET'.':'.'/')
+            md5('GET:/')
         );
 
         $this->request->setMethod('GET');
@@ -71,7 +71,7 @@ public function testInvalidDigest(): void
             $nc.':'.
             $cnonce.':'.
             'auth:'.
-            md5('GET'.':'.'/')
+            md5('GET:/')
         );
 
         $this->request->setMethod('GET');
@@ -107,7 +107,7 @@ public function testDigestAuthInt(): void
             $nc.':'.
             $cnonce.':'.
             'auth-int:'.
-            md5('POST'.':'.'/'.':'.md5('body'))
+            md5('POST:/:'.md5('body'))
         );
 
         $this->request->setMethod('POST');
@@ -135,7 +135,7 @@ public function testDigestAuthBoth(): void
             $nc.':'.
             $cnonce.':'.
             'auth-int:'.
-            md5('POST'.':'.'/'.':'.md5('body'))
+            md5('POST:/:'.md5('body'))
         );
 
         $this->request->setMethod('POST');

diff --git a/tests/HTTP/SapiTest.php b/tests/HTTP/SapiTest.php
@@ -144,6 +144,7 @@ public function testSend(): void
 
     /**
      * @runInSeparateProcess
+     *
      * @depends testSend
      */
     public function testSendLimitedByContentLengthString(): void
@@ -179,6 +180,7 @@ public function testRecognizeHttp2(): void
 
     /**
      * @runInSeparateProcess
+     *
      * @depends testSend
      */
     public function testSendLimitedByContentLengthStream(): void
@@ -203,7 +205,9 @@ public function testSendLimitedByContentLengthStream(): void
 
     /**
      * @runInSeparateProcess
+     *
      * @depends testSend
+     *
      * @dataProvider sendContentRangeStreamData
      */
     public function testSendContentRangeStream(
@@ -277,6 +281,7 @@ public function sendContentRangeStreamData(): array
 
     /**
      * @runInSeparateProcess
+     *
      * @depends testSend
      */
     public function testSendWorksWithCallbackAsBody(): void
@@ -295,4 +300,35 @@ public function testSendWorksWithCallbackAsBody(): void
 
         $this->assertEquals('foo', $result);
     }
+
+    public function testSendConnectionAborted(): void
+    {
+        $baseUrl = getenv('BASEURL');
+        if (!$baseUrl) {
+            $this->markTestSkipped('Set an environment value BASEURL to continue');
+        }
+
+        $url = rtrim($baseUrl, '/').'/connection_aborted.php';
+        $chunk_size = 4 * 1024 * 1024;
+        $fetch_size = 6 * 1024 * 1024;
+
+        $stream = fopen($url, 'r');
+        $size = 0;
+
+        while ($size <= $fetch_size) {
+            $temp = fread($stream, 8192);
+            if (false === $temp) {
+                break;
+            }
+            $size += strlen($temp);
+        }
+
+        fclose($stream);
+
+        sleep(5);
+
+        $bytes_read = file_get_contents(sys_get_temp_dir().'/dummy_stream_read_counter');
+        $this->assertEquals($chunk_size * 2, $bytes_read);
+        $this->assertGreaterThanOrEqual($fetch_size, $bytes_read);
+    }
 }
diff --git a/tests/www/connection_aborted.php b/tests/www/connection_aborted.php
@@ -0,0 +1,69 @@
+<?php
+
+declare(strict_types=1);
+
+use Sabre\HTTP;
+
+include '../bootstrap.php';
+
+class DummyStream
+{
+    private int $position;
+
+    public function stream_open(string $path, string $mode, int $options, ?string &$opened_path): bool
+    {
+        $this->position = 0;
+
+        return true;
+    }
+
+    public function stream_read(int $count): string
+    {
+        $this->position += $count;
+
+        return random_bytes($count);
+    }
+
+    public function stream_tell(): int
+    {
+        return $this->position;
+    }
+
+    public function stream_eof(): bool
+    {
+        return $this->position > 25 * 1024 * 1024;
+    }
+
+    public function stream_close(): void
+    {
+        file_put_contents(sys_get_temp_dir().'/dummy_stream_read_counter', $this->position);
+    }
+}
+
+/*
+ * The DummyStream wrapper has two functions:
+ * - Provide dummy data.
+ * - Count how many bytes have been read.
+ */
+stream_wrapper_register('dummy', DummyStream::class);
+
+/*
+ * Overwrite default connection handling.
+ * The default behaviour is however for your script to be aborted when the remote client disconnects.
+ *
+ * Nextcloud/ownCloud set ignore_user_abort(true) on purpose to work around
+ * some edge cases where the default behavior would end a script too early.
+ *
+ * https://github.com/owncloud/core/issues/22370
+ * https://github.com/owncloud/core/pull/26775
+ */
+ignore_user_abort(true);
+
+$body = fopen('dummy://hello', 'r');
+
+$response = new HTTP\Response();
+$response->setStatus(200);
+$response->addHeader('Content-Length', 25 * 1024 * 1024);
+$response->setBody($body);
+
+HTTP\Sapi::sendResponse($response);