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

Remove exclude option from ActionDispatch::SSL and fix secure cookies #5515

Merged
merged 2 commits into from Mar 19, 2012

Conversation

rafaelfranca
Copy link
Member

josevalim added a commit that referenced this pull request Mar 19, 2012
Remove exclude option from ActionDispatch::SSL and fix secure cookies
@josevalim josevalim merged commit ae97715 into rails:master Mar 19, 2012
@gtd
Copy link
Contributor

gtd commented Jan 3, 2013

What is the reasoning for removing the exclude option? That makes it impossible to get the benefit of the secure headers while still allowing some pages to be served over http doesn't it?

@rafaelfranca
Copy link
Member Author

This option was deprecated on Rack::SSL.

@gtd
Copy link
Contributor

gtd commented Jan 3, 2013

Huh? This feature is still in the current release of 1.3.2 which as far as I know is published from https://github.com/josh/rack-ssl/commits/master where there hasn't been any substantive commits in years, and no mention of deprecation of this feature anywhere I can see. Is there another repo somewhere? Discussion of the reasoning for this?

Seems to me rack-ssl serves two extremely important purposes: doing redirects and adding SSL headers. This change seems to cut off the utility of the middleware for anyone who doesn't want SSL everywhere and force them to duplicate the header functionality which is pretty much boilerplate that Rails should be able to take care of easily.

@rafaelfranca
Copy link
Member Author

@josh told us when we moved it into Rails that this feature was deprecated and should be removed. But I don't have more information.

@gtd
Copy link
Contributor

gtd commented Jan 3, 2013

@josh has an acute sense of style so I imagine he thought it was an inelegant hack, but currently it's the only hack that allows this middleware to be useful for mixed protocol sites. What do you think about a more straightforward method to disable the redirect? Would you accept a pull request for a simple disable_redirect option?

@josh
Copy link
Contributor

josh commented Jan 4, 2013

For rack-ssl specifically, its not design to handle "mixed protocol sites". STS is all or nothing on the domain.

If you want to juggle non-ssl and ssl connections, then it becomes an application concern best handled by AC. The middleware is great for people who just want SSL everywhere all the time.

@gtd
Copy link
Contributor

gtd commented Jan 4, 2013

@josh Thanks for the reply. Indeed I do handle the concern with AC, but the header munging is just boilerplate that every SSL request needs. It's orthogonal to the redirect functionality. Is there another middleware that handles this already? I'm sorry if I'm missing something here.

@pixeltrix
Copy link
Contributor

@gtd what they probably mean is that you should use the force_ssl filter in your controllers to manage which actions are SSL/non-SSL.

@gtd
Copy link
Contributor

gtd commented Jan 4, 2013

How come it seems like no one is reading my comment? I am not talking about redirection. I am talking about the header munging that rack-ssl performs. Is that redundant or somehow not necessary outside of rack-ssl? Help me understand.

@pixeltrix
Copy link
Contributor

@gtd can you elaborate on what you mean by secure headers?

@gtd
Copy link
Contributor

gtd commented Jan 4, 2013

The bottom half of the middleware:

  # http://tools.ietf.org/html/draft-hodges-strict-transport-sec-02
  def hsts_headers
    if @hsts
      value = "max-age=#{@hsts[:expires]}"
      value += "; includeSubDomains" if @hsts[:subdomains]
      { 'Strict-Transport-Security' => value }
    else
      {}
    end
  end

  def flag_cookies_as_secure!(headers)
    if cookies = headers['Set-Cookie']
      cookies = cookies.split("\n")

      headers['Set-Cookie'] = cookies.map { |cookie|
        if cookie !~ /; secure(;|$)/
          "#{cookie}; secure"
        else
          cookie
        end
      }.join("\n")
    end
  end

@pixeltrix
Copy link
Contributor

@gtd any reason you can't use that code in an after filter?

@gtd
Copy link
Contributor

gtd commented Jan 4, 2013

Of course not but it's boilerplate. This is very generic code that anyone with SSL ought to be able to make use of. I'm trying to make a suggestion to improve Rails, but interest seems a bit frigid.

@pixeltrix
Copy link
Contributor

If you want to send us a PR that adds the strict transport headers to force_ssl then I don't see any reason why that can't be merged, however I'd leave setting secure cookies to the application itself, either in session settings or the individual cookies usages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants