Use `Base.strict_decode64` instead of `Base.decode64` #10635

Merged
merged 1 commit into from Dec 6, 2013

Conversation

Projects
None yet
4 participants
@vipulnsward
Member

vipulnsward commented May 15, 2013

Use Base.strict_decode64 instead of Base.decode64 just as we do in encoding;

Also change map to map! on new array, to map inplace and reduce extra object allocation

@vipulnsward

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

cc @NZKoz

Member

vipulnsward commented May 15, 2013

cc @NZKoz

@jeremy

View changes

activesupport/lib/active_support/message_encryptor.rb
@@ -76,12 +76,12 @@ def _encrypt(value)
encrypted_data = cipher.update(@serializer.dump(value))
encrypted_data << cipher.final
- [encrypted_data, iv].map {|v| ::Base64.strict_encode64(v)}.join("--")
+ [encrypted_data, iv].map! {|v| ::Base64.strict_encode64(v)}.join("--")

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

I'f you're looking to save object allocations, use

"#{::Base64.strict_encode64 encrypted_data}--#{::Base64.strict_encode64 iv}"

instead of #map + #join.

@jeremy

jeremy May 15, 2013

Owner

I'f you're looking to save object allocations, use

"#{::Base64.strict_encode64 encrypted_data}--#{::Base64.strict_encode64 iv}"

instead of #map + #join.

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

makes sense

@vipulnsward

vipulnsward May 15, 2013

Member

makes sense

end
def _decrypt(encrypted_message)
cipher = new_cipher
- encrypted_data, iv = encrypted_message.split("--").map {|v| ::Base64.decode64(v)}
+ encrypted_data, iv = encrypted_message.split("--").map {|v| ::Base64.strict_decode64(v)}

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

We do use strict_encode64, but does that mean we should be using strict decoding? Why?

@jeremy

jeremy May 15, 2013

Owner

We do use strict_encode64, but does that mean we should be using strict decoding? Why?

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

Since we ensure that data is encoded strictly, so should we be checking on any data that comes into the system is decoded strictly too.

@vipulnsward

vipulnsward May 15, 2013

Member

Since we ensure that data is encoded strictly, so should we be checking on any data that comes into the system is decoded strictly too.

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

@jeremy is this a good reason do be doing so?

@vipulnsward

vipulnsward May 15, 2013

Member

@jeremy is this a good reason do be doing so?

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

They aren't necessarily symmetric, despite the names of the methods.

It turns out that #strict_* implement RFC 4648 whereas the other methods implement RFC 2045.

RFC 2045 decoding of RFC 4648-encoded messages works fine. But we should use #strict_* for both 👍

@jeremy

jeremy May 15, 2013

Owner

They aren't necessarily symmetric, despite the names of the methods.

It turns out that #strict_* implement RFC 4648 whereas the other methods implement RFC 2045.

RFC 2045 decoding of RFC 4648-encoded messages works fine. But we should use #strict_* for both 👍

This comment has been minimized.

Show comment Hide comment
@NZKoz

NZKoz May 15, 2013

Member

Also have we always used strict encoding? We can't start throwing InvalidSignature on data that we previously decoded successfully. Similarly there's no security implications of allowing the non-strict decoding, the data is signed anyway.

@NZKoz

NZKoz May 15, 2013

Member

Also have we always used strict encoding? We can't start throwing InvalidSignature on data that we previously decoded successfully. Similarly there's no security implications of allowing the non-strict decoding, the data is signed anyway.

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 16, 2013

Member

As far as I could tell from the commits, we've always been using strict encoding, but not decoding. I couldn't find the reason behind not decoding strictly though. This PR just adds another check over data signing.

@vipulnsward

vipulnsward May 16, 2013

Member

As far as I could tell from the commits, we've always been using strict encoding, but not decoding. I couldn't find the reason behind not decoding strictly though. This PR just adds another check over data signing.

@@ -91,7 +91,7 @@ def _decrypt(encrypted_message)
decrypted_data << cipher.final
@serializer.load(decrypted_data)
- rescue OpenSSLCipherError, TypeError
+ rescue OpenSSLCipherError, TypeError, ArgumentError

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

