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
Add option to limit the size of request bodies for the default body parser #627
Conversation
test/fastify-options.test.js
Outdated
| const t = require('tap') | ||
| const test = t.test | ||
|
|
||
| test('jsonBodyLimit option', t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you just name this file ‘jsonBodyLimit’?
lib/handleRequest.js
Outdated
| } | ||
|
|
||
| const req = request.req | ||
| const chunks = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m confident that accumulating as a string is faster that accumulating in an array. Can you restore that?
You can call setEncoding(‘utf8’) to avoid relying on defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you can keep receiving buffers, but accumulate as String (it might be simpler to do the rest of the checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @mcollina.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some benchmarking, I found that you're right @mcollina, accumulating as a string is faster.
I also found that performance can be doubled by doing
body += chunk.toString() rather than body += chunk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use flatstr after that step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. flatstr is only useful when sending data out.
lib/handleRequest.js
Outdated
| function removeHandlers () { | ||
| req.removeListener('data', onData) | ||
| req.removeListener('end', onEnd) | ||
| req.removeListener('error', onEnd) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you avoid allocating this closure at all? (copy-paste is ok).
lib/handleRequest.js
Outdated
| function onEnd () { | ||
|
|
||
| function onEnd (err) { | ||
| removeHandlers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might not be needed to remove the handlers there. Removing those is costly, so we can eve skip it altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If error is emitted, is it impossible for other events to be emitted after? Even if a user does req.emit('error')?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave them there, we had no problems so far.
It won’t protect from direct emit, but it’s protected from the core point of view!
lib/handleRequest.js
Outdated
| var req = request.req | ||
| req.on('error', onError) | ||
| function jsonBody (request, reply, limit) { | ||
| const contentLength = Number.parseInt(request.headers['content-length'], 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not assume Content-Length is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short circuit is good. Just wrap it with a check for the header first.
fastify.js
Outdated
| var jsonBodyLimit = _fastify._jsonBodyLimit | ||
| if (opts.jsonBodyLimit !== undefined) { | ||
| if (!Number.isInteger(opts.jsonBodyLimit)) { | ||
| throw new TypeError(`'jsonBodyLimit' option must be an integer. Got: '${opts.jsonBodyLimit}'`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a typeof after "Got".
Got: '${typeof opts.jsonBodyLimit}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this suggestion from @StarpTech, users will need to see the actual value to understand why it wasn't valid.
fastify.js
Outdated
| } | ||
| fastify._jsonBodyLimit = options.jsonBodyLimit | ||
| } else { | ||
| fastify._jsonBodyLimit = 1000 * 1000 // 1 MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use an external constant for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s 1024 * 1024.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we want MiB then? I would actually agree with this since base-2 values are more common in networking applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use base2 values.
fastify.js
Outdated
| throw new TypeError(`'jsonBodyLimit' option must be an integer. Got: '${opts.jsonBodyLimit}'`) | ||
| } | ||
| jsonBodyLimit = opts.jsonBodyLimit | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is made twice. Can you refactor this point ?
lib/handleRequest.js
Outdated
| receivedLength += chunk.length | ||
|
|
||
| if (receivedLength > limit) { | ||
| removeHandlers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onData handler is binded to the event emitter. So this here is the req. Use it in order to avoid the closure in removeHandlers function.
lib/handleRequest.js
Outdated
| @@ -23,7 +23,7 @@ function handleRequest (req, res, params, context) { | |||
| if (method === 'POST' || method === 'PUT' || method === 'PATCH') { | |||
| // application/json content type | |||
| if (contentType && contentType.indexOf('application/json') > -1) { | |||
| return jsonBody(request, reply) | |||
| return jsonBody(request, reply, context._jsonBodyLimit) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An object can be better here: it's future proof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allevo Yes I agree it would be future proof. It would be nice to not allocate an object though. It would also be nice to wait until Node 4 support is dropped so we can use object destructuring in jsonBody.
I can add it if you insist though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an internal function we can leave it as is.
It will not be nice, but is fast :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truly the parser options can be stored inside the route definition aka Context. Without create any object at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me
How does this look?
diff --git a/fastify.js b/fastify.js
index 6d35bdc..d6155ee 100644
--- a/fastify.js
+++ b/fastify.js
@@ -494,7 +494,9 @@ function build (options) {
this.config = config
this.errorHandler = errorHandler
this._middie = middie
- this._jsonBodyLimit = jsonBodyLimit
+ this._jsonParserOptions = {
+ limit: jsonBodyLimit
+ }
this._fastify = fastify
}
diff --git a/lib/handleRequest.js b/lib/handleRequest.js
index d07bd29..79a8bb2 100644
--- a/lib/handleRequest.js
+++ b/lib/handleRequest.js
@@ -23,7 +23,7 @@ function handleRequest (req, res, params, context) {
if (method === 'POST' || method === 'PUT' || method === 'PATCH') {
// application/json content type
if (contentType && contentType.indexOf('application/json') > -1) {
- return jsonBody(request, reply, context._jsonBodyLimit)
+ return jsonBody(request, reply, context._jsonParserOptions)
}
// custom parser for a given content type
@@ -42,7 +42,7 @@ function handleRequest (req, res, params, context) {
// application/json content type
if (contentType.indexOf('application/json') > -1) {
- return jsonBody(request, reply, context._jsonBodyLimit)
+ return jsonBody(request, reply, context._jsonParserOptions)
}
// custom parser for a given content type
if (context.contentTypeParser.fastHasHeader(contentType)) {
@@ -57,7 +57,8 @@ function handleRequest (req, res, params, context) {
return
}
-function jsonBody (request, reply, limit) {
+function jsonBody (request, reply, options) {
+ const limit = options.limit
const contentLength = request.headers['content-length'] === undefinedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s ok.
| } | ||
|
|
||
| if (!Number.isNaN(contentLength) && receivedLength !== contentLength) { | ||
| reply.code(400).send(new Error('Request body size did not match Content-Length')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 400 the correct HTTP status code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, according to this https://github.com/stream-utils/raw-body/blob/master/index.js#L263 it's correct because we evaluate just the request (headers) not the content (payload).
fastify.js
Outdated
| @@ -394,6 +405,14 @@ function build (options) { | |||
| throw new Error(`Missing handler function for ${opts.method}:${opts.url} route.`) | |||
| } | |||
|
|
|||
| var jsonBodyLimit = _fastify._jsonBodyLimit | |||
| if (opts.jsonBodyLimit !== undefined) { | |||
| if (!Number.isInteger(opts.jsonBodyLimit)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only allow unsigned integers.
if (!Number.isInteger(opts.jsonBodyLimit)) {
if(opts.jsonBodyLimit <= 0) {
throw new Error(`jsonBodyLimit should be positive integer ....`)
}
}|
Updated |
| }) | ||
|
|
||
| // Node errors for OPTIONS requests with a stream body and no Content-Length header | ||
| if (upMethod !== 'OPTIONS') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there's a bug in Node. On Node 4 this test fails with an error and on the other versions it tries to attach too many error event handlers to the req, which produces a Node warning and errors with a 400 status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many error handlers are attached to req?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I'm getting from Node:
(node:10380) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added. Use emitter.setMaxListeners() to increase limit
at _addListener (events.js:280:19)
at Socket.addListener (events.js:297:10)
at Socket.Readable.on (_stream_readable.js:772:35)
at Socket.socketOnWrap [as on] (_http_server.js:651:37)
at Socket.socketOnError (_http_server.js:456:8)
at onParserExecuteCommon (_http_server.js:467:19)
at onParserExecute (_http_server.js:450:3)
I guess I meant the handlers are being attached to the socket, not req specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Updated to use a |
| return | ||
| } | ||
|
|
||
| body += chunk.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk + '' performs better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my benchmarks that performs exactly the same as body += chunk
(I tried on both Node 8.9.4 and 9.3.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Adds a
jsonBodyLimitoption that can be configured with the initialfastifycall and at routes.Default limit is
1 MiB.Checklist
npm run testandnpm run benchmark