Merge pull request #1462 from acelaya-forks/feature/search-with-all-tags

Fixed error when filtering short URLs by ALL tags and search term

.github/workflows/ci.yml

@@ -23,7 +23,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0
+          extensions: openswoole-4.11.1
           coverage: none
       - run: composer install --no-interaction --prefer-dist
       - run: composer ${{ matrix.command }}
@@ -45,7 +45,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0
+          extensions: openswoole-4.11.1
           coverage: pcov
           ini-values: pcov.directory=module
       - run: composer install --no-interaction --prefer-dist
@@ -80,7 +80,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0, pdo_sqlsrv-5.10.0
+          extensions: openswoole-4.11.1, pdo_sqlsrv-5.10.0
           coverage: pcov
           ini-values: pcov.directory=module
       - run: composer install --no-interaction --prefer-dist
@@ -115,7 +115,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0
+          extensions: openswoole-4.11.1
           coverage: pcov
           ini-values: pcov.directory=module
       - run: composer install --no-interaction --prefer-dist

.github/workflows/publish-release.yml

@@ -20,7 +20,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0
+          extensions: openswoole-4.11.1
       - if: ${{ matrix.swoole == 'yes' }}
         run: ./build.sh ${GITHUB_REF#refs/tags/v}
       - if: ${{ matrix.swoole == 'no' }}

.github/workflows/publish-swagger-spec.yml

@@ -23,7 +23,7 @@ jobs:
         with:
           php-version: ${{ matrix.php-version }}
           tools: composer
-          extensions: openswoole-4.11.0
+          extensions: openswoole-4.11.1
           coverage: none
       - run: composer install --no-interaction --prefer-dist
       - run: composer swagger:inline

CHANGELOG.md

@@ -4,6 +4,41 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## [3.1.2] - 2022-06-04
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1448](https://github.com/shlinkio/shlink/issues/1448) Fixed HTML entities not being properly parsed when auto-resolving page titles.
+* [#1458](https://github.com/shlinkio/shlink/issues/1458) Fixed 500 error when filtering short URLs by ALL tags and search term.
+
+
+## [3.1.1] - 2022-05-09
+### Added
+* *Nothing*
+
+### Changed
+* [#1444](https://github.com/shlinkio/shlink/issues/1444) Updated docker image to openswoole 4.11.1, in an attempt to fix error.
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1439](https://github.com/shlinkio/shlink/issues/1439) Fixed crash when trying to auto-resolve titles for URLs which serve large binary files.
+
+
 ## [3.1.0] - 2022-04-23
 ### Added
 * [#1294](https://github.com/shlinkio/shlink/issues/1294) Allowed to provide a specific domain when importing URLs from YOURLS.

Dockerfile

@@ -2,7 +2,7 @@ FROM php:8.1.5-alpine3.15 as base
 
 ARG SHLINK_VERSION=latest
 ENV SHLINK_VERSION ${SHLINK_VERSION}
-ENV OPENSWOOLE_VERSION 4.11.0
+ENV OPENSWOOLE_VERSION 4.11.1
 ENV PDO_SQLSRV_VERSION 5.10.0
 ENV MS_ODBC_SQL_VERSION 17.5.2.2
 ENV LC_ALL "C"

composer.json

@@ -50,8 +50,8 @@
         "shlinkio/shlink-common": "^4.4",
         "shlinkio/shlink-config": "^1.6",
         "shlinkio/shlink-event-dispatcher": "^2.3",
-        "shlinkio/shlink-importer": "dev-main#af0e05e as 3.0",
-        "shlinkio/shlink-installer": "dev-develop#fbbc8f5 as 7.1",
+        "shlinkio/shlink-importer": "^3.0",
+        "shlinkio/shlink-installer": "^7.1",
         "shlinkio/shlink-ip-geolocation": "^2.2",
         "symfony/console": "^6.0",
         "symfony/filesystem": "^6.0",
@@ -65,7 +65,7 @@
         "devster/ubench": "^2.1",
         "dms/phpunit-arraysubset-asserts": "^0.3.0",
         "infection/infection": "^0.26.5",
-        "openswoole/ide-helper": "~4.11.0",
+        "openswoole/ide-helper": "~4.11.1",
         "phpspec/prophecy-phpunit": "^2.0",
         "phpstan/phpstan": "^1.2",
         "phpstan/phpstan-doctrine": "^1.0",
@@ -139,7 +139,7 @@
         "test:api": "bin/test/run-api-tests.sh",
         "test:api:ci": "GENERATE_COVERAGE=yes composer test:api",
         "infect:ci:base": "infection --threads=4 --log-verbosity=default --only-covered --only-covering-test-cases --skip-initial-tests",
-        "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=85",
+        "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=84",
         "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json",
         "infect:ci:api": "@infect:ci:base --coverage=build/coverage-api --min-msi=80 --configuration=infection-api.json",
         "infect:ci": "@parallel infect:ci:unit infect:ci:db infect:ci:api",

data/infra/swoole.Dockerfile

@@ -3,7 +3,7 @@ MAINTAINER Alejandro Celaya <alejandro@alejandrocelaya.com>
 
 ENV APCU_VERSION 5.1.21
 ENV INOTIFY_VERSION 3.0.0
-ENV OPENSWOOLE_VERSION 4.11.0
+ENV OPENSWOOLE_VERSION 4.11.1
 ENV PDO_SQLSRV_VERSION 5.10.0
 ENV MS_ODBC_SQL_VERSION 17.5.2.2
 

module/Core/src/Repository/ShortUrlRepository.php

@@ -102,15 +102,22 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU
                 $qb->leftJoin('s.tags', 't');
             }
 
-            // Apply search conditions
+            // Apply general search conditions
+            $conditions = [
+                $qb->expr()->like('s.longUrl', ':searchPattern'),
+                $qb->expr()->like('s.shortCode', ':searchPattern'),
+                $qb->expr()->like('s.title', ':searchPattern'),
+                $qb->expr()->like('d.authority', ':searchPattern'),
+            ];
+
+            // Apply tag conditions, only when not filtering by all provided tags
+            $tagsMode = $filtering->tagsMode() ?? ShortUrlsParams::TAGS_MODE_ANY;
+            if (empty($tags) || $tagsMode === ShortUrlsParams::TAGS_MODE_ANY) {
+                $conditions[] = $qb->expr()->like('t.name', ':searchPattern');
+            }
+
             $qb->leftJoin('s.domain', 'd')
-               ->andWhere($qb->expr()->orX(
-                   $qb->expr()->like('s.longUrl', ':searchPattern'),
-                   $qb->expr()->like('s.shortCode', ':searchPattern'),
-                   $qb->expr()->like('s.title', ':searchPattern'),
-                   $qb->expr()->like('t.name', ':searchPattern'),
-                   $qb->expr()->like('d.authority', ':searchPattern'),
-               ))
+               ->andWhere($qb->expr()->orX(...$conditions))
                ->setParameter('searchPattern', '%' . $searchTerm . '%');
         }
 

module/Core/src/Util/UrlValidator.php

@@ -11,8 +11,13 @@ use GuzzleHttp\RequestOptions;
 use Psr\Http\Message\ResponseInterface;
 use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+use Throwable;
 
+use function html_entity_decode;
 use function preg_match;
+use function str_contains;
+use function str_starts_with;
+use function strtolower;
 use function trim;
 
 use const Shlinkio\Shlink\TITLE_TAG_VALUE;
@@ -36,7 +41,7 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
             return;
         }
 
-        $this->validateUrlAndGetResponse($url, true);
+        $this->validateUrlAndGetResponse($url);
     }
 
     public function validateUrlWithTitle(string $url, bool $doValidate): ?string
