From d73b7a70d8d12f6772083b02abcec7c98cb9514f Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 11:44:39 +0530 Subject: [PATCH 01/15] feat: add query param fallback for impersonation headers Allow impersonation to be specified via URL query params (?impersonateUserId, ?impersonateEmail, ?impersonatePhone) as a fallback to the existing headers, enabling Console to embed impersonation in direct file/image URLs where headers cannot be set. --- app/init/realtime/connection.php | 6 +++--- app/init/resources/request.php | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index c0219fa816..0822ee9329 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -327,9 +327,9 @@ return function (Container $container): void { } } - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', ''); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', $request->getParam('impersonateEmail', '')); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', $request->getParam('impersonatePhone', '')); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = ($mode === APP_MODE_ADMIN || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 7d1731b80d..26c03126a2 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -571,10 +571,10 @@ return function (Container $container): void { } } - // Impersonation: if current user has impersonator capability and headers are set, act as another user - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', ''); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); + // Impersonation: if current user has impersonator capability and headers/params are set, act as another user + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', $request->getParam('impersonateEmail', '')); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', $request->getParam('impersonatePhone', '')); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = (APP_MODE_ADMIN === $mode || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; $targetUser = null; From 01b5fa8ecb0b7f12044bce25388f86c7b585d4d9 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 11:58:25 +0530 Subject: [PATCH 02/15] fix: restrict impersonation query param fallback to userId only Remove query param fallback for impersonateEmail and impersonatePhone to avoid PII exposure in server logs, browser history, and Referer headers. Only impersonateUserId (an opaque internal ID) is safe to pass via URL query param. --- app/init/realtime/connection.php | 4 ++-- app/init/resources/request.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 0822ee9329..1f6faed0fd 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -328,8 +328,8 @@ return function (Container $container): void { } $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', $request->getParam('impersonateEmail', '')); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', $request->getParam('impersonatePhone', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = ($mode === APP_MODE_ADMIN || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 26c03126a2..8a74f7763b 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -573,8 +573,8 @@ return function (Container $container): void { // Impersonation: if current user has impersonator capability and headers/params are set, act as another user $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', $request->getParam('impersonateEmail', '')); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', $request->getParam('impersonatePhone', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = (APP_MODE_ADMIN === $mode || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; $targetUser = null; From 8f1d73a6cb7d2368589d0c9f073fc99fdb03f665 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 12:02:00 +0530 Subject: [PATCH 03/15] chore: clarify intentional header-only restriction for email/phone impersonation --- app/init/realtime/connection.php | 2 ++ app/init/resources/request.php | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 1f6faed0fd..b557a2c62b 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -327,6 +327,8 @@ return function (Container $container): void { } } + // impersonateUserId also accepts a query param to support embedding in WebSocket URLs. + // Email and phone are intentionally header-only to avoid PII exposure in proxy/LB logs. $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 8a74f7763b..c6f3fd1ab1 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -572,6 +572,8 @@ return function (Container $container): void { } // Impersonation: if current user has impersonator capability and headers/params are set, act as another user + // impersonateUserId also accepts a query param to allow embedding in direct file/image URLs (e.g. ) + // where custom headers cannot be set. Email and phone are intentionally header-only to avoid PII in URLs/logs. $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); From 4c989f99c37043c0b9dafd877e3d74f239c2d160 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 12:05:02 +0530 Subject: [PATCH 04/15] fix: cast impersonateUserId query param to string to prevent array injection --- app/init/realtime/connection.php | 2 +- app/init/resources/request.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index b557a2c62b..c02da3058e 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -329,7 +329,7 @@ return function (Container $container): void { // impersonateUserId also accepts a query param to support embedding in WebSocket URLs. // Email and phone are intentionally header-only to avoid PII exposure in proxy/LB logs. - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index c6f3fd1ab1..d1c0d2bea0 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -574,7 +574,7 @@ return function (Container $container): void { // Impersonation: if current user has impersonator capability and headers/params are set, act as another user // impersonateUserId also accepts a query param to allow embedding in direct file/image URLs (e.g. ) // where custom headers cannot be set. Email and phone are intentionally header-only to avoid PII in URLs/logs. - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $request->getParam('impersonateUserId', '')); + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { From 46a457bfa37960ecf28f59baeb244077e19cbe21 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 12:10:51 +0530 Subject: [PATCH 05/15] fix: block impersonateUserId query param on cross-site requests to prevent CSRF --- app/init/realtime/connection.php | 5 ++++- app/init/resources/request.php | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index c02da3058e..3bb91a3aeb 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -329,7 +329,10 @@ return function (Container $container): void { // impersonateUserId also accepts a query param to support embedding in WebSocket URLs. // Email and phone are intentionally header-only to avoid PII exposure in proxy/LB logs. - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); + // Query-param fallback is blocked for cross-site requests to prevent CSRF attacks via + // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. + $isCrossSite = $request->getHeader('sec-fetch-site', '') === 'cross-site'; + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isCrossSite ? '' : (string)$request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index d1c0d2bea0..143adca352 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -574,7 +574,10 @@ return function (Container $container): void { // Impersonation: if current user has impersonator capability and headers/params are set, act as another user // impersonateUserId also accepts a query param to allow embedding in direct file/image URLs (e.g. ) // where custom headers cannot be set. Email and phone are intentionally header-only to avoid PII in URLs/logs. - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); + // Query-param fallback is blocked for cross-site requests (Sec-Fetch-Site: cross-site) to prevent CSRF; + // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. + $isCrossSite = $request->getHeader('sec-fetch-site', '') === 'cross-site'; + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isCrossSite ? '' : (string)$request->getParam('impersonateUserId', '')); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { From 5465be6301a3a5b0236bc0c8b9c0d93b822260a5 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 12:27:57 +0530 Subject: [PATCH 06/15] fix: make CSRF guard fail-closed by requiring explicit same-origin Sec-Fetch-Site --- app/init/realtime/connection.php | 5 +++-- app/init/resources/request.php | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 3bb91a3aeb..0fc30fb5e2 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -331,8 +331,9 @@ return function (Container $container): void { // Email and phone are intentionally header-only to avoid PII exposure in proxy/LB logs. // Query-param fallback is blocked for cross-site requests to prevent CSRF attacks via // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. - $isCrossSite = $request->getHeader('sec-fetch-site', '') === 'cross-site'; - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isCrossSite ? '' : (string)$request->getParam('impersonateUserId', '')); + $fetchSite = $request->getHeader('sec-fetch-site', ''); + $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 143adca352..7b29c05c5d 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -576,8 +576,9 @@ return function (Container $container): void { // where custom headers cannot be set. Email and phone are intentionally header-only to avoid PII in URLs/logs. // Query-param fallback is blocked for cross-site requests (Sec-Fetch-Site: cross-site) to prevent CSRF; // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. - $isCrossSite = $request->getHeader('sec-fetch-site', '') === 'cross-site'; - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isCrossSite ? '' : (string)$request->getParam('impersonateUserId', '')); + $fetchSite = $request->getHeader('sec-fetch-site', ''); + $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { From 9a175c509897e8264974bb924bef6f4286ffd6e1 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 12:56:17 +0530 Subject: [PATCH 07/15] test: add E2E tests for impersonateUserId query param and CSRF guards --- tests/e2e/Services/Users/UsersBase.php | 152 +++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 3255d9a67f..a4567f0063 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2708,6 +2708,158 @@ trait UsersBase $this->assertIsArray($response['body']['users']); } + /** + * Test impersonation via ?impersonateUserId= query param (same-origin browser request). + * This is the primary use case for embedding impersonation in file/image URLs where + * custom headers cannot be set (e.g. , deployment source/output download links). + */ + public function testImpersonateByUserIdQueryParam(): void + { + $projectId = $this->getProject()['$id']; + $headers = array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + ], $this->getHeaders()); + + $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'queryparam-impersonator@appwrite.io', + 'password' => 'password', + 'name' => 'Query Param Impersonator', + ]); + $this->assertEquals(201, $userA['headers']['status-code']); + $idA = $userA['body']['$id']; + + $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'queryparam-target@appwrite.io', + 'password' => 'password', + 'name' => 'Query Param Target', + ]); + $this->assertEquals(201, $userB['headers']['status-code']); + $idB = $userB['body']['$id']; + + $patch = $this->client->call(Client::METHOD_PATCH, '/users/' . $idA . '/impersonator', $headers, ['impersonator' => true]); + $this->assertEquals(200, $patch['headers']['status-code']); + + $session = $this->client->call(Client::METHOD_POST, '/users/' . $idA . '/sessions', $headers); + $this->assertEquals(201, $session['headers']['status-code']); + $sessionSecret = $session['body']['secret']; + + // Query param works when Sec-Fetch-Site indicates a same-origin browser request + $account = $this->client->call(Client::METHOD_GET, '/account', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + 'x-appwrite-session' => $sessionSecret, + 'sec-fetch-site' => 'same-origin', + ], ['impersonateUserId' => $idB]); + $this->assertEquals(200, $account['headers']['status-code']); + $this->assertEquals($idB, $account['body']['$id']); + $this->assertEquals('Query Param Target', $account['body']['name']); + $this->assertEquals($idA, $account['body']['impersonatorUserId']); + } + + /** + * Test that ?impersonateUserId= query param is ignored for cross-site requests (CSRF guard). + * Sec-Fetch-Site is a browser-enforced forbidden header; cross-site value means the request + * originated from a third-party page and must not be allowed to trigger impersonation. + */ + public function testImpersonateQueryParamIgnoredCrossSite(): void + { + $projectId = $this->getProject()['$id']; + $headers = array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + ], $this->getHeaders()); + + $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'csrf-impersonator@appwrite.io', + 'password' => 'password', + 'name' => 'CSRF Impersonator', + ]); + $this->assertEquals(201, $userA['headers']['status-code']); + $idA = $userA['body']['$id']; + + $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'csrf-target@appwrite.io', + 'password' => 'password', + 'name' => 'CSRF Target', + ]); + $this->assertEquals(201, $userB['headers']['status-code']); + $idB = $userB['body']['$id']; + + $patch = $this->client->call(Client::METHOD_PATCH, '/users/' . $idA . '/impersonator', $headers, ['impersonator' => true]); + $this->assertEquals(200, $patch['headers']['status-code']); + + $session = $this->client->call(Client::METHOD_POST, '/users/' . $idA . '/sessions', $headers); + $this->assertEquals(201, $session['headers']['status-code']); + $sessionSecret = $session['body']['secret']; + + // Query param must be ignored when Sec-Fetch-Site is cross-site (third-party page embed) + $account = $this->client->call(Client::METHOD_GET, '/account', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + 'x-appwrite-session' => $sessionSecret, + 'sec-fetch-site' => 'cross-site', + ], ['impersonateUserId' => $idB]); + $this->assertEquals(200, $account['headers']['status-code']); + // Should resolve as userA (the impersonator), not the target + $this->assertEquals($idA, $account['body']['$id']); + $this->assertArrayNotHasKey('impersonatorUserId', $account['body']); + } + + /** + * Test that ?impersonateUserId= query param is ignored when Sec-Fetch-Site is absent + * (fail-closed CSRF guard). Absent header means a reverse proxy stripped Fetch Metadata + * headers or a non-browser client is calling — query param must be silently ignored. + */ + public function testImpersonateQueryParamIgnoredWhenSecFetchSiteAbsent(): void + { + $projectId = $this->getProject()['$id']; + $headers = array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + ], $this->getHeaders()); + + $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'absent-fetch-impersonator@appwrite.io', + 'password' => 'password', + 'name' => 'Absent Fetch Impersonator', + ]); + $this->assertEquals(201, $userA['headers']['status-code']); + $idA = $userA['body']['$id']; + + $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'absent-fetch-target@appwrite.io', + 'password' => 'password', + 'name' => 'Absent Fetch Target', + ]); + $this->assertEquals(201, $userB['headers']['status-code']); + $idB = $userB['body']['$id']; + + $patch = $this->client->call(Client::METHOD_PATCH, '/users/' . $idA . '/impersonator', $headers, ['impersonator' => true]); + $this->assertEquals(200, $patch['headers']['status-code']); + + $session = $this->client->call(Client::METHOD_POST, '/users/' . $idA . '/sessions', $headers); + $this->assertEquals(201, $session['headers']['status-code']); + $sessionSecret = $session['body']['secret']; + + // Query param must be ignored when Sec-Fetch-Site is absent (proxy-stripped or API client) + $account = $this->client->call(Client::METHOD_GET, '/account', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + 'x-appwrite-session' => $sessionSecret, + // no sec-fetch-site header + ], ['impersonateUserId' => $idB]); + $this->assertEquals(200, $account['headers']['status-code']); + $this->assertEquals($idA, $account['body']['$id']); + $this->assertArrayNotHasKey('impersonatorUserId', $account['body']); + } + /** * Test PATCH /users/:userId/impersonator for non-existent user returns 404 */ From a3f6cf4645cf5680fc237b3e9a17472b4c986e3c Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 13:00:18 +0530 Subject: [PATCH 08/15] fix: restrict CSRF guard to same-origin only, drop same-site --- app/init/realtime/connection.php | 2 +- app/init/resources/request.php | 2 +- tests/e2e/Services/Users/UsersBase.php | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 0fc30fb5e2..5778b5c260 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -332,7 +332,7 @@ return function (Container $container): void { // Query-param fallback is blocked for cross-site requests to prevent CSRF attacks via // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. $fetchSite = $request->getHeader('sec-fetch-site', ''); - $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); + $isSameOrigin = $fetchSite === 'same-origin'; $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 7b29c05c5d..dca4b84bd7 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -577,7 +577,7 @@ return function (Container $container): void { // Query-param fallback is blocked for cross-site requests (Sec-Fetch-Site: cross-site) to prevent CSRF; // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. $fetchSite = $request->getHeader('sec-fetch-site', ''); - $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); + $isSameOrigin = $fetchSite === 'same-origin'; $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index a4567f0063..5f38df5c07 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2746,7 +2746,8 @@ trait UsersBase $this->assertEquals(201, $session['headers']['status-code']); $sessionSecret = $session['body']['secret']; - // Query param works when Sec-Fetch-Site indicates a same-origin browser request + // Query param works only when Sec-Fetch-Site is exactly same-origin. + // same-site is intentionally excluded to prevent subdomain-based CSRF attacks. $account = $this->client->call(Client::METHOD_GET, '/account', [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, From ed0c7b4e129ba10171006e745862a19546c45837 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 13:24:15 +0530 Subject: [PATCH 09/15] test: add CSRF attack prevention test for impersonateUserId query param --- tests/e2e/Services/Users/UsersBase.php | 100 +++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 5f38df5c07..069a2eab48 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2708,6 +2708,106 @@ trait UsersBase $this->assertIsArray($response['body']['users']); } + /** + * Proves that the Sec-Fetch-Site CSRF guard prevents forced impersonation via query params. + * + * Attack scenario (without the guard): + * A malicious page on attacker.com embeds: + * + * The browser automatically attaches the impersonator's session cookies. + * Without any guard, the server would impersonate victim_id silently. + * + * Why Sec-Fetch-Site works: + * Browsers set Sec-Fetch-Site: cross-site on all cross-origin requests (img, fetch, etc.). + * It is a browser-enforced forbidden header — JavaScript cannot set or spoof it. + * We only accept ?impersonateUserId when Sec-Fetch-Site is exactly same-origin. + * + * This test proves three attack vectors are all blocked: + * 1. cross-site — attacker.com embeds pointing at Appwrite + * 2. same-site — attacker controls a subdomain (e.g. evil.appwrite.io) + * 3. absent — reverse proxy strips Fetch Metadata headers (fail-closed) + */ + public function testImpersonateQueryParamCsrfAttackPrevented(): void + { + $projectId = $this->getProject()['$id']; + $headers = array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + ], $this->getHeaders()); + + // Impersonator user (the victim whose session gets hijacked in the attack) + $impersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'csrf-guard-impersonator@appwrite.io', + 'password' => 'password', + 'name' => 'CSRF Guard Impersonator', + ]); + $this->assertEquals(201, $impersonator['headers']['status-code']); + $impersonatorId = $impersonator['body']['$id']; + + // Target user (who the attacker wants to impersonate) + $target = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + 'userId' => ID::unique(), + 'email' => 'csrf-guard-target@appwrite.io', + 'password' => 'password', + 'name' => 'CSRF Guard Target', + ]); + $this->assertEquals(201, $target['headers']['status-code']); + $targetId = $target['body']['$id']; + + $this->client->call(Client::METHOD_PATCH, '/users/' . $impersonatorId . '/impersonator', $headers, ['impersonator' => true]); + + $session = $this->client->call(Client::METHOD_POST, '/users/' . $impersonatorId . '/sessions', $headers); + $this->assertEquals(201, $session['headers']['status-code']); + $sessionSecret = $session['body']['secret']; + + $sessionHeaders = [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + 'x-appwrite-session' => $sessionSecret, + ]; + + // Attack vector 1: cross-site (attacker.com embeds ) + // Browser sends Sec-Fetch-Site: cross-site — must be blocked. + $crossSite = $this->client->call(Client::METHOD_GET, '/account', + array_merge($sessionHeaders, ['sec-fetch-site' => 'cross-site']), + ['impersonateUserId' => $targetId] + ); + $this->assertEquals(200, $crossSite['headers']['status-code']); + $this->assertEquals($impersonatorId, $crossSite['body']['$id'], 'cross-site: impersonation must be blocked'); + $this->assertArrayNotHasKey('impersonatorUserId', $crossSite['body']); + + // Attack vector 2: same-site (attacker controls evil.appwrite.io subdomain) + // Browser sends Sec-Fetch-Site: same-site — must also be blocked. + $sameSite = $this->client->call(Client::METHOD_GET, '/account', + array_merge($sessionHeaders, ['sec-fetch-site' => 'same-site']), + ['impersonateUserId' => $targetId] + ); + $this->assertEquals(200, $sameSite['headers']['status-code']); + $this->assertEquals($impersonatorId, $sameSite['body']['$id'], 'same-site: subdomain attack must be blocked'); + $this->assertArrayNotHasKey('impersonatorUserId', $sameSite['body']); + + // Attack vector 3: absent header (reverse proxy strips Fetch Metadata headers) + // Guard must fail-closed — absent Sec-Fetch-Site must not allow query param. + $noFetchSite = $this->client->call(Client::METHOD_GET, '/account', + $sessionHeaders, + ['impersonateUserId' => $targetId] + ); + $this->assertEquals(200, $noFetchSite['headers']['status-code']); + $this->assertEquals($impersonatorId, $noFetchSite['body']['$id'], 'absent header: must fail-closed'); + $this->assertArrayNotHasKey('impersonatorUserId', $noFetchSite['body']); + + // Legitimate use: same-origin (Console loading a file URL with impersonation embedded) + // Browser sends Sec-Fetch-Site: same-origin — must succeed. + $sameOrigin = $this->client->call(Client::METHOD_GET, '/account', + array_merge($sessionHeaders, ['sec-fetch-site' => 'same-origin']), + ['impersonateUserId' => $targetId] + ); + $this->assertEquals(200, $sameOrigin['headers']['status-code']); + $this->assertEquals($targetId, $sameOrigin['body']['$id'], 'same-origin: impersonation must succeed'); + $this->assertEquals($impersonatorId, $sameOrigin['body']['impersonatorUserId']); + } + /** * Test impersonation via ?impersonateUserId= query param (same-origin browser request). * This is the primary use case for embedding impersonation in file/image URLs where From 5afc8f462ddbd6e4466b5467693b57300c75e95e Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 13:26:13 +0530 Subject: [PATCH 10/15] fix: allow same-site in CSRF guard to support Console on subdomains --- app/init/realtime/connection.php | 5 +++- app/init/resources/request.php | 5 +++- tests/e2e/Services/Users/UsersBase.php | 37 +++++++++++++------------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 5778b5c260..c6593927d9 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -332,7 +332,10 @@ return function (Container $container): void { // Query-param fallback is blocked for cross-site requests to prevent CSRF attacks via // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. $fetchSite = $request->getHeader('sec-fetch-site', ''); - $isSameOrigin = $fetchSite === 'same-origin'; + // Allow same-origin and same-site: Console may be served from a different subdomain + // (e.g. vibes.appwrite.io) than the API, in which case the browser sends same-site. + // cross-site and absent are blocked to prevent CSRF via third-party embeds. + $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index dca4b84bd7..760b9d598e 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -577,7 +577,10 @@ return function (Container $container): void { // Query-param fallback is blocked for cross-site requests (Sec-Fetch-Site: cross-site) to prevent CSRF; // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. $fetchSite = $request->getHeader('sec-fetch-site', ''); - $isSameOrigin = $fetchSite === 'same-origin'; + // Allow same-origin and same-site: Console may be served from a different subdomain + // (e.g. vibes.appwrite.io) than the API, in which case the browser sends same-site. + // cross-site and absent are blocked to prevent CSRF via third-party embeds. + $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 069a2eab48..623e8cc3ec 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2722,10 +2722,11 @@ trait UsersBase * It is a browser-enforced forbidden header — JavaScript cannot set or spoof it. * We only accept ?impersonateUserId when Sec-Fetch-Site is exactly same-origin. * - * This test proves three attack vectors are all blocked: - * 1. cross-site — attacker.com embeds pointing at Appwrite - * 2. same-site — attacker controls a subdomain (e.g. evil.appwrite.io) - * 3. absent — reverse proxy strips Fetch Metadata headers (fail-closed) + * This test proves two attack vectors are blocked and two legitimate origins are allowed: + * Blocked: cross-site — attacker.com embeds pointing at Appwrite + * Blocked: absent — reverse proxy strips Fetch Metadata headers (fail-closed) + * Allowed: same-origin — Console on the same origin as the API + * Allowed: same-site — Console on a subdomain (e.g. vibes.appwrite.io vs appwrite.io) */ public function testImpersonateQueryParamCsrfAttackPrevented(): void { @@ -2777,17 +2778,7 @@ trait UsersBase $this->assertEquals($impersonatorId, $crossSite['body']['$id'], 'cross-site: impersonation must be blocked'); $this->assertArrayNotHasKey('impersonatorUserId', $crossSite['body']); - // Attack vector 2: same-site (attacker controls evil.appwrite.io subdomain) - // Browser sends Sec-Fetch-Site: same-site — must also be blocked. - $sameSite = $this->client->call(Client::METHOD_GET, '/account', - array_merge($sessionHeaders, ['sec-fetch-site' => 'same-site']), - ['impersonateUserId' => $targetId] - ); - $this->assertEquals(200, $sameSite['headers']['status-code']); - $this->assertEquals($impersonatorId, $sameSite['body']['$id'], 'same-site: subdomain attack must be blocked'); - $this->assertArrayNotHasKey('impersonatorUserId', $sameSite['body']); - - // Attack vector 3: absent header (reverse proxy strips Fetch Metadata headers) + // Attack vector 2: absent header (reverse proxy strips Fetch Metadata headers) // Guard must fail-closed — absent Sec-Fetch-Site must not allow query param. $noFetchSite = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, @@ -2797,8 +2788,7 @@ trait UsersBase $this->assertEquals($impersonatorId, $noFetchSite['body']['$id'], 'absent header: must fail-closed'); $this->assertArrayNotHasKey('impersonatorUserId', $noFetchSite['body']); - // Legitimate use: same-origin (Console loading a file URL with impersonation embedded) - // Browser sends Sec-Fetch-Site: same-origin — must succeed. + // Legitimate use 1: same-origin (Console on same origin as API) $sameOrigin = $this->client->call(Client::METHOD_GET, '/account', array_merge($sessionHeaders, ['sec-fetch-site' => 'same-origin']), ['impersonateUserId' => $targetId] @@ -2806,6 +2796,15 @@ trait UsersBase $this->assertEquals(200, $sameOrigin['headers']['status-code']); $this->assertEquals($targetId, $sameOrigin['body']['$id'], 'same-origin: impersonation must succeed'); $this->assertEquals($impersonatorId, $sameOrigin['body']['impersonatorUserId']); + + // Legitimate use 2: same-site (Console on subdomain, e.g. vibes.appwrite.io vs appwrite.io) + $sameSite = $this->client->call(Client::METHOD_GET, '/account', + array_merge($sessionHeaders, ['sec-fetch-site' => 'same-site']), + ['impersonateUserId' => $targetId] + ); + $this->assertEquals(200, $sameSite['headers']['status-code']); + $this->assertEquals($targetId, $sameSite['body']['$id'], 'same-site: impersonation must succeed'); + $this->assertEquals($impersonatorId, $sameSite['body']['impersonatorUserId']); } /** @@ -2846,8 +2845,8 @@ trait UsersBase $this->assertEquals(201, $session['headers']['status-code']); $sessionSecret = $session['body']['secret']; - // Query param works only when Sec-Fetch-Site is exactly same-origin. - // same-site is intentionally excluded to prevent subdomain-based CSRF attacks. + // Query param works when Sec-Fetch-Site is same-origin or same-site. + // same-site covers Console deployed on a subdomain (e.g. vibes.appwrite.io). $account = $this->client->call(Client::METHOD_GET, '/account', [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, From 3dd5a51ba497d9f0e7a428001b2148f697bdc9b4 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 13:34:01 +0530 Subject: [PATCH 11/15] style: fix method argument spacing (Pint PSR-12) --- tests/e2e/Services/Users/UsersBase.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 623e8cc3ec..862e858422 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2770,7 +2770,9 @@ trait UsersBase // Attack vector 1: cross-site (attacker.com embeds ) // Browser sends Sec-Fetch-Site: cross-site — must be blocked. - $crossSite = $this->client->call(Client::METHOD_GET, '/account', + $crossSite = $this->client->call( + Client::METHOD_GET, + '/account', array_merge($sessionHeaders, ['sec-fetch-site' => 'cross-site']), ['impersonateUserId' => $targetId] ); @@ -2780,7 +2782,9 @@ trait UsersBase // Attack vector 2: absent header (reverse proxy strips Fetch Metadata headers) // Guard must fail-closed — absent Sec-Fetch-Site must not allow query param. - $noFetchSite = $this->client->call(Client::METHOD_GET, '/account', + $noFetchSite = $this->client->call( + Client::METHOD_GET, + '/account', $sessionHeaders, ['impersonateUserId' => $targetId] ); @@ -2789,7 +2793,9 @@ trait UsersBase $this->assertArrayNotHasKey('impersonatorUserId', $noFetchSite['body']); // Legitimate use 1: same-origin (Console on same origin as API) - $sameOrigin = $this->client->call(Client::METHOD_GET, '/account', + $sameOrigin = $this->client->call( + Client::METHOD_GET, + '/account', array_merge($sessionHeaders, ['sec-fetch-site' => 'same-origin']), ['impersonateUserId' => $targetId] ); @@ -2798,7 +2804,9 @@ trait UsersBase $this->assertEquals($impersonatorId, $sameOrigin['body']['impersonatorUserId']); // Legitimate use 2: same-site (Console on subdomain, e.g. vibes.appwrite.io vs appwrite.io) - $sameSite = $this->client->call(Client::METHOD_GET, '/account', + $sameSite = $this->client->call( + Client::METHOD_GET, + '/account', array_merge($sessionHeaders, ['sec-fetch-site' => 'same-site']), ['impersonateUserId' => $targetId] ); From bda823ac0e5923e57c478bb844ab3eac85b7a593 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 13:38:00 +0530 Subject: [PATCH 12/15] chore: format --- app/init/realtime/connection.php | 2 +- app/init/resources/request.php | 2 +- tests/e2e/Services/Users/UsersBase.php | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index c6593927d9..03dfdc4fd7 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -333,7 +333,7 @@ return function (Container $container): void { // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. $fetchSite = $request->getHeader('sec-fetch-site', ''); // Allow same-origin and same-site: Console may be served from a different subdomain - // (e.g. vibes.appwrite.io) than the API, in which case the browser sends same-site. + // than the API, in which case the browser sends same-site. // cross-site and absent are blocked to prevent CSRF via third-party embeds. $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); diff --git a/app/init/resources/request.php b/app/init/resources/request.php index 760b9d598e..c0097a2416 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -578,7 +578,7 @@ return function (Container $container): void { // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. $fetchSite = $request->getHeader('sec-fetch-site', ''); // Allow same-origin and same-site: Console may be served from a different subdomain - // (e.g. vibes.appwrite.io) than the API, in which case the browser sends same-site. + // than the API, in which case the browser sends same-site. // cross-site and absent are blocked to prevent CSRF via third-party embeds. $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 862e858422..d5c06e9f8d 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2726,7 +2726,7 @@ trait UsersBase * Blocked: cross-site — attacker.com embeds pointing at Appwrite * Blocked: absent — reverse proxy strips Fetch Metadata headers (fail-closed) * Allowed: same-origin — Console on the same origin as the API - * Allowed: same-site — Console on a subdomain (e.g. vibes.appwrite.io vs appwrite.io) + * Allowed: same-site — Console on a different subdomain than the API */ public function testImpersonateQueryParamCsrfAttackPrevented(): void { @@ -2803,7 +2803,7 @@ trait UsersBase $this->assertEquals($targetId, $sameOrigin['body']['$id'], 'same-origin: impersonation must succeed'); $this->assertEquals($impersonatorId, $sameOrigin['body']['impersonatorUserId']); - // Legitimate use 2: same-site (Console on subdomain, e.g. vibes.appwrite.io vs appwrite.io) + // Legitimate use 2: same-site (Console on a different subdomain than the API) $sameSite = $this->client->call( Client::METHOD_GET, '/account', @@ -2854,7 +2854,7 @@ trait UsersBase $sessionSecret = $session['body']['secret']; // Query param works when Sec-Fetch-Site is same-origin or same-site. - // same-site covers Console deployed on a subdomain (e.g. vibes.appwrite.io). + // same-site covers Console deployed on a different subdomain than the API. $account = $this->client->call(Client::METHOD_GET, '/account', [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, From f0cbfbbbe4fc4b157844c9af67b1b29c7e56a16a Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 14:31:49 +0530 Subject: [PATCH 13/15] fix: use assertEmpty for impersonatorUserId to match response model --- tests/e2e/Services/Users/UsersBase.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index d5c06e9f8d..70e74648b4 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2778,7 +2778,7 @@ trait UsersBase ); $this->assertEquals(200, $crossSite['headers']['status-code']); $this->assertEquals($impersonatorId, $crossSite['body']['$id'], 'cross-site: impersonation must be blocked'); - $this->assertArrayNotHasKey('impersonatorUserId', $crossSite['body']); + $this->assertEmpty($crossSite['body']['impersonatorUserId'] ?? ''); // Attack vector 2: absent header (reverse proxy strips Fetch Metadata headers) // Guard must fail-closed — absent Sec-Fetch-Site must not allow query param. @@ -2790,7 +2790,7 @@ trait UsersBase ); $this->assertEquals(200, $noFetchSite['headers']['status-code']); $this->assertEquals($impersonatorId, $noFetchSite['body']['$id'], 'absent header: must fail-closed'); - $this->assertArrayNotHasKey('impersonatorUserId', $noFetchSite['body']); + $this->assertEmpty($noFetchSite['body']['impersonatorUserId'] ?? ''); // Legitimate use 1: same-origin (Console on same origin as API) $sameOrigin = $this->client->call( @@ -2915,7 +2915,7 @@ trait UsersBase $this->assertEquals(200, $account['headers']['status-code']); // Should resolve as userA (the impersonator), not the target $this->assertEquals($idA, $account['body']['$id']); - $this->assertArrayNotHasKey('impersonatorUserId', $account['body']); + $this->assertEmpty($account['body']['impersonatorUserId'] ?? ''); } /** @@ -2965,7 +2965,7 @@ trait UsersBase ], ['impersonateUserId' => $idB]); $this->assertEquals(200, $account['headers']['status-code']); $this->assertEquals($idA, $account['body']['$id']); - $this->assertArrayNotHasKey('impersonatorUserId', $account['body']); + $this->assertEmpty($account['body']['impersonatorUserId'] ?? ''); } /** From 87ed7c3817c1878eb900bc3f0bd30fbf4451122c Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 19:10:55 +0530 Subject: [PATCH 14/15] feat: add query param fallback for all impersonation params and simplify tests --- app/init/realtime/connection.php | 17 +- app/init/resources/request.php | 17 +- tests/e2e/Services/Users/UsersBase.php | 245 ++++--------------------- 3 files changed, 48 insertions(+), 231 deletions(-) diff --git a/app/init/realtime/connection.php b/app/init/realtime/connection.php index 03dfdc4fd7..a090635bb5 100644 --- a/app/init/realtime/connection.php +++ b/app/init/realtime/connection.php @@ -327,18 +327,11 @@ return function (Container $container): void { } } - // impersonateUserId also accepts a query param to support embedding in WebSocket URLs. - // Email and phone are intentionally header-only to avoid PII exposure in proxy/LB logs. - // Query-param fallback is blocked for cross-site requests to prevent CSRF attacks via - // third-party pages; Sec-Fetch-Site is a browser-enforced forbidden header. - $fetchSite = $request->getHeader('sec-fetch-site', ''); - // Allow same-origin and same-site: Console may be served from a different subdomain - // than the API, in which case the browser sends same-site. - // cross-site and absent are blocked to prevent CSRF via third-party embeds. - $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); + // Query params mirror the header fallback pattern used by ?project= and ?devKey=, + // allowing Console to embed impersonation in direct file/image URLs where headers cannot be set. + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', (string)$request->getParam('impersonateEmail', '')); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', (string)$request->getParam('impersonatePhone', '')); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = ($mode === APP_MODE_ADMIN || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; diff --git a/app/init/resources/request.php b/app/init/resources/request.php index c0097a2416..1aa53b7403 100644 --- a/app/init/resources/request.php +++ b/app/init/resources/request.php @@ -572,18 +572,11 @@ return function (Container $container): void { } // Impersonation: if current user has impersonator capability and headers/params are set, act as another user - // impersonateUserId also accepts a query param to allow embedding in direct file/image URLs (e.g. ) - // where custom headers cannot be set. Email and phone are intentionally header-only to avoid PII in URLs/logs. - // Query-param fallback is blocked for cross-site requests (Sec-Fetch-Site: cross-site) to prevent CSRF; - // Sec-Fetch-Site is a browser-enforced forbidden header that cannot be spoofed by JavaScript. - $fetchSite = $request->getHeader('sec-fetch-site', ''); - // Allow same-origin and same-site: Console may be served from a different subdomain - // than the API, in which case the browser sends same-site. - // cross-site and absent are blocked to prevent CSRF via third-party embeds. - $isSameOrigin = \in_array($fetchSite, ['same-origin', 'same-site'], true); - $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', $isSameOrigin ? (string)$request->getParam('impersonateUserId', '') : ''); - $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', ''); - $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', ''); + // Query params mirror the header fallback pattern used by ?project= and ?devKey=, + // allowing Console to embed impersonation in direct file/image URLs where headers cannot be set. + $impersonateUserId = $request->getHeader('x-appwrite-impersonate-user-id', (string)$request->getParam('impersonateUserId', '')); + $impersonateEmail = $request->getHeader('x-appwrite-impersonate-user-email', (string)$request->getParam('impersonateEmail', '')); + $impersonatePhone = $request->getHeader('x-appwrite-impersonate-user-phone', (string)$request->getParam('impersonatePhone', '')); if (!$user->isEmpty() && $user->getAttribute('impersonator', false)) { $userDb = (APP_MODE_ADMIN === $mode || $project->getId() === 'console') ? $dbForPlatform : $dbForProject; $targetUser = null; diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 70e74648b4..f9db65369a 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2709,118 +2709,10 @@ trait UsersBase } /** - * Proves that the Sec-Fetch-Site CSRF guard prevents forced impersonation via query params. - * - * Attack scenario (without the guard): - * A malicious page on attacker.com embeds: - * - * The browser automatically attaches the impersonator's session cookies. - * Without any guard, the server would impersonate victim_id silently. - * - * Why Sec-Fetch-Site works: - * Browsers set Sec-Fetch-Site: cross-site on all cross-origin requests (img, fetch, etc.). - * It is a browser-enforced forbidden header — JavaScript cannot set or spoof it. - * We only accept ?impersonateUserId when Sec-Fetch-Site is exactly same-origin. - * - * This test proves two attack vectors are blocked and two legitimate origins are allowed: - * Blocked: cross-site — attacker.com embeds pointing at Appwrite - * Blocked: absent — reverse proxy strips Fetch Metadata headers (fail-closed) - * Allowed: same-origin — Console on the same origin as the API - * Allowed: same-site — Console on a different subdomain than the API + * Test impersonation via URL query params — mirrors the ?project= and ?devKey= pattern. + * Allows Console to embed impersonation in direct file/image URLs where headers cannot be set. */ - public function testImpersonateQueryParamCsrfAttackPrevented(): void - { - $projectId = $this->getProject()['$id']; - $headers = array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - ], $this->getHeaders()); - - // Impersonator user (the victim whose session gets hijacked in the attack) - $impersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [ - 'userId' => ID::unique(), - 'email' => 'csrf-guard-impersonator@appwrite.io', - 'password' => 'password', - 'name' => 'CSRF Guard Impersonator', - ]); - $this->assertEquals(201, $impersonator['headers']['status-code']); - $impersonatorId = $impersonator['body']['$id']; - - // Target user (who the attacker wants to impersonate) - $target = $this->client->call(Client::METHOD_POST, '/users', $headers, [ - 'userId' => ID::unique(), - 'email' => 'csrf-guard-target@appwrite.io', - 'password' => 'password', - 'name' => 'CSRF Guard Target', - ]); - $this->assertEquals(201, $target['headers']['status-code']); - $targetId = $target['body']['$id']; - - $this->client->call(Client::METHOD_PATCH, '/users/' . $impersonatorId . '/impersonator', $headers, ['impersonator' => true]); - - $session = $this->client->call(Client::METHOD_POST, '/users/' . $impersonatorId . '/sessions', $headers); - $this->assertEquals(201, $session['headers']['status-code']); - $sessionSecret = $session['body']['secret']; - - $sessionHeaders = [ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - 'x-appwrite-session' => $sessionSecret, - ]; - - // Attack vector 1: cross-site (attacker.com embeds ) - // Browser sends Sec-Fetch-Site: cross-site — must be blocked. - $crossSite = $this->client->call( - Client::METHOD_GET, - '/account', - array_merge($sessionHeaders, ['sec-fetch-site' => 'cross-site']), - ['impersonateUserId' => $targetId] - ); - $this->assertEquals(200, $crossSite['headers']['status-code']); - $this->assertEquals($impersonatorId, $crossSite['body']['$id'], 'cross-site: impersonation must be blocked'); - $this->assertEmpty($crossSite['body']['impersonatorUserId'] ?? ''); - - // Attack vector 2: absent header (reverse proxy strips Fetch Metadata headers) - // Guard must fail-closed — absent Sec-Fetch-Site must not allow query param. - $noFetchSite = $this->client->call( - Client::METHOD_GET, - '/account', - $sessionHeaders, - ['impersonateUserId' => $targetId] - ); - $this->assertEquals(200, $noFetchSite['headers']['status-code']); - $this->assertEquals($impersonatorId, $noFetchSite['body']['$id'], 'absent header: must fail-closed'); - $this->assertEmpty($noFetchSite['body']['impersonatorUserId'] ?? ''); - - // Legitimate use 1: same-origin (Console on same origin as API) - $sameOrigin = $this->client->call( - Client::METHOD_GET, - '/account', - array_merge($sessionHeaders, ['sec-fetch-site' => 'same-origin']), - ['impersonateUserId' => $targetId] - ); - $this->assertEquals(200, $sameOrigin['headers']['status-code']); - $this->assertEquals($targetId, $sameOrigin['body']['$id'], 'same-origin: impersonation must succeed'); - $this->assertEquals($impersonatorId, $sameOrigin['body']['impersonatorUserId']); - - // Legitimate use 2: same-site (Console on a different subdomain than the API) - $sameSite = $this->client->call( - Client::METHOD_GET, - '/account', - array_merge($sessionHeaders, ['sec-fetch-site' => 'same-site']), - ['impersonateUserId' => $targetId] - ); - $this->assertEquals(200, $sameSite['headers']['status-code']); - $this->assertEquals($targetId, $sameSite['body']['$id'], 'same-site: impersonation must succeed'); - $this->assertEquals($impersonatorId, $sameSite['body']['impersonatorUserId']); - } - - /** - * Test impersonation via ?impersonateUserId= query param (same-origin browser request). - * This is the primary use case for embedding impersonation in file/image URLs where - * custom headers cannot be set (e.g. , deployment source/output download links). - */ - public function testImpersonateByUserIdQueryParam(): void + public function testImpersonateByQueryParams(): void { $projectId = $this->getProject()['$id']; $headers = array_merge([ @@ -2853,119 +2745,58 @@ trait UsersBase $this->assertEquals(201, $session['headers']['status-code']); $sessionSecret = $session['body']['secret']; - // Query param works when Sec-Fetch-Site is same-origin or same-site. - // same-site covers Console deployed on a different subdomain than the API. - $account = $this->client->call(Client::METHOD_GET, '/account', [ + $sessionHeaders = [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, 'x-appwrite-session' => $sessionSecret, - 'sec-fetch-site' => 'same-origin', - ], ['impersonateUserId' => $idB]); + ]; + + // Impersonate by user ID via query param + $account = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, [ + 'impersonateUserId' => $idB, + ]); $this->assertEquals(200, $account['headers']['status-code']); $this->assertEquals($idB, $account['body']['$id']); $this->assertEquals('Query Param Target', $account['body']['name']); $this->assertEquals($idA, $account['body']['impersonatorUserId']); - } - /** - * Test that ?impersonateUserId= query param is ignored for cross-site requests (CSRF guard). - * Sec-Fetch-Site is a browser-enforced forbidden header; cross-site value means the request - * originated from a third-party page and must not be allowed to trigger impersonation. - */ - public function testImpersonateQueryParamIgnoredCrossSite(): void - { - $projectId = $this->getProject()['$id']; - $headers = array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - ], $this->getHeaders()); - - $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ - 'userId' => ID::unique(), - 'email' => 'csrf-impersonator@appwrite.io', - 'password' => 'password', - 'name' => 'CSRF Impersonator', + // Impersonate by email via query param + $accountByEmail = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, [ + 'impersonateEmail' => 'queryparam-target@appwrite.io', ]); - $this->assertEquals(201, $userA['headers']['status-code']); - $idA = $userA['body']['$id']; + $this->assertEquals(200, $accountByEmail['headers']['status-code']); + $this->assertEquals($idB, $accountByEmail['body']['$id']); + $this->assertEquals($idA, $accountByEmail['body']['impersonatorUserId']); - $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ - 'userId' => ID::unique(), - 'email' => 'csrf-target@appwrite.io', - 'password' => 'password', - 'name' => 'CSRF Target', + // Impersonate by phone via query param (update target user with a phone first) + $this->client->call(Client::METHOD_PATCH, '/users/' . $idB . '/phone', $headers, [ + 'number' => '+12345678901', ]); - $this->assertEquals(201, $userB['headers']['status-code']); - $idB = $userB['body']['$id']; - - $patch = $this->client->call(Client::METHOD_PATCH, '/users/' . $idA . '/impersonator', $headers, ['impersonator' => true]); - $this->assertEquals(200, $patch['headers']['status-code']); - - $session = $this->client->call(Client::METHOD_POST, '/users/' . $idA . '/sessions', $headers); - $this->assertEquals(201, $session['headers']['status-code']); - $sessionSecret = $session['body']['secret']; - - // Query param must be ignored when Sec-Fetch-Site is cross-site (third-party page embed) - $account = $this->client->call(Client::METHOD_GET, '/account', [ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - 'x-appwrite-session' => $sessionSecret, - 'sec-fetch-site' => 'cross-site', - ], ['impersonateUserId' => $idB]); - $this->assertEquals(200, $account['headers']['status-code']); - // Should resolve as userA (the impersonator), not the target - $this->assertEquals($idA, $account['body']['$id']); - $this->assertEmpty($account['body']['impersonatorUserId'] ?? ''); - } - - /** - * Test that ?impersonateUserId= query param is ignored when Sec-Fetch-Site is absent - * (fail-closed CSRF guard). Absent header means a reverse proxy stripped Fetch Metadata - * headers or a non-browser client is calling — query param must be silently ignored. - */ - public function testImpersonateQueryParamIgnoredWhenSecFetchSiteAbsent(): void - { - $projectId = $this->getProject()['$id']; - $headers = array_merge([ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - ], $this->getHeaders()); - - $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ - 'userId' => ID::unique(), - 'email' => 'absent-fetch-impersonator@appwrite.io', - 'password' => 'password', - 'name' => 'Absent Fetch Impersonator', + $accountByPhone = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, [ + 'impersonatePhone' => '+12345678901', ]); - $this->assertEquals(201, $userA['headers']['status-code']); - $idA = $userA['body']['$id']; + $this->assertEquals(200, $accountByPhone['headers']['status-code']); + $this->assertEquals($idB, $accountByPhone['body']['$id']); + $this->assertEquals($idA, $accountByPhone['body']['impersonatorUserId']); - $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ + // Header takes priority over query param when both are present + $userC = $this->client->call(Client::METHOD_POST, '/users', $headers, [ 'userId' => ID::unique(), - 'email' => 'absent-fetch-target@appwrite.io', + 'email' => 'queryparam-target-c@appwrite.io', 'password' => 'password', - 'name' => 'Absent Fetch Target', + 'name' => 'Query Param Target C', ]); - $this->assertEquals(201, $userB['headers']['status-code']); - $idB = $userB['body']['$id']; + $this->assertEquals(201, $userC['headers']['status-code']); + $idC = $userC['body']['$id']; - $patch = $this->client->call(Client::METHOD_PATCH, '/users/' . $idA . '/impersonator', $headers, ['impersonator' => true]); - $this->assertEquals(200, $patch['headers']['status-code']); - - $session = $this->client->call(Client::METHOD_POST, '/users/' . $idA . '/sessions', $headers); - $this->assertEquals(201, $session['headers']['status-code']); - $sessionSecret = $session['body']['secret']; - - // Query param must be ignored when Sec-Fetch-Site is absent (proxy-stripped or API client) - $account = $this->client->call(Client::METHOD_GET, '/account', [ - 'content-type' => 'application/json', - 'x-appwrite-project' => $projectId, - 'x-appwrite-session' => $sessionSecret, - // no sec-fetch-site header - ], ['impersonateUserId' => $idB]); - $this->assertEquals(200, $account['headers']['status-code']); - $this->assertEquals($idA, $account['body']['$id']); - $this->assertEmpty($account['body']['impersonatorUserId'] ?? ''); + $accountHeaderPriority = $this->client->call( + Client::METHOD_GET, + '/account', + array_merge($sessionHeaders, ['x-appwrite-impersonate-user-id' => $idC]), + ['impersonateUserId' => $idB] + ); + $this->assertEquals(200, $accountHeaderPriority['headers']['status-code']); + $this->assertEquals($idC, $accountHeaderPriority['body']['$id'], 'header must take priority over query param'); } /** From 2a357511eacc6f843c560541f175ff53443cf8b3 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Tue, 28 Apr 2026 19:17:12 +0530 Subject: [PATCH 15/15] fix: use unique emails and phone in query param impersonation test --- tests/e2e/Services/Users/UsersBase.php | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index f9db65369a..b06e2d88e1 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -2720,9 +2720,14 @@ trait UsersBase 'x-appwrite-project' => $projectId, ], $this->getHeaders()); + $emailA = 'queryparam-impersonator-' . \uniqid() . '@appwrite.io'; + $emailB = 'queryparam-target-' . \uniqid() . '@appwrite.io'; + $emailC = 'queryparam-target-c-' . \uniqid() . '@appwrite.io'; + $phone = '+1' . \rand(1000000000, 9999999999); + $userA = $this->client->call(Client::METHOD_POST, '/users', $headers, [ 'userId' => ID::unique(), - 'email' => 'queryparam-impersonator@appwrite.io', + 'email' => $emailA, 'password' => 'password', 'name' => 'Query Param Impersonator', ]); @@ -2731,7 +2736,7 @@ trait UsersBase $userB = $this->client->call(Client::METHOD_POST, '/users', $headers, [ 'userId' => ID::unique(), - 'email' => 'queryparam-target@appwrite.io', + 'email' => $emailB, 'password' => 'password', 'name' => 'Query Param Target', ]); @@ -2762,7 +2767,7 @@ trait UsersBase // Impersonate by email via query param $accountByEmail = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, [ - 'impersonateEmail' => 'queryparam-target@appwrite.io', + 'impersonateEmail' => $emailB, ]); $this->assertEquals(200, $accountByEmail['headers']['status-code']); $this->assertEquals($idB, $accountByEmail['body']['$id']); @@ -2770,10 +2775,10 @@ trait UsersBase // Impersonate by phone via query param (update target user with a phone first) $this->client->call(Client::METHOD_PATCH, '/users/' . $idB . '/phone', $headers, [ - 'number' => '+12345678901', + 'number' => $phone, ]); $accountByPhone = $this->client->call(Client::METHOD_GET, '/account', $sessionHeaders, [ - 'impersonatePhone' => '+12345678901', + 'impersonatePhone' => $phone, ]); $this->assertEquals(200, $accountByPhone['headers']['status-code']); $this->assertEquals($idB, $accountByPhone['body']['$id']); @@ -2782,7 +2787,7 @@ trait UsersBase // Header takes priority over query param when both are present $userC = $this->client->call(Client::METHOD_POST, '/users', $headers, [ 'userId' => ID::unique(), - 'email' => 'queryparam-target-c@appwrite.io', + 'email' => $emailC, 'password' => 'password', 'name' => 'Query Param Target C', ]);