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

Add support for Partitioned. #153

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Add support for Partitioned. #153

merged 1 commit into from
Nov 7, 2023

Conversation

pazguille
Copy link
Contributor

@pazguille pazguille commented Aug 30, 2023

@dougwilson I sent a complete PR for #151 🚀

@pazguille
Copy link
Contributor Author

Hi @dougwilson Any news?

@dougwilson
Copy link
Contributor

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
Copy link
Contributor Author

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

@vladimiratanasov
Copy link

vladimiratanasov commented Oct 12, 2023

@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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor Author

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

@dougwilson
Copy link
Contributor

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
Copy link

vladimiratanasov commented Oct 25, 2023

@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
Copy link

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

@dougwilson
Copy link
Contributor

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
Copy link

nicob-29 commented Nov 6, 2023

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
Copy link
Contributor

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

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.

4 participants