Why? Needs an explanation and a test case demonstrating why it's needed.

@jeremy

jeremy May 15, 2013

Owner

Why? Needs an explanation and a test case demonstrating why it's needed.

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

strict_decode64 throws this, will add a test to demonstrate this.

@vipulnsward

vipulnsward May 15, 2013

Member

strict_decode64 throws this, will add a test to demonstrate this.

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

👍

+ @serializer.load(::Base64.strict_decode64(data))
+ rescue ArgumentError
+ raise InvalidSignature
+ end

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 15, 2013

Owner

Ditto. Why? Needs an explanation and a test case.

@jeremy

jeremy May 15, 2013

Owner

Ditto. Why? Needs an explanation and a test case.

Use `Base.strict_decode64` instead of `Base.decode64` just as we do i…
…n encoding;

Also reduce extra object allocation by creating string directly instead of join on Array
@vipulnsward

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward May 15, 2013

Member

@jeremy Updated

Member

vipulnsward commented May 15, 2013

@jeremy Updated

@vipulnsward

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward Dec 6, 2013

Member

@NZKoz @jeremy let me know your thoughts on this, to close if its undesired behaviour.

Member

vipulnsward commented Dec 6, 2013

@NZKoz @jeremy let me know your thoughts on this, to close if its undesired behaviour.

jeremy added a commit that referenced this pull request Dec 6, 2013

Merge pull request #10635 from vipulnsward/change_to_strict
Use `Base.strict_decode64` instead of `Base.decode64`

@jeremy jeremy merged commit 8ef1ef1 into rails:master Dec 6, 2013

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Dec 6, 2013

Owner

👍 since we've always used strict encoding.

Owner

jeremy commented Dec 6, 2013

👍 since we've always used strict encoding.

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Dec 7, 2013

Member

This commit has has got some failure for Action Pack.

rails/actionpack (master)> /Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/bin/ruby -w -Ilib:test test/dispatch/session/cookie_store_test.rb                           ruby-2.0.0-p247
/Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/openssl/ssl.rb:101: warning: assigned but unused variable - id
Run options: --seed 54594

# Running:

..FF...................

Finished in 0.144534s, 159.1321 runs/s, 435.8836 assertions/s.

  1) Failure:
CookieStoreTest#test_deserializes_unloaded_classes_on_get_id [test/dispatch/session/cookie_store_test.rb:144]:
should auto-load unloaded class.
--- expected
+++ actual
@@ -1 +1 @@
-"id: ce8b0752a6ab7c7af3cdb8a80e6b9e46"
+"id: "



  2) Failure:
CookieStoreTest#test_deserializes_unloaded_classes_on_get_value [test/dispatch/session/cookie_store_test.rb:155]:
should auto-load unloaded class.
--- expected
+++ actual
@@ -1 +1 @@
-"foo: #<SessionAutoloadTest::Foo bar:\"baz\">"
+"foo: nil"

@vipulnsward can you please check?

Member

arunagw commented Dec 7, 2013

This commit has has got some failure for Action Pack.

rails/actionpack (master)> /Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/bin/ruby -w -Ilib:test test/dispatch/session/cookie_store_test.rb                           ruby-2.0.0-p247
/Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/openssl/ssl.rb:101: warning: assigned but unused variable - id
Run options: --seed 54594

# Running:

..FF...................

Finished in 0.144534s, 159.1321 runs/s, 435.8836 assertions/s.

  1) Failure:
CookieStoreTest#test_deserializes_unloaded_classes_on_get_id [test/dispatch/session/cookie_store_test.rb:144]:
should auto-load unloaded class.
--- expected
+++ actual
@@ -1 +1 @@
-"id: ce8b0752a6ab7c7af3cdb8a80e6b9e46"
+"id: "



  2) Failure:
