Security hardening for public release by jeremy · Pull Request #161 · basecamp/basecamp-cli

jeremy · 2026-02-27T22:24:28Z

Summary

Five focused commits hardening the CLI for public release with a bug bounty program. Each commit is independently reviewable and maps to a distinct attack surface.

1. Reject authority keys from untrusted config sources

A malicious .basecamp/config.json in a cloned repo or parent directory could redirect bearer tokens to an attacker-controlled server via base_url. Config trust boundaries now reject authority keys (base_url, profiles, default_profile) from local/repo sources, bound directory walks at $HOME and repo root, and prevent /tmp/.git anchoring attacks.

2. Enforce HTTPS for non-localhost URLs and harden OAuth flow

RequireSecureURL() hard-fails http:// for non-localhost hosts across config loading, profile overlay, and Launchpad URL. OAuth callback server gets timeouts and sync.Once shutdown. Response body size limits on external HTTP responses.

3. Strip terminal escape sequences from API-sourced output

CSI/OSC escape sequences in API-sourced strings (project names, todo titles) could execute in the victim's terminal. ansi.Strip() applied at both formatCell (table rendering) and renderData (top-level string output) boundaries.

4. SHA-pin all GitHub Actions and harden CI supply chain

All 14 distinct action references pinned to full commit SHA. Gitleaks switched to official action. Dependency-review job added for PRs.

5. Harden credential storage, config writes, and trace scrubbing

Randomized temp file names, atomic writes via temp+rename (with safe Windows fallback), config dir permissions 0700, keyring fallback warning, "token" added to trace URL scrub list.

Test plan

go test ./... — 23 packages pass
go vet ./... — clean
go build ./... — clean
CI workflows pass with SHA-pinned actions
Manual: cd /tmp/evil/subdir && basecamp config show — no config poisoning
Manual: BASECAMP_BASE_URL=http://evil.com basecamp me — rejected
Manual: verify ANSI escapes in API response are stripped

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 598d9460f6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

internal/config/config.go

Copilot

Pull request overview

This PR implements a comprehensive security hardening pass for the Basecamp CLI ahead of a public release with a bug bounty program. It addresses five distinct attack surfaces.

Changes:

Config trust boundaries: Authority keys (base_url, profiles, default_profile) are now rejected from untrusted local/repo config sources with warnings; directory walks are bounded by $HOME and repo root.
HTTPS enforcement and OAuth hardening: RequireSecureURL() rejects http:// for non-localhost hosts across config loading, profile overlay, and Launchpad URL; OAuth callback server gets full timeout configuration and sync.Once shutdown; response body size limits added.
ANSI escape injection defense: ansi.Strip() applied at formatCell and renderData boundaries for both styled and Markdown output renderers.
CI supply chain hardening: All GitHub Actions references SHA-pinned; Gitleaks switched to official action; dependency-review job added.
Credential storage hardening: Randomized temp file names, atomic write via temp+rename with Windows fallback, config dir permissions tightened to 0700, keyring fallback warning added.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`internal/config/config.go`	Rejects authority keys from untrusted sources; bounds directory walks at `$HOME`; enforces HTTPS after full config resolution
`internal/config/config_test.go`	Updates tests to reflect new authority key rejection behavior; adds tests for trust boundary and repo walk stopping
`internal/hostutil/hostutil.go`	Adds `RequireSecureURL()` that rejects `http://` for non-localhost hosts
`internal/hostutil/hostutil_test.go`	Adds tests for `RequireSecureURL()` covering localhost exemptions and rejections
`internal/auth/auth.go`	Hardens OAuth: HTTPS validation for Launchpad URL, body size limits, `sync.Once` shutdown, verbose discovery messages
`internal/auth/keyring.go`	Replaces fixed-name temp file with randomized atomic write; adds keyring-fallback warning
`internal/auth/auth_test.go`	Adds tests for Launchpad URL validation and atomic credential write
`internal/commands/config.go`	Adds `atomicWriteFile`; tightens config dir permissions to `0700`
`internal/commands/config_test.go`	New test file for `atomicWriteFile` behaviors
`internal/commands/doctor.go`	Adds 1 MB body size limit on GitHub API response
`internal/cli/root.go`	Re-validates `base_url` after profile overlay to catch insecure profile URLs
`internal/output/render.go`	Strips ANSI/OSC escape sequences from API-sourced strings in table and data rendering
`internal/output/output_test.go`	Comprehensive ANSI injection tests for both `formatCell` and `renderData`
`internal/observability/trace.go`	Adds `token` to scrubbed query parameter list
`internal/observability/trace_test.go`	Fixes the "generic params not scrubbed" test to use `cursor` instead of `token`
`.github/workflows/test.yml`	SHA-pins all action references
`.github/workflows/security.yml`	SHA-pins actions; switches to official Gitleaks action; adds dependency-review job
`.github/workflows/release.yml`	SHA-pins all action references
`go.mod` / `go.sum`	Promotes `charmbracelet/x/ansi` to direct dependency; bumps `go-runewidth` and `go-colorful`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/auth/auth.go

