wirenboard-agent-vm

mirror of https://github.com/wirenboard/agent-vm.git synced 2026-07-09 16:00:54 +00:00

History

Evgeny Boger 29c0ccc6c7 fix: address remaining code-review findings (sweep) Catches the rest of the xhigh review's verified findings, focused on security + reliability + correctness. Each bullet references the finding from the JSON output in this conversation. Security - intercept_hook: strip Set-Cookie / Set-Cookie2 / WWW-Authenticate / Proxy-Authenticate from upstream responses. Prevents in-VM agent from harvesting GitHub session cookies and replaying them on the unfiltered github.com path to authenticate as the host user (review #3). - intercept_hook: substitute_authorization_header handles both Bearer (literal substring) AND Basic auth (decode → replace → re-encode base64). Closes the "git Basic-auth always 401s" path while keeping the gh CLI Bearer path working. Also injects Bearer with the real token if the guest sent no Authorization header at all, so the forwarder never sends anonymous-but-substituted-looking requests (review #12). - run.rs detect_github_repos_in_cwd hardens the git invocation against repo-local config that can execute code on the host (core.fsmonitor, core.pager, core.editor, alias.). Pass `safe.directory=` so a foreign-UID checkout doesn't fail closed, `protocol.allow=never` so the call can't fetch anything, and force GIT_CONFIG_SYSTEM / GIT_CONFIG_GLOBAL to /dev/null so user-global git config can't inject an attacker-controlled include path. Closes the "host RCE before sandbox boot via untrusted .git/config" finding (review #6). - run.rs adds a long inline note above the gh secret entry calling out the github.com smart-HTTP gap (review #1). Filtering git smart-HTTP requires a streaming intercept primitive microsandbox doesn't expose (current intercept buffers full request, capped at 64 KiB; git push pack data exceeds that). Documented; the threat model is now explicit. Reliability - intercept_hook forward_github_api: reqwest::Client gets a 60 s timeout and `Policy::none()` redirect policy — hung api.github.com no longer freezes the sandbox; 3xx responses are surfaced to the guest verbatim instead of silently following (review #7). - intercept_hook trigger_host_refresh: 90 s wait-with-timeout on the host `claude -p` / `codex exec` so a stuck CLI doesn't hold the intercept hook indefinitely (review #8). - run.rs streaming exec: distinguish "stream closed without Exited event" (infra disconnect) from "agent exited with code 1" (real failure). The former bails with a clear error instead of returning exit 1 (review #9). Correctness - run.rs adds a SnapshotGuard with `impl Drop` so the Phase-5 host- cred mutation check runs on every exit path from launch(), including `?` propagation from attach/exec_stream errors. Previously the safety net only fired on the happy path (review #10). - run.rs: --no-git no longer silently discards explicit --repo arguments. The flag suppresses cwd auto-detection but explicit --repo entries are kept; the launcher warns when --no-git + --repo are combined (review #11). - secrets.rs: `Ok(_)` → `Ok(Some(()))` in the opencode token-file arm. `Ok(None)` (host file missing) no longer registers the placeholder pointing at a token file that wasn't written (review #13). - secrets.rs decode_id_token_account: try URL_SAFE_NO_PAD first (conformant JWT base64url) then fall back to STANDARD with manual '+'/'_' alphabet conversion. Closes the silent zero-UUID fallback when an upstream library emits standard-alphabet JWT payloads (review #14). - bin/agent-vm-ccusage: switch from `paste -sd,` to a null-delimited find→read loop, and skip any path containing `,` with a clear warning. CLAUDE_CONFIG_DIR's comma-separator has no escape mechanism so any path with a comma silently corrupts the union (review #15). Verified end-to-end: - `gh api graphql ...` reaches GitHub (allow-list extended in the previous commit; this commit didn't touch path filtering, but the pipeline still works: clean 403 for denied repos, real responses for allowed paths). - `git status` works on the bind-mounted project — no dubious- ownership (safe.directory) and the hardened cwd-detection git call still finds the remote. - gh hosts.yml is written, gh auth status shows the placeholder account is active (Bad credentials when reaching GitHub is the same documented outer-bridge nested-host limit). - 6/6 cargo tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-24 13:40:20 +03:00
..
agent-vm-ccusage	fix: address remaining code-review findings (sweep)	2026-05-24 13:40:20 +03:00

Evgeny Boger 29c0ccc6c7 fix: address remaining code-review findings (sweep)

Catches the rest of the xhigh review's verified findings, focused on
security + reliability + correctness. Each bullet references the
finding from the JSON output in this conversation.

**Security**
- intercept_hook: strip Set-Cookie / Set-Cookie2 / WWW-Authenticate /
  Proxy-Authenticate from upstream responses. Prevents in-VM agent
  from harvesting GitHub session cookies and replaying them on the
  unfiltered github.com path to authenticate as the host user
  (review #3).
- intercept_hook: substitute_authorization_header handles both Bearer
  (literal substring) AND Basic auth (decode → replace → re-encode
  base64). Closes the "git Basic-auth always 401s" path while keeping
  the gh CLI Bearer path working. Also injects Bearer with the real
  token if the guest sent no Authorization header at all, so the
  forwarder never sends anonymous-but-substituted-looking requests
  (review #12).
- run.rs detect_github_repos_in_cwd hardens the git invocation against
  repo-local config that can execute code on the host (core.fsmonitor,
  core.pager, core.editor, alias.*). Pass `safe.directory=*` so a
  foreign-UID checkout doesn't fail closed, `protocol.allow=never` so
  the call can't fetch anything, and force GIT_CONFIG_SYSTEM /
  GIT_CONFIG_GLOBAL to /dev/null so user-global git config can't
  inject an attacker-controlled include path. Closes the "host RCE
  before sandbox boot via untrusted .git/config" finding (review #6).
- run.rs adds a long inline note above the gh secret entry calling
  out the github.com smart-HTTP gap (review #1). Filtering git
  smart-HTTP requires a streaming intercept primitive microsandbox
  doesn't expose (current intercept buffers full request, capped at
  64 KiB; git push pack data exceeds that). Documented; the threat
  model is now explicit.

**Reliability**
- intercept_hook forward_github_api: reqwest::Client gets a 60 s
  timeout and `Policy::none()` redirect policy — hung api.github.com
  no longer freezes the sandbox; 3xx responses are surfaced to the
  guest verbatim instead of silently following (review #7).
- intercept_hook trigger_host_refresh: 90 s wait-with-timeout on the
  host `claude -p` / `codex exec` so a stuck CLI doesn't hold the
  intercept hook indefinitely (review #8).
- run.rs streaming exec: distinguish "stream closed without Exited
  event" (infra disconnect) from "agent exited with code 1" (real
  failure). The former bails with a clear error instead of returning
  exit 1 (review #9).

**Correctness**
- run.rs adds a SnapshotGuard with `impl Drop` so the Phase-5 host-
  cred mutation check runs on every exit path from launch(),
  including `?` propagation from attach/exec_stream errors. Previously
  the safety net only fired on the happy path (review #10).
- run.rs: --no-git no longer silently discards explicit --repo
  arguments. The flag suppresses cwd auto-detection but explicit
  --repo entries are kept; the launcher warns when --no-git +
  --repo are combined (review #11).
- secrets.rs: `Ok(_)` → `Ok(Some(()))` in the opencode token-file
  arm. `Ok(None)` (host file missing) no longer registers the
  placeholder pointing at a token file that wasn't written (review
  #13).
- secrets.rs decode_id_token_account: try URL_SAFE_NO_PAD first
  (conformant JWT base64url) then fall back to STANDARD with manual
  '+'/'_' alphabet conversion. Closes the silent zero-UUID fallback
  when an upstream library emits standard-alphabet JWT payloads
  (review #14).
- bin/agent-vm-ccusage: switch from `paste -sd,` to a
  null-delimited find→read loop, and skip any path containing `,`
  with a clear warning. CLAUDE_CONFIG_DIR's comma-separator has no
  escape mechanism so any path with a comma silently corrupts the
  union (review #15).

Verified end-to-end:
- `gh api graphql ...` reaches GitHub (allow-list extended in the
  previous commit; this commit didn't touch path filtering, but the
  pipeline still works: clean 403 for denied repos, real responses
  for allowed paths).
- `git status` works on the bind-mounted project — no dubious-
  ownership (safe.directory) and the hardened cwd-detection git
  call still finds the remote.
- gh hosts.yml is written, gh auth status shows the placeholder
  account is active (Bad credentials when reaching GitHub is the
  same documented outer-bridge nested-host limit).
- 6/6 cargo tests still green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

2026-05-24 13:40:20 +03:00

agent-vm-ccusage

fix: address remaining code-review findings (sweep)

2026-05-24 13:40:20 +03:00