feat: add KernelBrowser::assertContentType() and prevent saving cor…

…rupt files (#121) Fixes a bug where `Browser::saveSource()` could write corrupt files. Adding metadata to the file output now requires env `BROWSER_SOURCE_DEBUG=1` and even the, only adds if content-type is "text-like"
zenstruck · Feb 21, 2023 · d64b773 · d64b773
1 parent 41a5316
commit d64b773
Show file tree

Hide file tree

Showing 10 changed files with 88 additions and 19 deletions.
diff --git a/README.md b/README.md
@@ -235,6 +235,7 @@ $browser
     ->assertJson()
     ->assertXml()
     ->assertHtml()
+    ->assertContentType('zip')
 
     // by default, exceptions are caught and converted to a response
     // use the BROWSER_CATCH_EXCEPTIONS environment variable to change default

diff --git a/attachment.zip b/attachment.zip
diff --git a/src/Browser.php b/src/Browser.php
@@ -29,6 +29,7 @@ abstract class Browser
 {
     private Session $session;
     private ?string $sourceDir;
+    private bool $sourceDebug;
 
     /** @var string[] */
     private array $savedSources = [];
@@ -42,6 +43,7 @@ public function __construct(Driver $driver, array $options = [])
     {
         $this->session = new Session($driver);
         $this->sourceDir = $options['source_dir'] ?? null;
+        $this->sourceDebug = $options['source_debug'] ?? false;
     }
 
     final public function client(): AbstractBrowser
@@ -403,7 +405,7 @@ final public function saveSource(string $filename): self
             $filename = \sprintf('%s/%s', \rtrim($this->sourceDir, '/'), \ltrim($filename, '/'));
         }
 
-        (new Filesystem())->dumpFile($this->savedSources[] = $filename, $this->session()->source());
+        (new Filesystem())->dumpFile($this->savedSources[] = $filename, $this->session()->source($this->sourceDebug));
 
         return $this;
     }

diff --git a/src/Browser/KernelBrowser.php b/src/Browser/KernelBrowser.php
@@ -442,28 +442,36 @@ final public function assertHeaderContains(string $header, string $expected): se
         return $this;
     }
 
