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

Add option to set the session middleware when enabling sessions #1161

Merged
merged 4 commits into from Aug 9, 2016

Conversation

Projects
None yet
2 participants
@jkowens
Member

jkowens commented Aug 4, 2016

By setting the session middleware as an option, rack protection for
sessions will be enabled by default and other session settings will be
applied to the middleware. Fixes #1038.

Example:

 set :sessions, :session_store => Rack::Session::Pool, :expire_after => 2592000
Add option to set the session middleware when enabling sessions
By setting the session middleware as an option, rack protection for
sessions will be enabled by default and other session settings will be
applied to the middleware.
Show outdated Hide outdated README.md
```ruby
use Rack::Session::Pool, :expire_after => 2592000

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

If the use example still works I'd like to show both (since both should be supported).

@zzak

zzak Aug 4, 2016

Member

If the use example still works I'd like to show both (since both should be supported).

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

Good call 👍

@jkowens

jkowens Aug 4, 2016

Member

Good call 👍

Show outdated Hide outdated README.md
use Rack::Session::Pool
set :protection, :session => true
```
has been enabled.

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

nitpick "have been enabled".

Also, this is still true if you use the middleware option.

I don't want to ignore that use-case as many applications probably are doing it that way, and likely read it from here.

We should consider the README a kind of spec, because many applications rely on the behavior they learned from it, so we can't simply remove it without notice.

IMO this would mean having different sections under the "Using Sessions" chapter. Likely, we would start out with enable :session, like we do here including the note about defaulting to a cookie store. Followed by a section on "Choosing your own session middleware", which outlines both use-cases you've identified above.

As well, the section below this "Available settings" might also need to be updated wrt the "sessions" option. WDYT?

tl;dr I'm happy to support both cases independently.

@zzak

zzak Aug 4, 2016

Member

nitpick "have been enabled".

Also, this is still true if you use the middleware option.

I don't want to ignore that use-case as many applications probably are doing it that way, and likely read it from here.

We should consider the README a kind of spec, because many applications rely on the behavior they learned from it, so we can't simply remove it without notice.

IMO this would mean having different sections under the "Using Sessions" chapter. Likely, we would start out with enable :session, like we do here including the note about defaulting to a cookie store. Followed by a section on "Choosing your own session middleware", which outlines both use-cases you've identified above.

As well, the section below this "Available settings" might also need to be updated wrt the "sessions" option. WDYT?

tl;dr I'm happy to support both cases independently.

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

