fix(export): surface checkValidRev error message on bad :rev (#7792)

* fix(export): surface checkValidRev error message in response body

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 <title>Error</title> /
<pre>Internal Server Error</pre>. 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) <noreply@anthropic.com>

* fix(export): validate rev before attachment, broaden error catch

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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-05-17 13:20:05 +01:00 committed by GitHub
parent fba4a17fcc
commit 0e90184b9b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 28 additions and 8 deletions

View file

@ -45,6 +45,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;
@ -59,12 +68,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') {