Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SSL should not be disabled by default in any environment. #5023

Closed
wants to merge 1 commit into from

6 participants

@pat

Disabling SSL in any environment should be a decision made by each individual developer/team - really not keen on Rails forcing this. Also, I think it's far better to have a consistent environment as much as possible across all environments. So, this patch removes the Rails.env.development? check in force_ssl.

As a related aside, I've just been working on a gem that makes it far easier to use SSL in development and (Capybara-driven) test environments. It's built with bartt's version of ssl_requirement in mind, but I'm keen to have it work with force_ssl as well (I didn't realise it existed until just now).
https://github.com/freelancing-god/thin-glazed

@pixeltrix
Owner

So how would the following work for those who want it disabled in development?

class PagesController < ActionController::Base
  force_ssl

  def index
    #do stuff
  end
end

would you'd do it something like this?

class PagesController < ActionController::Base
  force_ssl unless Rails.env.development?

  def index
    #do stuff
  end
end

I think defaulting force_ssl to working in development is wrong - most devs won't have SSL setup properly (and don't need to) so we'd be making extra work for them. However I think that disabling force_ssl in development completely is also wrong - perhaps a ssl_configured setting that defaults to false in development and true everywhere else?

@pat

I guess a setting is a reasonable compromise, although I'm loath to add further complexity to Rails. And really, if you're dealing with SSL but haven't tested it locally through a browser, then I'd be a little worried.

And yes, dealing with SSL in development isn't quite so simple, but that's what the gem I've built addresses - and I'm sure others have their own work-arounds - such as what you've written in your second example.

@jonleighton
Collaborator

I agree with adding a config option - people are going to want to turn this off for dev mode but it definitely shouldn't be hardcoded.

/cc @josevalim

@josevalim
Owner

Ok. So let's just add a CHANGELOG entry mentioning that force_ssl now runs in development and that people will likely want to do unless Rails.env.development? to work around it.

@pat

@josevalim I think @jonleighton is actually suggesting adding a configuration option in as well, so people can choose whether they want SSL disabled on an environment basis. The idealist in me would rather just have my commit merged, note it in the change log and that's it. The realist in me is happy with the configuration option.

@josevalim
Owner

I don't think we need a config option. We already have one app wise. I agree with the idealistic you.

@pat

Well, you're the one with commit rights, so I bow to your wisdom ;)

@josevalim
Owner
@pat

Not sure - that doesn't feel obvious.

@josevalim
Owner

Ah, just now I realize what Jon meant with a config option. I was thinking he wanted an option on config/application.rb to configure the environment, but I guess he meant an option for the environment direct to force_ssl.

I agree with the config option then, better than the :if I proposed above. Sorry for the confusion folks.

@jonleighton
Collaborator

Yeah I was talking about config/environments/development.rb containing:

config.action_controller.force_ssl = false

Seems better than having unless Rails.env.development? everywhere in your controllers....

@josevalim
Owner

Nah, I don't like it. We already have a config.force_ssl option that is application wise. Maybe force_ssl :env => [] or simply enable it in all environments with a CHANGELOG notice.

@jonleighton
Collaborator

Do you disagree with the principle of being able to turn off SSL on a per-environment basis, or just that I wrote config.action_controller.force_ssl rather than config.force_ssl? Could we put config.force_ssl = false in development.rb and have it do the right thing?

@pixeltrix
Owner

Everybody realizes that config.force_ssl and force_ssl in a controller are different right? The config option sets up Rack::SSL whereas the controller method does it's own redirect - wouldn't it be better to have config.ssl_configured which defaults to false in development.

@josevalim
Owner
@pixeltrix
Owner

Being able to specify an array of environments to config.force_ssl doesn't help.

