From 4e1160c7fc0ea4bc46e9661bb1dd921a925dc7ec Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 5 Oct 2026 20:50:26 +0000 Subject: [PATCH 1/2] fix(cli): fail check when the database login fails and read the password file When a database is configured, `check` now connects up front and reports a failed connection or login as a `database/connection` error with a non-zero exit, instead of skipping the database-backed checks silently. `--disable-db` and `db.disableConnection` still check without a database; the config option was overridden by the CLI default before. Without a password from the configuration, the connection string, or PGPASSWORD, the password is looked up in PGPASSFILE or ~/.pgpass as libpq does, before falling back to the default `postgres`. Fixes #834 --- Cargo.lock | 1 + crates/pgls_cli/src/commands/check.rs | 21 +- crates/pgls_cli/src/diagnostics.rs | 35 +++ crates/pgls_cli/src/reporter/mod.rs | 8 + crates/pgls_cli/tests/assert_check.rs | 180 ++++++++++++ .../postgres-language-server.jsonc | 10 + .../tests/fixtures/unknown_column.sql | 1 + ...check_fails_when_database_login_fails.snap | 19 ++ crates/pgls_configuration/src/database.rs | 2 +- crates/pgls_env/src/lib.rs | 13 +- crates/pgls_lsp/src/server.rs | 1 + crates/pgls_workspace/Cargo.toml | 2 + crates/pgls_workspace/src/diagnostics.rs | 11 +- .../src/features/diagnostics.rs | 11 + crates/pgls_workspace/src/settings.rs | 12 +- crates/pgls_workspace/src/workspace.rs | 10 + crates/pgls_workspace/src/workspace/client.rs | 7 + crates/pgls_workspace/src/workspace/server.rs | 39 ++- .../workspace/server/connection_manager.rs | 146 ++++++++-- .../src/workspace/server/pgpass.rs | 267 ++++++++++++++++++ docs/codegen/src/env_variables.rs | 6 + docs/guides/configure_database.md | 20 +- docs/reference/env_variables.md | 4 + 23 files changed, 795 insertions(+), 31 deletions(-) create mode 100644 crates/pgls_cli/tests/fixtures/disable_connection/postgres-language-server.jsonc create mode 100644 crates/pgls_cli/tests/fixtures/unknown_column.sql create mode 100644 crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap create mode 100644 crates/pgls_workspace/src/workspace/server/pgpass.rs diff --git a/Cargo.lock b/Cargo.lock index 79f8f873d..c53d98375 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3019,6 +3019,7 @@ dependencies = [ "tokio", "tracing", "tree-sitter", + "url", ] [[package]] diff --git a/crates/pgls_cli/src/commands/check.rs b/crates/pgls_cli/src/commands/check.rs index a0b00ad21..8a2b1cd6a 100644 --- a/crates/pgls_cli/src/commands/check.rs +++ b/crates/pgls_cli/src/commands/check.rs @@ -1,14 +1,16 @@ use crate::cli_options::CliOptions; use crate::commands::get_files_to_process_with_cli_options; +use crate::diagnostics::DatabaseUnavailable; use crate::execute::{StdinPayload, run_files, run_stdin}; use crate::reporter::Report; use crate::{CliDiagnostic, CliSession, VcsIntegration}; use crate::{ExecutionConfig, ExecutionMode, VcsTargeting}; use pgls_configuration::PartialConfiguration; use pgls_console::Console; -use pgls_diagnostics::category; +use pgls_diagnostics::{Error, category}; use pgls_fs::FileSystem; use pgls_workspace::DynRef; +use pgls_workspace::features::diagnostics::CheckDatabaseConnectionParams; use std::ffi::OsString; pub struct CheckArgs { @@ -50,7 +52,11 @@ pub fn check( let payload = read_stdin_payload(stdin_path, session.console())?; run_stdin(&mut session, &execution, payload) } else { - let report: Report = run_files(&mut session, &execution, paths)?; + let database_error = check_database_connection(&session)?; + let mut report: Report = run_files(&mut session, &execution, paths)?; + if let Some(error) = database_error { + report.prepend_diagnostic(error); + } let exit_result = enforce_exit_codes(cli_options, &report); session.report("check", cli_options, &report)?; @@ -58,6 +64,17 @@ pub fn check( } } +/// A configured database that cannot be reached is an error: the checks that need it would +/// otherwise be skipped silently. +fn check_database_connection(session: &CliSession) -> Result, CliDiagnostic> { + let result = session + .workspace() + .check_database_connection(CheckDatabaseConnectionParams::default())?; + Ok(result + .error + .map(|message| Error::from(DatabaseUnavailable::new(message)))) +} + fn resolve_paths( fs: &DynRef<'_, dyn FileSystem>, configuration: &PartialConfiguration, diff --git a/crates/pgls_cli/src/diagnostics.rs b/crates/pgls_cli/src/diagnostics.rs index 0494bfd09..1b472ea79 100644 --- a/crates/pgls_cli/src/diagnostics.rs +++ b/crates/pgls_cli/src/diagnostics.rs @@ -163,6 +163,41 @@ pub struct CheckError { message: MessageAndDescription, } +/// The configured database could not be used, so the checks that need it did not run. +#[derive(Debug, Diagnostic)] +#[diagnostic(category = "database/connection", severity = Error)] +pub struct DatabaseUnavailable { + #[message] + #[description] + message: MessageAndDescription, + + #[advice] + advice: DatabaseUnavailableAdvice, +} + +impl DatabaseUnavailable { + pub fn new(message: impl Into) -> Self { + Self { + message: MessageAndDescription::from(message.into()), + advice: DatabaseUnavailableAdvice, + } + } +} + +#[derive(Debug)] +struct DatabaseUnavailableAdvice; + +impl Advices for DatabaseUnavailableAdvice { + fn record(&self, visitor: &mut dyn Visit) -> std::io::Result<()> { + visitor.record_log( + LogCategory::Info, + &markup! { + "The checks that need the database were skipped. Fix the connection settings, or pass ""--disable-db"" or set ""db.disableConnection"" to check without a database." + }, + ) + } +} + #[derive(Debug, Diagnostic)] #[diagnostic( severity = Error, diff --git a/crates/pgls_cli/src/reporter/mod.rs b/crates/pgls_cli/src/reporter/mod.rs index f184feed8..a409685d0 100644 --- a/crates/pgls_cli/src/reporter/mod.rs +++ b/crates/pgls_cli/src/reporter/mod.rs @@ -99,6 +99,14 @@ impl Report { traversal, } } + + /// Adds a diagnostic that does not belong to a single file, ahead of the file diagnostics. + pub fn prepend_diagnostic(&mut self, diagnostic: Error) { + let (errors, warnings) = count_levels(std::slice::from_ref(&diagnostic)); + self.errors += errors; + self.warnings += warnings; + self.diagnostics.insert(0, diagnostic); + } } pub trait ReportWriter { diff --git a/crates/pgls_cli/tests/assert_check.rs b/crates/pgls_cli/tests/assert_check.rs index 9f8341885..631d729b2 100644 --- a/crates/pgls_cli/tests/assert_check.rs +++ b/crates/pgls_cli/tests/assert_check.rs @@ -222,6 +222,186 @@ async fn check_accepts_tls_connection_strings(test_db: PgPool) { std::fs::remove_file(sql_file).expect("failed to remove temporary SQL file"); } +const UNKNOWN_COLUMN_PATH: &str = "tests/fixtures/unknown_column.sql"; + +#[cfg(target_os = "linux")] +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn check_fails_when_database_login_fails(test_db: PgPool) { + let opts = test_db.connect_options(); + let port = opts.get_port().to_string(); + + assert_snapshot!(run_check_without_db_env( + &[ + "--host", + opts.get_host(), + "--port", + &port, + "--username", + "postgres", + "--password", + "wrong-password", + "--database", + opts.get_database().unwrap_or("postgres"), + UNKNOWN_COLUMN_PATH, + ], + &[], + )); +} + +#[cfg(target_os = "linux")] +#[test] +fn check_fails_when_database_is_unreachable() { + // Nothing listens on port 1. + let output = run_check_without_db_env( + &[ + "--host", + "127.0.0.1", + "--port", + "1", + "--conn_timeout_secs", + "1", + UNKNOWN_COLUMN_PATH, + ], + &[], + ); + + assert!(output.starts_with("status: failure"), "{output}"); + assert!(output.contains("database/connection"), "{output}"); +} + +#[cfg(target_os = "linux")] +#[test] +fn check_without_database_succeeds_with_disable_db() { + let output = run_check_without_db_env( + &[ + "--host", + "127.0.0.1", + "--port", + "1", + "--conn_timeout_secs", + "1", + "--disable-db", + UNKNOWN_COLUMN_PATH, + ], + &[], + ); + + assert!(output.starts_with("status: success"), "{output}"); + assert!(!output.contains("database/connection"), "{output}"); +} + +#[cfg(target_os = "linux")] +#[test] +fn check_without_database_succeeds_with_disable_connection_in_config() { + let mut cmd = cargo_bin_cmd!("postgres-language-server"); + let output = cmd + .env_remove("DATABASE_URL") + .env_remove("PGHOST") + .args([ + "check", + "--config-path", + "tests/fixtures/disable_connection/postgres-language-server.jsonc", + "--log-level", + "none", + UNKNOWN_COLUMN_PATH, + ]) + .output() + .expect("failed to run CLI"); + let output = normalize_output( + output.status, + &String::from_utf8_lossy(&output.stdout), + &String::from_utf8_lossy(&output.stderr), + ); + + assert!(output.starts_with("status: success"), "{output}"); + assert!(!output.contains("database/connection"), "{output}"); +} + +#[cfg(target_os = "linux")] +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn check_reads_the_password_from_pgpassfile(test_db: PgPool) { + use std::os::unix::fs::PermissionsExt; + + let opts = test_db.connect_options(); + let host = opts.get_host().to_string(); + let port = opts.get_port().to_string(); + let database = opts.get_database().unwrap_or("postgres").to_string(); + let role = format!("pgls_pgpass_{}", std::process::id()); + sqlx::query(&format!("drop role if exists {role}")) + .execute(&test_db) + .await + .unwrap(); + sqlx::query(&format!( + "create role {role} login password 'pgpass:secret'" + )) + .execute(&test_db) + .await + .unwrap(); + + let passfile = std::env::temp_dir().join(format!("{role}.pgpass")); + std::fs::write( + &passfile, + format!("# comment\n{host}:{port}:{database}:{role}:pgpass\\:secret\n"), + ) + .unwrap(); + std::fs::set_permissions(&passfile, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let output = run_check_without_db_env( + &[ + "--host", + &host, + "--port", + &port, + "--username", + &role, + "--database", + &database, + UNKNOWN_COLUMN_PATH, + ], + &[("PGPASSFILE", passfile.to_str().unwrap())], + ); + + std::fs::remove_file(&passfile).unwrap(); + sqlx::query(&format!("drop role {role}")) + .execute(&test_db) + .await + .unwrap(); + + // The column check needs the database schema, so it only runs after a successful login. + assert!(!output.contains("database/connection"), "{output}"); + assert!(output.contains("lint/unknownColumn"), "{output}"); +} + +/// Runs `check` with the database settings given in `args` only, without the connection +/// settings of the environment running the tests. +#[cfg(target_os = "linux")] +fn run_check_without_db_env(args: &[&str], envs: &[(&str, &str)]) -> String { + let mut cmd = cargo_bin_cmd!("postgres-language-server"); + for var in [ + "DATABASE_URL", + "PGHOST", + "PGPORT", + "PGUSER", + "PGPASSWORD", + "PGDATABASE", + ] { + cmd.env_remove(var); + } + // Keep the passfile of the user running the tests out of it. + cmd.env("PGPASSFILE", "/nonexistent/pgpass"); + cmd.envs(envs.iter().copied()); + + let mut full_args = vec!["check", "--config-path", CONFIG_PATH, "--log-level", "none"]; + full_args.extend_from_slice(args); + let output = cmd.args(full_args).output().expect("failed to run CLI"); + + normalize_output( + output.status, + &String::from_utf8_lossy(&output.stdout), + &String::from_utf8_lossy(&output.stderr), + ) +} + fn run_check(args: &[&str]) -> String { let mut full_args = vec!["--config-path", CONFIG_PATH, "--log-level", "none"]; full_args.extend_from_slice(args); diff --git a/crates/pgls_cli/tests/fixtures/disable_connection/postgres-language-server.jsonc b/crates/pgls_cli/tests/fixtures/disable_connection/postgres-language-server.jsonc new file mode 100644 index 000000000..19a40ac83 --- /dev/null +++ b/crates/pgls_cli/tests/fixtures/disable_connection/postgres-language-server.jsonc @@ -0,0 +1,10 @@ +{ + "$schema": "https://pg-language-server.com/schema/postgres-language-server.schema.json", + "db": { + // Nothing listens on port 1. + "host": "127.0.0.1", + "port": 1, + "connTimeoutSecs": 1, + "disableConnection": true + } +} diff --git a/crates/pgls_cli/tests/fixtures/unknown_column.sql b/crates/pgls_cli/tests/fixtures/unknown_column.sql new file mode 100644 index 000000000..3dca987aa --- /dev/null +++ b/crates/pgls_cli/tests/fixtures/unknown_column.sql @@ -0,0 +1 @@ +select no_such_column from pg_catalog.pg_class; diff --git a/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap new file mode 100644 index 000000000..733587a1f --- /dev/null +++ b/crates/pgls_cli/tests/snapshots/assert_check__check_fails_when_database_login_fails.snap @@ -0,0 +1,19 @@ +--- +source: crates/pgls_cli/tests/assert_check.rs +expression: "run_check_without_db_env(&[\"--host\", opts.get_host(), \"--port\", &port,\n\"--username\", \"postgres\", \"--password\", \"wrong-password\", \"--database\",\nopts.get_database().unwrap_or(\"postgres\"), UNKNOWN_COLUMN_PATH,], &[],)" +--- +status: failure +stdout: +Checked 1 file in . No fixes applied. +Found 1 error. +stderr: +database/connection ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Database error: password authentication failed for user "postgres" + + i The checks that need the database were skipped. Fix the connection settings, or pass --disable-db or set db.disableConnection to check without a database. + + +check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Some errors were emitted while running checks. diff --git a/crates/pgls_configuration/src/database.rs b/crates/pgls_configuration/src/database.rs index 6afacef8a..2e650031d 100644 --- a/crates/pgls_configuration/src/database.rs +++ b/crates/pgls_configuration/src/database.rs @@ -51,7 +51,7 @@ pub struct DatabaseConfiguration { pub conn_timeout_secs: u16, /// Actively disable all database-related features. - #[partial(bpaf(long("disable-db"), switch, fallback(Some(false))))] + #[partial(bpaf(long("disable-db"), req_flag(true)))] #[partial(cfg_attr(feature = "schema", schemars(skip)))] pub disable_connection: bool, } diff --git a/crates/pgls_env/src/lib.rs b/crates/pgls_env/src/lib.rs index f8968e817..a146c4fd4 100644 --- a/crates/pgls_env/src/lib.rs +++ b/crates/pgls_env/src/lib.rs @@ -41,6 +41,7 @@ pub struct PgLSEnv { pub pgport: PgLSEnvVariable, pub pguser: PgLSEnvVariable, pub pgpassword: PgLSEnvVariable, + pub pgpassfile: PgLSEnvVariable, pub pgdatabase: PgLSEnvVariable, // DEPRECATED - kept for backward compatibility @@ -83,6 +84,10 @@ impl PgLSEnv { "PGPASSWORD", "The password to connect to the database.", ), + pgpassfile: PgLSEnvVariable::new( + "PGPASSFILE", + "The password file to look up the password in when none is configured. Default: `~/.pgpass`.", + ), pgdatabase: PgLSEnvVariable::new( "PGDATABASE", "The name of the database to connect to.", @@ -181,7 +186,13 @@ impl Display for PgLSEnv { }; let sensitive = [&self.database_url, &self.pgpassword]; - let non_sensitive = [&self.pghost, &self.pgport, &self.pguser, &self.pgdatabase]; + let non_sensitive = [ + &self.pghost, + &self.pgport, + &self.pguser, + &self.pgpassfile, + &self.pgdatabase, + ]; for var in sensitive { match var.value() { diff --git a/crates/pgls_lsp/src/server.rs b/crates/pgls_lsp/src/server.rs index 374dc5638..97b6e8c77 100644 --- a/crates/pgls_lsp/src/server.rs +++ b/crates/pgls_lsp/src/server.rs @@ -507,6 +507,7 @@ impl ServerFactory { workspace_method!(builder, change_file); workspace_method!(builder, close_file); workspace_method!(builder, pull_file_diagnostics); + workspace_method!(builder, check_database_connection); workspace_method!(builder, get_completions); workspace_method!(builder, register_project_folder); workspace_method!(builder, unregister_project_folder); diff --git a/crates/pgls_workspace/Cargo.toml b/crates/pgls_workspace/Cargo.toml index a8073186f..f1690ec80 100644 --- a/crates/pgls_workspace/Cargo.toml +++ b/crates/pgls_workspace/Cargo.toml @@ -57,6 +57,7 @@ pgls_splinter = { workspace = true, optional = true } pgls_typecheck = { workspace = true, optional = true } sqlx = { workspace = true, optional = true } tokio = { workspace = true, features = ["rt", "rt-multi-thread"], optional = true } +url = { version = "2.5.8", optional = true } [features] db = [ @@ -67,6 +68,7 @@ db = [ "dep:pgls_plpgsql_check", "dep:pgls_splinter", "dep:pgls_typecheck", + "dep:url", "pgls_catalog/db", ] default = ["db"] diff --git a/crates/pgls_workspace/src/diagnostics.rs b/crates/pgls_workspace/src/diagnostics.rs index 17bfc269a..7db9e96c3 100644 --- a/crates/pgls_workspace/src/diagnostics.rs +++ b/crates/pgls_workspace/src/diagnostics.rs @@ -242,14 +242,17 @@ impl From for WorkspaceError { } } -#[derive(Debug, Serialize, Deserialize, Diagnostic)] +#[derive(Clone, Debug, Serialize, Deserialize, Diagnostic)] #[diagnostic( category = "database/connection", - message = "Database error: {message}" + message( + message("Database error: "{self.message}), + description = "Database error: {message}" + ) )] pub struct DatabaseConnectionError { - message: String, - code: Option, + pub(crate) message: String, + pub(crate) code: Option, } #[cfg(feature = "db")] diff --git a/crates/pgls_workspace/src/features/diagnostics.rs b/crates/pgls_workspace/src/features/diagnostics.rs index e09bea76a..02c296221 100644 --- a/crates/pgls_workspace/src/features/diagnostics.rs +++ b/crates/pgls_workspace/src/features/diagnostics.rs @@ -19,6 +19,17 @@ pub struct PullDiagnosticsResult { pub skipped_diagnostics: u32, } +#[derive(Debug, Default, serde::Serialize, serde::Deserialize)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct CheckDatabaseConnectionParams {} + +#[derive(Debug, Default, serde::Serialize, serde::Deserialize)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +pub struct CheckDatabaseConnectionResult { + /// Why the configured database cannot be used, if it cannot. + pub error: Option, +} + #[derive(Debug, serde::Serialize, serde::Deserialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct PullDatabaseDiagnosticsParams { diff --git a/crates/pgls_workspace/src/settings.rs b/crates/pgls_workspace/src/settings.rs index 78268492b..f0dd50394 100644 --- a/crates/pgls_workspace/src/settings.rs +++ b/crates/pgls_workspace/src/settings.rs @@ -723,6 +723,9 @@ impl Default for TypecheckSettings { } } +/// The password used when neither the configuration, `PGPASSWORD`, nor the password file has one. +pub const DEFAULT_DATABASE_PASSWORD: &str = "postgres"; + /// Database settings for the entire workspace pub struct DatabaseSettings { pub enable_connection: bool, @@ -730,7 +733,9 @@ pub struct DatabaseSettings { pub host: String, pub port: u16, pub username: String, - pub password: String, + /// The password from the configuration or `PGPASSWORD`. Without one, the connection looks + /// it up in the password file and falls back to [`DEFAULT_DATABASE_PASSWORD`]. + pub password: Option, pub database: String, pub conn_timeout_secs: Duration, pub allow_statement_executions: bool, @@ -766,7 +771,7 @@ impl Default for DatabaseSettings { host: "127.0.0.1".to_string(), port: 5432, username: "postgres".to_string(), - password: "postgres".to_string(), + password: None, database: "postgres".to_string(), conn_timeout_secs: Duration::from_secs(10), allow_statement_executions: true, @@ -836,7 +841,8 @@ impl From for DatabaseSettings { port, username, - password: value.password.unwrap_or(d.password), + // An empty password counts as none, as in libpq. + password: value.password.filter(|password| !password.is_empty()), database, host, diff --git a/crates/pgls_workspace/src/workspace.rs b/crates/pgls_workspace/src/workspace.rs index 63d70fa6c..0ae2afed3 100644 --- a/crates/pgls_workspace/src/workspace.rs +++ b/crates/pgls_workspace/src/workspace.rs @@ -17,6 +17,7 @@ use crate::{ }, completions::{CompletionsResult, GetCompletionsParams}, diagnostics::{ + CheckDatabaseConnectionParams, CheckDatabaseConnectionResult, PullDatabaseDiagnosticsParams, PullDiagnosticsResult, PullFileDiagnosticsParams, }, format::{PullFileFormattingParams, PullFormattingResult}, @@ -118,6 +119,15 @@ pub trait Workspace: Send + Sync + RefUnwindSafe { params: PullDatabaseDiagnosticsParams, ) -> Result; + /// Connects to the configured database and loads its schema, which the database-backed + /// checks need. Reports why that failed, including for a connection that failed recently + /// and is waiting to be retried. Reports nothing when no database is configured or the + /// connection is disabled. + fn check_database_connection( + &self, + params: CheckDatabaseConnectionParams, + ) -> Result; + /// Retrieves a list of available code_actions for a file/cursor_position fn pull_code_actions( &self, diff --git a/crates/pgls_workspace/src/workspace/client.rs b/crates/pgls_workspace/src/workspace/client.rs index ed87761d5..1e60b4660 100644 --- a/crates/pgls_workspace/src/workspace/client.rs +++ b/crates/pgls_workspace/src/workspace/client.rs @@ -169,6 +169,13 @@ where self.request("pgls/pull_db_diagnostics", params) } + fn check_database_connection( + &self, + params: crate::features::diagnostics::CheckDatabaseConnectionParams, + ) -> Result { + self.request("pgls/check_database_connection", params) + } + fn get_completions( &self, params: super::GetCompletionsParams, diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index a6b7534b0..e66a8c225 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -50,7 +50,10 @@ use crate::{ CommandActionCategory, ExecuteStatementParams, ExecuteStatementResult, }, completions::{CompletionsResult, GetCompletionsParams, get_statement_for_completions}, - diagnostics::{PullDiagnosticsResult, PullFileDiagnosticsParams}, + diagnostics::{ + CheckDatabaseConnectionParams, CheckDatabaseConnectionResult, PullDiagnosticsResult, + PullFileDiagnosticsParams, + }, format::{PullFileFormattingParams, PullFormattingResult, StatementFormatResult}, on_hover::{OnHoverParams, OnHoverResult}, }, @@ -77,6 +80,8 @@ mod connection_manager; pub(crate) mod document; mod migration; mod pg_query; +#[cfg(feature = "db")] +mod pgpass; mod snapshot_manager; mod sql_function; mod statement_identifier; @@ -1202,6 +1207,38 @@ impl Workspace for WorkspaceServer { }) } + #[cfg(feature = "db")] + fn check_database_connection( + &self, + _params: CheckDatabaseConnectionParams, + ) -> Result { + let settings = self.workspaces(); + let Some(settings) = settings.settings() else { + return Ok(CheckDatabaseConnectionResult::default()); + }; + + let result = self + .connection + .try_with_pool(&settings.db, |pool| self.snapshot.load(pool).map(|_| ())); + match result { + None | Some(Ok(())) => Ok(CheckDatabaseConnectionResult::default()), + Some(Err(err @ WorkspaceError::DatabaseConnectionError(_))) => { + Ok(CheckDatabaseConnectionResult { + error: Some(err.to_string()), + }) + } + Some(Err(err)) => Err(err), + } + } + + #[cfg(not(feature = "db"))] + fn check_database_connection( + &self, + _params: CheckDatabaseConnectionParams, + ) -> Result { + Ok(CheckDatabaseConnectionResult::default()) + } + #[cfg(not(feature = "db"))] fn pull_db_diagnostics( &self, diff --git a/crates/pgls_workspace/src/workspace/server/connection_manager.rs b/crates/pgls_workspace/src/workspace/server/connection_manager.rs index 7fa3ab51c..0f3767d51 100644 --- a/crates/pgls_workspace/src/workspace/server/connection_manager.rs +++ b/crates/pgls_workspace/src/workspace/server/connection_manager.rs @@ -6,9 +6,14 @@ use std::time::{Duration, Instant}; use sqlx::{PgPool, Postgres, pool::PoolOptions, postgres::PgConnectOptions}; -use crate::{WorkspaceError, settings::DatabaseSettings}; +use crate::{ + WorkspaceError, + diagnostics::DatabaseConnectionError, + settings::{DEFAULT_DATABASE_PASSWORD, DatabaseSettings}, +}; use super::connection_key::ConnectionKey; +use super::pgpass::{PassfileTarget, password_from_passfile}; const INITIAL_FAILURE_BACKOFF: Duration = Duration::from_secs(5); const MAX_FAILURE_BACKOFF: Duration = Duration::from_secs(60); @@ -24,6 +29,48 @@ fn settings_fingerprint(settings: &DatabaseSettings) -> u64 { hasher.finish() } +/// The configured password, else the one from the password file, else the default. +fn password(settings: &DatabaseSettings) -> String { + settings + .password + .clone() + .or_else(|| { + password_from_passfile(&PassfileTarget { + host: &settings.host, + port: settings.port, + database: &settings.database, + username: &settings.username, + }) + }) + .unwrap_or_else(|| DEFAULT_DATABASE_PASSWORD.to_string()) +} + +/// sqlx looks up the password file for a connection string too, but unlike libpq it does not +/// unescape the password and falls back to `~/.pgpass` when `PGPASSFILE` has no entry. Prefer +/// the libpq lookup when the connection string and `PGPASSWORD` have no password. +fn connection_string_options(uri: &str) -> Result { + let options = PgConnectOptions::from_str(uri)?; + let url = url::Url::parse(uri).map_err(|err| sqlx::Error::Configuration(err.into()))?; + let has_password = url.password().is_some() + || url.query_pairs().any(|(key, _)| key == "password") + || std::env::var_os("PGPASSWORD").is_some_and(|password| !password.is_empty()); + if has_password { + return Ok(options); + } + + let password = password_from_passfile(&PassfileTarget { + host: options.get_host(), + port: options.get_port(), + // Postgres connects to the database named like the user by default. + database: options.get_database().unwrap_or(options.get_username()), + username: options.get_username(), + }); + Ok(match password { + Some(password) => options.password(&password), + None => options, + }) +} + /// Cached connection pool with last access time struct CachedPool { pool: PgPool, @@ -33,6 +80,7 @@ struct CachedPool { } struct CachedFailure { + error: DatabaseConnectionError, message: String, settings_fingerprint: u64, attempts: u32, @@ -112,7 +160,7 @@ impl ConnectionManager { // Create a new pool let config = if let Some(uri) = settings.connection_string.as_ref() { - match PgConnectOptions::from_str(uri) { + match connection_string_options(uri) { Ok(options) => options, Err(err) => { tracing::error!("Failed to parse database connection URI: {err}"); @@ -120,11 +168,11 @@ impl ConnectionManager { } } } else { - PgConnectOptions::new() + PgConnectOptions::new_without_pgpass() .host(&settings.host) .port(settings.port) .username(&settings.username) - .password(&settings.password) + .password(&password(settings)) .database(&settings.database) }; @@ -148,6 +196,25 @@ impl ConnectionManager { Some(pool) } + /// Like [`Self::with_pool`], but returns the error of a connection that failed recently + /// instead of skipping it during the backoff. + pub(crate) fn try_with_pool( + &self, + settings: &DatabaseSettings, + operation: impl FnOnce(&PgPool) -> Result, + ) -> Option> { + if settings.enable_connection + && let Some(error) = self.failure_in_backoff( + &ConnectionKey::from(settings), + settings_fingerprint(settings), + ) + { + return Some(Err(WorkspaceError::DatabaseConnectionError(error))); + } + + self.with_pool(settings, operation) + } + pub(crate) fn with_pool( &self, settings: &DatabaseSettings, @@ -168,14 +235,20 @@ impl ConnectionManager { ) { match result { Ok(_) => self.clear_failure(key), - Err(err @ WorkspaceError::DatabaseConnectionError(_)) => { - self.record_failure(key, settings_fingerprint, &err.to_string()); + Err(err @ WorkspaceError::DatabaseConnectionError(error)) => { + self.record_failure(key, settings_fingerprint, error, &err.to_string()); } Err(_) => {} } } - fn record_failure(&self, key: &ConnectionKey, settings_fingerprint: u64, error: &str) { + fn record_failure( + &self, + key: &ConnectionKey, + settings_fingerprint: u64, + error: &DatabaseConnectionError, + message: &str, + ) { let mut failures = self.failures.write().unwrap(); let now = Instant::now(); let attempts = failures.get(key).map_or(1, |failure| failure.attempts + 1); @@ -190,7 +263,8 @@ impl ConnectionManager { failures.insert( key.clone(), CachedFailure { - message: error.to_string(), + error: error.clone(), + message: message.to_string(), settings_fingerprint, attempts, next_retry_at: now + backoff, @@ -199,29 +273,36 @@ impl ConnectionManager { if was_cached { tracing::debug!( - "Database connection failed again. Retrying after {:?}: {error}", + "Database connection failed again. Retrying after {:?}: {message}", backoff ); } else { tracing::warn!( - "Database connection failed. Skipping database-backed features for {:?}: {error}", + "Database connection failed. Skipping database-backed features for {:?}: {message}", backoff ); } } fn connection_is_in_backoff(&self, key: &ConnectionKey, settings_fingerprint: u64) -> bool { + self.failure_in_backoff(key, settings_fingerprint).is_some() + } + + /// The failure that holds back the next connection attempt, if any. + fn failure_in_backoff( + &self, + key: &ConnectionKey, + settings_fingerprint: u64, + ) -> Option { let mut failures = self.failures.write().unwrap(); if failures .get(key) .is_some_and(|failure| failure.settings_fingerprint != settings_fingerprint) { failures.remove(key); - return false; + return None; } - let Some(failure) = failures.get(key) else { - return false; - }; + let failure = failures.get(key)?; let now = Instant::now(); if now < failure.next_retry_at { @@ -230,10 +311,10 @@ impl ConnectionManager { failure.next_retry_at, failure.message ); - return true; + return Some(failure.error.clone()); } - false + None } fn clear_failure(&self, key: &ConnectionKey) { @@ -273,7 +354,7 @@ mod tests { let first = manager.get_pool(&settings).unwrap(); first.close().await; - settings.password = "changed-password".to_string(); + settings.password = Some("changed-password".to_string()); let changed_fingerprint = settings_fingerprint(&settings); let pool = manager.get_pool(&settings).unwrap(); @@ -298,6 +379,7 @@ mod tests { manager.failures.write().unwrap().insert( key.clone(), CachedFailure { + error: authentication_failed(), message: "authentication failed".to_string(), settings_fingerprint: old_fingerprint, attempts: 1, @@ -305,10 +387,38 @@ mod tests { }, ); - settings.password = "corrected-password".to_string(); + settings.password = Some("corrected-password".to_string()); let new_fingerprint = settings_fingerprint(&settings); assert!(!manager.connection_is_in_backoff(&key, new_fingerprint)); assert!(!manager.failures.read().unwrap().contains_key(&key)); assert!(manager.get_pool(&settings).is_some()); } + + fn authentication_failed() -> DatabaseConnectionError { + DatabaseConnectionError { + message: "password authentication failed".to_string(), + code: Some("28P01".to_string()), + } + } + + #[test] + fn try_with_pool_reports_the_failure_during_backoff() { + let manager = ConnectionManager::new(); + let settings = settings(); + let key = ConnectionKey::from(&settings); + manager.record_failure( + &key, + settings_fingerprint(&settings), + &authentication_failed(), + "authentication failed", + ); + + assert!(manager.with_pool(&settings, |_| Ok(())).is_none()); + let result = manager.try_with_pool(&settings, |_| Ok(())); + assert!(matches!( + result, + Some(Err(WorkspaceError::DatabaseConnectionError(error))) + if error.message == "password authentication failed" + )); + } } diff --git a/crates/pgls_workspace/src/workspace/server/pgpass.rs b/crates/pgls_workspace/src/workspace/server/pgpass.rs new file mode 100644 index 000000000..b9127bcb4 --- /dev/null +++ b/crates/pgls_workspace/src/workspace/server/pgpass.rs @@ -0,0 +1,267 @@ +//! Looks up passwords in the password file, as libpq does. +//! +//! sqlx reads the password file too, but only while it builds the connection options from the +//! environment or a URL, and it does not tell whether it found a password. + +use std::path::{Path, PathBuf}; + +/// The connection a password is looked up for. +pub(crate) struct PassfileTarget<'a> { + pub host: &'a str, + pub port: u16, + pub database: &'a str, + pub username: &'a str, +} + +/// Looks up the password for the target in the password file: `PGPASSFILE`, or else +/// `~/.pgpass` (`%APPDATA%\postgresql\pgpass.conf` on Windows). +/// +/// Port of the lookup in `pqConnectOptions2`: +/// https://github.com/postgres/postgres/blob/REL_18_6/src/interfaces/libpq/fe-connect.c#L1423 +pub(crate) fn password_from_passfile(target: &PassfileTarget) -> Option { + password_from_file(&passfile_path()?, target) +} + +fn passfile_path() -> Option { + if let Some(path) = std::env::var_os("PGPASSFILE").filter(|path| !path.is_empty()) { + return Some(PathBuf::from(path)); + } + + #[cfg(windows)] + let path = std::env::var_os("APPDATA") + .map(|dir| PathBuf::from(dir).join("postgresql").join("pgpass.conf")); + #[cfg(not(windows))] + let path = std::env::home_dir().map(|dir| dir.join(".pgpass")); + + path +} + +/// Returns the password of the first entry in the password file at `path` that matches the +/// target, or `None` when there is none or the file cannot be used. +/// +/// Port of `passwordFromFile`: +/// https://github.com/postgres/postgres/blob/REL_18_6/src/interfaces/libpq/fe-connect.c#L7906 +pub(crate) fn password_from_file(path: &Path, target: &PassfileTarget) -> Option { + if target.database.is_empty() || target.username.is_empty() { + return None; + } + + // 'localhost' matches an empty host. + let host = if target.host.is_empty() { + "localhost" + } else { + target.host + }; + let port = target.port.to_string(); + + let content = read_passfile(path)?; + content + .split(|byte| *byte == b'\n') + .filter(|line| !line.starts_with(b"#")) + .map(|line| line.strip_suffix(b"\r").unwrap_or(line)) + .filter(|line| !line.is_empty()) + .find_map(|line| { + let rest = match_field(line, host.as_bytes())?; + let rest = match_field(rest, port.as_bytes())?; + let rest = match_field(rest, target.database.as_bytes())?; + let rest = match_field(rest, target.username.as_bytes())?; + Some(unescape_password(rest)) + }) + .and_then(|password| String::from_utf8(password).ok()) +} + +fn read_passfile(path: &Path) -> Option> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + let metadata = std::fs::metadata(path).ok()?; + if !metadata.is_file() { + tracing::warn!("Password file {} is not a plain file", path.display()); + return None; + } + if metadata.permissions().mode() & 0o077 != 0 { + tracing::warn!( + "Password file {} has group or world access; permissions should be u=rw (0600) or less", + path.display() + ); + return None; + } + } + + std::fs::read(path).ok() +} + +/// Matches the next field of an entry, which is `*` or the token with `:` and `\` escaped by +/// a backslash, and returns the rest of the entry. +/// +/// Port of `pwdfMatchesString`: +/// https://github.com/postgres/postgres/blob/REL_18_6/src/interfaces/libpq/fe-connect.c#L7869 +fn match_field<'a>(entry: &'a [u8], mut token: &[u8]) -> Option<&'a [u8]> { + if let Some(rest) = entry.strip_prefix(b"*:") { + return Some(rest); + } + + let mut bytes = entry.iter().enumerate(); + while let Some((index, &byte)) = bytes.next() { + let (index, byte, escaped) = if byte == b'\\' { + let (index, &byte) = bytes.next()?; + (index, byte, true) + } else { + (index, byte, false) + }; + + if byte == b':' && !escaped && token.is_empty() { + return Some(&entry[index + 1..]); + } + let (&expected, rest) = token.split_first()?; + if byte != expected { + return None; + } + token = rest; + } + + None +} + +/// The password is the rest of the entry up to an unescaped `:`. +fn unescape_password(field: &[u8]) -> Vec { + let mut password = Vec::with_capacity(field.len()); + let mut index = 0; + while index < field.len() && field[index] != b':' { + if field[index] == b'\\' && index + 1 < field.len() { + index += 1; + } + password.push(field[index]); + index += 1; + } + password +} + +#[cfg(test)] +mod tests { + use super::*; + + const TARGET: PassfileTarget<'static> = PassfileTarget { + host: "db.example.com", + port: 5432, + database: "app", + username: "alice", + }; + + fn passfile(content: &str) -> tempfile::NamedTempFile { + let file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(file.path(), content).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(file.path(), std::fs::Permissions::from_mode(0o600)).unwrap(); + } + file + } + + fn lookup(content: &str, target: &PassfileTarget) -> Option { + let file = passfile(content); + password_from_file(file.path(), target) + } + + #[test] + fn returns_the_password_of_the_first_matching_entry() { + let content = "\ +db.example.com:5432:app:bob:not-alice +db.example.com:5433:app:alice:other-port +other.example.com:5432:app:alice:other-host +db.example.com:5432:other:alice:other-database +db.example.com:5432:app:alice:secret +db.example.com:5432:app:alice:second-match +"; + assert_eq!(lookup(content, &TARGET).as_deref(), Some("secret")); + } + + #[test] + fn wildcards_match_any_value() { + assert_eq!( + lookup("*:*:*:alice:secret\n", &TARGET).as_deref(), + Some("secret") + ); + assert_eq!( + lookup("*:*:*:*:secret\n", &TARGET).as_deref(), + Some("secret") + ); + } + + #[test] + fn skips_comments_and_blank_lines() { + let content = "# db.example.com:5432:app:alice:commented\n\n*:*:*:*:secret\r\n"; + assert_eq!(lookup(content, &TARGET).as_deref(), Some("secret")); + assert_eq!( + lookup("*:*:*:*:trailing space \n", &TARGET).as_deref(), + Some("trailing space ") + ); + } + + #[test] + fn unescapes_colons_and_backslashes() { + let target = PassfileTarget { + database: "a:b", + username: "c\\d", + ..TARGET + }; + assert_eq!( + lookup("*:*:a\\:b:c\\\\d:pass\\:wo\\\\rd\n", &target).as_deref(), + Some("pass:wo\\rd") + ); + } + + #[test] + fn password_ends_at_an_unescaped_colon() { + assert_eq!( + lookup("*:*:*:*:secret:trailing\n", &TARGET).as_deref(), + Some("secret") + ); + } + + #[test] + fn empty_host_matches_localhost() { + let target = PassfileTarget { host: "", ..TARGET }; + assert_eq!( + lookup("localhost:5432:app:alice:secret\n", &target).as_deref(), + Some("secret") + ); + } + + #[test] + fn host_is_compared_literally() { + let target = PassfileTarget { + host: "127.0.0.1", + ..TARGET + }; + assert_eq!(lookup("localhost:5432:app:alice:secret\n", &target), None); + } + + #[test] + fn missing_file_has_no_password() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!( + password_from_file(&dir.path().join("pgpass"), &TARGET), + None + ); + } + + #[cfg(unix)] + #[test] + fn ignores_a_file_others_can_access() { + use std::os::unix::fs::PermissionsExt; + + let file = passfile("*:*:*:*:secret\n"); + std::fs::set_permissions(file.path(), std::fs::Permissions::from_mode(0o640)).unwrap(); + assert_eq!(password_from_file(file.path(), &TARGET), None); + } + + #[cfg(unix)] + #[test] + fn ignores_a_path_that_is_not_a_file() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!(password_from_file(dir.path(), &TARGET), None); + } +} diff --git a/docs/codegen/src/env_variables.rs b/docs/codegen/src/env_variables.rs index f19d4badb..a5405b7c4 100644 --- a/docs/codegen/src/env_variables.rs +++ b/docs/codegen/src/env_variables.rs @@ -69,6 +69,12 @@ pub fn generate_env_variables(docs_dir: &Path) -> Result<()> { env.pgpassword.name(), env.pgpassword.description() )?; + writeln!( + content, + "### `{}`\n\n {}\n", + env.pgpassfile.name(), + env.pgpassfile.description() + )?; writeln!( content, "### `{}`\n\n {}\n", diff --git a/docs/guides/configure_database.md b/docs/guides/configure_database.md index 9449b7c6c..8350807e7 100644 --- a/docs/guides/configure_database.md +++ b/docs/guides/configure_database.md @@ -24,7 +24,7 @@ Configure database connection details in your `postgres-language-server.jsonc` f "port": 5432, // Database username (default: "postgres") "username": "postgres", - // Database password (default: "postgres") + // Database password (default: `PGPASSWORD`, then the password file, then "postgres") "password": "your_password", // Database name to connect to (default: "postgres") "database": "your_database_name", @@ -38,6 +38,24 @@ Configure database connection details in your `postgres-language-server.jsonc` f } ``` +## Password file + +When neither the configuration, the connection string, nor `PGPASSWORD` provides a password, the password is looked up in the [password file](https://www.postgresql.org/docs/current/libpq-pgpass.html), the same way `psql` and other libpq clients do: + +- The file is `PGPASSFILE` if set, else `~/.pgpass` (`%APPDATA%\postgresql\pgpass.conf` on Windows). +- Each line is `hostname:port:database:username:password`. `*` matches any value, and `:` and `\` in a field are escaped with `\`. The first line that matches the host, port, database, and username of the connection wins. +- The host is compared literally: an entry for `localhost` does not match `"host": "127.0.0.1"`. +- On Unix-like systems such as Linux and macOS, the file is ignored if its group or others can access it. Restrict it with `chmod 0600 ~/.pgpass`. + +If no entry matches, the connection uses the default password `postgres`, except with a connection string, which then connects without a password. + +## Connection failures + +When a database is configured (`host` or `connectionString`, or `PGHOST` or `DATABASE_URL` in the environment), `postgres-language-server check` treats a failed connection or login as an error: it reports a `database/connection` diagnostic, still runs the checks that do not need the database, and exits with a non-zero code. This way a CI run cannot pass with the database-backed checks silently skipped. + +To check without a database, disable the connection with `--disable-db` or `"disableConnection": true` (see [Disabling Database Features](#disabling-database-features)). + +The language server keeps working without the database when the connection fails, and retries it in the background. ## Setting the connection from the editor diff --git a/docs/reference/env_variables.md b/docs/reference/env_variables.md index 47ab110b1..fbff0511a 100644 --- a/docs/reference/env_variables.md +++ b/docs/reference/env_variables.md @@ -39,6 +39,10 @@ The password to connect to the database. +### `PGPASSFILE` + + The password file to look up the password in when none is configured. Default: `~/.pgpass`. + ### `PGDATABASE` The name of the database to connect to. From cb1eaa3b503162ded87a5704e9d744c942ce8af7 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 5 Oct 2026 21:18:26 +0000 Subject: [PATCH 2/2] test: gate the linux-only fixture constant --- crates/pgls_cli/tests/assert_check.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/pgls_cli/tests/assert_check.rs b/crates/pgls_cli/tests/assert_check.rs index 631d729b2..53c48a6d3 100644 --- a/crates/pgls_cli/tests/assert_check.rs +++ b/crates/pgls_cli/tests/assert_check.rs @@ -222,6 +222,7 @@ async fn check_accepts_tls_connection_strings(test_db: PgPool) { std::fs::remove_file(sql_file).expect("failed to remove temporary SQL file"); } +#[cfg(target_os = "linux")] const UNKNOWN_COLUMN_PATH: &str = "tests/fixtures/unknown_column.sql"; #[cfg(target_os = "linux")]