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

WIP Add way to reject body with the wrong content type #274

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crccheck
Copy link

@crccheck crccheck commented Oct 24, 2017

I was messing around with an Express route I had bodyParser.json() on, and kind of assumed that if I gave it a random POST, it would fail. But after reading the README again, the way Body Parser works makes sense. I started to think about writing this logic apart, but it seemed really closely coupled to the logic of Body Parser, so I looked adding this.

I started off writing this as an issue but I figure a diff would help illustrate the idea a lot better.

This adds a new strictType option. It's false by default, but when enabled, it fails requests with a HTTP 415 Unsupported Media Type error

If there's interest, I'll finish out this PR:

  • add the same option to the other types
  • copy edits
  • README documentation

@dougwilson
Copy link
Contributor

Neat! Obviously we would want all the types to have this behavior, not just the json. There was a plan written out for how we wanted to add this (vety different from what you implemented here), I need to find that and link you to it, if you want to work on it. The method you did here was mentioned, but was quickly rejected because you cannot stack the parsers properly and we did come up with a good solution that no one has yet worked on if you would like to.

@dougwilson
Copy link
Contributor

If you're wondering, I'm searching for the conversation. From what I recall it was two main APIs, a bodyParser.only() and a bodyParser.none() or something. Basically some way to either (1) wrap a series of parsers that will 415 if none of them caused a parse and / or a middleware that will 415 when hit (i.e. an "I have declared all my parsers now, any unparsed body is rejected"). The thread had a lot more background and information, which is why I'm trying to find it.

@crccheck
Copy link
Author

because you cannot stack the parsers properly

I was wondering about that too. Express lets you add multiple middlewares, but I didn't see any reference to "chain" or "stack" in the README so went ahead with this approach.

@dougwilson
Copy link
Contributor

Because being able to stack things is implied with how middleware works; it is not expected that a middleware will respond to something it cannot handle which is why a non matching type just "falls through" so you can stack them. Almost all the example in the README here shows this behavior as well. A common example is the statix file serving, where requesting a non existant file doesn't cause the static middleware to respond with a 404, instead it just continyes down your middleware staxk to handle it some other way.

@crccheck
Copy link
Author

Something that would help inform the design is:

If you chained two body parsers, should the error message be able to say "Expected Content-Type application/json or application/x-www-form-urlencoded but received text/plain"

Middlewares are independent and generally don't know about each other, but a middleware generating a message like that would require outside knowledge.

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

Successfully merging this pull request may close these issues.

2 participants