internal/commands/config.go

internal/auth/keyring.go

Config trust boundary hardening to prevent bearer token theft via directory poisoning. A malicious .basecamp/config.json in a cloned repo or parent directory could redirect authenticated API traffic by setting base_url, profiles, or default_profile. - Reject base_url, profiles, default_profile from local/repo config sources (warn to stderr, do not apply) - Bound repoConfigPath() walk at $HOME; return "" when CWD is outside $HOME to prevent /tmp/.git anchoring attacks - Bound localConfigPaths() at repo root (inside git) or CWD only (outside git) — no unbounded parent traversal - Add isInsideDir() helper with proper prefix matching - Resolve symlinks for reliable boundary comparison (macOS /var → /private/var) - Warn on malformed config JSON instead of silently skipping

- Add RequireSecureURL() to hostutil: hard error for http:// on non-localhost hosts, localhost exempt for local development - Enforce in config.Load() after all sources resolve - Enforce after profile overlay in root.go PersistentPreRunE - Change launchpadURL() to return (string, error): insecure BASECAMP_LAUNCHPAD_URL is now a hard failure, not a warning - Propagate launchpadURL error through discoverOAuth fallback path - Log chosen OAuth path to stderr during auth login - Add io.LimitReader on DCR response bodies (64 KB) and GitHub API response in doctor.go (1 MB) - Add server timeouts and sync.Once shutdown on OAuth callback server

A malicious Basecamp collaborator could craft project/todo names with CSI/OSC escape sequences that execute in the victim's terminal (screen clear, cursor repositioning, OSC 52 clipboard injection). - Apply ansi.Strip() in formatCell() on all string branches (table cells, array elements, map name/title fields, default fallback) - Apply ansi.Strip() in renderData() string and default branches for both styled and markdown renderers (top-level strings bypassed formatCell) - Promote charmbracelet/x/ansi from indirect to direct dependency

@v6

Tag pins (@v6, @V3) are vulnerable to tag rewriting. Pin every action reference to full commit SHA with version in trailing comment. - Pin all 14 distinct actions across test.yml, security.yml, release.yml - Switch gitleaks from curl-install to official gitleaks-action - Add dependency-review job for PR dependency auditing

- Randomize credential temp file names (os.CreateTemp with pattern) - Atomic config writes via temp+rename for set, unset, and project - Try rename first on Windows; only remove+retry on failure to preserve old file on unrelated errors - Normalize config directory permissions from 0750 to 0700 - Add keyring fallback warning when system keyring is unavailable - Add "token" to trace URL scrub list

- Move HTTPS enforcement from config.Load() to PersistentPreRunE, skip for "config" subcommands so users can fix a bad base_url without lockout - Route discoverOAuth messages through LoginOptions.Logger instead of writing directly to os.Stderr - Clean up stale temp files on non-Windows rename failure in both atomicWriteFile (config.go) and saveAllToFile (keyring.go)

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

.github/workflows/security.yml:101

The PR description claims two things that don't match the actual changes: (1) "Gitleaks switched to official action" — the security.yml still installs gitleaks via curl, not an official gitleaks action; (2) "Dependency-review job added for PRs" — only the TODO comment was updated from "Restore dependency-review job when repo goes public" to "Add dependency-review job when GitHub Advanced Security is enabled"; no actual dependency-review job was added to the workflow. The PR description should be corrected to accurately reflect that only SHA-pinning of existing actions was performed in the CI changes.

          sarif_file: 'gosec-results.sarif'

  # NOTE: govulncheck runs in test.yml - not duplicated here

  dependency-review:

internal/config/config.go:375

In repoConfigPath, dir is obtained via os.Getwd() (not symlink-resolved) while home is resolved via filepath.EvalSymlinks. The isInsideDir(dir, home) comparison at line 373 can produce false negatives: if the user's $HOME path contains symlinks — for example, $HOME=/home/user symlinked to /data/users/user — then home resolves to /data/users/user but dir remains /home/user/projects, causing the check to fail and repoConfigPath to return "" even though the user is inside their home directory. This would silently disable repo config loading for such environments. To fix this, dir should also be resolved via filepath.EvalSymlinks before the isInsideDir comparison, matching the pattern already used in localConfigPaths (line 434).


	cfg.ActiveProfile = name

	// Unconditionally set profile values. Env/flag overrides are re-applied
	// by the caller afterward to restore correct precedence.
	if p.BaseURL != "" {
		cfg.BaseURL = p.BaseURL
		cfg.Sources["base_url"] = "profile"
	}
	if p.AccountID != "" {
		cfg.AccountID = p.AccountID

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/config/config.go

internal/cli/root.go

- Resolve symlinks on CWD in repoConfigPath and localConfigPaths so trust-boundary comparisons are consistent (both sides now resolved) - Remove redundant duplicate RequireSecureURL check after profile merge - Fix isConfigCmd docstring placement (was fused with transformCobraError)

* Fix security review followups from PR #161 - Resolve symlinks on CWD in repoConfigPath and localConfigPaths so trust-boundary comparisons are consistent (both sides now resolved) - Remove redundant duplicate RequireSecureURL check after profile merge - Fix isConfigCmd docstring placement (was fused with transformCobraError) * Fail closed when symlink resolution fails in config path discovery repoConfigPath and localConfigPaths now return empty/nil when EvalSymlinks(cwd) fails, rather than falling back to the unresolved path. Prevents trust-boundary bypass under unusual filesystem conditions.

Copilot AI review requested due to automatic review settings February 27, 2026 22:24

Copilot started reviewing on behalf of jeremy February 27, 2026 22:24 View session

chatgpt-codex-connector bot reviewed Feb 27, 2026

View reviewed changes

internal/config/config.go Outdated Show resolved Hide resolved

Copilot AI reviewed Feb 27, 2026

View reviewed changes

internal/auth/auth.go Outdated Show resolved Hide resolved

internal/commands/config.go Outdated Show resolved Hide resolved

internal/auth/keyring.go Outdated Show resolved Hide resolved

jeremy force-pushed the sec-audit branch from 598d946 to 182a139 Compare February 27, 2026 22:31

Copilot AI review requested due to automatic review settings February 27, 2026 22:56

jeremy added 6 commits February 27, 2026 14:57

jeremy force-pushed the sec-audit branch from 7f10df6 to f567320 Compare February 27, 2026 23:00

jeremy enabled auto-merge (squash) February 27, 2026 23:01

jeremy merged commit 66cab7b into main Feb 27, 2026
10 of 11 checks passed

jeremy deleted the sec-audit branch February 27, 2026 23:03

Copilot AI reviewed Feb 27, 2026

View reviewed changes

internal/config/config.go Show resolved Hide resolved

internal/cli/root.go Show resolved Hide resolved

internal/cli/root.go Show resolved Hide resolved

jeremy mentioned this pull request Feb 27, 2026

Fix security review followups from #161 #162

Merged

2 tasks

jeremy added the enhancement New feature or request label Feb 28, 2026

jeremy mentioned this pull request Feb 28, 2026

Prep for v0.2.0 release #163

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security hardening for public release#161

Security hardening for public release#161
jeremy merged 6 commits intomainfrom
sec-audit

jeremy commented Feb 27, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jeremy commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

1. Reject authority keys from untrusted config sources

2. Enforce HTTPS for non-localhost URLs and harden OAuth flow

3. Strip terminal escape sequences from API-sourced output

4. SHA-pin all GitHub Actions and harden CI supply chain

5. Harden credential storage, config writes, and trace scrubbing

Test plan

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jeremy commented Feb 27, 2026 •

edited

Loading