From 5bc7b96352594a4fd95abaa77c7ade027f5cb48d 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 a1b953b9c06546c1008fbf7ff8df2f93e70f1bc1 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 94500175064c1c3bf869b99061b236e69b2d7627 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 0970ff3b84ec46756be0099b38c98b8c70aa2f64 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 22e7573bc9c23b53584f40376a3505cbc8541fff 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 2f4a9ec6e1..d61cd3bfc8 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 0db11df86e71f01d884bc0487b55984df456e4c2 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 stable34 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') })