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

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

Open
edoardo-bluframe opened this issue Apr 18, 2024 · 1 comment
Open

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

edoardo-bluframe opened this issue Apr 18, 2024 · 1 comment
Labels
bug Bug or defect

Comments

@edoardo-bluframe
Copy link

edoardo-bluframe commented Apr 18, 2024

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 edoardo-bluframe added the bug Bug or defect label Apr 18, 2024
@damusix
Copy link
Contributor

damusix commented Jun 15, 2024

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

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

No branches or pull requests

2 participants