Harden Express request handling

README.md

@@ -64,9 +64,13 @@ DATABASE_URL=postgres://user:password@host:5432/database
 UPLOAD_PASSWORD=change-me
 CURRENT_ANALYSIS_VERSION=4
 BODY_LIMIT=25mb
+PUBLIC_FORM_BODY_LIMIT=100kb
 PORT=3000
 ```
 
+`BODY_LIMIT` applies to authenticated analyser JSON and text uploads.
+`PUBLIC_FORM_BODY_LIMIT` is the smaller limit for the public search form.
+
 Run migrations:
 
 ```sh

lib/asyncHandler.js

@@ -0,0 +1,10 @@
+'use strict';
+
+// Express 4 does not automatically forward rejected route promises to error
+// middleware. Keep async handlers concise while preserving the normal `next`
+// error path.
+module.exports = function asyncHandler(handler) {
+  return function wrappedAsyncHandler(req, res, next) {
+    return Promise.resolve(handler(req, res, next)).catch(next);
+  };
+};

routes/index.js

@@ -7,6 +7,7 @@ const jurisdiction = require('../lib/jurisdiction');
 const cache = require('../lib/cache');
 const { isValidAppId } = require('../lib/appId');
 const { classifyAnalysisFailure } = require('../lib/analysisFailure');
+const asyncHandler = require('../lib/asyncHandler');
 
 // Taken from https://reports.exodus-privacy.eu.org/api/trackers
 const exodusTrackers = JSON.parse(fs.readFileSync('./exodusTrackers.json', 'utf-8'))
@@ -150,7 +151,7 @@ async function getSiteData() {
   }
 }
 
