7.3 KiB
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 04-auth-passthrough | 2026-04-10T00:00:00Z | standard | 1 |
|
|
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:
# 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:
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):
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:
--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:
# 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:
# 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:
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:
# 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:
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