From d58e72264781cb173e420cf5ab052717db7e4f46 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 25 Jun 2026 16:17:22 -0400 Subject: [PATCH] fix(hooks): keep allowed_env_vars enforced for header interpolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTP-hook headers carry credentials, so a `{{ env.NAME }}` token in a header value must only resolve env vars the hook explicitly opts into via `allowed_env_vars`. The typed-interpolation rewrite dropped that allowlist: headers resolved any `{{ env.* }}` while `allowed_env_vars` was parsed, carried, and documented but ignored in the executor — a silent fail-open regression. Restore the allowlist as a fail-closed control without reviving the removed template env API. Header values now resolve through a scoped env lookup that returns a value only for names in the hook's `allowed_env_vars` and `None` otherwise, so a non-allowlisted `{{ env.X }}` resolves as absent (Missing) and the hook blocks before firing — never sending a half-resolved or empty credential header. An empty `allowed_env_vars` permits no env vars in headers, matching the prior behavior. The allowlist applies only to headers; url, command, prompt, and model keep no allowlist. Also: - Correct the resolve helper doc comments to say env is the only wired namespace; secrets/vars/inputs are unavailable here and fail closed. - Log the unresolved URL token source instead of the resolved URL on header/URL resolution failures, so a URL-embedded credential cannot leak into logs. - Update the hooks docs and the OpenAPI `headers`/`allowed_env_vars` descriptions to document `{{ env.NAME }}` token interpolation and the enforced header allowlist. Tests cover both directions: an allowlisted var resolves and the header is sent, while a non-allowlisted var blocks and never fires the request. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/public/agents/hooks.mdx | 9 +- docs/public/api-reference/fabro-api.yaml | 9 ++ lib/crates/fabro-hooks/src/executor.rs | 195 ++++++++++++++++++++--- 3 files changed, 185 insertions(+), 28 deletions(-) diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index ef634b4f8..aafdd356f 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -28,16 +28,17 @@ POST the event context as JSON to an HTTP endpoint. Useful for webhooks, externa event = "run_complete" type = "http" url = "https://hooks.example.com/done" +allowed_env_vars = ["API_KEY"] [hooks.headers] -Authorization = "Bearer $API_KEY" +Authorization = "Bearer {{ env.API_KEY }}" ``` | Field | Description | |---|---| -| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. | -| `headers` | Optional HTTP headers. Values support `$VAR` interpolation from `allowed_env_vars`. | -| `allowed_env_vars` | List of environment variable names that may be interpolated into headers. | +| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. Supports `{{ env.NAME }}` interpolation. | +| `headers` | Optional HTTP headers. Values support `{{ env.NAME }}` interpolation, scoped to the names in `allowed_env_vars`. A token for any other env var fails to resolve and the hook blocks (fail-closed). | +| `allowed_env_vars` | Allowlist of environment variable names a header may read via `{{ env.NAME }}`. Empty (the default) means no env vars may be interpolated into headers. | | `tls` | TLS mode: `"verify"` (default), `"no_verify"`, or `"off"`. | ### Prompt diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 7e0484dc0..dfe6fa0c0 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14222,10 +14222,19 @@ components: oneOf: - $ref: "#/components/schemas/StringMap" - type: "null" + description: >- + Optional HTTP headers for an http hook. Values support + `{{ env.NAME }}` interpolation, scoped to the names listed in + `allowed_env_vars`; a token for any other env var fails to resolve + and the hook blocks (fail-closed). allowed_env_vars: type: array items: type: string + description: >- + Allowlist of environment variable names that an http hook header may + read via `{{ env.NAME }}`. An empty list (the default) permits no env + vars in headers. tls: $ref: "#/components/schemas/TlsMode" prompt: diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 520b05e89..9b6caf685 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -56,7 +56,12 @@ pub trait HookExecutor: Send + Sync { } /// Resolve a typed [`InterpString`] hook segment at fire time, looking up -/// `{{ env.* }}` / `{{ secrets.* }}` tokens against `env`. +/// `{{ env.* }}` tokens against `env`. +/// +/// Only the `env` namespace is wired here; `{{ secrets.* }}`, `{{ vars.* }}`, +/// and `{{ inputs.* }}` tokens have no lookup in this context and resolve as +/// `Unavailable`, which is a hard error — so a hook that references one fails +/// closed rather than firing with a half-resolved value. /// /// The value stays typed end-to-end: it is carried as an `InterpString` /// through the config resolve layer and resolved here from its segments — @@ -73,6 +78,37 @@ where .map_err(|error| error.to_string()) } +/// Resolve an HTTP-hook **header** value at fire time, scoping its +/// `{{ env.* }}` lookups to `allowed_env_vars`. +/// +/// Headers carry credentials, so unlike every other hook field they read env +/// through an allowlist: a `{{ env.NAME }}` token resolves only when `NAME` is +/// listed in the hook's `allowed_env_vars`. A name outside the allowlist looks +/// up as absent, so it surfaces as the same fail-closed `Missing` error as an +/// unset variable and the hook blocks instead of firing. An empty +/// `allowed_env_vars` therefore permits no env vars in headers at all. This +/// mirrors the previous template-based `with_env_lookup_allowed` behavior +/// without reviving any template engine. +fn resolve_header( + value: &InterpString, + allowed_env_vars: &[String], + env: &E, +) -> Result +where + E: Env + ?Sized, +{ + value + .resolve(|name| { + if allowed_env_vars.iter().any(|allowed| allowed == name) { + env.var(name).ok() + } else { + None + } + }) + .map(|resolved| resolved.value) + .map_err(|error| error.to_string()) +} + /// Executes hooks via shell commands or HTTP POST. pub struct HookExecutorImpl; @@ -102,10 +138,10 @@ impl HookExecutorImpl { /// Resolve the prompt and optional model segments at fire time. /// - /// Fail-closed: a missing or out-of-scope `{{ env.* }}`/`{{ secrets.* }}` - /// token is a hard error so the hook never fires with a half-resolved - /// value. The caller turns the error into a `Block` decision, matching the - /// command-hook behavior. + /// Fail-closed: only `{{ env.* }}` is wired here; a missing env token (or a + /// token in any other, unavailable namespace) is a hard error so the hook + /// never fires with a half-resolved value. The caller turns the error into + /// a `Block` decision, matching the command-hook behavior. fn resolve_prompt_and_model( prompt: &InterpString, model: Option<&InterpString>, @@ -494,6 +530,7 @@ impl HookExecutorImpl { client: &fabro_http::HttpClient, url: &InterpString, headers: Option<&HashMap>, + allowed_env_vars: &[String], tls: &TlsMode, context: &HookContext, timeout: std::time::Duration, @@ -505,10 +542,22 @@ impl HookExecutorImpl { let resolved_url = match resolve_interp(url, env) { Ok(url) => url, Err(error) => { - tracing::error!( - error = %error, - "HTTP hook URL env resolution failed, not firing" - ); + // Log the unresolved source, never the resolved URL, so a + // credential embedded in the URL (userinfo/query) cannot leak. + #[expect( + clippy::disallowed_methods, + reason = "logging the unresolved token source on a failure path is \ + deliberate: it avoids materializing a URL-embedded credential \ + in logs, and resolution has already failed so there is nothing \ + resolved to log" + )] + { + tracing::error!( + url.source = %url.as_source(), + error = %error, + "HTTP hook URL env resolution failed, not firing" + ); + } return HookDecision::Block { reason: Some(error), }; @@ -533,15 +582,28 @@ impl HookExecutorImpl { if let Some(hdrs) = headers { for (key, value) in hdrs { - let interpolated = match resolve_interp(value, env) { + // Headers resolve through the per-hook env allowlist: a + // `{{ env.NAME }}` not in `allowed_env_vars` looks up as absent + // and blocks (fail-closed), exactly like an unset variable. + let interpolated = match resolve_header(value, allowed_env_vars, env) { Ok(rendered) => rendered, Err(error) => { - tracing::error!( - url = %resolved_url, - header = %key, - error = %error, - "HTTP hook header env resolution failed, not firing" - ); + // Log the unresolved URL source, not the resolved URL, + // so a URL-embedded credential cannot leak to logs. + #[expect( + clippy::disallowed_methods, + reason = "logging the unresolved URL token source on a failure \ + path is deliberate: it avoids materializing a \ + URL-embedded credential in logs" + )] + { + tracing::error!( + url.source = %url.as_source(), + header = %key, + error = %error, + "HTTP hook header env resolution failed, not firing" + ); + } return HookDecision::Block { reason: Some(error), }; @@ -657,13 +719,13 @@ impl HookExecutor for HookExecutorImpl { Cow::Borrowed(HookType::Http { ref url, ref headers, - allowed_env_vars: _, + ref allowed_env_vars, ref tls, }) | Cow::Owned(HookType::Http { ref url, ref headers, - allowed_env_vars: _, + ref allowed_env_vars, ref tls, }), ) => { @@ -672,6 +734,7 @@ impl HookExecutor for HookExecutorImpl { clients.get(*tls), url, headers.as_ref(), + allowed_env_vars, tls, context, definition.timeout(), @@ -1061,20 +1124,45 @@ mod tests { InterpString::parse(value) } - // Headers resolve via the same narrow `{{ ns.NAME }}` token resolver as - // every other hook field — no template engine, no allowlist. + // Headers resolve `{{ env.NAME }}` tokens through the per-hook + // `allowed_env_vars` allowlist: an allowlisted name resolves, anything else + // looks up as absent and fails closed. #[test] - fn header_resolves_narrow_token() { + fn header_resolves_allowlisted_var() { let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); - let result = resolve_interp(&interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), &env).unwrap(); + let result = resolve_header( + &interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), + &["FABRO_TEST_KEY_1".to_string()], + &env, + ) + .unwrap(); assert_eq!(result, "Bearer secret123"); } + // Fail-closed: a header may not read an env var that is set in the process + // but missing from `allowed_env_vars`. It resolves as absent (a hard error), + // so the value never reaches the request. + #[test] + fn header_rejects_unlisted_var() { + let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); + let err = resolve_header( + &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), + &[], + &env, + ) + .unwrap_err(); + assert!(err.contains("FABRO_TEST_KEY_3")); + } + #[test] fn header_missing_token_is_hard_error() { let env = test_env(&[]); - let err = - resolve_interp(&interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), &env).unwrap_err(); + let err = resolve_header( + &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), + &["FABRO_TEST_KEY_3".to_string()], + &env, + ) + .unwrap_err(); assert!(err.contains("FABRO_TEST_KEY_3")); } @@ -1124,6 +1212,7 @@ mod tests { &client, &interp(&server.url("/hook")), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1152,6 +1241,7 @@ mod tests { &client, &interp(&server.url("/hook")), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1178,6 +1268,7 @@ mod tests { &client, &interp(&server.url("/hook")), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1196,6 +1287,7 @@ mod tests { &client, &interp("http://127.0.0.1:1"), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(1), @@ -1230,6 +1322,7 @@ mod tests { &client, &interp(&server.url("/hook")), Some(&headers), + &["FABRO_TEST_TOKEN".to_string()], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1241,6 +1334,53 @@ mod tests { assert_eq!(decision, HookDecision::Proceed); } + // Fail-closed: a header that references an env var set in the process but + // absent from `allowed_env_vars` must block and never fire the request. + #[tokio::test] + async fn http_hook_unlisted_header_var_blocks_without_firing() { + let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); + + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method("POST").path("/hook"); + then.status(200).body(""); + }) + .await; + + let headers = HashMap::from([( + "Authorization".to_string(), + interp("Bearer {{ env.FABRO_TEST_TOKEN }}"), + )]); + + let client = test_http_client(); + let decision = HookExecutorImpl::execute_http( + &client, + &interp(&server.url("/hook")), + Some(&headers), + // Empty allowlist: the env var is set, but headers may read nothing. + &[], + &TlsMode::Off, + &make_context(), + std::time::Duration::from_secs(5), + &env, + ) + .await; + + assert_eq!(mock.calls_async().await, 0); + match decision { + HookDecision::Block { reason } => { + assert!( + reason + .as_deref() + .is_some_and(|reason| reason.contains("FABRO_TEST_TOKEN")), + "block reason should name the unlisted token, got: {reason:?}" + ); + } + other => panic!("expected Block on unlisted header var, got {other:?}"), + } + } + #[tokio::test] async fn http_hook_resolves_url_before_dispatch() { let server = httpmock::MockServer::start_async().await; @@ -1257,6 +1397,7 @@ mod tests { &client, &interp("{{ env.FABRO_TEST_URL }}"), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1283,6 +1424,7 @@ mod tests { &client, &interp("{{ env.FABRO_TEST_MISSING_URL }}/hook"), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1325,6 +1467,8 @@ mod tests { &client, &interp(&server.url("/hook")), Some(&headers), + // Allowlisted but unset: still blocks on the Missing lookup. + &["FABRO_TEST_MISSING_HEADER".to_string()], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1347,6 +1491,7 @@ mod tests { &client, &interp("http://example.com/hook"), None, + &[], &TlsMode::Verify, &make_context(), std::time::Duration::from_secs(5), @@ -1364,6 +1509,7 @@ mod tests { &client, &interp("http://example.com/hook"), None, + &[], &TlsMode::NoVerify, &make_context(), std::time::Duration::from_secs(5), @@ -1389,6 +1535,7 @@ mod tests { &client, &interp(&server.url("/hook")), None, + &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5),