From 8a5bece7c11dfa4349fb458166856ed0b4f4022e Mon Sep 17 00:00:00 2001 From: Akhil Date: Tue, 23 Jul 2024 18:11:21 +0530 Subject: [PATCH 1/8] Add security middleware to block change of user agent and IP address --- appinfo/routes.php | 2 +- lib/Controller/AccountController.php | 12 +++-- lib/Exception/SecurityException.php | 9 ++++ lib/Listeners/BeforeUserDeletedListener.php | 2 +- lib/Middleware/SecurityMiddleware.php | 51 +++++++++++++++++++++ lib/Service/CurlService.php | 10 ++-- lib/Service/UserService.php | 8 ++-- 7 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 lib/Exception/SecurityException.php create mode 100644 lib/Middleware/SecurityMiddleware.php diff --git a/appinfo/routes.php b/appinfo/routes.php index e89d7d76..eeeec68a 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -10,7 +10,7 @@ return ['routes' => [ ['name' => 'shop_account#check_shop_email_post_delete', 'url' => '/shop-accounts/check_shop_email_post_delete', 'verb' => 'GET'], [ 'name' => 'user#preflighted_cors', 'url' => '/api/{path}', - 'verb' => 'OPTIONS', 'requirements' => array('path' => '.+') + 'verb' => 'OPTIONS', 'requirements' => ['path' => '.+'] ], [ 'name' => 'beta_user#remove_user_in_group', diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index 6d78e275..b00e97df 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -29,7 +29,7 @@ use OCP\L10N\IFactory; class AccountController extends Controller { protected $appName; - protected $request; + protected IRequest $request; private $userService; private $newsletterService; private $captchaService; @@ -41,6 +41,8 @@ class AccountController extends Controller { private IConfig $config; private const SESSION_USERNAME_CHECK = 'username_check_passed'; private const CAPTCHA_VERIFIED_CHECK = 'captcha_verified'; + private const SESSION_USER_AGENT = 'USER_AGENT'; + private const SESSION_IP_ADDRESS = 'IP_ADDRESS'; private ILogger $logger; public function __construct( $AppName, @@ -57,6 +59,7 @@ class AccountController extends Controller { ) { parent::__construct($AppName, $request); $this->appName = $AppName; + $this->request = $request; $this->userService = $userService; $this->newsletterService = $newsletterService; $this->captchaService = $captchaService; @@ -82,7 +85,10 @@ class AccountController extends Controller { } $_SERVER['HTTP_ACCEPT_LANGUAGE'] = $lang; - + $ipAddr = $this->request->getRemoteAddress(); + $userAgent = $this->request->getHeader('USER_AGENT'); + $this->session->set(self::SESSION_IP_ADDRESS, $ipAddr); + $this->session->set(self::SESSION_USER_AGENT, $userAgent); return new TemplateResponse( Application::APP_ID, 'signup', @@ -193,7 +199,7 @@ class AccountController extends Controller { * * @return string|null If validation fails, a string describing the error; otherwise, null. */ - public function validateInput(string $inputName, string $value, int $maxLength = null) : ?string { + private function validateInput(string $inputName, string $value, ?int $maxLength = null) : ?string { if ($value === '') { return "$inputName is required."; } diff --git a/lib/Exception/SecurityException.php b/lib/Exception/SecurityException.php new file mode 100644 index 00000000..56e22b67 --- /dev/null +++ b/lib/Exception/SecurityException.php @@ -0,0 +1,9 @@ +LDAPConnectionService->isUserOnLDAPBackend($user); try { - $this->logger->info("PostDelete user {user}", array('user' => $uid)); + $this->logger->info("PostDelete user {user}", ['user' => $uid]); $this->userService->deleteEmailAccount($email); } catch (Exception $e) { $this->logger->error('Error deleting mail folder for user '. $uid . ' :' . $e->getMessage()); diff --git a/lib/Middleware/SecurityMiddleware.php b/lib/Middleware/SecurityMiddleware.php new file mode 100644 index 00000000..c3afb0de --- /dev/null +++ b/lib/Middleware/SecurityMiddleware.php @@ -0,0 +1,51 @@ +request = $request; + $this->session = $session; + } + + + public function beforeController($controller, $methodName) { + if (!$controller instanceof AccountController) { + return; + } + + if (!in_array($methodName, self::SESSION_METHODS)) { + return; + } + + if + + if ($this->session->exists(AccountController::SESSION_USER_AGENT) && ($this->session->get(AccountController::SESSION_USER_AGENT) !== $this->request->getHeader('USER_AGENT')) || + $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() + ) { + throw new SecurityException; + } + } + + public function afterException($controller, $methodName, \Exception $exception) { + if (!$controller instanceof AccountController || !$exception instanceof SecurityException) { + throw $exception; + } + + $response = new DataResponse(); + $response->setStatus(401); + return new DataResponse(['message' => 'Account creation blocked for security reasons! Please contact helpdesk if it is wrongly done!']); + } +} diff --git a/lib/Service/CurlService.php b/lib/Service/CurlService.php index 2c6678da..090e7744 100644 --- a/lib/Service/CurlService.php +++ b/lib/Service/CurlService.php @@ -25,7 +25,7 @@ class CurlService { * @param array $userOptions * @return mixed */ - public function get($url, $params = array(), $headers = array(), $userOptions = array()) { + public function get($url, $params = [], $headers = [], $userOptions = []) { return $this->request('GET', $url, $params, $headers, $userOptions); } @@ -38,7 +38,7 @@ class CurlService { * @param array $userOptions * @return mixed */ - public function post($url, $params = array(), $headers = array(), $userOptions = array()) { + public function post($url, $params = [], $headers = [], $userOptions = []) { return $this->request('POST', $url, $params, $headers, $userOptions); } @@ -88,13 +88,13 @@ class CurlService { * @return mixed * @throws Exception */ - private function request($method, $url, $params = array(), $headers = array(), $userOptions = array()) { + private function request($method, $url, $params = [], $headers = [], $userOptions = []) { $ch = curl_init(); $method = strtoupper($method); - $options = array( + $options = [ CURLOPT_RETURNTRANSFER => true, CURLOPT_HTTPHEADER => $headers - ); + ]; foreach ($userOptions as $key => $value) { $options[$key] = $value; } diff --git a/lib/Service/UserService.php b/lib/Service/UserService.php index 8fb5b220..531f2329 100644 --- a/lib/Service/UserService.php +++ b/lib/Service/UserService.php @@ -311,9 +311,9 @@ class UserService { $endpoint = $commonApiVersion . '/aliases/hide-my-email/'; $url = $commonServicesURL . $endpoint . $resultmail; - $data = array( + $data = [ "domain" => $aliasDomain - ); + ]; $headers = [ "Authorization: Bearer $token" ]; @@ -348,10 +348,10 @@ class UserService { $endpoint = $commonApiVersion . '/aliases/'; $url = $commonServicesURL . $endpoint . $userEmail; - $data = array( + $data = [ "alias" => $username, "domain" => $domain - ); + ]; $headers = [ "Authorization: Bearer $token" ]; -- GitLab From 048ab3248da52ceccb1b2c6cf50357dd872d2021 Mon Sep 17 00:00:00 2001 From: Akhil Date: Tue, 23 Jul 2024 18:41:30 +0530 Subject: [PATCH 2/8] Run php linter --- lib/Controller/AccountController.php | 4 ++-- lib/Middleware/SecurityMiddleware.php | 11 ++++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index b00e97df..0ad96566 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -41,8 +41,8 @@ class AccountController extends Controller { private IConfig $config; private const SESSION_USERNAME_CHECK = 'username_check_passed'; private const CAPTCHA_VERIFIED_CHECK = 'captcha_verified'; - private const SESSION_USER_AGENT = 'USER_AGENT'; - private const SESSION_IP_ADDRESS = 'IP_ADDRESS'; + public const SESSION_USER_AGENT = 'USER_AGENT'; + public const SESSION_IP_ADDRESS = 'IP_ADDRESS'; private ILogger $logger; public function __construct( $AppName, diff --git a/lib/Middleware/SecurityMiddleware.php b/lib/Middleware/SecurityMiddleware.php index c3afb0de..92f5a846 100644 --- a/lib/Middleware/SecurityMiddleware.php +++ b/lib/Middleware/SecurityMiddleware.php @@ -2,13 +2,12 @@ namespace OCA\EcloudAccounts\Middleware; -use OCP\AppFramework\Middleware; -use OCP\IRequest; -use OCP\ISession; use OCA\EcloudAccounts\Controller\AccountController; use OCA\EcloudAccounts\Exception\SecurityException; use OCP\AppFramework\Http\DataResponse; - +use OCP\AppFramework\Middleware; +use OCP\IRequest; +use OCP\ISession; class SecurityMiddleware extends Middleware { private IRequest $request; @@ -30,10 +29,8 @@ class SecurityMiddleware extends Middleware { return; } - if - if ($this->session->exists(AccountController::SESSION_USER_AGENT) && ($this->session->get(AccountController::SESSION_USER_AGENT) !== $this->request->getHeader('USER_AGENT')) || - $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() + $this->session->exists(AccountController::SESSION_IP_ADDRESS) && $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() ) { throw new SecurityException; } -- GitLab From 056b5abb6e13aca60175d55e1c72182a068928ae Mon Sep 17 00:00:00 2001 From: Akhil Date: Tue, 23 Jul 2024 19:36:04 +0530 Subject: [PATCH 3/8] register middleware --- lib/AppInfo/Application.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index b1635540..3bfbde62 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -31,6 +31,7 @@ use OCA\EcloudAccounts\Listeners\BeforeUserDeletedListener; use OCA\EcloudAccounts\Listeners\PasswordUpdatedListener; use OCA\EcloudAccounts\Listeners\TwoFactorStateChangedListener; use OCA\EcloudAccounts\Listeners\UserChangedListener; +use OCA\EcloudAccounts\Middleware\SecurityMiddleware; use OCA\EcloudAccounts\Service\LDAPConnectionService; use OCA\TwoFactorTOTP\Event\StateChanged; use OCP\AppFramework\App; @@ -56,6 +57,8 @@ class Application extends App implements IBootstrap { $context->registerEventListener(UserChangedEvent::class, UserChangedListener::class); $context->registerEventListener(StateChanged::class, TwoFactorStateChangedListener::class); $context->registerEventListener(PasswordUpdatedEvent::class, PasswordUpdatedListener::class); + + $context->registerMiddleware(SecurityMiddleware::class); } public function boot(IBootContext $context): void { -- GitLab From 30b62d426f56e06ad59ffbacff81d6ecff819913 Mon Sep 17 00:00:00 2001 From: Akhil Date: Wed, 24 Jul 2024 14:19:51 +0530 Subject: [PATCH 4/8] Don't set any message --- lib/Middleware/SecurityMiddleware.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Middleware/SecurityMiddleware.php b/lib/Middleware/SecurityMiddleware.php index 92f5a846..898c35bf 100644 --- a/lib/Middleware/SecurityMiddleware.php +++ b/lib/Middleware/SecurityMiddleware.php @@ -43,6 +43,6 @@ class SecurityMiddleware extends Middleware { $response = new DataResponse(); $response->setStatus(401); - return new DataResponse(['message' => 'Account creation blocked for security reasons! Please contact helpdesk if it is wrongly done!']); + return $response; } } -- GitLab From 87e2d6695bb4a31d8c6a3216b0311a267baac2b1 Mon Sep 17 00:00:00 2001 From: Akhil Date: Wed, 24 Jul 2024 15:38:31 +0530 Subject: [PATCH 5/8] Move all changes to middleware --- lib/Controller/AccountController.php | 9 ++------- lib/Middleware/SecurityMiddleware.php | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index 0ad96566..177b180b 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -41,8 +41,7 @@ class AccountController extends Controller { private IConfig $config; private const SESSION_USERNAME_CHECK = 'username_check_passed'; private const CAPTCHA_VERIFIED_CHECK = 'captcha_verified'; - public const SESSION_USER_AGENT = 'USER_AGENT'; - public const SESSION_IP_ADDRESS = 'IP_ADDRESS'; + private ILogger $logger; public function __construct( $AppName, @@ -59,7 +58,6 @@ class AccountController extends Controller { ) { parent::__construct($AppName, $request); $this->appName = $AppName; - $this->request = $request; $this->userService = $userService; $this->newsletterService = $newsletterService; $this->captchaService = $captchaService; @@ -85,10 +83,7 @@ class AccountController extends Controller { } $_SERVER['HTTP_ACCEPT_LANGUAGE'] = $lang; - $ipAddr = $this->request->getRemoteAddress(); - $userAgent = $this->request->getHeader('USER_AGENT'); - $this->session->set(self::SESSION_IP_ADDRESS, $ipAddr); - $this->session->set(self::SESSION_USER_AGENT, $userAgent); + return new TemplateResponse( Application::APP_ID, 'signup', diff --git a/lib/Middleware/SecurityMiddleware.php b/lib/Middleware/SecurityMiddleware.php index 898c35bf..26f58b77 100644 --- a/lib/Middleware/SecurityMiddleware.php +++ b/lib/Middleware/SecurityMiddleware.php @@ -14,6 +14,9 @@ class SecurityMiddleware extends Middleware { private ISession $session; private const SESSION_METHODS = ['create', 'checkUsernameAvailable', 'captcha', 'verifyCaptcha']; + private const SESSION_USER_AGENT = 'USER_AGENT'; + private const SESSION_IP_ADDRESS = 'IP_ADDRESS'; + public function __construct(IRequest $request, ISession $session) { $this->request = $request; $this->session = $session; @@ -25,12 +28,21 @@ class SecurityMiddleware extends Middleware { return; } + // Add the required params to session for index + if ($methodName === 'index') { + $ipAddr = $this->request->getRemoteAddress(); + $userAgent = $this->request->getHeader('USER_AGENT'); + $this->session->set(self::SESSION_IP_ADDRESS, $ipAddr); + $this->session->set(self::SESSION_USER_AGENT, $userAgent); + return; + } + if (!in_array($methodName, self::SESSION_METHODS)) { return; } - if ($this->session->exists(AccountController::SESSION_USER_AGENT) && ($this->session->get(AccountController::SESSION_USER_AGENT) !== $this->request->getHeader('USER_AGENT')) || - $this->session->exists(AccountController::SESSION_IP_ADDRESS) && $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() + if ($this->session->exists(self::SESSION_USER_AGENT) && ($this->session->get(AccountController::SESSION_USER_AGENT) !== $this->request->getHeader('USER_AGENT')) || + $this->session->exists(self::SESSION_IP_ADDRESS) && $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() ) { throw new SecurityException; } -- GitLab From c00d9bcf3028cb5d111765674a95f17476ad7513 Mon Sep 17 00:00:00 2001 From: Akhil Date: Wed, 24 Jul 2024 17:00:09 +0530 Subject: [PATCH 6/8] Refactor condition --- lib/AppInfo/Application.php | 4 ++-- ...tyMiddleware.php => AccountMiddleware.php} | 22 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) rename lib/Middleware/{SecurityMiddleware.php => AccountMiddleware.php} (68%) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 3bfbde62..24c5f795 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -31,7 +31,7 @@ use OCA\EcloudAccounts\Listeners\BeforeUserDeletedListener; use OCA\EcloudAccounts\Listeners\PasswordUpdatedListener; use OCA\EcloudAccounts\Listeners\TwoFactorStateChangedListener; use OCA\EcloudAccounts\Listeners\UserChangedListener; -use OCA\EcloudAccounts\Middleware\SecurityMiddleware; +use OCA\EcloudAccounts\Middleware\AccountMiddleware; use OCA\EcloudAccounts\Service\LDAPConnectionService; use OCA\TwoFactorTOTP\Event\StateChanged; use OCP\AppFramework\App; @@ -58,7 +58,7 @@ class Application extends App implements IBootstrap { $context->registerEventListener(StateChanged::class, TwoFactorStateChangedListener::class); $context->registerEventListener(PasswordUpdatedEvent::class, PasswordUpdatedListener::class); - $context->registerMiddleware(SecurityMiddleware::class); + $context->registerMiddleware(AccountMiddleware::class); } public function boot(IBootContext $context): void { diff --git a/lib/Middleware/SecurityMiddleware.php b/lib/Middleware/AccountMiddleware.php similarity index 68% rename from lib/Middleware/SecurityMiddleware.php rename to lib/Middleware/AccountMiddleware.php index 26f58b77..4dbf96d4 100644 --- a/lib/Middleware/SecurityMiddleware.php +++ b/lib/Middleware/AccountMiddleware.php @@ -9,13 +9,14 @@ use OCP\AppFramework\Middleware; use OCP\IRequest; use OCP\ISession; -class SecurityMiddleware extends Middleware { +class AccountMiddleware extends Middleware { private IRequest $request; private ISession $session; private const SESSION_METHODS = ['create', 'checkUsernameAvailable', 'captcha', 'verifyCaptcha']; private const SESSION_USER_AGENT = 'USER_AGENT'; private const SESSION_IP_ADDRESS = 'IP_ADDRESS'; + private const HEADER_USER_AGENT = 'USER_AGENT'; public function __construct(IRequest $request, ISession $session) { $this->request = $request; @@ -31,7 +32,7 @@ class SecurityMiddleware extends Middleware { // Add the required params to session for index if ($methodName === 'index') { $ipAddr = $this->request->getRemoteAddress(); - $userAgent = $this->request->getHeader('USER_AGENT'); + $userAgent = $this->request->getHeader(self::HEADER_USER_AGENT); $this->session->set(self::SESSION_IP_ADDRESS, $ipAddr); $this->session->set(self::SESSION_USER_AGENT, $userAgent); return; @@ -41,9 +42,10 @@ class SecurityMiddleware extends Middleware { return; } - if ($this->session->exists(self::SESSION_USER_AGENT) && ($this->session->get(AccountController::SESSION_USER_AGENT) !== $this->request->getHeader('USER_AGENT')) || - $this->session->exists(self::SESSION_IP_ADDRESS) && $this->session->get(AccountController::SESSION_IP_ADDRESS) !== $this->request->getRemoteAddress() - ) { + $ipAddr = $this->request->getRemoteAddress(); + $userAgent = $this->request->getHeader(self::HEADER_USER_AGENT); + if (!$this->isValidSessionParam(self::SESSION_IP_ADDRESS, $ipAddr) + || !$this->isValidSessionParam(self::SESSION_USER_AGENT, $userAgent)) { throw new SecurityException; } } @@ -57,4 +59,14 @@ class SecurityMiddleware extends Middleware { $response->setStatus(401); return $response; } + + private function isValidSessionParam(string $sessionParam, string $value) : bool { + if (!$this->session->exists($sessionParam)) { + return false; + } + if ($this->session->get($sessionParam) !== $value) { + return false; + } + return true; + } } -- GitLab From 7c908063cb1aa544a9acf54373c531dc5eb716ab Mon Sep 17 00:00:00 2001 From: Akhil Date: Wed, 24 Jul 2024 17:05:22 +0530 Subject: [PATCH 7/8] Don't define type of attribute request --- lib/Controller/AccountController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Controller/AccountController.php b/lib/Controller/AccountController.php index 177b180b..2b08647b 100644 --- a/lib/Controller/AccountController.php +++ b/lib/Controller/AccountController.php @@ -29,7 +29,7 @@ use OCP\L10N\IFactory; class AccountController extends Controller { protected $appName; - protected IRequest $request; + protected $request; private $userService; private $newsletterService; private $captchaService; -- GitLab From 103775a395f16ea940c19591d298276f31a44134 Mon Sep 17 00:00:00 2001 From: Akhil Date: Wed, 24 Jul 2024 17:26:49 +0530 Subject: [PATCH 8/8] Show generic error message if no message sent from server --- src/Signup.vue | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Signup.vue b/src/Signup.vue index 82e3158c..e86ab190 100644 --- a/src/Signup.vue +++ b/src/Signup.vue @@ -101,8 +101,13 @@ export default { this.showRecoveryEmailForm = false this.showSuccessSection = true } catch (error) { + const genericErrorMessage = 'An error occurred while creating your account!' // Handle network errors and unexpected response structures here - const errorMessage = error.response ? t(this.appName, error.response.data.message) : t(this.appName, error.message) + let errorMessage = error.response ? t(this.appName, error.response.data.message) : t(this.appName, error.message) + if (errorMessage === '') { + // Fallback to generic error message + errorMessage = t(this.appName, genericErrorMessage) + } this.showMessage(errorMessage, 'error') } }, -- GitLab