Skip to content

Add support for Partitioned.#153

Merged
dougwilson merged 1 commit intojshttp:masterfrom
pazguille:master
Nov 7, 2023
Merged

Add support for Partitioned.#153
dougwilson merged 1 commit intojshttp:masterfrom
pazguille:master

Conversation

@pazguille
Copy link
Copy Markdown
Contributor

@pazguille pazguille commented Aug 30, 2023

@dougwilson I sent a complete PR for #151 🚀

@pazguille
Copy link
Copy Markdown
Contributor Author

Hi @dougwilson Any news?

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

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

@vladimiratanasov
Copy link
Copy Markdown

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

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

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

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

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

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

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

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

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

@dougwilson dougwilson force-pushed the master branch 3 times, most recently from 188fd6e to bcebc74 Compare November 7, 2023 01:12
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