+    /**
+     * @return static
+     */
+    final public function assertContentType(string $contentType): self
+    {
+        return $this->assertHeaderContains('Content-Type', $contentType);
+    }
+
     /**
      * @return static
      */
     final public function assertJson(): self
     {
-        return $this->assertHeaderContains('Content-Type', 'json');
+        return $this->assertContentType('json');
     }
 
     /**
      * @return static
      */
     final public function assertXml(): self
     {
-        return $this->assertHeaderContains('Content-Type', 'xml');
+        return $this->assertContentType('xml');
     }
 
     /**
      * @return static
      */
     final public function assertHtml(): self
     {
-        return $this->assertHeaderContains('Content-Type', 'html');
+        return $this->assertContentType('html');
     }
 
     /**

diff --git a/src/Browser/Session.php b/src/Browser/Session.php
@@ -13,6 +13,7 @@
 
 use Behat\Mink\Element\DocumentElement;
 use Behat\Mink\Exception\DriverException;
+use Behat\Mink\Exception\UnsupportedDriverActionException;
 use Behat\Mink\Session as MinkSession;
 use Behat\Mink\WebAssert;
 use Symfony\Component\BrowserKit\AbstractBrowser;
@@ -93,28 +94,33 @@ public function expectException($expectedException, ?string $expectedMessage = n
         $this->getDriver()->expectException($expectedException, $expectedMessage);
     }
 
-    public function source(): string
+    public function source(bool $sourceDebug = false): string
     {
-        $ret = "<!--\n";
+        $ret = '';
 
-        try {
-            $ret .= "URL: {$this->getCurrentUrl()} ({$this->getStatusCode()})\n\n";
+        // We never want to prepend non-text files with metadata.
+        if ($sourceDebug && $this->isTextContent()) {
+            $ret .= "<!--\n";
+
+            try {
+                $ret .= "URL: {$this->getCurrentUrl()} ({$this->getStatusCode()})\n\n";
 
-            foreach ($this->getResponseHeaders() as $header => $values) {
-                foreach ((array) $values as $value) {
-                    $ret .= "{$header}: {$value}\n";
+                foreach ($this->getResponseHeaders() as $header => $values) {
+                    foreach ((array) $values as $value) {
+                        $ret .= "{$header}: {$value}\n";
+                    }
                 }
+            } catch (DriverException $e) {
+                $ret = "URL: {$this->getCurrentUrl()}\n";
             }
-        } catch (DriverException $e) {
-            $ret = "URL: {$this->getCurrentUrl()}\n";
-        }
 
-        $ret .= "-->\n";
+            $ret .= "-->\n";
+        }
 
         try {
             $ret .= $this->json();
         } catch (DriverException $e) {
-            $ret .= \trim($this->getDriver()->getContent());
+            $ret .= $this->getDriver()->getContent();
         }
 
         return $ret;
@@ -123,7 +129,7 @@ public function source(): string
     public function dump(?string $selector = null): void
     {
         if (!$selector) {
-            self::varDump($this->source());
+            self::varDump($this->source(true));
 
             return;
         }
@@ -171,4 +177,21 @@ private function ensureNoException(): void
             \count($messageNode) ? $messageNode->text() : 'unknown message',
         ]);
     }
+
+    private function isTextContent(): bool
+    {
+        try {
+            return match (true) {
+                str_contains((string) $this->getResponseHeader('Content-Type'), 'text/plain'),
+                str_contains((string) $this->getResponseHeader('Content-Type'), 'xml'), // to cover all possible XML content-types: "application/xml (is recommended as of RFC 7303 (section 4.1)), text/xml "
+                str_contains((string) $this->getResponseHeader('Content-Type'), 'html'), // to cover all possible (x)HTML content-types: "text/html, application/xhtml+xml"
+                str_contains((string) $this->getResponseHeader('Content-Type'), 'json'), // to cover all possible JSON content-types: "application/json, application/ld+json"
+                => true,
+                default => false,
+            };
+        } catch (UnsupportedDriverActionException) {
+            // We never want to add metadata to source content if Panther is used.
+            return false;
+        }
+    }
 }
diff --git a/src/Browser/Test/HasBrowser.php b/src/Browser/Test/HasBrowser.php
@@ -57,6 +57,7 @@ protected function pantherBrowser(array $options = [], array $kernelOptions = []
 
         $browserOptions = [
             'source_dir' => $_SERVER['BROWSER_SOURCE_DIR'] ?? './var/browser/source',
+            'source_debug' => $_SERVER['BROWSER_SOURCE_DEBUG'] ?? false,
             'screenshot_dir' => $_SERVER['BROWSER_SCREENSHOT_DIR'] ?? './var/browser/screenshots',
             'console_log_dir' => $_SERVER['BROWSER_CONSOLE_LOG_DIR'] ?? './var/browser/console-logs',
         ];
@@ -95,6 +96,7 @@ protected function browser(array $options = [], array $server = []): KernelBrows
 
         $browserOptions = [
             'source_dir' => $_SERVER['BROWSER_SOURCE_DIR'] ?? './var/browser/source',
+            'source_debug' => $_SERVER['BROWSER_SOURCE_DEBUG'] ?? false,
             'follow_redirects' => (bool) ($_SERVER['BROWSER_FOLLOW_REDIRECTS'] ?? true),
             'catch_exceptions' => (bool) ($_SERVER['BROWSER_CATCH_EXCEPTIONS'] ?? true),
         ];

diff --git a/tests/BrowserTests.php b/tests/BrowserTests.php
@@ -233,7 +233,11 @@ public function can_dump_response(): void
             ;
         });
 
-        $this->assertStringContainsString('/page1', $output[0]);
+        // Metadata are never prepended to source content with PantherBrowser
+        if ($this->browser() instanceof Browser\KernelBrowser) {
+            $this->assertStringContainsString('/page1', $output[0]);
+        }
+
         $this->assertStringContainsString('<html', $output[0]);
         $this->assertStringContainsString('<h1>h1 title</h1>', $output[0]);
     }
@@ -250,11 +254,28 @@ public function can_save_source(): void
             ;
         });
 
-        $this->assertStringContainsString('/page1', $contents);
         $this->assertStringContainsString('<html', $contents);
         $this->assertStringContainsString('<h1>h1 title</h1>', $contents);
     }
 
+    /**
+     * @test
+     */
+    public function can_save_source_as_zip(): void
+    {
+        $contents = self::catchFileContents(__DIR__.'/../var/browser/source/attachment.zip', function() {
+            $this->browser()
+                ->visit('/zip')
+                ->saveSource('attachment.zip')
+            ;
+        });
+
+        $this->assertEquals(
+            file_get_contents(__DIR__.'/../var/browser/source/attachment.zip'),
+            $contents
+        );
+    }
+
     /**
      * @test
      */

diff --git a/tests/Fixture/Kernel.php b/tests/Fixture/Kernel.php
@@ -18,6 +18,7 @@
 use Symfony\Component\Config\Loader\LoaderInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\HttpFoundation\HeaderUtils;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
@@ -157,6 +158,14 @@ public function logout(): Response
         return new Response();
     }
 
+    public function zip(): Response
+    {
+        return new Response(\file_get_contents(__DIR__.'/files/attachment.zip'), 200, [
+            'Content-Disposition' => HeaderUtils::makeDisposition(HeaderUtils::DISPOSITION_ATTACHMENT, 'attachment.zip'),
+            'Content-Type' => 'application/zip',
+        ]);
+    }
+
     public function registerBundles(): iterable
     {
         yield new FrameworkBundle();
@@ -213,5 +222,6 @@ private function configureRoutes(RoutingConfigurator $routes): void
         $routes->add('user', '/user')->controller('kernel::user');
         $routes->add('login', '/login')->controller('kernel::login');
         $routes->add('logout', '/logout')->controller('kernel::logout');
+        $routes->add('zip', '/zip')->controller('kernel::zip');
     }
 }
diff --git a/tests/Fixture/files/attachment.zip b/tests/Fixture/files/attachment.zip
diff --git a/tests/KernelBrowserTests.php b/tests/KernelBrowserTests.php
@@ -362,6 +362,8 @@ public function assert_content_types(): void
             ->assertXml()
             ->get('/page1')
             ->assertHtml()
+            ->get('/zip')
+            ->assertContentType('zip')
         ;
     }