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

fix: route body limit #36

Merged
merged 2 commits into from
Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,22 @@ function rawBody (fastify, opts, next) {
next()

function preparsingRawBody (request, reply, payload, done) {
const applyLimit = request.context._parserOptions.limit || fastify.initialConfig.bodyLimit

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you check for null or undefined instead? is 0 technically a valid limit?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fastify rejects it

'bodyLimit' option must be an integer > 0. Got '0'

I fixed the check anyway by using the ?? operator instead


getRawBody(runFirst ? request.raw : payload, {
length: null, // avoid content lenght check: fastify will do it
limit: fastify.initialConfig.bodyLimit, // limit to avoid memory leak or DoS
limit: applyLimit, // limit to avoid memory leak or DoS
encoding
}, function (err, string) {
if (err) {
/**
* the error is managed by fastify server
* so the request object will not have any
* `body` parsed
* `body` parsed.
*
* The preparsingRawBody decorates the request
* meanwhile the `payload` is processed by
* the fastify server.
*/
return
}
Expand All @@ -84,15 +90,15 @@ function rawBody (fastify, opts, next) {
}

try {
var json = secureJson.parse(body.toString('utf8'), {
const json = secureJson.parse(body.toString('utf8'), {
protoAction: fastify.initialConfig.onProtoPoisoning,
constructorAction: fastify.initialConfig.onConstructorPoisoning
})
done(null, json)
} catch (err) {
err.statusCode = 400
return done(err)
}
done(null, json)
}
}

Expand Down
6 changes: 6 additions & 0 deletions test/plugin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,12 @@ t.test('body limit', async t => {
payload
})
t.equal(res.statusCode, 413)
t.same(res.json(), {
statusCode: 413,
code: 'FST_ERR_CTP_BODY_TOO_LARGE',
error: 'Payload Too Large',
message: 'Request body is too large'
})
})

t.test('empty body', async t => {
Expand Down
85 changes: 85 additions & 0 deletions test/route-limit.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
'use strict'

const t = require('tap')
const Fastify = require('fastify')
const rawBody = require('../plugin')

t.test('body limit per route', async t => {
const app = Fastify({ bodyLimit: 5 })

const payload = { hello: '123456789012345678901234567890' }

await app.register(rawBody, {
field: 'rawBody',
global: false,
runFirst: false
})

app.post('/100', {
config: { rawBody: true },
bodyLimit: 100
}, (req, reply) => {
t.pass('body is ok')
return req.rawBody
})

app.post('/50', {
config: {
rawBody: true
},
bodyLimit: 41
}, (req, reply) => {
t.fail('body is not ok')
})

app.post('/server-limit', {
config: {
rawBody: true
}
}, (req, reply) => {
t.fail('body is not ok')
})

await t.test('must not throw if body is smaller than limit', async t => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd specify:

must not throw if body is smaller than route limit

const res = await app.inject({
method: 'POST',
url: '/100',
payload
})

t.equal(res.statusCode, 200)
t.equal(res.payload, JSON.stringify(payload))
})

await t.test('must reject if body is bigger than limit', async t => {
const res = await app.inject({
method: 'POST',
url: '/50',
payload
})

t.equal(res.statusCode, 413)
t.same(res.json(), {
statusCode: 413,
code: 'FST_ERR_CTP_BODY_TOO_LARGE',
error: 'Payload Too Large',
message: 'Request body is too large'
})
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using different routes instead of different payloads?

I'd rather have just two routes:

  1. route-limit
  2. server-limit

And you test both with a bigger and a smaller payload

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually split route-limit into two tests: one were route-limit is bigger than server-limit and one where route-limit is smaller then server-limit.

This way you can test that the route level limit is always respected

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


await t.test('must reject if body is bigger then the server limit', async t => {
const res = await app.inject({
method: 'POST',
url: '/server-limit',
payload: { hello: '1' }
})

t.equal(res.statusCode, 413)
t.same(res.json(), {
statusCode: 413,
code: 'FST_ERR_CTP_BODY_TOO_LARGE',
error: 'Payload Too Large',
message: 'Request body is too large'
})
})
})