-
Notifications
You must be signed in to change notification settings - Fork 258
fix(zod-openapi): prevent validation bypass when content-type is omitted #1376
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
base: main
Are you sure you want to change the base?
fix(zod-openapi): prevent validation bypass when content-type is omitted #1376
Conversation
This fixes a critical security issue where body validation could be bypassed by omitting the content-type header. The vulnerability was introduced in v0.15.2. Changes: - Made validation strict by default (when `required` is not specified) - Only allow empty body when `required` is explicitly set to `false` AND no content-type is provided - Return 400 error for mismatched content-types in single content-type scenarios - Skip validators gracefully when multiple content-types are supported - Added comprehensive security tests to prevent regression BREAKING CHANGE: Requests without content-type are now validated by default instead of bypassing validation. To allow optional body, explicitly set `required: false` in the route configuration. Fixes the security vulnerability reported where attackers could bypass validation by omitting the content-type header, potentially leading to unexpected behavior in handlers that expect validated payloads.
🦋 Changeset detectedLatest commit: 2a57b5a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
==========================================
- Coverage 79.32% 4.35% -74.98%
==========================================
Files 81 103 +22
Lines 2443 3402 +959
Branches 633 877 +244
==========================================
- Hits 1938 148 -1790
- Misses 419 2648 +2229
- Partials 86 606 +520
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the PR. I started considering it. Please ignore the CI error. |
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do the two things:
- Add a changeset with
yarn changesetcommand on the top of the project. This is apatchchange. - Update the README for this change.
|
Thank you for the PR! I considered it, but you're absolutely right. This should be fixed right now. I've left comments; please check them. |
- Add multiple content-type support scenarios tests - Fix validation logic for multiple content types - Add final check to ensure validation runs for unsupported content-types - Update README with detailed body validation behavior documentation - Add changeset for the security patch This ensures requests with unsupported content-types or missing content-types are properly validated when multiple content types are supported.
|
Hi @yusukebe Thanks for the review!
|
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to consider what should happen with non-JSON/Form content types. Currently, the behavior is inconsistent.
For example, consider this test case:
describe('Non-JSON/Form content-types', () => {
const route = createRoute({
method: 'post',
path: '/text',
request: {
body: {
content: {
'text/plain': {
schema: z.string().max(1),
},
},
},
},
responses: {
200: {
description: 'Success',
content: {
'application/json': {
schema: z.object({ success: z.boolean() }),
},
},
},
},
})
const app = new OpenAPIHono()
app.openapi(route, async (c) => {
return c.json({ success: true })
})
it('should validate text/plain content and reject invalid data', async () => {
const res = await app.request('/text', {
method: 'POST',
body: 'body text',
headers: { 'Content-Type': 'text/plain' },
})
expect(res.status).toBe(400)
})
})Currently, this test fails - it returns 200 instead of 400 because text/plain validation is not implemented.
So, we need to decide:
- Should we validate
text/plain(orapplication/xmletc.) against the schema? - Should we pass it through without validation?
- Should we return an error for non-JSON/Form content types?
I think we don't need implement all things in this PR, but we have to decide the direction.
Summary
This PR fixes a critical security vulnerability where body validation could be bypassed by omitting the content-type header. The vulnerability was introduced in v0.15.2 (PR #686).
Problem
As reported in issue #891, starting from
@hono/zod-openapiv0.15.2, request body validation can be bypassed by simply omitting the content-type header. This allows attackers to invoke handlers with unvalidated payloads, potentially leading to:Solution
This PR makes validation strict by default to prevent the bypass:
requiredis not specified): Always validate the request bodyrequired: true: Always validate the request bodyrequired: false: Allow empty body only when no content-type is providedKey Changes
Breaking Change
To maintain backward compatibility for legitimate use cases, explicitly set
required: falsein the route configuration to allow optional body validation.Test Plan
validation-security.test.ts)Related Issues
Fixes #891