Repository: shlinkio/shlink (php)

Issue:
AUTO_RESOLVE_TITLES=false ignored when validating URLs during creation
<!--
Before opening an issue, just take into account that this is a completely free of charge and open source project.
I'm always happy to help and provide support, but some understanding will be expected.
I do this in my own free time, so expect some delays when implementing new features and fixing bugs, and don't take it personal if an issue gets eventually closed.
You may also be asked to provide tests or ways to reproduce reported bugs.
Try to be polite, and understand it is impossible for an OSS project to cover all use cases.

With that said, please fill in the information requested next. More information might be requested next (like logs or system configs).
-->

#### How Shlink is set-up

* Shlink Version: 3.0.0
* PHP Version: 8.1.2
* How do you serve Shlink: Self-hosted nginx
* Database engine used: PostgreSQL

#### Summary

<!-- Provide a summary describing the problem you are experiencing. -->

I have `AUTO_RESOLVE_TITLES` set to false and now the instance is resolving the title for me anyway.

#### Current behavior

<!-- How is it actually behaving (and it shouldn't)? -->

It is automatically resolving the titles

#### Expected behavior

<!-- How did you expected to behave? -->

It should not resolve the title.

#### How to reproduce

<!-- Provide steps to reproduce the bug. -->

Create a new shortlink with `shlink-php-sdk`.


Pull request description:
Feature/fix autoresolve titles
Closes #1363 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fede80b6..3bfe5979 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,23 @@ 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).
 
+## [Unreleased]
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1363](https://github.com/shlinkio/shlink/issues/1363) Fixed titles being resolved no matter what when `validateUrl` is not set or is explicity set to true.
+
+
 ## [3.0.0] - 2022-01-28
 ### Added
 * [#767](https://github.com/shlinkio/shlink/issues/767) Added full support to use emojis everywhere, whether it is custom slugs, titles, referrers, etc.
diff --git a/composer.json b/composer.json
index 496e961e..a7fc457c 100644
--- a/composer.json
+++ b/composer.json
@@ -128,7 +128,7 @@
         ],
         "test:unit": "@php vendor/bin/phpunit --order-by=random --colors=always --coverage-php build/coverage-unit.cov --testdox",
         "test:unit:ci": "@test:unit --coverage-xml=build/coverage-unit/coverage-xml --log-junit=build/coverage-unit/junit.xml",
-        "test:unit:pretty": "@php vendor/bin/phpunit --order-by=random --colors=always --coverage-html build/coverage-unit/coverage-html",
+        "test:unit:pretty": "@test:unit --coverage-html build/coverage-unit/coverage-html",
         "test:db": "@parallel test:db:sqlite:ci test:db:mysql test:db:maria test:db:postgres test:db:ms",
         "test:db:sqlite": "APP_ENV=test php vendor/bin/phpunit --order-by=random --colors=always --testdox -c phpunit-db.xml",
         "test:db:sqlite:ci": "@test:db:sqlite --coverage-php build/coverage-db.cov --coverage-xml=build/coverage-db/coverage-xml --log-junit=build/coverage-db/junit.xml",
diff --git a/config/constants.php b/config/constants.php
index 978964c5..a7bd0bb7 100644
--- a/config/constants.php
+++ b/config/constants.php
@@ -12,7 +12,7 @@ const MIN_SHORT_CODES_LENGTH = 4;
 const DEFAULT_REDIRECT_STATUS_CODE = StatusCodeInterface::STATUS_FOUND;
 const DEFAULT_REDIRECT_CACHE_LIFETIME = 30;
 const LOCAL_LOCK_FACTORY = 'Shlinkio\Shlink\LocalLockFactory';
-const TITLE_TAG_VALUE = '/<title[^>]*>(.*?)<\/title>/i'; // Matches the value inside an html title tag
+const TITLE_TAG_VALUE = '/<title[^>]*>(.*?)<\/title>/i'; // Matches the value inside a html title tag
 const DEFAULT_QR_CODE_SIZE = 300;
 const DEFAULT_QR_CODE_MARGIN = 0;
 const DEFAULT_QR_CODE_FORMAT = 'png';
diff --git a/module/Core/src/Model/ShortUrlEdit.php b/module/Core/src/Model/ShortUrlEdit.php
index 8e187cf6..d27d1fe6 100644
--- a/module/Core/src/Model/ShortUrlEdit.php
+++ b/module/Core/src/Model/ShortUrlEdit.php
@@ -29,7 +29,7 @@ final class ShortUrlEdit implements TitleResolutionModelInterface
     private bool $titlePropWasProvided = false;
     private ?string $title = null;
     private bool $titleWasAutoResolved = false;
-    private ?bool $validateUrl = null;
+    private bool $validateUrl = false;
     private bool $crawlablePropWasProvided = false;
     private bool $crawlable = false;
     private bool $forwardQueryPropWasProvided = false;
@@ -72,7 +72,7 @@ final class ShortUrlEdit implements TitleResolutionModelInterface
         $this->validSince = parseDateField($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE));
         $this->validUntil = parseDateField($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL));
         $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS);
