From 9ea8f3b59081ee71b25d19d5b8eec82a7d9e5ee4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 1 Feb 2022 19:12:53 +0100 Subject: [PATCH 1/6] Fixed URL validation still being true by default --- config/constants.php | 2 +- module/Core/src/Model/ShortUrlEdit.php | 6 ++-- module/Core/src/Model/ShortUrlMeta.php | 6 ++-- .../Core/src/Options/UrlShortenerOptions.php | 11 ------ .../Helper/TitleResolutionModelInterface.php | 2 +- module/Core/src/Util/UrlValidator.php | 9 ++--- .../Core/src/Util/UrlValidatorInterface.php | 4 +-- .../ShortUrlTitleResolutionHelperTest.php | 4 +-- module/Core/test/Util/UrlValidatorTest.php | 35 +++++-------------- .../test-api/Action/CreateShortUrlTest.php | 2 +- .../Rest/test-api/Action/EditShortUrlTest.php | 1 + 11 files changed, 26 insertions(+), 56 deletions(-) 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>/i'; // Matches the value inside an html title tag +const TITLE_TAG_VALUE = '/]*>(.*?)<\/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; } 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..51729743 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 { 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()); From d29c58dce5513cae9e8b216fe86e61de5818d85b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 1 Feb 2022 19:25:18 +0100 Subject: [PATCH 2/6] Unified test:unit:pretty command --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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", From d0fa6f7e03db01f316b82be105058124571ef63f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 1 Feb 2022 19:44:14 +0100 Subject: [PATCH 3/6] Added missing test covering URL validation with valid URL but title resolutio is disabled --- module/Core/test/Util/UrlValidatorTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index 51729743..55cb4d86 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -104,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 { From f22f50afa259f176518e82ddd3b699ec35651650 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 1 Feb 2022 19:46:36 +0100 Subject: [PATCH 4/6] Updated changelog --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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. From a1cd8baf3e0df172bf5ad242b4c76aea8c22f202 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 3 Feb 2022 22:03:20 +0100 Subject: [PATCH 5/6] Updated to stable pdo_sqlsrv in docker images --- .github/workflows/ci.yml | 2 +- CHANGELOG.md | 3 ++- Dockerfile | 2 +- data/infra/php.Dockerfile | 2 +- data/infra/swoole.Dockerfile | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d79e69a3..e6acf4e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,7 +80,7 @@ jobs: with: php-version: ${{ matrix.php-version }} tools: composer - extensions: openswoole-4.9.1, pdo_sqlsrv-5.10.0beta2 + extensions: openswoole-4.9.1, pdo_sqlsrv-5.10.0 coverage: pcov ini-values: pcov.directory=module - run: composer install --no-interaction --prefer-dist diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bfe5979..7a34363d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *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. +* [#1363](https://github.com/shlinkio/shlink/issues/1363) Fixed titles being resolved no matter what when `validateUrl` is not set or is explicitly set to true. +* [#1352](https://github.com/shlinkio/shlink/issues/1352) Updated to stable pdo_sqlsrv in docker image. ## [3.0.0] - 2022-01-28 diff --git a/Dockerfile b/Dockerfile index d72e6ca6..8ee2ffa9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,7 @@ FROM php:8.1.1-alpine3.15 as base ARG SHLINK_VERSION=latest ENV SHLINK_VERSION ${SHLINK_VERSION} ENV OPENSWOOLE_VERSION 4.9.1 -ENV PDO_SQLSRV_VERSION 5.10.0beta2 +ENV PDO_SQLSRV_VERSION 5.10.0 ENV MS_ODBC_SQL_VERSION 17.5.2.2 ENV LC_ALL "C" diff --git a/data/infra/php.Dockerfile b/data/infra/php.Dockerfile index 3380fd06..ee34034d 100644 --- a/data/infra/php.Dockerfile +++ b/data/infra/php.Dockerfile @@ -2,7 +2,7 @@ FROM php:8.1.1-fpm-alpine3.15 MAINTAINER Alejandro Celaya ENV APCU_VERSION 5.1.21 -ENV PDO_SQLSRV_VERSION 5.10.0beta2 +ENV PDO_SQLSRV_VERSION 5.10.0 ENV MS_ODBC_SQL_VERSION 17.5.2.2 RUN apk update diff --git a/data/infra/swoole.Dockerfile b/data/infra/swoole.Dockerfile index 2b86fb3e..24655a4f 100644 --- a/data/infra/swoole.Dockerfile +++ b/data/infra/swoole.Dockerfile @@ -4,7 +4,7 @@ MAINTAINER Alejandro Celaya ENV APCU_VERSION 5.1.21 ENV INOTIFY_VERSION 3.0.0 ENV OPENSWOOLE_VERSION 4.9.1 -ENV PDO_SQLSRV_VERSION 5.10.0beta2 +ENV PDO_SQLSRV_VERSION 5.10.0 ENV MS_ODBC_SQL_VERSION 17.5.2.2 RUN apk update From 5bf84144e7b20826ac9f64f82955ea920b98b4ab Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 4 Feb 2022 17:53:29 +0100 Subject: [PATCH 6/6] Tagged v3.0.1 in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a34363d..859d9e1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ 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] +## [3.0.1] - 2022-02-04 ### Added * *Nothing*