Set secure flag for remember_me cookie#295
Set secure flag for remember_me cookie#295paderinandrey wants to merge 2 commits intoSorcery:mainfrom
Conversation
|
Ah, so this allows users to set secure cookies to true without needing Just to sanity check, @paderinandrey does your use-case prevent you from using |
joshbuker
left a comment
joshbuker
left a comment
There was a problem hiding this comment.
This change is potentially dangerous, depending on how Rails handles the interaction between force_ssl and secure: false. Needs verification.
| httponly: Config.remember_me_httponly, | ||
| domain: Config.cookie_domain | ||
| domain: Config.cookie_domain, | ||
| secure: Config.remember_me_secure |
There was a problem hiding this comment.
I'd be worried that this has the potential to overwrite the default of secure: true when using force_ssl. We'll need to double check that interaction before merging this in. Ideally, adding a test or two.
| attr_accessor :remember_me_httponly, :remember_me_secure | ||
| def merge_remember_me_defaults! | ||
| @defaults.merge!(:@remember_me_httponly => true) | ||
| @defaults.merge!(:@remember_me_httponly => true, :@remember_me_secure => false) |
There was a problem hiding this comment.
nil would likely be a safer default here. Still would need to sanity check how this interacts with force_ssl either way.
|
As @joshbuker wrote, I think there is no problem if you add |
3 participants
Added the ability to set the secure flag for the remember_me cookie
Please ensure your pull request includes the following: