diff --git a/src/node/handler/ExportHandler.ts b/src/node/handler/ExportHandler.ts index 4ed2878eb03..115e206fe89 100644 --- a/src/node/handler/ExportHandler.ts +++ b/src/node/handler/ExportHandler.ts @@ -44,6 +44,15 @@ const tempDirectory = os.tmpdir(); * @param {String} type the type to export */ exports.doExport = async (req: any, res: any, padId: string, readOnlyId: string, type:string) => { + // Validate :rev BEFORE setting Content-Disposition. A bad rev causes + // checkValidRev to throw, which the route handler catches and renders as a + // plain-text 500. If we set the attachment header first, the browser would + // download the error message as a file instead of displaying it. + if (req.params.rev !== undefined) { + // modify req, as we use it in a later call to exportConvert + req.params.rev = checkValidRev(req.params.rev); + } + // avoid naming the read-only file as the original pad's id let fileName = readOnlyId ? readOnlyId : padId; @@ -58,12 +67,6 @@ exports.doExport = async (req: any, res: any, padId: string, readOnlyId: string, // tell the browser that this is a downloadable file res.attachment(`${fileName}.${type}`); - if (req.params.rev !== undefined) { - // ensure revision is a number - // modify req, as we use it in a later call to exportConvert - req.params.rev = checkValidRev(req.params.rev); - } - // if this is a plain text export, we can do this directly // We have to over engineer this because tabs are stored as attributes and not plain text if (type === 'etherpad') { diff --git a/src/node/hooks/express/importexport.ts b/src/node/hooks/express/importexport.ts index 6e8dd100399..ece765a3b64 100644 --- a/src/node/hooks/express/importexport.ts +++ b/src/node/hooks/express/importexport.ts @@ -71,7 +71,24 @@ exports.expressCreateServer = (hookName:string, args:ArgsExpressType, cb:Functio console.log(`Exporting pad "${req.params.pad}" in ${req.params.type} format`); await exportHandler.doExport(req, res, padId, readOnlyId, req.params.type); } - })().catch((err) => next(err || new Error(err))); + })().catch((err) => { + // Send a deterministic plain-text body for every export failure. + // checkValidRev throws CustomError('...', 'apierror') for a bad :rev, + // but conversion / fs / soffice errors also reach this handler — and + // without an explicit response, all of them would fall through to + // Express's default HTML error renderer, which is hostile to API + // callers (and would be saved as a file by the browser because of + // the attachment header set in doExport for non-apierror cases). + if (res.headersSent) return next(err || new Error(err)); + // Clear the download header so the error body renders inline instead + // of being saved as the requested filename. + res.removeHeader('Content-Disposition'); + // Log the full error server-side for operators. apierrors are + // user-facing validation errors and not worth a server-side log line. + if (!err || err.name !== 'apierror') console.error('Export error:', err); + const msg = (err && err.message) || 'Internal Server Error'; + return res.status(500).type('text/plain').send(msg); + }); }); // handle import requests diff --git a/src/tests/backend/specs/api/importexportGetPost.ts b/src/tests/backend/specs/api/importexportGetPost.ts index ec8c6536be6..6331008fb5b 100644 --- a/src/tests/backend/specs/api/importexportGetPost.ts +++ b/src/tests/backend/specs/api/importexportGetPost.ts @@ -614,7 +614,7 @@ describe(__filename, function () { .expect((res:any) => assert.equal(res.text, 'ofoo\n')); }); - it('txt request rev test1 is 403', async function () { + it('txt request rev test1 returns 500 with error message', async function () { await agent.get(`/p/${testPadId}/test1/export/txt`) .set("authorization", await common.generateJWTToken()) .expect(500)