Safeguard against accidentally using a remote DATABASE_URL

[svenfuchs]

We seen remote databases wiped by accidentally running a test suite with DATABASE_URL being exported.

The scenarios were:

• A developer sets up a local app to work with a remote staging database during development by exporting DATABASE_URL to the local environment. Then forgets to unset it and runs the test suite.
• A developer returns to an old terminal window that still has DATABASE_URL exported and runs a test suite.

While technically this might not be the responsibility of the library database_cleaner to take care of I think it would be great if some basic protection was turned on by default.

This PR adds a safeguard that looks for the environment variable DATABASE_URL and raises unless opted out.

The implementation is simple and probably should be improved and documented. I'm opening this PR to see if you were interested in such a safety mechanism before I'd then put more work into it.

[svenfuchs]

Not sure why exactly the tests are failing on Travis CI, but it seems as if there's been an issue with the Gemfile.lock for quite a while now. Let me know if you need any help fixing that.

[etagwerker]

@svenfuchs Thanks for submitting this! I think it's a good idea.

I wonder if we could make it even better. Maybe even check if ENV['RACK_ENV'] set to production and raise if that's the case?

[svenfuchs]

@etagwerker That's an interesting idea ...

In our cases we've run the tests against the remote databases. The test tooling (rspec) and/or the spec_helper would set the current env to test (we use ENV['ENV'] in several apps, but yeah). So having DatabaseCleaner check for that wouldn't have helped.

But yeah, I could see how fail unless (ENV['ENV'] || ENV['RACK_ENV'] || ENV['RAILS_ENV']) == 'test' might totally make sense in other scenarios.

[svenfuchs]

@etagwerker also, forgot to ask ... could you advise on what style/s you'd prefer for opting out of this, error messages raised, or any other details that you'd like to see improved?

I'd think we'd probably want to make opting out a Ruby config option such as DatabaseCleaner.allow_remote_database_url = true or similar.

[etagwerker]

@svenfuchs I like this format:

DatabaseCleaner.allow_remote_database_url = true

It'd be great if you could implement that in configuration.rb

So having DatabaseCleaner check for that wouldn't have helped.

Got it. I once saw this and I thought we could've stopped it if we had a safeguard for that: https://twitter.com/moubry/status/818867315970363392 - That's why I brought it up.

[svenfuchs]

@etagwerker i've made a few more changes:

• split the Safeguard class into two classes that run the checks for production and the database url
• add DatabaseCleaner.allow_production and DatabaseCleaner.allow_remote_database_url for opting out
• keep the ability to opt out via env vars, too (i figured that might be useful for one-off scenarios where one wouldn't want to change the code, but still opt out)
• namespace the exceptions, add more useful messages
• document safeguards in the readme
• add an entry to the history

I have also made these changes to fix Travis CI and cherrypicked them to this branch in an attempt to get the PR green, but there's an unstable spec that fails occasionally. Restarting this job should fix that. Also commented on that here.

[rwrede]

What about 127.0.0.1? Should that also be covered?

[svenfuchs]

@rwrede good call, thanks for the pointer! i've addressed that in svenfuchs@8b46dfc

[etagwerker]

@svenfuchs This looks really good. I made a few small changes to the spec over here: https://github.com/DatabaseCleaner/database_cleaner/tree/safeguard

If you approve I can go ahead and merge that branch into master

[svenfuchs]

@etagwerker yes, that looks great, thanks a lot!

[etagwerker]

@svenfuchs cool, just merged your commits. Thank you! 👍

[svenfuchs]

thank you so much, @etagwerker ❤️

[arp242]

Maybe it's a good idea to also include ::1 for IPv6?

(also, all 127.0.0.0/8 addresses are local, although not sure if it's worth the effort adding support for that, as it's not often used)

[etagwerker]

Definitely. However, I'd encourage you to file an issue first. In my opinion, it has to be a concrete issue first in order to get patched later.

[soberstadt]

@etagwerker Thanks for helping to get this in! Are you planning to cut a new version of the gem to deploy this?

[lowang]

@etagwerker I suppose that there is a simpler solution than extra config variable, just run following:
ActiveRecord::Tasks::DatabaseTasks.check_protected_environments!
I've added this to rails_spec.rb but having this inside DatabaseCleaner would be awesome

[etagwerker]

@soberstadt Yes, I'll try to cut a release this weekend.

@lowang Interesting! That works well when you're using ActiveRecord as your adapter. The solution implemented by Sven is generic enough that it will work with any adapter.

[etagwerker]

@soberstadt Just released a new version. 👍

[joshm1204]

I am getting

An error occurred in a `before(:suite)` hook.
Failure/Error: DatabaseCleaner.clean_with(:truncation)
DatabaseCleaner::Safeguard::Error::RemoteDatabaseUrl:
  ENV['DATABASE_URL'] is set to a remote URL. Please refer to https://github.com/DatabaseCleaner/database_cleaner#safeguards

When doing CI on Heroku.
I think I need to add

DatabaseCleaner.allow_remote_database_url = true
Is this the solution and where do I add it?