Skip to content

Enforce test sandbox isolation: nothing prevents a helper from reaching the real home #362

Description

@bguidolim

Problem

Environment(home:) is the isolation seam for tests, but nothing enforces that every layer honours it. A single helper that resolves the process home voids the sandbox for every caller above it — and when that helper writes, swift test mutates the developer's real files.

This already happened. GitignoreManager.resolveGlobalGitignorePath() resolved FileManager.default.homeDirectoryForCurrentUser and ran git config --global core.excludesFile with the inherited HOME, so a correctly-injected GitignoreCheck still read and wrote the real ~/.config/git/ignore. It surfaced as a CI failure that looked platform-specific and never reproduced locally — the developer machine happened to have the file, so the check always passed there. Fixed in #361, but only for that one type.

Known remaining gaps

  • ComponentExecutor holds an injected environment but its already-installed short-circuit calls component.supplementaryChecks(nil, Environment()) with a fresh default, so supplementary checks evaluate against the real ~/ during sync and in ComponentExecutorTests. One-line fix, but it is the same class of defect.
  • Ten test sites construct a bare Environment(). Several are benign (ShellRunnerTests and ScriptRunnerTests exercise the shell itself; ExternalPackAdapterTests only builds check objects without running them). The ones worth auditing are ProjectSyncTests, ComponentExecutorTests, ExternalDoctorCheckTests, and the defaulted parameter in TestHelpers.
  • ExternalPackAdapter defaults its shell to ShellRunner(environment: Environment()), so an adapter built without an explicit sandboxed shell reads the real home — the pre-existing "known gap" for pack-level supplementary checks.

Sources are otherwise clean: the only remaining NSHomeDirectory() is the legitimate default inside Environment itself.

Proposed enforcement

Two complementary mechanisms — the first catches the syntactic form, the second catches a path computed some other way.

1. SwiftLint custom_rules. Ban homeDirectoryForCurrentUser and NSHomeDirectory() outside Environment.swift, and ban a bare Environment() under Tests/. CI already runs swiftlint --strict, so this becomes a hard gate at no extra cost.

Blocker to sequence first: .swiftlint.yml currently has excluded: [Tests], so no rule can police the test directory. Un-excluding it will surface unrelated pre-existing violations and should be its own commit.

2. A CI tripwire around swift test. Record existence + mtime of ~/.claude, ~/.mcs, and ~/.config/git/ignore before and after the suite; fail if any changed. Catches escapes a grep cannot see, needs no lint config change, but does not identify which test was responsible.

A full container/sandbox-exec jail was considered and rejected as too heavy for macOS CI — it would also fight the tests that legitimately shell out to git.

Acceptance criteria

  • ComponentExecutor passes its stored environment to supplementaryChecks
  • Test sites that drive read/write paths use an injected sandbox home
  • Lint rules reject process-home lookups outside Environment.swift and bare Environment() in tests
  • Tests no longer excluded from SwiftLint (separate commit — expect unrelated violations)
  • CI fails if a test run modifies ~/.claude, ~/.mcs, or ~/.config/git/ignore

Reference

Fixed instance and diagnosis: #361.

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Normal prioritytech-debtTechnical debt and code quality improvementstest-gapMissing test coverage

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions