From 112f604856e8444fa4e27a263f1690cdb18308b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20M=C3=BChl?= Date: Fri, 10 Apr 2026 09:25:49 +0000 Subject: [PATCH] docs(04): add code review report --- .../phases/04-auth-passthrough/04-REVIEW.md | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 .planning/phases/04-auth-passthrough/04-REVIEW.md diff --git a/.planning/phases/04-auth-passthrough/04-REVIEW.md b/.planning/phases/04-auth-passthrough/04-REVIEW.md new file mode 100644 index 0000000..7fff506 --- /dev/null +++ b/.planning/phases/04-auth-passthrough/04-REVIEW.md @@ -0,0 +1,172 @@ +--- +phase: 04-auth-passthrough +reviewed: 2026-04-10T00:00:00Z +depth: standard +files_reviewed: 1 +files_reviewed_list: + - claudebox.sh +findings: + critical: 2 + warning: 2 + info: 1 + total: 5 +status: issues_found +--- + +# Phase 04: Code Review Report + +**Reviewed:** 2026-04-10 +**Depth:** standard +**Files Reviewed:** 1 +**Status:** issues_found + +## Summary + +This review covers the phase 4 credential passthrough changes to `claudebox.sh`. The diff introduces AUTH-01/AUTH-02: detecting `~/.claude/.credentials.json` on the host and conditionally bind-mounting it into the sandbox. The refactor also converts the inline `exec bwrap ...` to a `BWRAP_ARGS` array for conditional mount support, and unifies the env audit display format. + +Two critical issues were found: the credential path resolution is wrong on any system where `~/.claude` and `~/.claudebox` are different directories (the detection silently fails with `CREDS_MOUNT=false`), and the credential file is mounted read-write when read-only is correct. A mount-ordering issue with the symlink target path may also prevent the credentials bind from working at all on some bwrap versions. + +--- + +## Critical Issues + +### CR-01: CREDS_FILE path resolves against the wrong directory on the host + +**File:** `claudebox.sh:105` + +**Issue:** `CREDS_FILE` is set to `$HOME/.claude/.credentials.json`. On the host, `~/.claude` is the real Claude config directory — **not** `~/.claudebox`. The symlink `~/.claude -> ~/.claudebox` only exists *inside* the sandbox (it is created by the `--symlink` bwrap flag at runtime). Before the sandbox is entered, `$HOME/.claude` and `$HOME/.claudebox` are independent paths. + +If the user's credentials live in `~/.claude/.credentials.json` (the default Claude Code location) and `~/.claudebox` is a separate directory, the `-f "$CREDS_FILE"` test on line 106 may still succeed — but the file being tested and the file actually mounted will be the real `~/.claude/.credentials.json`, which is then bound into `$HOME/.claude/.credentials.json` inside the sandbox. Since the sandbox maps `$HOME/.claudebox` → `$HOME/.claude` (symlink), the bind target `$HOME/.claude/.credentials.json` is a symlink path that bwrap must traverse, creating the mount-ordering hazard in CR-02. + +More importantly, if the intent is to read credentials from `~/.claudebox/.credentials.json` (i.e. the claudebox-managed config dir), the detection path is wrong and will silently miss it. The correct host path to check is: + +```bash +# Correct: resolve against the claudebox config dir, not ~/.claude +CREDS_FILE="$HOME/.claudebox/.credentials.json" +``` + +If the intent is to read from the real `~/.claude`, that is correct as written, but then the mount target inside the sandbox must use the canonical path, not the symlink path (see CR-02). + +**Fix:** + +Choose one consistent interpretation. If credentials come from `~/.claudebox`: + +```bash +CREDS_FILE="$HOME/.claudebox/.credentials.json" +if [[ -f "$CREDS_FILE" ]]; then + CREDS_MOUNT=true +else + CREDS_MOUNT=false +fi +``` + +And mount it to the canonical destination (not through the symlink): + +```bash +BWRAP_ARGS+=(--ro-bind "$CREDS_FILE" "$HOME/.claudebox/.credentials.json") +``` + +--- + +### CR-02: Credentials bind target uses a symlink path — may silently fail + +**File:** `claudebox.sh:364` + +**Issue:** The credentials bind is: + +```bash +--bind "$CREDS_FILE" "$HOME/.claude/.credentials.json" +``` + +Inside the bwrap namespace, `$HOME/.claude` is a symlink (created by `--symlink "$HOME/.claudebox" "$HOME/.claude"` on line 361). Bwrap processes mount arguments in order and creates the symlink before this bind, but bwrap does **not** resolve symlinks in the destination path when applying `--bind` — it uses the literal path. The literal path `$HOME/.claude/.credentials.json` refers to a path whose parent is a dangling symlink at mount time (the symlink exists, but the directory component `$HOME/.claude/` is not a real directory). This means the bind either silently fails or errors, depending on bwrap version. + +The fix is to bind the file directly into the canonical directory path `$HOME/.claudebox/.credentials.json`, which is the real directory: + +```bash +# Wrong: target is through a symlink +BWRAP_ARGS+=(--bind "$CREDS_FILE" "$HOME/.claude/.credentials.json") + +# Correct: target is the real directory +BWRAP_ARGS+=(--ro-bind "$CREDS_FILE" "$HOME/.claudebox/.credentials.json") +``` + +The same fix applies to the `--dry-run` output path on line 331. + +--- + +## Warnings + +### WR-01: Credentials file mounted read-write instead of read-only + +**File:** `claudebox.sh:364` + +**Issue:** `--bind` creates a read-write mount. Claude Code needs to **read** credentials, not write them. Mounting the credentials file read-write gives the sandboxed agent the ability to overwrite or corrupt the host's credential store. This contradicts the project's principle of least privilege — the sandbox should only receive what it needs. + +**Fix:** + +```bash +# Change --bind to --ro-bind +BWRAP_ARGS+=(--ro-bind "$CREDS_FILE" "$HOME/.claudebox/.credentials.json") +``` + +The same applies in the `--dry-run` output block (line 331) — update `--bind` to `--ro-bind` there for accurate documentation of actual behavior. + +--- + +### WR-02: dry-run ENV_ARGS loop hard-codes stride of 3 — breaks if any non-setenv arg is added + +**File:** `claudebox.sh:309-312` + +**Issue:** The dry-run printer iterates `ENV_ARGS` with a fixed stride of 3: + +```bash +while (( dry_run_i < ${#ENV_ARGS[@]} )); do + printf ' %s %s %q \\\n' "${ENV_ARGS[$dry_run_i]}" "${ENV_ARGS[$((dry_run_i+1))]}" "${ENV_ARGS[$((dry_run_i+2))]}" + (( dry_run_i += 3 )) +done +``` + +This assumes every element is a `--setenv NAME VALUE` triplet. If any future env arg uses a 2-element form (e.g. `--unsetenv NAME`, `--clearenv`), the loop will misalign and print mangled output or access out-of-bounds indices. The `BWRAP_ARGS` construction path handles variadic args correctly via array appending; the dry-run printer does not. + +This is a latent fragility bug. It does not currently trigger because `ENV_ARGS` only contains `--setenv` triples, but it is a maintenance trap. + +**Fix:** Build the dry-run ENV section by iterating the same conditional logic used to populate `ENV_ARGS`, or use a parallel dry-run-safe array instead of re-parsing `ENV_ARGS`: + +```bash +# Print each --setenv entry (stride-3 is safe only while all entries are --setenv) +# Guard with an assertion: +if (( ${#ENV_ARGS[@]} % 3 != 0 )); then + echo "BUG: ENV_ARGS length ${#ENV_ARGS[@]} is not a multiple of 3" >&2 + exit 1 +fi +``` + +Or restructure to eliminate the re-parsing entirely. + +--- + +## Info + +### IN-01: Audit display reports credentials path as the symlink path, not the host path + +**File:** `claudebox.sh:268` + +**Issue:** The mounts section of `print_audit` displays: + +``` +credentials $HOME/.claude/.credentials.json (read-write) +``` + +`$HOME/.claude` on the host is the real Claude config directory (not a symlink), so the path shown is technically correct. However, once inside the sandbox `$HOME/.claude` becomes a symlink to `.claudebox`, making the display confusing when users compare the audit output with what they see on their host filesystem. Displaying the actual host source path (`$CREDS_FILE`) would be more accurate: + +```bash +printf ' %-12s %s (read-only)\n' "credentials" "$CREDS_FILE" >&2 +``` + +This also reflects the read-only fix from WR-01. + +--- + +_Reviewed: 2026-04-10_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_