@@ -45,31 +50,61 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
             return null;
         }
 
-        $response = $this->validateUrlAndGetResponse($url, $doValidate);
-        if ($response === null || ! $this->options->autoResolveTitles()) {
+        if (! $this->options->autoResolveTitles()) {
+            $this->validateUrlAndGetResponse($url, self::METHOD_HEAD);
             return null;
         }
 
-        $body = $response->getBody()->__toString();
-        preg_match(TITLE_TAG_VALUE, $body, $matches);
-        return isset($matches[1]) ? trim($matches[1]) : null;
+        $response = $doValidate ? $this->validateUrlAndGetResponse($url) : $this->getResponse($url);
+        if ($response === null) {
+            return null;
+        }
+
+        $contentType = strtolower($response->getHeaderLine('Content-Type'));
+        if (! str_starts_with($contentType, 'text/html')) {
+            return null;
+        }
+
+        $collectedBody = '';
+        $body = $response->getBody();
+        // With streaming enabled, we can walk the body until the </title> tag is found, and then stop
+        while (! str_contains($collectedBody, '</title>') && ! $body->eof()) {
+            $collectedBody .= $body->read(1024);
+        }
+        preg_match(TITLE_TAG_VALUE, $collectedBody, $matches);
+        return isset($matches[1]) ? $this->normalizeTitle($matches[1]) : null;
     }
 
