mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 22:37:03 +00:00
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: <url> answered
HTTP n ... They were not rejected; try again".
- No answer or an undecodable body: "no usable answer from <url>".
- 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
This commit is contained in:
@@ -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(`<html>forbidden</html>`))
|
||||
},
|
||||
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(`<html>bad gateway</html>`))
|
||||
},
|
||||
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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
+29
-8
@@ -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" {
|
||||
|
||||
@@ -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)
|
||||
|
||||
+30
-1
@@ -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) ---
|
||||
|
||||
Reference in New Issue
Block a user