-router.get('/', async (req, res) => {
+router.get('/', asyncHandler(async (req, res) => {
   try {
     const data = await getSiteData();
     return res.render('form', {
@@ -172,10 +173,10 @@ router.get('/', async (req, res) => {
       jurisdictionMeta: jurisdiction.classificationMeta
     });
   }
-});
+}));
 
 // Statistics detail page
-router.get('/statistics', async (req, res) => {
+router.get('/statistics', asyncHandler(async (req, res) => {
   try {
     const data = await getSiteData();
     return res.render('statistics', {
@@ -201,19 +202,19 @@ router.get('/statistics', async (req, res) => {
       xrayCompanyCount: jurisdiction.xrayCompanyCount
     });
   }
-});
+}));
 
-router.get('/healthz', async (req, res) => {
+router.get('/healthz', asyncHandler(async (req, res) => {
   try {
     await Apps.healthCheck();
     res.json({ ok: true });
   } catch (err) {
     console.error('Health check failed:', err.message);
     res.status(503).json({ ok: false });
   }
-});
+}));
 
-router.get('/healthz/analyser', async (req, res) => {
+router.get('/healthz/analyser', (req, res) => {
   const online = lastPing > Date.now() - 1000*60*60;
   res.status(online ? 200 : 503).json({ ok: online });
 });
@@ -224,7 +225,7 @@ router.post('/search',
       .isLength({ min: 1 })
       .withMessage('Please enter a search term'),
   ],
-  async (req, res) => {
+  asyncHandler(async (req, res) => {
     const errors = validationResult(req);
 
     if (errors.isEmpty()) {
@@ -252,9 +253,9 @@ router.post('/search',
         data: req.body,
       });
     };
-});
+}));
 
-router.get('/analysis/:appId', async (req, res) => {
+router.get('/analysis/:appId', asyncHandler(async (req, res) => {
   if (!isValidAppId(req.params.appId))
     return res.status(400).send('Please provide a valid App Store bundle ID.');
   let appId = req.params.appId;
@@ -325,35 +326,35 @@ router.get('/analysis/:appId', async (req, res) => {
     trackerNameToExodus: trackerNameToExodus,
     jurisdictionData: jurisdictionData
   });
-});
+}));
 
 // About page
-router.get('/about', async (req, res) => {
+router.get('/about', (req, res) => {
   res.render('about', {
     title: 'About'
   });
 });
 
 // serve next task to analyser
-router.get('/queue', async (req, res) => {
+router.get('/queue', asyncHandler(async (req, res) => {
   let app = await Apps.nextApp();
   console.log(app);
 
   if (!app)
     return res.send();
 
   res.send(app.appid);
-});
+}));
 
 // enable analyser to report online status
-router.get('/ping', async (req, res) => {
+router.get('/ping', (req, res) => {
     lastPing = Date.now();
 
     res.send("online");
 });
 
 // upload analysis results
-router.post('/uploadAnalysis', async (req, res) => {
+router.post('/uploadAnalysis', asyncHandler(async (req, res) => {
   if (!req.query.appId || !req.query.analysisVersion)
     return res.status(400).send('Please provide appId and analysisVersion');
   const appId = req.query.appId;
@@ -371,10 +372,10 @@ router.post('/uploadAnalysis', async (req, res) => {
   const result = await Apps.updateAnalysis(appId, analysis, analysisVersion);
   cache.invalidate('sitedata');
   res.send(result);
-});
+}));
 
 // avoid a loop: only analyse each app once
-router.post('/reportAnalysisFailure', async (req, res) => {
+router.post('/reportAnalysisFailure', asyncHandler(async (req, res) => {
   if (!req.query.appId || !req.query.analysisVersion)
     return res.status(400).send('Please provide appId and analysisVersion');
 
@@ -393,7 +394,7 @@ router.post('/reportAnalysisFailure', async (req, res) => {
   }, req.query.analysisVersion);
   cache.invalidate('sitedata');
   res.send(result);
-});
+}));
 
 /*router.get('/sitemap.xml', async (req, res) => {
     try {

server.js

@@ -48,7 +48,8 @@ if(os.hostname().indexOf("local") <= -1) { // only on remote host
   app.use(limiter)
 }
 
-const bodyLimit = process.env.BODY_LIMIT || '25mb';
+const analyserBodyLimit = process.env.BODY_LIMIT || '25mb';
+const publicFormBodyLimit = process.env.PUBLIC_FORM_BODY_LIMIT || '100kb';
 
 app.use((req, res, next) => {
   if (isAnalyserPath(req) && !analyserAuthenticated(req))
@@ -61,10 +62,15 @@ app.use((req, res, next) => {
 app.set('views', path.join(__dirname, 'views'));
 app.set('view engine', 'pug');
 
-// set up parsing of form inputs and of application/json
-app.use(bodyParser.urlencoded({ extended: true, limit: bodyLimit }));
-app.use(bodyParser.json({ limit: bodyLimit }));
-app.use(express.text({ limit: bodyLimit }));
+// Public requests only need the small search form parser. Large analyser
+// payload parsers are mounted on their authenticated endpoints so arbitrary
+// public and nonexistent routes cannot consume the analyser body allowance.
+app.post('/search', bodyParser.urlencoded({
+  extended: true,
+  limit: publicFormBodyLimit
+}));
+app.post('/uploadAnalysis', bodyParser.json({ limit: analyserBodyLimit }));
+app.post('/reportAnalysisFailure', express.text({ limit: analyserBodyLimit }));
 
 // serve static files
 app.use(express.static('public'));
@@ -77,4 +83,22 @@ app.use('/favicon.ico', express.static('favicon.ico'));
 const routes = require('./routes/index');
 app.use('/', routes);
 
+// Express 4 requires rejected async handlers to call next(err). Route handlers
+// use asyncHandler for that bridge and all errors terminate here.
+app.use((err, req, res, next) => {
+  if (res.headersSent)
+    return next(err);
+
+  const errorStatus = err.status || err.statusCode;
+  const status = Number.isInteger(errorStatus)
+    && errorStatus >= 400
+    && errorStatus <= 599
+    ? errorStatus
+    : 500;
+  if (status >= 500)
+    console.error('Request failed:', err.stack || err.message);
+  const message = err.expose ? err.message : 'Internal server error.';
+  return res.status(status).send(message);
+});
+
 module.exports = app; // make accessible to /start.js

test/requestHandling.test.js

@@ -0,0 +1,143 @@
+'use strict';
+
+const assert = require('node:assert/strict');
+const test = require('node:test');
+const Apps = require('../models/Apps');
+
+process.env.UPLOAD_PASSWORD = 'test-secret';
+process.env.BODY_LIMIT = '2kb';
+process.env.PUBLIC_FORM_BODY_LIMIT = '100b';
+
+const app = require('../server');
+
+async function withServer(run) {
+  const server = await new Promise((resolve) => {
+    const instance = app.listen(0, '127.0.0.1', () => resolve(instance));
+  });
+
+  try {
+    await run(`http://127.0.0.1:${server.address().port}`);
+  } finally {
+    await new Promise((resolve, reject) => {
+      server.close((err) => err ? reject(err) : resolve());
+    });
+  }
+}
+
+test('rejected async route promises reach centralized error middleware', async () => {
+  const originalNextApp = Apps.nextApp;
+  const originalConsoleError = console.error;
+  Apps.nextApp = async () => {
+    throw new Error('database unavailable');
+  };
+  console.error = () => {};
+
+  try {
+    await withServer(async (base) => {
+      const response = await fetch(`${base}/queue`, {
+        headers: { authorization: 'Bearer test-secret' }
+      });
+
+      assert.equal(response.status, 500);
+      assert.equal(await response.text(), 'Internal server error.');
+    });
+  } finally {
+    Apps.nextApp = originalNextApp;
+    console.error = originalConsoleError;
+  }
+});
+
+test('large body parsers are scoped to authenticated analyser endpoints', async () => {
+  const originalUpdateAnalysis = Apps.updateAnalysis;
+  const originalConsoleLog = console.log;
+  const updates = [];
+  Apps.updateAnalysis = async (...args) => {
+    updates.push(args);
+    return 'updated';
+  };
+  console.log = () => {};
+
+  const jsonPayload = JSON.stringify({ report: 'x'.repeat(250) });
+  const textPayload = 'failure log '.repeat(30);
+  const oversizedPayload = JSON.stringify({ report: 'x'.repeat(2100) });
+
+  try {
+    await withServer(async (base) => {
+      const missingResponse = await fetch(`${base}/does-not-exist`, {
+        method: 'POST',
+        headers: { 'content-type': 'application/json' },
+        body: oversizedPayload
+      });
+      assert.equal(missingResponse.status, 404);
+
+      const unauthenticatedResponse = await fetch(
+        `${base}/uploadAnalysis?appId=com.example.app&analysisVersion=1`,
+        {
+          method: 'POST',
+          headers: { 'content-type': 'application/json' },
+          body: oversizedPayload
+        }
+      );
+      assert.equal(unauthenticatedResponse.status, 400);
+
+      const publicFormResponse = await fetch(`${base}/search`, {
+        method: 'POST',
+        headers: { 'content-type': 'application/x-www-form-urlencoded' },
+        body: `search=${'x'.repeat(120)}`
+      });
+      assert.equal(publicFormResponse.status, 413);
+
+      const uploadResponse = await fetch(
+        `${base}/uploadAnalysis?appId=com.example.app&analysisVersion=1`,
+        {
+          method: 'POST',
+          headers: {
+            authorization: 'Bearer test-secret',
+            'content-type': 'application/json'
+          },
+          body: jsonPayload
+        }
+      );
+      assert.equal(uploadResponse.status, 200);
+      assert.equal(await uploadResponse.text(), 'updated');
+
+      const failureResponse = await fetch(
+        `${base}/reportAnalysisFailure?appId=com.example.app&analysisVersion=1`,
+        {
+          method: 'POST',
+          headers: {
+            authorization: 'Bearer test-secret',
+            'content-type': 'text/plain'
+          },
+          body: textPayload
+        }
+      );
+      assert.equal(failureResponse.status, 200);
+      assert.equal(await failureResponse.text(), 'updated');
+
+      const oversizedUploadResponse = await fetch(
+        `${base}/uploadAnalysis?appId=com.example.app&analysisVersion=1`,
+        {
+          method: 'POST',
+          headers: {
+            authorization: 'Bearer test-secret',
+            'content-type': 'application/json'
+          },
+          body: oversizedPayload
+        }
+      );
+      assert.equal(oversizedUploadResponse.status, 413);
+
+      assert.equal(updates.length, 2);
+      assert.deepEqual(updates[0].slice(0, 2), [
+        'com.example.app',
+        JSON.parse(jsonPayload)
+      ]);
+      assert.equal(updates[1][0], 'com.example.app');
+      assert.equal(updates[1][1].logs, textPayload);
+    });
+  } finally {
+    Apps.updateAnalysis = originalUpdateAnalysis;
+    console.log = originalConsoleLog;
+  }
+});