Imagine the following scenario - you only want to force SSL for a specific controller/action but don't have SSL configured in your development environment. This currently works out-of-the-box - if we merge this request then you'd have to add the unless Rails.env.development? in every controller you use force_ssl (ignore for the moment that it's at class level and won't work if you're trying to restrict to an action).

If we take your suggestion of an array for config.force_ssl then it doesn't help as we don't want Rack::SSL configured in any environment so we need to specify an empty array. The force_ssl controller method can't use config.force_ssl to see if it should run as it's not what we want.

If you're suggesting adding :env => [] to the force_ssl then there's no point as you'd still need to specify it everywhere and you can do it already using :if in a much cleaner way, e.g:

class AccountsController < ApplicationController
  force_ssl :if => :ssl_configured?

  def ssl_configured?
    !Rails.env.development?
  end
end

My concern is that the common case should work out of the box - maybe add a default implementation of the above that can be overridden.

@pixeltrix
Owner

Alternatively just merge this request and add the above example to the docs and changelog

@josevalim
Owner

Agreed. It is backwards incompatible but it is the best/cleanest solution imho. Let's add an entry to the CHANGELOG and merge this as is.

@pixeltrix pixeltrix closed this in c04a084
@pixeltrix
Owner

@josevalim backport this to 3.2 and 3.1 ? It'd be good to be consistent but I know how people hate backward incompatible changes.

@josevalim
Owner

No backport. No backward incompatible changes in stable releases.

@georgeG georgeG referenced this pull request from a commit in georgeG/rails
@georgeG georgeG Merge remote-tracking branch 'upstream/master'
* upstream/master: (76 commits)
  use content_tag instead strings
  A guide for upgrading Rails [ci skip]
  copy-edits [ci skip]
  Update changelogs with rails 3.0-stable branch info
  Remove wrong and redundant code.
  put and patch both are allowed for update
  fixes a test: rake routes now includes PATCH for resources
  use regular ruby for fewer method calls. we do not need :gift:s!
  consistently mention first patch, then put
  uses PATCH for the forms of persisted records, and routes PATCH and PUT to the update action of resources
  decouples the implementation of the inflector from its test suite
  removes the reconnect key from the database.yml generated for MySQL
  Fixed typo in composed_of example with Money#<=>, was comparing amount itself instead of other_money.amount
  explains why reconnect is false by default in the database.yml generated for MySQL
  removes verify_active_connections!
  Fix typo in match :to docs
  correct fetching :name option in form fields
  Update with new task names for building mysql and postgresql databases
  Document the :host option for force_ssl
  Update documentation for force_ssl - closes #5023.
  ...
12777f0
@ilyakatz

a bit late to the party, but i agree that it's annoying that it's disabled in development. why do you assume that people don't run ssl in development?

@ajbraus

Agreed to default to no SSL in development. However when I set force_ssl = true in config.application.rb my pow server (*******.dev url) still forces to ssl.

@pixeltrix
Owner

a bit late to the party, but i agree that it's annoying that it's disabled in development. why do you assume that people don't run ssl in development?

@ilyakatz it's not disabled in development since 4.0

Agreed to default to no SSL in development. However when I set force_ssl = true in config.application.rb my pow server (*******.dev url) still forces to ssl.

@ajbraus it will be enabled in development unless you take steps to disable like:

class AccountsController < ApplicationController
  force_ssl :if => :ssl_configured?

  def ssl_configured?
    !Rails.env.development?
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 13, 2012
  1. @pat
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/action_controller/metal/force_ssl.rb
@@ -26,7 +26,7 @@ module ClassMethods
def force_ssl(options = {})
host = options.delete(:host)
before_filter(options) do
- if !request.ssl? && !Rails.env.development?
+ unless request.ssl?
redirect_options = {:protocol => 'https://', :status => :moved_permanently}
redirect_options.merge!(:host => host) if host
redirect_options.merge!(:params => request.query_parameters)
View
14 actionpack/test/controller/force_ssl_test.rb
@@ -109,20 +109,6 @@ def test_cheeseburger_redirects_to_https
end
end
-class ForceSSLExcludeDevelopmentTest < ActionController::TestCase
- tests ForceSSLControllerLevel
-
- def setup
- Rails.env.stubs(:development?).returns(false)
- end
-
- def test_development_environment_not_redirects_to_https
- Rails.env.stubs(:development?).returns(true)
- get :banana
- assert_response 200
- end
-end
-
class ForceSSLFlashTest < ActionController::TestCase
tests ForceSSLFlash
Something went wrong with that request. Please try again.