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: unhandled error thrown by busboy #711

Merged
merged 3 commits into from
Oct 25, 2023
Merged

fix: unhandled error thrown by busboy #711

merged 3 commits into from
Oct 25, 2023

Conversation

b-marques
Copy link
Contributor

As per, https://fetch.spec.whatwg.org/#dom-body-formdata, TypeErrors are expected to be thrown. Busboy errors were being unhandled.

As per, https://fetch.spec.whatwg.org/#dom-body-formdata, TypeErrors are
expected to be thrown. Busboy errors were being unhandled.
@changeset-bot
Copy link

changeset-bot bot commented Oct 8, 2023

⚠️ No Changeset found

Latest commit: b84e8d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Hey! 👋 Thanks for the PR (and including tests! 🎉 )! I've added some comments. 👍

packages/core/src/standards/http.ts Outdated Show resolved Hide resolved
packages/core/src/standards/http.ts Outdated Show resolved Hide resolved
packages/core/test/standards/http.spec.ts Outdated Show resolved Hide resolved
packages/core/test/standards/http.spec.ts Outdated Show resolved Hide resolved
packages/core/test/standards/http.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: MrBBot <me@mrbbot.dev>
@b-marques
Copy link
Contributor Author

Sorry about the forced-push, this commit: 6c71c73, was a missclick from my side :/

Had to add some missing fix

@b-marques
Copy link
Contributor Author

@mrbbot FYI, from Nodejs v16.19.0 onwards, the undici is included with the multipart/form-data parser.

yusukebe pushed a commit to honojs/hono that referenced this pull request Oct 11, 2023
* fix: return status 500 when using validator 'form'

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.

* fix: json validator allowing Content-Type value other than json/application

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.

* fix: change check for json Content-Type header

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.

---------

Co-authored-by: Bruno Nascimento <bruno.nascimento@csghq.com>
@mrbbot
Copy link
Contributor

mrbbot commented Oct 19, 2023

The test failures seem to be caused by CI now using Node 21. I can fix those in a separate PR. If you fix that throwsAsync() issue, I'll get this merged. 👍

The expectations weren't included in the `throwAsync()`.
@mrbbot mrbbot merged commit 1837937 into cloudflare:master Oct 25, 2023
3 of 6 checks passed
@mrbbot
Copy link
Contributor

mrbbot commented Jan 17, 2024

Hey! 👋 Apologies for the delay, but this has now been released as part of miniflare@2.14.2. 👍

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