fix: mask sensitive .env values in LSP output

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<String, String>,

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.
 ///

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<String> {
+    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::<String>::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::<String>::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::<String>::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::<String>::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"
+        );
+    }
+}