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>

Author: JohnMcLear

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') {

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);
     });
   });