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.

Sign up for GitHub

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

Fix - substitutes DSN exposure with DBMS respective error messages #4508

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Fix - substitutes DSN exposure with DBMS respective error messages #4508

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1f0fb3d
Fix - replaces example establish_connection error messages with respe…
edonsec Feb 27, 2025
0ba587f
Added back error enum's url property. This may be superfluous.
edonsec Feb 27, 2025
316487d
Several other examples exposing the DSN.
edonsec Feb 28, 2025
a7fb2e2
Test fix for assertion in invalid_database_url.
edonsec Feb 28, 2025
9fb0fcd
Lint fixes
edonsec Feb 28, 2025
afb8ad6
Lint fix #2
edonsec Feb 28, 2025
9728659
Merge branch 'diesel-rs:master' into fix-dsn-password-leak
edonsec Feb 28, 2025
c7a5e1a
Database error correction to pass migration test suite.
edonsec Feb 28, 2025
4505534
Merge branch 'fix-dsn-password-leak' of https://github.com/edonsec/di…
edonsec Feb 28, 2025
f387aea
Main thing is an error is thrown. This check accomodates this across …
edonsec Feb 28, 2025
986af29
Suggested format fix.
edonsec Feb 28, 2025
941083c
Reverting change to address discussion: https://github.com/diesel-rs/…
edonsec Mar 7, 2025
afa0ba9
Reverting change to test to address discussion: https://github.com/di…
edonsec Mar 7, 2025
d4ee0dd
Reverting test to ensure pass condition.
edonsec Mar 8, 2025
8c8de90
Test fix
edonsec Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion diesel_cli/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ConnectionError {
error: diesel::ConnectionError,
url: String,
Expand Down
2 changes: 1 addition & 1 deletion diesel_cli/tests/migration_run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ fn error_migrations_when_use_invalid_database_url() {
assert!(!result.is_success());
assert!(result
.stderr()
.contains("Could not connect to database via `postgres://localhost/lemmy`:"));
.contains("Could not connect to database: connection to server at \"localhost\" (::1), port 5432 failed: fe_sendauth: no password supplied"));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion examples/mysql/getting_started_step_1/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ pub fn establish_connection() -> MysqlConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
MysqlConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}
2 changes: 1 addition & 1 deletion examples/mysql/getting_started_step_2/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn establish_connection() -> MysqlConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
MysqlConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut MysqlConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/mysql/getting_started_step_3/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn establish_connection() -> MysqlConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
MysqlConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut MysqlConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/composite_types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ pub fn establish_connection() -> PgConnection {

let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}
2 changes: 1 addition & 1 deletion examples/postgres/custom_arrays/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,7 @@ fn postgres_connection() -> PgConnection {
.expect("PG_DATABASE_URL must be set");

PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}
```

Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/custom_arrays/tests/parallel_service_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ fn postgres_connection() -> PgConnection {
.expect("PG_DATABASE_URL must be set");

let mut conn = PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url));
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e));
conn.begin_test_transaction()
.expect("Failed to begin test transaction");
run_db_migration(&mut conn);
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/custom_arrays/tests/single_service_test_run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fn postgres_connection() -> PgConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("PG_DATABASE_URL must be set");
let mut conn = PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url));
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e));
conn.begin_test_transaction()
.expect("Failed to begin test transaction");
conn
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/custom_types/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub struct Translation {
fn main() {
let database_url = std::env::var("DATABASE_URL").expect("DATABASE_URL must be set");
let conn = &mut PgConnection::establish(&database_url)
.unwrap_or_else(|e| panic!("Error connecting to {}: {}", database_url, e));
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e));

let _ = diesel::insert_into(translations::table)
.values(&Translation {
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/getting_started_step_1/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ pub fn establish_connection() -> PgConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}
2 changes: 1 addition & 1 deletion examples/postgres/getting_started_step_2/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn establish_connection() -> PgConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut PgConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/getting_started_step_3/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn establish_connection() -> PgConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut PgConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/postgres/relations/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ fn establish_connection() -> PgConnection {

let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
PgConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {database_url}"))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

fn new_author(conn: &mut PgConnection, name: &str) -> DbResult<Author> {
Expand Down
2 changes: 1 addition & 1 deletion examples/sqlite/getting_started_step_1/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ pub fn establish_connection() -> SqliteConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
SqliteConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}
2 changes: 1 addition & 1 deletion examples/sqlite/getting_started_step_2/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub fn establish_connection() -> SqliteConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
SqliteConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut SqliteConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/sqlite/getting_started_step_3/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub fn establish_connection() -> SqliteConnection {
.or_else(|_| env::var("DATABASE_URL"))
.expect("DATABASE_URL must be set");
SqliteConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

pub fn create_post(conn: &mut SqliteConnection, title: &str, body: &str) -> Post {
Expand Down
2 changes: 1 addition & 1 deletion examples/sqlite/relations/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ fn establish_connection() -> SqliteConnection {

let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
SqliteConnection::establish(&database_url)
.unwrap_or_else(|_| panic!("Error connecting to {database_url}"))
.unwrap_or_else(|e| panic!("Failed to connect, error: {}", e))
}

fn new_author(conn: &mut SqliteConnection, name: &str) -> DbResult<Author> {
Expand Down
Loading