Skip to content

Commit 1480ab4

Browse files
8bitAlexclaudeMr. MeeseeksCopilot
authored
fix: three critical issues blocking v1.0 (C1 telemetry persistence + C2 credential leak + C3 context race) (#95)
* fix(telemetry): split skip tiers so --json doesn't kill consent state (C1) The first-run consent prompt previously persisted `decided=off` on any skip signal — including transient ones like `--json`. A user running `raid context --json | jq` once was silently opted out of telemetry for life, never seeing the prompt in any future interactive run. Split MaybePromptForConsent into two skip tiers: - Persistent (--yes / --headless / RAID_HEADLESS / non-TTY / DO_NOT_TRACK): long-term "this host is non-interactive" signals that legitimately should persist decided=off so we don't keep re-prompting. - Transient (--json): per-invocation "this one command wants machine-readable output" that must NOT poison the consent state. Precedence: persistent wins when both signals are set (`--yes --json` together → persist). Reordered the body to check persistent first; a regression test pins this contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(security): scrub repo URL credentials + 0600 vars file (C2) Two related changes that close a credential-leak path: 1. setRepoVars now scrubs userinfo from each repo's URL before writing it to raidVars. A profile YAML using the common `https://user:token@github.com/...` clone-URL pattern no longer persists the credential to ~/.raid/vars, no longer surfaces it through the MCP `raid://workspace/vars` resource, and no longer lands verbatim in $RAID_REPO_<NAME>_URL for subprocesses to see. SSH-style `git@host:repo.git` URLs and unparseable inputs pass through unchanged. Re-exported as `raid.ScrubURL` and applied in the MCP `raid_list_repos` handler too. 2. execSetVar's atomic write now chmods the tempfile to 0600 before the rename so the vars file lands at the tight mode regardless of godotenv's 0644 default. loadRaidVars best-effort tightens existing 0644 files on the next load for migration. The vars file holds scrubbed-but-still-private RAID_REPO_*_URL entries and arbitrary user-defined Set values (which project authors may treat as secret-ish). 0644 + a shared dev machine is a real leak path; 0600 is the right baseline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(concurrency): atomic.Pointer for global workspace context (C3) The package-global `context *Context` in src/internal/lib was read concurrently by MCP read handlers (raid_list_repos, raid_describe_repo, raid_list_profiles, ...) while mutating handlers (raid_install, raid_env_switch) called ForceLoad which rewrote the pointer. mcp-go dispatches tool calls from a 5-worker pool, so the reader/writer interleave was a real undefined-behavior race — `go test -race` would have flagged it against any representative MCP scenario. Replace with atomic.Pointer[Context]. Reads go through loadContext(); writes through storeContext(). The value is always wholly replaced (never partially mutated) so the atomic pointer is the right primitive: zero contention on the read hot path, well-defined swap semantics for the rare write path. Mechanical sweep: - 15 production read sites: bind the result of loadContext() to a local before nil-checking + field access, so a swap landing between the nil-check and the read can't confuse subsequent reads from the same logical operation. - 89 test sites: same treatment, with the bare `context = &Context{}` test pattern replaced by storeContext(&Context{}). New TestContextRaceForceLoadVsReads stresses the interleave with 8 concurrent readers + 1 writer; the race detector enforces the contract. Any future contributor reintroducing a non-atomic write to the context pointer trips this test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address Copilot review — scrub semantics, vars chmod guard, test portability - ScrubURL: only strip userinfo from http/https schemes; preserve SSH usernames so `ssh://git@host/...` stays a valid clone URL after scrubbing. - loadRaidVars: Lstat + IsRegular guard before chmod so a misconfigured path pointing at a directory or symlink isn't tightened/followed. - handleListRepos: add credential-bearing-URL regression test at the MCP handler boundary so a future refactor can't bypass raid.ScrubURL. - prompt.go: realign the comment with the no-API-key short-circuit — dev builds don't persist decided=off, they just skip without recording state. - task_runner_extra_test: skip 0600 perm-bit assertions on Windows where os.Chmod only toggles the read-only attribute. - lib_test: add a start barrier to the C3 race regression so reads and writes are guaranteed to overlap; document the CI gap that the race detector isn't on by default in the project workflow. Co-Authored-By: Copilot <copilot@github.com> * chore: bump version 0.17.0 → 0.17.1 + what's-new for hotfix Three bug fixes per the versioning rule warrant a patch bump. Adds a 0.17.1 what's-new section covering all three: the --json consent bug, the credential-leak via vars file (with the explicit "rotate your token" warning for users on prior versions), the 0600 file permission tighten, and the MCP context race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Mr. Meeseeks <meeseeks@alexsalerno.dev> Co-authored-by: Copilot <copilot@github.com>
1 parent b70e93c commit 1480ab4

23 files changed

Lines changed: 705 additions & 262 deletions

site/docs/whats-new.mdx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,18 @@ description: Feature-by-feature release notes for Raid.
99

1010
User-visible changes per release, latest first. For full commit history see the [GitHub releases page](https://github.com/8bitalex/raid/releases).
1111

12+
## 0.17.1 — upcoming
13+
14+
Three pre-v1.0 hardening fixes from the release-readiness review. No behavior change for happy paths; all three are correctness/security bugs with regression tests now in place.
15+
16+
**Telemetry consent prompt no longer gets killed by `--json`.** The first-run prompt previously persisted `decided=off` on any skip signal — including transient ones like `--json`. A user who piped one command through `raid context --json | jq` was silently opted out of telemetry for life. The consent flow now splits skip signals into two tiers: persistent (`--yes` / `--headless` / `RAID_HEADLESS` / non-TTY / `DO_NOT_TRACK`) which still persist `decided=off`, and transient (`--json`) which skip the prompt without writing state. The next interactive run without the transient signal still gets prompted.
17+
18+
**Repo URL credentials no longer leak to `~/.raid/vars` or MCP.** A profile YAML using `https://user:token@github.com/...` as a clone URL no longer persists the credential to disk, no longer surfaces it through the MCP `raid://workspace/vars` resource, and no longer lands verbatim in `$RAID_REPO_<NAME>_URL` for subprocesses. SSH-style `git@host:repo.git` URLs round-trip unchanged. **If you've previously run raid against a profile with embedded clone credentials, rotate the affected token** — this fix prevents new leaks but doesn't scrub historical content from your existing vars file. Re-exported as `raid.ScrubURL`.
19+
20+
**`~/.raid/vars` now written with mode `0600` instead of `0644`.** The vars file persists raid-managed variables (scrubbed repo URLs, `Set`-task values, anything project authors treat as secret-ish) and shouldn't be world-readable. New writes land at `0600` atomically; existing 0644 files are best-effort tightened on the next load.
21+
22+
**MCP read/write race on the workspace context.** mcp-go dispatches tool calls from a 5-worker pool, so MCP read handlers (`raid_list_repos`, `raid_describe_repo`, …) could race with mutating handlers' `ForceLoad` rewrites of the package-global context. Replaced with `atomic.Pointer[Context]`; reads are now well-defined under concurrent writes. Race-detector test pins the contract.
23+
1224
## 0.17.0 — upcoming
1325

1426
**Optional opt-out telemetry follow-up.** When a user declines the first-run telemetry prompt, raid now asks one follow-up question: may we send a single anonymous event recording your denial? Default is no (capital N) just like the main prompt. If accepted, raid fires exactly one `raid_telemetry_opt_out` event (with `reason: "prompt-declined"`) under explicit per-event consent and then leaves telemetry permanently off — that's the whole transaction. If declined or skipped, raid touches zero endpoints for the remainder of the install. The follow-up runs *only* on the explicit-decline path: non-TTY, headless, `DO_NOT_TRACK`, already-decided, and `raid telemetry ...` invocations skip both prompts. Closes the opt-out-rate measurement gap that's otherwise impossible to fill from website analytics alone.

src/cmd/context/serve.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -520,7 +520,10 @@ func handleListRepos(_ stdctx.Context, req mcp.CallToolRequest) (*mcp.CallToolRe
520520

521521
urls := make(map[string]string, len(active.Repositories))
522522
for _, r := range active.Repositories {
523-
urls[r.Name] = r.URL
523+
// Scrub embedded userinfo so an HTTPS clone URL with a token
524+
// (`https://user:token@host/...`) doesn't leak through the
525+
// MCP tool result. See raid.ScrubURL for the contract.
526+
urls[r.Name] = raid.ScrubURL(r.URL)
524527
}
525528

526529
live := rctx.Get().Workspace.Repos

src/cmd/context/serve_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,44 @@ func TestHandleListRepos_returnsConfiguredReposWithURL(t *testing.T) {
563563
}
564564
}
565565

566+
// TestHandleListRepos_scrubsCredentialBearingURL pins bug C2 at the
567+
// MCP handler boundary: a clone URL with embedded userinfo
568+
// (`https://user:token@host/...`) must come back through
569+
// raid_list_repos with the credentials stripped. The unit test on
570+
// ScrubURL alone isn't enough — a future refactor that bypasses the
571+
// helper here would still pass that test but reintroduce the leak.
572+
func TestHandleListRepos_scrubsCredentialBearingURL(t *testing.T) {
573+
body := `name: test-fixture
574+
repositories:
575+
- name: demo-repo
576+
path: ` + "/tmp/raid-cmd-context-test-demo-repo-DOES-NOT-EXIST" + `
577+
url: https://alice:s3cret@github.com/org/demo.git
578+
`
579+
loadTestProfile(t, body)
580+
581+
res, err := handleListRepos(stdctx.Background(), mcp.CallToolRequest{})
582+
if err != nil {
583+
t.Fatalf("handleListRepos: %v", err)
584+
}
585+
if res.IsError {
586+
t.Fatalf("expected ok, got error: %s", toolResultText(res))
587+
}
588+
raw := toolResultText(res)
589+
if strings.Contains(raw, "alice") || strings.Contains(raw, "s3cret") {
590+
t.Fatalf("MCP response leaked credentials: %s", raw)
591+
}
592+
var entries []listReposEntry
593+
if err := json.Unmarshal([]byte(raw), &entries); err != nil {
594+
t.Fatalf("body not JSON: %v", err)
595+
}
596+
if len(entries) != 1 {
597+
t.Fatalf("expected 1 repo, got %d: %+v", len(entries), entries)
598+
}
599+
if entries[0].URL != "https://github.com/org/demo.git" {
600+
t.Errorf("repo URL = %q, want scrubbed scheme+host+path", entries[0].URL)
601+
}
602+
}
603+
566604
func TestHandleDescribeRepo_lookupByName(t *testing.T) {
567605
loadTestProfile(t, minimalTestProfileBody)
568606

src/cmd/raid.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,15 +169,20 @@ func executeRoot(args []string) int {
169169

170170
// First-run consent prompt for telemetry. Runs only for non-info,
171171
// non-telemetry-subcommand invocations to avoid prompting on
172-
// `raid --help`, `raid telemetry on`, and similar. The prompt
173-
// itself no-ops when stdin isn't a TTY, when --yes/--headless is
174-
// set, when --json is set, or when RAID_HEADLESS=1 in the env
175-
// (CI / agent-host opt-in path), so it's safe in non-interactive
176-
// contexts. See telemetry.MaybePromptForConsent for the full skip
177-
// matrix.
172+
// `raid --help`, `raid telemetry on`, and similar.
173+
//
174+
// Skip signals split into two tiers. Persistent — `--yes` /
175+
// `--headless`, `RAID_HEADLESS=1`, non-TTY stdin, DO_NOT_TRACK —
176+
// all reflect "this host is non-interactive long-term" and cause
177+
// MaybePromptForConsent to persist decided=off so we don't keep
178+
// re-prompting. Transient — `--json` — is just "this one command
179+
// wants machine-readable output" and must NOT poison the consent
180+
// state. The split was added after a real bug where one
181+
// `raid context --json | jq` silently opted the user out for life.
178182
if !info && !isTelemetrySubcommand(args) {
179-
skip := headlessFromArgs(args) || jsonModeFromArgs(args) || lib.IsHeadless()
180-
switch telemetry.MaybePromptForConsent(skip) {
183+
skipPersistent := headlessFromArgs(args) || lib.IsHeadless()
184+
skipTransient := jsonModeFromArgs(args)
185+
switch telemetry.MaybePromptForConsent(skipPersistent, skipTransient) {
181186
case telemetry.PromptAccepted:
182187
// User opted in via the first-run prompt — fire the
183188
// adoption event so `raid telemetry on` and the prompt

src/internal/lib/command.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,20 @@ func (c Command) IsZero() bool {
6767

6868
// GetCommands returns all commands available in the active profile.
6969
func GetCommands() []Command {
70-
if context == nil {
70+
ctx := loadContext()
71+
if ctx == nil {
7172
return nil
7273
}
73-
return context.Profile.Commands
74+
return ctx.Profile.Commands
7475
}
7576

7677
// GetRepos returns the repositories in the active profile.
7778
func GetRepos() []Repo {
78-
if context == nil {
79+
ctx := loadContext()
80+
if ctx == nil {
7981
return nil
8082
}
81-
return context.Profile.Repositories
83+
return ctx.Profile.Repositories
8284
}
8385

8486
// ExecuteCommand runs the tasks for the named command, applying any output configuration.

0 commit comments

Comments
 (0)