-        $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL);
+        $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false;
         $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS);
         $this->title = $inputFilter->getValue(ShortUrlInputFilter::TITLE);
         $this->crawlable = $inputFilter->getValue(ShortUrlInputFilter::CRAWLABLE);
@@ -166,7 +166,7 @@ final class ShortUrlEdit implements TitleResolutionModelInterface
         return $copy;
     }
 
-    public function doValidateUrl(): ?bool
+    public function doValidateUrl(): bool
     {
         return $this->validateUrl;
     }
diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php
index 74390281..86f2c9d1 100644
--- a/module/Core/src/Model/ShortUrlMeta.php
+++ b/module/Core/src/Model/ShortUrlMeta.php
@@ -26,7 +26,7 @@ final class ShortUrlMeta implements TitleResolutionModelInterface
     private ?bool $findIfExists = null;
     private ?string $domain = null;
     private int $shortCodeLength = 5;
-    private ?bool $validateUrl = null;
+    private bool $validateUrl = false;
     private ?ApiKey $apiKey = null;
     private array $tags = [];
     private ?string $title = null;
@@ -73,7 +73,7 @@ final class ShortUrlMeta implements TitleResolutionModelInterface
         $this->customSlug = $inputFilter->getValue(ShortUrlInputFilter::CUSTOM_SLUG);
         $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS);
         $this->findIfExists = $inputFilter->getValue(ShortUrlInputFilter::FIND_IF_EXISTS);
-        $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL);
+        $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false;
         $this->domain = $inputFilter->getValue(ShortUrlInputFilter::DOMAIN);
         $this->shortCodeLength = getOptionalIntFromInputFilter(
             $inputFilter,
@@ -151,7 +151,7 @@ final class ShortUrlMeta implements TitleResolutionModelInterface
         return $this->shortCodeLength;
     }
 
-    public function doValidateUrl(): ?bool
+    public function doValidateUrl(): bool
     {
         return $this->validateUrl;
     }
diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php
index 775254ce..57f4bc37 100644
--- a/module/Core/src/Options/UrlShortenerOptions.php
+++ b/module/Core/src/Options/UrlShortenerOptions.php
@@ -10,20 +10,9 @@ class UrlShortenerOptions extends AbstractOptions
 {
     protected $__strictMode__ = false; // phpcs:ignore
 
-    private bool $validateUrl = true;
     private bool $autoResolveTitles = false;
     private bool $appendExtraPath = false;
 
-    public function isUrlValidationEnabled(): bool
-    {
-        return $this->validateUrl;
-    }
-
-    protected function setValidateUrl(bool $validateUrl): void
-    {
-        $this->validateUrl = $validateUrl;
-    }
-
     public function autoResolveTitles(): bool
     {
         return $this->autoResolveTitles;
diff --git a/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php b/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php
index e17f04c2..8af28706 100644
--- a/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php
+++ b/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php
@@ -10,7 +10,7 @@ interface TitleResolutionModelInterface
 
     public function getLongUrl(): string;
 
-    public function doValidateUrl(): ?bool;
+    public function doValidateUrl(): bool;
 
     public function withResolvedTitle(string $title): self;
 }
diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php
index bd0a5cfb..da061c0d 100644
--- a/module/Core/src/Util/UrlValidator.php
+++ b/module/Core/src/Util/UrlValidator.php
@@ -30,10 +30,8 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
     /**
      * @throws InvalidUrlException
      */
-    public function validateUrl(string $url, ?bool $doValidate): void
+    public function validateUrl(string $url, bool $doValidate): void
     {
-        // If the URL validation is not enabled, or it was explicitly set to not validate, skip check
-        $doValidate = $doValidate ?? $this->options->isUrlValidationEnabled();
         if (! $doValidate) {
             return;
         }
@@ -41,15 +39,14 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
         $this->validateUrlAndGetResponse($url, true);
     }
 
-    public function validateUrlWithTitle(string $url, ?bool $doValidate): ?string
+    public function validateUrlWithTitle(string $url, bool $doValidate): ?string
     {
-        $doValidate = $doValidate ?? $this->options->isUrlValidationEnabled();
         if (! $doValidate && ! $this->options->autoResolveTitles()) {
             return null;
         }
 
         $response = $this->validateUrlAndGetResponse($url, $doValidate);
-        if ($response === null) {
+        if ($response === null || ! $this->options->autoResolveTitles()) {
             return null;
         }
 
diff --git a/module/Core/src/Util/UrlValidatorInterface.php b/module/Core/src/Util/UrlValidatorInterface.php
index f198d301..299bd22a 100644
--- a/module/Core/src/Util/UrlValidatorInterface.php
+++ b/module/Core/src/Util/UrlValidatorInterface.php
@@ -11,10 +11,10 @@ interface UrlValidatorInterface
     /**
      * @throws InvalidUrlException
      */
-    public function validateUrl(string $url, ?bool $doValidate): void;
+    public function validateUrl(string $url, bool $doValidate): void;
 
     /**
      * @throws InvalidUrlException
      */
-    public function validateUrlWithTitle(string $url, ?bool $doValidate): ?string;
+    public function validateUrlWithTitle(string $url, bool $doValidate): ?string;
 }


Test patch:
diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
index 6783303c..5a207cfd 100644
--- a/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
+++ b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
@@ -35,10 +35,10 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
             ShortUrlMeta::fromRawData(['longUrl' => $longUrl, 'title' => $title]),
         );
 
-        $this->urlValidator->validateUrlWithTitle($longUrl, null)->shouldHaveBeenCalledTimes(
+        $this->urlValidator->validateUrlWithTitle($longUrl, false)->shouldHaveBeenCalledTimes(
             $validateWithTitleCallsNum,
         );
-        $this->urlValidator->validateUrl($longUrl, null)->shouldHaveBeenCalledTimes($validateCallsNum);
+        $this->urlValidator->validateUrl($longUrl, false)->shouldHaveBeenCalledTimes($validateCallsNum);
     }
 
     public function provideTitles(): iterable
diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php
index 57a5d3ce..55cb4d86 100644
--- a/module/Core/test/Util/UrlValidatorTest.php
+++ b/module/Core/test/Util/UrlValidatorTest.php
@@ -42,7 +42,7 @@ class UrlValidatorTest extends TestCase
         $request->shouldBeCalledOnce();
         $this->expectException(InvalidUrlException::class);
 
-        $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar', null);
+        $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar', true);
     }
 
     /** @test */
@@ -65,50 +65,33 @@ class UrlValidatorTest extends TestCase
             }),
         )->willReturn(new Response());
 
-        $this->urlValidator->validateUrl($expectedUrl, null);
+        $this->urlValidator->validateUrl($expectedUrl, true);
 
         $request->shouldHaveBeenCalledOnce();
     }
 
