Merge pull request #1440 from acelaya-forks/feature/url-validation-memory-issue

Feature/url validation memory issue
This commit is contained in:
Alejandro Celaya 2022-05-01 12:14:52 +02:00 committed by GitHub
commit 2d51bd895d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 103 additions and 18 deletions

View file

@ -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
* [#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.

View file

@ -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",
@ -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",

View file

@ -11,8 +11,12 @@ use GuzzleHttp\RequestOptions;
use Psr\Http\Message\ResponseInterface;
use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
use Throwable;
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 +40,7 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
return;
}
$this->validateUrlAndGetResponse($url, true);
$this->validateUrlAndGetResponse($url);
}
public function validateUrlWithTitle(string $url, bool $doValidate): ?string
@ -45,30 +49,55 @@ 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);
$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]) ? trim($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;
}
}

View file

@ -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,7 +121,9 @@ 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);
@ -128,11 +132,46 @@ class UrlValidatorTest extends TestCase
$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 title</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;
}
}