diff --git a/laravel-lsp/src/cache_manager.rs b/laravel-lsp/src/cache_manager.rs index e74fa9a6..a3641755 100644 --- a/laravel-lsp/src/cache_manager.rs +++ b/laravel-lsp/src/cache_manager.rs @@ -32,7 +32,28 @@ use tracing::{debug, info, warn}; /// paths/namespaces). v4 caches lack them, so namespaced components would /// resolve as "not found" until a provider edit forced a rebuild — drop /// v4 on read so the namespace maps are indexed and cached up front. -const CACHE_VERSION: u32 = 5; +/// v6: Env variables are cached with their secrets removed (issue #344). +/// A name matching `completion_display::is_sensitive_env_name` is stored +/// with an empty value instead of its plaintext, and any *other* value is +/// stored through `completion_display::mask_url_credentials`, so a +/// `DATABASE_URL=mysql://user:pass@host/db` — a name no segment matches — +/// reaches the file with its password masked. A v5 cache was written by a +/// binary that stored `DB_PASSWORD` and friends in cleartext, so it holds +/// secrets this version refuses to write — drop it on read rather than +/// keep serving them, and let the rescan rebuild a redacted one. +/// (The value-shape half arrived while v6 was still unmerged, so no +/// released binary ever wrote a v6 cache without it; no further bump.) +const CACHE_VERSION: u32 = 6; + +/// The redaction bump must never be walked back (issue #344). +/// +/// Every cache at v5 or below was written by a binary that stored `.env` values +/// in cleartext, `DB_PASSWORD` included, and dropping a cache whose version +/// differs from `CACHE_VERSION` is the entire mechanism by which those files +/// stop being read. Lowering the constant would silently make them live again, +/// so make it a build failure rather than something a reviewer has to notice — +/// the same guard `completion_display` puts on its two display budgets. +const _: () = assert!(CACHE_VERSION >= 6); /// Get the XDG-compliant cache directory for a project /// @@ -197,6 +218,15 @@ pub struct CachedLaravelConfig { } /// Cached environment variables +/// +/// A name matching `completion_display::is_sensitive_env_name` is stored with +/// an **empty** value: the name is still needed on a warm start, the plaintext +/// never is (issue #344). Every other value goes through +/// `completion_display::mask_url_credentials` first, because a name the +/// predicate clears can still carry a credential inside its value +/// (`DATABASE_URL`). The writer in `main.rs` enforces both; readers must not +/// treat an empty value as "unset", because the surfaces redact by name +/// anyway. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct CachedEnvVars { pub variables: HashMap, diff --git a/laravel-lsp/src/completion_display.rs b/laravel-lsp/src/completion_display.rs index 69f731da..c3886092 100644 --- a/laravel-lsp/src/completion_display.rs +++ b/laravel-lsp/src/completion_display.rs @@ -1,4 +1,7 @@ -//! Length-split rendering for config and translation completion items. +//! Length-split rendering for config and translation completion items, plus +//! the two shared gates every `.env` value display consults: a name gate +//! ([`is_sensitive_env_name`]) and a value-shape gate +//! ([`mask_url_credentials`]). //! //! A completion item shows its value in two places that want two different //! lengths: @@ -24,6 +27,112 @@ use crate::completion_format::{CodeBlock, CompletionDoc}; use crate::display_truncate::truncate_for_display; +use std::borrow::Cow; + +/// What every *client-rendered* surface prints in place of a sensitive `.env` +/// value. +/// +/// One constant rather than a per-site literal: the four surfaces that used to +/// echo dotenv values (env completion, `.env` hover, `config('…')` completion, +/// and the warm-start disk cache) are meant to be indistinguishable to a +/// reader, and a second spelling is how they drift apart (issue #344). +/// +/// The server log is the one masked surface that does *not* use this string. +/// It renders `(set)` — see `database::mask_env_value_for_log` — because a log +/// line is read as a diagnostic, not as a popup, and that file already masked +/// the resolved DB password in exactly that spelling. +pub const REDACTED_ENV_VALUE: &str = "(redacted — matches sensitive-name pattern)"; + +/// Name segments that mark a `.env` variable as secret-bearing. +/// +/// Matched as whole `_`-delimited segments, never as substrings — `AUTHOR_NAME` +/// and `TOKENIZE_INPUT` are ordinary settings that a `contains` test would +/// redact for no reason. +const SENSITIVE_ENV_SEGMENTS: [&str; 8] = [ + "KEY", + "SECRET", + "PASSWORD", + "TOKEN", + "CREDENTIAL", + "PRIVATE", + "AUTH", + "PWD", +]; + +/// Whether a `.env` variable's *value* must never be rendered. +/// +/// A Laravel `.env` routinely holds `APP_KEY`, `DB_PASSWORD`, `MAIL_PASSWORD` +/// and third-party API tokens, and every surface that echoed them did so in a +/// popup most likely to be on screen during a screen-share or a recording +/// (issue #344). The heuristic is deliberately name-based and shared: a custom +/// name outside these segments still shows its value, but no surface may decide +/// that question for itself — including the server log, which is as visible in +/// a screen-share as any popup (`database::mask_env_value_for_log`). +/// +/// A name gate cannot see a credential that lives *inside* the value, so it is +/// only half the policy: every caller pairs it with [`mask_url_credentials`], +/// which catches `DATABASE_URL=mysql://user:hunter2@host/db` — a name this +/// function correctly returns `false` for. +/// +/// Case-insensitive, because `.env` keys are conventionally upper-case but +/// nothing enforces it. +pub fn is_sensitive_env_name(name: &str) -> bool { + name.split('_').any(|segment| { + SENSITIVE_ENV_SEGMENTS + .iter() + .any(|keyword| segment.eq_ignore_ascii_case(keyword)) + }) +} + +/// Mask a credential carried *inside* a `.env` value, whatever the variable is +/// called. +/// +/// [`is_sensitive_env_name`] reads the variable's **name**, and stock Laravel +/// ships `'url' => env('DATABASE_URL')` (and `env('REDIS_URL')`) whose value is +/// `mysql://user:hunter2@host/db`: the password lives in the value, and +/// `DATABASE_URL` splits to `DATABASE` / `URL`, matching none of +/// [`SENSITIVE_ENV_SEGMENTS`]. The two gates are complementary and every +/// surface applies both — the name gate drops a value whose *name* says it is +/// secret, this one masks a credential the value's own *shape* reveals +/// (issue #344). +/// +/// Matches the standard `scheme://user:password@host…` shape and replaces the +/// password with `***`. Best-effort and fail-open: a value with no `://`, no +/// `@`, or no `:` in its credentials comes back borrowed and untouched. A +/// display surface must not blank a value it merely failed to parse, and a log +/// line must not panic the server. +/// +/// Only the first `@` after the credentials is treated as the host separator, +/// so an `@` inside the password leaves its tail visible +/// (`postgres://user:***@ssw0rd@host/db`). Deliberate: the characters that +/// identify the credential are gone, and a stricter parse would trade a +/// diagnostic that works on real URLs for one that fails on unusual ones. +/// +/// Lives here rather than in `database`, where it started life as +/// `mask_url_password`: it is now the second half of the redaction policy the +/// four display surfaces and the server log share, and a second copy is how the +/// two spellings drift apart. +pub fn mask_url_credentials(value: &str) -> Cow<'_, str> { + // Find the `://` separator, then the `@` that ends the credentials. + let Some(scheme_end) = value.find("://") else { + return Cow::Borrowed(value); + }; + let creds_start = scheme_end + 3; + let Some(at_offset) = value[creds_start..].find('@') else { + return Cow::Borrowed(value); + }; + let creds_end = creds_start + at_offset; + // Credentials are `user[:password]`. Only mask if there is a `:`. + let Some(colon_offset) = value[creds_start..creds_end].find(':') else { + return Cow::Borrowed(value); + }; + let user_end = creds_start + colon_offset; + let mut masked = String::with_capacity(value.len()); + masked.push_str(&value[..user_end + 1]); // up to and including the `:` + masked.push_str("***"); + masked.push_str(&value[creds_end..]); // from the `@` onwards + Cow::Owned(masked) +} /// Char budget for the inline `detail` line beside a completion label. /// diff --git a/laravel-lsp/src/completion_display/tests.rs b/laravel-lsp/src/completion_display/tests.rs index ce259aff..020586ff 100644 --- a/laravel-lsp/src/completion_display/tests.rs +++ b/laravel-lsp/src/completion_display/tests.rs @@ -217,3 +217,133 @@ fn the_two_budgets_are_the_tuned_values() { assert_eq!(COMPLETION_DETAIL_LIMIT, 50); assert_eq!(COMPLETION_DOC_LIMIT, 200); } + +// ---- the sensitive-name gate (issue #344) -------------------------------- + +/// Run a fixture table through the predicate and report **every** row that +/// disagrees, not merely the first. +/// +/// A bare `assert!` per row inside a loop stops at the first failure, so +/// deleting one keyword from `SENSITIVE_ENV_SEGMENTS` would credit only one +/// fixture and hide the rest of the damage. Collecting first makes each row +/// discriminate on its own. +fn mismatches(names: &[&str], expected: bool) -> Vec { + names + .iter() + .filter(|name| is_sensitive_env_name(name) != expected) + .map(|name| format!("{name} (expected {expected})")) + .collect() +} + +/// One positive fixture per keyword, so removing any single entry from +/// `SENSITIVE_ENV_SEGMENTS` reddens this test. `AWS_SECRET_ACCESS_KEY` carries +/// two keywords deliberately — it is the real-world shape — but every keyword +/// also has a fixture that isolates it. +#[test] +fn every_sensitive_keyword_redacts_on_its_own() { + let names = [ + "APP_KEY", + "APP_SECRET", + "DB_PASSWORD", + "MAIL_PASSWORD", + "API_TOKEN", + "GOOGLE_CREDENTIAL_FILE", + "JWT_PRIVATE", + "BASIC_AUTH", + "DB_PWD", + "AWS_SECRET_ACCESS_KEY", + ]; + assert_eq!(mismatches(&names, true), Vec::::new()); +} + +/// Ordinary settings keep their values. The second group is the point of +/// splitting on `_`: every one of these *contains* a keyword as a substring, +/// and a `contains`-based predicate would redact all seven. +#[test] +fn ordinary_names_are_not_redacted() { + let plain = ["APP_NAME", "DB_CONNECTION", "APP_DEBUG"]; + assert_eq!(mismatches(&plain, false), Vec::::new()); + + let substring_but_not_segment = [ + "PASSKEYBOARD_LAYOUT", + "AUTHOR_NAME", + "SECRETARY_ID", + "TOKENIZE_INPUT", + "PRIVATELY_OWNED", + "CREDENTIALED_USER", + "PWDLESS_LOGIN", + ]; + assert_eq!( + mismatches(&substring_but_not_segment, false), + Vec::::new() + ); +} + +/// `.env` keys are upper-case by convention and by nothing else, so a +/// lower-case or mixed-case declaration must redact identically. +#[test] +fn the_gate_ignores_case() { + let names = ["db_password", "Api_Token", "app_key"]; + assert_eq!(mismatches(&names, true), Vec::::new()); +} + +/// A single-segment name has no `_` to split on, and an empty name must not +/// match anything. +#[test] +fn single_segment_and_empty_names_are_handled() { + assert!(is_sensitive_env_name("PASSWORD")); + assert!(!is_sensitive_env_name("PASSWORDS")); + assert!(!is_sensitive_env_name("")); +} + +// ---- the value-shape gate (issue #344, round 2) -------------------------- + +/// The shape the name gate cannot see. `DATABASE_URL` splits to `DATABASE` / +/// `URL` — no segment matches — and stock Laravel's `config/database.php` reads +/// `'url' => env('DATABASE_URL')`, so this is the default configuration, not a +/// contrived one. +#[test] +fn a_credential_inside_the_value_is_masked_whatever_the_name_says() { + assert!( + !is_sensitive_env_name("DATABASE_URL"), + "the premise: the name gate lets this one through, so the shape gate is \ + the only thing standing between it and the screen" + ); + assert_eq!( + mask_url_credentials("mysql://sail:secret@127.0.0.1:3306/db"), + "mysql://sail:***@127.0.0.1:3306/db" + ); + assert_eq!( + mask_url_credentials("postgres://user:p@ssw0rd@host/db"), + // Only the first `@` after the credentials is treated as the host + // separator — best-effort. An `@` inside the password leaves its tail + // visible, but the characters that identify the credential are gone. + "postgres://user:***@ssw0rd@host/db" + ); + assert_eq!( + mask_url_credentials("redis://default:redis-secret@redis:6379"), + "redis://default:***@redis:6379" + ); +} + +/// Fail-open, and borrowed while it is at it: a value the parser does not +/// recognise is returned untouched rather than blanked. `Cow::Borrowed` is +/// asserted rather than just string equality because it is the observable proof +/// that the untouched path never rebuilt the string — the property that lets +/// every surface call this unconditionally. +#[test] +fn a_value_carrying_no_credential_is_returned_untouched() { + for value in [ + "Example", // an ordinary setting + "not a url", // no scheme + "mysql://sail@127.0.0.1/db", // credentials, but no password + "https://example.com/webhook", // no credentials at all + "sqlite:///absolute/path.sqlite", // scheme, no `@` + "", + ] { + assert!( + matches!(mask_url_credentials(value), std::borrow::Cow::Borrowed(v) if v == value), + "{value:?} must come back borrowed and unchanged" + ); + } +} diff --git a/laravel-lsp/src/database.rs b/laravel-lsp/src/database.rs index a4090ed6..e6421878 100644 --- a/laravel-lsp/src/database.rs +++ b/laravel-lsp/src/database.rs @@ -3,7 +3,9 @@ //! Provides database schema information (tables and columns) for //! `exists:` and `unique:` validation rule autocomplete. +use crate::completion_display::{is_sensitive_env_name, mask_url_credentials}; use regex::Regex; +use std::borrow::Cow; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; @@ -471,34 +473,35 @@ fn userinfo(user: &str, password: &str) -> String { } } -/// Mask the password in a database URL for safe logging. Matches the -/// standard shape `driver://user:pass@host:...` and replaces the password -/// segment with `***`. If no password is present (or the URL doesn't match -/// the expected shape), returns the input unchanged. +/// Render a `.env`-sourced value for a log line, through both redaction gates. /// -/// This is best-effort — failing gracefully is safer than failing hard, -/// since logging shouldn't crash the LSP. -fn mask_url_password(url: &str) -> String { - // Find the `://` separator, then the `@` that ends the credentials. - let Some(scheme_end) = url.find("://") else { - return url.to_string(); - }; - let creds_start = scheme_end + 3; - let Some(at_offset) = url[creds_start..].find('@') else { - return url.to_string(); - }; - let creds_end = creds_start + at_offset; - let creds = &url[creds_start..creds_end]; - // Credentials are `user[:password]`. Only mask if there's a `:`. - let Some(colon_offset) = creds.find(':') else { - return url.to_string(); - }; - let user_end = creds_start + colon_offset; - let mut masked = String::with_capacity(url.len()); - masked.push_str(&url[..user_end + 1]); // up to and including the `:` - masked.push_str("***"); - masked.push_str(&url[creds_end..]); // from `@` onwards - masked +/// Logs are a display surface. With `RUST_LOG` unset the server installs +/// `EnvFilter::new("info,salsa=warn")` over stderr, which Zed shows in a visible +/// log panel — the same screen-share exposure the completion, hover, `config()` +/// and warm-start-cache redaction closes (issue #344). A value read under a +/// variable name that [`is_sensitive_env_name`] matches therefore never reaches a +/// log in the clear. +/// +/// Name-matched values render `(set)`, the spelling +/// [`DatabaseSchemaProvider::parse_database_config`] already prints for the +/// resolved DB password. There is no `(empty)` arm: every call site logs a value +/// that came back from [`crate::config::read_env_value`], which filters an empty +/// value to `None` before it can get here. +/// +/// A name the predicate does not match is not therefore safe: Laravel's stock +/// `config/database.php` reads `'url' => env('DATABASE_URL')`, and that value +/// carries the password inside itself while its name matches no segment. So the +/// unmatched arm is not the raw value — it is +/// [`mask_url_credentials`], the same helper `parse_database_config` has always +/// applied to the assembled `url` line. `DB_HOST` and `DB_DATABASE` still log in +/// full: neither gate matches them, and redacting them would spend the whole +/// diagnostic for nothing. +fn mask_env_value_for_log<'a>(name: &str, value: &'a str) -> Cow<'a, str> { + if is_sensitive_env_name(name) { + Cow::Borrowed("(set)") + } else { + mask_url_credentials(value) + } } /// One thing the connector should attempt: a URL to connect with, a short @@ -1118,7 +1121,7 @@ impl DatabaseSchemaProvider { if let Some(u) = &url { // Mask the password in the URL when logging — common shape is // `driver://user:pass@host:port/db`. Best-effort, fail-open. - info!("🗄️ url: {}", mask_url_password(u)); + info!("🗄️ url: {}", mask_url_credentials(u)); } if let Some(s) = &unix_socket { info!("🗄️ unix_socket: {}", s); @@ -1255,7 +1258,10 @@ impl DatabaseSchemaProvider { // Try to resolve from .env first if let Some(env_value) = self.resolve_env(&env_var) { - info!("🗄️ → resolved from .env: {}", env_value); + info!( + "🗄️ → resolved from .env: {}", + mask_env_value_for_log(&env_var, &env_value) + ); return env_value; } @@ -1420,7 +1426,13 @@ impl DatabaseSchemaProvider { // Delegates to the single hardened reader in `config` — see its doc // comment for why this logic must not be duplicated. let result = crate::config::read_env_value(&self.project_root, key); - debug!("🗄️ resolve_env({}): {:?}", key, result); + debug!( + "🗄️ resolve_env({}): {:?}", + key, + result + .as_deref() + .map(|value| mask_env_value_for_log(key, value)) + ); result } @@ -1456,7 +1468,7 @@ impl DatabaseSchemaProvider { info!( "MySQL: trying candidate '{}' with url={}", cand.label, - mask_url_password(&cand.url) + mask_url_credentials(&cand.url) ); match MySqlPoolOptions::new() .max_connections(1) diff --git a/laravel-lsp/src/database/tests.rs b/laravel-lsp/src/database/tests.rs index aa65c490..ab018436 100644 --- a/laravel-lsp/src/database/tests.rs +++ b/laravel-lsp/src/database/tests.rs @@ -176,40 +176,6 @@ fn host_candidates_empty_no_fallback() { ); } -// ---- mask_url_password ---- - -#[test] -fn mask_url_password_with_credentials() { - use super::mask_url_password; - assert_eq!( - mask_url_password("mysql://sail:secret@127.0.0.1:3306/db"), - "mysql://sail:***@127.0.0.1:3306/db" - ); - assert_eq!( - mask_url_password("postgres://user:p@ssw0rd@host/db"), - // Only the first `@` after creds is treated as the host separator — - // best-effort. Any `@` in the password trips this, but it's a - // diagnostic helper, not security-critical. - "postgres://user:***@ssw0rd@host/db" - ); -} - -#[test] -fn mask_url_password_no_password_no_change() { - use super::mask_url_password; - // No `:` in creds → no password to mask. - assert_eq!( - mask_url_password("mysql://sail@127.0.0.1/db"), - "mysql://sail@127.0.0.1/db" - ); -} - -#[test] -fn mask_url_password_no_scheme_returns_input() { - use super::mask_url_password; - assert_eq!(mask_url_password("not a url"), "not a url"); -} - // ---- build_*_candidates (DB_URL / unix_socket / TCP priority) ---- fn make_config_with(url: Option<&str>, socket: Option<&str>, host: &str) -> super::DatabaseConfig { @@ -1803,3 +1769,339 @@ async fn decision_cloud_end_to_end_variable_config_plus_sail_override() { "Sail override 127.0.0.2 detected once the host is correct: {labels:?}" ); } + +// ---- logs are a display surface (issue #344) ---- +// +// The four surfaces #344 enumerated all render into the client. Logs are the +// fifth: with `RUST_LOG` unset the server logs at `info` to stderr, which Zed +// shows in a visible panel, and `docs/troubleshooting.md` asks users to paste +// that panel into bug reports. A `.env` value read under a secret-bearing name +// must therefore be masked there too, by the same predicate the other four use. + +/// A plaintext distinctive enough that finding it in a log cannot be a +/// coincidence. +const LOG_SECRET: &str = "hunter2-log-issue-344"; +/// A second matched keyword category. One category alone cannot tell the shared +/// predicate from a hard-coded `contains("PASSWORD")`. +const LOG_TOKEN: &str = "tok-log-issue-344"; +/// The unmatched control: an ordinary setting whose value must still be logged +/// in full, or the masking has bought security by deleting the diagnostic. +const LOG_PLAIN_HOST: &str = "db.internal.example"; + +/// The credential the *name* gate cannot see. `DATABASE_URL` matches no +/// segment of `SENSITIVE_ENV_SEGMENTS`, and Laravel's stock +/// `config/database.php` ships `'url' => env('DATABASE_URL')`, so this value +/// reaches the log on the default configuration of an ordinary project. +const LOG_URL_SECRET: &str = "url-hunter2-issue-344"; + +fn log_fixture_db_url() -> String { + format!("mysql://sail:{LOG_URL_SECRET}@{LOG_PLAIN_HOST}:3306/laravel") +} + +/// The same fixture with the credential masked — the positive control. Without +/// it the leak assertion would also pass on a log line that dropped the URL +/// altogether, which would be a lost diagnostic rather than a fix. +fn log_fixture_db_url_masked() -> String { + format!("mysql://sail:***@{LOG_PLAIN_HOST}:3306/laravel") +} + +/// `config/database.php` with the `url` setting Laravel ships by default. +const CONFIG_WITH_URL: &str = r#" env('DB_CONNECTION', 'mysql'), + 'connections' => [ + 'mysql' => [ + 'driver' => 'mysql', + 'url' => env('DATABASE_URL'), + 'host' => env('DB_HOST', '127.0.0.1'), + 'port' => env('DB_PORT', '3306'), + 'database' => env('DB_DATABASE', 'laravel'), + 'username' => env('DB_USERNAME', 'root'), + 'password' => env('DB_PASSWORD', ''), + ], + ], +]; +"#; + +fn log_fixture_env() -> String { + format!( + "DB_CONNECTION=mysql\nDB_HOST={LOG_PLAIN_HOST}\nDB_DATABASE=laravel\n\ + DB_USERNAME=sail\nDB_PASSWORD={LOG_SECRET}\nMAIL_API_TOKEN={LOG_TOKEN}\n" + ) +} + +thread_local! { + /// Everything logged on this thread while a capture is running. `None` + /// means this thread is not capturing, and the subscriber's output is + /// dropped — which is what every other test in the binary wants. + static CAPTURED_LOG: std::cell::RefCell>> = + const { std::cell::RefCell::new(None) }; +} + +/// The writer behind the one subscriber the test binary installs. It routes +/// each event to the capturing thread's buffer, or to nowhere. +#[derive(Clone, Copy, Default)] +struct ThreadLocalLogWriter; + +impl std::io::Write for ThreadLocalLogWriter { + fn write(&mut self, bytes: &[u8]) -> std::io::Result { + CAPTURED_LOG.with(|slot| { + if let Some(buffer) = slot.borrow_mut().as_mut() { + buffer.extend_from_slice(bytes); + } + }); + Ok(bytes.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for ThreadLocalLogWriter { + type Writer = Self; + + fn make_writer(&'a self) -> Self::Writer { + *self + } +} + +/// Run `f` and return its result plus everything it logged. +/// +/// The subscriber is **global and installed once**, rather than scoped per +/// call with `tracing::subscriber::with_default`. Scoping looks tidier and is +/// wrong here: `tracing` caches each callsite's `Interest` the first time that +/// callsite is reached, so an ordinary test touching `resolve_env` on another +/// thread — with no subscriber in place — caches "never" for that macro and +/// every later capture silently misses it. That is not theory: it turned this +/// suite red on one CI runner out of three while passing locally, with the +/// resolver's own lines absent from a capture that held its neighbours. +/// +/// A global subscriber makes every callsite register against a real subscriber +/// (and `set_global_default` rebuilds the interest cache), so the answer is +/// "yes" for good. Isolation moves to the buffer, which is per-thread: a test +/// that is not capturing writes into `None` and drops its output, exactly as it +/// did when no subscriber existed at all. +fn capture_logs(f: impl FnOnce() -> T) -> (T, String) { + static INSTALL: std::sync::Once = std::sync::Once::new(); + INSTALL.call_once(|| { + let subscriber = tracing_subscriber::fmt() + .with_writer(ThreadLocalLogWriter) + .with_max_level(tracing::Level::DEBUG) + .with_ansi(false) + .finish(); + tracing::subscriber::set_global_default(subscriber) + .expect("no other global subscriber in the test binary"); + }); + + CAPTURED_LOG.with(|slot| *slot.borrow_mut() = Some(Vec::new())); + let result = f(); + let captured = CAPTURED_LOG.with(|slot| { + String::from_utf8(slot.borrow_mut().take().unwrap_or_default()) + .expect("log output is utf-8") + }); + (result, captured) +} + +#[test] +fn parsing_the_database_config_never_logs_the_dotenv_password() { + let dir = TempDir::new().unwrap(); + write_config_php(dir.path(), CONFIG_VAR_FORM); + write(dir.path(), ".env", &log_fixture_env()); + let provider = DatabaseSchemaProvider::new(dir.path().to_path_buf()); + + let (config, logs) = capture_logs(|| provider.parse_database_config().expect("config parsed")); + + assert_eq!( + config.password, LOG_SECRET, + "the parse still resolves the real password — masking is a log concern only" + ); + assert!( + !logs.contains(LOG_SECRET), + "the .env password reached the log in plaintext:\n{logs}" + ); + // Pinned to the line that owns the value. A bare `(set)` search would pass + // on the neighbouring `password: (set)` summary, which was already masked + // before this change and proves nothing about these sites. + assert!( + logs.contains("resolved from .env: (set)"), + "the resolver's info line carries the masked rendering:\n{logs}" + ); + assert!( + logs.contains(r#"resolve_env(DB_PASSWORD): Some("(set)")"#), + "the reader's debug line carries the masked rendering:\n{logs}" + ); + assert!( + logs.contains(LOG_PLAIN_HOST), + "an ordinary DB_HOST value is still logged in full:\n{logs}" + ); +} + +/// The name gate's blind spot, closed by the shape gate. +/// +/// `DATABASE_URL` matches no sensitive segment, so `mask_env_value_for_log` +/// takes its *unmatched* arm — which is why that arm is `mask_url_credentials` +/// and not the raw value. The resolver's `info!` fires under the default +/// `EnvFilter("info,salsa=warn")`, so an unmasked password here is on screen in +/// Zed's log panel out of the box. +#[test] +fn parsing_the_database_config_never_logs_a_credential_inside_a_url_value() { + let dir = TempDir::new().unwrap(); + write_config_php(dir.path(), CONFIG_WITH_URL); + write( + dir.path(), + ".env", + &format!( + "{}DATABASE_URL={}\n", + log_fixture_env(), + log_fixture_db_url() + ), + ); + let provider = DatabaseSchemaProvider::new(dir.path().to_path_buf()); + + let (config, logs) = capture_logs(|| provider.parse_database_config().expect("config parsed")); + + assert_eq!( + config.url.as_deref(), + Some(log_fixture_db_url().as_str()), + "the parse still hands the driver the real URL — masking is a display concern only" + ); + assert!( + !logs.contains(LOG_URL_SECRET), + "the password inside DATABASE_URL reached the log in plaintext:\n{logs}" + ); + // Two lines print this value, and both must mask it: the resolver's + // `resolved from .env:` line and the config summary's `url:` line. Pinned + // separately, because masking one and not its sibling is precisely the + // shape this fix exists to close. + assert!( + logs.contains(&format!( + "resolved from .env: {}", + log_fixture_db_url_masked() + )), + "the resolver's line must carry the masked URL, not nothing and not the secret:\n{logs}" + ); + assert!( + logs.contains(&format!("url: {}", log_fixture_db_url_masked())), + "the config summary's url line must carry the masked URL:\n{logs}" + ); + // The unmatched control still logs in full, so the fix did not buy safety + // by blanking the diagnostic. + assert!( + logs.contains(&format!("host: {LOG_PLAIN_HOST}")), + "an ordinary DB_HOST value is still logged in full:\n{logs}" + ); +} + +/// The same gate on the reader itself, one `RUST_LOG=debug` away from the +/// default filter — the level an ordinary troubleshooting session turns on, and +/// whose output gets pasted into bug reports. +#[test] +fn resolve_env_masks_a_credential_inside_a_url_value_in_its_debug_log() { + let dir = TempDir::new().unwrap(); + write( + dir.path(), + ".env", + &format!("DATABASE_URL={}\n", log_fixture_db_url()), + ); + let provider = DatabaseSchemaProvider::new(dir.path().to_path_buf()); + + let (url, logs) = capture_logs(|| provider.resolve_env("DATABASE_URL")); + + assert_eq!( + url.as_deref(), + Some(log_fixture_db_url().as_str()), + "caller still gets the real URL" + ); + assert!( + !logs.contains(LOG_URL_SECRET), + "the password inside DATABASE_URL reached the debug log in plaintext:\n{logs}" + ); + assert!( + logs.contains(&format!( + r#"resolve_env(DATABASE_URL): Some("{}")"#, + log_fixture_db_url_masked() + )), + "the debug line must carry the masked URL:\n{logs}" + ); +} + +#[test] +fn resolve_env_masks_every_matched_keyword_category_in_its_debug_log() { + let dir = TempDir::new().unwrap(); + write(dir.path(), ".env", &log_fixture_env()); + let provider = DatabaseSchemaProvider::new(dir.path().to_path_buf()); + + let ((password, token, host), logs) = capture_logs(|| { + ( + provider.resolve_env("DB_PASSWORD"), + provider.resolve_env("MAIL_API_TOKEN"), + provider.resolve_env("DB_HOST"), + ) + }); + + assert_eq!( + password.as_deref(), + Some(LOG_SECRET), + "caller still gets it" + ); + assert_eq!(token.as_deref(), Some(LOG_TOKEN), "caller still gets it"); + assert_eq!( + host.as_deref(), + Some(LOG_PLAIN_HOST), + "caller still gets it" + ); + + assert!( + !logs.contains(LOG_SECRET) && !logs.contains(LOG_TOKEN), + "a secret-named value reached the debug log in plaintext:\n{logs}" + ); + assert!( + logs.contains(r#"resolve_env(DB_PASSWORD): Some("(set)")"#), + "PASSWORD is masked:\n{logs}" + ); + assert!( + logs.contains(r#"resolve_env(MAIL_API_TOKEN): Some("(set)")"#), + "TOKEN is masked by the same gate, not by a PASSWORD-only check:\n{logs}" + ); + assert!( + logs.contains(&format!( + r#"resolve_env(DB_HOST): Some("{LOG_PLAIN_HOST}")"# + )), + "an unmatched name still logs its value in full:\n{logs}" + ); +} + +#[test] +fn parse_env_setting_masks_by_the_env_var_name_not_the_config_key() { + let dir = TempDir::new().unwrap(); + write(dir.path(), ".env", &log_fixture_env()); + let provider = DatabaseSchemaProvider::new(dir.path().to_path_buf()); + // `host` is an innocuous config key fed by a secret-named variable, and + // `password` a secret-sounding key fed by an ordinary one: the gate reads + // the env var's name, which is the only name the policy is defined over. + let block = "'host' => env('MAIL_API_TOKEN', '127.0.0.1'),\n\ + 'password' => env('DB_HOST', ''),"; + + let ((host, password), logs) = capture_logs(|| { + ( + provider.parse_env_setting(block, "host", "127.0.0.1"), + provider.parse_env_setting(block, "password", ""), + ) + }); + + assert_eq!(host, LOG_TOKEN, "the resolved value is unchanged"); + assert_eq!(password, LOG_PLAIN_HOST, "the resolved value is unchanged"); + assert!( + !logs.contains(LOG_TOKEN), + "the secret-named value leaked through an innocuous config key:\n{logs}" + ); + assert!( + logs.contains("resolved from .env: (set)"), + "the secret-named value is masked:\n{logs}" + ); + assert!( + logs.contains(&format!("resolved from .env: {LOG_PLAIN_HOST}")), + "the ordinary value is logged in full even under a `password` key:\n{logs}" + ); +} diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index be52afae..016907a4 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -7727,7 +7727,28 @@ impl LaravelLanguageServer { if let Ok(env_vars) = self.salsa.get_all_parsed_env_vars().await { let mut variables = std::collections::HashMap::new(); for var in &env_vars { - variables.insert(var.name.clone(), var.value.clone()); + // A secret-bearing name is cached by name with an empty value + // (issue #344): the plaintext never reaches the cache file, + // which is world-readable-ish, long-lived, and outside the + // project the developer thinks they are protecting. The key is + // kept rather than dropped so a warm start still knows the + // variable exists and can offer it — the surfaces redact by + // name, so it renders identically to a live parse. + // + // No `is_commented` filter, matching the loop's existing + // behaviour: a commented-out `# DB_PASSWORD=hunter2` is cached + // too, so it must be redacted here as well. + // + // An unmatched name is still not written raw: `DATABASE_URL` + // matches no segment and carries the password inside its value, + // so it goes to disk with the credential masked. The plaintext + // must not reach this file under either gate. + let value = if laravel_lsp::completion_display::is_sensitive_env_name(&var.name) { + String::new() + } else { + laravel_lsp::completion_display::mask_url_credentials(&var.value).into_owned() + }; + variables.insert(var.name.clone(), value); } debug!("Caching {} env variables", variables.len()); cache.set_env_vars(CachedEnvVars { variables }); @@ -14900,12 +14921,33 @@ impl LaravelLanguageServer { } /// Resolve an env() call to its actual value - /// Handles: env('VAR'), env('VAR', 'default'), env('VAR', default_value) + /// Handles: env('VAR'), env('VAR', 'default'), env('VAR', default_value), + /// and the `(bool) env('VAR')` cast spelling. + /// + /// A value read out of the project's `.env` for a secret-bearing variable + /// name is replaced here, before it reaches `ConfigKeyCompletion` and the + /// two render sites, so the config popup cannot echo a credential + /// (issue #344). Only the dotenv-sourced path is redacted — a literal + /// default written in the PHP source is already on screen in the file being + /// edited, and the `${VAR}` placeholder carries no value to leak. + /// + /// **One branch, on purpose.** A `(bool) env('APP_DEBUG', false)` cast used + /// to have a second `bool_env_pattern` arm below this one. That arm was + /// unreachable: `env_pattern` is unanchored, so it matches the + /// `env('APP_DEBUG', false)` *substring* of the cast and returns before the + /// second pattern is ever consulted. Two arms that cannot both run are not + /// redundancy — they are a place for redaction to be fixed in one and + /// forgotten in the other, and a test naming "both branches" that only ever + /// drove one. The cast spelling still resolves and still redacts; it does so + /// through the single arm below, which + /// `config_completion_redacts_only_the_dotenv_sourced_sensitive_values` + /// pins with a `(bool) env(…)` fixture. fn resolve_env_value( value: &str, env_vars: &std::collections::HashMap, ) -> String { - // Match env('VAR_NAME') or env('VAR_NAME', default) or env("VAR_NAME", default) + // Match env('VAR_NAME') or env('VAR_NAME', default) or env("VAR_NAME", default). + // Unanchored, so a `(bool) env('VAR')` cast matches here too. let env_pattern = regex::Regex::new(r#"env\s*\(\s*['"]([A-Z_][A-Z0-9_]*)['"](?:\s*,\s*(.+))?\s*\)"#) .unwrap(); @@ -14915,7 +14957,7 @@ impl LaravelLanguageServer { // Try to get value from env vars if let Some(env_value) = env_vars.get(var_name) { - return env_value.clone(); + return Self::env_display_value(var_name, env_value); } // Fall back to default if provided @@ -14929,31 +14971,28 @@ impl LaravelLanguageServer { return format!("${{{}}}", var_name); } - // Check for (bool) env(...) pattern - let bool_env_pattern = regex::Regex::new( - r#"\(bool\)\s*env\s*\(\s*['"]([A-Z_][A-Z0-9_]*)['"](?:\s*,\s*(.+))?\s*\)"#, - ) - .unwrap(); - - if let Some(caps) = bool_env_pattern.captures(value) { - let var_name = caps.get(1).unwrap().as_str(); - - if let Some(env_value) = env_vars.get(var_name) { - return env_value.clone(); - } - - if let Some(default_match) = caps.get(2) { - let default = default_match.as_str().trim(); - return default.to_string(); - } - - return format!("${{{}}}", var_name); - } - // Not an env() call, clean up and return as-is value.trim_matches('\'').trim_matches('"').to_string() } + /// A dotenv-sourced value as it may be displayed: the shared redaction + /// string when the variable's name is secret-bearing, otherwise the value + /// with any credential embedded in it masked. + /// + /// Both gates, because either alone leaks: `DB_PASSWORD` is caught by its + /// name and `DATABASE_URL=mysql://user:hunter2@host/db` only by its shape. + /// The `env()` resolver above is this function's only caller, and it is the + /// only route from the dotenv map into `ConfigKeyCompletion`, so the two + /// render sites (`completion_detail` and `config_documentation`) receive a + /// value that is already safe to print. + fn env_display_value(var_name: &str, env_value: &str) -> String { + if laravel_lsp::completion_display::is_sensitive_env_name(var_name) { + laravel_lsp::completion_display::REDACTED_ENV_VALUE.to_string() + } else { + laravel_lsp::completion_display::mask_url_credentials(env_value).into_owned() + } + } + // ======================================================================== // Code Action helpers // ======================================================================== @@ -20220,7 +20259,9 @@ return [ } /// Env — value as a plain code block, link to the `.env` file it was - /// read from. Commented-out entries render as a detail note. + /// read from. Commented-out entries render as a detail note, and a + /// secret-bearing name renders the redaction note instead of its value + /// (issue #344). async fn hover_for_env(&self, name: &str) -> String { use laravel_lsp::hover; let var = self @@ -20242,11 +20283,23 @@ return [ source_link: Some(&link), ..Default::default() }) + } else if laravel_lsp::completion_display::is_sensitive_env_name(&var.name) { + // The value is dropped, not truncated or masked character-by-character: + // a partial credential is still a credential, and the hover has no + // second field to hide it in. + hover::render(&hover::HoverContent { + detail: Some(laravel_lsp::completion_display::REDACTED_ENV_VALUE), + source_link: Some(&link), + ..Default::default() + }) } else { + // Unmatched by name, but the value may still carry a credential of + // its own — `DATABASE_URL` is the stock Laravel case (issue #344). + let shown = laravel_lsp::completion_display::mask_url_credentials(&var.value); hover::render(&hover::HoverContent { code: Some(hover::CodeBlock { language: hover::CodeLanguage::Plain, - content: &var.value, + content: &shown, }), source_link: Some(&link), ..Default::default() @@ -26664,17 +26717,36 @@ impl LanguageServer for LaravelLanguageServer { .and_then(|n| n.to_str()) .unwrap_or(".env"); + // A secret-bearing name never shows its value here, whatever + // the value is (issue #344). The check precedes the empty-value + // display below on purpose: `DB_PASSWORD=` renders as redacted, + // not as `(empty)`, so the popup never distinguishes "unset" + // from "set" for a credential. + let sensitive = laravel_lsp::completion_display::is_sensitive_env_name(&v.name); + // The second gate, for a value whose *name* clears the first + // one: `DATABASE_URL` matches no segment, and stock Laravel + // fills it with `mysql://user:hunter2@host/db`. Identity for + // every value that is not a credential-bearing URL, so an + // ordinary variable renders exactly as it did. + let shown = laravel_lsp::completion_display::mask_url_credentials(&v.value); + CompletionItem { label: v.name.clone(), kind: Some(CompletionItemKind::VARIABLE), - detail: Some(format!("{} (from {})", v.value, source_file)), + detail: Some(if sensitive { + format!("(from {})", source_file) + } else { + format!("{} (from {})", shown, source_file) + }), documentation: Some( CompletionDoc::new() .header(&v.name) - .summary(if v.value.is_empty() { + .summary(if sensitive { + laravel_lsp::completion_display::REDACTED_ENV_VALUE.to_string() + } else if v.value.is_empty() { "(empty)".to_string() } else { - v.value.clone() + shown.to_string() }) .section(format!("Source: {}", source_file)) .into_documentation(), diff --git a/laravel-lsp/src/tests/env_completion_system_leak.rs b/laravel-lsp/src/tests/env_completion_system_leak.rs index a406abae..f85c846e 100644 --- a/laravel-lsp/src/tests/env_completion_system_leak.rs +++ b/laravel-lsp/src/tests/env_completion_system_leak.rs @@ -34,9 +34,9 @@ //! stays green under that mutation by design: its fixture `.env` declares the //! process variable's own name, so the deleted loop's own //! `!seen_names.contains(&name)` guard skipped that variable even before this -//! fix. A second mutation discriminates it — change the `.env` echo format -//! (`format!("{} (from {})", …)` in `completion()`) and its `detail` assertion -//! fails. +//! fix. A second mutation discriminates it — that name is secret-bearing, so +//! dropping the redaction branch added for issue #344 changes its `detail` and +//! its documentation panel, and both assertions fail. use crate::LaravelLanguageServer; use std::path::PathBuf; @@ -65,7 +65,13 @@ const SECRET_VALUE: &str = "s3cr3t-value-set-only-by-issue-342-tests"; const TYPED_PREFIX: &str = "AWS_SECRET"; /// A variable the *project* declares, sharing `TYPED_PREFIX` with the secret. -const DECLARED_NAME: &str = "AWS_SECRET_DECLARED"; +/// +/// `SECRETARIAT` deliberately is not the segment `SECRET`: this control asserts +/// the untouched `.env` echo format, so its name must fall outside +/// `completion_display::is_sensitive_env_name` (issue #344) — which redacts by +/// whole `_`-delimited segment, and would otherwise blank the very value this +/// control is here to see. +const DECLARED_NAME: &str = "AWS_SECRETARIAT_REGION"; const DECLARED_VALUE: &str = "declared-in-dotenv"; /// `.env` fixture: one variable under the typed prefix, one outside it. Built @@ -457,14 +463,22 @@ async fn dotenv_declaration_shadowing_a_process_var_still_completes_from_the_fil "the declared variable must be offered exactly once, got {:?}", items.iter().map(|i| &i.label).collect::>() ); + // `AWS_SECRET_ACCESS_KEY_TEST` is secret-bearing, so issue #344 redacts its + // value on this surface. That is orthogonal to what this test is about — + // the declaration is still the one that wins, and it is still offered + // exactly once — but the rendering it asserts is now the redacted one. assert_eq!( shadowing[0].detail.as_deref(), - Some("dotenv-owns-this-name (from .env)"), - "the declared value and source file must be reported unchanged" + Some("(from .env)"), + "the source file must still be reported, with the value redacted" ); assert_eq!( documentation_markdown(shadowing[0], "shadowing"), - expected_documentation(SECRET_NAME, "dotenv-owns-this-name", ".env"), - "the documentation panel must report the declared value and source unchanged" + expected_documentation( + SECRET_NAME, + laravel_lsp::completion_display::REDACTED_ENV_VALUE, + ".env" + ), + "the documentation panel must report the redaction string and the source" ); } diff --git a/laravel-lsp/src/tests/env_value_redaction.rs b/laravel-lsp/src/tests/env_value_redaction.rs new file mode 100644 index 00000000..3cb8f894 --- /dev/null +++ b/laravel-lsp/src/tests/env_value_redaction.rs @@ -0,0 +1,653 @@ +//! Secret-bearing `.env` values never reach a rendered surface (issue #344). +//! +//! #342 closed the *process*-environment leak. The project's own `.env` is a +//! milder but real second source: a Laravel `.env` routinely holds `APP_KEY`, +//! `DB_PASSWORD`, `MAIL_PASSWORD` and third-party tokens, and four surfaces +//! echoed them — env completion, `.env` hover, `config('…')` completion, and +//! the warm-start disk cache. +//! +//! A fifth was found in review: the server log. It consults the same predicate +//! and is covered next to the code that logs, in `database::tests` — these four +//! are the client-rendered ones. +//! +//! All four now consult the same two gates: `is_sensitive_env_name`, which +//! reads the variable's **name**, and `mask_url_credentials`, which reads the +//! value's **shape**. The second exists because the first cannot see +//! `DATABASE_URL=mysql://user:pass@host/db` — a name matching no sensitive +//! segment, filled by stock Laravel's `'url' => env('DATABASE_URL')`, whose +//! password sits inside the value. These tests drive the real entry points — +//! `completion()`, `hover_for_env`, `get_all_config_keys`, and a genuine +//! `CacheManager` save/load round trip — with two different matched keyword +//! categories (`DB_PASSWORD` and `API_TOKEN`) plus the URL shape crossing every +//! one of them, so a surface that re-implemented either check locally and +//! drifted is caught by observed behaviour rather than by reading the diff. +//! +//! Leak assertions run over the **whole serialized response**, not the fields +//! this change touches: `insertText`, `label`, `sortText` and friends are just +//! as visible in a screen-share as `detail` is. The cache assertion reads the +//! deserialized struct rather than the file's bytes, because a reversible +//! encoding of the value would pass a bytes-only check and still leak. +//! +//! Every leak assertion carries a positive control — the ordinary `APP_NAME` +//! renders exactly as it did before this change — so a fixture that never +//! reached the code under test cannot pass vacuously on an empty response. + +use crate::LaravelLanguageServer; +use laravel_lsp::cache_manager::CacheManager; +use laravel_lsp::completion_display::REDACTED_ENV_VALUE; +use std::path::{Path, PathBuf}; +use tempfile::TempDir; +use tower_lsp::lsp_types::{ + CompletionItem, CompletionParams, CompletionResponse, Documentation, MarkupContent, MarkupKind, + PartialResultParams, Position, TextDocumentIdentifier, TextDocumentPositionParams, Url, + WorkDoneProgressParams, +}; +use tower_lsp::{LanguageServer, LspService}; + +/// Two matched names from two different keyword categories. One category alone +/// cannot tell a shared predicate from a hard-coded `contains("PASSWORD")`. +const PASSWORD_NAME: &str = "DB_PASSWORD"; +const PASSWORD_VALUE: &str = "hunter2-issue-344"; +const TOKEN_NAME: &str = "API_TOKEN"; +const TOKEN_VALUE: &str = "tok-issue-344-only"; + +/// The unmatched control. Its value must survive every surface untouched. +const PLAIN_NAME: &str = "APP_NAME"; +const PLAIN_VALUE: &str = "Example"; + +/// The name gate's blind spot: `DATABASE_URL` splits to `DATABASE` / `URL` and +/// matches no sensitive segment, yet stock Laravel's `config/database.php` +/// reads `'url' => env('DATABASE_URL')` and the value carries the password +/// inside itself. Caught by shape, not by name. +const URL_NAME: &str = "DATABASE_URL"; +const URL_SECRET: &str = "url-hunter2-issue-344"; + +fn url_value() -> String { + format!("mysql://sail:{URL_SECRET}@db.internal.example:3306/laravel") +} + +/// What every surface must render instead: masked, not dropped. The host, port +/// and database are the whole reason a developer looks at this value. +fn url_masked() -> String { + "mysql://sail:***@db.internal.example:3306/laravel".to_string() +} + +/// A matched name that is *commented out* in the fixture. The client-visible +/// surfaces skip commented entries already; the cache-write loop never did. +const COMMENTED_NAME: &str = "MAIL_PASSWORD"; +const COMMENTED_VALUE: &str = "mail-hunter2-issue-344"; + +/// The project `.env` every test registers with Salsa. +fn dotenv() -> String { + format!( + "{PLAIN_NAME}={PLAIN_VALUE}\n{PASSWORD_NAME}={PASSWORD_VALUE}\n{TOKEN_NAME}={TOKEN_VALUE}\n{URL_NAME}={}\n# {COMMENTED_NAME}={COMMENTED_VALUE}\n", + url_value() + ) +} + +fn test_server() -> LaravelLanguageServer { + let (service, _socket) = LspService::new(LaravelLanguageServer::new); + service.inner().clone() +} + +/// A project on disk with `dotenv` registered with Salsa (priority 2, matching +/// `register_env_files_with_salsa`) and `root_path` primed, so both the env and +/// the `config('…')` completion branches are reachable. +async fn project(dotenv: &str) -> (TempDir, PathBuf, LaravelLanguageServer) { + let dir = TempDir::new().expect("tempdir"); + let root = dir.path().to_path_buf(); + let env_path = root.join(".env"); + std::fs::write(&env_path, dotenv).expect("write .env"); + + let server = test_server(); + *server.root_path.write().await = Some(root.clone()); + *server.auto_complete_debounce_ms.write().await = 0; + server + .salsa + .register_env_source(env_path, dotenv.to_string(), 2) + .await + .expect("register .env with salsa"); + (dir, root, server) +} + +/// Open `file_name` holding the single line `buffer`, and drive the real +/// completion handler with the cursor at the end of it. +async fn complete_in( + server: &LaravelLanguageServer, + root: &Path, + file_name: &str, + buffer: &str, +) -> Option { + let uri = Url::from_file_path(root.join(file_name)).expect("file url"); + server + .documents + .write() + .await + .insert(uri.clone(), (buffer.to_string(), 1)); + server + .completion(CompletionParams { + text_document_position: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri }, + position: Position { + line: 0, + character: buffer.chars().count() as u32, + }, + }, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: PartialResultParams::default(), + context: None, + }) + .await + .expect("completion handler must not error") +} + +/// Split a response into its items and its serialized form. `None` is a real +/// outcome (an empty item list is reported as `None`), so it becomes zero items +/// and still runs every assertion rather than being skipped. +fn dissect(response: Option) -> (Vec, String) { + match response { + None => (Vec::new(), String::new()), + Some(CompletionResponse::Array(items)) => { + let json = serde_json::to_string(&items).expect("serialize items"); + (items, json) + } + Some(CompletionResponse::List(list)) => { + let json = serde_json::to_string(&list).expect("serialize list"); + (list.items, json) + } + } +} + +/// No secret survives anywhere in the serialized response — any field, any +/// item. Returns the items so callers can add positive assertions. +fn assert_no_secret_leak( + response: Option, + context: &str, +) -> Vec { + let (items, json) = dissect(response); + for secret in [PASSWORD_VALUE, TOKEN_VALUE, COMMENTED_VALUE, URL_SECRET] { + assert!( + !json.contains(secret), + "{context}: {secret} leaked into the completion response: {json}" + ); + } + items +} + +fn item<'a>(items: &'a [CompletionItem], label: &str, context: &str) -> &'a CompletionItem { + items.iter().find(|i| i.label == label).unwrap_or_else(|| { + panic!( + "{context}: expected {label} to be offered, got {:?}", + items.iter().map(|i| &i.label).collect::>() + ) + }) +} + +fn documentation_markdown(item: &CompletionItem, context: &str) -> String { + match item.documentation.as_ref() { + Some(Documentation::MarkupContent(MarkupContent { + kind: MarkupKind::Markdown, + value, + })) => value.clone(), + other => panic!("{context}: expected markdown documentation, got {other:?}"), + } +} + +/// The panel `completion()` builds for a `.env` variable: name, summary, +/// `Source: `. Asserted whole, so dropping any builder call fails. +fn expected_panel(name: &str, summary: &str) -> String { + format!("**{name}**\n\n{summary}\n\nSource: .env") +} + +// ======================================================================== +// Surface 1 — env completion +// ======================================================================== + +#[tokio::test] +async fn env_completion_redacts_every_matched_category_and_leaves_the_rest_alone() { + let (_dir, root, server) = project(&dotenv()).await; + let line = " String { + format!( + " env('{PASSWORD_NAME}'),\n 'token' => (bool) env('{TOKEN_NAME}'),\n 'url' => env('{URL_NAME}'),\n 'name' => env('{PLAIN_NAME}', 'Laravel'),\n 'fallback' => env('MISSING_PASSWORD', 'fallback-default'),\n 'absent' => env('ABSENT_TOKEN'),\n 'secret_key' => 'plain-literal-value',\n];\n" + ) +} + +fn write(root: &Path, rel: &str, contents: &str) { + let path = root.join(rel); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, contents).unwrap(); +} + +#[tokio::test] +async fn config_completion_redacts_only_the_dotenv_sourced_sensitive_values() { + let (_dir, root, server) = project(&dotenv()).await; + write(&root, "config/app.php", &config_php()); + + let keys = server.get_all_config_keys().await; + let value_of = |key: &str| { + keys.iter() + .find(|c| c.key == key) + .unwrap_or_else(|| { + panic!( + "expected {key}, got {:?}", + keys.iter().map(|c| &c.key).collect::>() + ) + }) + .value + .clone() + }; + + // Both `env()` spellings redact, and they do so through the *same* arm of + // the resolver: `env_pattern` is unanchored, so it matches the + // `env('API_TOKEN')` substring of the cast and the `(bool)` prefix never + // reaches a branch of its own. The fixture is here to pin that the cast + // spelling resolves and redacts at all — not to claim a second code path + // exists, which is what the deleted `bool_env_pattern` arm falsely implied. + assert_eq!(value_of("app.password"), REDACTED_ENV_VALUE); + assert_eq!(value_of("app.token"), REDACTED_ENV_VALUE); + + // Caught by shape, not by name: `DATABASE_URL` clears the predicate. + assert_eq!( + value_of("app.url"), + url_masked(), + "a credential inside the value is masked even though the name is not sensitive" + ); + + // The three exemptions, each of which has nothing to leak. + assert_eq!( + value_of("app.name"), + PLAIN_VALUE, + "an unmatched dotenv value is untouched" + ); + assert_eq!( + value_of("app.fallback"), + "fallback-default", + "a literal default is already visible in the PHP file being edited" + ); + assert_eq!( + value_of("app.absent"), + "${ABSENT_TOKEN}", + "the not-found placeholder carries no value to redact" + ); + assert_eq!( + value_of("app.secret_key"), + "plain-literal-value", + "the predicate reads the env var's name, never the config key's" + ); +} + +/// End-to-end through the real handler, so the render sites are covered too: +/// `completion_detail` and `config_documentation` both receive the redacted +/// value, and nothing else in the response carries the secret. +#[tokio::test] +async fn the_config_completion_response_carries_no_dotenv_secret() { + let (_dir, root, server) = project(&dotenv()).await; + write(&root, "config/app.php", &config_php()); + + let line = " CacheManager { + *server.cache.write().await = Some(CacheManager::load(root)); + server.populate_cache_from_salsa().await; + server + .cache + .read() + .await + .as_ref() + .expect("cache present") + .save() + .expect("cache should persist"); + CacheManager::load(root) +} + +#[tokio::test] +async fn the_disk_cache_keeps_sensitive_names_but_never_their_values() { + let (_dir, root, server) = project(&dotenv()).await; + let reloaded = round_trip_cache(&server, &root).await; + let variables = &reloaded + .get_env_vars() + .expect("env vars should round-trip") + .variables; + + // Asserted on the deserialized struct, not on the file's bytes: a + // reversible encoding would pass a substring check and still leak. + for name in [PASSWORD_NAME, TOKEN_NAME, COMMENTED_NAME] { + assert_eq!( + variables.get(name).map(String::as_str), + Some(""), + "{name} must be cached by name with an empty value — present, never plaintext" + ); + } + // Not name-matched, so not emptied — but the credential inside it must not + // reach the file either. This is the worst of the four surfaces to get + // wrong: the cache is long-lived, sits outside the project, and no one + // reads it before it leaks. + assert_eq!( + variables.get(URL_NAME).map(String::as_str), + Some(url_masked().as_str()), + "{URL_NAME} must be cached with its password masked, never in plaintext" + ); + assert_eq!( + variables.get(PLAIN_NAME).map(String::as_str), + Some(PLAIN_VALUE), + "an unmatched variable must round-trip unchanged" + ); +} + +/// A cache entry survives the round trip byte-for-byte for an unmatched name, +/// and registering the reloaded map back into a cold server is accepted — the +/// path `load_cache_data` takes on a warm start. +/// +/// It stops there deliberately. `register_cached_env_vars` writes the Salsa +/// actor's `env_variables` map, and **nothing renders from that map**: +/// `get_env_variable` / `get_env_variable_names` have no caller outside +/// `salsa_impl`, and all four surfaces read `get_all_parsed_env_vars` / +/// `get_parsed_env_var`, which walk the registered `.env` *sources* only. So +/// redaction in the cache is about the plaintext sitting on disk, not about +/// what a warm start displays; asserting a rendering difference here would be +/// asserting about code that never runs. The rendering half of the parity +/// claim is pinned instead by +/// `an_empty_sensitive_value_redacts_rather_than_reading_empty`, whose fixture +/// is exactly the shape a cache read produces: a matched name with an empty +/// value. +#[tokio::test] +async fn the_reloaded_cache_registers_cleanly_on_a_cold_server() { + let (_dir, root, server) = project(&dotenv()).await; + let reloaded = round_trip_cache(&server, &root).await; + let cached = reloaded.get_env_vars().expect("env vars").variables.clone(); + assert_eq!( + cached.get(PLAIN_NAME).map(String::as_str), + Some(PLAIN_VALUE), + "the unmatched value must survive the round trip unchanged" + ); + + let warm = test_server(); + warm.salsa + .register_cached_env_vars(cached) + .await + .expect("a redacted cache must still register on warm start"); +} + +/// A cache file written by a pre-fix binary already holds plaintext secrets. +/// The `CACHE_VERSION` bump is what stops it being trusted and served — and the +/// rescan it forces must leave an ordinary variable exactly as it was. +/// +/// Both halves live in one test on purpose: "the old file is dropped" and "the +/// replacement is correct" are one behaviour, and splitting them let AC #7's +/// regression guard be satisfied by combining two fixtures neither of which +/// contained both variables. +#[tokio::test] +async fn a_pre_bump_cache_holding_plaintext_is_rejected_and_rescanned() { + let (_dir, root, server) = project(&dotenv()).await; + + // Write a well-formed current-version cache carrying the plaintext, then + // rewind only its version field. Hand-writing the JSON instead would risk + // the test passing on a parse error rather than on the version check. + let mut cache = CacheManager::load(&root); + cache.set_env_vars(laravel_lsp::cache_manager::CachedEnvVars { + variables: [ + (PASSWORD_NAME.to_string(), PASSWORD_VALUE.to_string()), + // The regression guard rides in the same planted file: an ordinary + // variable is dropped along with the secret, and has to come back + // unchanged from the rescan. + (PLAIN_NAME.to_string(), PLAIN_VALUE.to_string()), + ] + .into_iter() + .collect(), + }); + cache.save().expect("cache should persist"); + let path = cache.cache_path().expect("cache path").to_path_buf(); + + let current = std::fs::read_to_string(&path).expect("read cache"); + let mut json: serde_json::Value = serde_json::from_str(¤t).expect("cache json"); + // Derived from the file rather than written as a literal, so the next + // version bump doesn't quietly turn this into a test of nothing. + let version = json["version"].as_u64().expect("version field"); + json["version"] = serde_json::json!(version - 1); + let previous = serde_json::to_string_pretty(&json).expect("serialize cache"); + assert!( + previous.contains(PASSWORD_VALUE), + "the planted cache must actually hold the plaintext this test is about" + ); + std::fs::write(&path, previous).expect("plant pre-bump cache"); + + // `get_env_vars()` is the assertion that discriminates: `has_cached_data()` + // reads only the vendor/app/config sections and would answer the same with + // the version check deleted. + assert!( + CacheManager::load(&root).get_env_vars().is_none(), + "a pre-bump cache must be dropped, not served — it holds plaintext secrets" + ); + + // The rescan the rejection forces, through the real populate/save/load path. + let rescanned = round_trip_cache(&server, &root).await; + let variables = &rescanned + .get_env_vars() + .expect("the rescan must repopulate the cache") + .variables; + assert_eq!( + variables.get(PLAIN_NAME).map(String::as_str), + Some(PLAIN_VALUE), + "an ordinary variable must survive the forced rescan unchanged" + ); + assert_eq!( + variables.get(PASSWORD_NAME).map(String::as_str), + Some(""), + "the rescan must rewrite the secret as an empty value, not restore it" + ); + assert!( + !std::fs::read_to_string(&path) + .expect("read rescanned cache") + .contains(PASSWORD_VALUE), + "the rescanned file must not carry the plaintext the planted one did" + ); +} diff --git a/laravel-lsp/src/tests/mod.rs b/laravel-lsp/src/tests/mod.rs index 9fd33eed..06c84edb 100644 --- a/laravel-lsp/src/tests/mod.rs +++ b/laravel-lsp/src/tests/mod.rs @@ -22,6 +22,7 @@ mod directive_view_fallback; mod dynamic_where_sparseness; mod env_completion_system_leak; mod env_source_registration_gate; +mod env_value_redaction; mod expected_path_diagnostic_containment; mod factory_goto_def_handler; mod flux_component_context;