From b537f1688dc14ff78d0a8d5e8fb8d6da7bc8646f Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 17 May 2026 13:03:45 +0100 Subject: [PATCH 1/2] fix(export): surface checkValidRev error message in response body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-numeric :rev (e.g. /p/foo/test1/export/txt) was reaching checkValidRev, which throws CustomError('rev is not a number', 'apierror'). The error fell through the route handler's .catch(next), so Express's default error renderer kicked in and returned a 500 with the generic HTML page Error / 
Internal Server Error
. The thrown message never made it to the body, so callers had no way to tell why the request failed. Catch the apierror in the route handler and send err.message as a text/plain 500 body. Other errors still propagate to next(err) so unrelated failures keep their existing handling. Also retitle the test (was "is 403" while asserting expect(500)) — leftover label from an earlier expectation. Fixes #7788 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/node/hooks/express/importexport.ts | 10 +++++++++- src/tests/backend/specs/api/importexportGetPost.ts | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/node/hooks/express/importexport.ts b/src/node/hooks/express/importexport.ts index 6e8dd100399..4430984440f 100644 --- a/src/node/hooks/express/importexport.ts +++ b/src/node/hooks/express/importexport.ts @@ -71,7 +71,15 @@ 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) => { + // checkValidRev throws CustomError('...', 'apierror') for non-numeric or + // out-of-range :rev. Surface the message as a plain-text body so callers + // see why the request failed instead of Express's default HTML page. + if (err && err.name === 'apierror' && !res.headersSent) { + return res.status(500).type('text/plain').send(err.message); + } + return next(err || new Error(err)); + }); }); // 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) From a075b498da9e47929aa7cb5427fad010b42fad72 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 17 May 2026 13:18:56 +0100 Subject: [PATCH 2/2] fix(export): validate rev before attachment, broaden error catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo feedback on #7792: 1) ExportHandler.doExport set Content-Disposition (via res.attachment) before calling checkValidRev. If the rev was invalid, the route-level catch returned a plain-text 500 — but the attachment header was still in place, so browsers offered to save the error message as a file. Move checkValidRev to the top of doExport so an invalid rev never touches the attachment header. 2) The catch only converted CustomError('...', 'apierror') into a plain-text response. Other export errors (conversion failures, fs issues, soffice problems) still fell through to Express's default HTML renderer — non-deterministic for API callers and confusing when they were already half-downloading a file. Surface every export failure as a deterministic text/plain 500. apierrors carry user-facing messages, so send err.message verbatim. For other errors, log the full stack server-side and still emit err.message (or 'Internal Server Error' if absent) so the response body is never the Express HTML stack page. Also clear Content-Disposition in the catch as a safety net. Backend tests (importexportGetPost.ts): 58 passing, 0 failing. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/node/handler/ExportHandler.ts | 15 +++++++++------ src/node/hooks/express/importexport.ts | 23 ++++++++++++++++------- 2 files changed, 25 insertions(+), 13 deletions(-) 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 4430984440f..ece765a3b64 100644 --- a/src/node/hooks/express/importexport.ts +++ b/src/node/hooks/express/importexport.ts @@ -72,13 +72,22 @@ exports.expressCreateServer = (hookName:string, args:ArgsExpressType, cb:Functio await exportHandler.doExport(req, res, padId, readOnlyId, req.params.type); } })().catch((err) => { - // checkValidRev throws CustomError('...', 'apierror') for non-numeric or - // out-of-range :rev. Surface the message as a plain-text body so callers - // see why the request failed instead of Express's default HTML page. - if (err && err.name === 'apierror' && !res.headersSent) { - return res.status(500).type('text/plain').send(err.message); - } - return next(err || new Error(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); }); });