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

415 error on routes that does not exist with body multipart/form-data #381

Closed
2 tasks done
mage1k99 opened this issue Aug 17, 2022 · 5 comments
Closed
2 tasks done
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@mage1k99
Copy link

mage1k99 commented Aug 17, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.0.0

Plugin version

7.1.0

Node.js version

v16.16.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.5 (21G72)

Description

Fastify handles 404 for us, but when used with fastify multipart. it creates an 415 error instead of 404 for routes that does not exist. To demonstrate this I have created a project with fastify cli and tracked the steps to reproduce in each commits Link to repo.

I have tested a bunch of request with different method

Method route Shows 415 ?
GET /notfound false, 404 error thrown
POST /notfound true
PUT /notfound true
PATCH /notfound true
DELETE /notfound true

As you could see I'm getting 404 for a route that does not exist when using no body in POST request method

image

Now lets change the body type to multipart/form-data and send the request

image

This issue is the cause of issue #379 and created new issue as #379 (comment)

Steps to Reproduce

  • create a project with fastify cli
  • add the @fastify/multipart plugin
  • register the multipart plugin by copy pasting the below code
import { FastifyMultipartAttactFieldsToBodyOptions } from "@fastify/multipart";
import fp from "fastify-plugin";

export interface RootPluginOptions {
  // Specify Support plugin options here
}

// The use of fastify-plugin is required to be able
// to export the decorators to the outer scope
export default fp<RootPluginOptions>(async (fastify, opts) => {
  fastify.register<FastifyMultipartAttactFieldsToBodyOptions>(
    require("@fastify/multipart"),
    {
      attachFieldsToBody: true,
    }
  );
});
  • create route that accepts multipart/form-data in request
  • send POST request with multipart/form-data as body to any route that does not exist. In our case I used /notfound route.
  • the error is shown

Expected Behavior

  • Throw 404 error instead of 415 error when a POST, PUT, PATCH, DELETE route that does not exist.
  • This also creates confusion when development phase as the developer might get confused if the plugin got registred or not instead of checking the url
@mcollina
Copy link
Member

Good spot! We should basically skip all the incoming body if it's a 404 request. This can be done by adding a few if checks in fastify-multipart using the req.is404 property (https://www.fastify.io/docs/latest/Reference/Request/).

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added bug Confirmed bug good first issue Good for newcomers labels Aug 17, 2022
@mage1k99
Copy link
Author

mage1k99 commented Sep 2, 2022

Sorry about the delay, the email notification did not work. will look at it as soon as possible

@mage1k99
Copy link
Author

Hey @mcollina I don't think that this is the issue with fastify-multipart, this is issue with Should this be done in fastify itself ? This is because unregistered content type can cause 415 so checking if the route exist before checking the content type is registered is a good idea

image

As you could see at that 404 routes are handled by the default handler in onRequest

so adding request.is404 in ContentTypeParser.js Right here should do the job right ?

image

@mage1k99
Copy link
Author

I have made a pull request, fastify/fastify#4286

@mcollina
Copy link
Member

Good spot, thanks!

mage1k99 added a commit to mage1k99/fastify that referenced this issue Sep 17, 2022
- check if the route is 404 only if the content-parser is undefined
by this we can avoid breaking changes.
- this also passes previous tests

this resolves issue
- fastify/fastify-multipart#381
Eomm added a commit to fastify/fastify that referenced this issue Sep 18, 2022
* check if route exist before checking content type

* added test for route check before content type

* pass tests

- check if the route is 404 only if the content-parser is undefined
by this we can avoid breaking changes.
- this also passes previous tests

this resolves issue
- fastify/fastify-multipart#381

* Update lib/contentTypeParser.js

Noted, thanks for the review

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

* pass tests from wrong return

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants