fix: leave a dbt_profile's own env_var() credential for dbt to render

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/windmill-api/src/dbt.rs

@@ -28,9 +28,9 @@ async fn get_warehouse(
     Path((w_id, name)): Path<(String, String)>,
 ) -> Result<Json<DbtWarehouseConnection>> {
     // Scoped to a running DBT job, and the reason it must stay that way: the
-    // response carries the warehouse's credentials. A dbt job already holds them
-    // in its rendered `profiles.yml`, so serving them changes nothing for it —
-    // but every script job's token carries a job id too, and any other language
+    // response carries the warehouse's credentials. A dbt job connects with them
+    // and so already holds them, so serving them changes nothing for it — but
+    // every script job's token carries a job id too, and any other language
     // asking for them would be reading a credential it was never given.
     // In no-auth mode every request is the synthetic superadmin and carries no
     // job, so the scoping below has nothing to check. Refusing there would make
@@ -61,7 +61,11 @@ async fn get_warehouse(
                 ))
             })?;
             let resource_type = warehouse_resource_type(&db, &w_id, &resource_path).await?;
-            return Ok(Json(DbtWarehouseConnection { value, target, resource_type }));
+            return Ok(Json(DbtWarehouseConnection {
+                value,
+                target,
+                resource_type,
+            }));
         }
         return Err(Error::BadRequest(
             "this route resolves a dbt warehouse for a running job and needs a job token"
@@ -104,7 +108,11 @@ async fn get_warehouse(
         ))
     })?;
     let resource_type = warehouse_resource_type(&db, &w_id, &resource_path).await?;
-    Ok(Json(DbtWarehouseConnection { value, target, resource_type }))
+    Ok(Json(DbtWarehouseConnection {
+        value,
+        target,
+        resource_type,
+    }))
 }
 
 /// A warehouse resource's type, which decides whether its value is translated
@@ -118,7 +126,11 @@ async fn warehouse_resource_type(db: &DB, w_id: &str, path: &str) -> Result<Stri
     )
     .fetch_optional(db)
     .await?
-    .ok_or_else(|| Error::NotFound(format!("the dbt warehouse points at `{path}`, which does not exist")))
+    .ok_or_else(|| {
+        Error::NotFound(format!(
+            "the dbt warehouse points at `{path}`, which does not exist"
+        ))
+    })
 }
 
 /// A settled node's state, for a worker that cannot write the database.

backend/windmill-worker/src/dbt_executor.rs

@@ -1773,10 +1773,11 @@ async fn resolve_warehouse(
 
 /// Identifies the connection a rendered profile describes, for run identity.
 ///
-/// The credentials are no longer IN the rendered text, so `env` is hashed
-/// alongside it: without them a resource repointed at another warehouse with the
-/// same host and database names — a different account, a rotated password —
-/// would present the identity of the one a retry saved its failures against.
+/// The rendered text names the credentials rather than carrying them, so `env`
+/// is hashed alongside it: on the text alone, a resource repointed at another
+/// warehouse with the same host and database names — a different account, a
+/// rotated password — would present the identity a retry saved its failures
+/// against.
 ///
 /// Three things belong to the ATTEMPT rather than the connection, and hashing
 /// any of them as-is makes a retry reject its own predecessor — it compares
@@ -1787,8 +1788,8 @@ async fn resolve_warehouse(
 ///   hashed in place of its path.
 /// * the job's own token, where the warehouse resource interpolates `$WM_TOKEN`
 ///   (a warehouse reached through an OIDC or on-behalf flow does). Every
-///   attempt is a new job with a new token, and it reaches the yaml through a
-///   credential now, so both sides are normalized.
+///   attempt is a new job with a new token, and it arrives as a credential, so
+///   both halves are normalized.
 /// * the secret variable names, a fresh nonce per render.
 fn profile_identity_digest(
     yaml: &str,
@@ -5068,11 +5069,11 @@ mod tests {
         assert_ne!(first, recerted);
     }
 
-    // Two things the rendered profile carries belong to the attempt: the job's
-    // own token, where the warehouse resource interpolates `$WM_TOKEN`, and the
+    // Two things about a rendered profile belong to the attempt: the job's own
+    // token, where the warehouse resource interpolates `$WM_TOKEN`, and the
     // nonce the credential variables are named on, fresh per render. Neither may
     // move the identity, or a retry never recognizes its own saved run — while
-    // the credentials themselves, which the file no longer holds, still must.
+    // the credentials, which reach the digest beside the text, still must.
     #[test]
     fn profile_identity_ignores_the_attempt_but_not_the_credential() {
         let dir = Path::new("/tmp/windmill/w/job-1/profiles");

backend/windmill-worker/src/dbt_profiles.rs

@@ -53,6 +53,10 @@ const CREDENTIAL_KEY_MARKERS: &[&str] = &[
     "api_key",
     "apikey",
     "access_key",
+    // The whole word, not `auth`: several adapters spell an `authenticator`
+    // naming a METHOD (`oauth`, `externalbrowser`), and redacting `oauth` from
+    // every line of a run is the failure above.
+    "authorization",
 ];
 
 fn is_credential_key(key: &str) -> bool {
@@ -93,15 +97,35 @@ impl ProfileSecrets {
     }
 
     /// One target key's value: hidden when the key names a credential, inline
-    /// otherwise. An empty value is always inline — dbt scrubs by substring, and
-    /// an empty secret matches between every pair of characters in the log.
+    /// otherwise.
     fn field(&mut self, key: &str, value: &str) -> ProfileValue {
-        if is_credential_key(key) && !value.is_empty() {
+        self.leaf(is_credential_key(key), value)
+    }
+
+    /// A value whose classification the caller already knows, from its own key
+    /// or an enclosing one.
+    fn leaf(&mut self, secret: bool, value: &str) -> ProfileValue {
+        if secret && Self::hideable(value) {
             self.hide(value)
         } else {
             quoted(value)
         }
     }
+
+    /// Whether standing in for this value is safe.
+    ///
+    /// An EMPTY value never is: dbt redacts by substring, and an empty one
+    /// matches between every pair of characters in the log.
+    ///
+    /// Neither is a Jinja expression, which NAMES a credential rather than being
+    /// one — a `dbt_profile` block is pasted from a working `profiles.yml`,
+    /// where `password: "{{ env_var('PGPASSWORD') }}"` is the ordinary shape,
+    /// resolved against the descriptor's `env`. dbt renders a profile value once
+    /// and does not re-render what `env_var()` returns, so standing in for that
+    /// text would hand the adapter the template instead of the password.
+    fn hideable(value: &str) -> bool {
+        !value.is_empty() && !value.contains("{{") && !value.contains("{%")
+    }
 }
 
 /// The per-adapter facts, so each adapter states them together and a new one
@@ -873,7 +897,7 @@ pub fn render_dbt_profile(
         if (k == schema_key && schema_override.is_some()) || (k == "threads" && threads.is_some()) {
             continue;
         }
-        emit_entry(&mut yaml, 6, k, v, &mut secrets);
+        emit_entry(&mut yaml, 6, k, v, false, &mut secrets);
     }
     if root_certificate_pem.is_some() {
         yaml.push_str(&format!(
@@ -907,19 +931,31 @@ pub fn render_dbt_profile(
 /// Emit one target key, nesting as deep as the value goes — an adapter's credential can be
 /// a mapping (bigquery's `keyfile_json`) or a list. Keys are quoted like values: one nothing
 /// here enumerates is as free-form as a password.
-fn emit_entry(out: &mut String, indent: usize, key: &str, v: &Value, secrets: &mut ProfileSecrets) {
+///
+/// `enclosing_secret` is what a key ABOVE this one decided. Everything under a key naming a
+/// credential is credential material, whatever it is called — an `oauth_credentials` mapping
+/// spells its own keys, and Windmill has no list to check them against.
+fn emit_entry(
+    out: &mut String,
+    indent: usize,
+    key: &str,
+    v: &Value,
+    enclosing_secret: bool,
+    secrets: &mut ProfileSecrets,
+) {
     out.push_str(&format!("{}{}:", " ".repeat(indent), yaml_scalar(key)));
-    emit_value(out, indent, v, is_credential_key(key), secrets);
+    emit_value(
+        out,
+        indent,
+        v,
+        enclosing_secret || is_credential_key(key),
+        secrets,
+    );
 }
 
 /// The value half, after `key:`. An empty collection is emitted INLINE: a block with no
 /// children reads back as `null`, so `extensions: []` would reach the adapter as a missing
 /// value rather than the empty list dbt was handed.
-///
-/// `secret` says whether the key this value hangs off names a credential. A list inherits
-/// it, since its items share that one key; a mapping does not, because each of its own keys
-/// answers for itself — a `keyfile_json` holds a private key beside a project id, and dbt
-/// redacts a secret's value wherever it appears in the run's output.
 fn emit_value(
     out: &mut String,
     indent: usize,
@@ -938,7 +974,7 @@ fn emit_value(
             }
             out.push('\n');
             for (k, v) in kept {
-                emit_entry(out, indent + 2, k, v, secrets);
+                emit_entry(out, indent + 2, k, v, secret, secrets);
             }
         }
         Value::Array(items) => {
@@ -954,9 +990,7 @@ fn emit_value(
                 emit_value(out, indent + 2, item, secret, secrets);
             }
         }
-        Value::String(s) if secret && !s.is_empty() => {
-            out.push_str(&format!(" {}\n", secrets.hide(s).render()))
-        }
+        Value::String(s) => out.push_str(&format!(" {}\n", secrets.leaf(secret, s).render())),
         _ => out.push_str(&format!(" {}\n", yaml_value(v))),
     }
 }
@@ -1535,12 +1569,14 @@ mod tests {
     }
 
     // A `dbt_profile` block is written for an adapter Windmill may know nothing
-    // about, so the credential is recognized by key name — including nested, where
-    // a mapping's own keys answer for themselves rather than inheriting.
+    // about, so the credential is recognized by key name, and everything under
+    // such a key with it.
     #[test]
     fn a_dbt_profile_hides_its_credential_keys() {
         let r = json!({"type": "trino", "host": "trino.internal", "schema": "analytics",
-                       "user": "u", "password": "pw", "http_headers": {"x-api-key": "k"},
+                       "user": "u", "password": "pw",
+                       "http_headers": {"x-api-key": "k", "X-Trace": "on"},
+                       "oauth_credentials": {"client": "c"},
                        "keyfile_json": {"project_id": "proj", "private_key": "pem"}});
         let p = render_dbt_profile(
             &DbtAdapter::from_dbt_type("trino").unwrap(),
@@ -1554,15 +1590,44 @@ mod tests {
         .unwrap();
         let mut hidden: Vec<&str> = p.env.iter().map(|(_, v)| v.as_str()).collect();
         hidden.sort();
-        assert_eq!(hidden, vec!["k", "pem", "pw"], "{}", p.yaml);
+        // `client` for the enclosing `oauth_credentials`; `X-Trace` names nothing
+        // and its enclosing key names nothing either.
+        assert_eq!(hidden, vec!["c", "k", "pem", "pw"], "{}", p.yaml);
         let parsed: serde_yml::Value = serde_yml::from_str(&p.yaml).unwrap();
         let target = &parsed["wm"]["outputs"]["prod"];
         assert_eq!(target["host"].as_str(), Some("trino.internal"));
         assert_eq!(target["user"].as_str(), Some("u"));
+        assert_eq!(target["http_headers"]["X-Trace"].as_str(), Some("on"));
         assert_eq!(target["keyfile_json"]["project_id"].as_str(), Some("proj"));
         assert_eq!(p.schema.as_deref(), Some("analytics"));
     }
 
+    // The block is pasted from a working `profiles.yml`, where a credential is
+    // ordinarily an `env_var()` resolved against the descriptor's `env`. dbt
+    // renders a profile value once, so standing in for that expression would
+    // hand the adapter the template text instead of the password.
+    #[test]
+    fn a_dbt_profile_leaves_an_env_var_expression_for_dbt_to_render() {
+        let r = json!({"type": "clickhouse", "host": "ch.internal",
+                       "password": "{{ env_var('CH_PASSWORD') }}"});
+        let p = render_dbt_profile(
+            &KnownAdapter::Clickhouse.into(),
+            r.as_object().unwrap(),
+            "wm",
+            "prod",
+            None,
+            None,
+            std::path::Path::new("/tmp/p"),
+        )
+        .unwrap();
+        assert!(p.env.is_empty(), "{:?}", p.env);
+        let parsed: serde_yml::Value = serde_yml::from_str(&p.yaml).unwrap();
+        assert_eq!(
+            parsed["wm"]["outputs"]["prod"]["password"].as_str(),
+            Some("{{ env_var('CH_PASSWORD') }}")
+        );
+    }
+
     // A RUNTIME check, because one dbt executor serves every adapter. A refactor
     // reaching for a bare `LICENSE_KEY_VALID` would silently let CE through, since
     // the OSS variant initializes it to `true`.

docs/dbt-runtime.md

@@ -30,7 +30,7 @@ the dominant way dbt is orchestrated today.
 | 5 | Project storage | The project is the script's module bundle; nothing is cloned. See "Where the dbt project lives" |
 | 6 | Multiple run configs | Per-run `select` on one script; N scripts means N projects |
 | 7 | Run-time `select` | Descriptor default plus run-arg override |
-| 8 | Credentials | Workspace warehouses, plus `profiles.yml` passthrough. A descriptor never names a resource. A rendered profile names its credentials through `env_var()` and carries none. See below |
+| 8 | Credentials | Workspace warehouses, plus `profiles.yml` passthrough. A descriptor never names a resource. A rendered profile names its credentials through `env_var()` rather than carrying them. See below |
 | 9 | Adapter mappings | postgres, redshift, mysql, snowflake, bigquery, databricks translate from their Windmill resource; **every** adapter dbt has is reachable from a `dbt_profile` resource, or the project's own `profiles.yml` |
 | 10 | Private repo auth | Not applicable: the project is synced, not fetched |
 | 11 | Asset kind | `dbt://<warehouse>/<schema>/<name>` — keyed on the relation, not on dbt's node id. See below |
@@ -359,20 +359,43 @@ value is replaced with `*****` wherever dbt prints it, which is what Windmill
 streams into the job log. A plainly named variable would be worth nothing: the
 project would read it back with one `env_var()`.
 
-Four things this had to get right, each of which quietly undoes it otherwise.
+Five things this had to get right, each of which quietly undoes it otherwise.
 
-**Only credentials go through a variable.** dbt redacts a secret's VALUE from
-every line it prints, so routing a coordinate through one redacts the run: a
-`user` of `postgres` turns `Registered adapter: postgres=1.11.0` into
-`Registered adapter: *****=1.11.0`, and a `schema` of `public` would blank out
-half the log. A translated resource knows its own credential fields; a
-`dbt_profile` block is written for an adapter Windmill may know nothing about
-(decision 24), so there it is by key name — `password`, `token`, `secret`,
-`private_key`, `passphrase`, `credential`, `api_key`, `access_key`, matched as a
-substring with `-` folded to `_`, so `client_secret` and an `http_headers`
-`x-api-key` are covered. Nested, each mapping key answers for itself rather than
-inheriting: a `keyfile_json` holds a private key beside a project id. The rule
-errs toward leaving a value inline, which is where it already was.
+**Only credentials go through a variable, and hiding everything is not the safer
+choice.** dbt redacts a secret's VALUE from every line it emits — the console
+log Windmill streams to the job, *and* the JSON event log Windmill parses for
+per-model status and relation names. Registering the string `public` as a secret
+does not merely make the log ugly: every relation whose name contains it is
+recorded as `*****`, so the run succeeds while the asset graph fills with
+nonsense. Measured, not feared — a `user` of `postgres` turns
+`Registered adapter: postgres=1.11.0` into `Registered adapter: *****=1.11.0`,
+and a schema registered as a secret vanishes from all 64 places it appears in the
+JSON log. Fail-closed is the wrong direction here.
+
+So: a translated resource knows exactly which of its fields are credentials,
+and there the file provably carries none. A `dbt_profile` block is written for an
+adapter Windmill may know nothing about (decision 24), so there the rule is by
+key name — `password`, `token`, `secret`, `private_key`, `passphrase`,
+`credential`, `api_key`, `access_key`, `authorization`, matched as a substring
+with `-` folded to `_`, so `client_secret` and an `http_headers` `x-api-key` are
+covered — and everything nested under such a key goes with it, since an
+`oauth_credentials` mapping spells its own keys and there is no list to check
+them against. `authorization` rather than `auth`, because several adapters spell
+an `authenticator` naming a METHOD and redacting `oauth` from a whole run is the
+failure above.
+
+That rule is not exhaustive over an open adapter set, and it is not claimed to
+be: a key nobody recognized stays exactly where it already was, inline. The
+guarantee is the translated path; the block path is a large reduction whose
+boundary is a list one line long to extend.
+
+**A value that is already an `env_var()` expression stays inline.** A
+`dbt_profile` block is pasted from a working `profiles.yml`, where
+`password: "{{ env_var('PGPASSWORD') }}"` is the ordinary shape, resolved against
+the descriptor's `env`. dbt renders a profile value ONCE and does not re-render
+what `env_var()` returns, so standing in for that text would hand the adapter the
+template instead of the password — and there is nothing to hide either way, since
+such a value names a credential rather than being one.
 
 **The names are a fresh nonce per render.** `packages.yml` is rendered under the
 same secret context as `profiles.yml`, on every dbt invocation and not only
@@ -385,12 +408,12 @@ descriptor's `env` and the script's environment variables, alongside the
 while a project's own `DBT_ENV_SECRET_*` package token, outside Windmill's
 namespace, still works.
 
-**Run identity moved with them.** `profile_digest` is a one-way digest of the
-resolved connection, and the connection is no longer all in the text, so the
-values are hashed beside it — otherwise a resource repointed at another
+**Run identity covers what the text no longer states.** `profile_digest` is a
+one-way digest of the resolved connection, so the credential values are hashed
+beside the rendered text — on the text alone, a resource repointed at another
 warehouse with the same host and database names would present the identity a
 retry saved its failures against. The nonce is normalized out of both halves for
-the same reason the job's own token already was: it belongs to the attempt.
+the same reason the job's own token is: it belongs to the attempt.
 
 **A project's own `profiles.yml` is passed through untouched.** It is written by
 the same author as the macros that would read it, so there is no credential to