Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

[Fixes #1133] Limit the path on the nonce and correlation id cookies #1262

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

javiercn
Copy link
Member

No description provided.

@javiercn javiercn requested a review from Tratcher June 13, 2017 16:13
@dnfclas
Copy link

dnfclas commented Jun 13, 2017

@javiercn,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@Tratcher
Copy link
Member

Did you intentionally skip Twitter? How about CookiePolicy?

@@ -332,7 +332,18 @@ protected virtual Task<bool> HandleSignOutCallbackAsync()
if (Options.ProtocolValidator.RequireNonce)
{
message.Nonce = Options.ProtocolValidator.GenerateNonce();
WriteNonceCookie(message.Nonce);
var cookieOptions = new CookieOptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this back inside WriteNonceCookie so it's out of the way?

@kevinchalet
Copy link
Contributor

Why not events instead of options delegates?

@Tratcher
Copy link
Member

An interesting thought, but we want a consistent pattern across the stack and most other components don't use events.

@javiercn
Copy link
Member Author

@Tratcher I specifically left out anything that is not part of the OpenIdConnect middleware hierarchy.
I took a quick look at the repo and seems like twitter is the only other middleware that emits a cookie, we can path that one too if we choose to do so.

Here is the query that I used: https://github.com/aspnet/Security/search?utf8=%E2%9C%93&q=Cookies.Append&type=

As for the Action<HttpContext,CookieOptions> addition for other cookies I'm going to leave that to whomever does aspnet/HttpAbstractions#853. That said, I think only twitter should have that option. We shouldn't do this in the cookiepolicy middleware as it would broaden the scope of the middleware a lot (and would require us to think more carefully of the consequences).

@javiercn javiercn merged commit 879f0b7 into dev Jun 15, 2017
@Tratcher Tratcher deleted the javiercn/1133 branch August 7, 2017 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants