feat(filter): add identity header guard filter - #709
Conversation
leseb
left a comment
There was a problem hiding this comment.
would core be more suitable for this instead of this repo? it's not so much "ai" related
praxis-bot
left a comment
There was a problem hiding this comment.
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.
|
Flagging a likely conflict: this filter's 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 |
praxis-bot
left a comment
There was a problem hiding this comment.
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.
|
@yossiovadia please address the bot's review or dismiss with a reason |
|
Good catch @jordigilh. This filter and #581 are designed to work together, not independently:
The overlap in stripping is intentional redundancy: if The canonical metadata namespace is cc @noyitz — this is the same pattern as IPP's |
|
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? |
|
Good question. Looked at the core
Could these be added to the core filter? Yes — as a
I'm fine either way — if the team prefers this as a core cc @alexsnaps |
|
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. |
ea02554 to
dd29829
Compare
|
Rebased onto latest Intent is unchanged from the original PR: this stays the self-contained temporary bridge @leseb green-lit in #708, writing captured identity to 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 |
a6bca42 to
86c4dc9
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
One new medium finding on untested validation logic. All previous findings are resolved.
86c4dc9 to
7e85535
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (5/5)
One new medium finding. The previous finding (#6, empty-namespace validation test) is still open and not repeated here.
8428c38 to
0389131
Compare
|
@yossiovadia please rebase |
Captures request 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 while making them available to downstream filters like external_metering. Fixes praxis-proxy#698 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
- Add registry assertion for identity_header_guard in build_ai_registry_includes_ai_and_builtin_filters test - Add test for non-UTF-8 header values (stripped but not captured) - Add test for duplicate headers (last-value-wins behavior) - Clarify default namespace test with comment explaining the indirect verification via captures_matching_headers_to_metadata Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
…filters Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Reflowed the use list after the rebase conflict resolution; the manual wrap did not match nightly rustfmt's line-filling. Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
0389131 to
e44cc69
Compare
|
Rebased onto current main (post-#581). Conflict resolution was limited to the three registration sites (register.rs import list, examples/README.md table, integration suite mod.rs) — all both-sides-added-a-line merges, verified against the xtask sync checks. All gates green locally: filter unit + doctests + integration example, clippy (both feature sets), nightly fmt, machete, xtask suite. One note for reviewers: this force-push also removes four previously stacked, now-superseded commits (MCP metadata block / credential redaction, #436/#438/#578, plus a chores commit) — upstream reimplemented that redaction/SSRF work in faa043f. The PR now contains only the identity_header_guard work. @shaneutt fresh eyes when you get a chance — the rebase supersedes the earlier review. |
|
@leseb can you confirm and approve this one? |
Summary
identity_header_guardfilter that captures headers matching a configurable prefix intofilter_metadataand strips them before upstream forwardingx-tenant-username,x-tenant-group) from leaking to LLM providersexternal_metering(feat(filter): add external metering filter for usage reporting and balance checks #577)Fixes #698
Motivation
The
external_meteringfilter 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_headersin core only handles hardcodedx-praxis-*prefixes with no metadata capture and no configurable prefixes (core TODO #186).Design
x-tenant-)filter_metadataunder a configurable namespace (prevents collision with verified auth metadata)request_headers_to_removeWhat's included
filters/src/identity_guard/— filter, config, 11 unit tests + 1 doctesttests/integration/tests/suite/examples/identity_header_guard.rs— 3 integration tests (config parse + header capture + strip)examples/configs/identity-header-guard.yaml— example configTest plan
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 warningsexternal_meteringconsuming captured identity