diff --git a/docs/API_v3.md b/docs/API_v3.md index 72656055d..a8de1054c 100644 --- a/docs/API_v3.md +++ b/docs/API_v3.md @@ -54,7 +54,7 @@ This file contains the API-Documentation. For more information on the returned D - `GET /api/v3/forms/{formId}/questions/{questionId}` to get a single question - `POST /api/v3/forms/{formId}/questions/{questionId}/options` does now accept more options at once - `PATCH /api/v3/forms/{formId}/questions/{questionId}/options` to reorder the options (request body `newOrder`, optional `optionType`) - - `POST /api/v3/forms/{formId}/submissions/files/{questionId}` to upload a file to a file question before submitting the form + - `POST /api/v3/forms/{formId}/submissions/files/{questionId}` to upload a file to a file question before submitting the form (response includes `uploadToken`; submissions must include it with `uploadedFileId`) - `GET /api/v3/forms/{formId}/submissions/{submissionId}` to get a single submission - `PUT /api/v3/forms/{formId}/submissions/{submissionId}` to update an existing submission - In API version 2.5 the following endpoints were introduced: @@ -838,7 +838,7 @@ Delete all Submissions to a form ### Upload a file -Upload a file to an answer before form submission +Upload a file to a file question before form submission. Each uploaded file is stored temporarily and bound to the given `formId` and `questionId`. The response includes an `uploadToken` that must be sent back when submitting the form, together with `uploadedFileId`. - Endpoint: `/api/v3/forms/{formId}/submissions/files/{questionId}` - Method: `POST` @@ -846,17 +846,26 @@ Upload a file to an answer before form submission | Parameter | Type | Description | |--------------|----------------|-------------| | _formId_ | Integer | ID of the form to upload the file to | - | _questionId_ | Integer | ID of the question to upload the file to | + | _questionId_ | Integer | ID of the file question to upload the file to | - Parameters: | Parameter | Type | Description | |--------------|----------------|-------------| | _files_ | Array of files | Files to upload | -- Response: **Status-Code OK**, as well as the id of the uploaded file and it's name. + | _shareHash_ | String | optional, only necessary for uploads on a public share link | +- Response: **Status-Code OK**, as well as a list of uploaded files. ``` -"data": {"uploadedFileId": integer, "fileName": "string"} +"data": [ + { + "uploadedFileId": 42, + "fileName": "document.pdf", + "uploadToken": "a1b2c3d4e5f6..." + } +] ``` +When submitting the form, each file answer must include both `uploadedFileId` and `uploadToken` from this response for the same `questionId`. Uploads from other forms, questions, or sessions are rejected. + ### Get a specific submission Get a specific submission of a form. Viewing another user's submission requires the `results` permission; otherwise only the submission owner may view it. @@ -913,15 +922,15 @@ Store Submission to Database - QuestionID as key - An **array** of values as value --> Even for short Text Answers, wrapped into Array. - For Question-Types with pre-defined answers (`multiple`, `multiple_unique`, `dropdown`), the array contains the corresponding option-IDs. - - For File-Uploads, the array contains the objects with key `uploadedFileId` (value from Upload a file endpoint). + - For File-Uploads, the array contains objects with `uploadedFileId` and `uploadToken` (both from the [Upload a file] endpoint for the same `questionId`). ``` { "1":[27,32], // dropdown or multiple "2":["ShortTextAnswer"], // All Text-Based Question-Types "3":[ // File-Upload - {"uploadedFileId": integer}, - {"uploadedFileId": integer} + {"uploadedFileId": 42, "uploadToken": "a1b2c3d4e5f6..."}, + {"uploadedFileId": 43, "uploadToken": "f6e5d4c3b2a1..."} ], } ``` diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 09a40d241..946dc0445 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -53,6 +53,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; /** @@ -90,6 +91,7 @@ public function __construct( private readonly UploadedFileMapper $uploadedFileMapper, private readonly IMimeTypeDetector $mimeTypeDetector, private readonly IJobList $jobList, + private readonly ISecureRandom $secureRandom, ) { parent::__construct($appName, $request); $this->currentUser = $userSession->getUser(); @@ -1428,7 +1430,7 @@ public function newSubmission(int $formId, array $answers, string $shareHash = ' $questions = $this->formsService->getQuestions($formId); try { // Is the submission valid - $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId()); + $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId(), $formId); } catch (\InvalidArgumentException $e) { throw new OCSBadRequestException($e->getMessage()); } @@ -1524,7 +1526,7 @@ public function updateSubmission(int $formId, int $submissionId, array $answers) $questions = $this->formsService->getQuestions($formId); try { // Is the submission valid - $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId()); + $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId(), $formId); } catch (\InvalidArgumentException $e) { throw new OCSBadRequestException($e->getMessage()); } @@ -1777,8 +1779,11 @@ public function uploadFiles(int $formId, int $questionId, string $shareHash = '' $fileName = $folder->getNonExistingName($uploadedFile['name']); $file = $folder->newFile($fileName, file_get_contents($uploadedFile['tmp_name'])); + $uploadToken = $this->secureRandom->generate(32, ISecureRandom::CHAR_ALPHANUMERIC); $uploadedFileEntity = new UploadedFile(); $uploadedFileEntity->setFormId($formId); + $uploadedFileEntity->setQuestionId($questionId); + $uploadedFileEntity->setUploadToken($uploadToken); $uploadedFileEntity->setOriginalFileName($fileName); $uploadedFileEntity->setFileId($file->getId()); $uploadedFileEntity->setCreated(time()); @@ -1787,6 +1792,7 @@ public function uploadFiles(int $formId, int $questionId, string $shareHash = '' $response[] = [ 'uploadedFileId' => $uploadedFileEntity->getId(), 'fileName' => $fileName, + 'uploadToken' => $uploadToken, ]; } @@ -1801,7 +1807,7 @@ public function uploadFiles(int $formId, int $questionId, string $shareHash = '' * @param Form $form * @param int $submissionId * @param array $question - * @param string[]|array $answerArray + * @param string[]|array $answerArray */ private function storeAnswersForQuestion(Form $form, $submissionId, array $question, array $answerArray): void { if ($question['type'] === Constants::ANSWER_TYPE_GRID || $question['type'] === Constants::ANSWER_TYPE_RANKING) { @@ -1837,7 +1843,12 @@ private function storeAnswersForQuestion(Form $form, $submissionId, array $quest $answerText = str_replace(Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX, '', $answer); } } elseif ($question['type'] === Constants::ANSWER_TYPE_FILE) { - $uploadedFile = $this->uploadedFileMapper->getByUploadedFileId($answer['uploadedFileId']); + $uploadedFile = $this->uploadedFileMapper->getForSubmission( + (int)$answer['uploadedFileId'], + $form->getId(), + $question['id'], + $answer['uploadToken'] ?? '', + ); $answerEntity->setFileId($uploadedFile->getFileId()); $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); diff --git a/lib/Db/UploadedFile.php b/lib/Db/UploadedFile.php index 7e5a92761..b1b2cc320 100644 --- a/lib/Db/UploadedFile.php +++ b/lib/Db/UploadedFile.php @@ -14,6 +14,10 @@ /** * @method int getFormId() * @method void setFormId(int $value) + * @method ?int getQuestionId() + * @method void setQuestionId(?int $value) + * @method ?string getUploadToken() + * @method void setUploadToken(?string $value) * @method string getOriginalFileName() * @method void setOriginalFileName(string $value) * @method int getFileId() @@ -23,6 +27,8 @@ */ class UploadedFile extends Entity { protected $formId; + protected ?int $questionId = null; + protected ?string $uploadToken = null; protected $originalFileName; protected $fileId; protected $created; @@ -32,6 +38,8 @@ class UploadedFile extends Entity { */ public function __construct() { $this->addType('formId', 'integer'); + $this->addType('questionId', 'integer'); + $this->addType('uploadToken', 'string'); $this->addType('originalFileName', 'string'); $this->addType('fileId', 'integer'); $this->addType('created', 'integer'); @@ -41,6 +49,7 @@ public function read(): array { return [ 'id' => $this->getId(), 'formId' => $this->getFormId(), + 'questionId' => $this->getQuestionId(), 'originalFileName' => $this->getOriginalFileName(), 'fileId' => $this->getFileId(), 'created' => $this->getCreated(), diff --git a/lib/Db/UploadedFileMapper.php b/lib/Db/UploadedFileMapper.php index ec2a369ee..00629e7f7 100644 --- a/lib/Db/UploadedFileMapper.php +++ b/lib/Db/UploadedFileMapper.php @@ -55,6 +55,30 @@ public function getByUploadedFileId(string $uploadedFileId): UploadedFile { return $uploadedFile; } + /** + * find all uploaded files for a given form and question and compare with the given upload token. + * + * @throws DoesNotExistException + */ + public function getForSubmission(int $uploadedFileId, int $formId, int $questionId, string $uploadToken): UploadedFile { + $qb = $this->db->getQueryBuilder(); + + $qb->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($uploadedFileId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('form_id', $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('question_id', $qb->createNamedParameter($questionId, IQueryBuilder::PARAM_INT))); + + $uploadedFile = $this->findEntities($qb)[0] ?? null; + if ($uploadedFile === null + || $uploadedFile->getUploadToken() === null + || !hash_equals($uploadedFile->getUploadToken(), $uploadToken)) { + throw new DoesNotExistException(sprintf('Uploaded file with id "%d" not found', $uploadedFileId)); + } + + return $uploadedFile; + } + /** * @param \DateTimeImmutable $dateTime * @return UploadedFile[] diff --git a/lib/Migration/Version050300Date20260713180000.php b/lib/Migration/Version050300Date20260713180000.php new file mode 100644 index 000000000..af85ca6dd --- /dev/null +++ b/lib/Migration/Version050300Date20260713180000.php @@ -0,0 +1,56 @@ +hasTable('forms_v2_uploaded_files')) { + return null; + } + + $table = $schema->getTable('forms_v2_uploaded_files'); + $changed = false; + + if (!$table->hasColumn('question_id')) { + $table->addColumn('question_id', Types::INTEGER, [ + 'notnull' => false, + ]); + $changed = true; + } + + if (!$table->hasColumn('upload_token')) { + $table->addColumn('upload_token', Types::STRING, [ + 'notnull' => false, + 'length' => 64, + ]); + $changed = true; + } + + return $changed ? $schema : null; + } +} diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 439e196b5..2eed57e39 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -151,7 +151,8 @@ * * @psalm-type FormsUploadedFile = array{ * uploadedFileId: int, - * fileName: string + * fileName: string, + * uploadToken: string, * } */ class ResponseDefinitions { diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index 21b524b75..560da02fb 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -470,9 +470,10 @@ private function escapeCSV(string $value): string { * @param array $questions Array of the questions of the form * @param array $answers Array of the submitted answers * @param string $formOwnerId Owner of the form + * @param int $formId Id of the form being submitted * @throw \InvalidArgumentException if validation failed */ - public function validateSubmission(array $questions, array $answers, string $formOwnerId): void { + public function validateSubmission(array $questions, array $answers, string $formOwnerId, int $formId): void { // Check by questions foreach ($questions as $question) { $questionId = $question['id']; @@ -485,7 +486,7 @@ public function validateSubmission(array $questions, array $answers, string $for if (is_array($value)) { // file type if (isset($value['uploadedFileId'])) { - return !empty($value['uploadedFileId']); + return !empty($value['uploadedFileId']) && !empty($value['uploadToken']); } // Grid questions @@ -613,8 +614,14 @@ public function validateSubmission(array $questions, array $answers, string $for } foreach ($answers[$questionId] as $answer) { - $uploadedFile = $this->uploadedFileMapper->findByUploadedFileId($answer['uploadedFileId']); - if (!$uploadedFile) { + try { + $uploadedFile = $this->uploadedFileMapper->getForSubmission( + (int)$answer['uploadedFileId'], + $formId, + $questionId, + $answer['uploadToken'] ?? '', + ); + } catch (DoesNotExistException) { throw new \InvalidArgumentException(sprintf('File "%s" for question "%s" not exists anymore. Please delete and re-upload the file.', $answer['fileName'] ?? $answer['uploadedFileId'], $question['text'])); } diff --git a/openapi.json b/openapi.json index 3ddf90c46..fc3850658 100644 --- a/openapi.json +++ b/openapi.json @@ -703,7 +703,8 @@ "type": "object", "required": [ "uploadedFileId", - "fileName" + "fileName", + "uploadToken" ], "properties": { "uploadedFileId": { @@ -712,6 +713,9 @@ }, "fileName": { "type": "string" + }, + "uploadToken": { + "type": "string" } } } diff --git a/tests/Integration/Api/ApiV3Test.php b/tests/Integration/Api/ApiV3Test.php index 36e98f028..e9816f496 100644 --- a/tests/Integration/Api/ApiV3Test.php +++ b/tests/Integration/Api/ApiV3Test.php @@ -766,12 +766,13 @@ public function testDeleteFormWithSubmissions() { $uploadedFileData = $this->OcsResponse2Data($uploadedFileResponse); $uploadedFileId = $uploadedFileData[0]['uploadedFileId']; + $uploadToken = $uploadedFileData[0]['uploadToken']; // Create a submission with the file $resp = $this->http->request('POST', "api/v3/forms/$formId/submissions", [ 'json' => [ 'answers' => [ - $questionId => [['uploadedFileId' => $uploadedFileId]] + $questionId => [['uploadedFileId' => $uploadedFileId, 'uploadToken' => $uploadToken]] ] ] ]); @@ -1389,13 +1390,14 @@ public function testDeleteSubmissionWithFiles() { $uploadedFileData = $this->OcsResponse2Data($uploadedFileResponse); $uploadedFileId = $uploadedFileData[0]['uploadedFileId']; + $uploadToken = $uploadedFileData[0]['uploadToken']; // Create a new submission with the file $resp = $this->http->request('POST', "api/v3/forms/{$this->testForms[0]['id']}/submissions", [ 'json' => [ 'answers' => [ $this->testForms[0]['questions'][0]['id'] => ['Test Answer'], - $this->testForms[0]['questions'][2]['id'] => [['uploadedFileId' => $uploadedFileId]] + $this->testForms[0]['questions'][2]['id'] => [['uploadedFileId' => $uploadedFileId, 'uploadToken' => $uploadToken]] ] ] ]); @@ -1504,6 +1506,7 @@ public function testNewSubmission() { $data = $this->OcsResponse2Data($uploadedFileResponse); $uploadedFileId = $data[0]['uploadedFileId']; + $uploadToken = $data[0]['uploadToken']; $resp = $this->http->request('POST', "api/v3/forms/{$this->testForms[0]['id']}/submissions", [ 'json' => [ @@ -1512,7 +1515,7 @@ public function testNewSubmission() { $this->testForms[0]['questions'][1]['id'] => [ $this->testForms[0]['questions'][1]['options'][0]['id'] ], - $this->testForms[0]['questions'][2]['id'] => [['uploadedFileId' => $uploadedFileId]] + $this->testForms[0]['questions'][2]['id'] => [['uploadedFileId' => $uploadedFileId, 'uploadToken' => $uploadToken]] ] ] ]); diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 79d908675..4ef57fba9 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -40,6 +40,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCA\Forms\Db\QuestionMapper; use OCA\Forms\Db\Submission; use OCA\Forms\Db\SubmissionMapper; +use OCA\Forms\Db\UploadedFile; use OCA\Forms\Db\UploadedFileMapper; use OCA\Forms\Exception\NoSuchFormException; use OCA\Forms\Service\ConfigService; @@ -64,6 +65,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -87,6 +89,7 @@ class ApiControllerTest extends TestCase { private UploadedFileMapper|MockObject $uploadedFileMapper; private IMimeTypeDetector|MockObject $mimeTypeDetector; private IJobList|MockObject $jobList; + private ISecureRandom|MockObject $secureRandom; public function setUp(): void { $this->answerMapper = $this->createMock(AnswerMapper::class); @@ -109,6 +112,8 @@ public function setUp(): void { $this->uploadedFileMapper = $this->createMock(UploadedFileMapper::class); $this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class); $this->jobList = $this->createMock(IJobList::class); + $this->secureRandom = $this->createMock(ISecureRandom::class); + $this->secureRandom->method('generate')->willReturn('abcdefghijklmnopqrstuvwxyz012345'); $this->apiController = new ApiController( 'forms', @@ -130,6 +135,7 @@ public function setUp(): void { $this->uploadedFileMapper, $this->mimeTypeDetector, $this->jobList, + $this->secureRandom, ); } @@ -784,7 +790,7 @@ public function testNewSubmission_answers() { 1 => ['3'], 2 => ['2', '5', Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX . 'other answer'], 3 => ['short anwer'], - 4 => [['uploadedFileId' => 100]], + 4 => [['uploadedFileId' => 100, 'uploadToken' => 'valid-upload-token']], 5 => ['ignore unknown question'], ]; @@ -849,6 +855,13 @@ public function testNewSubmission_answers() { $file = $this->createMock(File::class); + $uploadedFile = new UploadedFile(); + $uploadedFile->setFileId(100); + $this->uploadedFileMapper->expects($this->once()) + ->method('getForSubmission') + ->with(100, 1, 4, 'valid-upload-token') + ->willReturn($uploadedFile); + $userFolder->expects($this->once()) ->method('getById') ->willReturn([$file]); diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index 08266087b..d1400ec0c 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -1361,7 +1361,34 @@ public function testValidateSubmission(array $questions, array $answers, ?string $this->expectExceptionMessage($expected); } - $this->submissionService->validateSubmission($questions, $answers, 'admin'); + $this->submissionService->validateSubmission($questions, $answers, 'admin', 1); $this->assertTrue(true); } + + public function testValidateSubmission_rejectsForeignUploadToken(): void { + $questions = [[ + 'id' => 8, + 'type' => Constants::ANSWER_TYPE_FILE, + 'text' => 'File question', + 'isRequired' => false, + 'extraSettings' => [], + ]]; + $answers = [ + 8 => [[ + 'uploadedFileId' => 10, + 'fileName' => 'victim.rtf', + 'uploadToken' => 'attacker-token', + ]], + ]; + + $this->uploadedFileMapper->expects($this->once()) + ->method('getForSubmission') + ->with(10, 7, 8, 'attacker-token') + ->willThrowException(new DoesNotExistException('Upload not found')); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('File "victim.rtf" for question "File question" not exists anymore. Please delete and re-upload the file.'); + + $this->submissionService->validateSubmission($questions, $answers, 'alice', 7); + } };