scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-11 03:40:05 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix(hooks): keep allowed_env_vars enforced for header interpolation

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-06-25 16:17:22 -04:00
parent c9a87ca0b4
commit d58e722647
3 changed files with 185 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
docs/public/agents/hooks.mdx
View file

@ -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

9
docs/public/api-reference/fabro-api.yaml
View file

@ -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:

195
lib/crates/fabro-hooks/src/executor.rs
View file

@ -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<E>(
value: &InterpString,
allowed_env_vars: &[String],
env: &E,
) -> Result<String, String>
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<E>(
prompt: &InterpString,
model: Option<&InterpString>,
@ -494,6 +530,7 @@ impl HookExecutorImpl {
client: &fabro_http::HttpClient,
url: &InterpString,
headers: Option<&HashMap<String, InterpString>>,
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),