From 7975702e051e977c0b260f6d5c208497c9bf8ba3 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Wed, 9 Oct 2024 18:13:45 +0600 Subject: [PATCH 1/2] Improve userName & displayName validation on accountCreation stage When creating new account, frontend calls `/accounts/validate_fields` api to validate username & displayname. Then it again calls `/accounts/create` to create new account. There is a chance any malicious user can detect it & validate userName & displayName against proper names but create account with invalid values by making js calls. To mitigate this issue, we will save the validated username & displayname on user's session & `/create` endpoint won't take these params. These values will be fetched from the session. issue: https://gitlab.e.foundation/e/infra/backlog/-/issues/3551 --- lib/Controller/AccountController.php | 28 ++++++++++++++++++++++------ src/Signup.vue | 2 -- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index 81a2e997..cb6fff15 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -45,7 +45,8 @@ class AccountController extends Controller { private IConfig $config; private IInitialState $initialState; private IAppData $appData; - private const SESSION_USERNAME_CHECK = 'username_check_passed'; + private const SESSION_VARIFIED_USERNAME = 'varified_username'; + private const SESSION_VARIFIED_DISPLAYNAME = 'varified_displayname'; private const CAPTCHA_VERIFIED_CHECK = 'captcha_verified'; private const ALLOWED_CAPTCHA_PROVIDERS = ['image', 'hcaptcha']; private const DEFAULT_CAPTCHA_PROVIDER = 'image'; @@ -143,7 +144,7 @@ class AccountController extends Controller { * * @return \OCP\AppFramework\Http\DataResponse */ - public function create(string $displayname = '', string $recoveryEmail = '', string $username = '', string $password = '', string $language = 'en', bool $newsletterEos = false, bool $newsletterProduct = false): DataResponse { + public function create(string $recoveryEmail = '', string $password = '', string $language = 'en', bool $newsletterEos = false, bool $newsletterProduct = false): DataResponse { $response = new DataResponse(); @@ -153,7 +154,10 @@ class AccountController extends Controller { return $response; } - if (!$this->session->get(self::SESSION_USERNAME_CHECK)) { + $displayname = $this->session->get(self::SESSION_VARIFIED_DISPLAYNAME); + $username = $this->session->get(self::SESSION_VARIFIED_USERNAME); + + if ($this->isNullOrEmptyInput($displayname) || $this->isNullOrEmptyInput($username)) { $response->setData(['message' => 'Username is already taken.', 'success' => false]); $response->setStatus(400); return $response; @@ -200,7 +204,8 @@ class AccountController extends Controller { $this->userService->sendWelcomeEmail($displayname, $username, $userEmail, $language); - $this->session->remove(self::SESSION_USERNAME_CHECK); + $this->session->remove(self::SESSION_VARIFIED_USERNAME); + $this->session->remove(self::SESSION_VARIFIED_DISPLAYNAME); $this->session->remove(self::CAPTCHA_VERIFIED_CHECK); $ipAddress = $this->request->getRemoteAddress(); $this->userService->addUsernameToCommonDataStore($username, $ipAddress, $recoveryEmail); @@ -227,6 +232,15 @@ class AccountController extends Controller { return $response; } + + private function isNullOrEmptyInput(string|null $input): bool { + if($input === null || empty(trim($input))) { + return true; + } + + return false; + } + /** * Validate input for a given input name, value, and optional maximum length. * @@ -259,7 +273,8 @@ class AccountController extends Controller { * @return \OCP\AppFramework\Http\DataResponse */ public function validateFields(string $username, string $displayname) : DataResponse { - $this->session->remove(self::SESSION_USERNAME_CHECK); + $this->session->remove(self::SESSION_VARIFIED_DISPLAYNAME); + $this->session->remove(self::SESSION_VARIFIED_USERNAME); $response = new DataResponse(); $response->setStatus(400); @@ -304,7 +319,8 @@ class AccountController extends Controller { $response->setData(['message' => 'Username is already taken.', 'field' => 'username', 'success' => false]); } elseif (!$this->userService->userExists($username) && !$this->userService->isUsernameTaken($username)) { $response->setStatus(200); - $this->session->set(self::SESSION_USERNAME_CHECK, true); + $this->session->set(self::SESSION_VARIFIED_USERNAME, $username); + $this->session->set(self::SESSION_VARIFIED_DISPLAYNAME, $displayname); } else { $response->setData(['message' => 'Username is already taken.', 'field' => 'username', 'success' => false]); } diff --git a/src/Signup.vue b/src/Signup.vue index a14320ae..719b96ff 100644 --- a/src/Signup.vue +++ b/src/Signup.vue @@ -92,8 +92,6 @@ export default { submitRecoveryEmailForm(data) { if (data.isFormValid) { const data = { - displayname: this.formData.displayname, - username: this.formData.username, password: this.formData.password, recoveryEmail: this.formData.email, language: this.formData.selectedLanguage, -- GitLab From 7f18be535348bd0d78e5cce87e11f057225aea99 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Thu, 10 Oct 2024 11:24:58 +0600 Subject: [PATCH 2/2] fix: typo-> `verified` --- lib/Controller/AccountController.php | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index cb6fff15..83584199 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -45,8 +45,8 @@ class AccountController extends Controller { private IConfig $config; private IInitialState $initialState; private IAppData $appData; - private const SESSION_VARIFIED_USERNAME = 'varified_username'; - private const SESSION_VARIFIED_DISPLAYNAME = 'varified_displayname'; + private const SESSION_VERIFIED_USERNAME = 'verified_username'; + private const SESSION_VERIFIED_DISPLAYNAME = 'verified_displayname'; private const CAPTCHA_VERIFIED_CHECK = 'captcha_verified'; private const ALLOWED_CAPTCHA_PROVIDERS = ['image', 'hcaptcha']; private const DEFAULT_CAPTCHA_PROVIDER = 'image'; @@ -154,8 +154,8 @@ class AccountController extends Controller { return $response; } - $displayname = $this->session->get(self::SESSION_VARIFIED_DISPLAYNAME); - $username = $this->session->get(self::SESSION_VARIFIED_USERNAME); + $displayname = $this->session->get(self::SESSION_VERIFIED_DISPLAYNAME); + $username = $this->session->get(self::SESSION_VERIFIED_USERNAME); if ($this->isNullOrEmptyInput($displayname) || $this->isNullOrEmptyInput($username)) { $response->setData(['message' => 'Username is already taken.', 'success' => false]); @@ -204,8 +204,8 @@ class AccountController extends Controller { $this->userService->sendWelcomeEmail($displayname, $username, $userEmail, $language); - $this->session->remove(self::SESSION_VARIFIED_USERNAME); - $this->session->remove(self::SESSION_VARIFIED_DISPLAYNAME); + $this->session->remove(self::SESSION_VERIFIED_USERNAME); + $this->session->remove(self::SESSION_VERIFIED_DISPLAYNAME); $this->session->remove(self::CAPTCHA_VERIFIED_CHECK); $ipAddress = $this->request->getRemoteAddress(); $this->userService->addUsernameToCommonDataStore($username, $ipAddress, $recoveryEmail); @@ -273,8 +273,8 @@ class AccountController extends Controller { * @return \OCP\AppFramework\Http\DataResponse */ public function validateFields(string $username, string $displayname) : DataResponse { - $this->session->remove(self::SESSION_VARIFIED_DISPLAYNAME); - $this->session->remove(self::SESSION_VARIFIED_USERNAME); + $this->session->remove(self::SESSION_VERIFIED_DISPLAYNAME); + $this->session->remove(self::SESSION_VERIFIED_USERNAME); $response = new DataResponse(); $response->setStatus(400); @@ -319,8 +319,8 @@ class AccountController extends Controller { $response->setData(['message' => 'Username is already taken.', 'field' => 'username', 'success' => false]); } elseif (!$this->userService->userExists($username) && !$this->userService->isUsernameTaken($username)) { $response->setStatus(200); - $this->session->set(self::SESSION_VARIFIED_USERNAME, $username); - $this->session->set(self::SESSION_VARIFIED_DISPLAYNAME, $displayname); + $this->session->set(self::SESSION_VERIFIED_USERNAME, $username); + $this->session->set(self::SESSION_VERIFIED_DISPLAYNAME, $displayname); } else { $response->setData(['message' => 'Username is already taken.', 'field' => 'username', 'success' => false]); } -- GitLab