Skip to content

Add digest config option for SignedCookiesJar #16434

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 8, 2014
Merged

Add digest config option for SignedCookiesJar #16434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfbedd3
Add config option for cookies digest
lukaszx0 Aug 8, 2014
629f4e0
Return preconfigured AS::MessageVerifier
lukaszx0 Aug 8, 2014
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
5 changes: 5 additions & 0 deletions actionpack/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
* Add `config.action_dispatch.cookies_digest` option for setting custom
digest. The default remains the same - 'SHA1'.

*Łukasz Strzałkowski*

* Extract source code for the entire exception stack trace for
better debugging and diagnosis.

Expand Down
10 changes: 8 additions & 2 deletions actionpack/lib/action_dispatch/middleware/cookies.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class Cookies
SECRET_TOKEN = "action_dispatch.secret_token".freeze
SECRET_KEY_BASE = "action_dispatch.secret_key_base".freeze
COOKIES_SERIALIZER = "action_dispatch.cookies_serializer".freeze
COOKIES_DIGEST = "action_dispatch.cookies_digest".freeze

# Cookies can typically store 4096 bytes.
MAX_COOKIE_SIZE = 4096
Expand Down Expand Up @@ -212,7 +213,8 @@ def self.options_for_env(env) #:nodoc:
secret_token: env[SECRET_TOKEN],
secret_key_base: env[SECRET_KEY_BASE],
upgrade_legacy_signed_cookies: env[SECRET_TOKEN].present? && env[SECRET_KEY_BASE].present?,
serializer: env[COOKIES_SERIALIZER]
serializer: env[COOKIES_SERIALIZER],
digest: env[COOKIES_DIGEST]
}
end

Expand Down Expand Up @@ -441,6 +443,10 @@ def serializer
serializer
end
end

def digest
@options[:digest] || 'SHA1'
end
end

class SignedCookieJar #:nodoc:
Expand All @@ -451,7 +457,7 @@ def initialize(parent_jar, key_generator, options = {})
@parent_jar = parent_jar
@options = options
secret = key_generator.generate_key(@options[:signed_cookie_salt])
@verifier = ActiveSupport::MessageVerifier.new(secret, serializer: NullSerializer)
@verifier = ActiveSupport::MessageVerifier.new(secret, digest: digest, serializer: NullSerializer)
end

def [](name)
Expand Down
17 changes: 17 additions & 0 deletions actionpack/test/dispatch/cookies_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,23 @@ def test_read_permanent_cookie
assert_equal 'Jamie', @controller.send(:cookies).permanent[:user_name]
end

def test_signed_cookie_using_default_digest
get :set_signed_cookie
cookies = @controller.send :cookies
assert_not_equal 45, cookies[:user_id]
assert_equal 45, cookies.signed[:user_id]
assert_equal 'SHA1', cookies.signed.instance_variable_get(:"@verifier").instance_variable_get(:"@digest")
end

def test_signed_cookie_using_custom_digest
@request.env["action_dispatch.cookies_digest"] = 'SHA256'
get :set_signed_cookie
cookies = @controller.send :cookies
assert_not_equal 45, cookies[:user_id]
assert_equal 45, cookies.signed[:user_id]
assert_equal 'SHA256', cookies.signed.instance_variable_get(:"@verifier").instance_variable_get(:"@digest")
Copy link
Member Author

@lukaszx0 lukaszx0 Aug 8, 2014

Choose a reason for hiding this comment

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

Any idea how to test this nicer?

Copy link
Contributor

@tamird tamird Aug 8, 2014

Choose a reason for hiding this comment

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

you could test the behaviour by verifying the cookie yourself with a new verifier

Copy link
Member Author

@lukaszx0 lukaszx0 Aug 8, 2014

Choose a reason for hiding this comment

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

Initially wanted to do it like this:

get :set_signed_cookie
cookies_sha1 = @controller.send :cookies
@request.env["action_dispatch.cookies_digest"] = 'SHA256'
get :set_signed_cookie
cookies_sha256 = @controller.send :cookies

and compare the two, but after first request verifier object gets cached.

Copy link
Contributor

@tamird tamird Aug 8, 2014

Choose a reason for hiding this comment

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

i don't get it, why do you need two requests? just make one, then instantiate a new AS::MV and verify the result with the digest you expect

Copy link
Member Author

@lukaszx0 lukaszx0 Aug 8, 2014

Choose a reason for hiding this comment

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

Oh right! Yep, that will do, thanks.

end

def test_signed_cookie_using_default_serializer
get :set_signed_cookie
cookies = @controller.send :cookies
Expand Down
6 changes: 6 additions & 0 deletions railties/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
* The `Rails.application.message_verifier` now returns ActiveSupport::MessageVerifier
preconfigured with `:digest` and `:serializer` values set to `config.action_dispatch.cookies_digest`
and `config.action_dispatch.cookies_serializer` accordingly.

*Łukasz Strzałkowski*

* Add `after_bundle` callbacks in Rails templates. Useful for allowing the
generated binstubs to be added to version control.

Expand Down
3 changes: 2 additions & 1 deletion railties/lib/rails/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def key_generator
def message_verifier(verifier_name)
@message_verifiers[verifier_name] ||= begin
secret = key_generator.generate_key(verifier_name.to_s)
ActiveSupport::MessageVerifier.new(secret)
ActiveSupport::MessageVerifier.new(secret, digest: config.action_dispatch.cookies_digest, serializer: config.action_dispatch.cookies_serializer)
end
end

Expand Down Expand Up @@ -257,6 +257,7 @@ def env_config
"action_dispatch.encrypted_cookie_salt" => config.action_dispatch.encrypted_cookie_salt,
"action_dispatch.encrypted_signed_cookie_salt" => config.action_dispatch.encrypted_signed_cookie_salt,
"action_dispatch.cookies_serializer" => config.action_dispatch.cookies_serializer
Copy link
Contributor

@egilburg egilburg Aug 8, 2014

Choose a reason for hiding this comment

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

missing comma, that's what broke the build

Copy link
Contributor

@spastorino spastorino Aug 9, 2014

Choose a reason for hiding this comment

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

yep

"action_dispatch.cookies_digest" => config.action_dispatch.cookies_digest
})
end
end
Expand Down