service timeouts + no signals + no exit codes

medilies · Aug 12, 2024 · 9401559 · 9401559
1 parent 09b0411
commit 9401559
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 102 deletions.
diff --git a/src/Dompurify/DompurifyService.php b/src/Dompurify/DompurifyService.php
@@ -7,7 +7,6 @@
 use Medilies\Xssless\ConfigInterface;
 use Medilies\Xssless\HasSetupInterface;
 use Medilies\Xssless\ServiceInterface;
-use Medilies\Xssless\XsslessException;
 use Symfony\Component\Process\Exception\ProcessFailedException;
 use Symfony\Component\Process\Process;
 
@@ -58,15 +57,24 @@ public function start(): static
             $this->config->port,
         ]);
 
+        $this->serviceProcess->setIdleTimeout($this->config->startupTimeoutMs / 1000);
+
         $this->serviceProcess->start();
 
+        // ? is there a possibility to miss output before running the waitUntil
         $this->serviceProcess->waitUntil(function (string $type, string $buffer) {
-            // ? timeout
-            // ! ensure service always returns output
-            return strlen($buffer) > 5;
+            if ($type === Process::ERR) {
+                throw new ProcessFailedException($this->serviceProcess);
+            }
+
+            // Must output when service is listening
+            return strlen($buffer) > 4;
         });
 
-        if (! $this->serviceProcess->isRunning()) {
+        $this->serviceProcess->setIdleTimeout(null);
+
+        // ? rm check
+        if (! $this->isRunning()) {
             throw new ProcessFailedException($this->serviceProcess);
         }
 
@@ -75,9 +83,7 @@ public function start(): static
 
     public function stop(): static
     {
-        if ($this->isRunning()) {
-            $this->serviceProcess->stop();
-        }
+        $this->serviceProcess->stop();
 
         return $this;
     }
@@ -96,53 +102,4 @@ public function getIncrementalErrorOutput(): string
     {
         return $this->serviceProcess->getIncrementalErrorOutput();
     }
-
-    public function throwIfFailedOnTerm(): void
-    {
-        if ($this->serviceProcess->isRunning()) {
-            // ? stop it
-            throw new XsslessException('The service is still running');
-        }
-
-        // TODO: fix windows check
-        // https://github.com/medilies/xssless/actions/runs/10288495452/job/28474063301#step:7:26
-        if ($this->isSigTerm() || $this->isWindows()) {
-            return;
-        }
-
-        throw new ProcessFailedException($this->serviceProcess);
-    }
-
-    // ? interface
-    public function waitForTermination(int $timeout): void
-    {
-        $elapsed = 0;
-        $sleepInterval = 100; // Sleep for 100 milliseconds
-
-        while ($this->serviceProcess->isRunning() && $elapsed < $timeout) {
-            usleep($sleepInterval * 1000);
-            $elapsed += $sleepInterval;
-        }
-
-        if ($this->serviceProcess->isRunning()) {
-            throw new XsslessException('Process did not terminate within the given timeout');
-        }
-    }
-
-    // ========================================================================
-
-    private function isWindows(): bool
-    {
-        return strtoupper(substr(PHP_OS, 0, 3)) === 'WIN';
-    }
-
-    private function isSigTerm(): bool
-    {
-        return $this->serviceProcess->getTermSignal() === 15;
-    }
-
-    // private function isSigHup(): bool
-    // {
-    //     return $this->serviceProcess->getTermSignal() === 1;
-    // }
 }
diff --git a/src/Dompurify/DompurifyServiceConfig.php b/src/Dompurify/DompurifyServiceConfig.php
@@ -14,6 +14,7 @@ public function __construct(
         public string $host = '127.0.0.1',
         public int $port = 63000,
         public ?string $binary = null,
+        public int $startupTimeoutMs = 6000,
     ) {
         $this->class = DompurifyService::class;
     }

diff --git a/src/ServiceInterface.php b/src/ServiceInterface.php
@@ -17,6 +17,4 @@ public function isRunning(): bool;
     public function getIncrementalOutput(): string;
 
     public function getIncrementalErrorOutput(): string;
-
-    public function throwIfFailedOnTerm(): void;
 }
diff --git a/src/laravel/Commands/StartCommand.php b/src/laravel/Commands/StartCommand.php
@@ -24,6 +24,7 @@ public function handle(): void
             exit;
         };
 
+        // ? Is this necessary
         pcntl_signal(SIGTERM, $terminate);
         pcntl_signal(SIGINT, $terminate);
 
@@ -43,7 +44,5 @@ public function handle(): void
             // Sleep for a short period to avoid busy-waiting
             usleep(100_000);
         }
-
-        $service->throwIfFailedOnTerm();
     }
 }
diff --git a/tests/Dompurify/DompurifyCliTest.php b/tests/Dompurify/DompurifyCliTest.php
@@ -14,6 +14,22 @@
     expect(fn () => $cleaner->exec('foo'))->toThrow(ProcessFailedException::class);
 });
 
+it('throws when cannot find binary file', function () {
+    $cleaner = (new DompurifyCli)->configure(new DompurifyCliConfig(
+        binary: __DIR__.'/js-mocks/x.js',
+    ));
+
+    expect(fn () => $cleaner->exec('foo'))->toThrow(XsslessException::class);
+});
+
+it('throws when cannot locate temp folder', function () {
+    $cleaner = (new DompurifyCli)->configure(new DompurifyCliConfig(
+        tempFolder: __DIR__.'/x',
+    ));
+
+    expect(fn () => $cleaner->exec('foo'))->toThrow(XsslessException::class);
+});
+
 test('setup()', function () {
     $cleaner = (new Xssless)->using(new DompurifyCliConfig);
 
@@ -44,20 +60,4 @@
     ));
 
     expect(fn () => $cleaner->exec('foo'))->toThrow(XsslessException::class);
-})->depends('setup()');
-
-it('throws when cannot find binary file', function () {
-    $cleaner = (new DompurifyCli)->configure(new DompurifyCliConfig(
-        binary: __DIR__.'/js-mocks/x.js',
-    ));
-
-    expect(fn () => $cleaner->exec('foo'))->toThrow(XsslessException::class);
-})->depends('setup()');
-
-it('throws when cannot locate temp folder', function () {
-    $cleaner = (new DompurifyCli)->configure(new DompurifyCliConfig(
-        tempFolder: __DIR__.'/x',
-    ));
-
-    expect(fn () => $cleaner->exec('foo'))->toThrow(XsslessException::class);
-})->depends('setup()');
+});
diff --git a/tests/Dompurify/DompurifyServiceTest.php b/tests/Dompurify/DompurifyServiceTest.php
@@ -5,6 +5,42 @@
 use Medilies\Xssless\Dompurify\DompurifyServiceConfig;
 use Medilies\Xssless\Xssless;
 use Symfony\Component\Process\Exception\ProcessFailedException;
+use Symfony\Component\Process\Exception\ProcessTimedOutException;
+
+it('throws on bad node path', function () {
+    $service = (new DompurifyService)->configure(new DompurifyServiceConfig(
+        node: 'nodeZz',
+    ));
+
+    expect(fn () => $service->start())->toThrow(ProcessFailedException::class);
+});
+
+it('throws when cannot find binary file', function () {
+    $cleaner = (new DompurifyService)->configure(new DompurifyServiceConfig(
+        binary: __DIR__.'/js-mocks/x.js',
+    ));
+
+    expect(fn () => $cleaner->start('foo'))->toThrow(ProcessFailedException::class);
+});
+
+it('throws on start() timeout', function () {
+    $service = (new DompurifyService)->configure(new DompurifyServiceConfig(
+        binary: __DIR__.'/js-mocks/service-start-timeout.js',
+        startupTimeoutMs: 50,
+    ));
+
+    expect(fn () => $service->start())->toThrow(ProcessTimedOutException::class);
+});
+
+it('throws on bad host', function () {
+    $cleaner = (new DompurifyService)->configure(new DompurifyServiceConfig(
+        host: 'a.b.c.example.com',
+    ));
+
+    $dirty = '<IMG """><SCRIPT>alert("XSS")</SCRIPT>">';
+
+    expect(fn () => $cleaner->send($dirty))->toThrow(ConnectException::class);
+});
 
 test('setup()', function () {
     $cleaner = (new DompurifyService)->configure(new DompurifyServiceConfig);
@@ -21,7 +57,7 @@
 
     $clean = $cleaner->send($dirty);
 
-    $cleaner->stop()->throwIfFailedOnTerm();
+    $cleaner->stop();
 
     expect($clean)->toBe(str_repeat('*/', 34 * 1000).'<img>"&gt;');
 })->depends('setup()');
@@ -39,29 +75,7 @@
 
     $clean = $cleaner->clean($dirty);
 
-    $service->stop()->throwIfFailedOnTerm();
+    $service->stop();
 
     expect($clean)->toBe('<img>"&gt;');
 })->depends('setup()');
-
-it('throws on bad host', function () {
-    $cleaner = (new DompurifyService)->configure(new DompurifyServiceConfig(
-        host: 'a.b.c.example.com',
-    ));
-
-    $dirty = '<IMG """><SCRIPT>alert("XSS")</SCRIPT>">';
-
-    expect(fn () => $cleaner->send($dirty))->toThrow(ConnectException::class);
-});
-
-it('throws on bad node path', function () {
-    $service = (new DompurifyService)->configure(new DompurifyServiceConfig(
-        node: 'nodeZz',
-    ));
-
-    expect(fn () => $service->start())->toThrow(ProcessFailedException::class);
-
-    // $service->waitForTermination(2000);
-    // expect($service->serviceProcess->getExitCode())->toBe(127);
-    // TODO: fix https://github.com/medilies/xssless/actions/runs/10284119857/job/28459470742#step:7:28
-});
diff --git a/tests/Dompurify/js-mocks/service-start-timeout.js b/tests/Dompurify/js-mocks/service-start-timeout.js
@@ -0,0 +1,4 @@
+const start = Date.now();
+while (Date.now() - start < 500) {
+
+}