Sounds good. To enable session based protection using the middleware option the following code would need to be used though (as described in #1038):

 disable :protection
 use Rack::Session::Pool
 use Rack::Protection

or

use Rack::Session::Pool
use ::Rack::Protection::RemoteToken
use ::Rack::Protection::SessionHijacking

The code sample used before was incorrect, it didn't work.

@jkowens

jkowens Aug 4, 2016

Member

Sounds good. To enable session based protection using the middleware option the following code would need to be used though (as described in #1038):

 disable :protection
 use Rack::Session::Pool
 use Rack::Protection

or

use Rack::Session::Pool
use ::Rack::Protection::RemoteToken
use ::Rack::Protection::SessionHijacking

The code sample used before was incorrect, it didn't work.

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

How about to provide a regression test (if there aren't any) to ensure this behavior works?

@zzak

zzak Aug 4, 2016

Member

How about to provide a regression test (if there aren't any) to ensure this behavior works?

Show outdated Hide outdated lib/sinatra/base.rb
@@ -1709,7 +1709,8 @@ def setup_sessions(builder)
options = {}
options[:secret] = session_secret if session_secret?
options.merge! sessions.to_hash if sessions.respond_to? :to_hash
builder.use Rack::Session::Cookie, options
session_store = options.delete(:session_store) { Rack::Session::Cookie }

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Should we be concerned about mutating the options hash at this point?

@zzak

zzak Aug 4, 2016

Member

Should we be concerned about mutating the options hash at this point?

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

I don't think I see an issue with it, since the original sessions hash is preserved.

@jkowens

jkowens Aug 4, 2016

Member

I don't think I see an issue with it, since the original sessions hash is preserved.

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 4, 2016

Member

@zzak I think I've just thought of a better way to handle setting a custom session middleware. What do you think about this:

enable :sessions
# override the default which is Rack::Session::Cookie
set :session_store, Rack::Session::Pool

If someone needed to set an option on the session middleware they could do this:

# enable sessions and set option hash to be passed to session middleware
set :sessions, :expire_after => 2592000
set :session_store, Rack::Session::Pool
Member

jkowens commented Aug 4, 2016

@zzak I think I've just thought of a better way to handle setting a custom session middleware. What do you think about this:

enable :sessions
# override the default which is Rack::Session::Cookie
set :session_store, Rack::Session::Pool

If someone needed to set an option on the session middleware they could do this:

# enable sessions and set option hash to be passed to session middleware
set :sessions, :expire_after => 2592000
set :session_store, Rack::Session::Pool
@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 4, 2016

Member

@jkowens I think you're on the right track, let me know what you come up with after some polish :)

Member

zzak commented Aug 4, 2016

@jkowens I think you're on the right track, let me know what you come up with after some polish :)

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 4, 2016

Member

@zzak ok I think I'm ready for you to give it another look. Hopefully the documentation is up to snuff. Thanks!

Member

jkowens commented Aug 4, 2016

@zzak ok I think I'm ready for you to give it another look. Hopefully the documentation is up to snuff. Thanks!

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 4, 2016

Member

I will squash my commits once everything is good.

Member

jkowens commented Aug 4, 2016

I will squash my commits once everything is good.

Show outdated Hide outdated README.md
set :session_store, Rack::Session::Pool
```
Or to enable sessions with a hash of options:

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

I think using "enable" here is slightly confusing because enable is a method too, mentioned above

@zzak

zzak Aug 4, 2016

Member

I think using "enable" here is slightly confusing because enable is a method too, mentioned above

Show outdated Hide outdated README.md
use Rack::Session::Pool, :expire_after => 2592000
```
It is important to note that when using this method, session based protection (see 'Configuring attack protection') will not be enabled by default. The Rack middleware to do that will also need to be added:

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Let's make "will not be enabled by default" in bold, so they really get the hint!

After that, we could mention calling setup_protection manually (although I'm not sure if this is private, or even a good idea ;p )

@zzak

zzak Aug 4, 2016

Member

Let's make "will not be enabled by default" in bold, so they really get the hint!

After that, we could mention calling setup_protection manually (although I'm not sure if this is private, or even a good idea ;p )

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

I'm not sure I would mention setup_protection. It could be called but to avoid duplicate middleware being added it would have to set up like this:

disable :protection
use Rack::Session::Pool
setup_protection(self)

That would be the same as:

disable :protection
use Rack::Session::Pool
use Rack::Protection

Would you like me to also mention one of those methods?

@jkowens

jkowens Aug 4, 2016

Member

I'm not sure I would mention setup_protection. It could be called but to avoid duplicate middleware being added it would have to set up like this:

disable :protection
use Rack::Session::Pool
setup_protection(self)

That would be the same as:

disable :protection
use Rack::Session::Pool
use Rack::Protection

Would you like me to also mention one of those methods?

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Well setup_protection does quite a bit more, but my guess is since it is actually private then we should avoid mentioning it in public docs.

Scrap that idea, and just update the font weight if you would, please <3

@zzak

zzak Aug 4, 2016

Member

Well setup_protection does quite a bit more, but my guess is since it is actually private then we should avoid mentioning it in public docs.

Scrap that idea, and just update the font weight if you would, please <3

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Also as per 86cae2d, sessions must be enabled in order for SessionHijacking and RemoteToken middleware to be activated.

We should probably add a test for this too

@zzak

zzak Aug 4, 2016

Member

Also as per 86cae2d, sessions must be enabled in order for SessionHijacking and RemoteToken middleware to be activated.

We should probably add a test for this too

This comment has been minimized.

@jkowens

jkowens Aug 5, 2016

Member

I think that is what this test is supposed to verify.

@jkowens

jkowens Aug 5, 2016

Member

I think that is what this test is supposed to verify.

This comment has been minimized.

@zzak

zzak Aug 5, 2016

Member

Ah, so we do have a test for this behavior too:
https://github.com/sinatra/sinatra/blob/master/test/settings_test.rb#L575

How about adding one which uses SessionPool?

@zzak

zzak Aug 5, 2016

Member

Ah, so we do have a test for this behavior too:
https://github.com/sinatra/sinatra/blob/master/test/settings_test.rb#L575

How about adding one which uses SessionPool?

This comment has been minimized.

@zzak

zzak Aug 5, 2016

Member

@jkowens If that is correct, then the original example below should still be right:

use Rack::Session::Pool
set :protection, :session => true

Yes? The only thing missing is setting a custom session middleware, as mentioned above.

@zzak

zzak Aug 5, 2016

Member

@jkowens If that is correct, then the original example below should still be right:

use Rack::Session::Pool
set :protection, :session => true

Yes? The only thing missing is setting a custom session middleware, as mentioned above.

This comment has been minimized.

@jkowens

jkowens Aug 6, 2016

Member

The reason that doesn't work is that set :protection, :session => true loads Rack::Protection::RemoteToken and Rack::Protection::SessionHijacking earlier in the middleware chain than Rack::Session::Pool. A session won't be available when RemoteToken checks so a RuntimeError is raised when a request is made.

I haven't been able to think of a reason calling set :protection, :session => true would be useful?

@jkowens

jkowens Aug 6, 2016

Member

The reason that doesn't work is that set :protection, :session => true loads Rack::Protection::RemoteToken and Rack::Protection::SessionHijacking earlier in the middleware chain than Rack::Session::Pool. A session won't be available when RemoteToken checks so a RuntimeError is raised when a request is made.

I haven't been able to think of a reason calling set :protection, :session => true would be useful?

This comment has been minimized.

@zzak

zzak Aug 6, 2016

Member

But that's exactly what this test does:

     it 'sets up RemoteToken if it is configured to' do
       MiddlewareTracker.track do
         Sinatra.new { set :protection, :session => true }.new
         assert_include MiddlewareTracker.used, Rack::Protection::RemoteToken
       end
    end

The only difference, is not using custom session middleware. But that is what the documentation says you can do, so it must be right :trollface:

@zzak

zzak Aug 6, 2016

Member

But that's exactly what this test does:

     it 'sets up RemoteToken if it is configured to' do
       MiddlewareTracker.track do
         Sinatra.new { set :protection, :session => true }.new
         assert_include MiddlewareTracker.used, Rack::Protection::RemoteToken
       end
    end

The only difference, is not using custom session middleware. But that is what the documentation says you can do, so it must be right :trollface:

This comment has been minimized.

@jkowens

jkowens Aug 6, 2016

Member

Haha right. So we could change that test to use Rack::Session::Pool and it would pass, but in reality when a request is made it errors out.

@jkowens

jkowens Aug 6, 2016

Member

Haha right. So we could change that test to use Rack::Session::Pool and it would pass, but in reality when a request is made it errors out.

Show outdated Hide outdated README.md
use Rack::Session::Pool
set :protection, :session => true
```
have been enabled. See 'Using Sessions'.

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Like I was saying before, if this example doesn't work I'd like to add a regression test if you're up for it!

@zzak

zzak Aug 4, 2016

Member

Like I was saying before, if this example doesn't work I'd like to add a regression test if you're up for it!

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

I was thinking about this. I'm not sure what a good regression test would be? A test that expects that example to raise a RuntimeError?

@jkowens

jkowens Aug 4, 2016

Member

I was thinking about this. I'm not sure what a good regression test would be? A test that expects that example to raise a RuntimeError?

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Well my point was, this example was in the docs so it should have worked, unless something broke it and was missed because there wasn't a proper test covering it. :(

@zzak

zzak Aug 4, 2016

Member

Well my point was, this example was in the docs so it should have worked, unless something broke it and was missed because there wasn't a proper test covering it. :(

This comment has been minimized.

@jkowens

jkowens Aug 4, 2016

Member

Oh I see what you mean. That's the weird thing, I can't figure out how that example code would have ever did what it claimed to do!

@jkowens

jkowens Aug 4, 2016

Member

Oh I see what you mean. That's the weird thing, I can't figure out how that example code would have ever did what it claimed to do!

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

It could be assuming that enable :sessions was already called, but I can't tell.

If there is no equivalent test, then we broke the unspoken spec that is our README :(
When I say "we" it was probably me.

@zzak

zzak Aug 4, 2016

Member

It could be assuming that enable :sessions was already called, but I can't tell.

If there is no equivalent test, then we broke the unspoken spec that is our README :(
When I say "we" it was probably me.

Show outdated Hide outdated README.md
```ruby
use Rack::Session::Pool, :expire_after => 2592000
```

This comment has been minimized.

@zzak

zzak Aug 4, 2016

Member

Let's just remove this example all together, since it can actually lead to vulnerability.

@zzak

zzak Aug 4, 2016

Member

Let's just remove this example all together, since it can actually lead to vulnerability.

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 8, 2016

Member

Just for reference: Issue #601 was closed by 5aa1c7c, but in issue #757, it turns out it didn't really address the problem. #1038 is really just the same issue being brought up again.

Member

jkowens commented Aug 8, 2016

Just for reference: Issue #601 was closed by 5aa1c7c, but in issue #757, it turns out it didn't really address the problem. #1038 is really just the same issue being brought up again.

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 8, 2016

Member

@zzak after finding #757, I understand the purpose of set :protection, :session => true and updated the readme.

Member

jkowens commented Aug 8, 2016

@zzak after finding #757, I understand the purpose of set :protection, :session => true and updated the readme.

@zzak zzak merged commit 4797c02 into sinatra:master Aug 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Aug 9, 2016

Member

@jkowens LGTM thanks for your patience and contribution!

Member

zzak commented Aug 9, 2016

@jkowens LGTM thanks for your patience and contribution!

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Aug 9, 2016

Member

Cool, thanks for your help 😄

Member

jkowens commented Aug 9, 2016

Cool, thanks for your help 😄

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