feat: new built-in middleware bodyLimit #2077
Conversation
|
the little time, sry, i will provide an explanation without specific codes. I believe that it is also a (D)DoS countermeasure. |
|
I believe there are a couple of features missing on this. The first is the error handler call from the context object mentioned earlier. (But this can be seen as already done.) The second is when the maximum size is not specified. |
|
Initially, the size limit is not set. |
|
The following test file will give you a rough idea of how to use it. import { Hono } from '../../hono'
import { Uint, bodyParser } from '.'
describe('bodyParse works', () => {
const app = new Hono()
app.post(
'/max-64mb/body',
bodyParser({
type: 'body',
limit: 64 * Uint.mb,
}),
(c) => {
return c.json({
size: c.var.body<Blob>().size,
type: c.var.body<Blob>().type,
})
}
)
it('Should return same body on POST', async () => {
const data = {
hono: 'is',
}
const res = await app.request(
new Request('https://localhost/max-64mb/body', {
method: 'POST',
body: JSON.stringify(data),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.json()).toEqual({
size: 13,
type: 'text/plain;charset=utf-8',
})
})
app.post(
'/max-64mb/json',
bodyParser({
type: 'json',
limit: 64 * Uint.mb,
}),
(c) => {
return c.json(c.var.body())
}
)
it('Should return same json on POST', async () => {
const data = {
hono: 'is',
}
const res = await app.request(
new Request('https://localhost/max-64mb/json', {
method: 'POST',
body: JSON.stringify(data),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.json()).toEqual(data)
})
app.post(
'/max-64mb/json-error',
bodyParser({
type: 'json',
limit: 64 * Uint.mb,
}),
(c) => {
return c.json(c.var.body())
}
)
it('Should return json-error on POST', async () => {
const data = `{
hono: 'is',
}cool`
const res = await app.request(
new Request('https://localhost/max-64mb/json-error', {
method: 'POST',
body: data,
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(500)
expect(await res.text()).toBe('Internal Server Error')
})
app.post(
'/max-64mb/text',
bodyParser({
type: 'text',
limit: 64 * Uint.mb,
}),
(c) => {
return c.text(c.var.body<string>())
}
)
it('Should return same text on POST', async () => {
const data = 'hono is cool'
const res = await app.request(
new Request('https://localhost/max-64mb/text', {
method: 'POST',
body: data,
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
app.post(
'/max-64mb/form',
bodyParser({
type: 'form',
limit: 64 * Uint.mb,
}),
(c) => {
return c.text(c.var.body<FormData>().toString())
}
)
it('Should return same formData on POST', async () => {
const data = new FormData()
const res = await app.request(
new Request('https://localhost/max-64mb/form', {
method: 'POST',
body: data,
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('[object FormData]')
})
app.use(
'/max-32bit/*',
bodyParser({
type: 'body',
})
)
app.all('/max-32bit', (c) => c.text('hono is cool'))
it('Should return hono is cool on GET', async () => {
const res = await app.request(
new Request('https://localhost/max-32bit', {
method: 'POST',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
it('Should return hono is cool on POST', async () => {
const res = await app.request(
new Request('https://localhost/max-32bit', {
method: 'POST',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
it('Should return hono is cool on PUT', async () => {
const res = await app.request(
new Request('https://localhost/max-32bit', {
method: 'PUT',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
it('Should return hono is cool on PATCH', async () => {
const res = await app.request(
new Request('https://localhost/max-32bit', {
method: 'PATCH',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
app.use(
'/max-8byte',
bodyParser({
type: 'text',
limit: 8 * Uint.b,
})
)
app.post('/max-8byte', (c) => c.text('hono is cool'))
it('Should return 413 Request Entity Too Large on POST', async () => {
const res = await app.request(
new Request('https://localhost/max-8byte', {
method: 'POST',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(413)
expect(await res.text()).toBe('413 Request Entity Too Large')
})
it('Should return hono is cool on POST', async () => {
const res = await app.request(
new Request('https://localhost/max-8byte', {
method: 'POST',
body: 'hono',
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
app.use(
'/max-8byte-custom',
bodyParser({
type: 'text',
limit: 8 * Uint.b,
handler: (c) => {
return c.text('not cool', 413)
},
})
)
app.post('/max-8byte-custom', (c) => c.text('hono is cool'))
it('Should return not cool with 413 on POST', async () => {
const res = await app.request(
new Request('https://localhost/max-8byte-custom', {
method: 'POST',
body: JSON.stringify({
hono: 'is',
}),
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(413)
expect(await res.text()).toBe('not cool')
})
it('Should return hono is cool on POST', async () => {
const res = await app.request(
new Request('https://localhost/max-8byte-custom', {
method: 'POST',
body: 'hono',
})
)
expect(res).not.toBeNull()
expect(res.status).toBe(200)
expect(await res.text()).toBe('hono is cool')
})
}) |
|
Hi @EdamAme-x I think the purpose of this feature is to limit the size. In that case, I think the name |
|
To be honest, I was thinking that too. |
|
I thought about it for a while, and since the name of koa's middleware with equivalent functionality is also |
|
@EdamAme-x @yusukebe In Koa, the middleware with this name is literally used to parse the body. app.post(
'/',
bodyLimit({
type: 'body',
limit: 64 * Uint.mb,
}),
(c) => {}
)… or |
|
@EdamAme-x |
|
I like |
|
Thanks. @nabeken5 @yusukebe |
Can you give me a sample code for this? |
|
I am thinking of narrowing this down to the ability to limit the size of the BODY. I think Hono's function is sufficient for parsing. |
|
I think there are times when we want to put a limit on a specific TYPE like this: app.post(
'/',
bodyLimit([
{
type: 'text',
limit: 0,
},
{
type: 'json',
limit: 64 * Uint.mb,
},
{
type: 'form',
limit: 0,
}
]),
(c) => {}
) |
me too
I feel like this process is not what bodyLimit should be doing, can it be omitted? |
This idea is great! |
EdamAme-x
changed the title
|
Sorry if you all know this, but it's just a comment for your information. Please point out any errors in my understanding. Need to limit size of bodyIt is very important to limit the size of body on the server side. For example, as @EdamAme-x wrote, it is necessary for countermeasures against DOS attacks. What can we do with the Web Standard's API?In hono, I don't disagree that checking the size here before const app = new Hono()
app.use('*', async (c, next) => {
console.log((await c.req.raw.blob()).size) // request body is already loaded into memory here.
await new Promise((resolve) => setTimeout(resolve, Infinity))
await next()
})
app.get('/', (c) => c.text('Hello World'))Effective methods for countermeasures against DOS attacksIf you need to severely limit the size of the body in a production environment, you should consider the following method instead of hono.
|
I'm thinking the same thing. It may be out of Hono's scope, since the Web standards API does not have the feature to know the body size before loading contents on the memory. It is like a fact we can't get an IP address from a If this feature is useful, it is to know the size of the JSON before parsing it, but it does not have that much impact. Now, I'm asking if Cloudflare Workers can limit the size of a request body or not, but I think it can't do it. |
Thanks, I understand! |
|
Cloudflare Workers and Deno Deploy work with v8 isolate and often the process dies off by itself. Of course, we want to prevent DoS attacks, but this is no more important than a server that has to be up all the time. Also, a Fastly Compute application starts a process every time a request is made. And with the Edge platform, the platform itself has an approach to defend against DoS attacks, so I don't think applications like Cloudflare Workers need to worry about it not so much. We need to discuss which platforms Hono should target, but the mindset is different from the case that the server is always running like Node.js and Bun(but I think such like a Bun Deploy might be Edge-like) and the edge platform. |
There was a problem hiding this comment.
Hi @EdamAme-x
content-type
If the Content-Type is not what is expected, the size will not be checked. I don't think this is the behavior you expect.
app.post(
'/',
bodyLimit({
type: 'json',
limit: 1,
}),
async (c) => {
return c.text(JSON.stringify(await c.req.json()))
}
)$ curl -X POST -H "Content-Type: application/json" -d '{"message":"OK"}' http://127.0.0.1:8787
413 Request Entity Too Large
$ curl -X POST -H "Content-Type: text/plain" -d '{"message":"OK"}' http://127.0.0.1:8787
{"message":"OK"}
in bun
This is FYI, but in bun, c.req.raw.clone() does not seem to get the original body, I think it is a bug in bun, but we need to be aware of this limitation if we merge this middleware.
c.req.raw.clone().blob()
This may depend on the execution environment, but when checked with workerd, it appears that the memory usage is larger, albeit temporarily, when c.req.raw.clone().blob() is executed compared to a simple c.req.parseBody(). We need to know that these overheads will occur.
app.post('/c.req.parseBody', async (c) => {
const data = await c.req.parseBody()
console.log(Object.keys(data))
console.log((data['file1'] as File).size)
console.log((data['file2'] as File).size)
await new Promise((resolve) => setTimeout(resolve, 5000))
return c.text('Hello')
})
app.post('/c.req.raw.blob', async (c) => {
const data = await c.req.raw.blob()
console.log(data.size)
await new Promise((resolve) => setTimeout(resolve, 5000))
return c.text('Hello')
})
app.post('/c.req.raw.clone.blob', async (c) => {
const data = await c.req.raw.clone().blob()
console.log(data.size)
await new Promise((resolve) => setTimeout(resolve, 5000))
return c.text('Hello')
})$ curl -X POST -F file1=@100MB -F file2=@100MB http://127.0.0.1:8787/c.req.parseBody & sleep 3; ps -xm -o rss -p $(pgrep workerd)
RSS
490800
# reload app
$ curl -X POST -F file1=@100MB -F file2=@100MB http://127.0.0.1:8787/c.req.raw.blob & sleep 3; ps -xm -o rss -p $(pgrep workerd)
RSS
302080
# reload app
$ curl -X POST -F file1=@100MB -F file2=@100MB http://127.0.0.1:8787/c.req.raw.clone.blob & sleep 3; ps -xm -o rss -p $(pgrep workerd)
RSS
648032
API
I don't think I understand what limit is if it is just bodyLimit and limit. If it is middleware to limit size, I think it is more appropriate to have the keyword size somewhere.
bodyLimit({
type: 'json',
maxSize: 1,
})bodyLimit({
type: 'json',
size: 1,
})
|
To be honest, in my opinion, although I admit that this middleware is effective in some situations, the results obtained may not be worth the cost of implementation and maintenance, since the scope of the web standard API does not allow checking before loading into memory, and a naive implementation will inevitably increase memory when checking. |
|
yes.
I will change to I will continue my research on this. |
|
@EdamAme-x @nabeken5 @usualoma Hey, forks! I've implemented it with another approach and created PR #2103. Check it! |
|
@yusukebe Here are the issues I have identified at #2103
|
|
For example, if it is the case that we only need to limit it to a value as small as 10 MB, I think the current implementation of #2103 will give us the results we are hoping for. The use of memory consumption became larger at the stage of accepting requests in bun, and even when using #2103, memory consumption tended to be higher than in other runtimes. Whether #2103 improves the situation or not depends on the runtime. |
|
Sorry. |
|
I refactored this implementation as a pullrequest. |
|
Replacing With #2109, any runtime will not consume significantly more memory than if it were not used |
|
Hi @usualoma #2109 is awesome! I've merged honojs/node-server#133 and released a new version of |
|
Postscript. I did a little more research and found that bun, deno, wrangler dev, node-server, or any environment, did not read data longer than the const app = new Hono()
app.post('/', async (c) => c.text('hello: ' + await c.req.raw.text()))For this script, for bun, deno, and wrangler dev, the following is true. On node-server I get "400 Bad Request". Given this behavior, we would prefer to rely on Content-Length when it is present. |
I think this is good. |
|
I think the items this middleware needs are as follows
Also, we rarely want to limit the size per content type, and we think it is fine to limit the size to the entire target request. We think it is important to be able to use the system easily without having to specify difficult details. I think the middleware shown in the following commit hash satisfies these conditions. @EdamAme-x is the one who pulled this middleware discussion to this point, so if we can continue with this PR, that is better, but if not, I think it is better to use #2103 and put @EdamAme-x as co-author. |
|
thanks. |
|
Thanks for the helpful comment! I, too, would like @EdamAme-x to complete this PR. However, it will be difficult to get it in time for the v4 release on Feb. 9, so let's do it after that. No need to rush. |
|
Hi, folks! I'm planning to merge #2103 into the "next" branch for the new release And thinking of adding #2109.
One thing that concerns me about this. But I don't think So, can you please finish #2109? |
Author should do the followings, if applicable
I saw this DISCUSSION and implemented it.
https://github.com/orgs/honojs/discussions/2048
It takes a little time to prepare an explanation.
yarn denoifyto generate files for Deno