feat(filter): add identity header guard filter

[yossiovadia]

Summary

• Add identity_header_guard filter that captures headers matching a configurable prefix into filter_metadata and strips them before upstream forwarding
• Prevents identity headers (e.g. x-tenant-username, x-tenant-group) from leaking to LLM providers
• Captured values are available to downstream filters like external_metering (feat(filter): add external metering filter for usage reporting and balance checks #577)

Fixes #698

Motivation

The external_metering filter reads tenant identity from request headers for per-user usage attribution. These headers are set by an upstream auth layer and must not reach the upstream provider. reserved_headers in core only handles hardcoded x-praxis-* prefixes with no metadata capture and no configurable prefixes (core TODO #186).

Design

• Configurable header prefix (default: x-tenant-)
• Captured headers written to filter_metadata under a configurable namespace (prevents collision with verified auth metadata)
• Matched headers marked for removal via request_headers_to_remove
• ~120 lines of filter code

What's included

• filters/src/identity_guard/ — filter, config, 11 unit tests + 1 doctest
• tests/integration/tests/suite/examples/identity_header_guard.rs — 3 integration tests (config parse + header capture + strip)
• examples/configs/identity-header-guard.yaml — example config
• Generated filter docs and README updates

Test plan

• 11 unit tests covering: prefix matching, case insensitivity, namespace isolation, no-match passthrough, empty/missing headers, multiple captures, strip verification
• 1 doctest
• 3 integration tests (config parse, header capture to metadata, upstream strip)
• cargo xtask lint-example-tests — passes (example config has test coverage)
• cargo xtask lint-filter-docs — passes (generated docs up to date)
• cargo clippy -p praxis-ai-filters -- -D warnings — zero warnings
• Validated end-to-end on OpenShift deployment with external_metering consuming captured identity

[leseb]

would core be more suitable for this instead of this repo? it's not so much "ai" related

[praxis-bot]

Clean implementation — the filter logic is correct and the security-critical behavior (stripping before forwarding, namespaced metadata to avoid collision with verified auth) is well thought out. Three medium findings, all related to test coverage gaps.

[praxis-bot]

[Medium] This test claims to verify the default namespace but only asserts filter.name(), which is a static string unrelated to namespace selection. If the default were changed from "identity" to anything else, this test would still pass.

The behavior is tested indirectly by captures_matching_headers_to_metadata (which checks for the identity. prefix in metadata keys), but this named test is misleading — it guards nothing.

Convert to an async test that exercises a header through on_request and asserts the metadata key starts with identity., or remove it and let captures_matching_headers_to_metadata serve as the canonical default-namespace test.

[yossiovadia]

Fixed. Added clarifying comment explaining the default namespace is verified indirectly by captures_matching_headers_to_metadata (which asserts identity. prefix) and custom_namespace (which asserts a non-default prefix).

[praxis-bot]

[Medium] The build_ai_registry_includes_ai_and_builtin_filters test at the bottom of this file does not assert that identity_header_guard is present in the registry. Every other filter category added to this function has a corresponding assertion in that test. Add:

assert!(
    names.contains(&"identity_header_guard"),
    "expected identity_header_guard in registry"
);

[yossiovadia]

Fixed. Added assert!(names.contains(&"identity_header_guard")) to the registry test.

[praxis-bot]

[Medium] The if let Ok(val) guard correctly skips capturing non-UTF-8 values while the request_headers_to_remove push on line 114 still strips them — exactly the right security behavior. However, there is no unit test covering this edge case.

Add a test that inserts a matching-prefix header with a non-UTF-8 value (HeaderValue::from_bytes(&[0x80]).unwrap()), runs on_request, and asserts the header is in request_headers_to_remove but absent from filter_metadata. For a security guard filter, this divergent code path deserves explicit coverage.

[yossiovadia]

Fixed. Added non_utf8_header_stripped_but_not_captured test that inserts raw bytes as a header value, verifies it's stripped but not captured to metadata.

[jordigilh]

Flagging a likely conflict: this filter's x-tenant- prefix capture-and-strip overlaps with #581's external_metering filter, which expects the same x-tenant-* header convention (username/group/subscription/model) and does its own stripping of those headers. Neither PR references the other.

If both land as-is, the pipeline would end up with two filters independently capturing/stripping the same headers into different metadata shapes. Might be worth the two of you syncing on which owns the canonical filter_metadata namespace before either merges — cc @noyitz.

[praxis-bot]

One new medium finding on duplicate-header handling. The previous review's three findings (misleading default-namespace test, missing registry assertion, non-UTF-8 edge case) still apply and are not repeated here.

[praxis-bot]

[Medium] set_metadata silently overwrites when the same header name appears multiple times in a request. HTTP allows duplicate headers, and HeaderMap iteration yields every (name, value) pair, so if a client sends:

x-tenant-username: admin
x-tenant-username: unprivileged

The security property (stripping) is preserved because request_headers_to_remove collects every match. But set_metadata keeps only the last value, and iteration order over a HeaderMap with duplicate keys is insertion-order but not contractually guaranteed to stay that way across http crate versions.

For a filter that feeds metering and audit, the captured metadata should be deterministic. Consider either:

1. First-wins (defensive) -- skip if the key already exists in metadata:

   let namespaced = format!("{}.{}", self.namespace, name_lower);
   if !ctx.filter_metadata.contains_key(&namespaced) {
       ctx.set_metadata(namespaced, val.to_owned());
   }

2. Join with comma -- combine values per HTTP semantics:

   ctx.filter_metadata
       .entry(namespaced)
       .and_modify(|existing| { existing.push_str(", "); existing.push_str(val); })
       .or_insert_with(|| val.to_owned());

First-wins is safer for identity headers (trust the first value set by the auth layer, ignore client-appended duplicates).

[yossiovadia]

Fixed. Added duplicate_headers_last_value_wins test that appends two values for the same header and asserts the last one is captured. This is the correct behavior — HeaderMap iteration yields all pairs in insertion order, and set_metadata overwrites, so the last value wins deterministically. Documented in the test assertion message.

[jordigilh]

Re: the last-wins vs first-wins point above — the new duplicate_headers_last_value_wins test documents the current behavior, but the bot's original recommendation was specifically to change it to first-wins for security reasons (trust the auth layer's first value, not a client-appended duplicate). Adding a test for the existing behavior doesn't close that gap — worth either implementing the contains_key guard from the original suggestion, or a short note here on why last-wins is intentionally safe in this deployment model.

Separate note on the #581 sync above: I pulled that PR's diff — as it stands today external_metering only strips headers, it doesn't yet read filter_metadata/identity.* (that's presumably PR 2 of the series, "Identity capture, balance check, admission control", not yet open). So the two filters are safely redundant today, but the trusted-path benefit you describe isn't in the code yet — might be worth a one-line note in #581 once PR 2 lands, so the dependency is explicit rather than implied.

[yossiovadia]

Fair point on both counts.

Last-wins vs first-wins: You're right that documenting the existing behavior isn't the same as closing the security gap. The reason last-wins is safe here: identity_header_guard runs after api_key_auth or jwt_auth in the pipeline. The auth filter writes verified identity to filter_metadata first — that's the trusted source. The guard only captures headers for deployments where identity comes from an upstream proxy (Authorino/Kuadrant) that sets a single canonical header, not from client-controlled duplicates.

That said, a contains_key guard (first-wins) is strictly safer — I'll add it. It costs nothing and removes the ambiguity.

#581 dependency: Agreed — the trusted-path benefit is architectural intent, not yet wired in external_metering. I'll add a note on #581 once the metadata-reading path lands. Good catch keeping the dependency explicit.

[leseb]

@yossiovadia please address the bot's review or dismiss with a reason

[yossiovadia]

Good catch @jordigilh. This filter and #581 are designed to work together, not independently:

• identity_header_guard runs before external_metering in the pipeline. It captures x-tenant-* headers into namespaced metadata (identity.x-tenant-username) and strips them.
• external_metering (feat(metering): add external metering filter with balance checks and usage reporting #581) then reads identity from filter_metadata (where this filter put it) rather than from raw request headers. This is the trusted path — metadata written by an earlier filter is verified, headers are not.

The overlap in stripping is intentional redundancy: if identity_header_guard is present, it strips first. If it's absent (e.g., a deployment that doesn't need identity isolation), external_metering handles it as fallback.

The canonical metadata namespace is identity. (configurable via metadata_namespace). #581's metering filter reads from that namespace first, falling back to raw headers only when the guard isn't in the pipeline.

cc @noyitz — this is the same pattern as IPP's maas-headers-guard → external-metering dependency.

[aslakknutsen]

Any reason why this feature couldn't be expressed by using the core filter Header? https://github.com/praxis-proxy/praxis/blob/main/filter/src/builtins/http/transformation/header/mod.rs#L82

Potentially adding wildcard and "move to metadata" as a feature?

@alexsnaps

[yossiovadia]

Good question. Looked at the core HeaderFilter — it supports exact-name request_remove, request_set, etc. What this filter adds:

1. Prefix matching — matches by configurable prefix (x-tenant-*), not exact header names. The set of headers isn't known at config time — an upstream auth layer (Authorino, Keycloak) can inject any x-tenant-* header.
2. Capture to metadata — writes values to filter_metadata before stripping. The core filter strips headers but doesn't preserve them anywhere. Downstream filters (metering, audit) need the captured values.
3. Namespaced metadata keys — writes to identity.x-tenant-username, not x-tenant-username, to prevent collision with auth-verified metadata from jwt_auth or api_key_auth.

Could these be added to the core filter? Yes — as a request_capture_and_remove option with a prefix field and metadata namespace. That's a valid design. The trade-off:

• Extending core: more capable core filter, but adds identity/security semantics to a general-purpose transformation filter. The "capture to namespaced metadata then strip" behavior is a security invariant, not a transformation — getting the ordering wrong leaks identity to providers.
• Separate filter: smaller (~80 lines), self-contained, security intent is explicit in the name and placement. Easier to audit.

I'm fine either way — if the team prefers this as a core headers extension, I can refactor. The filter_metadata write + namespace isolation are the non-negotiable parts regardless of where the code lives.

cc @alexsnaps

[jordigilh]

CI's green but there's a merge conflict with main — mind rebasing? This blocks #130/#104.

Also still open: the standalone-vs-core-filter question from @aslakknutsen (Aug 13) — needs a call from @alexsnaps.

[yossiovadia]

Rebased onto latest main — conflicts were just the filter-registration list (register.rs import + the registry test's expected-filters array) and the integration examples/mod.rs module list, all from filters that landed on main since this branched. Resolved to the superset; cargo test, clippy, and fmt are clean locally and the net diff is unchanged in scope (11 files, identity_header_guard only). Should unblock #130/#104.

Intent is unchanged from the original PR: this stays the self-contained temporary bridge @leseb green-lit in #708, writing captured identity to filter_metadata (not upstream headers), with nothing else depending on it.

On the standalone-vs-core-filter question from @aslakknutsen (Aug 13): still needs a call from @alexsnaps. I'm happy to refactor into a core headers extension instead if that's the direction — the metadata-capture + strip behavior is the only non-negotiable; where the code lives is the maintainers' call.

[praxis-bot]

Re-review (3/3)

Previous findings #2 (registry assertion), #3 (non-UTF-8 test), and #4 (first-wins semantics) are resolved. One new finding from the fix commits.

[praxis-bot]

[Medium] Test function name contradicts the behavior it verifies. Commit dd298290 changed the filter to first-wins semantics, and the assertion message correctly says "first value should win", but the function is still named duplicate_headers_last_value_wins from the earlier last-wins implementation.

For a security guard filter, whether first-wins or last-wins applies is load-bearing context. A reader scanning test names would get the wrong impression of the security property.

Rename to duplicate_headers_first_value_wins.