allow secure session cookies through proxy or to localhost

[jrmcgarvey]

It should be possible to make session cookies secure but also to send them when not running SSL, in two cases:

• first, if the requesting host is localhost.
• second, if the server is deployed behind a proxy that handles the SSL.

For the latter case, an options flag is added, trust_proxy, and the secure cookie is sent if that is true.

The direction of browser vendors is not to allow credentials to be included on fetch requests unless the cookies are secure and sameSite=None. This change allows Rails to return the session cookie as a credential for subsequent fetch requests. Also, it is useful to be able to make the session cookie secure in typical cloud deployments, when the Rails server is behind a proxy.

resolves issue #40

[ioquatix]

This is looking like a step in the right direction, but we should also add tests.

[jrmcgarvey]

I would need some guidance on how to write tests.

[ioquatix]

@jrmcgarvey review the existing test suite for examples.

I'd recommend a test for the default option, as well as the request.host == 'localhost'.

You only need to unit test def security_matches?(request, options), not the full integration test.

[jeremyevans]

Seems fine once a test is added.

[matthewd]

Something like assume_ssl might be a little more specific?

"I trust my proxy [not to... do a bad thing?]" is not the same as "I trust my proxy to recognise and apply Secure attribute semantics on cookies it relays [or to do so indirectly by forcing all connections to be SSL]".

[jrmcgarvey]

Ok, tests added.

[jrmcgarvey]

I chose "trust_proxy" because that is what is used in Express for the same purpose. Perhaps it should be "always_write_cookie", as that is what is used in Rails for non-session cookies, although this is not documented.

[ioquatix]

What about force_secure? Is there any other meaning behind trust_proxy in this context?

[jrmcgarvey]

We need to reflect on what is implemented today. I took a look at the logic in https://github.com/rack/rack/blob/main/lib/rack/request.rb . It seems like one could spoof things just by setting X-Forwarded-Proto to HTTPS, or some other equivalent header. I haven't seen any best practices on how proxies are to be configured for this. If there are intermediate proxies between the one that terminates the HTTPS connection and the actual server, should X-Forwarded-Proto be passed on through to the server? Having several hops might defeat the purpose of SSL in protecting data in transit. And, of course, not all proxies are even configured to set such headers.

The 'trust proxy' setting in Express provides various ways to configure things, viz: proxies, for which Rails has no equivalent. I don't know how this configuration affects things other than secure cookies.

force_secure sounds like one is forcing the cookie to be secure. Maybe trust_proxy_with_cookie?

[ioquatix]

force_secure sounds like one is forcing the cookie to be secure

I see - so what's happening here is, we have security_matches? and it gets called with the request and the options of the cookie we want to send back. If the options includes :secure, we need to make sure that the security of the cookie can be guaranteed. Previously, it was guaranteed if the request was using ssl (e.g. https), but now we are introducing two other ways in which we are okay to write a secure cookie back to the client:

2. If the request was to localhost, we assume the request is "development mode" and we assume we can trust all cookies. In this case, I'm a little cautious now. As you correctly called out, there are situations which might trigger this logic (e.g. x-forwarded-host or forwarded: host= https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded). I could also imagine someone running an incorrectly configured proxy server. e.g. the application is running on localhost, and they've pointed nginx at that host.
3. We can force the cookie handling machinery to just "trust the proxy", as in, even if the request wasn't SSL, we trust that somewhere down the line, it is.

        def security_matches?(request, options)
          return true unless options[:secure]
          request.ssl?
          # OK to send a secure token over ssl, or to local host
          # or if the instance is running behind a proxy that handles ssl
          request.ssl? || request.host == 'localhost' || @trust_proxy == true
        end

I understand now what @matthewd was saying. What we are really doing here is saying "We assume downstream that the connection is SSL/secure, e.g. assume_ssl. That makes sense to me now. I'd also be okay with assume_secure or something.

Additionally, I'm not sure that request.host == 'localhost' is safe. Do you feel confident about introducing that? https://github.com/rack/rack/blob/dff6cfd249832d32ab190e6d20605bce0d6c702d/lib/rack/request.rb#L332-L335

I'd like you to feel the full weight of the security implications here. If we are wrong about this, it could be a major security issue. Therefore, might I suggest the following:

• I'm okay with an option to assume_ssl or assume_secure since this is opt in by the application. I'm not sure I like trust_proxy as it assumes there is a proxy, but we are making no such validation/assertion.
• I'd like you to consider removing the localhost check, and open a separate PR.

We can definitely merge the former, but the latter has a higher bar IMHO. Is this a reasonable course of action?

[jrmcgarvey]

Ok, I will take out the localhost part, and change to assume_ssl. One could set that value for development, and not set it, if it is inappropriate, in production. It would be nice if other Rails cookies were handled with the same flag.

[ioquatix]

LGTM.

[ioquatix]

One idea I had, is we could allow @assume_ssl to be a proc, e.g.

@assume_ssl == true || (@assume_ssl.respond_to?(:call) && @assume_ssl.call(request))

(separate PR after this one).