CookieStoreTest#test_deserializes_unloaded_classes_on_get_value [test/dispatch/session/cookie_store_test.rb:155]:
should auto-load unloaded class.
--- expected
+++ actual
@@ -1 +1 @@
-"foo: #<SessionAutoloadTest::Foo bar:\"baz\">"
+"foo: nil"

@vipulnsward can you please check?

@vipulnsward

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward Dec 7, 2013

Member

Checking.

On Saturday, December 7, 2013, Arun Agrawal wrote:

This commit has has got some failure for Action Pack.

rails/actionpack (master)> /Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/bin/ruby -w -Ilib:test test/dispatch/sessio
n
/cookie_store_test.rb ruby-2.0.0-p247/Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/openssl/ssl.rb:101: warning: assigned but
unused variable - idRun options: --seed 54594

Running:

..FF...................
Finished in 0.144534s, 159.1321 runs/s, 435.8836 assertions/s.

  1. Failure:CookieStoreTest#test_deserializes_unloaded_classes_on_get_id [test/dispatch/session/cookie_store_test.rb:144]:should auto-load unloaded class.--- expected+++ actual@@ -1 +1 @@-"id: ce8b0752a6ab7c7af3cdb8a80e6b9e46"+"id: "

  2. Failure:CookieStoreTest#test_deserializes_unloaded_classes_on_get_value [test/dispatch/session/cookie_store_test.rb:155]:should auto-load unloaded class.--- expected+++ actual@@ -1 +1 @@-"foo: #<SessionAutoloadTest::Foo bar:"baz">"+"foo: nil"

@vipulnsward https://github.com/vipulnsward can you please check?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/10635#issuecomment-30050508
.

Vipul A.M.
+91-8149-204995

Member

vipulnsward commented Dec 7, 2013

Checking.

On Saturday, December 7, 2013, Arun Agrawal wrote:

This commit has has got some failure for Action Pack.

rails/actionpack (master)> /Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/bin/ruby -w -Ilib:test test/dispatch/sessio
n
/cookie_store_test.rb ruby-2.0.0-p247/Users/arunagw/.rvm/rubies/ruby-2.0.0-p247/lib/ruby/2.0.0/openssl/ssl.rb:101: warning: assigned but
unused variable - idRun options: --seed 54594

Running:

..FF...................
Finished in 0.144534s, 159.1321 runs/s, 435.8836 assertions/s.

  1. Failure:CookieStoreTest#test_deserializes_unloaded_classes_on_get_id [test/dispatch/session/cookie_store_test.rb:144]:should auto-load unloaded class.--- expected+++ actual@@ -1 +1 @@-"id: ce8b0752a6ab7c7af3cdb8a80e6b9e46"+"id: "

  2. Failure:CookieStoreTest#test_deserializes_unloaded_classes_on_get_value [test/dispatch/session/cookie_store_test.rb:155]:should auto-load unloaded class.--- expected+++ actual@@ -1 +1 @@-"foo: #<SessionAutoloadTest::Foo bar:"baz">"+"foo: nil"

@vipulnsward https://github.com/vipulnsward can you please check?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/10635#issuecomment-30050508
.

Vipul A.M.
+91-8149-204995

@vipulnsward

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward Dec 7, 2013

Member

Strange why the class it isn't being autoloaded. Investigating more.

Member

vipulnsward commented Dec 7, 2013

Strange why the class it isn't being autoloaded. Investigating more.

vipulnsward added a commit to vipulnsward/rails that referenced this pull request Dec 12, 2013

PR #10635 introduces rescue from ArgumentError thrown by `Base64.stri…
…ct_decode64`.

This broke natural order of things for `StaleSessionCheck#stale_session_check!` which tried auto_loading a class based on `ArgumentError` message , and later retrying the `Marshal#load` of class, successfully allowing auto_loading.

  This PR tries to fix this behavior by forwarding `ArgumentError` 's not raised  by `Base64.strict_decode64` , as is, ahead to `StaleSessionCheck#stale_session_check!`

rafaelfranca added a commit that referenced this pull request Dec 12, 2013

@vipulnsward vipulnsward deleted the vipulnsward:change_to_strict branch Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment