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

Reading DATABASE_URL from .env files implicitly can be very dangerous/potentially destructive #3722

Closed
benwilber opened this issue Feb 1, 2025 · 5 comments
Labels

Comments

@benwilber
Copy link
Contributor

benwilber commented Feb 1, 2025

I have found these related issues/pull requests

Somewhat related to #3061

Description

$ sqlx migrate run --help
Run all pending migrations
...
  -D, --database-url <DATABASE_URL>
          Location of the DB, by default will be read from the DATABASE_URL env var or `.env` files
...

sqlx cli reads DATABASE_URL from .env files implicitly which can be very dangerous and potentially destructive when running database commands against different environments (prod, staging, dev, etc.)

It is easy to accidentally run a migrate run/revert against a database that you did not intend to just by accidentally being in the wrong directory. Is it not sufficient for the developer to use something like dotenv, foreman, etc. like they normally do to explicitly load their .env files into the environment when running sqlx commands? This is much safer and less surprising. "Principle of Least Surprise" and all that.

Reproduction steps

Run an sqlx migrate command whilst in a directory with a .env file pointing to a server different than the one you thought (maybe you just changed directories, or are working in a script).

SQLx version

sqlx-cli 0.7.4

Enabled SQLx features

"chrono", "postgres", "runtime-tokio"

Database server and version

PG 14

Operating system

macOS

Rust version

rustc 1.84.0 (9fc6b4312 2025-01-07)

@benwilber benwilber added the bug label Feb 1, 2025
@abonander
Copy link
Collaborator

A lot of commands can be dangerous/destructive if run in the wrong folder. If this is a serious concern for you, perhaps you might consider not using .env files in general.

For the most destructive commands (db drop/db reset), we have a confirmation prompt. I could see adding one for migrate revert, but having to do it every time for migrate run might get a little annoying. I would also probably consider adding confirmation prompts to commands that didn't previously have them to be a breaking behavior change, since it would break any kind of automated script.

@benwilber
Copy link
Contributor Author

benwilber commented Feb 2, 2025

If this is a serious concern for you, perhaps you might consider not using .env files in general.

I like using .env files. I just don't expect every command I run to load and run with them implicitly.

A lot of commands can be dangerous/destructive if run in the wrong folder.

I agree that this is probably true. But I'm struggling to think of one that works this way just by implicitly reading a magic (and hidden) file in that folder

@abonander
Copy link
Collaborator

Well, I'm happy to discuss alternatives but otherwise there isn't really anything actionable here.

@benwilber
Copy link
Contributor Author

I think a reasonable and non-breaking change would be to have a top-level switch like --no-dotenv that will disable the automatic reading of .env files. This would make it safer for scripts to run without any of the "auto-detection" features for DATABASE_URL. A "safe mode" of sorts. It would also allow paranoid people like me to just alias sqlx like

alias sqlx="sqlx --no-dotenv"

@benwilber
Copy link
Contributor Author

@abonander I have opened a PR that addresses my concerns about this. Please let me know if this is something you're interested in. Thanks!

#3724

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

No branches or pull requests

2 participants