jsonBodyParser fails with invalid JSON when the first invalid character is %

[Temetz]

Bug Report

Sending invalid JSON with % as the first invalid character to bodyParser will crash restify.

For example the following payload will always fail:

{"answer": "firefoxurl:test|"%20-new-window%20javascript:alert(\'Cross%2520Browser%2520Scripting!\');""}

With other invalid characters the jsonBodyParser works as expeceted.

Restify Version

v 5.0.0

Node.js Version

v 6.10.3

Expected behaviour

Should return:

HTTP/1.1 400 Bad Request
{"code":"InvalidContent","message":"Invalid JSON: Unexpected token % in JSON at position 29"}

Actual behaviour

/home/temetz/work-project/backend/node_modules/restify-errors/node_modules/extsprintf/lib/extsprintf.js:72
                        throw (new Error('too few args to sprintf'));
                        ^

Error: too few args to sprintf
    at jsSprintf (/home/temetz/work-project/backend/node_modules/restify-errors/node_modules/extsprintf/lib/extsprintf

.js:72:11)
    at parseConstructorArguments (/home/temetz/work-project/backend/node_modules/restify-errors/node_modules/verror/li
b/verror.js:120:26)
    at WError (/home/temetz/work-project/backend/node_modules/restify-errors/node_modules/verror/lib/verror.js:414:11)
    at HttpError (/home/temetz/work-project/backend/node_modules/restify-errors/lib/baseClasses/HttpError.js:35:12)
    at RestError (/home/temetz/work-project/backend/node_modules/restify-errors/lib/baseClasses/RestError.js:27:15)
    at new acc.(anonymous function) (/home/temetz/work-project/backend/node_modules/restify-errors/lib/restErrors.js:5
9:19)
    at parseJson (/home/temetz/work-project/backend/node_modules/restify/lib/plugins/jsonBodyParser.js:40:25)
    at Server.parseBody (/home/temetz/work-project/backend/node_modules/restify/lib/plugins/bodyParser.js:96:13)
    at next (/home/temetz/work-project/backend/node_modules/restify/lib/server.js:1028:30)
    at f (/home/temetz/work-project/backend/node_modules/once/once.js:25:25)
    at IncomingMessage.done (/home/temetz/work-project/backend/node_modules/restify/lib/plugins/bodyReader.js:135:13)
    at IncomingMessage.g (events.js:292:16)
    at emitNone (events.js:86:13)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)

Repro Case

See: #1411 (comment)

Cause

lib/plugins/jsonBodyParser.js
https://github.com/restify/node-restify/blob/5.x/lib/plugins/jsonBodyParser.js#L37

        try {
            params = JSON.parse(req.body, opts.reviver);
        } catch (e) {
            return next(new errors.InvalidContentError('Invalid JSON:' +
              e.message));
        }

If the e.message contains the character % the crash will occur.

[retrohacker]

Woah, thanks for providing such a through bug report @Temetz! ❤️

I'm marking this as a bug and "unconfirmed" until a maintainer has an opportunity to reproduce this issue. At first glance, this appears to be caused by: #1384 but hasn't been ported to the 5.x branch.

[Temetz]

Curretly the only workaround is doing:

const server = restify.createServer({
  handleUncaughtExceptions: true,
});

server.on('uncaughtException', (req, res, route, err) => {
  res.send(err.code || 500, {
    code: err.code || 500,
    message: 'Internal Server Error',
  });
});

However!
This will cause deprecation warnings to appear. (At least the app wont crash now...)
DEPRECATION WARNING: Due to deprecation of the domain module in node.js, all features in restify that depend on it have been deprecated as well. This includes `handleUncaughtExceptions` and `next.ifError()`. They will continue to work in 5.x, but consider them unsupported and likely to be removed from future versions of restify.

[Temetz]

Here is a small code snippet to replicate.

index.js

const restify = require('restify');

function respond(req, res, next) {
  res.send(req.body);
  next();
}

const server = restify.createServer();

server.use(restify.plugins.bodyParser({}));

server.post('/', respond);

server.listen(8080, function() {
  console.log('%s listening at %s', server.name, server.url);
});

CURL

curl -v --insecure -XPOST -H "Content-type: application/json" -d '{"answer": "firefoxurl:test|"%this is broken :("}' 'http://localhost:8080/'

Error

/home/temetz/code/restify-bug/node_modules/extsprintf/lib/extsprintf.js:72
			throw (new Error('too few args to sprintf'));
			^

Error: too few args to sprintf
    at jsSprintf (/home/temetz/code/restify-bug/node_modules/extsprintf/lib/extsprintf.js:72:11)
    at parseConstructorArguments (/home/temetz/code/restify-bug/node_modules/verror/lib/verror.js:120:26)
    at WError (/home/temetz/code/restify-bug/node_modules/verror/lib/verror.js:414:11)
    at HttpError (/home/temetz/code/restify-bug/node_modules/restify/node_modules/restify-errors/lib/baseClasses/HttpError.js:35:12)
    at RestError (/home/temetz/code/restify-bug/node_modules/restify/node_modules/restify-errors/lib/baseClasses/RestError.js:27:15)
    at new acc.(anonymous function) (/home/temetz/code/restify-bug/node_modules/restify/node_modules/restify-errors/lib/restErrors.js:59:19)
    at parseJson (/home/temetz/code/restify-bug/node_modules/restify/lib/plugins/jsonBodyParser.js:40:25)
    at Server.parseBody (/home/temetz/code/restify-bug/node_modules/restify/lib/plugins/bodyParser.js:96:13)
    at next (/home/temetz/code/restify-bug/node_modules/restify/lib/server.js:1028:30)
    at f (/home/temetz/code/restify-bug/node_modules/once/once.js:25:25)

package.json

{
  "name": "restify-bug",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "restify": "^5.1.0"
  }
}

[wojtkowiak]

It does not seem to be fixed. I just got the same error on the newest version:

/opt/events-api/node_modules/extsprintf/lib/extsprintf.js:72
                        throw (new Error('too few args to sprintf'));

The payload mas a malformed JSON: { "level": 30% }

[retrohacker]

I'm not confident this has shipped yet.

[hekike]

It's already fixed.