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.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..6c295be --- /dev/null +++ b/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") +} 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", "")) +}