From 1510d827e80d51c396d6295241011bf135013500 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 11 Sep 2026 11:54:42 -0400 Subject: [PATCH 1/6] 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 5b1a82c7b3..f3ce454a88 100644 --- a/src/components/AdminSettings/GlobalTemplates.vue +++ b/src/components/AdminSettings/GlobalTemplates.vue @@ -111,6 +111,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 { @@ -123,18 +127,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) { const url = generateUrl('/apps/richdocuments/template/' + templateId) From e138ed0a43f0112e0ecb44d445af8cb4197e2b00 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:30:17 -0400 Subject: [PATCH 2/6] 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 f3ce454a88..85076968b1 100644 --- a/src/components/AdminSettings/GlobalTemplates.vue +++ b/src/components/AdminSettings/GlobalTemplates.vue @@ -104,11 +104,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) { @@ -117,8 +127,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 = '' } }, async uploadTemplate(file) { From 1b81cbbb5be39e2d4153555989c0796c2cf38f32 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:36:02 -0400 Subject: [PATCH 3/6] 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 aac2c697e19aee90c4481c0a1ba9891a4b055631 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:42:22 -0400 Subject: [PATCH 4/6] 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 47cde94634dd0df650b5225eaaf0fa3dc4bd55b4 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Fri, 9 Oct 2026 13:51:24 -0400 Subject: [PATCH 5/6] 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 d375250094..b9cbdd487a 100644 --- a/cypress/e2e/templates.spec.js +++ b/cypress/e2e/templates.spec.js @@ -59,6 +59,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 b088575281..4efb9f93e1 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -372,6 +372,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') @@ -380,11 +388,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) => { From caa0d603550389b593f4b33ce3b9da43f5b1394c Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Mon, 19 Oct 2026 14:22:04 -0400 Subject: [PATCH 6/6] test: adjust assertion for older dialogs version On stable35 the @nextcloud/dialogs library is on v6, but the test assertions were done with v7 in mind on main. So they need to be adjusted to correctly identify the toast message and assert that the content matches Signed-off-by: Elizabeth Danzberger Assisted-by: ClaudeCode:claude-opus-5 --- cypress/e2e/templates.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cypress/e2e/templates.spec.js b/cypress/e2e/templates.spec.js index b9cbdd487a..3e04a1d522 100644 --- a/cypress/e2e/templates.spec.js +++ b/cypress/e2e/templates.spec.js @@ -72,7 +72,7 @@ describe('Global templates', function() { }) cy.wait('@templateUploadRequest') - cy.contains('[role="alert"]', 'File is too big').should('be.visible') + cy.get('.toast-error').contains('File is too big').should('be.visible') cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') }) @@ -89,7 +89,7 @@ describe('Global templates', function() { }) cy.wait('@templateUploadRequest') - cy.contains('[role="alert"]', 'Failed to upload the file').should('be.visible') + cy.get('.toast-error').contains('Failed to upload the file').should('be.visible') cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') })