fix: security boundary hardening, stream/scrollback bounds, SDK-compatible auth - #12
Merged
Conversation
Three production-readiness fixes ahead of wider deployment. 1. SSE streams could hang forever (src/api) Both streaming backends awaited the next event with no bound. Only connect_timeout was set, and deliberately no overall request timeout — correct, since legitimate streams run for minutes. But that left no guard at all once connected: a silently dropped TCP connection (NAT idle reaper, laptop sleep, VPN drop) parked the read future forever. The TUI hung with no error and no recovery short of killing it. Adds next_sse_event(), an *inter-event* timeout (120s) shared by the Anthropic backend and the OpenAI-compat backend — the latter also serves Ollama, so all three streaming paths are covered by one guard. A healthy connection always delivers a delta, ping, or keepalive well inside that window, so valid long streams are unaffected. 2. TUI scrollback grew without bound (src/tui) input_history was capped at 500; entries never was. A long agent loop grew it monotonically, taking render cost up with it. entries is display state only — the transcript lives in the query engine's messages and is persisted to the session file — so eviction loses no conversation data, exactly like terminal scrollback. Trims at 2000 while following the newest content (invisible to the user), and defers to a hard 10000 ceiling while scrolled up so history is not yanked out mid-read. Enforced at the single pre-draw choke point every mutation path already passes through, rather than at ~40 push sites. 3. Cost summary could abort the process (src/cost.rs) sort_by used partial_cmp().unwrap() on f64. Release sets panic = "abort", so a non-finite cost would kill the session just to render a report. NaN is unreachable today; total_cmp costs nothing and removes the failure mode permanently. Also applies the 4 outstanding clippy lints (collapsible if-let chains, match-to-if-let), leaving clippy --all-targets --all-features clean. Adds 11 tests. Each was verified to fail against its reintroduced bug: the cost test panics at the unwrap, 3 of 5 scrollback tests fail when eviction is neutered, and the stall test hangs until killed (exit 124) without the timeout — the production symptom exactly. Suite: 454 passed, 0 failed. Release builds at 19.1 MB. Co-Authored-By: Arch Linux <noreply@archlinux.org>
…on bad mode
Phase 1 of the phased review (security & sandbox boundary, 1,286 LOC).
Five files, none of which had inline tests. Zero panics in scope.
1. PowerShell bypassed BOTH security controls (most severe)
`PowerShellTool` is in the live tool registry (tools/mod.rs:453) and
executes arbitrary commands, but:
- "PowerShell" was absent from SENSITIVE_TOOLS, so check_with_input
returned Allow on the non-sensitive early-return — no approval
prompt at all, ever.
- apply_sandbox was called from bash.rs and nowhere else, so an
enabled sandbox had no effect on it.
On any machine with pwsh installed, the model could run shell commands
unprompted and unsandboxed. Explicit deny rules still applied (they are
checked before the early return), but the default posture was open.
Adds PowerShell to SENSITIVE_TOOLS, teaches rule_matches and
describe_tool_call about it, and routes it through a new
sandbox::guard_unwrappable_tool. The namespace wrappers hard-code
/bin/sh -c and cannot wrap a PowerShell script, so that gate applies
pattern blocking in every mode and fails closed under bwrap/firejail
rather than running outside the sandbox the user enabled.
2. apply_sandbox failed OPEN on an unrecognised mode
The `_` arm returned the command unchanged — unsandboxed AND skipping
strict_check — while the UI still reported the sandbox as enabled.
Reachable in practice: `/sandbox enable` validates the mode string but
settings.json does not, so a typo in `sandboxMode` silently disabled
the sandbox. Now fails closed and names the offending value.
3. firejail silently ignored sandbox_allow_network
firejail_wrap took no allow_network parameter, so firejail mode always
had full egress while bwrap honoured the setting — the same config
meaning two different things. Adds --net=none.
4. Bash: unbounded output buffering (OOM)
BufReader::lines() accumulates until a newline, so newline-free output
(`yes | tr -d '\n'`) buffered the whole stream into one String; the
MAX_OUTPUT_BYTES check ran per line and never tripped.
Capping the reader alone is wrong — the child then blocks on a full
pipe and every command over the cap burns the whole timeout (measured:
60s vs 1.2s). Switches to chunked reads so output is bounded while the
pipe keeps draining and the child still exits. Line splitting keeps
partial lines as bytes so multi-byte UTF-8 spanning a chunk boundary
is not mangled.
5. Bash + PowerShell inherited the TUI's stdin
stdin defaulted to inherit, so an interactive command (sudo, ssh, a
bare `read`) competed with crossterm for the user's keystrokes and hung
until the timeout. Redirected from /dev/null so it hits EOF instead.
Adds 19 tests. Each verified to fail against its reintroduced bug:
removing PowerShell from SENSITIVE_TOOLS fails 3, restoring the fail-open
arm and dropping the firejail flag fails 3, and the output-bounds tests
regress from 1.2s to a 60s timeout.
Suite: 486 passed, 0 failed. Clippy clean.
Deferred to the tracker (not fixed here): strict_check is a substring
blocklist and is the automatic fallback on macOS/Windows where neither
bwrap nor firejail exists; bwrap omits --new-session; hooks have no
timeout, no output cap, and treat both a spawn failure and a
signal-killed hook as "allow".
Co-Authored-By: Arch Linux <noreply@archlinux.org>
… tokens
RustyClaw read ANTHROPIC_API_KEY and nothing else, so it ignored credentials
the user may already have configured for Claude Code, the official SDKs, or
the `ant` CLI — all of which share one documented resolution order.
Adds src/auth.rs implementing that order:
ANTHROPIC_API_KEY → ANTHROPIC_AUTH_TOKEN → `ant auth login` profile
RustyClaw's own explicit mechanisms (RUSTYCLAW_API_KEY_FILE_DESCRIPTOR,
apiKeyHelper) keep their existing position between the env vars and the
profile: explicit local configuration should beat ambient machine state,
and nothing that worked before changes behaviour.
Wire format now follows the credential kind. A static key goes in
`x-api-key`; an OAuth access token goes in `Authorization: Bearer` and
additionally requires the `oauth-2025-04-20` beta. Sending both auth
headers is rejected by the API, so exactly one is ever set. The beta is
merged into the per-request `anthropic-beta` rather than set as a default
header, because reqwest's `header()` appends — a default plus a per-request
value would send the field twice.
Profile tokens are read via `ant auth print-credentials --access-token`
rather than by parsing credentials/<profile>.json. That command refreshes
the short-lived token before printing, so there is no OAuth refresh flow to
implement, and we stay on a supported interface instead of an on-disk format
that is an implementation detail. Bounded at 10s so a wedged binary cannot
hang startup.
Deliberate divergence from the SDKs: an empty ANTHROPIC_API_KEY="" falls
through to the next source with a warning, rather than winning its slot and
authenticating with an empty key (which 401s confusingly). /doctor now
reports which source won and surfaces the "stale env var is shadowing your
profile" trap, which otherwise silently sends requests to a different
org/workspace.
Resolution is a pure function over an injected AuthEnv, so ordering is
tested without mutating process env (which races under the parallel harness)
or requiring `ant` on PATH.
Adds 18 tests. Verified live against the real API:
- ANTHROPIC_API_KEY + ANTHROPIC_AUTH_TOKEN both set → "invalid x-api-key"
(key wins, sent as x-api-key)
- ANTHROPIC_API_KEY="" + bogus token → "OAuth access token is invalid"
(empty key ignored; token sent as bearer and recognised AS an OAuth
token, confirming the beta header is present)
Suite: 522 passed, 0 failed. Clippy clean.
KNOWN GAP — not yet explained: with ANTHROPIC_API_KEY fully *unset* (rather
than empty) a bogus ANTHROPIC_AUTH_TOKEN does not produce the expected 401.
Those two paths should be identical in resolve_stage. Something downstream
(model routing, or a second config path in headless -p mode) appears to be
intervening. Do not treat OAuth as fully verified until this is understood.
Workload Identity Federation (4th in the documented chain) is not
implemented.
Co-Authored-By: Arch Linux <noreply@archlinux.org>
…ication gap Resolves the "known gap" flagged in 8b3feea. There was no bug in the resolver — the test methodology was wrong. `env -u ANTHROPIC_API_KEY` does not produce an absent variable, because load_dotenv_auto() then populates it from ~/.env (it only skips keys where `std::env::var(key).is_err()`). Setting ANTHROPIC_API_KEY="" instead leaves it *set*, so dotenv declines to overwrite it and the empty value falls through to the token. That is why the two cases diverged. Re-verified with HOME redirected to an empty directory so no ~/.env exists and the variable is genuinely absent: API_KEY=Err(NotPresent), AUTH_TOKEN set → resolved is_oauth=true, source=ANTHROPIC_AUTH_TOKEN → API returns "OAuth access token is invalid" Both credential paths and their precedence are now confirmed end-to-end against the live API. OAuth support is verified, not provisional. The investigation did surface a real defect: SAFE_ENV_KEYS (the .env allowlist) contained ANTHROPIC_API_KEY but not ANTHROPIC_AUTH_TOKEN or ANTHROPIC_PROFILE. A project authenticating with an OAuth token from .env would have had it silently dropped and fallen back to whatever key was in the ambient environment — the exact confusion this feature exists to remove. Both are now allowed. ANTHROPIC_BASE_URL stays excluded, with a comment saying why: it redirects every API call, so a hostile repo .env could point real credentials at an attacker-controlled host. Adds 2 tests: one asserting the allowlist covers the whole credential chain, one asserting the base-URL redirect stays out of it. Suite: 524 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux <noreply@archlinux.org>
…nd them Phase 1 findings 1.9 and 1.10. `execute_hook` had two silent fail-open paths and no resource bounds at all, on a code path that runs before every tool call. Fail-open #1 — spawn failure returned allow() A hook that could not start (missing binary, typo, E2BIG) hit `HookResult::allow()` after a `tracing::warn!` the user never sees in the TUI. A PreToolUse hook is a gate; one that never ran has not approved anything, so a broken gate silently disabled itself. Fail-open #2 — signal-killed hooks read as success `status.code().unwrap_or(0)` maps None (terminated by signal) to exit 0, the documented "allow" code. A PreToolUse hook killed by the OOM killer — or by anything else that can signal it — was read as approval. This is the one an attacker would reach for. Both now route through `hook_unevaluable`, which fails closed **only for gating events**. `is_gating_event` is `PreToolUse` alone: it is the only event that can block a tool call, so it is the only one where an unevaluable hook is a security-relevant outcome. Notification, Stop, PostToolUse and friends stay non-blocking — failing closed there would break sessions for no safety benefit. The documented exit-code contract is unchanged: 0 allows, 2 blocks, other non-zero stays a non-blocking error. Fail-closed applies to hooks that could not be *evaluated*, not to hooks that ran and reported failure. Resource bounds (1.9), all previously absent: - 60s timeout. A hook waiting on input, a network call, or a lock blocked the agent forever with no diagnostic. On timeout the whole process group is killed so anything the hook spawned dies with it. - stdout/stderr capped at 256 KB per stream, but still drained to EOF — capping without draining would leave the hook blocked on a full pipe until the timeout, turning a fast hook into a 60s stall. Both pipes are read concurrently; draining one to EOF first deadlocks if the hook fills the other. - stdin from /dev/null. Inherited stdin let a hook that reads input compete with the TUI for the user's keystrokes. - env values capped at 64 KB. Linux limits one env entry to ~128 KB (MAX_ARG_STRLEN), so a large TOOL_INPUT pushed spawn to E2BIG — which, under the old fail-open, meant the PreToolUse gate was skipped precisely on the largest tool calls. Verified: removing the cap makes the regression test fail. Adds 12 tests. Verified against each reintroduced bug: restoring `unwrap_or(0)` fails the signal test, restoring fail-open fails 2, removing the env cap fails the oversized-input test. One test was initially too weak to catch the env-cap regression — it asserted only that the call was blocked, which is true whether the hook ran and blocked or spawn failed and fail-closed caught it. It now asserts the block did not come from the fail-closed path. Suite: 548 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux <noreply@archlinux.org>
Fixes the Windows CI regression introduced by 8b3feea. `main` was green on windows-latest; this branch was not. Root cause is a name collision. `ant` is Apache Ant on many systems — including the GitHub windows-latest runner image. Credential resolution called `Command::new("ant")` unconditionally whenever no API key was set, so on every credential-less startup RustyClaw executed whatever `ant` happened to be on PATH: an unrelated build tool, twice, each with a 10s timeout. That delay pushed `test_health_check_via_headless` past its 10s response window (the job ran 77s before failing). Wrong for reasons beyond CI: executing an arbitrary PATH binary on startup is a poor default even when it is harmless, and it is pure cost for the common case where the user has never run `ant auth login`. Now gated on the credentials directory existing — `$ANTHROPIC_CONFIG_DIR`, else `~/.config/anthropic` on Unix or `%APPDATA%\Anthropic` on Windows. A cheap `is_dir()` replaces a process spawn, so nothing is executed unless the real CLI has actually stored a profile on this machine. The same gate serves `ant_profile_present`, which no longer needs to shell out at all. Timeout also tightened 10s → 5s as defence in depth. Verified: with no credential env and a nonexistent config dir, the previously-failing sdk_integration test passes locally in 3.9s. Adds 2 tests — one asserting no subprocess is attempted when the profile directory is absent, one covering the ANTHROPIC_CONFIG_DIR override. Suite: 552 passed, 0 failed. Clippy clean. Co-Authored-By: Arch Linux <noreply@archlinux.org>
Windows CI failure from 6eb9e68. `ExitStatus::code()` returns `Some` on Windows for every exit path — there is no POSIX signal termination — so the `unwrap_or(0)` fail-open being tested cannot occur there, and `kill -9 $$` does not produce a signal-terminated child. Both the defect and the regression test are Unix-only concepts. The production guard itself needs no cfg: on Windows the `else` branch is simply unreachable. The rest of the hooks suite is portable and passes on windows-latest (196 passed, this was the only failure). Co-Authored-By: Arch Linux <noreply@archlinux.org>
ForkedInTime
added a commit
that referenced
this pull request
Aug 3, 2026
…#15) Phase 1 re-audit. Three defects the first pass missed, two exploitable. - split_compound_command ignored newline and bare '&' as separators, so an allow-rule like Bash(prefix:git ) auto-approved 'git status\nrm -rf /' — the whole string still starts with the allowed prefix. Defeats the purpose of prefix rules entirely. - PowerShell got prefix rules in #12 but compound checking was dispatched only for Bash, so 'Get-Process; Remove-Item -Recurse C:\' was auto-allowed under a Get- rule. That regression was introduced by the previous fix. - stream_tx is unbounded and emit_line forwarded every line before the cap check, so #12's output bounding covered the captured buffer only, not memory. - PowerShell had drifted from Bash on two already-fixed classes: unbounded output read, and no kill on timeout. Both now reuse Bash's ProcessGroupGuard and bounded reader. Also closed two coverage gaps found while verifying: the run.rs dispatch was untested (now a testable is_command_tool predicate asserted against SENSITIVE_TOOLS), and nothing measured the streaming channel. 588 tests, 0 failures. Clippy clean. Zero panics in production code across all five Phase 1 files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four commits from a phased review of the codebase. Two are security fixes, one closes a
class of hangs and an OOM, one adds credential resolution matching the official SDKs.
524 tests pass, 0 failures. Clippy clean at
--all-targets --all-features.Every new test was verified to fail against its reintroduced bug — details per section.
1. PowerShell bypassed both security controls (
96545c6) 🔴The most serious finding.
PowerShellToolis in the live tool registry and executesarbitrary commands, but:
"PowerShell"was absent fromSENSITIVE_TOOLS, socheck_with_inputreturnedAllowon the non-sensitive early return — no approval prompt, ever
apply_sandboxwas called frombash.rsand nowhere else — never sandboxedOn any machine with
pwshinstalled, the model could run shell commands unprompted andunsandboxed. Explicit
denyrules still applied (they are checked before the early return),but the default posture was open.
Adds it to
SENSITIVE_TOOLS, teachesrule_matches/describe_tool_callabout it, and routesit through a new
sandbox::guard_unwrappable_tool. The namespace wrappers hard-code/bin/sh -cand cannot correctly wrap a PowerShell script, so that gate applies patternblocking in every mode and fails closed under bwrap/firejail rather than running outside
the sandbox the user enabled.
2. Sandbox failed open (
96545c6) 🔴apply_sandbox's_arm returned the command unchanged — unsandboxed and skippingstrict_check— while the UI still reported the sandbox as enabled. Reachable in practice:/sandbox enablevalidates the mode string butsettings.jsondoes not, so a typo insandboxModesilently disabled the sandbox. Now fails closed and names the bad value.Separately,
firejail_wraptook noallow_networkparameter, so firejail mode always hadfull egress while bwrap honoured the setting — the same config meaning two different things.
3. Unbounded output buffering, and inherited stdin (
96545c6)BufReader::lines()accumulates until it sees a newline, so newline-free output(
yes | tr -d '\n') buffered the whole stream into oneString; theMAX_OUTPUT_BYTEScheck ran per line and never tripped.
Capping the reader alone is the wrong fix — the child then blocks on a full pipe and every
command over the cap burns the entire timeout. Measured: 60s vs 1.2s. Switched to chunked
reads so output is bounded while the pipe keeps draining, and the child still exits. Partial
lines are carried as bytes so multi-byte UTF-8 spanning a chunk boundary is not mangled.
Both Bash and PowerShell also inherited the TUI's stdin, so an interactive command (
sudo,ssh, a bareread) competed with crossterm for the user's keystrokes and hung until timeout.Now redirected from
/dev/null.4. SSE streams could hang forever; TUI scrollback grew without bound (
c920568)Both streaming backends awaited the next event with no bound — only
connect_timeoutwas set.A silently dropped TCP connection (NAT reaper, laptop sleep, VPN drop) parked the read future
forever: the UI hung with no error and no recovery. The defect was in two backends, and
since
openai_compatalso serves Ollama, all three streaming paths were exposed. Fixed oncevia a shared
next_sse_event()with a 120s inter-event budget — deliberately not awhole-request timeout, so legitimate multi-minute streams are untouched.
app.entrieshad no cap (input_historydid, at 500), so a long agent loop grew memorymonotonically. It is display-only state — the transcript lives in
messagesand persists tothe session file — so eviction loses no conversation data. Trims at 2000 while following the
newest content, deferring to a hard 10000 ceiling while scrolled up so history is not yanked
out mid-read.
Also
cost.rsusedpartial_cmp().unwrap()onf64; the release profile setspanic = "abort", so a non-finite cost would have killed the session to render a report.5. Credential resolution matching the official SDKs (
8b3feea,91399e0)RustyClaw read
ANTHROPIC_API_KEYand nothing else, ignoring credentials the user may alreadyhave configured for Claude Code, the official SDKs, or the
antCLI — all of which share onedocumented order. Adds
src/auth.rsimplementing it:RustyClaw's own explicit mechanisms (
RUSTYCLAW_API_KEY_FILE_DESCRIPTOR,apiKeyHelper) keeptheir existing position between the env vars and the profile — explicit local config should
beat ambient machine state, and nothing that worked before changes.
Wire format now follows the credential kind: a static key goes in
x-api-key; an OAuth tokengoes in
Authorization: Bearerand requires theoauth-2025-04-20beta. The beta ismerged into the per-request
anthropic-betarather than set as a default header, becausereqwest's
header()appends — a default plus a per-request value would send the field twice.Profile tokens come from
ant auth print-credentials --access-tokenrather than parsingcredentials/<profile>.json. That command refreshes the short-lived token before printing, sothere is no OAuth refresh flow to implement, and it keeps us on a supported interface
instead of an on-disk format that is an implementation detail. Bounded at 10s so a wedged
binary cannot hang startup.
Deliberate divergence from the SDKs: an empty
ANTHROPIC_API_KEY=""falls through with awarning rather than winning its slot and authenticating with an empty key.
/doctornowreports which source won and surfaces the "stale env var is shadowing your profile" trap.
91399e0fixes a real defect this surfaced: the.envallowlist (SAFE_ENV_KEYS) containedANTHROPIC_API_KEYbut notANTHROPIC_AUTH_TOKENorANTHROPIC_PROFILE, so a projectauthenticating with a token from
.envwould have had it silently dropped.ANTHROPIC_BASE_URLstays excluded — it redirects every API call, so a hostile repo.envcould point real credentials at an attacker-controlled host.
Verified end-to-end against the live API, with
HOMEredirected to an empty directory sono
~/.envcould repopulate the variable:invalid x-api-key— key wins, sent asx-api-keyOAuth access token is invalid— sent as bearer, recognised as OAuth"", bogus tokenVerification method
Every fix has a regression test that was confirmed to fail against the reintroduced bug:
PowerShellfromSENSITIVE_TOOLS→ 3 tests failtotal_cmp→ panics at theunwrapNot addressed here
Left open in the review tracker, deliberately out of scope for this PR:
strict_checkis a lowercase substring blocklist (rm -fr /passes) and is the automaticfallback on macOS/Windows, where neither bwrap nor firejail exists
--new-session(TIOCSTI keystroke-injection class)hooks.rshas no timeout, no output cap, and fails open twice — a hook that fails to spawnreturns
allow(), and a signal-killed hook is treated as success viaunwrap_or(0)Note for reviewers
env -u ANTHROPIC_API_KEYdoes not give you an absent variable in this codebase —load_dotenv_auto()repopulates any allowlisted key from~/.envwhen it is unset. RedirectHOMEto test a genuinely-absent credential. This cost a debugging cycle and is now recordedin the tracker.