From 833e309052d2d0843069eb4b8335d83472b6557f Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 8 Oct 2026 15:03:28 -0400 Subject: [PATCH] fix(cli): whoami says what a failed check established, and exits non-zero (BUG-2706) (#1901) * fix(cli): whoami says what a failed check established, and exits non-zero (BUG-2706) whoami mapped EVERY failed identity check to a rejection ("PAD_TOKEN is set but the server rejected it." / "Session expired.") and exited 0. Measured on main: PAD_TOKEN set, server unreachable, "rejected", exit 0. whoami is the safe-check scripts and agents run first, so a network blip sent operators to rotate a healthy token, and exit 0 told scripts they were signed in (chatgpt-review-seed.sh's `whoami || die` could never fire). - Only a 401 or 403 is a rejection (PAD_TOKEN: "rejected it (HTTP n)"; stored session: "session expired"). - Any other status: "could not check your credentials: answered HTTP n ... They were not rejected; try again". - No answer or an undecodable body: "no usable answer from ". - Every failure, and "not logged in", is now an error (exit 1). - cli.StatusError: a non-2xx with a non-envelope body (a proxy page) used to be a plain fmt error with the status only in its text. It keeps the same Error() text and now carries the status; cli.HTTPStatus reads it from either error type. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * fix(cli): a 403 on whoami's stored session is a refusal, not an expiry (BUG-2706 codex r1) Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS --- cmd/pad/bug2706_whoami_failure_test.go | 122 +++++++++++++++++++++++++ cmd/pad/cmd_auth.go | 37 ++++++-- cmd/pad/env_token_override_test.go | 9 +- internal/cli/client.go | 31 ++++++- 4 files changed, 186 insertions(+), 13 deletions(-) create mode 100644 cmd/pad/bug2706_whoami_failure_test.go diff --git a/cmd/pad/bug2706_whoami_failure_test.go b/cmd/pad/bug2706_whoami_failure_test.go new file mode 100644 index 000000000..cae420312 --- /dev/null +++ b/cmd/pad/bug2706_whoami_failure_test.go @@ -0,0 +1,122 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// BUG-2706: whoami reported EVERY failed identity check as a rejection +// ("PAD_TOKEN is set but the server rejected it." / "Session expired.") and +// exited 0. Measured on main: PAD_TOKEN set, server unreachable, "rejected", +// exit 0. Only a 401 or 403 is a rejection; anything else means the +// credential was not checked, and every failure is an error. +func TestBUG2706_WhoamiSaysWhatTheFailureEstablished(t *testing.T) { + const rejected = "rejected" + cases := []struct { + name string + envToken bool + handler http.HandlerFunc // nil: nothing listens + want string + mustNot string + }{ + { + name: "env token refused (JSON 401)", envToken: true, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":{"code":"unauthorized","message":"Authentication required"}}`)) + }, + want: "PAD_TOKEN is set but the server rejected it (HTTP 401)", + }, + { + name: "env token refused (non-JSON 403)", envToken: true, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`forbidden`)) + }, + want: "rejected it (HTTP 403)", + }, + { + name: "stored session refused", envToken: false, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":{"code":"unauthorized","message":"Authentication required"}}`)) + }, + want: "session expired", + }, + { + name: "stored session forbidden (403)", envToken: false, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":{"code":"forbidden","message":"Forbidden"}}`)) + }, + want: "refused your stored session (HTTP 403)", mustNot: "expired", + }, + { + name: "server error (JSON 500)", envToken: true, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"code":"internal","message":"boom"}}`)) + }, + want: "answered HTTP 500", mustNot: rejected + " it", + }, + { + name: "proxy page (502)", envToken: false, + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = w.Write([]byte(`bad gateway`)) + }, + want: "answered HTTP 502", mustNot: "expired", + }, + { + name: "malformed 200 body", envToken: true, + handler: func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`not json`)) + }, + want: "no usable answer", mustNot: rejected + " it", + }, + { + name: "unreachable", envToken: true, handler: nil, + want: "no usable answer", mustNot: rejected + " it", + }, + { + name: "unreachable, stored session", envToken: false, handler: nil, + want: "no usable answer", mustNot: "expired", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + home := setTempHomeMain(t) + var url string + if c.handler != nil { + srv := httptest.NewServer(c.handler) + t.Cleanup(srv.Close) + url = srv.URL + } else { + srv := httptest.NewServer(http.NotFoundHandler()) + url = srv.URL + srv.Close() // the port now refuses connections + } + t.Setenv("PAD_URL", url) + if c.envToken { + t.Setenv("PAD_TOKEN", "pad_envtoken") + } else { + t.Setenv("PAD_TOKEN", "") + writeCredStore(t, home, url, "padsess_stored") + } + + cmd := whoamiCmd() + err := cmd.RunE(cmd, nil) + if err == nil { + t.Fatal("whoami returned nil (exit 0) on a failed identity check") + } + if !strings.Contains(err.Error(), c.want) { + t.Errorf("err = %q, want it to contain %q", err, c.want) + } + if c.mustNot != "" && strings.Contains(err.Error(), c.mustNot) { + t.Errorf("err = %q claims %q, but the credential was never checked", err, c.mustNot) + } + }) + } +} diff --git a/cmd/pad/cmd_auth.go b/cmd/pad/cmd_auth.go index 9f359c4db..19d02158b 100644 --- a/cmd/pad/cmd_auth.go +++ b/cmd/pad/cmd_auth.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "os" "os/signal" "path/filepath" @@ -874,6 +875,32 @@ func logoutCmd() *cobra.Command { } } +// whoamiFailure says what a failed identity check actually established +// (BUG-2706). Only a 401 or 403 means the credential was refused. Anything +// else, unreachable, a server error, a body that would not decode, means the +// credential was NOT checked, and it used to be reported as rejected or +// expired, which sends the operator to rotate a healthy token. Every failure +// exits non-zero: whoami is the safe-check scripts and agents run first, and +// exit 0 told them they were signed in. +func whoamiFailure(err error, envToken bool, baseURL string) error { + switch status := cli.HTTPStatus(err); { + case status == http.StatusUnauthorized || status == http.StatusForbidden: + if envToken { + return fmt.Errorf("PAD_TOKEN is set but the server rejected it (HTTP %d). Fix or unset PAD_TOKEN", status) + } + if status == http.StatusForbidden { + // Not an expiry: the server knows the session and refuses it + // (a disabled account, say), so logging in again may not help. + return errors.New("the server refused your stored session (HTTP 403). Run 'pad auth login', and ask an admin if it persists") + } + return errors.New("session expired. Run 'pad auth login'") + case status != 0: + return fmt.Errorf("could not check your credentials: %s answered HTTP %d (%v). They were not rejected; try again", baseURL, status, err) + default: + return fmt.Errorf("could not check your credentials: no usable answer from %s (%v). They were not rejected; try again", baseURL, err) + } +} + func whoamiCmd() *cobra.Command { return &cobra.Command{ Use: "whoami", @@ -899,8 +926,7 @@ func whoamiCmd() *cobra.Command { } creds = store.Get(cfg.BaseURL()) if creds == nil || creds.Token == "" { - fmt.Println("Not logged in. Run 'pad auth login'.") - return nil + return errors.New("not logged in. Run 'pad auth login'") } } @@ -916,12 +942,7 @@ func whoamiCmd() *cobra.Command { user, err := client.GetCurrentUser() if err != nil { - if envToken != "" { - fmt.Println("PAD_TOKEN is set but the server rejected it.") - return nil - } - fmt.Println("Session expired. Run 'pad auth login'.") - return nil + return whoamiFailure(err, envToken != "", cfg.BaseURL()) } if formatFlag == "json" { diff --git a/cmd/pad/env_token_override_test.go b/cmd/pad/env_token_override_test.go index c7552ce39..29af3d113 100644 --- a/cmd/pad/env_token_override_test.go +++ b/cmd/pad/env_token_override_test.go @@ -96,8 +96,9 @@ func TestWhoami_UsesEnvToken(t *testing.T) { } } -// Without PAD_TOKEN and without credentials, whoami keeps its existing -// "Not logged in" behaviour and never contacts the server. +// Without PAD_TOKEN and without credentials, whoami says "not logged in" +// and never contacts the server. Since BUG-2706 it says so as an error (exit +// non-zero): exit 0 told scripts they were signed in. func TestWhoami_NoTokenNoStoreUnchanged(t *testing.T) { setTempHomeMain(t) srv := newStubAuthServer(t, "unused") @@ -105,8 +106,8 @@ func TestWhoami_NoTokenNoStoreUnchanged(t *testing.T) { t.Setenv("PAD_TOKEN", "") cmd := whoamiCmd() - if err := cmd.RunE(cmd, nil); err != nil { - t.Fatalf("whoami: %v", err) + if err := cmd.RunE(cmd, nil); err == nil || !strings.Contains(err.Error(), "not logged in") { + t.Fatalf("whoami: err = %v, want a not-logged-in error", err) } if srv.meHits != 0 { t.Errorf("expected no server contact when unauthenticated, got %d /auth/me hits", srv.meHits) diff --git a/internal/cli/client.go b/internal/cli/client.go index 5e5fe46ec..ddd386c64 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -2602,7 +2602,36 @@ func parseErrorBody(status int, body []byte) error { errResp.Error.Status = status return &errResp.Error } - return fmt.Errorf("API error: %d %s", status, string(body)) + return &StatusError{Status: status, Body: string(body)} +} + +// StatusError is a non-2xx answer whose body was not the server's JSON error +// envelope: a proxy's page, an empty body, a stub. It used to be a plain +// fmt error, which kept the status only inside its text, so a caller could +// not tell a 401 from a network failure (BUG-2706). Error() is the text the +// plain error had. +type StatusError struct { + Status int + Body string +} + +func (e *StatusError) Error() string { + return fmt.Sprintf("API error: %d %s", e.Status, e.Body) +} + +// HTTPStatus is the HTTP status a request error carries, or 0 when the +// request got no answer at all (the server was unreachable) or the error did +// not come from an HTTP answer. +func HTTPStatus(err error) int { + var apiErr *APIError + if errors.As(err, &apiErr) { + return apiErr.Status + } + var statusErr *StatusError + if errors.As(err, &statusErr) { + return statusErr.Status + } + return 0 } // --- Item reminders (IDEA-2641) ---