claudebox/.planning/phases/03-sandbox-aware-prompting/03-REVIEW.md

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
03-sandbox-aware-prompting	2026-04-09T12:00:00Z	standard	1	claudebox.sh	critical warning info total 1 3 1 5	issues_found

Phase 3: Code Review Report

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

Summary

Reviewed claudebox.sh, the main wrapper script for sandboxed Claude Code execution. The script is well-structured with a clear security model (allowlist-based env, bwrap isolation, secret masking). Found one critical resource leak caused by exec bypassing the EXIT trap, two security-adjacent warnings around gitconfig trust and the env escape hatch, and a masking logic issue that can leak most of a short secret.

Critical Issues

CR-01: EXIT trap never fires -- temp gitconfig leaks on every invocation

File: claudebox.sh:160-161 Issue: The script sets trap 'rm -f "$GITCONFIG_TMP"' EXIT at line 160, but the script always terminates via exec bwrap at line 326. exec replaces the current process, so the EXIT trap never executes. A new temp file is leaked in /tmp on every claudebox invocation. Fix: The gitconfig tmpfile is bind-mounted read-only into the sandbox, so it must exist for the lifetime of the bwrap process. Clean it up by running bwrap in the background, waiting, then cleaning up -- or write it to a deterministic path that gets overwritten each launch:

# Option A: Use a fixed path instead of mktemp (simplest)
GITCONFIG_TMP="$HOME/.claudebox/.gitconfig.tmp"
# Remove the trap entirely -- file is overwritten each launch

# Option B: Fork instead of exec, clean up after
bwrap ... &
BWRAP_PID=$!
wait "$BWRAP_PID"
EXIT_CODE=$?
rm -f "$GITCONFIG_TMP"
exit "$EXIT_CODE"

Option A is recommended -- it eliminates the leak with no complexity cost and the file lives in the user-owned .claudebox directory.

Warnings

WR-01: safe.directory = * trusts all git directories

File: claudebox.sh:167 Issue: The generated gitconfig sets safe.directory = *, which tells git to trust ownership of any directory. While the sandbox limits mounted paths, this is broader than necessary -- only $CWD needs to be trusted. If mount scope changes in the future, this becomes a real risk. Fix:

cat > "$GITCONFIG_TMP" <<GITEOF
[user]
    name = $GIT_NAME
    email = $GIT_EMAIL
[safe]
    directory = $CWD
GITEOF

WR-02: CLAUDEBOX_EXTRA_ENV can smuggle secrets past the allowlist

File: claudebox.sh:213-223 Issue: The CLAUDEBOX_EXTRA_ENV escape hatch passes any named host variable into the sandbox without validation. A parent process (or shell profile) setting CLAUDEBOX_EXTRA_ENV=AWS_SECRET_ACCESS_KEY,GH_TOKEN would inject secrets that the core allowlist intentionally excludes. The audit display provides visibility but not prevention. Fix: Consider a denylist check for known secret-bearing variable names:

DENY_PATTERN="^(AWS_|GH_TOKEN|GITHUB_TOKEN|SSH_|GPG_|AGE_)"
for var in "${EXTRAS[@]}"; do
  var="${var// /}"
  if [[ "$var" =~ $DENY_PATTERN ]]; then
    echo "${RED}Blocked${RESET} $var -- secret variable cannot be passed via CLAUDEBOX_EXTRA_ENV" >&2
    continue
  fi
  # ... existing logic
done

WR-03: mask_value leaks most of short secrets

File: claudebox.sh:83-86 Issue: The masking threshold is 11 characters. For a 12-character secret, mask_value displays the first 7 and last 4 characters (11 of 12 visible). This effectively leaks the entire value. The issue scales: a 15-char secret shows 11 of 15 characters. Fix: Raise the threshold so the masked portion is always the majority of the value:

mask_value() {
  local name="$1" value="$2"
  local upper="${name^^}"
  if [[ "$upper" == *KEY* || "$upper" == *TOKEN* || "$upper" == *SECRET* || "$upper" == *PASSWORD* || "$upper" == *CREDENTIAL* ]]; then
    if (( ${#value} > 20 )); then
      echo "${value:0:4}...${value: -4}"
    else
      echo "***"
    fi
  else
    echo "$value"
  fi
}

Info

IN-01: --dangerously-skip-permissions is always passed to claude

File: claudebox.sh:287 Issue: The --dangerously-skip-permissions flag is unconditionally added to the Claude command. This is intentional per the design (bwrap IS the permission boundary), but it means the sandbox's security depends entirely on the bwrap mount list being correct. Worth documenting this invariant prominently. Fix: Add a comment explaining the security invariant:

# Security invariant: --dangerously-skip-permissions is safe here because
# bwrap's mount list is the security boundary, not Claude's permission system.
# If you change the mount list, audit what Claude can access.

---

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