Fix - substitutes DSN exposure with DBMS respective error messages #4508
Conversation
…ctive dbms messages to suppress password leakage.
There was a problem hiding this comment.
Thanks for submitting. The changes to the example project look good to me. I would like to keep the error in diesel_cli as it is, as there is really not much possibility to leak anything there, given that the URL is often provided explicitly and the application is designed to be run during development.
Edit: If you pull in the latest master branch, the CI error should also be fixed.
diesel_cli/src/errors.rs
Outdated
| @@ -7,7 +7,7 @@ use crate::infer_schema_internals::TableName; | |||
| pub enum Error { | |||
| #[error("Initializing `.env` file failed: {0}")] | |||
| DotenvError(#[from] dotenvy::Error), | |||
| #[error("Could not connect to database via `{url}`: {error}")] | |||
| #[error("Could not connect to database: {error}")] | |||
There was a problem hiding this comment.
Given that this is a CLI application that expects the URL as explicit argument there is really not much what could be leaked here. So that change should be reverted.
There was a problem hiding this comment.
Thanks for reviewing and fixing the CI error, I have now merged the latest master.
I was curious about this one - but is there a value in exposing this? There would still be concerns if the cli command was say implemented in a CI/CD pipeline and using the environmental DATABASE_URL, where it could also leak the credentials in certain cases.
There was a problem hiding this comment.
Yes there is value in exposing this. The URL can come from different input sources, like a .env file, an environment variable or a command line flag. That means it's not always 100% clear which URL is used. By explicitly printing out the URL, users can easily notice if the wrong URL is used.
And yes it could be exposed in CI situations, but again that environment already has access to this kind of information anyways.
There was a problem hiding this comment.
Thanks for reviewing weiznich. I have reverted the change to this file and its subsequent test.
I do strongly advise this be reconsidered for a future release. The DSN being exposed is a far bigger risk than a developer being unable to identify which DSN is in effect.
Perhaps some alternative approaches to consider:
- Sanitizing the DSN to redact the password component
- Having an additional argument in the CLI tool for exposing debug information to limit this behaviour by default.
Happy to prepare additional PR requests for either of the above if desired.
…esel into fix-dsn-password-leak
…different conditions (i.e. Database does not exist, or password not present).
This PR aims to address some security concerns raised within #4504.