-    private function validateUrlAndGetResponse(string $url, bool $throwOnError): ?ResponseInterface
+    /**
+     * @param self::METHOD_GET|self::METHOD_HEAD $method
+     * @throws InvalidUrlException
+     */
+    private function validateUrlAndGetResponse(string $url, string $method = self::METHOD_GET): ResponseInterface
     {
         try {
-            return $this->httpClient->request(self::METHOD_GET, $url, [
+            return $this->httpClient->request($method, $url, [
                 RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS],
                 RequestOptions::IDN_CONVERSION => true,
                 // Making the request with a browser's user agent makes the validation closer to a real user
                 RequestOptions::HEADERS => ['User-Agent' => self::CHROME_USER_AGENT],
+                RequestOptions::STREAM => true, // This ensures large files are not fully downloaded if not needed
             ]);
         } catch (GuzzleException $e) {
-            if ($throwOnError) {
-                throw InvalidUrlException::fromUrl($url, $e);
-            }
+            throw InvalidUrlException::fromUrl($url, $e);
+        }
+    }
 
+    private function getResponse(string $url): ?ResponseInterface
+    {
+        try {
+            return $this->validateUrlAndGetResponse($url);
+        } catch (Throwable) {
             return null;
         }
     }
+
+    private function normalizeTitle(string $title): string
+    {
+        return html_entity_decode(trim($title));
+    }
 }

module/Core/test-db/Repository/ShortUrlRepositoryTest.php

@@ -128,6 +128,12 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
         self::assertEquals(1, $this->repo->countList(new ShortUrlsCountFiltering('foo', ['bar'])));
         self::assertSame($foo, $result[0]);
 
+        // Assert searched text also applies to tags
+        $result = $this->repo->findList(new ShortUrlsListFiltering(null, null, Ordering::emptyInstance(), 'bar'));
+        self::assertCount(2, $result);
+        self::assertEquals(2, $this->repo->countList(new ShortUrlsCountFiltering('bar')));
+        self::assertContains($foo, $result);
+
         $result = $this->repo->findList(new ShortUrlsListFiltering(null, null, Ordering::emptyInstance()));
         self::assertCount(3, $result);
 

module/Core/test/Util/UrlValidatorTest.php

@@ -107,7 +107,9 @@ class UrlValidatorTest extends TestCase
     /** @test */
     public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabledAndValidationIsEnabled(): void
     {
-        $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle());
+        $request = $this->httpClient->request(RequestMethodInterface::METHOD_HEAD, Argument::cetera())->willReturn(
+            $this->respWithTitle(),
+        );
         $this->options->autoResolveTitles = false;
 
         $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
@@ -119,20 +121,57 @@ class UrlValidatorTest extends TestCase
     /** @test */
     public function validateUrlWithTitleResolvesTitleWhenAutoResolutionIsEnabled(): void
     {
-        $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle());
+        $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn(
+            $this->respWithTitle(),
+        );
         $this->options->autoResolveTitles = true;
 
         $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
 
-        self::assertEquals('Resolved title', $result);
+        self::assertEquals('Resolved "title"', $result);
+        $request->shouldHaveBeenCalledOnce();
+    }
+
+    /** @test */
+    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsEnabledAndReturnedContentTypeIsInvalid(): void
+    {
+        $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn(
+            new Response('php://memory', 200, ['Content-Type' => 'application/octet-stream']),
+        );
+        $this->options->autoResolveTitles = true;
+
+        $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
+
+        self::assertNull($result);
+        $request->shouldHaveBeenCalledOnce();
+    }
+
+    /** @test */
+    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsEnabledAndBodyDoesNotContainTitle(): void
+    {
+        $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn(
+            new Response($this->createStreamWithContent('<body>No title</body>'), 200, ['Content-Type' => 'text/html']),
+        );
+        $this->options->autoResolveTitles = true;
+
+        $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
+
+        self::assertNull($result);
         $request->shouldHaveBeenCalledOnce();
     }
 
     private function respWithTitle(): Response
     {
-        $body = new Stream('php://temp', 'wr');
-        $body->write('<title>  Resolved title</title>');
+        $body = $this->createStreamWithContent('<title data-foo="bar">  Resolved &quot;title&quot; </title>');
+        return new Response($body, 200, ['Content-Type' => 'TEXT/html; charset=utf-8']);
+    }
 
-        return new Response($body);
+    private function createStreamWithContent(string $content): Stream
+    {
+        $body = new Stream('php://temp', 'wr');
+        $body->write($content);
+        $body->rewind();
+
+        return $body;
     }
 }