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

[2.1] Trouble deleting SameSite=None cookies without Secure #17833

Closed
Tratcher opened this issue Dec 12, 2019 · 7 comments
Closed

[2.1] Trouble deleting SameSite=None cookies without Secure #17833

Tratcher opened this issue Dec 12, 2019 · 7 comments
Assignees
Milestone

Comments

@Tratcher
Copy link
Contributor

@Tratcher Tratcher commented Dec 12, 2019

IdentityServer/IdentityServer4#3901

One of the changes in the new SameSite spec is that cookies marked as SameSite=None can only be set if also marked as Secure. This is fine when we're setting cookies, but it also causes a problem when trying to delete cookies.

There are a few code paths such as the ChunkingCookieManager used by CookieAuth that don't properly flow the Secure attribute on the delete code path.
https://github.com/aspnet/AspNetCore/blob/049cdec742ac8a582d6a5d85ed4ae590b42db681/src/Shared/ChunkingCookieManager/ChunkingCookieManager.cs#L287

This was fixed in 3.0 but should be backported to 2.1. 2.2 is also affected, but has reached end-of-life.

@blowdart @anurse

@Tratcher

This comment has been minimized.

Copy link
Contributor Author

@Tratcher Tratcher commented Dec 12, 2019

Mitigation: The CookieManager can be set, replace the 2.1 ChunkingCookieManager with the code from 3.0. You may also be able to adjust this behavior using CookiePolicy similar to the example shown in #14996.

@anurse

This comment has been minimized.

Copy link
Contributor

@anurse anurse commented Dec 12, 2019

This seems like it would meet the 2.1 bar for me. It requires a large workaround (that is basically porting the 3.0 change). The original SameSite changes met the bar and this is a clear continuation of that work. (cc @Pilchie in case he has thoughts)

@ulrichb

This comment has been minimized.

Copy link

@ulrichb ulrichb commented Dec 12, 2019

I think the commit which just could be cherry-picked is 6fa9398#diff-2f4e8f46b2b0ca7fa8ec27a1608968d5.

@ulrichb

This comment has been minimized.

Copy link

@ulrichb ulrichb commented Dec 12, 2019

@anurse And 2.2?

@anurse

This comment has been minimized.

Copy link
Contributor

@anurse anurse commented Dec 12, 2019

2.2 will be end-of-life on December 23rd, 2019. Our next patch will be in January. So no, 2.2 will not be patched with this change.

@gingters

This comment has been minimized.

Copy link

@gingters gingters commented Dec 20, 2019

Mitigation: The CookieManager can be set, replace the 2.1 ChunkingCookieManager with the code from 3.0. You may also be able to adjust this behavior using CookiePolicy similar to the example shown in #14996.

Just to double-check:
@Tratcher Is it really as simple as copying that patched class in your project and setting a new instance of it to the CookieAuthenticationOptions.CookieManager property when configuring the services?

It does not help to put that in the DI, as its not taken from there anyways, right?

@Tratcher

This comment has been minimized.

Copy link
Contributor Author

@Tratcher Tratcher commented Dec 20, 2019

@gingters a copy should work, though I haven't tried it yet. You will want to rename it.

You're correct that DI is not involved here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.