diff --git a/.claude/commands/c1i.md b/.claude/commands/c1i.md index 5c7125e..6ad6d83 100644 --- a/.claude/commands/c1i.md +++ b/.claude/commands/c1i.md @@ -67,10 +67,10 @@ falls through to whatever the config file names. (never headers or bodies). `--debug` and `--max-retries` take effect only on the paths built on the shared -transport (REST, `mcp gateway`, `auth login`, `upgrade`). They are inert on the -`docs` subcommands that fetch — `docs search`, `docs page`, `docs openapi`, -`docs endpoints`, `docs endpoint` — which issue their own HTTP, so no trace -there does not mean no request was sent. +transport (REST, `mcp gateway`, `auth login`, `upgrade`, `docs openapi`, +`docs endpoints`, `docs endpoint`). They are inert on `docs search` and +`docs page`, which issue their own HTTP, so no trace there does not mean no +request was sent. ## Errors & exit codes diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d56edf..20b4082 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,61 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Upgrading from 0.9.1 + +With no cached spec, a failed `docs openapi`, `docs endpoints` or +`docs endpoint` now exits 4, 5, 6 or 8 instead of 1, and a 429, a transient +5xx or a network error is retried up to 4 times by default, so an unreachable +host takes several seconds to fail; `--max-retries 0` restores a single +attempt. When the config directory resolves to a relative path (a relative +`HOME`, `%AppData%` or `XDG_CONFIG_HOME`), every command that needs stored +credentials fails instead of using the working directory. With a relative +`HOME`, `~/.c1i.yaml` is ignored. + +### Changed + +- `golang.org/x/term` moves to 0.47.0 and `golang.org/x/sys` to 0.49.0. + +### Fixed + +- **`--debug` and `--max-retries` did nothing on `docs openapi`, + `docs endpoints` and `docs endpoint`.** The spec fetch now uses the shared + transport, so it traces, refuses a redirect to a different path or host, and + treats a response over 32 MiB as a failed fetch. With no cache to fall back + on, it retries a 429, a transient 5xx or a network error up to 4 times by + default, and a failed fetch exits 4 for a 404, 5 for a 429, 6 for a 5xx or a + redirect loop, and 8 otherwise, instead of 1. With a cache, it makes one + attempt unless you set `--max-retries` or `C1I_MAX_RETRIES`. + +- **An unreadable OpenAPI cache under a day old failed the `docs` OpenAPI + commands** instead of refetching, for example when the cache path was a + directory. They now fetch the spec. + +- **The token-file sweep could delete a token another c1i process had just + written**, when that process renamed it over an expired file the sweep had + read. The sweep now removes a file only if it is still the one it read. + +- **A relative `HOME`, `%AppData%` or `XDG_CONFIG_HOME` put credentials and + tokens under the working directory.** When the config directory resolves to + a relative path, the credential file fallback now fails with an error naming + it (a browser `auth login` then deletes the credential it just created), and + the token file cache is skipped. + +- **`upgrade` no longer runs `go env`.** It ran the go command to find + `GOBIN` and `GOPATH`, which made Go write telemetry counters under your + config dir. It now reads the environment and Go's env file itself. It reads + that file even when Go isn't installed, so a `GOBIN` set there marks a + binary in that dir as a `go install`. + +- **With a relative `HOME`, c1i read and wrote `.c1i.yaml` in the working + directory**, so a `.c1i.yaml` planted in a repository could point it, and + any `C1I_CLIENT_ID`/`C1I_CLIENT_SECRET` credentials, at another host. It + now neither reads nor writes the config file then; `auth login` warns that + the URL wasn't saved. + +- The README's uninstall steps now say where each install method puts the + binary, and that the Linux config directory follows `XDG_CONFIG_HOME`. + ## [0.9.1] - 2026-10-09 ### Security diff --git a/CLAUDE.md b/CLAUDE.md index 9d865ab..66c0faf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -113,8 +113,7 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. `--error-format` / `C1I_ERROR_FORMAT` (`text`|`json`), `--dry-run` / `C1I_DRY_RUN` (preview a mutating request without sending it), `--debug` / `C1I_DEBUG` (trace HTTP requests to stderr). `--debug` and `--max-retries` - are inert on the fetching `docs` subcommands (`docs search`, `docs page`, - `docs openapi`, `docs endpoints`, `docs endpoint`) — see README for behavior. + are inert on `docs search` and `docs page` — see README for behavior. ### Patterns to follow when adding/changing commands @@ -305,15 +304,14 @@ a package, verify each of these against the new code: retry count the flag can't change. Every caller passes them by hand — `cmd/client.go:15-16` (the single viper read every REST command inherits via `newClient`), `internal/client` from its own config, and `cmd/mcp_gateway.go`, - `cmd/auth_login.go`, `cmd/auth_token.go`, `cmd/upgrade.go` from viper. The - deliberate exception is `auth login`'s device-flow polling leg, where - `PollForToken` forces `WithMaxRetries(0)`: the RFC 8628 poll interval is that - call's retry strategy. The accidental one is the fetching `docs` subcommands — - `docs search`, `docs page` (`cmd/docs_search.go`) and `docs openapi`, - `docs endpoints`, `docs endpoint` (`cmd/docs_openapi.go`) — which call - `http.DefaultClient.Do` directly: both flags are inert at those three call + `cmd/auth_login.go`, `cmd/auth_token.go`, `cmd/upgrade.go`, + `cmd/docs_openapi.go` from viper. The deliberate exception is `auth login`'s + device-flow polling leg, where `PollForToken` forces `WithMaxRetries(0)`: the + RFC 8628 poll interval is that call's retry strategy. The accidental one is + `docs search` and `docs page` (`cmd/docs_search.go`), which call + `http.DefaultClient.Do` directly: both flags are inert at those two call sites, no path or redirect guard applies, and they return bare `fmt.Errorf` - rather than a classifiable error. Don't add a fourth. + rather than a classifiable error. Don't add a third. When implementing a wire protocol or stream parser (JSON-RPC, SSE, MCP, …), code and test against the **full input space the spec permits**, not just the shape a diff --git a/README.md b/README.md index 51e74b2..2fb8785 100644 --- a/README.md +++ b/README.md @@ -874,7 +874,7 @@ c1i docs guide c1i docs guide register-mcp-server ``` -`docs openapi`, `docs endpoints`, and `docs endpoint` use the current public API contract at `https://www.c1.ai/api/openapi.yaml`, including MCP server, tool, and toolset operations. The spec is cached for 24 hours at `~/.c1i/cache/api-openapi.yaml`; fetch failures fall back to that cache, however old, with a one-line warning on stderr naming its age. +`docs openapi`, `docs endpoints`, and `docs endpoint` use the current public API contract at `https://www.c1.ai/api/openapi.yaml`, including MCP server, tool, and toolset operations. The spec is cached for 24 hours at `~/.c1i/cache/api-openapi.yaml`; fetch failures fall back to that cache, however old, with a one-line warning on stderr naming its age. A response over 32 MiB counts as a failed fetch. With a cache to fall back on, a fetch makes one attempt unless you set `--max-retries` or `C1I_MAX_RETRIES`. With no cache, a failed fetch exits `4` for a `404`, `5` for a `429`, `6` for a `5xx` or a redirect loop, and `8` for anything else, such as an unreachable host or a refused redirect. `docs search` is a semantic search with no relevance threshold: every query returns up to 10 nearest matches, so even a nonsense query comes back with plausible-looking hits. A returned hit is not proof a concept exists, and an unexpected hit is not proof the thing you searched for is absent — read the snippet, or fetch the page with `docs page`, to judge. To check whether an API endpoint exists, use `docs endpoints --filter`, which has a real no-match. @@ -1050,13 +1050,13 @@ A chain of allowed redirects that doesn't settle within five hops fails as a remote error (exit `6`) rather than looping. This applies to every command built on the shared transport: the REST client, -the MCP gateway, the login handshake, and `upgrade`, so the path and redirect -guards, `--debug` tracing, and `--max-retries` cover all of them, not just REST -commands (`upgrade` maps a refused redirect to exit `8`; see Upgrading). -**None of those four** applies to the `docs` subcommands that fetch — -`docs search`, `docs page`, `docs openapi`, `docs endpoints`, `docs endpoint` — -which call Go's default HTTP client directly: no path or -redirect guard there, and `--debug` and `--max-retries` are both inert. +the MCP gateway, the login handshake, `upgrade`, and `docs openapi`, +`docs endpoints` and `docs endpoint`, so the path and redirect guards, `--debug` +tracing, and `--max-retries` cover all of them, not just REST commands +(`upgrade` and those `docs` commands map a refused redirect to exit `8`). +**None of those four** applies to `docs search` and `docs page`, which call +Go's default HTTP client directly: no path or redirect guard there, and +`--debug` and `--max-retries` are both inert. One narrower carve-out inside login: the device-flow token poll forces its own retry count to zero, because RFC 8628's polling interval already *is* that @@ -1118,7 +1118,8 @@ Set it via (in order of precedence): A terminal `c1i auth login` given the URL another way offers to save it there. If the file isn't a valid YAML mapping, login warns and leaves it - unchanged. + unchanged. With a relative `HOME`, c1i neither reads nor writes it, so a + `.c1i.yaml` in the working directory is never used. These are equivalent: - `--url https://mycompany.conductor.one` @@ -1307,7 +1308,10 @@ never written to disk — a new one is minted per invocation. `c1i auth login` writes to the OS keyring when it can and falls back to the file backend transparently. `c1i auth status` tells you which source served -the active credentials. +the active credentials. If the config directory resolves to a relative path, +from a relative `HOME`, `%AppData%` or `XDG_CONFIG_HOME`, the file fallback +fails with an error and the token cache below skips its file, rather than +writing under the working directory. ### Token cache @@ -1392,16 +1396,26 @@ one older than the one you run. ## Uninstalling +Remove the binary the way you installed it: `brew uninstall c1i` for +Homebrew; for `go install`, delete `c1i` from `GOBIN` (set in the environment +or with `go env -w`), or from `$GOPATH/bin` (`~/go/bin` by default) when +`GOBIN` is unset; a downloaded binary is wherever you put it; and +`docker rmi public.ecr.aws/conductorone/c1i:` for the container image. + Removing the binary leaves these behind: - `~/.c1i/`, the OpenAPI spec cache. - `~/.c1i.yaml`, the config file. - `/c1i/`, holding the file-fallback credentials (`credentials/`) and the access-token file cache (`tokens/`). See - [Credential sources](#credential-sources) for each OS's config directory. + [Credential sources](#credential-sources) for each OS's config directory; + on Linux it is `$XDG_CONFIG_HOME` when that is set. - OS keyring entries: credentials under `c1i/`, and cached access tokens - under `com.conductorone.c1i.tokens`. Older releases stored a + (including tokens for `C1I_CLIENT_ID`/`C1I_CLIENT_SECRET` credentials) under + `com.conductorone.c1i.tokens`. Older releases stored a `.conductor.one` tenant's credentials under `c1i/`. +- Any [shell completion](#shell-completion) script you installed, and the + `c1i completion powershell` line if you added it to your PowerShell profile. c1i can't list keyring entries or keep track of the tenants you've logged in to. To remove its keyring credentials, run `auth logout` once per tenant before you diff --git a/cmd/agents.md b/cmd/agents.md index 014d4a1..96f88d7 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -19,8 +19,9 @@ Get this right first. A wrong tenant returns plausible-looking data with exit 0. `c1i` resolves the tenant in this order: `--url`, `C1I_URL`, then `url:` in -`~/.c1i.yaml`. It must be a full host — `mycompany.conductor.one` or -`mycompany.c1eu.ai` (EU) — and `https` is required: +`~/.c1i.yaml` (never read with a relative `HOME`). It must be a full host — +`mycompany.conductor.one` or `mycompany.c1eu.ai` (EU) — and `https` is +required: a bare `mycompany`, a non-https scheme, and a malformed host (an embedded space or control character, or a stray scheme like `://host`) are all usage errors (exit `2`) before any request is sent. @@ -42,6 +43,10 @@ subcommands that also take an external server's address — that one is Credentials resolve in this order: `C1I_CLIENT_ID` + `C1I_CLIENT_SECRET` env vars (read-only — c1i never writes them), the OS keyring, then a `0600` file used automatically where no keyring exists (headless Linux, CI, containers). +With a relative `HOME` and no keyring, that file fallback errors rather than +write under the working directory, so every command that needs stored +credentials fails: `auth login`, `auth logout`, and any authenticated command +run without the env vars. Only the bearer c1i attaches automatically for REST commands is cached and reused across invocations until it nears expiry, so a run of one-shot commands does not write a `client_credentials` audit event each time. It uses the OS @@ -99,29 +104,33 @@ when the failure came from the API. Still branch on the exit code — the JSON i for the detail, not the classification. `--debug` and `--max-retries` cover `mcp gateway`, `auth login` (except its -device-code polling, which never retries) and `upgrade` as well as the REST -commands: the gateway client threads both into its bearer mint and its JSON-RPC -calls. On a `mcp gateway call` that hangs or fails oddly, `--debug` is the -fastest way to see which request stopped. - -They do **not** reach the `docs` subcommands that fetch — `docs search`, -`docs page`, `docs openapi`, `docs endpoints`, `docs endpoint`. Those bypass -the shared transport for Go's default HTTP client, so `--debug` prints nothing -and `--max-retries` is ignored. Silent `--debug` output there means the flag +device-code polling, which never retries), `upgrade`, `docs openapi`, +`docs endpoints` and `docs endpoint` as well as the REST commands: the gateway +client threads both into its bearer mint and its JSON-RPC calls. On a +`mcp gateway call` that hangs or fails oddly, `--debug` is the fastest way to +see which request stopped. + +They do **not** reach `docs search` or `docs page`. Those bypass the shared +transport for Go's default HTTP client, so `--debug` prints nothing and +`--max-retries` is ignored. Silent `--debug` output there means the flag never reached that path, NOT that no request was sent — don't read it as evidence either way when a `docs` command comes back empty. -Nor do those five call the same place, which matters for egress rules and for -why one can fail while another works: `docs openapi`, `docs endpoints` and -`docs endpoint` fetch `https://www.c1.ai/api/openapi.yaml` (cached 24h at +Nor do the fetching `docs` commands call the same place, which matters for +egress rules and for why one can fail while another works: `docs openapi`, +`docs endpoints` and `docs endpoint` fetch +`https://www.c1.ai/api/openapi.yaml` (cached 24h at `~/.c1i/cache/api-openapi.yaml`, so a run can return rows without sending a request), while `docs search` and `docs page` call a third party — `api.mintlify.com` — with a public client-side key. `docs search` is semantic with no relevance threshold, so it always returns up to 10 plausible hits even for a nonsense query: never read a result as proof a concept exists, or an unexpected result as proof it is absent. `docs endpoints --filter` DOES have a -true no-match. If a fetch fails, an expired cache is served with a one-line -`Warning:` on stderr naming its age. +true no-match. If a spec fetch fails, an expired cache is served with a +one-line `Warning:` on stderr naming its age; with no cache, it exits 4 for a +404, 5 for a 429, 6 for a 5xx or a redirect loop, and 8 otherwise. With a cache +to fall back on, the fetch makes one attempt unless you set `--max-retries` or +`C1I_MAX_RETRIES`. Don't store files in `~/.c1i/cache/`: a spec refresh deletes any file there that the running version doesn't use. diff --git a/cmd/cache_test.go b/cmd/cache_test.go index 8da1245..14e6109 100644 --- a/cmd/cache_test.go +++ b/cmd/cache_test.go @@ -12,19 +12,43 @@ import ( "strings" "testing" "time" + + "github.com/ConductorOne/c1i/internal/client" ) type stubRoundTripper func(*http.Request) (*http.Response, error) func (f stubRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } +// stubOpenAPIBase makes rt serve every spec fetch. +func stubOpenAPIBase(t *testing.T, rt http.RoundTripper) { + t.Helper() + orig := openAPIBase + openAPIBase = rt + t.Cleanup(func() { openAPIBase = orig }) +} + +// setRootFlag sets one of rootCmd's persistent flags for the test. +func setRootFlag(t *testing.T, name, value string) { + t.Helper() + f := rootCmd.PersistentFlags().Lookup(name) + orig, origChanged := f.Value.String(), f.Changed + if err := f.Value.Set(value); err != nil { + t.Fatal(err) + } + f.Changed = true + t.Cleanup(func() { + _ = f.Value.Set(orig) + f.Changed = origChanged + }) +} + // runDocsOpenapi runs `docs openapi` with rt serving every request and returns -// its stdout and stderr. +// its stdout and stderr. Retries are off, so one stub response is final. func runDocsOpenapi(t *testing.T, rt stubRoundTripper) (string, string, error) { t.Helper() - orig := http.DefaultClient.Transport - http.DefaultClient.Transport = rt - t.Cleanup(func() { http.DefaultClient.Transport = orig }) + stubOpenAPIBase(t, rt) + setRootFlag(t, "max-retries", "0") out, errOut := &bytes.Buffer{}, &bytes.Buffer{} prevCtx := docsOpenapiCmd.Context() @@ -338,3 +362,191 @@ func TestFormatAgeClampsFutureMtime(t *testing.T) { } } } + +// runDocsOpenapiRoot runs `c1i docs openapi args...` through rootCmd, so the +// global flags are parsed as a user passes them. +func runDocsOpenapiRoot(t *testing.T, rt http.RoundTripper, args ...string) (string, error) { + t.Helper() + stubOpenAPIBase(t, rt) + for _, name := range []string{"debug", "max-retries"} { + f := rootCmd.PersistentFlags().Lookup(name) + t.Cleanup(func() { + _ = f.Value.Set(f.DefValue) + f.Changed = false + }) + } + var out bytes.Buffer + rootCmd.SetOut(&out) + rootCmd.SetErr(&out) + prevCtx := docsOpenapiCmd.Context() + docsOpenapiCmd.SetContext(t.Context()) // cobra keeps a subcommand's first context + t.Cleanup(func() { + rootCmd.SetOut(nil) + rootCmd.SetErr(nil) + docsOpenapiCmd.SetContext(prevCtx) + }) + rootCmd.SetArgs(append([]string{"docs", "openapi"}, args...)) + err := rootCmd.ExecuteContext(t.Context()) + return out.String(), err +} + +func TestOpenAPIMaxRetriesFlag(t *testing.T) { + for flag, want := range map[string]int{"0": 1, "1": 2} { + t.Run("--max-retries="+flag, func(t *testing.T) { + useTempHome(t) + attempts := 0 + _, err := runDocsOpenapiRoot(t, stubRoundTripper(func(r *http.Request) (*http.Response, error) { + attempts++ + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody, Request: r}, nil + }), "--max-retries", flag) + if err == nil || exitCode(err) != exitServer { + t.Errorf("err = %v (exit %d), want exit %d", err, exitCode(err), exitServer) + } + if attempts != want { + t.Errorf("sent %d requests, want %d", attempts, want) + } + }) + } +} + +func TestOpenAPIDebugTracesFetch(t *testing.T) { + useTempHome(t) + var err error + trace := captureStderr(t, func() { + _, err = runDocsOpenapiRoot(t, serveSpec(stubOpenAPISpec), "--debug") + }) + if err != nil { + t.Fatalf("docs openapi --debug: %v", err) + } + if want := "> GET " + openapiURL; !strings.Contains(trace, want) { + t.Errorf("stderr = %q, want a trace line %q", trace, want) + } +} + +func TestOpenAPIRefusesCrossHostRedirect(t *testing.T) { + useTempHome(t) + _, err := runDocsOpenapiRoot(t, stubRoundTripper(func(r *http.Request) (*http.Response, error) { + if r.URL.String() != openapiURL { + t.Errorf("followed a redirect to %s", r.URL) + return serveSpec(stubOpenAPISpec)(r) + } + h := http.Header{"Location": {"https://elsewhere.example/api/openapi.yaml"}} + return &http.Response{StatusCode: http.StatusFound, Header: h, Body: http.NoBody, Request: r}, nil + })) + if err == nil || !strings.Contains(err.Error(), "refusing to follow redirect") || exitCode(err) != exitUpstream { + t.Errorf("err = %v (exit %d), want a refused redirect, exit %d", err, exitCode(err), exitUpstream) + } +} + +// A body over the cap is a failed fetch, even one that would parse as a spec. +func TestOpenAPIOverCapBody(t *testing.T) { + filler := "# filler\n" + body := "paths: {}\n" + strings.Repeat(filler, maxOpenAPISpecBytes/len(filler)+1) + t.Run("no cache", func(t *testing.T) { + useTempHome(t) + _, _, err := runDocsOpenapi(t, serveSpec(body)) + if err == nil || exitCode(err) != exitUpstream { + t.Errorf("err = %v (exit %d), want exit %d", err, exitCode(err), exitUpstream) + } + }) + t.Run("expired cache", func(t *testing.T) { + writeExpiredCache(t, useTempHome(t), stubOpenAPISpec) + got, stderr, err := runDocsOpenapi(t, serveSpec(body)) + if err != nil { + t.Fatalf("docs openapi: %v", err) + } + if got != stubOpenAPISpec || !strings.Contains(stderr, "Warning: using cached OpenAPI spec") { + t.Errorf("stdout = %.40q, stderr = %q; want the cached spec and a warning", got, stderr) + } + }) +} + +// A fresh cache that can't be read is skipped, not returned as an error. +func TestOpenAPIUnreadableFreshCacheFetches(t *testing.T) { + if err := os.MkdirAll(useTempHome(t), 0o700); err != nil { + t.Fatal(err) + } + got, _, err := runDocsOpenapi(t, serveSpec(stubOpenAPISpec)) + if err != nil { + t.Fatalf("docs openapi: %v", err) + } + if got != stubOpenAPISpec { + t.Errorf("printed %q, want the fetched spec", got) + } +} + +// unsetRetriesEnv clears C1I_MAX_RETRIES for the test. +func unsetRetriesEnv(t *testing.T) { + t.Helper() + t.Setenv("C1I_MAX_RETRIES", "") + _ = os.Unsetenv("C1I_MAX_RETRIES") +} + +func serve503(attempts *int) stubRoundTripper { + return func(r *http.Request) (*http.Response, error) { + *attempts++ + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody, Request: r}, nil + } +} + +// With a cache to fall back on, retries only delay the answer. +func TestOpenAPIStaleCacheMakesOneAttempt(t *testing.T) { + unsetRetriesEnv(t) + writeExpiredCache(t, useTempHome(t), stubOpenAPISpec) + attempts := 0 + out, err := runDocsOpenapiRoot(t, serve503(&attempts)) + if err != nil { + t.Fatalf("docs openapi: %v", err) + } + if attempts != 1 || !strings.Contains(out, "Warning: using cached OpenAPI spec") { + t.Errorf("sent %d requests, output %.200q; want 1 and the stale-cache warning", attempts, out) + } +} + +func TestOpenAPIExplicitRetriesApplyWithCache(t *testing.T) { + unsetRetriesEnv(t) + writeExpiredCache(t, useTempHome(t), stubOpenAPISpec) + attempts := 0 + if _, err := runDocsOpenapiRoot(t, serve503(&attempts), "--max-retries", "2"); err != nil { + t.Fatalf("docs openapi: %v", err) + } + if attempts != 3 { + t.Errorf("sent %d requests, want 3", attempts) + } +} + +func TestOpenAPIRetries(t *testing.T) { + unsetRetriesEnv(t) + cachePath := useTempHome(t) + if got := openAPIRetries(cachePath); got != client.DefaultMaxRetries { + t.Errorf("no cache: %d retries, want the default %d", got, client.DefaultMaxRetries) + } + writeExpiredCache(t, cachePath, stubOpenAPISpec) + if got := openAPIRetries(cachePath); got != 0 { + t.Errorf("cache: %d retries, want 0", got) + } + t.Setenv("C1I_MAX_RETRIES", "2") + if got := openAPIRetries(cachePath); got != 2 { + t.Errorf("cache and C1I_MAX_RETRIES=2: %d retries, want 2", got) + } +} + +// Without a cache, every failed fetch exits 8 unless its status has its own code. +func TestOpenAPIFailureExitCodesWithoutCache(t *testing.T) { + for name, c := range map[string]struct { + rt stubRoundTripper + want int + }{ + "CDN 403": {func(r *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusForbidden, Body: http.NoBody, Request: r}, nil + }, exitUpstream}, + "not a spec": {serveSpec(notASpec[0]), exitUpstream}, + } { + t.Run(name, func(t *testing.T) { + useTempHome(t) + if _, _, err := runDocsOpenapi(t, c.rt); exitCode(err) != c.want { + t.Errorf("err = %v (exit %d), want exit %d", err, exitCode(err), c.want) + } + }) + } +} diff --git a/cmd/docs_openapi.go b/cmd/docs_openapi.go index 5e1378a..6390164 100644 --- a/cmd/docs_openapi.go +++ b/cmd/docs_openapi.go @@ -3,7 +3,6 @@ package cmd import ( "encoding/json" "fmt" - "io" "net/http" "os" "path/filepath" @@ -11,7 +10,10 @@ import ( "strings" "time" + "github.com/ConductorOne/c1i/internal/client" + "github.com/ConductorOne/c1i/internal/transport" "github.com/spf13/cobra" + "github.com/spf13/viper" "go.yaml.in/yaml/v3" ) @@ -20,8 +22,14 @@ const ( cacheMaxAge = 24 * time.Hour cacheDirName = ".c1i" cacheFileName = "api-openapi.yaml" + + // maxOpenAPISpecBytes is several times the ~4 MB spec. + maxOpenAPISpecBytes = 32 << 20 ) +// openAPIBase is the spec fetch's innermost transport; tests stub it. +var openAPIBase http.RoundTripper + var docsOpenapiCmd = &cobra.Command{ Use: "openapi", Short: "Dump the raw C1 OpenAPI spec (no auth required)", @@ -171,7 +179,10 @@ func fetchOpenAPISpec(cmd *cobra.Command) ([]byte, error) { if cachePath != "" { if info, err := os.Stat(cachePath); err == nil && time.Since(info.ModTime()) < cacheMaxAge { - return os.ReadFile(cachePath) // #nosec G304 -- cachePath is a fixed internal path (openAPICachePath), not caller input + // An unreadable cache falls through to a fetch. + if data, err := os.ReadFile(cachePath); err == nil { // #nosec G304 -- cachePath is a fixed internal path (openAPICachePath), not caller input + return data, nil + } } } @@ -180,26 +191,26 @@ func fetchOpenAPISpec(cmd *cobra.Command) ([]byte, error) { return nil, err } - resp, err := http.DefaultClient.Do(req) + hc := transport.New(openAPIBase, + transport.WithMaxRetries(openAPIRetries(cachePath)), + transport.WithDebug(viper.GetBool("debug")), + transport.WithMaxResponseBytes(maxOpenAPISpecBytes), + ) + resp, err := hc.Do(req) if err != nil { - return staleOpenAPISpec(cmd, cachePath, fmt.Errorf("fetching OpenAPI spec: %w", err)) + return staleOpenAPISpec(cmd, cachePath, distError(err, "fetching OpenAPI spec")) } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - return staleOpenAPISpec(cmd, cachePath, fmt.Errorf("fetching OpenAPI spec: HTTP %d", resp.StatusCode)) - } - - data, err := io.ReadAll(resp.Body) - if err != nil { - return staleOpenAPISpec(cmd, cachePath, fmt.Errorf("reading OpenAPI spec: %w", err)) + apiErr := &client.APIError{Method: req.Method, Path: req.URL.Path, StatusCode: resp.StatusCode} + return staleOpenAPISpec(cmd, cachePath, distError(apiErr, "fetching OpenAPI spec")) } + data := resp.Body // A captive portal answers 200 with HTML, and caching that would serve it // until cacheMaxAge. var doc map[string]any err = yaml.Unmarshal(data, &doc) if _, ok := doc["paths"]; err != nil || !ok { - return staleOpenAPISpec(cmd, cachePath, fmt.Errorf("fetching OpenAPI spec: %s did not return an OpenAPI document", openapiURL)) + return staleOpenAPISpec(cmd, cachePath, distError(fmt.Errorf("%s did not return an OpenAPI document", openapiURL), "fetching OpenAPI spec")) } if cachePath != "" { @@ -208,6 +219,17 @@ func fetchOpenAPISpec(cmd *cobra.Command) ([]byte, error) { return data, nil } +// openAPIRetries is the configured retry count, except that a fetch with a +// cached spec to fall back on makes one attempt unless the user set +// --max-retries or C1I_MAX_RETRIES: retries would only delay the fallback. +func openAPIRetries(cachePath string) int { + if info, err := os.Stat(cachePath); err == nil && info.Mode().IsRegular() && + os.Getenv("C1I_MAX_RETRIES") == "" && !rootCmd.PersistentFlags().Changed("max-retries") { + return 0 + } + return viper.GetInt("max_retries") +} + // staleOpenAPISpec returns the cached spec, however old, or err if there is // none. It warns on stderr, leaving stdout clean for pipes. func staleOpenAPISpec(cmd *cobra.Command, cachePath string, err error) ([]byte, error) { diff --git a/cmd/flag_scope_docs_test.go b/cmd/flag_scope_docs_test.go index d65621f..4c811b2 100644 --- a/cmd/flag_scope_docs_test.go +++ b/cmd/flag_scope_docs_test.go @@ -14,7 +14,7 @@ import ( // Flag SCOPE coverage: --debug/--max-retries do not reach every command. // -// The fetching `docs` subcommands send HTTP without internal/transport, so both +// `docs search` and `docs page` send HTTP without internal/transport, so both // flags are inert there and no path/redirect guard applies. That single fact // reached six documents, stated as an unqualified universal in four of them, // before anyone checked it against the code — and the cost is a reader who @@ -31,11 +31,7 @@ import ( var scopedFlags = []string{"--debug", "--max-retries"} // fetchingDocsSubcommands are the `docs` leaves that issue their own HTTP. -// "docs endpoints" and "docs endpoint" are distinct commands, matched with a -// trailing boundary so the longer name can't satisfy a mention of the shorter. -var fetchingDocsSubcommands = []string{ - "docs search", "docs page", "docs openapi", "docs endpoints", "docs endpoint", -} +var fetchingDocsSubcommands = []string{"docs search", "docs page"} // flagScopeDocs are the documents that describe what --debug and // --max-retries do. agentsTemplate is the embedded copy `c1i docs agents` @@ -50,8 +46,8 @@ func flagScopeDocs(t *testing.T) []struct{ name, body string } { } } -// namesFetchingDocsCommands counts how many of the five fetching subcommands a -// block of text names. +// namesFetchingDocsCommands counts how many of the fetching subcommands a block +// of text names. func namesFetchingDocsCommands(block string) int { n := 0 for _, c := range fetchingDocsSubcommands { @@ -68,14 +64,14 @@ func namesFetchingDocsCommands(block string) int { var exceptionMarkerRe = regexp.MustCompile(`(?i)\b(not|never|inert|ignored|bypass|exception|only|don't|doesn't)\b`) // hasFlagScopeCarveOut reports whether doc contains a blank-line-separated -// block naming both scoped flags and at least two fetching docs subcommands, -// and saying they do not apply. +// block naming both scoped flags and both fetching docs subcommands, and saying +// they do not apply. func hasFlagScopeCarveOut(doc string) bool { for _, block := range strings.Split(doc, "\n\n") { if !strings.Contains(block, scopedFlags[0]) || !strings.Contains(block, scopedFlags[1]) { continue } - if namesFetchingDocsCommands(block) >= 2 && exceptionMarkerRe.MatchString(block) { + if namesFetchingDocsCommands(block) == len(fetchingDocsSubcommands) && exceptionMarkerRe.MatchString(block) { return true } } @@ -99,7 +95,7 @@ func TestFlagScopeExceptionDocumented(t *testing.T) { stated++ if !hasFlagScopeCarveOut(d.body) { t.Errorf("%s documents %v but never carves out the fetching docs subcommands; "+ - "add a paragraph naming both flags, at least two of %v, and saying they do not apply there", + "add a paragraph naming both flags and each of %v, and saying they do not apply there", d.name, scopedFlags, fetchingDocsSubcommands) } } @@ -227,15 +223,14 @@ func transportFreeSenders(path string) ([]string, error) { // from this set in either direction. // // Trap for the next reader: entries mean two different things and the loops -// below treat them identically. The two cmd/ files ARE bypasses, documented as -// such in the four docs. internal/transport is the opposite — it is the shared +// below treat them identically. The cmd/ file IS a bypass, documented as such +// in the four docs. internal/transport is the opposite — it is the shared // transport, so its http.Client is the one every other package is supposed to // inherit; it is listed only to keep it from reporting itself. A new sender // appended to transport.go would therefore pass silently, which is accepted: // that file IS the transport, and changing it is not the drift this guards. var httpBypassFiles = map[string]string{ "cmd/docs_search.go": "bypass: docs search / docs page -> api.mintlify.com", - "cmd/docs_openapi.go": "bypass: docs openapi / endpoints / endpoint -> www.c1.ai", "internal/transport/transport.go": "NOT a bypass: the shared transport itself", } diff --git a/cmd/geturl_test.go b/cmd/geturl_test.go index 53c65a4..25b61c8 100644 --- a/cmd/geturl_test.go +++ b/cmd/geturl_test.go @@ -3,6 +3,7 @@ package cmd import ( "bytes" "context" + "os" "strings" "testing" @@ -268,3 +269,19 @@ func TestGetBaseURLFullHostsUnaffected(t *testing.T) { // -HOME'd read within the same process unreliable). Not attempted here for // that reason; TestGetBaseURLBareTokenFromConfigNamesConfigFile covers the // same GetBaseURLWithSource code path via viper.Set instead. + +// A relative HOME must not read a .c1i.yaml planted in the working directory, +// which could point the CLI, and any env-var credentials, at another host. +func TestRelativeHomeSkipsConfigFile(t *testing.T) { + t.Setenv("HOME", ".") + t.Setenv("USERPROFILE", ".") + t.Chdir(t.TempDir()) + if err := os.WriteFile(".c1i.yaml", []byte("url: https://attacker.example\n"), 0o600); err != nil { + t.Fatal(err) + } + v := viper.New() + loadConfigFile(v) + if got := v.GetString("url"); got != "" { + t.Errorf("read url %q from the working directory's .c1i.yaml", got) + } +} diff --git a/cmd/root.go b/cmd/root.go index 8f4228b..054ac43 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -148,14 +148,18 @@ func init() { _ = viper.BindEnv("dry_run", "C1I_DRY_RUN") } -func initConfig() { - home, err := os.UserHomeDir() - if err == nil { - viper.AddConfigPath(home) +func initConfig() { loadConfigFile(viper.GetViper()) } + +// loadConfigFile reads ~/.c1i.yaml into v, or nothing without a usable home. +func loadConfigFile(v *viper.Viper) { + home, err := config.HomeDir() + if err != nil { + return } - viper.SetConfigName(".c1i") - viper.SetConfigType("yaml") - _ = viper.ReadInConfig() + v.AddConfigPath(home) + v.SetConfigName(".c1i") + v.SetConfigType("yaml") + _ = v.ReadInConfig() } // GetBaseURL returns the configured base URL or exits with an error. Embedded diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 00c360b..e922f65 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -231,10 +231,11 @@ func writeUpgradeReport(cmd *cobra.Command, current, target, channel string, ava return writeObject(cmd, data) } -// distError classifies a failure fetching or verifying a release as upstream -// (8), except that a dist 404, 429 or 5xx keeps its own exit code. dist needs -// no auth and takes no input from the caller, so for a refused redirect, a bad -// path, or any other 4xx the status is dropped and the exit code is 8. +// distError classifies a failure fetching or verifying a release, or fetching +// the OpenAPI spec, as upstream (8), except that a 404, 429 or 5xx keeps its +// own exit code. Neither host needs auth or takes input from the caller, so for +// a refused redirect, a bad path, or any other 4xx the status is dropped and +// the exit code is 8. func distError(err error, format string, args ...any) error { msg := fmt.Sprintf(format, args...) var apiErr *client.APIError diff --git a/go.mod b/go.mod index ba563df..c2bcd70 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( go.yaml.in/yaml/v3 v3.0.5 golang.org/x/mod v0.41.0 golang.org/x/oauth2 v0.37.0 - golang.org/x/term v0.46.0 + golang.org/x/term v0.47.0 ) require ( @@ -81,7 +81,7 @@ require ( golang.org/x/crypto v0.57.0 // indirect golang.org/x/net v0.60.0 // indirect golang.org/x/sync v0.23.0 // indirect - golang.org/x/sys v0.48.0 // indirect + golang.org/x/sys v0.49.0 // indirect golang.org/x/text v0.42.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20261005182115-fad411399dd8 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20261005182115-fad411399dd8 // indirect diff --git a/go.sum b/go.sum index e3b4d6c..598adc4 100644 --- a/go.sum +++ b/go.sum @@ -343,10 +343,10 @@ golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= golang.org/x/sync v0.23.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= -golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= -golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= -golang.org/x/term v0.46.0 h1:3+OXuTbaKDgwk8jTi3aSLHRlmWqHEUDUtxnbFigO4YE= -golang.org/x/term v0.46.0/go.mod h1:+K02xbkittuwc0Am4abfA3Fc+XRGXkvBXNO88NCXPoc= +golang.org/x/sys v0.49.0 h1:XbzkgYJHdqh/8m2Uu0W/dQv8nktxx4BFHp1M0gROTXA= +golang.org/x/sys v0.49.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= +golang.org/x/term v0.47.0 h1:cpyOwQkr2Nzxe2p6KnrTG7Ei2wtzjsuSjhpXXepZ/BM= +golang.org/x/term v0.47.0/go.mod h1:efAfAuG6RKABvg1ArGebRkvX6LXul14SM67o2e4mUCY= golang.org/x/text v0.42.0 h1:JbOZXgfeCPU9gacVtYliJqOhD+zhrEqK4LfdpmlUZqI= golang.org/x/text v0.42.0/go.mod h1:ojzP1Z+2QtioaF8DTtO8K5q7JWVVYwZKenzujK0Zd0E= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= diff --git a/internal/config/save.go b/internal/config/save.go index b3c8471..fc4480f 100644 --- a/internal/config/save.go +++ b/internal/config/save.go @@ -10,11 +10,24 @@ import ( "go.yaml.in/yaml/v3" ) +// HomeDir is os.UserHomeDir, refusing a relative result: with HOME="." the +// config file would be read from, and written to, the working directory. +func HomeDir() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + if !filepath.IsAbs(home) { + return "", fmt.Errorf("home dir %q is not an absolute path", home) + } + return home, nil +} + // SaveToConfigFile reads ~/.c1i.yaml, sets the given key to value, and writes it back. // The file is created with mode 0600 if it does not exist. A file it can't read // or parse is left untouched and reported, so the user's settings aren't lost. func SaveToConfigFile(key, value string) error { - home, err := os.UserHomeDir() + home, err := HomeDir() if err != nil { return err } diff --git a/internal/config/save_test.go b/internal/config/save_test.go index 354c198..a95d2e8 100644 --- a/internal/config/save_test.go +++ b/internal/config/save_test.go @@ -141,3 +141,16 @@ func TestSaveToConfigFileUnreadableFileLeftAlone(t *testing.T) { t.Errorf("file = %q, want it unchanged", b) } } + +func TestSaveToConfigFileRefusesRelativeHome(t *testing.T) { + t.Setenv("HOME", ".") + t.Setenv("USERPROFILE", ".") + wd := t.TempDir() + t.Chdir(wd) + if err := SaveToConfigFile("url", "https://example.conductor.one"); err == nil { + t.Error("SaveToConfigFile = nil, want an error for a relative home dir") + } + if entries, _ := os.ReadDir(wd); len(entries) != 0 { + t.Errorf("wrote %v under the working directory", entries) + } +} diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index 6653858..de5e7b5 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -217,11 +217,24 @@ type fileCreds struct { ClientSecret string `json:"client_secret"` } -func filePath(service string) (string, error) { +// ConfigDir is os.UserConfigDir, refusing a relative result: a relative HOME +// or %AppData% would put credentials and tokens under the working directory. +func ConfigDir() (string, error) { dir, err := os.UserConfigDir() if err != nil { return "", fmt.Errorf("locating config dir: %w", err) } + if !filepath.IsAbs(dir) { + return "", fmt.Errorf("locating config dir: %q is not an absolute path", dir) + } + return dir, nil +} + +func filePath(service string) (string, error) { + dir, err := ConfigDir() + if err != nil { + return "", err + } return filepath.Join(dir, "c1i", "credentials", sanitizeService(service)+".json"), nil } diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 0c2634e..b3027a0 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -428,3 +428,32 @@ func TestStoredClientIDIgnoresEnv(t *testing.T) { t.Errorf("StoredClientID = %q, want the stored %q despite env credentials", id, testID) } } + +// useRelativeConfigDir makes os.UserConfigDir relative and returns the working +// directory it would resolve under. +func useRelativeConfigDir(t *testing.T) string { + t.Helper() + t.Setenv("HOME", ".") + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("AppData", ".") + wd := t.TempDir() + t.Chdir(wd) + return wd +} + +// A relative config dir must not put credentials under the working directory. +func TestFileFallbackRefusesRelativeConfigDir(t *testing.T) { + keyring.MockInitWithError(keyring.ErrUnsupportedPlatform) + clearEnv(t) + wd := useRelativeConfigDir(t) + + if _, err := Store(testService, testID, testSecret); err == nil || !strings.Contains(err.Error(), "config dir") { + t.Errorf("Store err = %v, want a config dir error", err) + } + if _, _, _, err := Load(testService); err == nil || !strings.Contains(err.Error(), "config dir") { + t.Errorf("Load err = %v, want a config dir error", err) + } + if entries, _ := os.ReadDir(wd); len(entries) != 0 { + t.Errorf("wrote %v under the working directory", entries) + } +} diff --git a/internal/selfupdate/install.go b/internal/selfupdate/install.go index 1a8ec9e..15dda34 100644 --- a/internal/selfupdate/install.go +++ b/internal/selfupdate/install.go @@ -1,9 +1,8 @@ package selfupdate import ( - "encoding/json" + "cmp" "os" - "os/exec" "path/filepath" "strings" ) @@ -127,21 +126,42 @@ type goEnv struct { GOPATH string } -// readGoEnv runs from a neutral directory with GOTOOLCHAIN=local, so a go.mod -// or go.work in the caller's cwd can't trigger a toolchain download. -var readGoEnv = func() (goEnv, error) { - cmd := exec.Command("go", "env", "-json", "GOBIN", "GOPATH") - cmd.Dir = os.TempDir() - cmd.Env = append(os.Environ(), "GOTOOLCHAIN=local") - output, err := cmd.Output() - if err != nil { - return goEnv{}, err +// readGoEnv resolves GOBIN and GOPATH as the go command does: the environment, +// then the go env file, then GOPATH's default. It doesn't run go, because any +// go command writes telemetry counters under the user's config dir. +var readGoEnv = func() goEnv { + var file goEnv + if path := goEnvFile(); path != "" { + data, _ := os.ReadFile(path) // #nosec G304 -- the go command's own env file + for _, line := range strings.Split(string(data), "\n") { + switch key, val, _ := strings.Cut(line, "="); key { + case "GOBIN": + file.GOBIN = val + case "GOPATH": + file.GOPATH = val + } + } } - var env goEnv - if err := json.Unmarshal(output, &env); err != nil { - return goEnv{}, err + env := goEnv{GOBIN: cmp.Or(os.Getenv("GOBIN"), file.GOBIN), GOPATH: cmp.Or(os.Getenv("GOPATH"), file.GOPATH)} + if home, err := os.UserHomeDir(); err == nil && env.GOPATH == "" { + env.GOPATH = filepath.Join(home, "go") } - return env, nil + return env +} + +// goEnvFile is where `go env -w` writes, or "" with GOENV=off. +func goEnvFile() string { + if file := os.Getenv("GOENV"); file != "" { + if file == "off" { + return "" + } + return file + } + dir, err := os.UserConfigDir() + if err != nil { + return "" + } + return filepath.Join(dir, "go", "env") } // isGoInstall reports whether execPath is under any effective Go install target. @@ -168,12 +188,11 @@ func goInstallTargets() []string { targets = append(targets, gobin) } addGoPaths(os.Getenv("GOPATH")) - if persisted, err := readGoEnv(); err == nil { - if persisted.GOBIN != "" { - targets = append(targets, persisted.GOBIN) - } - addGoPaths(persisted.GOPATH) + persisted := readGoEnv() + if persisted.GOBIN != "" { + targets = append(targets, persisted.GOBIN) } + addGoPaths(persisted.GOPATH) if home, err := os.UserHomeDir(); err == nil { targets = append(targets, filepath.Join(home, "go", "bin")) } diff --git a/internal/selfupdate/selfupdate_test.go b/internal/selfupdate/selfupdate_test.go index 59b8a0c..96bcc06 100644 --- a/internal/selfupdate/selfupdate_test.go +++ b/internal/selfupdate/selfupdate_test.go @@ -126,12 +126,90 @@ func TestReadGoEnvIgnoresCallerModule(t *testing.T) { t.Chdir(dir) t.Setenv("GOTOOLCHAIN", "auto") t.Setenv("GOPROXY", "off") - env, err := readGoEnv() + if env := readGoEnv(); env.GOPATH == "" { + t.Error("readGoEnv returned an empty GOPATH") + } +} + +// isolateGoEnv points everything readGoEnv consults at an empty temp home and +// returns that home and the config dir it implies. +func isolateGoEnv(t *testing.T) (home, configDir string) { + t.Helper() + home = t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, "config")) + t.Setenv("AppData", filepath.Join(home, "config")) + for _, k := range []string{"GOBIN", "GOPATH", "GOENV"} { + t.Setenv(k, "") + } + configDir, err := os.UserConfigDir() if err != nil { - t.Fatalf("readGoEnv: %v", err) + t.Fatal(err) } - if env.GOPATH == "" { - t.Error("readGoEnv returned an empty GOPATH") + return home, configDir +} + +func TestDetectGoInstallSources(t *testing.T) { + writeEnvFile := func(t *testing.T, path, gobin string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + // The format `go env -w GOBIN=...` writes. + if err := os.WriteFile(path, []byte("GOFLAGS=-mod=mod\nGOBIN="+gobin+"\n"), 0o600); err != nil { + t.Fatal(err) + } + } + for name, c := range map[string]struct { + setup func(t *testing.T, home, configDir string) string // returns the binary's path + want Method + }{ + "GOBIN env var": {func(t *testing.T, home, _ string) string { + t.Setenv("GOBIN", filepath.Join(home, "gobin")) + return filepath.Join(home, "gobin", "c1i") + }, GoInstall}, + "GOPATH list": {func(t *testing.T, home, _ string) string { + t.Setenv("GOPATH", filepath.Join(home, "a")+string(filepath.ListSeparator)+filepath.Join(home, "b")) + return filepath.Join(home, "b", "bin", "c1i") + }, GoInstall}, + "go env file": {func(t *testing.T, home, configDir string) string { + writeEnvFile(t, filepath.Join(configDir, "go", "env"), filepath.Join(home, "persisted")) + return filepath.Join(home, "persisted", "c1i") + }, GoInstall}, + "GOENV file": {func(t *testing.T, home, _ string) string { + writeEnvFile(t, filepath.Join(home, "custom-env"), filepath.Join(home, "persisted")) + t.Setenv("GOENV", filepath.Join(home, "custom-env")) + return filepath.Join(home, "persisted", "c1i") + }, GoInstall}, + "GOENV=off": {func(t *testing.T, home, configDir string) string { + writeEnvFile(t, filepath.Join(configDir, "go", "env"), filepath.Join(home, "persisted")) + t.Setenv("GOENV", "off") + return filepath.Join(home, "persisted", "c1i") + }, Standalone}, + "default GOPATH": {func(t *testing.T, home, _ string) string { + return filepath.Join(home, "go", "bin", "c1i") + }, GoInstall}, + "elsewhere": {func(t *testing.T, home, _ string) string { + return filepath.Join(home, "downloads", "c1i") + }, Standalone}, + } { + t.Run(name, func(t *testing.T) { + home, configDir := isolateGoEnv(t) + path := c.setup(t, home, configDir) + if m, _ := Detect(path, "darwin"); m != c.want { + t.Errorf("%s -> %v, want %v", path, m, c.want) + } + }) + } +} + +// Running the go command writes telemetry counters under the config dir. +func TestReadGoEnvWritesNoTelemetry(t *testing.T) { + _, configDir := isolateGoEnv(t) + readGoEnv() + if _, err := os.Stat(filepath.Join(configDir, "go", "telemetry")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("readGoEnv left go/telemetry under the config dir: %v", err) } } @@ -141,8 +219,8 @@ func TestDetectSystemAndPersistedGoInstall(t *testing.T) { } original := readGoEnv - readGoEnv = func() (goEnv, error) { - return goEnv{GOBIN: "/opt/custom-go/bin", GOPATH: "/work/a:/work/b"}, nil + readGoEnv = func() goEnv { + return goEnv{GOBIN: "/opt/custom-go/bin", GOPATH: "/work/a:/work/b"} } t.Cleanup(func() { readGoEnv = original }) t.Setenv("GOBIN", "") diff --git a/internal/tokensource/cache.go b/internal/tokensource/cache.go index eae785a..2e7f1d1 100644 --- a/internal/tokensource/cache.go +++ b/internal/tokensource/cache.go @@ -4,7 +4,6 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" - "fmt" "io" "os" "path/filepath" @@ -47,6 +46,10 @@ var ( tokenKeyringGet = keychain.GetSecret tokenKeyringSet = keychain.SetSecret tokenKeyringDelete = keychain.DeleteSecret + + // sweepBeforeRemove runs between the sweep's check of a file and its + // remove; tests use it to race a store. + sweepBeforeRemove = func(string) {} ) type cachedToken struct { @@ -64,9 +67,9 @@ func cacheKey(tokenHost, clientID, clientSecret string) string { } func cachePath(key string) (string, error) { - dir, err := os.UserConfigDir() + dir, err := keychain.ConfigDir() if err != nil { - return "", fmt.Errorf("locating config dir: %w", err) + return "", err } return filepath.Join(dir, "c1i", "tokens", key+".json"), nil } @@ -220,6 +223,7 @@ func pruneTokenDir(dir, keep string) { if err != nil || !info.Mode().IsRegular() || name == keep { continue } + _ = os.SameFile(info, info) // Windows reads the file id lazily; pin it now isToken, isTemp := tokenFileName(name) switch { case isToken: @@ -234,7 +238,12 @@ func pruneTokenDir(dir, keep string) { default: continue } - _ = os.Remove(p) + sweepBeforeRemove(p) + // A concurrent store may have renamed a fresh token over p since the + // first Lstat. This narrows that window; it can't close it. + if now, err := os.Lstat(p); err == nil && os.SameFile(info, now) { + _ = os.Remove(p) + } } } diff --git a/internal/tokensource/cache_test.go b/internal/tokensource/cache_test.go index 689d78c..bfc53f3 100644 --- a/internal/tokensource/cache_test.go +++ b/internal/tokensource/cache_test.go @@ -628,3 +628,55 @@ func TestTokenSweepSkipsSymlinkedDir(t *testing.T) { }) } } + +// A store that renames a fresh token over an expired one after the sweep read +// it must not lose the fresh token. +func TestStoreSweepSparesTokenReplacedMidSweep(t *testing.T) { + useTempConfig(t) + keep, _ := cachePath(testCacheKey("host", "client")) + dir := filepath.Dir(keep) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + raced := filepath.Join(dir, cacheKey("h", "c", "raced-secret")+".json") + writeTokenFile(t, raced, time.Now().Add(-time.Hour)) + orig := sweepBeforeRemove + t.Cleanup(func() { sweepBeforeRemove = orig }) + sweepBeforeRemove = func(p string) { + if p != raced { + return + } + tmp := filepath.Join(dir, ".raced.tmp") + writeTokenFile(t, tmp, time.Now().Add(time.Hour)) + if err := os.Rename(tmp, raced); err != nil { + t.Fatal(err) + } + } + + storeCachedToken(testCacheKey("host", "client"), freshToken(30*time.Minute)) + + if readCachedTokenFile(raced) == nil { + t.Error("the sweep deleted a fresh token renamed over the expired one it read") + } +} + +// A relative config dir skips the file cache instead of writing under the +// working directory. +func TestRelativeConfigDirSkipsFileCache(t *testing.T) { + useTempConfig(t) + t.Setenv("HOME", ".") + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("AppData", ".") + wd := t.TempDir() + t.Chdir(wd) + key := testCacheKey("host", "client") + + storeCachedToken(key, freshToken(time.Hour)) + + if loadCachedToken(key) != nil { + t.Error("loaded a token from a relative config dir") + } + if entries, _ := os.ReadDir(wd); len(entries) != 0 { + t.Errorf("wrote %v under the working directory", entries) + } +} diff --git a/internal/transport/errors.go b/internal/transport/errors.go index 2fb411c..c40482f 100644 --- a/internal/transport/errors.go +++ b/internal/transport/errors.go @@ -22,6 +22,9 @@ type APIError struct { } func (e *APIError) Error() string { + if e.Body == "" { + return fmt.Sprintf("API %s %s returned %d", e.Method, e.Path, e.StatusCode) + } return fmt.Sprintf("API %s %s returned %d: %s", e.Method, e.Path, e.StatusCode, e.Body) } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index f7342b1..e28aadf 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -511,3 +511,14 @@ func TestNew_WiresTimeout(t *testing.T) { t.Errorf("httpClient.Timeout = %v, want the WithTimeout override (%v)", c.httpClient.Timeout, want) } } + +func TestAPIErrorWithoutBody(t *testing.T) { + for body, want := range map[string]string{ + "": "API GET /x returned 403", + "nope": "API GET /x returned 403: nope", + } { + if got := (&APIError{Method: "GET", Path: "/x", StatusCode: 403, Body: body}).Error(); got != want { + t.Errorf("body %q: %q, want %q", body, got, want) + } + } +}