Skip to content
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

Deprecate controller level force_ssl #32277

Merged
merged 1 commit into from Mar 30, 2018

Conversation

Projects
None yet
@derekprior
Copy link
Contributor

derekprior commented Mar 17, 2018

Today there are two common ways for Rails developers to force their
applications to communicate over HTTPS:

  • config.force_ssl is a setting in environment configurations that
    enables the ActionDispatch::SSL middleware. With this middleware
    enabled, all HTTP communication to your application will be redirected
    to HTTPS. The middleware also takes care of other best practices by
    setting HSTS headers, upgrading all cookies to secure only, etc.
  • The force_ssl controller method redirects HTTP requests to certain
    controllers to HTTPS.

As a consultant, I've seen many applications with misconfigured HTTPS
setups due to developers adding force_ssl to ApplicationController
and not enabling config.force_ssl. With this configuration, many
application requests can be served over HTTP such as assets, requests
that hit mounted engines, etc. In addition, because cookies are not
upgraded to secure only in this configuration and HSTS headers are not
set, it's possible for cookies that are meant to be secure to be sent
over HTTP.

The confusion between these two methods of forcing HTTPS is compounded
by the fact that they share an identical name. This makes finding
documentation on the "right" method confusing.

HTTPS throughout is quickly becomming table stakes for all web sites.
Sites are expected to operate over HTTPS for all communication,
sensitive or otherwise. Let's encourage use of the broader-reaching
ActionDispatch::SSL middleware and elminate this source of user
confusion. If, for some reason, applications need to expose certain
endpoints over HTTP they can do so by properly configuring
config.ssl_options.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Mar 17, 2018

r? @sgrif

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

guides/source/upgrading_ruby_on_rails.md Outdated
### Force SSL

The `force_ssl` method on controllers has been deprecated and will be removed in
Rails 6.0. you are encouraged to enable `config.force_ssl` to enforce HTTPS

This comment has been minimized.

@composerinteralia

composerinteralia Mar 18, 2018

Member

capital 'Y' in 'You'

Unrelated: I enjoy your show 🚲 🔔 and I am looking forward to your talk at RailsConf.

This comment has been minimized.

@sikachu

sikachu Mar 18, 2018

Member

Another place here re: 6.0 -> 6.1.

This comment has been minimized.

@sikachu

sikachu Mar 28, 2018

Member

One more thing — Look like "You" in "you are encouraged ..." has to be capitalized.

@sikachu
Copy link
Member

sikachu left a comment

I have a few comments. Would love to @fxn's or @pixeltrix thought about the documentation aspect of this though. 🙇

Show resolved Hide resolved actionpack/lib/action_controller/metal/force_ssl.rb Outdated
Show resolved Hide resolved actionpack/lib/action_controller/metal/force_ssl.rb Outdated
guides/source/upgrading_ruby_on_rails.md Outdated
### Force SSL

The `force_ssl` method on controllers has been deprecated and will be removed in
Rails 6.0. you are encouraged to enable `config.force_ssl` to enforce HTTPS

This comment has been minimized.

@sikachu

sikachu Mar 18, 2018

Member

Another place here re: 6.0 -> 6.1.

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Mar 19, 2018

@derekprior not 100% convinced of the misconfiguration argument - it's not something I've personally seen in the past few years on a variety of Rails apps and there's been one occurrence on a high-profile website I support where adding config.force_ssl = true broke other parts of the site due to the domain wide nature of HSTS headers. I think there's a time when this will be appropriate but I'm unsure we want to remove the flexibility that doing it in the controller provides whilst sites are still transitioning to HTTPS.

However it's not a hill I willing to die on if other Rails maintainers feel differently.

@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Mar 19, 2018

@pixeltrix I should have been more specific when I said "many apps". I've seen it 4 times that I remember in the last few years of projects (3~6/year). Most recently, I submitted a PR to a project to switch to config.force_ssl and was told of incompatibilities in one aspect of the application the last time someone tried this (similar to what you cite). I think the better response to those incompatibilities is proper configuration of config.ssl_options until such time the incompatibilities can be remedied.

That said, I certainly expected some debate of this PR. I can see your points as well. I just think the expectation is that applications work entirely over HTTPS these days and that Rails should optimize for that. I feel like I'd be less likely to object to the controller-level construct if it wasn't identically named to the middleware level configuration.

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Mar 19, 2018

I think the better response to those incompatibilities is proper configuration of config.ssl_options until such time the incompatibilities can be remedied.

The problem is that not everything is possible through config.force_ssl - we initially tried that in the scenario I cite and then had to drop back down to the controller level.

Maybe this is fine given the timeframe - it's not going to be removed until 6.1 which will likely be at least 18-24 months away.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Mar 28, 2018

I'm 100% in favor of this change (other than the version changes), but I will defer to @pixeltrix on this.

r? @pixeltrix

@rails-bot rails-bot assigned pixeltrix and unassigned sgrif Mar 28, 2018

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Mar 28, 2018

Giving the timing I'm fine with deprecating this feature in Rails 6.0 too.

