Skip to content

fix: prevent tests from writing to real ~/.spawn/history.json#2420

Closed
la14-1 wants to merge 2 commits intomainfrom
fix/test-fs-guardrails
Closed

fix: prevent tests from writing to real ~/.spawn/history.json#2420
la14-1 wants to merge 2 commits intomainfrom
fix/test-fs-guardrails

Conversation

@la14-1
Copy link
Copy Markdown
Member

@la14-1 la14-1 commented Mar 10, 2026

Summary

Builds on #2417. Tests that call cmdRun/saveSpawnRecord without explicitly setting SPAWN_HOME were writing to the real ~/.spawn/history.json on the developer's machine.

  • Set SPAWN_HOME to the sandbox in preload.ts as the global default — no individual test can miss it
  • Add fs-sandbox.test.ts guardrail that verifies HOME, SPAWN_HOME, and XDG_CACHE_HOME all point to the temp sandbox
  • Update .claude/rules/testing.md with mandatory filesystem isolation rules (never import homedir from node:os, always use process.env.HOME, etc.)

3 test files were silently modifying ~/.spawn/history.json:

  • commands-error-paths.test.ts
  • commands-resolve-run.test.ts
  • commands-swap-resolve.test.ts

Test plan

  • bunx @biomejs/biome check src/ — 0 errors
  • bun test — 1469 pass, 0 fail
  • Verified ~/.spawn/history.json mtime unchanged before/after full test suite
  • New fs-sandbox.test.ts passes (5 assertions)

🤖 Generated with Claude Code

louisgv and others added 2 commits March 10, 2026 07:06
Bun's os.homedir() reads from getpwuid() and ignores runtime changes to
process.env.HOME. Named imports capture the native function binding, so
patching os.homedir on the default export doesn't propagate. This caused
all test files using homedir() to write .spawn-test-* dirs to the real
home directory instead of the preload sandbox.

Add getUserHome() helper to shared/ui.ts that prefers process.env.HOME,
replace all direct homedir() calls in production and test code.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test preload now sets SPAWN_HOME to the sandbox directory by default,
so tests that call cmdRun/saveSpawnRecord without explicitly setting
SPAWN_HOME no longer write to the real ~/.spawn/history.json.

Add fs-sandbox.test.ts that verifies the sandbox is correctly configured
(HOME, SPAWN_HOME, XDG vars all point to temp). Update testing.md with
mandatory filesystem isolation rules.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@louisgv louisgv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review

Verdict: APPROVED
Commit: 5588a21

Findings

No security vulnerabilities found. This PR implements defensive security improvements by isolating test filesystem writes to temporary directories.

What this PR does:

  • Adds test sandbox preload that redirects HOME/SPAWN_HOME/XDG_* to temp directories
  • Prevents tests from accidentally writing to real user files (~/.spawn/history.json, ~/.ssh/, etc.)
  • Introduces getUserHome() helper to work around Bun's os.homedir() ignoring process.env.HOME
  • Adds fs-sandbox.test.ts guardrail to verify the sandbox is active
  • Replaces all test imports of 'homedir' from node:os with process.env.HOME

Security impact: ✅ POSITIVE - prevents test pollution and protects developer environments

Tests

  • bun test: ✅ PASS (1469 tests pass, 0 fail)
  • bash -n: N/A (no shell scripts modified)

-- security/pr-reviewer

@louisgv
Copy link
Copy Markdown
Member

louisgv commented Mar 10, 2026

Merge Conflicts Detected

This PR has been approved from a security perspective but cannot be merged due to conflicts with the main branch.

Required action: Please rebase this PR on the latest main branch to resolve the conflicts.

git fetch origin
git rebase origin/main
git push --force-with-lease

Once the conflicts are resolved, the PR can be merged immediately (all tests pass, security review complete).


-- security/pr-reviewer

@la14-1
Copy link
Copy Markdown
Member Author

la14-1 commented Mar 10, 2026

Superseded by a new PR with merge conflicts resolved and rebased on current main. Also adds root-level bunfig.toml so bun test works from the repo root.

@la14-1 la14-1 closed this Mar 10, 2026
@la14-1 la14-1 deleted the fix/test-fs-guardrails branch March 10, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants