diff --git a/.github/workflows/installer-smoke.yml b/.github/workflows/installer-smoke.yml index f0a661d71..2110df668 100644 --- a/.github/workflows/installer-smoke.yml +++ b/.github/workflows/installer-smoke.yml @@ -17,13 +17,13 @@ on: permissions: {} -# The post-install skill step reaches for the OS keyring; on a headless macOS -# runner the `security` tool blocks forever on the locked keychain (caught by -# this canary's first run). Also keeps the Windows legs out of Credential -# Manager. Same escape hatch the Test workflow uses. -env: - BASECAMP_NO_KEYRING: "1" - +# Deliberately NO workflow-level BASECAMP_NO_KEYRING: the deployed installer +# sets it per-command for its own best-effort children (the belt for release +# binaries <= v0.7.2, whose eager startup keyring probe hangs on a locked +# headless keychain), and the canary must exercise that deployed belt — on the +# macOS leg a missing belt means a hung `security` child, a 10-minute timeout, +# and a notify. The direct `basecamp version` asserts don't need the var: +# help/version skip the CLI's PersistentPreRunE and never reach the probe. concurrency: group: installer-smoke-${{ github.ref }} cancel-in-progress: false diff --git a/e2e/installer.bats b/e2e/installer.bats index 5310adf0e..ee10cd96e 100644 --- a/e2e/installer.bats +++ b/e2e/installer.bats @@ -48,6 +48,25 @@ write_stub() { chmod +x "$STUB_DIR/basecamp" } +# write_nk_stub emits a stub that logs BASECAMP_NO_KEYRING alongside argv, for +# asserting the escape-hatch belt. Separate from write_stub so the existing +# argv-shape assertions stay byte-exact. mode mirrors write_stub's new/old. +write_nk_stub() { + local mode="$1" + { + echo '#!/usr/bin/env bash' + echo "echo \"nk=\${BASECAMP_NO_KEYRING:-unset} \$@\" >> \"$LOG\"" + echo 'if [[ "$1 $2" == "setup --help" ]]; then' + if [[ "$mode" == "new" ]]; then + echo ' echo " agents Install the Basecamp skill and connect detected coding agents"' + fi + echo ' exit 0' + echo 'fi' + echo 'exit 0' + } > "$STUB_DIR/basecamp" + chmod +x "$STUB_DIR/basecamp" +} + run_post_install_setup() { run bash -c " set -euo pipefail @@ -157,4 +176,102 @@ run_post_install_setup() { # Explicit claude|codex selectors must be capability-checked before dispatch, # so an old binary never gets an unadvertised subcommand as a stray arg. grep -qF 'match "(?m)^\s+$selector\s"' "$INSTALL_PS1" + # The keyring escape hatch belt (see the BASECAMP_NO_KEYRING tests below). + grep -q 'BASECAMP_NO_KEYRING' "$INSTALL_PS1" +} + +# Release binaries up to v0.7.2 probe the OS keyring on startup for every +# command, and a locked headless keychain blocks that probe forever (#568 +# canary discovery). The installer's best-effort children never touch +# credentials, so they must carry BASECAMP_NO_KEYRING=1 — per-command, not +# blanket: the `setup --help` capability probe stays bare (help +# short-circuits before the probe). e2e/run.sh exports BASECAMP_NO_KEYRING +# suite-wide, so the test shell must unset it first or the stub would see +# nk=1 even without the installer's belt. +@test "new binary: real setup calls carry BASECAMP_NO_KEYRING=1, capability probe does not" { + write_nk_stub new + run bash -c " + set -euo pipefail + unset BASECAMP_NO_KEYRING + source '$INSTALL_SH' + BIN_DIR='$STUB_DIR' + post_install_setup basecamp + cat '$LOG' + " + [[ "$status" -eq 0 ]] + [[ "$output" == *"nk=unset setup --help"* ]] + [[ "$output" == *"nk=1 setup agents"* ]] +} + +@test "old binary: skill install fallback carries BASECAMP_NO_KEYRING=1" { + write_nk_stub old + run bash -c " + set -euo pipefail + unset BASECAMP_NO_KEYRING + source '$INSTALL_SH' + BIN_DIR='$STUB_DIR' + post_install_setup basecamp + cat '$LOG' + " + [[ "$status" -eq 0 ]] + [[ "$output" == *"nk=unset setup --help"* ]] + [[ "$output" == *"nk=1 skill install"* ]] +} + +# The Windows canary can never prove the ps1 belt — Credential Manager works +# headlessly with or without it — so pin the behavior here. The function under +# test is extracted from install.ps1's AST and evaluated alone: Main never +# runs, and the installer needs no test hooks. +@test "install.ps1 Invoke-PostInstallSetup sets and restores BASECAMP_NO_KEYRING" { + if ! command -v pwsh >/dev/null 2>&1; then + if [[ -n "${CI:-}" ]]; then + # Fail closed: a runner image dropping pwsh must surface as a failure, + # not silent coverage loss. + echo "pwsh is required in CI for install.ps1 belt coverage" >&2 + return 1 + fi + skip "pwsh not installed" + fi + + PS_LOG="$STUB_DIR/ps-calls.log" + + cat > "$STUB_DIR/basecamp-stub.ps1" <<'EOF' +$rest = $args -join ' ' +$nk = if ($null -eq $env:BASECAMP_NO_KEYRING) { 'unset' } else { $env:BASECAMP_NO_KEYRING } +Add-Content -LiteralPath $env:PS_LOG "nk=$nk $rest" +if ($rest -eq 'setup --help') { + ' agents Install the Basecamp skill and connect detected coding agents' +} +EOF + + cat > "$STUB_DIR/driver.ps1" <<'EOF' +$ErrorActionPreference = 'Stop' +$tokens = $null; $parseErrors = $null +$ast = [System.Management.Automation.Language.Parser]::ParseFile($env:INSTALL_PS1_PATH, [ref]$tokens, [ref]$parseErrors) +if ($parseErrors.Count -gt 0) { throw "install.ps1 parse errors: $($parseErrors -join '; ')" } +$fn = $ast.Find({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq 'Invoke-PostInstallSetup' }, $true) +if (-not $fn) { throw 'Invoke-PostInstallSetup not found in install.ps1' } +# Evaluate only the function definition — the installer's Main never runs. +. ([scriptblock]::Create($fn.Extent.Text)) +Invoke-PostInstallSetup $env:PS_STUB +if ($null -ne $env:BASECAMP_NO_KEYRING) { throw "BASECAMP_NO_KEYRING not restored: '$env:BASECAMP_NO_KEYRING'" } +'restored-ok' +# Second pass: a caller-set value must survive, not just absence. +$env:BASECAMP_NO_KEYRING = '0' +Invoke-PostInstallSetup $env:PS_STUB +if ($env:BASECAMP_NO_KEYRING -ne '0') { throw "existing BASECAMP_NO_KEYRING value not restored: '$env:BASECAMP_NO_KEYRING'" } +'restored-value-ok' +EOF + + run bash -c " + set -euo pipefail + unset BASECAMP_NO_KEYRING + export PS_LOG='$PS_LOG' PS_STUB='$STUB_DIR/basecamp-stub.ps1' INSTALL_PS1_PATH='$INSTALL_PS1' + pwsh -NoProfile -File '$STUB_DIR/driver.ps1' + cat '$PS_LOG' + " + [[ "$status" -eq 0 ]] + [[ "$output" == *"restored-ok"* ]] + [[ "$output" == *"restored-value-ok"* ]] + [[ "$output" == *"nk=1 setup agents"* ]] } diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index e180b6141..6fcc52a42 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -22,25 +22,47 @@ type Credentials struct { } // Store wraps credstore.Store with typed Credentials marshaling. +// +// Construction of the underlying credstore.Store is deferred to first use: +// credstore.NewStore probes the OS keyring availability with a write, and on +// a locked keychain with no TTY or GUI (headless macOS) that probe blocks +// forever in an uncancellable `security` child process. Credential-free +// commands must never pay it. type Store struct { - inner *credstore.Store - warnOnce sync.Once + fallbackDir string + initOnce sync.Once + inner *credstore.Store + warnOnce sync.Once } -// NewStore creates a credential store. +// newCredStore is replaceable in tests to avoid real keyring access. +var newCredStore = credstore.NewStore + +// NewStore creates a credential store. The OS keyring is not touched until +// the first credential operation. func NewStore(fallbackDir string) *Store { - s := credstore.NewStore(credstore.StoreOptions{ - ServiceName: "basecamp", - DisableEnvVar: "BASECAMP_NO_KEYRING", - FallbackDir: fallbackDir, + return &Store{fallbackDir: fallbackDir} +} + +// ensure constructs the underlying store on first use. Callers reach here +// from paths that don't hold Manager.mu (e.g. IsAuthenticated), so the +// sync.Once provides the synchronization. +func (s *Store) ensure() *credstore.Store { + s.initOnce.Do(func() { + s.inner = newCredStore(credstore.StoreOptions{ + ServiceName: "basecamp", + DisableEnvVar: "BASECAMP_NO_KEYRING", + FallbackDir: s.fallbackDir, + }) }) - return &Store{inner: s} + return s.inner } // warnFallback prints the keyring fallback warning once, on first credential write. func (s *Store) warnFallback() { + inner := s.ensure() s.warnOnce.Do(func() { - if w := s.inner.FallbackWarning(); w != "" { + if w := inner.FallbackWarning(); w != "" { fmt.Fprintf(os.Stderr, "warning: %s\n", w) } }) @@ -48,7 +70,7 @@ func (s *Store) warnFallback() { // Load retrieves credentials for the given origin. func (s *Store) Load(origin string) (*Credentials, error) { - data, err := s.inner.Load(origin) + data, err := s.ensure().Load(origin) if err != nil { return nil, err } @@ -66,14 +88,14 @@ func (s *Store) Save(origin string, creds *Credentials) error { if err != nil { return err } - return s.inner.Save(origin, data) + return s.ensure().Save(origin, data) } // Delete removes credentials for the given origin. -func (s *Store) Delete(origin string) error { return s.inner.Delete(origin) } +func (s *Store) Delete(origin string) error { return s.ensure().Delete(origin) } // MigrateToKeyring migrates credentials from file to keyring. -func (s *Store) MigrateToKeyring() error { return s.inner.MigrateToKeyring() } +func (s *Store) MigrateToKeyring() error { return s.ensure().MigrateToKeyring() } // UsingKeyring returns true if the store is using the system keyring. -func (s *Store) UsingKeyring() bool { return s.inner.UsingKeyring() } +func (s *Store) UsingKeyring() bool { return s.ensure().UsingKeyring() } diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go new file mode 100644 index 000000000..08a08f26b --- /dev/null +++ b/internal/auth/keyring_test.go @@ -0,0 +1,46 @@ +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/cli/credstore" +) + +// swapNewCredStore replaces the credstore constructor seam for the test. +func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) *credstore.Store) { + t.Helper() + orig := newCredStore + newCredStore = fn + t.Cleanup(func() { newCredStore = orig }) +} + +// TestNewStoreIsLazy proves the constructor never touches credstore: +// credstore.NewStore probes the OS keyring with a write, and that probe can +// block forever on a locked headless keychain. This test pins constructor +// laziness only — the "credential-free commands never probe" conclusion +// additionally rests on those command paths (setup agents, skill install, +// version, help) not performing credential operations, which is audited at +// the command layer, not asserted here. +func TestNewStoreIsLazy(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring + + calls := 0 + swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store { + calls++ + return credstore.NewStore(opts) + }) + + store := NewStore(t.TempDir()) + require.NotNil(t, store) + assert.Equal(t, 0, calls, "NewStore must not construct the credstore") + + _, err := store.Load("https://test.example.com") + require.Error(t, err, "no credentials saved") + assert.Equal(t, 1, calls, "first operation constructs the credstore") + + store.UsingKeyring() + assert.Equal(t, 1, calls, "construction happens once") +} diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 4efb14be8..61e274b37 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -222,35 +222,45 @@ function Test-InteractiveSession { # binary supports; unset/auto/ambiguous installs the shared skill only. Each # native call is individually guarded so a nonzero exit never aborts the install. function Invoke-PostInstallSetup([string]$Binary) { - try { $help = & $Binary setup --help 2>$null } catch { $help = '' } + # None of these calls touch credentials, but release binaries up to v0.7.2 + # probe the OS keyring on startup for every command, and a locked headless + # keychain blocks that probe forever. Set the escape hatch for the duration + # of setup only, restoring the caller's value (or absence) on the way out. + $savedNoKeyring = $env:BASECAMP_NO_KEYRING + $env:BASECAMP_NO_KEYRING = '1' + try { + try { $help = & $Binary setup --help 2>$null } catch { $help = '' } - if ($help -match '(?m)^\s+agents\s') { - try { & $Binary setup agents } catch { } - return - } + if ($help -match '(?m)^\s+agents\s') { + try { & $Binary setup agents } catch { } + return + } - $selector = $env:BASECAMP_SETUP_AGENT - if ($selector -in @('claude', 'codex')) { - # Capability-check first: an old `setup` parent accepts an unadvertised agent - # id as a stray arg and launches the INTERACTIVE wizard. Degrade to the skill. - if ($help -match "(?m)^\s+$selector\s") { - try { & $Binary setup $selector } catch { } + $selector = $env:BASECAMP_SETUP_AGENT + if ($selector -in @('claude', 'codex')) { + # Capability-check first: an old `setup` parent accepts an unadvertised agent + # id as a stray arg and launches the INTERACTIVE wizard. Degrade to the skill. + if ($help -match "(?m)^\s+$selector\s") { + try { & $Binary setup $selector } catch { } + } else { + try { & $Binary skill install } catch { } + } + } elseif ($selector -eq 'all') { + $ranAgent = $false + foreach ($agent in @('claude', 'codex')) { + if ($help -match "(?m)^\s+$agent\s") { + # Mark attempted (not succeeded) — matches install.sh's `ran_agent=1`, + # which is set regardless of the setup call's exit status. + $ranAgent = $true + try { & $Binary setup $agent } catch { } + } + } + if (-not $ranAgent) { try { & $Binary skill install } catch { } } } else { try { & $Binary skill install } catch { } } - } elseif ($selector -eq 'all') { - $ranAgent = $false - foreach ($agent in @('claude', 'codex')) { - if ($help -match "(?m)^\s+$agent\s") { - # Mark attempted (not succeeded) — matches install.sh's `ran_agent=1`, - # which is set regardless of the setup call's exit status. - $ranAgent = $true - try { & $Binary setup $agent } catch { } - } - } - if (-not $ranAgent) { try { & $Binary skill install } catch { } } - } else { - try { & $Binary skill install } catch { } + } finally { + $env:BASECAMP_NO_KEYRING = $savedNoKeyring } } diff --git a/scripts/install.sh b/scripts/install.sh index 6b0dd5a6a..e15730f44 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -499,12 +499,18 @@ binary_supports_setup_agent() { # Claude-first bug — only an *explicitly* selected agent is connected. `all` # runs every per-agent setup the binary supports; an unset, auto, or ambiguous # selector installs the shared skill only (`skill install`). +# +# Every real invocation carries BASECAMP_NO_KEYRING=1, per-command rather than +# exported: these calls never touch credentials, but release binaries up to +# v0.7.2 probe the OS keyring on startup for every command, and on a locked +# headless keychain (CI, ssh) that probe blocks forever. The `setup --help` +# capability probes stay bare — help short-circuits before the probe. post_install_setup() { local binary_name="$1" local bin="$BIN_DIR/$binary_name" if binary_supports_setup_agents "$bin"; then - "$bin" setup agents || true + BASECAMP_NO_KEYRING=1 "$bin" setup agents || true return 0 fi @@ -514,9 +520,9 @@ post_install_setup() { # agent id as a stray positional arg and launches the INTERACTIVE wizard, # violating the non-interactive contract. Degrade to the shared skill. if binary_supports_setup_agent "$bin" "${BASECAMP_SETUP_AGENT}"; then - "$bin" setup "${BASECAMP_SETUP_AGENT}" || true + BASECAMP_NO_KEYRING=1 "$bin" setup "${BASECAMP_SETUP_AGENT}" || true else - "$bin" skill install || true + BASECAMP_NO_KEYRING=1 "$bin" skill install || true fi ;; all) @@ -525,16 +531,16 @@ post_install_setup() { local ran_agent=0 agent for agent in claude codex; do if binary_supports_setup_agent "$bin" "$agent"; then - "$bin" setup "$agent" || true + BASECAMP_NO_KEYRING=1 "$bin" setup "$agent" || true ran_agent=1 fi done - [[ "$ran_agent" -eq 1 ]] || "$bin" skill install || true + [[ "$ran_agent" -eq 1 ]] || BASECAMP_NO_KEYRING=1 "$bin" skill install || true ;; *) # Intent-neutral on old binaries: install the shared skill, never pick an # agent. The user connects one via the printed "Next steps". - "$bin" skill install || true + BASECAMP_NO_KEYRING=1 "$bin" skill install || true ;; esac }