172 lines
7.3 KiB
Markdown
172 lines
7.3 KiB
Markdown
---
|
|
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_
|