honojs
[zod-openapi][security] Unexpected validation bypass when omitting content-type
Hey there,
starting from "@hono/zod-openapi": "0.15.2" the body validation is skipped if the content-type is not matched - ref PR.
Personally I was very surprised by this as this means you can effectively bypass validations and invoke the handler as long as you just omit the content-type. I was also not able to find any mentioning of this in the docs. I think this can be a significant security risk as it causes unexpected behaviour in the handler which might expect a validated payload.
I would argue that the mention PR should be reverted and the handler should fail if the content-type is not matched, but at least this should be clearly documented and have been marked as a breaking change.
Reproduction
import { createRoute, z, OpenAPIHono } from '@hono/zod-openapi';
const route = createRoute({
method: 'POST',
path: '/book',
request: {
// This would mitigate the issue as we enforce the usage of `application/json` content-type
//headers: z.object({
// 'content-type': z.literal('application/json'),
//}),
body: {
content: {
'application/json': {
schema: z.object({
title: z.string().startsWith('[TITLE]').min(10).max(30),
}),
},
},
},
},
});
const app = new OpenAPIHono();
app.openapi(route, (c) => {
const validatedBody = c.req.valid('json');
console.log('This will only be logged when the request body passed validation, right?!', validatedBody);
return c.json({ processed: true });
});
console.log("Case I: sending a valid request with content-type `application/json`")
/**
* Output:
*
* This will only be logged when the request body passed validation, right?! { title: '[TITLE] Hello World' }
* { status: 200, response: { processed: true } }
*/
await app.request('/book', {
method: 'POST',
headers: new Headers({ 'Content-Type': 'application/json' }),
body: JSON.stringify({ title: '[TITLE] Hello World' }),
}).then(async response => console.log({ status: response.status, response: await response.json() }));
console.log("\n\nCase II: sending a malformed request with content-type `application/json`")
/**
* Output:
*
* {
* status: 400,
* response: { success: false, error: { issues: [Array], name: 'ZodError' } }
* }
*/
await app.request('/book', {
method: 'POST',
headers: new Headers({ 'Content-Type': 'application/json' }),
body: JSON.stringify({ title: 'not valid' }),
}).then(async response => console.log({ status: response.status, response: await response.json() }));
console.log("\n\nCase III: sending a malformed request without any content-type")
/**
* Output:
*
* Case III: sending a malformed request without any content-type
* This will only be logged when the request body passed validation, right?! {}
* { status: 200, response: { processed: true } }
*/
await app.request('/book', {
method: 'POST',
body: JSON.stringify({ title: 'not valid' }),
}).then(async response => console.log({ status: response.status, response: await response.json() }));
With "@hono/zod-openapi": "0.15.1" you got the following behaviour - which I would expect:
Case I: sending a valid request with content-type `application/json`
This will only be logged when the request body passed validation, right?! { title: '[TITLE] Hello World' }
{ status: 200, response: { processed: true } }
Case II: sending a malformed request with content-type `application/json`
{
status: 400,
response: { success: false, error: { issues: [Array], name: 'ZodError' } }
}
Case III: sending a malformed request without any content-type
{
status: 400,
response: { success: false, error: { issues: [Array], name: 'ZodError' } }
}
starting from "@hono/zod-openapi": "0.15.2" the handler is invoked if the content-type is omitted:
Case I: sending a valid request with content-type `application/json`
This will only be logged when the request body passed validation, right?! { title: '[TITLE] Hello World' }
{ status: 200, response: { processed: true } }
Case II: sending a malformed request with content-type `application/json`
{
status: 400,
response: { success: false, error: { issues: [Array], name: 'ZodError' } }
}
// ‼️ This is the problem ‼️
Case III: sending a malformed request without any content-type
This will only be logged when the request body passed validation, right?! {}
{ status: 200, response: { processed: true } }
as a mitigation the content-type can be enforced by validating the incoming content-type header.
Thanks!
I think in this case we'd want to return 400 if the request content-type is not in the router definition.
const me = createRoute({
request: {
body: { content: { 'application/json': { schema: z.null() } } },
},
});
Such that ^ request invoked with multipart/form-data for example would throw 400 even before validating.
if (c.req.header('content-type')) {
if (isJSONContentType(c.req.header('content-type')!)) {
return await validator(c, next)
}
}
c.req.addValidatedData('json', {})
await next()
Although as far i understand, in this code segment we ignore that malformed payload. Idk what security issues might come from this? It's not like we can send a payload & skip the validation for it... Any request sent like this would most likely result in a 500 error due to referenced values being undefined.
It's still an unpredicted behavior and something that I believe should be addressed. @yusukebe pls ping me if you have any thoughts on this and I can make a PR to resolve this if needed.
The reason I believe it's unpredicted is typing: in the endpoint when call c.req.valid('json') we already expect the JSON to be there no matter what. This can result in a bunch of foolish incidents if the request is sent with a wrong content-type. The client would be left thinking that the 500: Server Error is the server's fault.
Hi @tobiasdcl
I'm sorry for putting in that change without notice.
In this case, you can force validation by adding true to request.body.required. So, I think this problem can be solved without changing the code by writing this in the documentation.
const route = createRoute({
method: 'POST',
path: '/book',
request: {
body: {
required: true, // <=== add
content: {
'application/json': {
schema: z.object({
title: z.string().startsWith('[TITLE]').min(10).max(30)
})
}
}
}
}
})
Although as far i understand, in this code segment we ignore that malformed payload. Idk what security issues might come from this? It's not like we can send a payload & skip the validation for it... Any request sent like this would most likely result in a 500 error due to referenced values being
undefined.
I would challenge this statement. Depending on the business logic having undefined instead of a validated value might result in unexpected behaviour which could have security implications based on the implementation of the endpoint.
The reason I believe it's unpredicted is typing: in the endpoint when call c.req.valid('json') we already expect the JSON to be there no matter what. This can result in a bunch of foolish incidents if the request is sent with a wrong content-type.
I agree with that statement and that was exactly my concern. E.g. expecting a validated enum value and instead getting undefined which is then passed to a DB filter function or something along these lines.
In this case, you can force validation by adding
truetorequest.body.required. So, I think this problem can be solved without changing the code by writing this in the documentation.
Thanks that is great! I would even argue that this should be the default as this is the behaviour a user would most likely expect + it prevents unintended skipping of the validation logic.
Thanks for the super quick response ❤️
I would challenge this statement. Depending on the business logic having undefined instead of a validated value might result in unexpected behaviour which could have security implications based on the implementation of the endpoint.
Agreed. And I agree on the point that this should be the default