rethinkdb / horizon

Horizon is a realtime, open-source backend for JavaScript apps.
MIT License
6.78k stars 351 forks source link

nonce cookies not set if developing over HTTP #559

Open download13 opened 8 years ago

download13 commented 8 years ago

If you take a look at the oauth utils you'll notice that the secure option for setting cookies is true.

Shouldn't this respond to the secure horizon option?

I'm developing an application over HTTP because it's a lot more convenient and the express server it's running from won't support HTTPS anyway (that's going to be nginx's job). Right now OAuth logins don't work at all because of this.

deontologician commented 8 years ago

ping @Tryneus

Tryneus commented 8 years ago

@download13 - while we could make this change, I'm not convinced it would help in your situation. If you do not already have your HTTPS proxy running, I imagine you would run into #508 instead. If you do have your HTTPS proxy running, it should work in either case, as the client is interacting over HTTPS (but if I'm not mistaken it would be vulnerable if we made this change as the cookie could then be exposed to MitM attacks over HTTP).

Let me know your thoughts on this if it's still an issue for you.

download13 commented 8 years ago

It seems to me that there is an advantage to making developing with your framework as user-friendly as possible. If someone comes along who can't figure out how to set up nginx on their Windows development machine, that's one more person who just drops Horizon and goes to find another framework.

I'm all for forcing as much security as you can into production mode, but getting devs up and running as quickly and easily as possible is just as important if you want lots of people to start using Horizon (which I would love to see because these issues aside, it's pretty cool).

I probably don't have to tell you that as a developer, using a library where everything "just works" and you can immediately see it's utility and potential is a great feeling.

It doesn't have to be a security risk because secure should only be set to false during development. The only real danger is someone forgetting they left it set to false, but that can be mitigated by showing a warning message if !secure && NODE_ENV !== 'development' or something similar.

I think #508 should probably follow this pattern as well; allowing security to be turned off during development for the sake of convenience. Some OAuth providers may change http to https but at least developers can test their setup with one that doesn't (and I would hope some of the providers are smart enough to not do that for localhost and 127.0.0.1 redirect uris).

Just my two cents.

deontologician commented 8 years ago

I think the major thing is OAuth providers won't redirect to http, so we can allow it on our end, but it's going to cause a bug. Would it help if we print out an error if you try to use OAuth with --secure=false ?

marshall007 commented 8 years ago

I don't think that's necessarily true. I'm fairly certain at least Twitter and GitHub allow you to specify localhost as your redirect_uri (without TLS). Providing no workaround for this and #508 during local development seems kinda bad. Consider also that the developer might be mocking out the calls to the OAuth provider for testing purposes.

deontologician commented 8 years ago

Ok, reopening

Tryneus commented 8 years ago

@download13 - in your original post, you indicated you would be running the Horizon server over HTTP behind an HTTPS nginx proxy. If we disable secure: true for the nonce cookie when Horizon is serving HTTP, then this would still be vulnerable in your proposed production setup. In order to make this safe by default, we would need a separate option that would disable secure: true on the nonce cookie which would be set by dev mode.

I am mildly opposed to this since more options is not user-friendly. However, I don't think we should punish people running reverse proxies with insecure defaults, and if this is something people need we should add it. Perhaps we should just break up the secure command-line option into several more granular options.

download13 commented 8 years ago

I think you are right about splitting up secure.

As I understand it secure just tells the client which protocol to use. The client's choice may be completely independent of the server's in the case of a proxy or other intermediary program. For the same reason, the server's protocol choice can be independent of the protocol in the redirect URI.

Here's what I'm thinking:

Maybe there's a way to get it into fewer options.

marshall007 commented 8 years ago

This is also one of those things that will more broadly be fixed by #24.

deontologician commented 8 years ago

Mentioning #794 and pinging @Tryneus to re-evaluate whether this issue makes sense still