hapijs / hapi

The Simple, Secure Framework Developers Trust
https://hapi.dev
Other
14.63k stars 1.34k forks source link

multipart validation doesn't comply with new fetch api #4500

Open edoardo-bluframe opened 6 months ago

edoardo-bluframe commented 6 months ago

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 :smile:

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 :blush: - 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?

damusix commented 5 months ago

@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: