Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Temetz opened this issue Jul 13, 2017 · 6 comments
Closed
Labels

Comments

@Temetz
Copy link

Temetz commented Jul 13, 2017

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
Copy link
Member

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
Copy link
Author

Temetz commented Aug 11, 2017

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
Copy link
Author

Temetz commented Aug 14, 2017

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
Copy link
Contributor

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
Copy link
Member

I'm not confident this has shipped yet.

@hekike
Copy link
Member

hekike commented Apr 10, 2018

It's already fixed.

@hekike hekike closed this as completed Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants