Skip to content

fix: security boundary hardening, stream/scrollback bounds, SDK-compatible auth - #12

Merged
ForkedInTime merged 7 commits into
mainfrom
fix/qa-hardening-2026-08
Aug 2, 2026
Merged

fix: security boundary hardening, stream/scrollback bounds, SDK-compatible auth#12
ForkedInTime merged 7 commits into
mainfrom
fix/qa-hardening-2026-08

Conversation

@ForkedInTime

Copy link
Copy Markdown
Owner

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. PowerShellTool is in the live tool registry 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, ever
  • apply_sandbox was called from bash.rs and nowhere else — never sandboxed

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 it to SENSITIVE_TOOLS, teaches rule_matches/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 correctly 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. Sandbox failed open (96545c6) 🔴

apply_sandbox's _ 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 bad value.

Separately, 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.

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 one String; the MAX_OUTPUT_BYTES
check 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 bare read) 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_timeout was 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_compat also serves Ollama, all three streaming paths were exposed. Fixed once
via a shared next_sse_event() with a 120s inter-event budget — deliberately not a
whole-request timeout, so legitimate multi-minute streams are untouched.

app.entries had no cap (input_history did, at 500), so a long agent loop grew memory
monotonically. It is display-only state — the transcript lives in messages and persists to
the 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.rs used partial_cmp().unwrap() on f64; the release profile sets
panic = "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_KEY and nothing else, ignoring credentials the user may already
have configured for Claude Code, the official SDKs, or the ant CLI — all of which share one
documented order. Adds src/auth.rs implementing it:

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 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 token
goes in Authorization: Bearer and requires the oauth-2025-04-20 beta. 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 come from ant auth print-credentials --access-token rather than parsing
credentials/<profile>.json. That command refreshes the short-lived token before printing, so
there 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 a
warning rather than winning its slot and authenticating with an empty key. /doctor now
reports which source won and surfaces the "stale env var is shadowing your profile" trap.

91399e0 fixes a real defect this surfaced: the .env allowlist (SAFE_ENV_KEYS) contained
ANTHROPIC_API_KEY but not ANTHROPIC_AUTH_TOKEN or ANTHROPIC_PROFILE, so a project
authenticating with a token from .env would have had it silently dropped.
ANTHROPIC_BASE_URL stays excluded — it redirects every API call, so a hostile repo .env
could point real credentials at an attacker-controlled host.

Verified end-to-end against the live API, with HOME redirected to an empty directory so
no ~/.env could repopulate the variable:

Setup Result
both credentials set invalid x-api-key — key wins, sent as x-api-key
key absent, bogus token OAuth access token is invalid — sent as bearer, recognised as OAuth
key "", bogus token same — empty value correctly ignored
neither set new guidance message listing all four sources

Verification method

Every fix has a regression test that was confirmed to fail against the reintroduced bug:

  • removing PowerShell from SENSITIVE_TOOLS → 3 tests fail
  • restoring the fail-open sandbox arm and dropping the firejail flag → 3 tests fail
  • neutering scrollback eviction → 3 of 5 fail (the 2 that pass are the correctly-non-evicting cases)
  • removing the SSE timeout → the stall test hangs until killed (exit 124), the production symptom
  • reverting total_cmp → panics at the unwrap

Not addressed here

Left open in the review tracker, deliberately out of scope for this PR:

  • strict_check is a lowercase substring blocklist (rm -fr / passes) and is the automatic
    fallback on macOS/Windows
    , where neither bwrap nor firejail exists
  • bwrap omits --new-session (TIOCSTI keystroke-injection class)
  • hooks.rs has no timeout, no output cap, and fails open twice — a hook that fails to spawn
    returns allow(), and a signal-killed hook is treated as success via unwrap_or(0)
  • Workload Identity Federation (4th in the credential chain) is not implemented

Note for reviewers

env -u ANTHROPIC_API_KEY does not give you an absent variable in this codebase —
load_dotenv_auto() repopulates any allowlisted key from ~/.env when it is unset. Redirect
HOME to test a genuinely-absent credential. This cost a debugging cycle and is now recorded
in the tracker.

ForkedInTime and others added 7 commits August 2, 2026 11:51
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
ForkedInTime merged commit 95a7ca2 into main Aug 2, 2026
5 checks passed
@ForkedInTime
ForkedInTime deleted the fix/qa-hardening-2026-08 branch August 2, 2026 23:05
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant