From 64f5dfb8831d0af555739211dbd47956fb7af886 Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Tue, 28 Jul 2026 11:57:57 +1000 Subject: [PATCH 1/2] fix: KEEP-1049 validate credentials against an endpoint that requires auth Doctor and API-key validation both probed endpoints that answer 200 to anonymous callers, so neither could fail. /api/auth/get-session returns a null session rather than 401, and /api/workflows resolves auth with required:false and returns an empty list. Doctor reported every caller as authenticated - including with no credential stored - and kh auth status accepted any string beginning with kh_. Both now resolve their endpoint from one shared constant pointing at /api/projects, which authenticates via resolveOrganizationId and answers 401 when nothing authenticates. A test pins the constant against the known anonymous-tolerant endpoints, since a test at either call site would just follow the constant wherever it points - which is how this went unnoticed. Doctor also never sent a credential at all: doGet applied Cloudflare Access headers but no Authorization. That is why wallet and spend-cap reported 'requires authentication' even for a valid key. It now attaches the resolved token, so those checks report real state. This repairs the login skip merged in #73. It decides whether a stored credential is still good via FetchTokenInfo, so a revoked key counted as valid and left the user needing --force to recover. Verified live: valid key passes, bogus key fails both checks, no credential fails, and login re-runs the device flow on a revoked key without --force. --- cmd/doctor/doctor.go | 15 ++++- cmd/doctor/doctor_auth_test.go | 90 ++++++++++++++++++++++++++ internal/auth/token.go | 10 ++- internal/auth/token_test.go | 76 ++++++++++++++++++++++ internal/http/credential_probe.go | 20 ++++++ internal/http/credential_probe_test.go | 42 ++++++++++++ internal/http/version.go | 4 +- internal/http/version_remedy_test.go | 60 +++++++++++++++++ 8 files changed, 314 insertions(+), 3 deletions(-) create mode 100644 cmd/doctor/doctor_auth_test.go create mode 100644 internal/auth/token_test.go create mode 100644 internal/http/credential_probe.go create mode 100644 internal/http/credential_probe_test.go create mode 100644 internal/http/version_remedy_test.go diff --git a/cmd/doctor/doctor.go b/cmd/doctor/doctor.go index b3fdfa8..be6f2a0 100644 --- a/cmd/doctor/doctor.go +++ b/cmd/doctor/doctor.go @@ -11,6 +11,7 @@ import ( "sync" "time" + internalauth "github.com/keeperhub/cli/internal/auth" khhttp "github.com/keeperhub/cli/internal/http" "github.com/keeperhub/cli/pkg/cmdutil" "github.com/keeperhub/cli/pkg/iostreams" @@ -148,6 +149,14 @@ func doGet(ctx context.Context, client *http.Client, host, url string) (*http.Re return nil, err } khhttp.ApplyHostHeaders(req, host) + // Every doctor check runs through here, and none of them used to send a + // credential. The auth check only appeared to pass because it probed an + // endpoint that answers 200 to anonymous callers; the wallet and spend-cap + // checks reported "requires authentication" even for a valid key, because + // they genuinely were anonymous. + if resolved, resolveErr := internalauth.ResolveToken(host); resolveErr == nil && resolved.Token != "" { + req.Header.Set("Authorization", "Bearer "+resolved.Token) + } return client.Do(req) } @@ -171,7 +180,11 @@ func checkAuth(ctx context.Context, f *cmdutil.Factory) CheckResult { return CheckResult{Status: "warn", Message: "could not create HTTP client"} } - url := khhttp.BuildBaseURL(host) + "/api/auth/get-session" + // This previously probed /api/auth/get-session, which answers 200 with a + // null session for unauthenticated callers instead of 401. Switching on the + // status code therefore reported every caller as authenticated, including + // callers with no credential stored at all. + url := khhttp.BuildBaseURL(host) + khhttp.CredentialProbePath resp, err := doGet(ctx, client, host, url) if err != nil { if isContextTimeout(err) { diff --git a/cmd/doctor/doctor_auth_test.go b/cmd/doctor/doctor_auth_test.go new file mode 100644 index 0000000..4058acb --- /dev/null +++ b/cmd/doctor/doctor_auth_test.go @@ -0,0 +1,90 @@ +package doctor_test + +// The auth check used to probe /api/auth/get-session and switch on the status +// code. That endpoint answers 200 with a null session for unauthenticated +// callers rather than 401, so doctor reported every caller as authenticated - +// including callers with no credential at all, while Wallet and Spend Cap in the +// same run correctly reported them as unauthenticated. These tests pin that the +// auth check now fails when the server refuses the credential. + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/keeperhub/cli/cmd/doctor" + khhttp "github.com/keeperhub/cli/internal/http" + "github.com/keeperhub/cli/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// authLine returns the doctor output line reporting the Auth check. +func authLine(out string) string { + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "Auth:") { + return line + } + } + return "" +} + +func TestDoctorCmd_AuthFailsWhenCredentialIsRefused(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == khhttp.CredentialProbePath { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer svr.Close() + + ios, outBuf, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(newDoctorFactory(ios, svr)) + require.NoError(t, tc.Execute([]string{})) + + line := authLine(outBuf.String()) + assert.Contains(t, line, "not authenticated") + assert.NotContains(t, line, "pass") +} + +func TestDoctorCmd_AuthPassesWhenCredentialIsAccepted(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer svr.Close() + + ios, outBuf, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(newDoctorFactory(ios, svr)) + require.NoError(t, tc.Execute([]string{})) + + assert.Contains(t, authLine(outBuf.String()), "authenticated") +} + +func TestDoctorCmd_AuthDoesNotProbeTheAnonymousTolerantEndpoint(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + seen := make(map[string]bool) + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen[r.URL.Path] = true + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer svr.Close() + + ios, _, _, _ := iostreams.Test() + tc := doctor.NewTestableCmd(newDoctorFactory(ios, svr)) + require.NoError(t, tc.Execute([]string{})) + + assert.True(t, seen[khhttp.CredentialProbePath], "auth check should probe the credential endpoint") + // get-session answers 200 to anyone, so probing it can only produce a + // false green. + assert.False(t, seen["/api/auth/get-session"], "auth check must not probe get-session") +} diff --git a/internal/auth/token.go b/internal/auth/token.go index bc97ed5..f33678a 100644 --- a/internal/auth/token.go +++ b/internal/auth/token.go @@ -138,7 +138,15 @@ func fetchSessionInfo(host, token string) (TokenInfo, error) { func fetchAPIKeyInfo(host, token string) (TokenInfo, error) { client := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest(http.MethodGet, khhttp.BuildBaseURL(host)+"/api/workflows?limit=1", nil) + // Must be an endpoint that requires auth. This previously probed + // /api/workflows, which resolves auth with required:false and answers 200 + // with an empty list to anonymous callers - so every string beginning with + // kh_ validated, including revoked and fabricated keys. + req, err := http.NewRequest( + http.MethodGet, + khhttp.BuildBaseURL(host)+khhttp.CredentialProbePath, + nil, + ) if err != nil { return TokenInfo{}, fmt.Errorf("creating validation request: %w", err) } diff --git a/internal/auth/token_test.go b/internal/auth/token_test.go new file mode 100644 index 0000000..7a0fa2f --- /dev/null +++ b/internal/auth/token_test.go @@ -0,0 +1,76 @@ +package auth + +// API-key validation used to probe /api/workflows, which resolves auth with +// required:false and answers 200 with an empty list to anonymous callers. Every +// string beginning with kh_ therefore validated, including revoked and +// fabricated keys - and `kh auth login` reads this to decide whether a stored +// credential is still good, so a revoked key made the login skip unrecoverable +// without --force. These tests pin that an unaccepted key is now rejected. + +import ( + "net/http" + "net/http/httptest" + "testing" + + khhttp "github.com/keeperhub/cli/internal/http" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFetchTokenInfo_RejectsAPIKeyTheServerDoesNotAccept(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + _, err := FetchTokenInfo(srv.URL, "kh_revokedOrFabricated") + + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid or revoked") +} + +func TestFetchTokenInfo_AcceptsAPIKeyTheServerAccepts(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + info, err := FetchTokenInfo(srv.URL, "kh_valid") + + require.NoError(t, err) + assert.Equal(t, AuthMethodAPIKey, info.Method) + assert.Equal(t, "api-key", info.Role) +} + +func TestFetchTokenInfo_ProbesAnAuthRequiredEndpoint(t *testing.T) { + var gotPath, gotAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + gotAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + _, err := FetchTokenInfo(srv.URL, "kh_valid") + require.NoError(t, err) + + // Guards against regressing onto an endpoint that tolerates anonymous + // callers - the defect this replaced. + assert.Equal(t, khhttp.CredentialProbePath, gotPath) + assert.NotEqual(t, "/api/workflows", gotPath) + assert.Equal(t, "Bearer kh_valid", gotAuth) +} + +func TestFetchTokenInfo_TreatsOtherFailuresAsInvalid(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + _, err := FetchTokenInfo(srv.URL, "kh_valid") + + require.Error(t, err) +} diff --git a/internal/http/credential_probe.go b/internal/http/credential_probe.go new file mode 100644 index 0000000..217df0d --- /dev/null +++ b/internal/http/credential_probe.go @@ -0,0 +1,20 @@ +package khhttp + +// CredentialProbePath is the endpoint used to decide whether a stored +// credential is actually accepted by the server. +// +// It must be a route that **requires** authentication. This is not a free +// choice: `/api/workflows` and `/api/auth/get-session` both answer 200 to +// anonymous callers - the former resolves auth with `required: false` and +// returns an empty list, the latter returns a null session rather than a 401. +// Probing either one reports every caller as authenticated, including callers +// with no credential at all. +// +// `/api/projects` resolves auth through resolveOrganizationId, which tries +// OAuth, then API key, then session, and answers 401 when none of them +// authenticates. That covers `kh_` API keys, which is what the device-login +// flow now issues. +// +// Both the doctor auth check and API-key validation share this constant so the +// two cannot drift onto different endpoints with different auth semantics. +const CredentialProbePath = "/api/projects" diff --git a/internal/http/credential_probe_test.go b/internal/http/credential_probe_test.go new file mode 100644 index 0000000..916a921 --- /dev/null +++ b/internal/http/credential_probe_test.go @@ -0,0 +1,42 @@ +package khhttp_test + +// Both the doctor auth check and API-key validation resolve their endpoint from +// CredentialProbePath, so the semantic requirement - that the endpoint actually +// requires authentication - can only be pinned on the constant itself. A test at +// either call site would simply follow the constant wherever it points, which is +// how the original defect went unnoticed. + +import ( + "testing" + + khhttp "github.com/keeperhub/cli/internal/http" + "github.com/stretchr/testify/assert" +) + +// Endpoints known to answer 200 to anonymous callers. Probing any of them +// reports every caller as authenticated. +// +// /api/workflows - resolves auth with required:false, returns an empty list +// /api/auth/get-session - returns a null session rather than 401 +// /api/openapi - public schema +func TestCredentialProbePath_IsNotAnonymousTolerant(t *testing.T) { + anonymousTolerant := []string{ + "/api/workflows", + "/api/auth/get-session", + "/api/openapi", + } + + for _, path := range anonymousTolerant { + assert.NotEqual( + t, + path, + khhttp.CredentialProbePath, + "%s answers 200 to anonymous callers, so probing it cannot detect a bad credential", + path, + ) + } +} + +func TestCredentialProbePath_IsAnAPIRoute(t *testing.T) { + assert.Equal(t, "/api/projects", khhttp.CredentialProbePath) +} diff --git a/internal/http/version.go b/internal/http/version.go index bfd19e5..2b1de7e 100644 --- a/internal/http/version.go +++ b/internal/http/version.go @@ -67,6 +67,8 @@ func checkVersion(current string, resp *http.Response, errOut io.Writer) { return } if semverLessThan(current, minimum) { - fmt.Fprintf(errOut, "warning: your CLI version (%s) is outdated; minimum required is %s\n", current, minimum) + // Updates are manual and unprompted, so this line is the only signal an + // out-of-date CLI gets. Name the remedy rather than only the mismatch. + fmt.Fprintf(errOut, "warning: your CLI version (%s) is outdated; minimum required is %s. Run: kh update\n", current, minimum) } } diff --git a/internal/http/version_remedy_test.go b/internal/http/version_remedy_test.go new file mode 100644 index 0000000..a37c6e6 --- /dev/null +++ b/internal/http/version_remedy_test.go @@ -0,0 +1,60 @@ +package khhttp_test + +// Updates are manual and unprompted, so the KH-Minimum-CLI-Version warning is +// the only signal an out-of-date CLI ever gets. It has to name the remedy, not +// just state the mismatch. + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/hashicorp/go-retryablehttp" + khhttp "github.com/keeperhub/cli/internal/http" + "github.com/keeperhub/cli/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func doWithMinimumVersion(t *testing.T, appVersion, minimum string) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if minimum != "" { + w.Header().Set("KH-Minimum-CLI-Version", minimum) + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + ios, _, errBuf, _ := iostreams.Test() + client := khhttp.NewClient(khhttp.ClientOptions{ + Host: srv.URL, + AppVersion: appVersion, + IOStreams: ios, + }) + + req, err := retryablehttp.NewRequest(http.MethodGet, srv.URL+"/test", nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + return errBuf.String() +} + +func TestCheckVersion_OutdatedWarningNamesTheRemedy(t *testing.T) { + out := doWithMinimumVersion(t, "0.3.0", "0.11.1") + + assert.Contains(t, out, "outdated") + assert.Contains(t, out, "0.11.1") + // Stating the mismatch without the remedy leaves the user to guess. + assert.Contains(t, out, "kh update") +} + +func TestCheckVersion_SilentWhenCurrent(t *testing.T) { + assert.Empty(t, doWithMinimumVersion(t, "0.12.0", "0.11.1")) +} + +func TestCheckVersion_SilentWhenServerAdvertisesNothing(t *testing.T) { + assert.Empty(t, doWithMinimumVersion(t, "0.3.0", "")) +} From 9f6a32300e465c3fd09059072f2eab3564cf4795 Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Tue, 28 Jul 2026 12:32:14 +1000 Subject: [PATCH 2/2] fix: KEEP-1049 guard the probe-path assertion and simplify a login condition Doctor fans its checks out across goroutines, so the httptest handler in the new probe-path test ran concurrently and wrote an unguarded map. It passed locally and failed under CI's go test -race. Guarded with a mutex. Also applies De Morgan to the login skip condition, which staticcheck QF1001 flags. That line came from #73 and is failing lint on main, not only on this branch. --- cmd/auth/login.go | 2 +- cmd/doctor/doctor_auth_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 454554a..bd2b927 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -73,7 +73,7 @@ See also: kh auth status, kh auth logout`, // Re-running login on an already-authenticated host would strand // the stored key and leave a new one behind on every invocation, // so stop early unless the caller asked for a fresh credential. - if !(withToken || force) { + if !withToken && !force { if entry, ok := hosts.HostEntry(host); ok && entry.Token != "" { if info, infoErr := FetchTokenInfoFunc(host, entry.Token); infoErr == nil { fmt.Fprintf(f.IOStreams.Out, diff --git a/cmd/doctor/doctor_auth_test.go b/cmd/doctor/doctor_auth_test.go index 4058acb..6c295be 100644 --- a/cmd/doctor/doctor_auth_test.go +++ b/cmd/doctor/doctor_auth_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "testing" "github.com/keeperhub/cli/cmd/doctor" @@ -70,9 +71,14 @@ func TestDoctorCmd_AuthPassesWhenCredentialIsAccepted(t *testing.T) { func TestDoctorCmd_AuthDoesNotProbeTheAnonymousTolerantEndpoint(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + // Doctor fans its checks out across goroutines, so the handler runs + // concurrently and the record of seen paths has to be guarded. + var mu sync.Mutex seen := make(map[string]bool) svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() seen[r.URL.Path] = true + mu.Unlock() w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{}`)) @@ -83,6 +89,8 @@ func TestDoctorCmd_AuthDoesNotProbeTheAnonymousTolerantEndpoint(t *testing.T) { tc := doctor.NewTestableCmd(newDoctorFactory(ios, svr)) require.NoError(t, tc.Execute([]string{})) + mu.Lock() + defer mu.Unlock() assert.True(t, seen[khhttp.CredentialProbePath], "auth check should probe the credential endpoint") // get-session answers 200 to anyone, so probing it can only produce a // false green.