Skip to content

Commit

Permalink
fix: Add brute force protection to form endpoints
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@susnux
susnux committed Jan 17, 2025
1 parent 9a1c6cb commit 13870cd
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 96 deletions.
2 changes: 2 additions & 0 deletions lib/AppInfo/Application.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Check warning on line 47 in lib/AppInfo/Application.php

View check run for this annotation

Codecov / codecov/patch

lib/AppInfo/Application.php#L47 

Added line #L47 was not covered by tests
$context->registerSearchProvider(SearchProvider::class);
$context->registerUserMigrator(FormsMigrator::class);
}
Expand Down
154 changes: 73 additions & 81 deletions lib/Controller/ApiController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Check warning on line 235 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L235 

Added line #L235 was not covered by tests

return new DataResponse($this->formsService->getForm($form));
}
Expand All @@ -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}', [
Expand Down Expand Up @@ -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}', [
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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}', [
Expand All @@ -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');

Check warning on line 608 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L605-L608 

Added lines #L605 - L608 were not covered by tests
}

try {
$question = $this->questionMapper->findById($questionId);
} catch (IMapperException $e) {
Expand All @@ -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.');
Expand Down Expand Up @@ -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');

Check warning on line 682 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L679-L682 

Added lines #L679 - L682 were not covered by tests
}

try {
$question = $this->questionMapper->findById($questionId);
} catch (IMapperException $e) {
Expand All @@ -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();

Expand Down Expand Up @@ -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}', [
Expand Down Expand Up @@ -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}', [
Expand Down Expand Up @@ -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}', [
Expand Down Expand Up @@ -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}', [
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Check warning on line 1209 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1209 

Added line #L1209 was not covered by tests

// Delete all submissions (incl. Answers)
$this->submissionMapper->deleteByForm($formId);
Expand Down Expand Up @@ -1337,31 +1321,26 @@ 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()) {
$this->logger->debug('Submission doesn\'t belong to given form');
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);
Expand All @@ -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());
Expand All @@ -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}', [
Expand Down Expand Up @@ -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.)
Expand All @@ -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');
}
}

Expand All @@ -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);

Check warning on line 1637 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1635-L1637 

Added lines #L1635 - L1637 were not covered by tests
}
break;

Check warning on line 1639 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1639 

Added line #L1639 was not covered by tests
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;
}
Expand Down
Loading

0 comments on commit 13870cd

Please sign in to comment.