From 84932b71472742f41d23dede2fdae45d26e04263 Mon Sep 17 00:00:00 2001 From: SebastianKrupinski Date: Tue, 8 Apr 2025 11:58:25 -0400 Subject: [PATCH 1/2] fix: quoted email addresses and status responses Signed-off-by: SebastianKrupinski --- lib/Controller/AvatarsController.php | 48 ++++++++++--------- .../Unit/Controller/AvatarControllerTest.php | 30 ++++++++++-- 2 files changed, 53 insertions(+), 25 deletions(-) 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..8927e8cd89 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,32 @@ 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 function testNormalizeEmailWithExtraSpaces() { + $email = ' john@doe.com '; + $normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]); + $this->assertEquals('john@doe.com', $normalizedEmail); + } + + public function testNormalizeEmailWithSingleQuotes() { + $email = "'john@doe.com'"; + $normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]); + $this->assertEquals('john@doe.com', $normalizedEmail); + } + + public function testValidateEmailWithValidEmail() { + $email = 'valid.email@example.com'; + $isValid = $this->invokePrivate($this->controller, 'validateEmail', [$email]); + $this->assertTrue($isValid); + } + + public function testValidateEmailWithInvalidEmail() { + $email = 'valid.email.example.com'; + $isValid = $this->invokePrivate($this->controller, 'validateEmail', [$email]); + $this->assertFalse($isValid); + } } From cab78991ace908b6a8959b833890eeaa8c7806c0 Mon Sep 17 00:00:00 2001 From: SebastianKrupinski Date: Tue, 8 Apr 2025 13:03:08 -0400 Subject: [PATCH 2/2] fixup! fix: quoted email addresses and status responses Signed-off-by: SebastianKrupinski --- .../Unit/Controller/AvatarControllerTest.php | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/tests/Unit/Controller/AvatarControllerTest.php b/tests/Unit/Controller/AvatarControllerTest.php index 8927e8cd89..1bd6ef2819 100644 --- a/tests/Unit/Controller/AvatarControllerTest.php +++ b/tests/Unit/Controller/AvatarControllerTest.php @@ -113,27 +113,46 @@ public function testGetImageNotFound() { $this->assertEquals($expected, $resp); } - public function testNormalizeEmailWithExtraSpaces() { - $email = ' john@doe.com '; - $normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]); - $this->assertEquals('john@doe.com', $normalizedEmail); + 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'], + ]; } - - public function testNormalizeEmailWithSingleQuotes() { - $email = "'john@doe.com'"; + + /** + * @dataProvider normalizeEmailDataProvider + */ + public function testNormalizeEmail(string $email, string $expected): void { $normalizedEmail = $this->invokePrivate($this->controller, 'normalizeEmail', [$email]); - $this->assertEquals('john@doe.com', $normalizedEmail); + $this->assertEquals($expected, $normalizedEmail); } - public function testValidateEmailWithValidEmail() { - $email = 'valid.email@example.com'; - $isValid = $this->invokePrivate($this->controller, 'validateEmail', [$email]); - $this->assertTrue($isValid); + 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], + ]; } - public function testValidateEmailWithInvalidEmail() { - $email = 'valid.email.example.com'; + /** + * @dataProvider validateEmailDataProvider + */ + public function testValidateEmail(string $email, bool $expected) { $isValid = $this->invokePrivate($this->controller, 'validateEmail', [$email]); - $this->assertFalse($isValid); + $this->assertEquals($expected, $isValid); } }