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

Set secure flag for remember_me cookie #295

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Changelog
## HEAD

## 0.16.3

* Add secure flag for remember_me cookie [#295](https://github.com/Sorcery/sorcery/pull/295)

## 0.16.2

* Inline core migration index definition [#281](https://github.com/Sorcery/sorcery/pull/281)
Expand Down
5 changes: 5 additions & 0 deletions lib/generators/sorcery/templates/initializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
#
# config.remember_me_httponly =

# Set secure flag for remember_me cookie
# Default: `false`
#
# config.remember_me_secure =

# Set token randomness. (e.g. user activation tokens)
# The length of the result string is about 4/3 of `token_randomness`.
# Default: `15`
Expand Down
7 changes: 4 additions & 3 deletions lib/sorcery/controller/submodules/remember_me.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ def self.included(base)
base.send(:include, InstanceMethods)
Config.module_eval do
class << self
attr_accessor :remember_me_httponly
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)
Copy link
Member

Choose a reason for hiding this comment

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

nil would likely be a safer default here. Still would need to sanity check how this interacts with force_ssl either way.

end
end
merge_remember_me_defaults!
Expand Down Expand Up @@ -71,7 +71,8 @@ def set_remember_me_cookie!(user)
value: user.send(user.sorcery_config.remember_me_token_attribute_name),
expires: user.send(user.sorcery_config.remember_me_token_expires_at_attribute_name),
httponly: Config.remember_me_httponly,
domain: Config.cookie_domain
domain: Config.cookie_domain,
secure: Config.remember_me_secure
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
end
end
Expand Down