Permalink
Browse files

Revert "join the remaining instead of reversing"

This reverts commit 9a4700f.
  • Loading branch information...
spastorino committed Dec 6, 2013
1 parent 9a4700f commit 734a00c5f4bb46e9a5e6e2677d89a2f285dcc185
Showing with 3 additions and 2 deletions.
  1. +3 −2 lib/rack/session/cookie.rb
@@ -135,8 +135,9 @@ def unpacked_cookie_data(env)
session_data = request.cookies[@key]
if @secrets.size > 0 && session_data
- *data, digest = session_data.split("--")
- session_data = data.join("--")
+ digest, session_data = session_data.reverse.split("--", 2)
+ digest.reverse! if digest
+ session_data.reverse! if session_data
session_data = nil unless digest_match?(session_data, digest)
end

6 comments on commit 734a00c

@tonytonyjan

This comment has been minimized.

Show comment Hide comment
@tonytonyjan

tonytonyjan Dec 7, 2016

Contributor

@spastorino any reason to this reverting? I think the reverse way is more difficult to either read or figure out the purpose without any comment.

Contributor

tonytonyjan replied Dec 7, 2016

@spastorino any reason to this reverting? I think the reverse way is more difficult to either read or figure out the purpose without any comment.

@spastorino

This comment has been minimized.

Show comment Hide comment
@spastorino

spastorino Dec 10, 2016

Member

I don't remember please do investigate about it :).

Member

spastorino replied Dec 10, 2016

I don't remember please do investigate about it :).

@tonytonyjan

This comment has been minimized.

Show comment Hide comment
@tonytonyjan

tonytonyjan Dec 11, 2016

Contributor
@mikeycgto

This comment has been minimized.

Show comment Hide comment
@mikeycgto

mikeycgto Jun 4, 2017

Since the digest is the at the end of the session_data string and is a fixed length (depending on the hash function used in the HMAC), can we just slice out N characters from the end of the string and treat the remaining string as the session data. I updated the benchmark for this here: https://gist.github.com/mikeycgto/0339162c97ffd4257d674c0bfb71e37f. Seems this "slice" approach is a lot faster performance wise.

I am working on introducing AES GCM encryption to session cookies for Sinatra and Rack (see issue sinatra/sinatra#1300) and stumbled upon this code and was confused by the double reversing.

Would be happy to make a PR to change this code to use the "slice" approach. I plan on making my AES GCM PR for Rack session cookies able to seamlessly upgrade existing cookies to the new scheme (similiar to the approach in my PR for Rails here rails/rails#28132). I can alternatively include this change in that PR. Thoughts?

Since the digest is the at the end of the session_data string and is a fixed length (depending on the hash function used in the HMAC), can we just slice out N characters from the end of the string and treat the remaining string as the session data. I updated the benchmark for this here: https://gist.github.com/mikeycgto/0339162c97ffd4257d674c0bfb71e37f. Seems this "slice" approach is a lot faster performance wise.

I am working on introducing AES GCM encryption to session cookies for Sinatra and Rack (see issue sinatra/sinatra#1300) and stumbled upon this code and was confused by the double reversing.

Would be happy to make a PR to change this code to use the "slice" approach. I plan on making my AES GCM PR for Rack session cookies able to seamlessly upgrade existing cookies to the new scheme (similiar to the approach in my PR for Rails here rails/rails#28132). I can alternatively include this change in that PR. Thoughts?

@spastorino

This comment has been minimized.

Show comment Hide comment
@spastorino

spastorino Jun 4, 2017

Member

@mikeycgto I'd say is better if you make a PR and explain all this :)

Member

spastorino replied Jun 4, 2017

@mikeycgto I'd say is better if you make a PR and explain all this :)

@mikeycgto

This comment has been minimized.

Show comment Hide comment
@mikeycgto

mikeycgto Jun 4, 2017

I figured as much so I made rack/rack#1173. I can add to the explanation there if need be. Simple enough of a change though and definitely a considerable performance boost. Thanks for the quick feedback.

mikeycgto replied Jun 4, 2017

I figured as much so I made rack/rack#1173. I can add to the explanation there if need be. Simple enough of a change though and definitely a considerable performance boost. Thanks for the quick feedback.

Please sign in to comment.