Fix #14: Middleware hangs if rawBody is already parsed

[dblock]

I don't believe it does what you say it does until there's a test! And the existing test is relying on some externals. Show me a test that uses body-parser and that hangs without this change?

[tejashah88]

Show me a test that uses body-parser and that hangs without this change?

Seems completely pointless since #14 is labeled as a "confirmed bug."

[dblock]

If I look at this diff I am not convinced that it fixes the issue. I could try it against that scenario in #14 that reproduces the issue, but there're no guarantees that you or someone else doesn't reintroduce this bug in a future version. Makes sense?

[dblock]

And to prove my point, here's a test that still hangs with this change, so it doesn't fix the bug.

https://github.com/dblock/alexa-verifier-middleware/commit/592cdedd52b3b90862b419e850ddc931ae775824

test('with express.js and body-parser incorrectly mounted', function(t) {
  var express = require('express');
  var app = express();
  app.use(require('body-parser').json);
  app.use(verifier);
  var server = app.listen(3000);
  app.get('/', function(req, res) {
    res.status(200).send('ok');
  });
  var request = require('supertest');
  request(server)
    .get('/')
    .end(function (err, res) {
      t.equal(res.statusCode, 401);
      t.deepEqual(res.body, {
        "reason": "signature is not base64 encoded",
        "status": "failure"
      });
      server.close();
      t.end();
    });
});

[tejashah88]

This line: app.use(require('body-parser').json); is not correct because .json is a function that takes in a set of options (original documentation).

[dblock]

Ah yes, you're right.

Now I cannot get it to hang in a test. But that bug was real, no?

[tejashah88]

Technically yes, although the .json function itself is not a middleware function.

[tejashah88]

Moving on, can this PR be merged, since #21 should account for the extra test?

[dblock]

Except that the test in #21 doesn't hang, so I am unclear how this change here fixes anything?

[tejashah88]

I think the return statement here helped fix the bug. Basically, you said that adding a return statement should abort the request immediately. If you remove it, you'd need to call next(), to tell express that the middleware is done, otherwise it hangs.

If the current middleware function does not end the request-response cycle, it must call next() to pass control to the next middleware function. Otherwise, the request will be left hanging.

Source: http://expressjs.com/en/guide/using-middleware.html

[dblock]

Nope, you're guessing again :) I removed it and the test in #19 doesn't hang.

Anyway, I think this change is still right, having looked at the body-parser code it only checks _body and the rest is very specific to this library. I'll merge, lets move on.