From 262c9a10f723c27fc03f5c5d89f30beb181789eb Mon Sep 17 00:00:00 2001 From: Binyamin Yawitz <316103+byawitz@users.noreply.github.com> Date: Thu, 18 Jul 2024 18:09:39 -0400 Subject: [PATCH 1/2] feat: Clean mail variables --- src/Appwrite/Event/Mail.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Appwrite/Event/Mail.php b/src/Appwrite/Event/Mail.php index 9bdbf6044d..dc65c6d36a 100644 --- a/src/Appwrite/Event/Mail.php +++ b/src/Appwrite/Event/Mail.php @@ -334,7 +334,7 @@ class Mail extends Event */ public function setVariables(array $variables): self { - $this->variables = $variables; + $this->variables = array_map(fn ($var) => strip_tags($var), $variables); return $this; } From 4eee8689d3a533a0ccf35e9f79a06f3dad0de304 Mon Sep 17 00:00:00 2001 From: Binyamin Yawitz <316103+byawitz@users.noreply.github.com> Date: Mon, 22 Jul 2024 09:37:28 -0400 Subject: [PATCH 2/2] fixes: Clean URL variables --- app/controllers/api/account.php | 4 +- app/controllers/api/teams.php | 1 + src/Appwrite/Event/Mail.php | 2 +- .../Services/Teams/TeamsCustomClientTest.php | 52 +++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 16538f68b6..bbd4d31420 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -1672,10 +1672,10 @@ App::post('/v1/account/tokens/magic-url') ->inject('queueForEvents') ->inject('queueForMails') ->action(function (string $userId, string $email, string $url, bool $phrase, Request $request, Response $response, Document $user, Document $project, Database $dbForProject, Locale $locale, Event $queueForEvents, Mail $queueForMails) { - if (empty(System::getEnv('_APP_SMTP_HOST'))) { throw new Exception(Exception::GENERAL_SMTP_DISABLED, 'SMTP disabled'); } + $url = htmlentities($url); if ($phrase === true) { $phrase = Phrase::generate(); @@ -2860,6 +2860,7 @@ App::post('/v1/account/recovery') if (empty(System::getEnv('_APP_SMTP_HOST'))) { throw new Exception(Exception::GENERAL_SMTP_DISABLED, 'SMTP Disabled'); } + $url = htmlentities($url); $roles = Authorization::getRoles(); $isPrivilegedUser = Auth::isPrivilegedUser($roles); @@ -3123,6 +3124,7 @@ App::post('/v1/account/verification') throw new Exception(Exception::GENERAL_SMTP_DISABLED, 'SMTP Disabled'); } + $url = htmlentities($url); if ($user->getAttribute('emailVerification')) { throw new Exception(Exception::USER_EMAIL_ALREADY_VERIFIED); } diff --git a/app/controllers/api/teams.php b/app/controllers/api/teams.php index 5891fb67f0..a4ef8c2a4c 100644 --- a/app/controllers/api/teams.php +++ b/app/controllers/api/teams.php @@ -401,6 +401,7 @@ App::post('/v1/teams/:teamId/memberships') $isAPIKey = Auth::isAppUser(Authorization::getRoles()); $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::getRoles()); + $url = htmlentities($url); if (empty($url)) { if (!$isAPIKey && !$isPrivilegedUser) { throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'URL is required'); diff --git a/src/Appwrite/Event/Mail.php b/src/Appwrite/Event/Mail.php index dc65c6d36a..9bdbf6044d 100644 --- a/src/Appwrite/Event/Mail.php +++ b/src/Appwrite/Event/Mail.php @@ -334,7 +334,7 @@ class Mail extends Event */ public function setVariables(array $variables): self { - $this->variables = array_map(fn ($var) => strip_tags($var), $variables); + $this->variables = $variables; return $this; } diff --git a/tests/e2e/Services/Teams/TeamsCustomClientTest.php b/tests/e2e/Services/Teams/TeamsCustomClientTest.php index 83a8080d1d..1de22f743f 100644 --- a/tests/e2e/Services/Teams/TeamsCustomClientTest.php +++ b/tests/e2e/Services/Teams/TeamsCustomClientTest.php @@ -2,6 +2,7 @@ namespace Tests\E2E\Services\Teams; +use Tests\E2E\Client; use Tests\E2E\Scopes\ProjectCustom; use Tests\E2E\Scopes\Scope; use Tests\E2E\Scopes\SideClient; @@ -12,4 +13,55 @@ class TeamsCustomClientTest extends Scope use TeamsBaseClient; use ProjectCustom; use SideClient; + + /** + * @depends testUpdateTeamMembership + */ + public function testTeamsInviteHTMLInjection($data): array + { + $teamUid = $data['teamUid'] ?? ''; + $email = uniqid() . 'friend@localhost.test'; + $name = 'Friend User'; + $password = 'password'; + + // Create a user account before we create a invite so we can check if the user has permissions when it shouldn't + $user = $this->client->call(Client::METHOD_POST, '/account', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => 'console'], [ + 'userId' => 'unique()', + 'email' => $email, + 'password' => $password, + 'name' => $name, + ], false); + + $this->assertEquals(201, $user['headers']['status-code']); + + $response = $this->client->call(Client::METHOD_POST, '/teams/' . $teamUid . '/memberships', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'email' => $email, + 'name' => $name, + 'roles' => ['admin', 'editor'], + 'url' => 'http://localhost:5000/join-us\">

INJECTED

' + ]); + + $this->assertEquals(201, $response['headers']['status-code']); + + $email = $this->getLastEmail(); + $encoded = 'http://localhost:5000/join-us\"></a><h1>INJECTED</h1>?'; + + $this->assertStringNotContainsString('

INJECTED

', $email['html']); + $this->assertStringContainsString($encoded, $email['html']); + $this->assertStringContainsString($encoded, $email['text']); + + $response = $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamUid . '/memberships/'.$response['body']['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + $this->assertEquals(204, $response['headers']['status-code']); + + + return $data; + } }