-    /**
-     * @test
-     * @dataProvider provideDisabledCombinations
-     */
-    public function noCheckIsPerformedWhenUrlValidationIsDisabled(?bool $doValidate, bool $validateUrl): void
+    /** @test */
+    public function noCheckIsPerformedWhenUrlValidationIsDisabled(): void
     {
         $request = $this->httpClient->request(Argument::cetera())->willReturn(new Response());
-        $this->options->validateUrl = $validateUrl;
 
-        $this->urlValidator->validateUrl('', $doValidate);
+        $this->urlValidator->validateUrl('', false);
 
         $request->shouldNotHaveBeenCalled();
     }
 
-    /**
-     * @test
-     * @dataProvider provideDisabledCombinations
-     */
-    public function validateUrlWithTitleReturnsNullWhenRequestFailsAndValidationIsDisabled(
-        ?bool $doValidate,
-        bool $validateUrl,
-    ): void {
+    /** @test */
+    public function validateUrlWithTitleReturnsNullWhenRequestFailsAndValidationIsDisabled(): void
+    {
         $request = $this->httpClient->request(Argument::cetera())->willThrow(ClientException::class);
-        $this->options->validateUrl = $validateUrl;
         $this->options->autoResolveTitles = true;
 
-        $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', $doValidate);
+        $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false);
 
         self::assertNull($result);
         $request->shouldHaveBeenCalledOnce();
     }
 
-    public function provideDisabledCombinations(): iterable
-    {
-        yield 'config is disabled and no runtime option is provided' => [null, false];
-        yield 'config is enabled but runtime option is disabled' => [false, true];
-        yield 'both config and runtime option are disabled' => [false, false];
-    }
-
     /** @test */
     public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabled(): void
     {
@@ -121,6 +104,18 @@ class UrlValidatorTest extends TestCase
         $request->shouldNotHaveBeenCalled();
     }
 
+    /** @test */
+    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabledAndValidationIsEnabled(): void
+    {
+        $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle());
+        $this->options->autoResolveTitles = false;
+
+        $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
+
+        self::assertNull($result);
+        $request->shouldHaveBeenCalledOnce();
+    }
+
     /** @test */
     public function validateUrlWithTitleResolvesTitleWhenAutoResolutionIsEnabled(): void
     {
diff --git a/module/Rest/test-api/Action/CreateShortUrlTest.php b/module/Rest/test-api/Action/CreateShortUrlTest.php
index 0abd2021..2fe529a3 100644
--- a/module/Rest/test-api/Action/CreateShortUrlTest.php
+++ b/module/Rest/test-api/Action/CreateShortUrlTest.php
@@ -230,7 +230,7 @@ class CreateShortUrlTest extends ApiTestCase
     {
         $expectedDetail = sprintf('Provided URL %s is invalid. Try with a different one.', $url);
 
-        [$statusCode, $payload] = $this->createShortUrl(['longUrl' => $url]);
+        [$statusCode, $payload] = $this->createShortUrl(['longUrl' => $url, 'validateUrl' => true]);
 
         self::assertEquals(self::STATUS_BAD_REQUEST, $statusCode);
         self::assertEquals(self::STATUS_BAD_REQUEST, $payload['status']);
diff --git a/module/Rest/test-api/Action/EditShortUrlTest.php b/module/Rest/test-api/Action/EditShortUrlTest.php
index a25ccddd..92f9bbe0 100644
--- a/module/Rest/test-api/Action/EditShortUrlTest.php
+++ b/module/Rest/test-api/Action/EditShortUrlTest.php
@@ -82,6 +82,7 @@ class EditShortUrlTest extends ApiTestCase
 
         $resp = $this->callApiWithKey(self::METHOD_PATCH, $url, [RequestOptions::JSON => [
             'longUrl' => $longUrl,
+            'validateUrl' => true,
         ]]);
 
         self::assertEquals($expectedStatus, $resp->getStatusCode());