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

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

@@ -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

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)