jshttp / cookie

HTTP server cookie parsing and serialization
MIT License
1.36k stars 159 forks source link

Add support for Partitioned. #153

Closed pazguille closed 1 year ago

pazguille commented 1 year ago

@dougwilson I sent a complete PR for #151 🚀

pazguille commented 1 year ago

Hi @dougwilson Any news?

dougwilson commented 1 year ago

Oh, didn't realize there was another PR for this; I checked the original a few days ago. I will take a look at this one ASAP

pazguille commented 1 year ago

Hi @dougwilson, Any news? We already need this feature.

vladimiratanasov commented 1 year ago

@pazguille thank you for opening this PR. May I suggest adding the following to the readme as well, it is important for setting the Partitioned state

Partitioned cookies must be set with Secure and Path=/. In addition, it is recommended to use the __Host prefix when setting partitioned cookies to make them bound to the hostname and not the registrable domain.

Source: https://developer.mozilla.org/en-US/docs/Web/Privacy/Partitioned_cookies#how_does_chips_work

It's probably worth adding some validations in the code if the Secure and Path properties are set correctly and throw some warning/error. I don't think it's a good idea to change the state of the cookie automatically.

dougwilson commented 1 year ago

We don't do that with others flags like samesite, though. Do we feel that is a necessary thing to do? Probably should decide that before we merge bc it will be hard to add that in later without major version bumps and stuff. The samesite stuff kept changing on us (it didn't even start out with that name) as well, so I wouldn't be surprised if partitioned changes over time too, so may be better to just leave it as this PR is at least until the draft gets ironed out I think, right?

vladimiratanasov commented 1 year ago

@dougwilson, makes sense. A disclaimer in the readme might be enough, stating that this library does not validate fields that partitioned depends on and this should be done on the client side. Once all browsers adapt a common standard (if ever), a validation can be added.

pazguille commented 1 year ago

Is this necessary for merge this PR? Please let me know. We need this to update our cookies.

dougwilson commented 1 year ago

I'm not sure it is really necessary, no. Anyway, any objections for me landing this pr as is in lile 24 hours please let me know otherwise i plan to release it.

vladimiratanasov commented 1 year ago

@dougwilson are you ok with merging this PR and releasing it? We will then have to wait for express to adopt the change, at least in our case since we don't use the package directly, and that might take a while.

Edit: I see you are also a maintainer of express, so hopefully the change will be adopted quickly :)

nicob-29 commented 1 year ago

@dougwilson, Any Update ? This PR is blocked other products .... :-(

dougwilson commented 1 year ago

Sorry I was looking into getting the CI passing on the PR and life got in the way. I think I got it figured out though and should have it releaaed soon. Cannot merge a PR where the CI is failing.

nicob-29 commented 1 year ago

Hi @dougwilson Tests about NodeJS 0.6, 0.8, 0.10, 0.12 don't make sense. These version are longtime ago unsupported by NodeJS. How can we progress on it ?

dougwilson commented 1 year ago

They are all fixed now. I just need to rebasw and merge this when I get home, just ran out of time yesterday.