-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Enable CSRF cookie to has secure flag enabled #2768
Conversation
…the csrf cookies secure flag. Resolves #2649
src/Nancy/Security/Csrf.cs
Outdated
@@ -26,7 +26,8 @@ public static class Csrf | |||
/// <remarks>This is disabled by default.</remarks> | |||
/// <param name="pipelines">The application pipelines.</param> | |||
/// <param name="cryptographyConfiguration">The cryptography configuration. This is <see langword="null" /> by default.</param> | |||
public static void Enable(IPipelines pipelines, CryptographyConfiguration cryptographyConfiguration = null) | |||
/// <param name="csrfCookieSecureFlag">Set the CSRF cookie secure flag. This is <see langword="false"/> by default</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better named as just useSecureCookie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
src/Nancy/Security/Csrf.cs
Outdated
@@ -64,7 +65,7 @@ public static void Enable(IPipelines pipelines, CryptographyConfiguration crypto | |||
var tokenString = GenerateTokenString(cryptographyConfiguration); | |||
|
|||
context.Items[CsrfToken.DEFAULT_CSRF_KEY] = tokenString; | |||
context.Response.Cookies.Add(new NancyCookie(CsrfToken.DEFAULT_CSRF_KEY, tokenString, true)); | |||
context.Response.Cookies.Add(new NancyCookie(CsrfToken.DEFAULT_CSRF_KEY, tokenString, httpOnly: true, secure: csrfCookieSecureFlag)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really use named parameters anywhere else in this section of the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a whore for clarity true, true
requires intellisense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be true, true
, it would be true, useSecureCookie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true; but true still just means true 😉
Entirely up to you; your code base, I just occasionally have to touch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of those changes are in
Prerequisites
Description
Adds optional bool to signature of Csrf.Enable(this.pipelines) to set the csrf cookies secure flag.
I've set it to false by default to avoid unintended activation.
Resolves #2649