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 `constraint_to` option to SSL middleware #22591

Merged
merged 1 commit into from Feb 28, 2016

Conversation

Projects
None yet
8 participants
@gregmolnar
Member

gregmolnar commented Dec 14, 2015

The force_ssl option redirects each request to https, but it would be great to be able to exclude a path from the redirection. My use-case would be when the app is behind a load balancer, I'd like to be able to do an http health check against the app.
If you think this is reasonable, I will add a changelog entry and add this to the guides.

@rails-bot

This comment has been minimized.

rails-bot commented Dec 14, 2015

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch Dec 15, 2015

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Dec 26, 2015

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 31, 2015

It is reasonable to me, but I want to hear what @jeremy and @matthewd think about it.

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch 3 times, most recently Jan 20, 2016

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Jan 20, 2016

@rafaelfranca I rebased this against master and moved the setting under redirect.
//cc @jeremy @matthewd

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Feb 18, 2016

Do you have any news on this @rafaelfranca ?

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 18, 2016

Would prefer something like yielding the request to a block that decides whether to redirect. Exclude-paths is one of many behaviors that could proliferate here. Leave that up to apps to decide.

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Feb 19, 2016

@jeremy Could you please give me some guidelines on how should I implement that?

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 24, 2016

Could add a constrain_to block to the redirect options:

config.ssl_options.redirect = { port: 1234, constrain_to: -> request { request.path !~ /healthcheck/ }

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch Feb 28, 2016

@gregmolnar gregmolnar changed the title from add excluded_paths option to ssl middleware to add `constraint_to` option to SSL middleware Feb 28, 2016

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Feb 28, 2016

@kaspth thanks for the tip, I did it that way. @jeremy what do you think of this implementation?

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
return redirect_to_https request if @redirect
if @redirect && (@redirect[:constrain_to].nil? || @redirect[:constrain_to].call(request))
return redirect_to_https request
end

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

Could remove the nil check here with return redirect_to_https request if @constrain_to.call(request) and adding @constrain_to = @redirect && @redirect[:constrain_to] || proc { @redirect } after @redirect has been assigned to something.

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
@@ -34,6 +34,8 @@ module ActionDispatch
# original HSTS directive until it expires. Instead, use the header to tell browsers to
# expire HSTS immediately. Setting `hsts: false` is a shortcut for
# `hsts: { expires: 0 }`.
# To exclude a path from redirection set the `excluded_paths` options:
# `redirect: { excluded_paths: ['/healtcheck'] }`

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

Outdated docs. Would write:

# Redirection can be constrained to only whitelisted requests with `constrain_to`:
#
#    config.ssl_options = { redirect: { constrain_to: -> request { request.path !~ /healthcheck/ } } }

This comment has been minimized.

@gregmolnar

gregmolnar Feb 28, 2016

Member

Well spotted, thanks!

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch 2 times, most recently Feb 28, 2016

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
# config.ssl_options = { redirect: { constrain_to: -> request { request.path !~ /healthcheck/ } } }
#
# If set, requests are only redirected if the `constrain_to` block returns true.

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

I ended up updating the suggested documentation a few times, can you use the latest revision?

(We also don't need the blank line after the documentation comments.)

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
# return redirect_to_https request
# end
=======
>>>>>>> Stashed changes

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

There's a merge conflict ☝️

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch Feb 28, 2016

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/ssl.rb Outdated
@app.call(env)
end

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

We're gonna need this 😁

This comment has been minimized.

@gregmolnar

gregmolnar Feb 28, 2016

Member

This was an accidental push. I had some merge conflicts and messed up when I removed the git merge diff from the file.

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch 2 times, most recently Feb 28, 2016

@kaspth

View changes

actionpack/test/dispatch/ssl_test.rb Outdated
get 'http://example.org/healthcheck'
assert_response 200
get 'http://example.org/'
assert_redirected_to 'https://example.org/'

This comment has been minimized.

@kaspth

kaspth Feb 28, 2016

Member

We should use the custom built assertions from just above:

test 'constrain to can avoid redirect' do
  constraining = { constrain_to: -> request { request.path !~ /healthcheck/ } }

  assert_not_redirected 'http://example.org/healthcheck', redirect: constraining
  assert_redirected from: 'https://example.org/', redirect: constraining
end

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch Feb 28, 2016

@gregmolnar gregmolnar force-pushed the gregmolnar:ssl branch to 97b9e32 Feb 28, 2016

kaspth added a commit that referenced this pull request Feb 28, 2016

Merge pull request #22591 from gregmolnar/ssl
add `constraint_to` option to SSL middleware

@kaspth kaspth merged commit caa6fb3 into rails:master Feb 28, 2016

1 check passed

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

This comment has been minimized.

Member

kaspth commented Feb 28, 2016

Nice! 👍

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Feb 29, 2016

@kaspth thanks for the review and the help!

@halo

This comment has been minimized.

halo commented Mar 4, 2016

Exactly what I needed. Great work, thank you.

@trevorturk

This comment has been minimized.

Contributor

trevorturk commented Mar 7, 2016

Thank you! I was just wishing for this feature a few weeks ago. Nice work 😁

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Mar 7, 2016

@halo @trevorturk I am glad I am not the only one who uses this feature :)

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