SensitiveDataFilter fails open on dict keys, kwarg names, bytes, and sets

[higagan]

The default filter inspects dict values but silently passes several other carriers. Reproduced against current main (0.3.7):

dict KEY   {"password": "x"}      -> EXECUTED (fail-open)   # JSON-native, model-reachable
kwarg NAME send(api_key="abc")    -> EXECUTED (fail-open)
kwarg NAME send(password="x")     -> EXECUTED (fail-open)
bytes      b"my password"         -> EXECUTED (fail-open)
set        {"my password"}        -> EXECUTED (fail-open)

dict VALUE {"k": "my password"}   -> BLOCKED               # the one path that works

Three distinct defects, in priority order:

1. Dict keys are never inspected — and this one is model-reachable

SensitiveDataFilter._check_recursive in src/modelfuzz/rules.py iterates .values(). Dicts are JSON-native, so this is reachable straight from model-supplied tool arguments with no help from your own code:

import json
payload = json.loads('{"password": "hunter2"}')   # straight off the wire
send(data=payload)                                 # executes

That's semantically the exact payload the README quickstart demonstrates blocking. This sub-case is why the issue is labelled security.

2. Keyword-argument names are never inspected either

src/modelfuzz/decorator.py iterates list(args) + list(kwargs.values()) — the keys of kwargs never reach the engine:

send(api_key="abc")   # executes
send(password="x")    # executes

Correction (see discussion below): do not fix this by feeding names through the filter.
A parameter legitimately named password / api_key / secret would then trip on every
call — def login(username, password) would be permanently unusable regardless of the value.
"This parameter is sensitive" belongs in the signature-binding work, where a policy can be
scoped per-parameter, not in a blanket keyword match on names. Out of scope here.

3. Unrecognized types fail open

bytes, bytearray, set, frozenset, objects and dataclasses all fall through to the implicit return None, which the engine reads as allowed. A guardrail whose default for "I don't understand this value" is allow has the wrong default. Reachability here is narrower than for dicts (LLM arguments arrive as JSON, so these only appear if your own code builds them) — a real fail-open default with developer-dependent reachability.

Note

This is partly disclosed today — the README Limitations section states "only str, list, tuple, and dict values are inspected." So it's documented behaviour, not a hidden claim. But the dict-key case (#1) is model-reachable and semantically identical to the quickstart's own blocked example, so it's worth closing rather than only documenting.

Proposal — scope

In scope (value-side; strictly widens inspection, breaks nothing):

• Iterate .items() for dicts so keys are checked
• Explicit branches for bytes / bytearray (decode, then check)
• Traverse non-str iterables (set, frozenset) like list / tuple
• Optionally: objects via model_dump() / dataclasses.asdict() / __dict__

Out of scope — deliberately deferred:

• Pass keyword-argument names through from the decorator — see the correction above; it makes
  any tool with a password / api_key parameter permanently unusable.
• A final else that fails closed — that branch is reached by every ordinary scalar
  (int, float, bool, None, date, Path, a callback function), so blanket fail-closed
  would block routine tool calls for no security gain. If revisited, it needs to separate scalars
  that cannot carry a string (pass) from opaque objects that might (configurable, e.g.
  SensitiveDataFilter(on_uninspectable="allow"|"block"), defaulting to allow).

Acceptance criteria

• A parametrized test asserting dict keys, bytes, bytearray, set, frozenset are inspected
• send(data={"api_key": "sk-12345"}) is blocked
• An ordinary call passing int / float / bool / None still succeeds (regression guard)
• The README Limitations section is updated to match what is now inspected

[dyson-025]

Hi i would like to work on this issue

[dyson-025]

I have implemented the dict-key and kwarg-name fixes. For the remaining proposal (bytes, sets, object inspection, fail-closed), would you prefer those in the same PR or a follow-up? The fail-closed behavior changes the public behavior of SensitiveDataFilter, so I wanted to confirm before implementing it.

[higagan]

Great question, and you flagged exactly the right thing. Short answer: split it — keep the value-side fixes in your current PR, and leave fail-closed for a follow-up. But your instinct is worth more than that: checking before implementing caught a real flaw in how I specified this issue, and one of the two items you've already built needs rethinking.

Fail-closed is wrong as a blanket policy

You're right that it changes public behaviour, and it's worse than that — it would break almost every real tool. The else branch isn't only reached by exotic carriers; it's reached by every ordinary scalar. A completely mundane call:

realistic_tool("https://api.ok.com", "hello",
               retries=3, timeout=1.5, verify=True,
               when=date.today(), path=Path("/tmp/x"), cb=lambda: None)

passes int, float, bool, date, Path, and a function — none of which the filter recognises. Under fail-closed, that call gets blocked. An int provably cannot contain a keyword, so blocking it buys no safety and costs all usability.

If we ever do this, it needs to distinguish scalars that cannot carry a string (pass) from opaque objects that might (configurable), probably as something like SensitiveDataFilter(on_uninspectable="allow"|"block") defaulting to allow. That's a design decision, not a patch — please leave it out.

The kwarg-name fix has the same problem — my spec was wrong

This one I got wrong in the issue, and I'd rather say so than have you build it. Feeding parameter names through the filter means:

parameter named 'password'   -> TRIPS the filter
parameter named 'api_key'    -> TRIPS the filter
parameter named 'secret'     -> TRIPS the filter

So def login(username, password) called as login("bob", password="correct-horse") would be blocked on every call — the value is clean, but the parameter name trips it. The tool becomes permanently unusable. That's a footgun, and it's my fault for writing "pass keyword-argument names through" without thinking it to the end.

The real intent — "this parameter is sensitive" — belongs in the signature-binding work, where a policy can be scoped per-parameter (URLAllowList([...], params=["url"])), not in a blanket keyword match on names. Please drop the kwarg-name change; sorry for the wasted work.

What to include in this PR

Keep it to the value-side fixes. They strictly widen what the filter inspects, break nothing, and are unambiguously correct:

• dict keys via .items() — the model-reachable case, and the main reason this issue exists
• bytes / bytearray — decode, then check
• set / frozenset — traverse like list/tuple

That's a clean, self-contained PR. Object inspection (model_dump() / asdict() / __dict__) is fine to include if you want it, but it's also a reasonable follow-up — your call.

I'll update the issue body so the acceptance criteria match this rather than the original wording. Thanks for asking instead of just building it — that's the right reflex, and it saved us shipping a breaking change.