From 13c301d26c1dd666732cfbc6baaa5a4b7fdb8817 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 11 Sep 2026 11:54:42 -0400 Subject: [PATCH 1/5] fix(templates): handle upload errors properly When trying to upload a global template that failed (e.g. file was too large), it would try to detect the MIME type of the file even if it didn't exist, causing a 500 Internal Server Error response. Instead, we should check for errors first, and then try to detect the MIME type only after we know there are no errors. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Elizabeth Danzberger --- lib/Controller/TemplatesController.php | 9 +++++++-- src/components/AdminSettings/GlobalTemplates.vue | 16 +++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/lib/Controller/TemplatesController.php b/lib/Controller/TemplatesController.php index 6d90f489f2..692efaa30a 100644 --- a/lib/Controller/TemplatesController.php +++ b/lib/Controller/TemplatesController.php @@ -101,17 +101,22 @@ public function add() { $files = $this->request->getUploadedFile('files'); if (!is_null($files)) { - $mimeType = !empty($files['type'] ?? '') ? $files['type'] : $this->mimeTypeDetector->detect($files['tmp_name']); $error = $files['error'] ?? 0; if ($error !== 0) { $this->logger->error('Failed to get the uploaded file. PHP file upload error code: ' . $error); + $message = match ($error) { + UPLOAD_ERR_INI_SIZE, UPLOAD_ERR_FORM_SIZE => $this->l10n->t('File is too big'), + default => $this->l10n->t('Failed to upload the file'), + }; return new JSONResponse( - ['data' => ['message' => $this->l10n->t('Failed to upload the file')]], + ['data' => ['message' => $message]], Http::STATUS_BAD_REQUEST ); } + $mimeType = !empty($files['type'] ?? '') ? $files['type'] : $this->mimeTypeDetector->detect($files['tmp_name']); + if (is_uploaded_file($files['tmp_name']) && !Filesystem::isFileBlacklisted($files['tmp_name'])) { if ($files['size'] > $this->maxSize) { return new JSONResponse( diff --git a/src/components/AdminSettings/GlobalTemplates.vue b/src/components/AdminSettings/GlobalTemplates.vue index 1efb85e68d..b2c9d75413 100644 --- a/src/components/AdminSettings/GlobalTemplates.vue +++ b/src/components/AdminSettings/GlobalTemplates.vue @@ -119,6 +119,10 @@ export default { if (!templateAlreadyExists) { const template = await this.uploadTemplate(selectedFile) + if (template === null) { + return + } + this.existingTemplates.push(template) showSuccess(t('richdocuments', 'Uploaded template "{name}"', { name: template.name })) } else { @@ -132,18 +136,20 @@ export default { formData.append('files', file) - let res = null try { - res = await axios.post(url, formData, { + const res = await axios.post(url, formData, { headers: { 'Content-Type': 'multipart/form-data', }, }) + + return res.data.data } catch (error) { - showError(error.response.data.data.message) - } + showError(error.response?.data?.data?.message + ?? t('richdocuments', 'Failed to upload the file')) - return res.data.data + return null + } }, async deleteTemplate(templateId) { From 3cac650cda6b68026a240e0aa57ada2b30da957b Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:30:17 -0400 Subject: [PATCH 2/5] fix(templates): reset file input after upload Signed-off-by: Elizabeth Danzberger Assisted-by: ClaudeCode:claude-opus-5 --- .../AdminSettings/GlobalTemplates.vue | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/components/AdminSettings/GlobalTemplates.vue b/src/components/AdminSettings/GlobalTemplates.vue index b2c9d75413..595605d9a2 100644 --- a/src/components/AdminSettings/GlobalTemplates.vue +++ b/src/components/AdminSettings/GlobalTemplates.vue @@ -112,11 +112,21 @@ export default { async selectFile() { const selectedFile = this.$refs.newTemplateInput?.files[0] - const templateAlreadyExists = this.existingTemplates.some((template) => { - return template.name === selectedFile.name - }) - if (!templateAlreadyExists) { + if (!selectedFile) { + return + } + + try { + const templateAlreadyExists = this.existingTemplates.some((template) => { + return template.name === selectedFile.name + }) + + if (templateAlreadyExists) { + showError(t('richdocuments', 'Template "{name}" already exists', { name: selectedFile.name })) + return + } + const template = await this.uploadTemplate(selectedFile) if (template === null) { @@ -125,8 +135,9 @@ export default { this.existingTemplates.push(template) showSuccess(t('richdocuments', 'Uploaded template "{name}"', { name: template.name })) - } else { - showError(t('richdocuments', 'Template "{name}" already exists', { name: selectedFile.name })) + } finally { + // Allow selecting the same file again after a failed upload + this.$refs.newTemplateInput.value = '' } }, From 46b51e9cc8ee07e704a0c6dbc672a98c9fd9ab98 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:36:02 -0400 Subject: [PATCH 3/5] fix(templates): failed vs. empty uploads Distinguish between an upload that failed and an upload that simply contained nothing Signed-off-by: Elizabeth Danzberger Assisted-by: ClaudeCode:claude-opus-5 --- lib/Controller/TemplatesController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Controller/TemplatesController.php b/lib/Controller/TemplatesController.php index 692efaa30a..e19ca4c464 100644 --- a/lib/Controller/TemplatesController.php +++ b/lib/Controller/TemplatesController.php @@ -107,6 +107,7 @@ public function add() { $this->logger->error('Failed to get the uploaded file. PHP file upload error code: ' . $error); $message = match ($error) { UPLOAD_ERR_INI_SIZE, UPLOAD_ERR_FORM_SIZE => $this->l10n->t('File is too big'), + UPLOAD_ERR_NO_FILE => $this->l10n->t('No file was uploaded'), default => $this->l10n->t('Failed to upload the file'), }; return new JSONResponse( From 1404ae2040831d4e2ca9edb8369a421e5f7fabee Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:42:22 -0400 Subject: [PATCH 4/5] test: template controller unit tests Signed-off-by: Elizabeth Danzberger Assisted-by: ClaudeCode:claude-opus-5 --- .../Controller/TemplatesControllerTest.php | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 tests/lib/Controller/TemplatesControllerTest.php diff --git a/tests/lib/Controller/TemplatesControllerTest.php b/tests/lib/Controller/TemplatesControllerTest.php new file mode 100644 index 0000000000..0f150fd604 --- /dev/null +++ b/tests/lib/Controller/TemplatesControllerTest.php @@ -0,0 +1,137 @@ +request = $this->createMock(IRequest::class); + $this->l10n = $this->createMock(IL10N::class); + $this->manager = $this->createMock(TemplateManager::class); + $this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->l10n->method('t')->willReturnArgument(0); + } + + private function makeController(): TemplatesController { + return new TemplatesController( + 'richdocuments', + $this->request, + $this->l10n, + $this->manager, + $this->createMock(IPreview::class), + $this->mimeTypeDetector, + $this->logger, + ); + } + + private function givenUploadedFile(?array $file): void { + $this->request->method('getUploadedFile')->with('files')->willReturn($file); + } + + private function getMessage(Http\Response $response): ?string { + return $response->getData()['data']['message'] ?? null; + } + + public static function uploadErrorProvider(): array { + return [ + 'exceeds upload_max_filesize' => [UPLOAD_ERR_INI_SIZE, 'File is too big'], + 'exceeds MAX_FILE_SIZE' => [UPLOAD_ERR_FORM_SIZE, 'File is too big'], + 'no file submitted' => [UPLOAD_ERR_NO_FILE, 'No file was uploaded'], + 'partial upload' => [UPLOAD_ERR_PARTIAL, 'Failed to upload the file'], + 'missing temp dir' => [UPLOAD_ERR_NO_TMP_DIR, 'Failed to upload the file'], + 'write failed' => [UPLOAD_ERR_CANT_WRITE, 'Failed to upload the file'], + 'stopped by extension' => [UPLOAD_ERR_EXTENSION, 'Failed to upload the file'], + ]; + } + + /** + * @dataProvider uploadErrorProvider + */ + public function testAddReportsUploadErrors(int $error, string $expectedMessage): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => '', + 'tmp_name' => '', + 'error' => $error, + 'size' => 0, + ]); + $this->logger->expects($this->once())->method('error'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame($expectedMessage, $this->getMessage($response)); + } + + /** + * A failed upload leaves tmp_name empty, and detecting the mime type of an empty path + * raises a ValueError that surfaces as a 500 instead of the upload error. + * + * @dataProvider uploadErrorProvider + */ + public function testAddDoesNotDetectMimeTypeOfAFailedUpload(int $error): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => '', + 'tmp_name' => '', + 'error' => $error, + 'size' => 0, + ]); + $this->mimeTypeDetector->expects($this->never())->method('detect'); + + $this->makeController()->add(); + } + + public function testAddRejectsAMissingUpload(): void { + $this->givenUploadedFile(null); + $this->mimeTypeDetector->expects($this->never())->method('detect'); + $this->manager->expects($this->never())->method('add'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame('Invalid file provided', $this->getMessage($response)); + } + + public function testAddRejectsAFileThatWasNotUploaded(): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => 'application/vnd.oasis.opendocument.presentation-template', + 'tmp_name' => '/etc/passwd', + 'error' => UPLOAD_ERR_OK, + 'size' => 1024, + ]); + $this->manager->expects($this->never())->method('add'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame('Invalid file provided', $this->getMessage($response)); + } +} From 453edc03aef0bea9cd60ecefb7c6997a82e47d11 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:51:24 -0400 Subject: [PATCH 5/5] test: cypress coverage for errors Signed-off-by: Elizabeth Danzberger Assisted-by: ClaudeCode:claude-opus-5 --- cypress/e2e/templates.spec.js | 56 +++++++++++++++++++++++++++++++++++ cypress/support/commands.js | 14 +++++---- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/cypress/e2e/templates.spec.js b/cypress/e2e/templates.spec.js index 741195b7db..8d7c897a43 100644 --- a/cypress/e2e/templates.spec.js +++ b/cypress/e2e/templates.spec.js @@ -58,6 +58,62 @@ describe('Global templates', function() { }) }) + it('Surfaces the reason the server rejected the upload', function() { + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 400, + body: { data: { message: 'File is too big' } }, + }).as('templateUploadRequest') + + cy.uploadSystemTemplate({ + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + }) + + cy.wait('@templateUploadRequest') + cy.contains('[role="alert"]', 'File is too big').should('be.visible') + cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') + }) + + it('Reports a failure even when the error response carries no message', function() { + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 500, + body: '', + }).as('templateUploadRequest') + + cy.uploadSystemTemplate({ + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + }) + + cy.wait('@templateUploadRequest') + cy.contains('[role="alert"]', 'Failed to upload the file').should('be.visible') + cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') + }) + + it('Can be picked a second time after its upload failed', function() { + const template = { + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + } + + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 400, + body: { data: { message: 'Failed to upload the file' } }, + }).as('templateUploadRequest') + + cy.uploadSystemTemplate(template) + cy.wait('@templateUploadRequest') + + // Picking the same file only fires a change event if the input was reset + cy.selectSystemTemplate(template) + cy.wait('@templateUploadRequest') + + cy.get('@templateUploadRequest.all').should('have.length', 2) + }) + it('Can be created by a user', () => { cy.uploadSystemTemplate({ fixturePath: 'templates/presentation.otp', diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 6630aab64a..893d6ca7ed 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -368,6 +368,14 @@ Cypress.Commands.add('verifyOpen', (filename) => { }) }) +Cypress.Commands.add('selectSystemTemplate', ({ fixturePath, fileName, mimeType }) => { + cy.get('.settings-section input[type="file"]').selectFile({ + contents: `cypress/fixtures/${fixturePath}`, + fileName, + mimeType, + }, { force: true }) +}) + Cypress.Commands.add('uploadSystemTemplate', ({ fixturePath, fileName, mimeType }) => { cy.login(new User('admin', 'admin')) cy.visit('/settings/admin/richdocuments') @@ -376,11 +384,7 @@ Cypress.Commands.add('uploadSystemTemplate', ({ fixturePath, fileName, mimeType .contains('Global Templates') .scrollIntoView() - cy.get('.settings-section input[type="file"]').selectFile({ - contents: `cypress/fixtures/${fixturePath}`, - fileName, - mimeType, - }, { force: true }) + cy.selectSystemTemplate({ fixturePath, fileName, mimeType }) }) Cypress.Commands.add('submitTemplateFields', (fields) => {