fix: KEEP-1049 validate credentials against an endpoint that requires auth

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,

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) {

cmd/doctor/doctor_auth_test.go

@@ -0,0 +1,98 @@
+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"
+	"sync"
+	"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())
+	// 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(`{}`))
+	}))
+	defer svr.Close()
+
+	ios, _, _, _ := iostreams.Test()
+	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.
+	assert.False(t, seen["/api/auth/get-session"], "auth check must not probe get-session")
+}

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)
 	}

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)
+}

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"

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)
+}

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)
 	}
 }

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", ""))
+}