8000 [HttpFoundation] Cookies Having Independent Partitioned State (CHIPS) by fabricecw · Pull Request #52002 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Cookies Having Independent Partitioned State (CHIPS) #52002

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2023
Merged

[HttpFoundation] Cookies Having Independent Partitioned State (CHIPS) #52002

merged 1 commit into from
Oct 16, 2023

Conversation

fabricecw
Copy link
Contributor
@fabricecw fabricecw commented Oct 12, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT

Due to Chrome's roadmap (and all other major browsers) to phase out third-party cookies starting from midway through 2024, "partitioned" cookies were introduced. If a cookie is flagged as partitioned, its cross-site boundry is tied to the 8000 top-level site.

Considerations: According to current security design, browser will only accept partitioned cookies with the secure flag and SameSite attribute none (otherwise it isn't a third-party cookie...). I classified this as an implementation topic and therefore omitted this validation in the Cookie class itself.

Further information:

@carsonbot carsonbot added this to the 6.4 milestone Oct 12, 2023
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

OskarStark
OskarStark reviewed Oct 12, 2023
View reviewed changes
OskarStark
OskarStark reviewed Oct 12, 2023
View reviewed changes
OskarStark
OskarStark reviewed Oct 12, 2023
View reviewed changes
@OskarStark
Copy link
Contributor

Please rebase your branch. We cannot merge PRs containing merge commits. Thanks

@fabricecw
Copy link
Contributor Author

Please rebase your branch. We cannot merge PRs containing merge commits. Thanks

Thanks @OskarStark, that was unintentional. Cleaned up.

@Nyholm
Copy link
Member
Nyholm commented Oct 12, 2023

Lovely. Thank you for this PR. What a great first contribution!

I agree with all your changes, but I am a bit hesitant on adding types to protected properties (which is not mentioned in our BC policy). If you want this PR to get a quick merge, I suggest you remove all unrelated changes. Example: Changing the existing properties and doing assertSame() on existing tests.

Doing this will make sure the discussions are only focus on the partitioned cookie feature.

Also, can you please have a look on the fabbot.io CI job?

@fabricecw
Copy link
Contributor Author
fabricecw commented Oct 12, 2023 •
edited
Loading

I agree. Apart from a very low likelyhood of a BC, typed attributes can wait until a major release. I further reverted the assertSame change as suggested by @OskarStark even I like the stricter assertions.

All HttpFoundation tests are passing.

stof
stof requested changes Oct 13, 2023
View reviewed changes
@fabricecw
Copy link
Contributor Author

Status: Needs Review