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: return status 500 when using validator 'form' #1554

Merged
merged 3 commits into from
Oct 11, 2023
Merged

fix: return status 500 when using validator 'form' #1554

merged 3 commits into from
Oct 11, 2023

Conversation

b-marques
Copy link
Contributor

@b-marques b-marques commented Oct 8, 2023

When using validator('form', ...) hono is returning a 500 status when receiving a POST request with a JSON in request body, instead of a bad request 400, .

This is happenning due to a unhandled error in an
underlying library (@miniflare).
cloudflare/miniflare#711

The code changes in this PR are responsible to prepare the code to handle possible TypeError that can be thrown in the future, by the lib doing the FormData parsing, as per, https://fetch.spec.whatwg.org/#dom-body-formdata.

This PR should wait for bugfix on @miniflare.

Author should do the followings, if applicable

  • Add tests
  • Run tests (don't have deno to perform yarn test:all
  • yarn denoify to generate files for Deno

@yusukebe
Copy link
Member

yusukebe commented Oct 9, 2023

Hi @b-marques

Thank you for the PR!

I think it is a miniflare problem that the test fails. The following PR does not use vitest-environment-miniflare but only the Node.js native Web APIs. Using this, that errors will not occur.

#1558

Since vitest-environment-miniflare may not be maintained in the future, I wanted to use the Node.js's. I will merge #1558 and then I want you to fetch it over here and try again. What do you think?

@b-marques
Copy link
Contributor Author

I see...

For sure, I will wait for #1558 to be merged to proceed with tests on my side.

@yusukebe
Copy link
Member

Hi @b-marques

I've merged #1558 into the main. Try it.

When using `validator('form', ...)` hono is returning a 500 status
when receiving a POST request with a JSON in request body, instead
of a bad request 400, .

This is happenning due to a unhandled error in an
underlying library (@miniflare).
cloudflare/miniflare#711

The code changes in this PR are responsible to prepare the code to
handle possible TypeError that can be thrown in the future, by the lib
doing the FormData parsing, as per, https://fetch.spec.whatwg.org/#dom-body-formdata.

This PR should wait for bugfix on @miniflare.
@b-marques
Copy link
Contributor Author

Hi @yusukebe , it stills relevant.

The Node.js native Web APIs can also throw Errors that hono is not handling.

@b-marques
Copy link
Contributor Author

@yusukebe I would like also to mention about the possibility to add a check for the 'appliaction/json' header in the validator. Since a forgery attack trying to avoid preflight request can send a json payload with wrong content-type, and hono will accept and validate it.

@yusukebe
Copy link
Member

@b-marques

The Node.js native Web APIs can also throw Errors that hono is not handling.

The test on my local and CI passed, is there still a problem?

I would like also to mention about the possibility to add a check for the 'appliaction/json' header in the validator.

That's right. If this PR is merged, can you create another one?

@b-marques
Copy link
Contributor Author

b-marques commented Oct 10, 2023

@yusukebe

The test on my local and CI passed, is there still a problem?

With the changes in this PR, it's not going to be a problem anymore. But without those changes, we still receive a 500 status, when we should receive a 400 Bad request.

That's right. If this PR is merged, can you create another one?

For sure.

…cation

Forgery attacks will try to avoid preflight requests when POSTing JSON
payloads manipulating the HTTP header Content-Type. For example, it will
send a JSON payload with `Content-Type=text/plain`, but the request stills
containing a JSON in its body. Those requests must be rejected.

Thus, when using the validator with the target set to `json`, we must
check the Content-Type header.
@b-marques
Copy link
Contributor Author

@yusukebe done (:

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

@b-marques

Thanks! I've left one comment.

src/validator/validator.ts Outdated Show resolved Hide resolved
Change JSON validation to only allow Content-Type header starting with
'application/json'.

Change from regexp test to starsWith builtin function, to make code more
expressive.
@yusukebe
Copy link
Member

Hi @b-marques!

Perfect! I'll merge it now. Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants