diff --git a/lib/Controller/AvatarsController.php b/lib/Controller/AvatarsController.php index 431600bfb9..9446067d47 100644 --- a/lib/Controller/AvatarsController.php +++ b/lib/Controller/AvatarsController.php @@ -43,11 +43,8 @@ public function __construct(string $appName, */ #[TrapError] public function url(string $email): JSONResponse { - if (empty($email)) { - return new JSONResponse([], Http::STATUS_BAD_REQUEST); - } - - if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { + $email = $this->normalizeEmail($email); + if ($this->validateEmail($email) === false) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } @@ -55,19 +52,13 @@ public function url(string $email): JSONResponse { if (is_null($avatar)) { // No avatar found $response = new JSONResponse([], Http::STATUS_NO_CONTENT); - - // Debounce this a bit - // (cache for one day) - $response->cacheFor(24 * 60 * 60, false, true); - + $response->cacheFor(60 * 60, false, true); return $response; } $response = new JSONResponse($avatar); - // Let the browser cache this for a week $response->cacheFor(7 * 24 * 60 * 60, false, true); - return $response; } @@ -80,7 +71,8 @@ public function url(string $email): JSONResponse { */ #[TrapError] public function image(string $email): Response { - if (empty($email)) { + $email = $this->normalizeEmail($email); + if ($this->validateEmail($email) === false) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } @@ -89,23 +81,35 @@ public function image(string $email): Response { if (is_null($imageData) || !$avatar->isExternal()) { // This could happen if the cache invalidated meanwhile - return $this->noAvatarFoundResponse(); + $response = new Response(Http::STATUS_NO_CONTENT); + // Clear cache + $response->cacheFor(0); + return $response; } $resp = new AvatarDownloadResponse($image); $resp->addHeader('Content-Type', $avatar->getMime()); - // Let the browser cache this for a week $resp->cacheFor(7 * 24 * 60 * 60, false, true); - return $resp; } - private function noAvatarFoundResponse(): Response { - $response = new Response(); - $response->setStatus(Http::STATUS_NOT_FOUND); - // Clear cache - $response->cacheFor(0); - return $response; + private function normalizeEmail(string $email): string { + // remove single quotes and whitespace the might exist + // Examples: + // user1@example.com + // 'user1@example.com' + // ' user1@example.com' + return trim(trim($email, "'")); + } + + private function validateEmail(string $email): bool { + if (empty($email)) { + return false; + } + if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { + return false; + } + return true; } } diff --git a/tests/Unit/Controller/AvatarControllerTest.php b/tests/Unit/Controller/AvatarControllerTest.php index c566493cbe..1bd6ef2819 100644 --- a/tests/Unit/Controller/AvatarControllerTest.php +++ b/tests/Unit/Controller/AvatarControllerTest.php @@ -7,7 +7,6 @@ namespace OCA\Mail\Tests\Unit\Controller; -use ChristophWurst\Nextcloud\Testing\TestCase; use OCA\Mail\Contracts\IAvatarService; use OCA\Mail\Controller\AvatarsController; use OCA\Mail\Http\AvatarDownloadResponse; @@ -18,6 +17,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\IRequest; use PHPUnit_Framework_MockObject_MockObject; +use Test\TestCase; class AvatarControllerTest extends TestCase { /** @var IAvatarService|PHPUnit_Framework_MockObject_MockObject */ @@ -79,7 +79,7 @@ public function testGetUrlNoAvatarFound() { $resp = $this->controller->url($email); $expected = new JSONResponse([], Http::STATUS_NO_CONTENT); - $expected->cacheFor(24 * 60 * 60, false, true); + $expected->cacheFor(60 * 60, false, true); $this->assertEquals($expected, $resp); } @@ -108,8 +108,51 @@ public function testGetImageNotFound() { $resp = $this->controller->image($email); $expected = new Response(); - $expected->setStatus(Http::STATUS_NOT_FOUND); + $expected->setStatus(Http::STATUS_NO_CONTENT); $expected->cacheFor(0); $this->assertEquals($expected, $resp); } + + public static function normalizeEmailDataProvider(): array { + return [ + ['john@doe.com', 'john@doe.com'], + [' john@doe.com ', 'john@doe.com'], + [' john@doe.com', 'john@doe.com'], + ['john@doe.com ', 'john@doe.com'], + ["'john@doe.com'", 'john@doe.com'], + ["' john@doe.com'", 'john@doe.com'], + ["'john@doe.com '", 'john@doe.com'], + ]; + } + + /** + * @dataProvider normalizeEmailDataProvider + */ + public function testNormalizeEmail(string $email, string $expected): void { + $normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]); + $this->assertEquals($expected, $normalizedEmail); + } + + public static function validateEmailDataProvider(): array { + return [ + ['john@doe.com', true], + ['john.doe@doe.com', true], + ['john-doe@doe.com', true], + ['john@do-e.com', true], + ['', false], + ['john@@doe.com', false], + ['john@doe.com.', false], + ['john@doe..com', false], + ['johndoe.com', false], + ['john.doe.com', false], + ]; + } + + /** + * @dataProvider validateEmailDataProvider + */ + public function testValidateEmail(string $email, bool $expected) { + $isValid = $this->invokePrivate($this->controller, 'validateEmail', [$email]); + $this->assertEquals($expected, $isValid); + } }