Handle additional bodyParser errors (#1333)

Closes getodk/central#788. --------- Co-authored-by: alxndrsn <alxndrsn>
getodk · Dec 9, 2024 · e99419a · e99419a
1 parent f931a4e
commit e99419a
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 9 deletions.
diff --git a/lib/http/service.js b/lib/http/service.js
@@ -122,14 +122,18 @@ module.exports = (container) => {
   service.use((error, request, response, next) => {
     if (response.headersSent === true) {
       // In this case, we'll just let Express fail the request out.
-      next(error);
-    } else if (error?.type === 'entity.parse.failed') {
-      // catch body-parser middleware problems. we only ask it to parse JSON, which
-      // isn't part of OpenRosa, so we can assume a plain JSON response.
-      defaultErrorWriter(Problem.user.unparseable({ format: 'json', rawLength: error.body.length }), request, response);
-    } else {
-      defaultErrorWriter(error, request, response);
+      return next(error);
+    }
+
+    // catch body-parser middleware problems. we only ask it to parse JSON, which
+    // isn't part of OpenRosa, so we can assume a plain JSON response.
+    switch (error?.type) {
+      case 'encoding.unsupported': return defaultErrorWriter(Problem.user.encodingNotSupported(), request, response);
+      case 'entity.parse.failed': return defaultErrorWriter(Problem.user.unparseable({ format: 'json', rawLength: error.body.length }), request, response);
+      case 'entity.too.large': return defaultErrorWriter(Problem.user.requestTooLarge(), request, response);
+      default: return defaultErrorWriter(error, request, response);
     }
+
   });
 
   return service;

diff --git a/lib/util/problem.js b/lib/util/problem.js
@@ -136,6 +136,10 @@ const problems = {
 
     odataInvalidSkiptoken: problem(400.35, () => 'Invalid $skiptoken'),
 
+    requestTooLarge: problem(400.36, () => 'Request body too large.'),
+
+    encodingNotSupported: problem(400.37, () => 'Encoding not supported.'),
+
     // no detail information for security reasons.
     authenticationFailed: problem(401.2, () => 'Could not authenticate with the provided credentials.'),
 

diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js
@@ -3816,9 +3816,12 @@ describe('Entities API', () => {
       // 250kb limit = 256000 bytes (1024 bytes per kb)
       await asAlice.post('/v1/projects/1/datasets')
         .send({ name: 'x'.repeat(256001) })
-        .expect(500)
+        .expect(400)
         .then(({ body }) => {
-          body.message.should.equal('Internal Server Error');
+          body.should.eql({
+            code: 400.36,
+            message: 'Request body too large.',
+          });
         });
     }));
 

diff --git a/test/integration/other/body-parser.js b/test/integration/other/body-parser.js
@@ -1,3 +1,4 @@
+const { randomBytes } = require('node:crypto');
 const { testService } = require('../setup');
 
 describe('bodyParser', () => {
@@ -12,6 +13,33 @@ describe('bodyParser', () => {
           body.details.should.eql({ format: 'json', rawLength: 15 });
         }))));
 
+  it('should return a reasonable error on too big requests', testService((service) =>
+    service.login('alice', (asAlice) =>
+      asAlice.post('/v1/projects')
+        .set('Content-Type', 'application/json')
+        .send(JSON.stringify({ data: randomBytes(191_992).toString('base64') }))
+        .expect(400)
+        .then(({ body }) => {
+          body.should.eql({
+            code: 400.36,
+            message: 'Request body too large.',
+          });
+        }))));
+
+  it('should return a reasonable error on bad Content-Encoding header', testService((service) =>
+    service.login('alice', (asAlice) =>
+      asAlice.post('/v1/projects')
+        .set('Content-Type', 'application/json')
+        .set('Content-Encoding', 'wat')
+        .send('{}')
+        .expect(400)
+        .then(({ body }) => {
+          body.should.eql({
+            code: 400.37,
+            message: 'Encoding not supported.',
+          });
+        }))));
+
   it('should return a formatted 404 on routematch failure', testService((service) =>
     service.get('/v1/nonexistent')
       .expect(404)