multipart validation doesn't comply with new fetch api
edoardo-bluframe opened this issue · 1 comments
Runtime
node.js
Runtime version
18+
Module version
21.3.3
Last module version without issue
No response
Used with
No response
Any other relevant information
No response
What are you trying to achieve or the steps to reproduce?
Set multipart payload validation:
payload: {
// 2MB
maxBytes: 2097152,
maxParts: 2,
multipart: {
output: "stream"
},
parse: true
},
Pass a payload larger than limit via fetch
from web
try {
const response = await fetch(`${HOST}:${PORT}/image`, {
body: image,
headers: {
Authorization: `Bearer ${token}`
},
method: "POST"
})
if (!response.ok) {
if(response.status === 413) {
console.error("Image too large!")
}
}
} catch(error) {
console.error("Generic error")
console.error(error)
}
Expect to handle 413 when response.status === 413
What was the result you got?
Generic error
What result did you expect?
Image too large!
The problem here is actually in hapijs/pez
and the way it handles maxBytes
- and other multipart errors
pez
hasn't been touched in a year and a half though and has near zero activity, but it is officially part of Hapi so I raised it here where activity is actually happening 😄
On line 136 of lib/index.js
in hapijs/pez
:
const onReqData = (data) => {
this._bytesCount += Buffer.byteLength(data);
if (this._bytesCount > this._maxBytes) {
finish(Boom.entityTooLarge('Maximum size exceeded'));
}
};
This works but it happens before the actual Hapi validation occurs. I understand that multipart is being parsed before but this doesn't work with the new fetch
API
From MDN Docs:
A fetch() promise only rejects when the request fails, for example, because of a badly-formed request URL or a network error. A fetch() promise does not reject if the server responds with HTTP status codes that indicate errors (404, 504, etc.). Instead, a then() handler must check the Response.ok and/or Response.status properties.
Every other validation in Hapi - say within failAction
- behaves exactly as the fetch
API expects. Request fails with !response.ok
and response contains both response.statusText
and response.status
Because of how early hapijs/pez
throws the error the fetch
API actually interprets it as a lower level error. fetch
actually throws
and never gets to the !response.ok
part
That would actually be okay as we can catch it, if it wasn't that that way we completely lose statusText
and status
and those are... essential for actually displaying a formatted error say in the UI. This example has console.error
for simplicity
We use hapi
in production everywhere for our projects - I really like hapi
😊 - and we just ran into this one
I know hapijs/pez
doesn't get worked on a lot but this is a pretty major issue for UI/UX
I can help with a PR if you want me to - maybe a little guidance on best practices?
@edoardo-bluframe If you can setup a codepen example, or a repo reproducing this issue, that'd be great. Yes, feel free to open a PR if you think you know how to fix the issue! If it's found to be an issue, this seems like a good find. Thank you!
Guidance on PRs:
- Make sure you address failing tests if any
- Add tests if required (in this case it would be fitting to do so)
- If it's exposed, be sure to document it (i don't think it'll be this case)
- If it affects typings, be sure to add typings
- If it's a breaking fix, it will need to be included in a major release (don't think is your case)