claudebox/.planning/phases/02-env-audit-and-cli-polish/02-REVIEW.md

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
02-env-audit-and-cli-polish	2026-04-09T12:00:00Z	standard	2	claudebox.sh flake.nix	critical warning info total 1 3 2 6	issues_found

Phase 2: Code Review Report

Reviewed: 2026-04-09 Depth: standard Files Reviewed: 2 Status: issues_found

Summary

Reviewed claudebox.sh (298 lines, shell script) and flake.nix (49 lines, Nix expression). The shell script is well-structured with good separation of concerns (flag parsing, audit display, sandbox exec). The Nix flake is clean and idiomatic.

Key concerns: the CLAUDEBOX_EXTRA_ENV escape hatch allows passing arbitrary environment variables into the sandbox without any blocklist, which undermines the core security model. There are also several robustness issues around variable handling and the dry-run output not matching actual execution.

Critical Issues

CR-01: CLAUDEBOX_EXTRA_ENV allows smuggling secrets into sandbox

File: claudebox.sh:162-172 Issue: The CLAUDEBOX_EXTRA_ENV escape hatch accepts any variable name without validation. A user (or a parent process) could set CLAUDEBOX_EXTRA_ENV=SSH_AUTH_SOCK,GPG_AGENT_INFO,AWS_SECRET_ACCESS_KEY and those values would be injected into the sandbox. This directly violates the project's core constraint: "Secrets never enter the Claude Code environment." While mask_value hides sensitive values in the audit display, the actual values are still passed via --setenv. An attacker or misconfigured environment could bypass the entire allowlist model. Fix: Add a blocklist check before accepting extra vars:

# Blocklist of vars that must never enter the sandbox
BLOCKED_VARS="SSH_AUTH_SOCK|SSH_AGENT_PID|GPG_AGENT_INFO|GPG_TTY|AWS_SECRET_ACCESS_KEY|AWS_SESSION_TOKEN|GITHUB_TOKEN|GH_TOKEN|TAILSCALE_.*"

for var in "${EXTRAS[@]}"; do
  var="${var// /}"
  if [[ -n "$var" ]] && [[ -v "$var" ]]; then
    if [[ "$var" =~ ^($BLOCKED_VARS)$ ]]; then
      echo "${RED}Blocked:${RESET} $var is not allowed in sandbox" >&2
      continue
    fi
    ENV_ARGS+=(--setenv "$var" "${!var}")
    AUDIT_EXTRA_KEYS+=("$var")
    AUDIT_EXTRA_VALS[$var]="${!var}"
  fi
done

Warnings

WR-01: Dry-run output diverges from actual bwrap invocation

File: claudebox.sh:240-272 Issue: The dry-run block manually reconstructs the bwrap command with echo statements (lines 242-270) rather than deriving it from the same data used by the actual exec bwrap call (lines 275-298). If a mount is added or changed in the real invocation, the dry-run output will silently become stale. The --bind /nix/var/nix mount and the --ro-bind /etc/nix mount are both present in both places today, but this is a maintenance hazard -- the two blocks must be kept in sync manually. Fix: Build the bwrap args array once, then use it for both dry-run printing and actual execution:

BWRAP_ARGS=(
  --clearenv
  "${ENV_ARGS[@]}"
  --tmpfs /
  --proc /proc
  # ... all mounts ...
  -- "${SANDBOX_CMD[@]}"
)

if [[ "$DRY_RUN" == true ]]; then
  printf 'bwrap'
  printf ' %q' "${BWRAP_ARGS[@]}"
  printf '\n'
  exit 0
fi

exec bwrap "${BWRAP_ARGS[@]}"

WR-02: Temp file cleanup race with exec

File: claudebox.sh:108-109 Issue: Line 109 sets trap 'rm -f "$GITCONFIG_TMP"' EXIT to clean up the temp gitconfig. However, line 275 uses exec bwrap ... which replaces the current process -- the EXIT trap never fires. The temp file leaks on every successful run. This is not a security issue (the file contains only name/email), but it accumulates stale files in /tmp. Fix: Either accept the leak (temp files are cleaned by the OS on reboot) and remove the misleading trap, or delete the file after bwrap starts by restructuring to not use exec:

# Option A: Remove the misleading trap, add a comment
GITCONFIG_TMP=$(mktemp)
# Note: leaked intentionally -- exec replaces process, trap won't fire.
# /tmp is cleaned on reboot. File contains only git name/email.

WR-03: Unquoted variables in dry-run echo statements

File: claudebox.sh:264-268 Issue: Several dry-run echo statements use unquoted shell variables: $HOME, $CWD. If these contain spaces (unlikely for HOME but possible for CWD), the dry-run output would be misleading -- it would show split words rather than the actual path. The real exec bwrap call properly quotes these on lines 293-297. Fix:

echo "  --tmpfs \"$HOME\" \\"
echo "  --bind \"$HOME/.claudebox\" \"$HOME/.claude\" \\"
echo "  --bind \"$CWD\" \"$CWD\" \\"
echo "  --chdir \"$CWD\" \\"

Info

IN-01: Hardcoded system architecture in flake.nix

File: flake.nix:18 Issue: system = "x86_64-linux"; hardcodes the architecture. This prevents the flake from being used on aarch64-linux or other systems. This is fine if claudebox is strictly for the developer's NixOS desktop, but limits portability. Fix: Use flake-utils or nixpkgs.lib.genAttrs to support multiple systems:

nixpkgs.lib.genAttrs ["x86_64-linux" "aarch64-linux"] (system: ...)

IN-02: pass variable in --check uses string comparison instead of boolean

File: claudebox.sh:23,31,56 Issue: The pass variable is set to the string true/false and compared with [[ "$pass" == true ]]. This works in bash but is a code smell -- it looks like a boolean but is a string. Minor readability concern, no functional impact given writeShellApplication ensures bash. Fix: No change needed -- this is idiomatic bash. Noting for awareness only.

---

Reviewed: 2026-04-09 Reviewer: Claude (gsd-code-reviewer) Depth: standard