[zod-openapi][security] Unexpected validation bypass when omitting content-type

[tobiasdcl]

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!

[askorupskyy]

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.

[askorupskyy]

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.

[yusukebe]

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)
          })
        }
      }
    }
  }
})

[tobiasdcl]

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 true to request.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 ❤️

[askorupskyy]

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