@jeremy care to review this too given you did the last improvement on the SSL middleware?

@jeremy

jeremy approved these changes Mar 28, 2018

Copy link
Member

jeremy left a comment

👏

@sikachu

This comment has been minimized.

Copy link
Member

sikachu commented Mar 28, 2018

@derekprior would you mind fixing the documentation issue? We should be able to merge it in afterward.

@derekprior derekprior force-pushed the derekprior:dp-deprecate-force-ssl branch Mar 29, 2018

@derekprior

This comment has been minimized.

Copy link
Contributor Author

derekprior commented Mar 29, 2018

@sikachu Docs updated. I added :nodoc: as requested but also left the comments I had left on the module for future code-spelunkers. Let me know if I should just remove that.

@sikachu
Copy link
Member

sikachu left a comment

Yep, that's fine! I want to leave the comment there for original code spelunker as well, just want it to disappear from the doc.

It looks like you need to do :nodoc: at the end of the class instead. Do you mind checking my diff?

actionpack/lib/action_controller/metal/force_ssl.rb Outdated
# transferred safely over the internet. You _should_ always force the browser
# to use HTTPS when you're transferring sensitive information such as
# user authentication, account information, or credit card information.
# :nodoc:

This comment has been minimized.

@sikachu

sikachu Mar 30, 2018

Member

I tried this, and it actually didn't remove this from the documentation. Looks like you need to do explicit :nodoc: next to each class:

diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb
index 4b54b4a19e..b73d54edb3 100644
--- a/actionpack/lib/action_controller/metal/force_ssl.rb
+++ b/actionpack/lib/action_controller/metal/force_ssl.rb
@@ -4,12 +4,11 @@
 require "active_support/core_ext/hash/slice"
 
 module ActionController
-  # :nodoc:
   # This module is deprecated in favor of +config.force_ssl+ in your environment
   # config file. This will ensure all communication to non-whitelisted endpoints
   # served by your application occurs over HTTPS.
   #
-  module ForceSSL
+  module ForceSSL # :nodoc:
     extend ActiveSupport::Concern
     include AbstractController::Callbacks
 
@@ -17,7 +16,7 @@ module ForceSSL
     URL_OPTIONS = [:protocol, :host, :domain, :subdomain, :port, :path]
     REDIRECT_OPTIONS = [:status, :flash, :alert, :notice]
 
-    module ClassMethods
+    module ClassMethods # :nodoc:
       def force_ssl(options = {})
         ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
           Controller-level `force_ssl` is deprecated and will be removed from

This comment has been minimized.

@derekprior

derekprior Mar 30, 2018

Author Contributor

Thanks, updated.

Deprecate controller level force_ssl
Today there are two common ways for Rails developers to force their
applications to communicate over HTTPS:

* `config.force_ssl` is a setting in environment configurations that
  enables the `ActionDispatch::SSL` middleware. With this middleware
  enabled, all HTTP communication to your application will be redirected
  to HTTPS. The middleware also takes care of other best practices by
  setting HSTS headers, upgrading all cookies to secure only, etc.
* The `force_ssl` controller method redirects HTTP requests to certain
  controllers to HTTPS.

As a consultant, I've seen many applications with misconfigured HTTPS
setups due to developers adding `force_ssl` to `ApplicationController`
and not enabling `config.force_ssl`. With this configuration, many
application requests can be served over HTTP such as assets, requests
that hit mounted engines, etc. In addition, because cookies are not
upgraded to secure only in this configuration and HSTS headers are not
set, it's possible for cookies that are meant to be secure to be sent
over HTTP.

The confusion between these two methods of forcing HTTPS is compounded
by the fact that they share an identical name. This makes finding
documentation on the "right" method confusing.

HTTPS throughout is quickly becomming table stakes for all web sites.
Sites are expected to operate over HTTPS for all communication,
sensitive or otherwise. Let's encourage use of the broader-reaching
`ActionDispatch::SSL` middleware and elminate this source of user
confusion. If, for some reason, applications need to expose certain
endpoints over HTTP they can do so by properly configuring
`config.ssl_options`.

@derekprior derekprior force-pushed the derekprior:dp-deprecate-force-ssl branch to 4701a50 Mar 30, 2018

@sikachu
Copy link
Member

sikachu left a comment

🚀

@guilleiguaran guilleiguaran merged commit c680080 into rails:master Mar 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate All good!
Details
@frenkel

This comment has been minimized.

Copy link
Contributor

frenkel commented Mar 6, 2019

Anybody have an idea how to allow Google Load Balancer health check with config.force_ssl? They always connect over HTTP and will thus get a redirect. Currently we can work around this by using force_ssl in the ApplicationController.

@yskkin

This comment has been minimized.

Copy link
Contributor

yskkin commented Mar 6, 2019

I use this for AWS's ALB

  config.force_ssl = true
  config.ssl_options = {
    redirect: {
      exclude: ->(request) { request.env["HTTP_X_FORWARDED_FOR"].blank? }
    }
  }
@frenkel

This comment has been minimized.

Copy link
Contributor

frenkel commented Mar 6, 2019

Great, thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.