From d6648a0780faa9888d7ae43073a025f23f5f7592 Mon Sep 17 00:00:00 2001
From: Gabriel Wicke <gwicke@wikimedia.org>
Date: Thu, 23 Apr 2015 16:47:33 -0700
Subject: [PATCH] Improve invalid request handling

- Fix reading of PUT / JSON POST requests
- Catch & report errors for invalid POSTs
- Make sure the internal request is always defined, so that we can use it in
  error reports
- Add tests
---
 lib/rbUtil.js                           | 14 +++------
 lib/server.js                           | 41 ++++++++++++++++---------
 test/features/errors/invalid_request.js | 41 +++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 24 deletions(-)
 create mode 100644 test/features/errors/invalid_request.js

diff --git a/lib/rbUtil.js b/lib/rbUtil.js
index 4dc6ad747..39327e5d9 100644
--- a/lib/rbUtil.js
+++ b/lib/rbUtil.js
@@ -54,7 +54,7 @@ function read(req) {
         });
 
         req.on('end', function() {
-            return Buffer.concat(chunks);
+            resolve(Buffer.concat(chunks));
         });
     });
 }
@@ -64,7 +64,8 @@ rbUtil.parsePOST = function parsePOST(req) {
     var readIt =
         (req.method === 'PUT') ||
         (req.method === 'POST' && req.headers &&
-         /^application\/json/i.test(req.headers['content-type']));
+         (/^application\/json/i.test(req.headers['content-type'])
+            || !req.headers['content-type']));
 
     if (readIt) {
         return read(req);
@@ -72,17 +73,10 @@ rbUtil.parsePOST = function parsePOST(req) {
         return P.resolve();
     } else {
         // Parse the POST
-        var headers = req.headers;
-        if (!headers['content-type']) {
-            headers = {
-                'content-type': 'application/binary'
-            };
-        }
-
         return new P(function(resolve) {
             // Parse POST data
             var bboy = new Busboy({
-                headers: headers,
+                headers: req.headers,
                 // Increase the form field size limit from the 1M default.
                 limits: { fieldSize: 15 * 1024 * 1024 }
             });
diff --git a/lib/server.js b/lib/server.js
index cbdef4d4f..42726544a 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -84,20 +84,24 @@ function handleResponse (opts, req, resp, response) {
                 uri: rBody.uri
             };
 
+            if (!response.headers['content-type']) {
+                response.headers['content-type'] = 'application/problem+json';
+            }
+
             if (response.status === 404) {
                 if (!body.type) { body.type = 'not_found'; }
                 if (!body.title) { body.title = 'Not found.'; }
-                if (!response.headers['content-type']) {
-                    response.headers['content-type'] = 'application/problem+json';
-                }
             }
+
             if (response.status >= 400) {
                 if (!body.uri) { body.uri = req.uri; }
                 if (!body.method) { body.method = req.method; }
             }
+
             if (!body.type) {
                 body.type = 'unknown_error';
             }
+
             // Prefix error base URL
             // XXX: make the prefix configurable
             body.type = 'https://restbase.org/errors/' + body.type;
@@ -165,8 +169,6 @@ function handleResponse (opts, req, resp, response) {
 
 // Handle a single request
 function handleRequest (opts, req, resp) {
-    var newReq;
-
     // set the request ID early on for external requests
     req.headers = req.headers || {};
     req.headers['x-request-id'] = req.headers['x-request-id'] || rbUtil.generateRequestId();
@@ -186,14 +188,30 @@ function handleRequest (opts, req, resp) {
     };
     reqOpts.log = reqOpts.logger.log.bind(reqOpts.logger);
 
+    // Create a new, clean request object
+    var urlData = rbUtil.parseURL(req.url);
+
+    var newReq = {
+        uri: new URI(urlData.pathname),
+        query: urlData.query,
+        method: req.method.toLowerCase(),
+        headers: req.headers,
+    };
 
     // Start off by parsing any POST data with BusBoy
     return rbUtil.parsePOST(req)
 
+    .catch(function(e) {
+        throw new rbUtil.HTTPError({
+            status: 400,
+            body: {
+                type: 'invalid_request'
+            }
+        });
+    })
+
     // Then process the request
     .then(function(body) {
-        // Create a new, clean request object
-        var urlData = rbUtil.parseURL(req.url);
 
         if (/^application\/json/i.test(req.headers['content-type'])) {
             try {
@@ -202,13 +220,8 @@ function handleRequest (opts, req, resp) {
                 reqOpts.log('error/request/json-parsing', e);
             }
         }
-        newReq = {
-            uri: new URI(urlData.pathname),
-            query: urlData.query,
-            method: req.method.toLowerCase(),
-            headers: req.headers,
-            body: body
-        };
+
+        newReq.body = body;
 
         // Quick hack to set up general CORS
         if (newReq.method === 'options') {
diff --git a/test/features/errors/invalid_request.js b/test/features/errors/invalid_request.js
new file mode 100644
index 000000000..6b6c9ae46
--- /dev/null
+++ b/test/features/errors/invalid_request.js
@@ -0,0 +1,41 @@
+'use strict';
+
+// mocha defines to avoid JSHint breakage
+/* global describe, it, before, beforeEach, after, afterEach */
+
+var assert = require('../../utils/assert.js');
+var preq   = require('preq');
+var server = require('../../utils/server.js');
+
+describe('400 handling', function() {
+    this.timeout(20000);
+
+    before(function () { return server.start(); });
+
+    it('should return a proper 400 for an empty POST', function() {
+        return preq.post({
+            uri: server.config.hostPort,
+            headers: {
+                'content-type': 'foo/bar'
+            },
+        })
+        .catch(function(e) {
+            assert.deepEqual(e.status, 400);
+            assert.contentType(e, 'application/problem+json');
+        });
+    });
+
+    it('should return a proper 400 for an invalid POST', function() {
+        return preq.post({
+            uri: server.config.hostPort,
+            headers: {
+                'content-type': 'foo/bar'
+            },
+            body: 'baz'
+        })
+        .catch(function(e) {
+            assert.deepEqual(e.status, 400);
+            assert.contentType(e, 'application/problem+json');
+        });
+    });
+});