From 13870cd5172c7d2e94890d0509f9ae04a5b54c6e Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 7 Aug 2024 11:27:49 +0200 Subject: [PATCH] fix: Add brute force protection to form endpoints Endpoints that query for forms are now protected against brute force attacks to find valid forms, invalid hashes or IDs. Signed-off-by: Ferdinand Thiessen --- lib/AppInfo/Application.php | 2 + lib/Controller/ApiController.php | 154 +++++++++--------- lib/Exception/NoSuchFormException.php | 19 +++ .../ThrottleFormAccessMiddleware.php | 34 ++++ tests/Unit/Controller/ApiControllerTest.php | 46 ++++-- 5 files changed, 159 insertions(+), 96 deletions(-) create mode 100644 lib/Exception/NoSuchFormException.php create mode 100644 lib/Middleware/ThrottleFormAccessMiddleware.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 319e48f3e..82c17f12f 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -14,6 +14,7 @@ use OCA\Forms\FormsMigrator; use OCA\Forms\Listener\AnalyticsDatasourceListener; use OCA\Forms\Listener\UserDeletedListener; +use OCA\Forms\Middleware\ThrottleFormAccessMiddleware; use OCA\Forms\Search\SearchProvider; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; @@ -43,6 +44,7 @@ public function register(IRegistrationContext $context): void { $context->registerCapability(Capabilities::class); $context->registerEventListener(UserDeletedEvent::class, UserDeletedListener::class); $context->registerEventListener(DatasourceEvent::class, AnalyticsDatasourceListener::class); + $context->registerMiddleware(ThrottleFormAccessMiddleware::class); $context->registerSearchProvider(SearchProvider::class); $context->registerUserMigrator(FormsMigrator::class); } diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 7c636757e..db466b7ae 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -22,6 +22,7 @@ use OCA\Forms\Db\SubmissionMapper; use OCA\Forms\Db\UploadedFile; use OCA\Forms\Db\UploadedFileMapper; +use OCA\Forms\Exception\NoSuchFormException; use OCA\Forms\ResponseDefinitions; use OCA\Forms\Service\ConfigService; use OCA\Forms\Service\FormsService; @@ -31,6 +32,7 @@ use OCP\AppFramework\Db\IMapperException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\ApiRoute; +use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\CORS; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; @@ -147,6 +149,7 @@ public function getForms(string $type = 'owned'): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms')] public function newForm(?int $fromId = null): DataResponse { // Check if user is allowed @@ -226,19 +229,10 @@ public function newForm(?int $fromId = null): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}')] public function getForm(int $formId): DataResponse { - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form'); - throw new OCSBadRequestException('Could not find form'); - } - - if (!$this->formsService->hasUserAccess($form)) { - $this->logger->debug('User has no permissions to get this form'); - throw new OCSForbiddenException('User has no permissions to get this form'); - } + $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_SUBMIT); return new DataResponse($this->formsService->getForm($form)); } @@ -259,6 +253,7 @@ public function getForm(int $formId): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}')] public function updateForm(int $formId, array $keyValuePairs): DataResponse { $this->logger->debug('Updating form: formId: {formId}, values: {keyValuePairs}', [ @@ -359,6 +354,7 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}')] public function deleteForm(int $formId): DataResponse { $this->logger->debug('Delete Form: {formId}', [ @@ -472,6 +468,7 @@ public function getQuestion(int $formId, int $questionId): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions')] public function newQuestion(int $formId, ?string $type = null, string $text = '', ?int $fromId = null): DataResponse { $form = $this->getFormIfAllowed($formId); @@ -594,6 +591,7 @@ public function newQuestion(int $formId, ?string $type = null, string $text = '' */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}')] public function updateQuestion(int $formId, int $questionId, array $keyValuePairs): DataResponse { $this->logger->debug('Updating question: formId: {formId}, questionId: {questionId}, values: {keyValuePairs}', [ @@ -602,6 +600,14 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair 'keyValuePairs' => $keyValuePairs ]); + // Make sure we query the form first to check the user has permissions + // So the user does not get information about "questions" if they do not even have permissions to the form + $form = $this->getFormIfAllowed($formId); + if ($this->formsService->isFormArchived($form)) { + $this->logger->debug('This form is archived and can not be modified'); + throw new OCSForbiddenException('This form is archived and can not be modified'); + } + try { $question = $this->questionMapper->findById($questionId); } catch (IMapperException $e) { @@ -613,12 +619,6 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair throw new OCSBadRequestException('Question doesn\'t belong to given Form'); } - $form = $this->getFormIfAllowed($formId); - if ($this->formsService->isFormArchived($form)) { - $this->logger->debug('This form is archived and can not be modified'); - throw new OCSForbiddenException('This form is archived and can not be modified'); - } - // Don't allow empty array if (sizeof($keyValuePairs) === 0) { $this->logger->info('Empty keyValuePairs, will not update.'); @@ -668,12 +668,20 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/questions/{questionId}')] public function deleteQuestion(int $formId, int $questionId): DataResponse { $this->logger->debug('Mark question as deleted: {questionId}', [ 'questionId' => $questionId, ]); + + $form = $this->getFormIfAllowed($formId); + if ($this->formsService->isFormArchived($form)) { + $this->logger->debug('This form is archived and can not be modified'); + throw new OCSForbiddenException('This form is archived and can not be modified'); + } + try { $question = $this->questionMapper->findById($questionId); } catch (IMapperException $e) { @@ -685,12 +693,6 @@ public function deleteQuestion(int $formId, int $questionId): DataResponse { throw new OCSBadRequestException('Question doesn\'t belong to given Form'); } - $form = $this->getFormIfAllowed($formId); - if ($this->formsService->isFormArchived($form)) { - $this->logger->debug('This form is archived and can not be modified'); - throw new OCSForbiddenException('This form is archived and can not be modified'); - } - // Store Order of deleted Question $deletedOrder = $question->getOrder(); @@ -732,6 +734,7 @@ public function deleteQuestion(int $formId, int $questionId): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions')] public function reorderQuestions(int $formId, array $newOrder): DataResponse { $this->logger->debug('Reordering Questions on Form {formId} as Question-Ids {newOrder}', [ @@ -829,6 +832,7 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions/{questionId}/options')] public function newOption(int $formId, int $questionId, array $optionTexts): DataResponse { $this->logger->debug('Adding new options: formId: {formId}, questionId: {questionId}, text: {text}', [ @@ -909,6 +913,7 @@ public function newOption(int $formId, int $questionId, array $optionTexts): Dat */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}/options/{optionId}')] public function updateOption(int $formId, int $questionId, int $optionId, array $keyValuePairs): DataResponse { $this->logger->debug('Updating option: form: {formId}, question: {questionId}, option: {optionId}, values: {keyValuePairs}', [ @@ -978,6 +983,7 @@ public function updateOption(int $formId, int $questionId, int $optionId, array */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/questions/{questionId}/options/{optionId}')] public function deleteOption(int $formId, int $questionId, int $optionId): DataResponse { $this->logger->debug('Deleting option: {optionId}', [ @@ -1037,6 +1043,7 @@ public function deleteOption(int $formId, int $questionId, int $optionId): DataR */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}/options')] public function reorderOptions(int $formId, int $questionId, array $newOrder) { $form = $this->getFormIfAllowed($formId); @@ -1134,19 +1141,10 @@ public function reorderOptions(int $formId, int $questionId, array $newOrder) { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}/submissions')] public function getSubmissions(int $formId, ?string $fileFormat = null): DataResponse|DataDownloadResponse { - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form'); - throw new OCSNotFoundException('Could not find form'); - } - - if (!$this->formsService->canSeeResults($form)) { - $this->logger->debug('The current user has no permission to get the results for this form'); - throw new OCSForbiddenException('The current user has no permission to get the results for this form'); - } + $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); if ($fileFormat !== null) { $submissionsData = $this->submissionService->getSubmissionsData($form, $fileFormat); @@ -1205,24 +1203,10 @@ public function getSubmissions(int $formId, ?string $fileFormat = null): DataRes */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/submissions')] public function deleteAllSubmissions(int $formId): DataResponse { - $this->logger->debug('Delete all submissions to form: {formId}', [ - 'formId' => $formId, - ]); - - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form'); - throw new OCSNotFoundException('Could not find form'); - } - - // The current user has permissions to remove submissions - if (!$this->formsService->canDeleteResults($form)) { - $this->logger->debug('This form is not owned by the current user and user has no `results_delete` permission'); - throw new OCSForbiddenException('This form is not owned by the current user and user has no `results_delete` permission'); - } + $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); // Delete all submissions (incl. Answers) $this->submissionMapper->deleteByForm($formId); @@ -1337,18 +1321,19 @@ public function newSubmission(int $formId, array $answers, string $shareHash = ' */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/submissions/{submissionId}')] public function deleteSubmission(int $formId, int $submissionId): DataResponse { $this->logger->debug('Delete Submission: {submissionId}', [ 'submissionId' => $submissionId, ]); + $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); try { $submission = $this->submissionMapper->findById($submissionId); - $form = $this->formMapper->findById($formId); } catch (IMapperException $e) { - $this->logger->debug('Could not find form or submission'); - throw new OCSNotFoundException('Could not find form or submission'); + $this->logger->debug('Could not find submission'); + throw new OCSNotFoundException('Could not find submission'); } if ($formId !== $submission->getFormId()) { @@ -1356,12 +1341,6 @@ public function deleteSubmission(int $formId, int $submissionId): DataResponse { throw new OCSBadRequestException('Submission doesn\'t belong to given form'); } - // The current user has permissions to remove submissions - if (!$this->formsService->canDeleteResults($form)) { - $this->logger->debug('This form is not owned by the current user and user has no `results_delete` permission'); - throw new OCSForbiddenException('This form is not owned by the current user and user has no `results_delete` permission'); - } - // Delete submission (incl. Answers) $this->submissionMapper->deleteById($submissionId); $this->formMapper->update($form); @@ -1383,20 +1362,10 @@ public function deleteSubmission(int $formId, int $submissionId): DataResponse { */ #[CORS()] #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/submissions/export')] public function exportSubmissionsToCloud(int $formId, string $path, string $fileFormat = Constants::DEFAULT_FILE_FORMAT) { - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form'); - throw new OCSNotFoundException('Could not find form'); - } - - if (!$this->formsService->canSeeResults($form)) { - $this->logger->debug('The current user has no permission to get the results for this form'); - throw new OCSForbiddenException('The current user has no permission to get the results for this form'); - } - + $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); $file = $this->submissionService->writeFileToCloud($form, $path, $fileFormat); return new DataResponse($file->getName()); @@ -1421,8 +1390,9 @@ public function exportSubmissionsToCloud(int $formId, string $path, string $file * 200: the file id and name of the uploaded file */ #[CORS()] - #[NoAdminRequired()] #[PublicPage()] + #[NoAdminRequired()] + #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/submissions/files/{questionId}')] public function uploadFiles(int $formId, int $questionId, string $shareHash = ''): DataResponse { $this->logger->debug('Uploading files for formId: {formId}, questionId: {questionId}', [ @@ -1614,7 +1584,7 @@ private function loadFormForSubmission(int $formId, string $shareHash): Form { $form = $this->formMapper->findById($formId); } catch (IMapperException $e) { $this->logger->debug('Could not find form'); - throw new OCSNotFoundException('Could not find form'); + throw new NoSuchFormException('Could not find form'); } // Does the user have access to the form (Either by logged-in user, or by providing public share-hash.) @@ -1634,7 +1604,7 @@ private function loadFormForSubmission(int $formId, string $shareHash): Form { } finally { // Now forbid, if no public share and no direct share. if (!$isPublicShare && !$this->formsService->hasUserAccess($form)) { - throw new OCSForbiddenException('Not allowed to access this form'); + throw new NoSuchFormException('Not allowed to access this form'); } } @@ -1650,20 +1620,42 @@ private function loadFormForSubmission(int $formId, string $shareHash): Form { * Helper that retrieves a form if the current user is allowed to edit it * This throws an exception in case either the form is not found or permissions are missing. * @param int $formId The form ID to retrieve - * @throws OCSNotFoundException If the form was not found - * @throws OCSForbiddenException If the current user has no permission to edit + * @throws NoSuchFormException If the form was not found or the current user has no permission to edit */ - private function getFormIfAllowed(int $formId): Form { + private function getFormIfAllowed(int $formId, string $permissions = 'all'): Form { try { $form = $this->formMapper->findById($formId); } catch (IMapperException $e) { $this->logger->debug('Could not find form'); - throw new OCSNotFoundException('Could not find form'); + throw new NoSuchFormException('Could not find form'); } - if ($form->getOwnerId() !== $this->currentUser->getUID()) { - $this->logger->debug('This form is not owned by the current user'); - throw new OCSForbiddenException('This form is not owned by the current user'); + switch ($permissions) { + case Constants::PERMISSION_SUBMIT: + if (!$this->formsService->hasUserAccess($form)) { + $this->logger->debug('User has no permissions to get this form'); + throw new NoSuchFormException('User has no permissions to get this form', Http::STATUS_FORBIDDEN); + } + break; + case Constants::PERMISSION_RESULTS: + if (!$this->formsService->canSeeResults($form)) { + $this->logger->debug('The current user has no permission to get the results for this form'); + throw new NoSuchFormException('The current user has no permission to get the results for this form', Http::STATUS_FORBIDDEN); + } + break; + case Constants::PERMISSION_RESULTS_DELETE: + if (!$this->formsService->canDeleteResults($form)) { + $this->logger->debug('This form is not owned by the current user and user has no `results_delete` permission'); + throw new NoSuchFormException('This form is not owned by the current user and user has no `results_delete` permission', Http::STATUS_FORBIDDEN); + } + break; + default: + // By default we request full permissions + if ($form->getOwnerId() !== $this->currentUser->getUID()) { + $this->logger->debug('This form is not owned by the current user'); + throw new NoSuchFormException('This form is not owned by the current user', Http::STATUS_FORBIDDEN); + } + break; } return $form; } diff --git a/lib/Exception/NoSuchFormException.php b/lib/Exception/NoSuchFormException.php new file mode 100644 index 000000000..1d25f2231 --- /dev/null +++ b/lib/Exception/NoSuchFormException.php @@ -0,0 +1,19 @@ +getMessage(), + $exception->getCode(), + ); + $response->throttle(['action' => 'form']); + return $response; + } +} diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 10945494e..20bf8e2f4 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -43,6 +43,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCA\Forms\Db\Submission; use OCA\Forms\Db\SubmissionMapper; use OCA\Forms\Db\UploadedFileMapper; +use OCA\Forms\Exception\NoSuchFormException; use OCA\Forms\Service\ConfigService; use OCA\Forms\Service\FormsService; use OCA\Forms\Service\SubmissionService; @@ -200,7 +201,7 @@ public function testGetSubmissions_invalidForm() { ->method('findById') ->with(1) ->willThrowException($exception); - $this->expectException(OCSNotFoundException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->getSubmissions(1); } @@ -219,7 +220,7 @@ public function testGetSubmissions_noPermissions() { ->with($form) ->willReturn(false); - $this->expectException(OCSForbiddenException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->getSubmissions(1); } @@ -305,7 +306,7 @@ public function testExportSubmissions_invalidForm() { ->method('findById') ->with(99) ->willThrowException($exception); - $this->expectException(OCSNotFoundException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->getSubmissions(99, 'csv'); } @@ -324,7 +325,7 @@ public function testExportSubmissions_noPermissions() { ->with($form) ->willReturn(false); - $this->expectException(OCSForbiddenException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->getSubmissions(1, 'csv'); } @@ -364,7 +365,7 @@ public function testExportSubmissionsToCloud_invalidForm() { ->method('findById') ->with(1) ->willThrowException($exception); - $this->expectException(OCSNotFoundException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->exportSubmissionsToCloud(1, ''); } @@ -437,7 +438,7 @@ public function dataCloneForm_exceptions() { 'not found' => [ 'canCreate' => true, 'callback' => fn ($id): Form => $this->throwMockedException(MockedMapperException::class), - 'exception' => OCSNotFoundException::class + 'exception' => NoSuchFormException::class ], 'not owned' => [ 'canCreate' => true, @@ -447,7 +448,7 @@ public function dataCloneForm_exceptions() { $form->setOwnerId('otherUser'); return $form; }, - 'exception' => OCSForbiddenException::class + 'exception' => NoSuchFormException::class ] ]; } @@ -766,7 +767,7 @@ public function testNewSubmission_formNotFound() { ->method('findById') ->with(1) ->willThrowException($exception); - $this->expectException(OCSNotFoundException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->newSubmission(1, [], ''); } @@ -775,16 +776,16 @@ public function testNewSubmission_formNotFound() { */ public function dataForCheckForbiddenException() { return [ - 'user_dont_have_access_to_form' => [false, true, true], - 'form_expired' => [true, true, true], - 'not_allowed_to_submit' => [true, false, false], + 'user_dont_have_access_to_form' => [false, true, true, NoSuchFormException::class], + 'form_expired' => [true, true, true, OCSForbiddenException::class], + 'not_allowed_to_submit' => [true, false, false, OCSForbiddenException::class], ]; } /** * @dataProvider dataForCheckForbiddenException() */ - public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExpired, $canSubmit) { + public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExpired, $canSubmit, $exception) { $form = new Form(); $form->setId(1); $form->setOwnerId('admin'); @@ -800,7 +801,7 @@ public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExp $this->formAccess($hasUserAccess, $hasFormExpired, $canSubmit); - $this->expectException(OCSForbiddenException::class); + $this->expectException($exception); $this->apiController->newSubmission(1, [], ''); } @@ -834,12 +835,27 @@ public function testNewSubmission_validateSubmission() { public function testDeleteSubmissionNotFound() { $exception = $this->createMock(MapperException::class); + $form = new Form(); + $form->setId(1); + $form->setOwnerId('currentUser'); + + $this->formMapper->expects(self::once()) + ->method('findById') + ->with(1) + ->willReturn($form); + + $this->formsService->expects(self::once()) + ->method('canDeleteResults') + ->with($form) + ->willReturn(true); + $this->submissionMapper ->expects($this->once()) ->method('findById') ->with(42) ->willThrowException($exception); + // Not found as this is about the submission, not the form $this->expectException(OCSNotFoundException::class); $this->apiController->deleteSubmission(1, 42); } @@ -867,7 +883,7 @@ public function testDeleteSubmissionNoPermission($submissionData, $formData) { ->with($form) ->willReturn(false); - $this->expectException(OCSForbiddenException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->deleteSubmission(1, 42); } @@ -938,7 +954,7 @@ public function testTransferOwnerNotOwner() { ->with(1) ->willReturn($form); - $this->expectException(OCSForbiddenException::class); + $this->expectException(NoSuchFormException::class); $this->apiController->updateForm(1, ['ownerId' => 'newOwner']); }