search

expressjs / cookie-parser

Parse HTTP request cookies
MIT License
1.96k stars 220 forks source link

Update cookie dependency for SameSite=None #53

Closed rowan-m closed 5 years ago

rowan-m commented 5 years ago

The 0.4.0 release of jshttp/cookie adds support for the SameSite=None attribute as per https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-03. This pull request updates the dependency on cookie.

I'm happy to update the other dependencies as the test suite happily passes with current versions for everything, but I didn't want to be over enthusiastic there!

dougwilson commented 5 years ago

Out of curiosity, how would you even use SameSite=None with this module (cookie-parser)?

As far as I know, parsing the Cookie header doesn't know anything about the state of the SameSite attribute?

rowan-m commented 5 years ago

Oh, wait - you know what… ignore me. :sweat_smile: I was too fixated on updating my own demo and was just naively assuming there was a dependency there. I see you've already update the express deps, so that fixes my issue! Closing off this.

artem-malko commented 4 years ago

@dougwilson I have some problems with it) Express require cookie-parser middleware to parse cookie Header. So, we have to require express and cookie-parser. express has its own dep cookie, which has the last version of this package. But cookie-parse has 0.3.1 of cookie package. And 0.3.1 does not support SameSite=None. So, I have two questions:

  1. What is the problem to update dep in cookie-parser?
  2. How it works with express, if both packages has its own deps?
dougwilson commented 4 years ago

Hi @artem-malko perhaps you can provide an example application where it is not working?

artem-malko commented 4 years ago

@dougwilson I'm not saying, that it is not working) I just can not understand how it works, and what is the problem to update dependency?)

dougwilson commented 4 years ago

I will in due time. There are much more urgent issues to attend to first. If there is no issue you are having, then it is not worth spending time on doing. It is all about managing priorities.

artem-malko commented 4 years ago

@dougwilson I've created a small project, there you can see the problem with old cookie version in deps. https://github.com/artem-malko/express-cookie-parser-test-samesite-none You will see all requirements in Readme. Wait for feedback from you)

artem-malko commented 4 years ago

By the way as I can see, you use cookie.parse from cookie package. So, it means, that I will get an error with cookie sameSite: 'none', if I will use cookie-parser without express even without typescript and webpack. Is it enough to update cookie dep version?)

dougwilson commented 4 years ago

What am I supposed to see? Cloning the repo and following the instructions, I then call /none route and get back a hello world with a set-cookie header that contains samesite=none

dougwilson commented 4 years ago

you use cookie.parse from cookie package. So, it means, that I will get an error with cookie sameSite: 'none'

How so? The samesite change only modified cookie.seralize .

artem-malko commented 4 years ago

What am I supposed to see? Cloning the repo and following the instructions, I then call /none route and get back a hello world with a set-cookie header that contains samesite=none

Just try to refresh page sometimes. I have an error in output:

BAD COOKIE SAME SITE 
TypeError: option sameSite is invalid 

TypeError: option sameSite is invalid at Object.serialize (webpack:///./node_modules/cookie/index.js?:174:15) at 
ServerResponse.res.cookie (webpack:///./node_modules/express/lib/response.js?:857:36) at eval 
(webpack:///./src/index.ts?:9:13) at Layer.handle [as handle_request] 
(webpack:///./node_modules/express/lib/router/layer.js?:95:5) at next 
(webpack:///./node_modules/express/lib/router/route.js?:137:13) at Route.dispatch 
(webpack:///./node_modules/express/lib/router/route.js?:112:3) at Layer.handle [as handle_request] 
(webpack:///./node_modules/express/lib/router/layer.js?:95:5) at eval 
(webpack:///./node_modules/express/lib/router/index.js?:281:22) at Function.process_params 
(webpack:///./node_modules/express/lib/router/index.js?:335:12) at next 
(webpack:///./node_modules/express/lib/router/index.js?:275:10)

How so? The samesite change only modified cookie.seralize .

If cookie header will have sameSite: 'none', cookie.parse will catch error "TypeError: option sameSite is invalid"

dougwilson commented 4 years ago

Just try to refresh page sometimes. I have an error in output:

I have been refreshing a bunch of times and no errors. Maybe you have something cached somewhere?

If cookie header will have sameSite: 'none', cookie.parse will catch error "TypeError: option sameSite is invalid"

No it won't, as the cookie header would never have that value, but even if it did since cookie header is just name value pairs, it would parse. This should not happen. Can you show a cookie.parse call that throws that error?

dougwilson commented 4 years ago

@artem-malko i think this is your issue: https://github.com/webpack/webpack/issues/6538 and i don't know what about my install is working, but maybe that would help with your install?

artem-malko commented 4 years ago

@artem-malko i think this is your issue: webpack/webpack#6538 and i don't know what about my install is working, but maybe that would help with your install?

It's like a magic, really. It works with advice from issue from webpack. But I can't realize, how it can be possible, that it works on your computer)

No it won't, as the cookie header would never have that value, but even if it did since cookie header is just name value pairs, it would parse. This should not happen. Can you show a cookie.parse call that throws that error?

I agree with you. This should not happen. Sorry, my fault)