arb / celebrate

A joi validation middleware for Express.
MIT License
1.34k stars 65 forks source link

Add support for cookie-parser #99

Closed jdtzmn closed 5 years ago

jdtzmn commented 5 years ago

Added support for cookie-parser middleware

PS: 🥂to how cleanly your code is written.

jaw187 commented 5 years ago

LGTM

arb commented 5 years ago

Hey @jdtzmn thanks for the PR! Since you're adding this, I assume you have a use case for it right now? What are you trying to validate in cookies? Just some state in there or does this have something to do with authenticating users?

jdtzmn commented 5 years ago

I do have a use case. I am implementing a Spotify OAuth flow and one of the requests to their API recommends that a state query is tagged along to verify callback authenticity. To double check the returned state from their API, I am storing a value in the site's cookies that can be compared against later by the server.

So what I am trying to validate in cookies is that the callback request to the server not only has a query parameter named state, but a signedCookies parameter named spotify_state.

jdtzmn commented 5 years ago

One feature that I might add (I don't know if this is a part of Joi already or not) is that it is not possible for extra cookies to be accepted by the server. For example, if the client requests a route on the server, I'm not sure that the client should be given an error response just because they have extra cookies—the request should only be declined if the necessary cookies are missing or of the wrong type. Not sure how I would implement this though.

For my specific app: When I requested the callback route as the redirectUri in the Spotify authentication, an error response status was returned because the request had extra cookies even though it did meet the necessary cookies requirements.

arb commented 5 years ago

@jdtzmn to allow extra cookies, you'd add .unknown() to your cookie schema. I did it one of the tests for headers because there are always extra headers.

I'm a little leary to add cookie support because of the different ways parsed cookies can be represented depending on the parser used. This would require the user of celebreate to know how weird cookies are handled and write schema accordingly. Otherwise, I could end up with a bunch of non-issues being opened because of how different cookie parser middleware work on the edge cases.

On the other hand, hapi does this and part of the goal of this lib is to bring hapi-like validation to express. I haven't reviewed the actual code yet as I'm still on the fence if I want to support cookie valiadtion or not. This is the first time this feature has been requested.

jdtzmn commented 5 years ago

@arb That sounds reasonable. Just keep me posted as to what you decide.

jdtzmn commented 5 years ago

@arb Wait I'm confused. It makes sense that you would not add cookie support, but you have added support for body-parser, which is quite similar in many respects. It is also made and maintained by the same people. Just so I can know for the future, how is cookie-parser support different than having body-parser support?

arb commented 5 years ago

Great question @jdtzmn! I supported req.body because I think most middleware functions that parse the incoming request payload put it in req.body. Since express leaves this kind of thing out in userland code, I can't KNOW with certainty where certain values on the request object are going to live. req.body seems pretty standard so I rolled the dice on validating there.

Cookies could be under req.cookie, req.cookies, req.signedCookies... I'm not very knowledgable in the express ecosystem to be able to say what cookie parsing middleware is the defacto standard.

This is just speculation on my part. Do people use anything other than the official cookie-parser for cookie needs in their express servers?

jdtzmn commented 5 years ago

@arb I don’t think that people use anything other than the official cookie-parser. While I am sure that people construct their own, cookie-parser, like body-parser, is maintained by the creators of express and is officially used by most express apps (for example, it is part of the express middleware documentation)