Add support for per-parser body limits #800

jsumners · 2018-02-20T14:04:05Z

This PR allows custom parser plugins to specify a maximum body limit that supersedes the global limit. This allows for more granular control of payload sizes while retaining a catch-all limit.

Checklist

run npm run test ~~and npm run benchmark~~
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

mcollina

LGTM

StarpTech

LGTM

delvedor · 2018-02-20T15:53:22Z

lib/ContentTypeParser.js

@@ -28,6 +28,7 @@ ContentTypeParser.prototype.add = function (contentType, opts, parserFn) {
  const parser = new Parser(
    opts.parseAs === 'string',
    opts.parseAs === 'buffer',
+    opts.bodyLimit || null,


Can we configure here the body limit option and not at line 98?
I think you can access the global limit also here, or even better in the addContentTypeParser api inside fastify.js. In this way we can skip that check at runtime, that will likely be falsy very often.

Doing so makes it really complicated to also add the default application/json parser and not break tests. Basically, it'll require refactoring how the default parser is added and detected.

Maybe not. I'm looking into it.

Is there a reason we are expecting OPTIONS to always reject bodies?

fastify/test/helper.js

Lines 287 to 296 in 034d20f

sget({

method: upMethod,

url: `http://localhost:${fastify.server.address().port}/with-limit`,

headers: { 'Content-Type': 'application/json' },

body: {},

json: true

}, (err, response, body) => {

t.error(err)

t.strictEqual(response.statusCode, 413)

})

The spec allows them:

If the OPTIONS request includes an entity-body (as indicated by the
presence of Content-Length or Transfer-Encoding), then the media type
MUST be indicated by a Content-Type field.

https://tools.ietf.org/html/rfc2616#section-9.2

??
That's a 413 - Payload Too Large test so that test is supposed to reject the body of any request that has a body larger that 1 byte.

Where is that limit being set? I don't see it. I'm passing the configured body limit into ContentTypeParser so that it can pass it along to the default JSON parser. With this modification this test, among others, is failing. This seems to rely on the body limit in the default JSON parser being null.

It's set way up in the test file:

fastify/test/helper.js

Line 67 in 034d20f

fastify[loMethod]('/with-limit', { bodyLimit: 1 }, function (req, reply) {

That's a completely different test. They should not be sharing state.

@jsumners tell me if I can help :)

jsumners · 2018-02-24T12:50:39Z

I have pushed up a fix to @delvedor's suggestion, but the tests are failing. I have pushed the failing implementation to get some feedback on how to resolve the failures. The reason for the failure is https://github.com/fastify/fastify/pull/800/files#diff-81727096069e1b87a7a2b559519d722fR8 where null is changed to bodyLimit.

It seems to me that the failing tests are relying on prior state or for the default JSON parser to be set to null as the default limit. Or maybe both. My inclination is to refactor the tests so that they are individually independent, but that would be contrary to their purpose -- making sure new changes retain prior functionality.

mcollina · 2018-02-25T10:00:43Z

lib/ContentTypeParser.js

@@ -94,7 +95,7 @@ ContentTypeParser.prototype.run = function (contentType, handler, request, reply

 function rawBody (request, reply, options, parser, done) {
  var asString = parser.asString
-  var limit = options.limit
+  var limit = parser.bodyLimit


I'm a bit lost. Should the name of the options be options.bodyLimit?

I think the actual implementation here should be something like options.bodyLimit >= 0 ? options.bodyLimit : parser.bodyLimit?

No. options.bodyLimit comes from the root Fastify object:

fastify/lib/ContentTypeParser.js

Line 74 in 7cb07ce

reply.context._parserOptions,

Thus, options.bodyLimit will always be >= 0 since the Fastify constructor doesn't allow anything else. So the option needs to be set in the same way as the option to determine if the parser should parse the incoming body as a string or a buffer -- by attaching it to the parser object during its construction.

Looks like there's going to be trouble deciding which body limit to use. IMO the per-route limit should have the highest priority (with the per-parser limit having priority over the Fastify constructor's limit) but there's no way to tell the difference between a route limit and the constructor's limit (unless a flag is added to Context).

fastify/fastify.js

Line 474 in 9cec944

opts.bodyLimit = opts.bodyLimit || _fastify._bodyLimit

Feel free to push whatever fixes you like to this branch.

I'd really like this to make it into v1.0.0. So if anyone has any ideas, please do submit them; preferably via code.

I'll review this asap.

mcollina · 2018-02-26T17:21:53Z

Fix pushed.

jsumners · 2018-02-26T17:26:33Z

Looks good to me if others agree.

nwoltman · 2018-02-26T20:47:39Z

I just realized that the global limit isn't necessary since it's the default value for the parser. I can update the branch.

jsumners · 2018-02-26T21:34:57Z

Excellent.

mcollina

LGTM

nwoltman · 2018-02-26T23:53:48Z

docs/ContentTypeParser.md

@@ -48,6 +48,10 @@ As you can see, now the function signature is `(req, body, done)` instead of `(r

 See [`example/parser.js`](https://github.com/fastify/fastify/blob/master/examples/parser.js) for an example.

+##### Custom Parser Options
+ `parseAs` (string): Either `'string'` or `'buffer'` to designate how the incoming data should be collected. Default: `'buffer'`.
+ `bodyLimit` (number): May be supplied to override the global maximum body limit on a per parser basis. Default: `null`.


@jsumners What do you think of this?

+ `bodyLimit` (number): The maximum payload size, in bytes, that the custom parser will accept. Defaults to the global body limit passed to the [`Fastify factory function`](https://github.com/fastify/fastify/blob/master/docs/Factory.md#bodylimit).

I like it. Push the change.

jsumners · 2018-02-27T03:07:16Z

@delvedor have your concerns been adequately addressed?

delvedor

LGTM

github-actions · 2022-02-18T00:17:38Z

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.

Add support for per-parser body limits

961f493

mcollina approved these changes Feb 20, 2018

View reviewed changes

StarpTech approved these changes Feb 20, 2018

View reviewed changes

delvedor reviewed Feb 20, 2018

View reviewed changes

delvedor added the feature label Feb 20, 2018

Set parser body limit via addContentTypeParser

7cb07ce

mcollina reviewed Feb 25, 2018

View reviewed changes

Select which bodyLimit to apply when parsing a request

4e4cc7d

Use either the route's body limit or the parser's body limit

de1fc8b

mcollina approved these changes Feb 26, 2018

View reviewed changes

nwoltman reviewed Feb 26, 2018

View reviewed changes

Update custom ContentTypeParser bodyLimit documentation

069f54b

delvedor approved these changes Feb 27, 2018

View reviewed changes

jsumners merged commit dab20bd into master Feb 27, 2018

jsumners deleted the parser-plugin-limit branch February 27, 2018 12:51

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for per-parser body limits #800

Add support for per-parser body limits #800

jsumners commented Feb 20, 2018

mcollina left a comment

StarpTech left a comment

delvedor Feb 20, 2018

jsumners Feb 21, 2018

jsumners Feb 21, 2018

jsumners Feb 21, 2018

nwoltman Feb 21, 2018 •

edited

jsumners Feb 21, 2018

nwoltman Feb 21, 2018

jsumners Feb 21, 2018

nwoltman Feb 21, 2018

delvedor Feb 21, 2018

jsumners commented Feb 24, 2018

mcollina Feb 25, 2018

jsumners Feb 25, 2018

nwoltman Feb 25, 2018

jsumners Feb 25, 2018

jsumners Feb 26, 2018

delvedor Feb 26, 2018

mcollina commented Feb 26, 2018

jsumners commented Feb 26, 2018

nwoltman commented Feb 26, 2018

jsumners commented Feb 26, 2018

mcollina left a comment

nwoltman Feb 26, 2018

jsumners Feb 27, 2018

jsumners commented Feb 27, 2018

delvedor left a comment

github-actions bot commented Feb 18, 2022

	sget({
	method: upMethod,
	url: `http://localhost:${fastify.server.address().port}/with-limit`,
	headers: { 'Content-Type': 'application/json' },
	body: {},
	json: true
	}, (err, response, body) => {
	t.error(err)
	t.strictEqual(response.statusCode, 413)
	})

Add support for per-parser body limits #800

Add support for per-parser body limits #800

Conversation

jsumners commented Feb 20, 2018

Checklist

mcollina left a comment

Choose a reason for hiding this comment

StarpTech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwoltman Feb 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 26, 2018

jsumners commented Feb 26, 2018

nwoltman commented Feb 26, 2018

jsumners commented Feb 26, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners commented Feb 27, 2018

delvedor left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2022

nwoltman Feb 21, 2018 •

edited