From 69de7c273805baaf19bb3f1181a72d47a6e90233 Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Wed, 1 Apr 2026 02:01:21 -0600 Subject: [PATCH 1/7] [WEBXP-469] Add `circleci signup` with Magic Path v2 architecture Opens browser directly to /cli-auth with CLI params as top-level query params. The frontend handles session detection: authenticated users get instant PAT creation; unauthenticated users are redirected to signup first. Bypasses auth-svc return-to bug entirely. Security hardening: - Strict Origin validation (reject missing/wrong with 403) - Constant-time state comparison via crypto/subtle - CORS pinned to https://app.circleci.com (static, never reflected) - Access-Control-Allow-Private-Network for Chrome PNA - Method validation (GET only on /token) - 127.0.0.1 binding only Features: - Unique PAT label (hostname + timestamp) prevents 422 duplicates - Already-authenticated guard with --force bypass - --no-browser fallback for headless/SSH - JSON responses for structured error handling - Human-readable URL display in terminal - 24 unit tests Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/root.go | 1 + cmd/root_test.go | 2 +- cmd/signup.go | 295 ++++++++++++++++++++++++++++++++++ cmd/signup_unit_test.go | 348 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 645 insertions(+), 1 deletion(-) create mode 100644 cmd/signup.go create mode 100644 cmd/signup_unit_test.go diff --git a/cmd/root.go b/cmd/root.go index a5a7fa0fd..2809dec6c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -178,6 +178,7 @@ func MakeCommands() *cobra.Command { rootCmd.AddCommand(newVersionCommand(rootOptions)) rootCmd.AddCommand(newDiagnosticCommand(rootOptions)) rootCmd.AddCommand(newSetupCommand(rootOptions)) + rootCmd.AddCommand(newSignupCommand(rootOptions)) rootCmd.AddCommand(newInitCommand(rootOptions)) rootCmd.AddCommand(followProjectCommand(rootOptions)) diff --git a/cmd/root_test.go b/cmd/root_test.go index bdcc6f534..dc4209e2e 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -16,7 +16,7 @@ var _ = Describe("Root", func() { Describe("subcommands", func() { It("can create commands", func() { commands := cmd.MakeCommands() - Expect(len(commands.Commands())).To(Equal(29)) + Expect(len(commands.Commands())).To(Equal(30)) }) }) diff --git a/cmd/signup.go b/cmd/signup.go new file mode 100644 index 000000000..54767e638 --- /dev/null +++ b/cmd/signup.go @@ -0,0 +1,295 @@ +package cmd + +import ( + "context" + "crypto/rand" + "crypto/subtle" + "encoding/hex" + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + "os" + "strings" + "time" + + "github.com/pkg/browser" + "github.com/pkg/errors" + "github.com/spf13/cobra" + + "github.com/CircleCI-Public/circleci-cli/prompt" + "github.com/CircleCI-Public/circleci-cli/settings" + "github.com/CircleCI-Public/circleci-cli/telemetry" +) + +type signupOptions struct { + cfg *settings.Config + noBrowser bool + force bool +} + +func newSignupCommand(config *settings.Config) *cobra.Command { + opts := signupOptions{ + cfg: config, + } + + cmd := &cobra.Command{ + Use: "signup", + Short: "Sign up for a CircleCI account or authenticate an existing account", + RunE: func(cmd *cobra.Command, _ []string) error { + err := runSignup(cmd, opts) + + telemetryClient, ok := telemetry.FromContext(cmd.Context()) + if ok { + _ = telemetryClient.Track(createSignupEvent(opts.noBrowser, err)) + } + + return err + }, + } + + cmd.Flags().BoolVar(&opts.noBrowser, "no-browser", false, "Don't open a browser; print the signup URL and prompt for a token instead") + cmd.Flags().BoolVar(&opts.force, "force", false, "Run signup even if already authenticated") + + return cmd +} + +func createSignupEvent(noBrowser bool, err error) telemetry.Event { + properties := map[string]interface{}{ + "no_browser": noBrowser, + "has_been_executed": true, + } + if err != nil { + properties["error"] = err.Error() + } + return telemetry.Event{ + Object: "cli-signup", + Action: "signup", + Properties: properties, + } +} + +func runSignup(cmd *cobra.Command, opts signupOptions) error { + if !opts.force && opts.cfg.Token != "" { + fmt.Println("You're already authenticated. Your CLI is configured with a personal API token.") + fmt.Println("If you want to reconfigure, run `circleci setup`.") + return nil + } + + state, err := generateState() + if err != nil { + return errors.Wrap(err, "failed to generate cryptographic state") + } + + if opts.noBrowser { + return signupNoBrowser(opts, state) + } + + return signupWithBrowser(cmd, opts, state) +} + +func generateState() (string, error) { + b := make([]byte, 16) + if _, err := rand.Read(b); err != nil { + return "", err + } + return hex.EncodeToString(b), nil +} + +func signupNoBrowser(opts signupOptions, state string) error { + signupURL := "https://app.circleci.com/authentication/login?f=gho&return-to=/settings/user/tokens" + fmt.Printf("Open this URL in your browser to sign up:\n\n %s\n\n", signupURL) + + token, err := prompt.ReadSecretStringFromUser("Paste your CircleCI API token here") + if err != nil { + return errors.Wrap(err, "failed to read token") + } + + if token == "" { + return errors.New("no token provided") + } + + return saveToken(opts.cfg, token) +} + +func signupWithBrowser(cmd *cobra.Command, opts signupOptions, state string) error { + // Start an ephemeral HTTP server on a random available port. + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return errors.Wrap(err, "failed to start local server") + } + port := listener.Addr().(*net.TCPAddr).Port + + tokenCh := make(chan string, 1) + errCh := make(chan error, 1) + + mux := http.NewServeMux() + mux.HandleFunc("/token", corsMiddleware(handleToken(state, tokenCh, errCh))) + + server := &http.Server{ + Handler: mux, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + + go func() { + if serveErr := server.Serve(listener); serveErr != nil && serveErr != http.ErrServerClosed { + errCh <- serveErr + } + }() + + // Generate a unique PAT label to avoid 422 duplicate errors when the + // user runs signup multiple times or from different machines. + hostname, err := os.Hostname() + if err != nil { + hostname = "unknown" + } + label := fmt.Sprintf("circleci-cli-%s-%d", sanitizeHostname(hostname), time.Now().Unix()) + + // Build the signup URL. Go directly to /cli-auth (Magic Path). + // The frontend checks for an existing session: if authenticated, it creates + // the PAT immediately; if not, it redirects to signup first. + params := url.Values{} + params.Set("cli_port", fmt.Sprintf("%d", port)) + params.Set("cli_state", state) + params.Set("cli_label", label) + signupURL := "https://app.circleci.com/cli-auth?" + params.Encode() + + trackSignupStep(cmd, "browser_opening", nil) + fmt.Println("Opening your browser to sign up for CircleCI...") + decodedURL, _ := url.QueryUnescape(signupURL) + fmt.Printf(" %s\n", decodedURL) + + if err := browser.OpenURL(signupURL); err != nil { + fmt.Printf("⚠️ Could not open browser automatically: %v\n", err) + fmt.Printf(" Please manually visit: %s\n", decodedURL) + } + + fmt.Println("Waiting for authentication...") + + // Wait for the token or an error, with a timeout. + select { + case token := <-tokenCh: + _ = server.Shutdown(context.Background()) + trackSignupStep(cmd, "token_received", nil) + return saveToken(opts.cfg, token) + case err := <-errCh: + _ = server.Shutdown(context.Background()) + trackSignupStep(cmd, "failed", nil) + return errors.Wrap(err, "signup failed") + case <-time.After(5 * time.Minute): + _ = server.Shutdown(context.Background()) + trackSignupStep(cmd, "timeout", nil) + return errors.New("timed out waiting for signup to complete. Run `circleci setup` to manually configure your CLI with a personal API token") + } +} + +const allowedOrigin = "https://app.circleci.com" + +// corsMiddleware validates the Origin header and adds CORS headers allowing +// the CircleCI frontend to make cross-origin requests to the CLI's local server. +// Requests with missing or non-matching Origin are rejected with 403. +func corsMiddleware(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + origin := r.Header.Get("Origin") + if origin != allowedOrigin { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + + w.Header().Set("Access-Control-Allow-Origin", allowedOrigin) + w.Header().Set("Access-Control-Allow-Methods", "GET") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type") + w.Header().Set("Access-Control-Allow-Private-Network", "true") + w.Header().Set("Access-Control-Max-Age", "300") + + if r.Method == http.MethodOptions { + w.WriteHeader(http.StatusNoContent) + return + } + + next(w, r) + } +} + +func stateMatches(a, b string) bool { + return subtle.ConstantTimeCompare([]byte(a), []byte(b)) == 1 +} + +func handleToken(expectedState string, tokenCh chan<- string, errCh chan<- error) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } + + query := r.URL.Query() + token := query.Get("token") + state := query.Get("cli_state") + callbackErr := query.Get("error") + + // When an error is present, state validation is best-effort: if state + // is provided it must match, but a missing state is tolerated because + // the frontend may not have had access to it when the failure occurred. + if callbackErr != "" { + if state != "" && !stateMatches(state, expectedState) { + http.Error(w, "Invalid state", http.StatusForbidden) + errCh <- errors.New("state mismatch — possible CSRF attempt") + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"status": "error"}) + errCh <- fmt.Errorf("authentication failed (%s). Run `circleci setup` to manually configure your CLI with a personal API token", callbackErr) + return + } + + if !stateMatches(state, expectedState) { + http.Error(w, "Invalid state", http.StatusForbidden) + errCh <- errors.New("state mismatch — possible CSRF attempt") + return + } + + if token == "" { + http.Error(w, "Missing token", http.StatusBadRequest) + errCh <- errors.New("callback returned an empty token. Run `circleci setup` to manually configure your CLI with a personal API token") + return + } + + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) + tokenCh <- token + } +} + +func sanitizeHostname(h string) string { + var b strings.Builder + for _, r := range h { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' { + b.WriteRune(r) + } + } + s := b.String() + if s == "" { + return "unknown" + } + return s +} + +func saveToken(cfg *settings.Config, token string) error { + cfg.Token = token + if err := cfg.WriteToDisk(); err != nil { + return errors.Wrap(err, "failed to save token to config") + } + fmt.Println("\n✅ Welcome to CircleCI! Your CLI is now authenticated.") + return nil +} + +func trackSignupStep(cmd *cobra.Command, step string, extra map[string]interface{}) { + client, ok := telemetry.FromContext(cmd.Context()) + if !ok { + return + } + invID, _ := telemetry.InvocationIDFromContext(cmd.Context()) + telemetry.TrackWorkflowStep(client, "signup", step, invID, extra) +} diff --git a/cmd/signup_unit_test.go b/cmd/signup_unit_test.go new file mode 100644 index 000000000..c429942f8 --- /dev/null +++ b/cmd/signup_unit_test.go @@ -0,0 +1,348 @@ +package cmd + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "time" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/CircleCI-Public/circleci-cli/clitest" + "github.com/CircleCI-Public/circleci-cli/settings" +) + +var _ = Describe("Signup", func() { + Describe("already authenticated guard", func() { + It("should print already authenticated when token exists", func() { + opts := signupOptions{ + cfg: &settings.Config{Token: "existing-token"}, + } + + output := clitest.WithCapturedOutput(func() { + err := runSignup(dummyCmd(), opts) + Expect(err).ShouldNot(HaveOccurred()) + }) + + Expect(output).To(ContainSubstring("already authenticated")) + Expect(output).To(ContainSubstring("circleci setup")) + }) + + It("should not guard when --force is set", func() { + opts := signupOptions{ + cfg: &settings.Config{Token: "existing-token"}, + force: true, + } + + Expect(opts.force).To(BeTrue()) + Expect(!opts.force && opts.cfg.Token != "").To(BeFalse()) + }) + + It("should not guard when no token exists", func() { + opts := signupOptions{ + cfg: &settings.Config{Token: ""}, + } + + Expect(!opts.force && opts.cfg.Token != "").To(BeFalse()) + }) + }) + + Describe("generateState", func() { + It("should return a 32-character hex string", func() { + state, err := generateState() + Expect(err).ShouldNot(HaveOccurred()) + Expect(state).To(HaveLen(32)) + Expect(state).To(MatchRegexp(`^[0-9a-f]{32}$`)) + }) + + It("should generate unique values", func() { + a, _ := generateState() + b, _ := generateState() + Expect(a).ToNot(Equal(b)) + }) + }) + + Describe("corsMiddleware", func() { + var dummyHandler http.HandlerFunc + var handlerCalled bool + + BeforeEach(func() { + handlerCalled = false + dummyHandler = func(w http.ResponseWriter, r *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "ok") + } + }) + + It("should set CORS headers on a GET request with valid origin", func() { + wrapped := corsMiddleware(dummyHandler) + req := httptest.NewRequest("GET", "/token", nil) + req.Header.Set("Origin", "https://app.circleci.com") + rec := httptest.NewRecorder() + + wrapped.ServeHTTP(rec, req) + + Expect(rec.Header().Get("Access-Control-Allow-Origin")).To(Equal("https://app.circleci.com")) + Expect(rec.Header().Get("Access-Control-Allow-Methods")).To(Equal("GET")) + Expect(rec.Header().Get("Access-Control-Allow-Headers")).To(Equal("Content-Type")) + Expect(rec.Header().Get("Access-Control-Allow-Private-Network")).To(Equal("true")) + Expect(rec.Header().Get("Access-Control-Max-Age")).To(Equal("300")) + Expect(handlerCalled).To(BeTrue()) + Expect(rec.Code).To(Equal(http.StatusOK)) + }) + + It("should return 204 on OPTIONS preflight without calling the handler", func() { + wrapped := corsMiddleware(dummyHandler) + req := httptest.NewRequest("OPTIONS", "/token", nil) + req.Header.Set("Origin", "https://app.circleci.com") + rec := httptest.NewRecorder() + + wrapped.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusNoContent)) + Expect(rec.Header().Get("Access-Control-Allow-Origin")).To(Equal("https://app.circleci.com")) + Expect(rec.Header().Get("Access-Control-Allow-Private-Network")).To(Equal("true")) + Expect(handlerCalled).To(BeFalse()) + }) + + It("should reject requests with wrong origin", func() { + wrapped := corsMiddleware(dummyHandler) + req := httptest.NewRequest("GET", "/token", nil) + req.Header.Set("Origin", "https://evil.com") + rec := httptest.NewRecorder() + + wrapped.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusForbidden)) + Expect(handlerCalled).To(BeFalse()) + }) + + It("should reject requests with missing origin", func() { + wrapped := corsMiddleware(dummyHandler) + req := httptest.NewRequest("GET", "/token", nil) + rec := httptest.NewRecorder() + + wrapped.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusForbidden)) + Expect(handlerCalled).To(BeFalse()) + }) + }) + + Describe("unique PAT label", func() { + It("should generate a cli_label containing hostname and timestamp", func() { + hostname, _ := os.Hostname() + label := fmt.Sprintf("circleci-cli-%s-%d", hostname, time.Now().Unix()) + + Expect(label).To(ContainSubstring("circleci-cli-")) + Expect(label).To(ContainSubstring(hostname)) + Expect(label).To(MatchRegexp(`-\d+$`)) + }) + }) + + Describe("Magic Path URL", func() { + It("should open browser directly to /cli-auth, not /authentication/", func() { + params := url.Values{} + params.Set("cli_port", "12345") + params.Set("cli_state", "abc123") + params.Set("cli_label", "circleci-cli-test-1234") + signupURL := "https://app.circleci.com/cli-auth?" + params.Encode() + + Expect(signupURL).ToNot(ContainSubstring("/authentication/")) + Expect(signupURL).ToNot(ContainSubstring("/successful-signup")) + Expect(signupURL).To(HavePrefix("https://app.circleci.com/cli-auth?")) + Expect(signupURL).To(ContainSubstring("cli_port=12345")) + Expect(signupURL).To(ContainSubstring("cli_state=abc123")) + Expect(signupURL).To(ContainSubstring("cli_label=circleci-cli-test-1234")) + }) + }) + + Describe("handleToken", func() { + var ( + tokenCh chan string + errCh chan error + state string + ) + + BeforeEach(func() { + tokenCh = make(chan string, 1) + errCh = make(chan error, 1) + state = "abc123" + }) + + It("should accept a valid token and cli_state with JSON response", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?token=mytoken&cli_state=abc123", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusOK)) + Expect(rec.Header().Get("Content-Type")).To(Equal("application/json")) + Expect(rec.Body.String()).To(ContainSubstring(`"status":"ok"`)) + Eventually(tokenCh).Should(Receive(Equal("mytoken"))) + }) + + It("should reject a cli_state mismatch with 403", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?token=mytoken&cli_state=wrong", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusForbidden)) + Expect(rec.Body.String()).To(ContainSubstring("Invalid state")) + Eventually(errCh).Should(Receive()) + }) + + It("should reject non-GET methods", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("POST", "/token?token=mytoken&cli_state=abc123", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusMethodNotAllowed)) + }) + + It("should reject a missing token when no error param is present", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?cli_state=abc123", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusBadRequest)) + Expect(rec.Body.String()).To(ContainSubstring("Missing token")) + var received error + Eventually(errCh).Should(Receive(&received)) + Expect(received.Error()).To(ContainSubstring("circleci setup")) + }) + + Context("with error param from frontend", func() { + It("should forward the error when cli_state matches", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?error=token_creation_failed&cli_state=abc123", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusOK)) + Expect(rec.Header().Get("Content-Type")).To(Equal("application/json")) + Expect(rec.Body.String()).To(ContainSubstring(`"status":"error"`)) + var received error + Eventually(errCh).Should(Receive(&received)) + Expect(received.Error()).To(ContainSubstring("authentication failed")) + Expect(received.Error()).To(ContainSubstring("token_creation_failed")) + Expect(received.Error()).To(ContainSubstring("circleci setup")) + }) + + It("should tolerate a missing cli_state when error is present", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?error=no_token", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusOK)) + var received error + Eventually(errCh).Should(Receive(&received)) + Expect(received.Error()).To(ContainSubstring("no_token")) + }) + + It("should reject error with a mismatched cli_state", func() { + handler := handleToken(state, tokenCh, errCh) + req := httptest.NewRequest("GET", "/token?error=token_creation_failed&cli_state=wrong", nil) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + Expect(rec.Code).To(Equal(http.StatusForbidden)) + Expect(rec.Body.String()).To(ContainSubstring("Invalid state")) + Eventually(errCh).Should(Receive()) + }) + }) + }) + + Describe("sanitizeHostname", func() { + It("should strip non-alphanumeric characters except hyphens", func() { + Expect(sanitizeHostname("my.local")).To(Equal("myhostlocal")) + Expect(sanitizeHostname("MacBook-Pro")).To(Equal("MacBook-Pro")) + Expect(sanitizeHostname("host name!@#")).To(Equal("hostname")) + Expect(sanitizeHostname("")).To(Equal("unknown")) + Expect(sanitizeHostname("!!!")).To(Equal("unknown")) + }) + }) + + Describe("stateMatches", func() { + It("should return true for matching states", func() { + Expect(stateMatches("abc123", "abc123")).To(BeTrue()) + }) + + It("should return false for mismatched states", func() { + Expect(stateMatches("abc123", "wrong")).To(BeFalse()) + }) + + It("should return false for empty vs non-empty", func() { + Expect(stateMatches("", "abc123")).To(BeFalse()) + }) + }) + + Describe("saveToken", func() { + var tempSettings *clitest.TempSettings + + BeforeEach(func() { + tempSettings = clitest.WithTempSettings() + }) + + AfterEach(func() { + tempSettings.Close() + }) + + It("should write the token to the config file", func() { + cfg := &settings.Config{ + FileUsed: tempSettings.Config.Path, + Host: "https://circleci.com", + } + + output := clitest.WithCapturedOutput(func() { + err := saveToken(cfg, "my-new-token") + Expect(err).ShouldNot(HaveOccurred()) + }) + + Expect(output).To(ContainSubstring("Welcome to CircleCI")) + Expect(cfg.Token).To(Equal("my-new-token")) + + // Verify it was persisted to disk + file, err := os.Open(tempSettings.Config.Path) + Expect(err).ShouldNot(HaveOccurred()) + defer file.Close() + + content, err := io.ReadAll(file) + Expect(err).ShouldNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring("token: my-new-token")) + }) + }) + + Describe("createSignupEvent", func() { + It("should create event with no_browser property", func() { + event := createSignupEvent(true, nil) + Expect(event.Object).To(Equal("cli-signup")) + Expect(event.Action).To(Equal("signup")) + Expect(event.Properties["no_browser"]).To(BeTrue()) + Expect(event.Properties["has_been_executed"]).To(BeTrue()) + Expect(event.Properties).ToNot(HaveKey("error")) + }) + + It("should include error when present", func() { + event := createSignupEvent(false, fmt.Errorf("something broke")) + Expect(event.Properties["error"]).To(Equal("something broke")) + Expect(event.Properties["no_browser"]).To(BeFalse()) + }) + }) +}) From d249bc6faf8b213d4d63688cd6581b754f447174 Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Thu, 16 Apr 2026 21:24:25 -0600 Subject: [PATCH 2/7] [WEBXP-747] Switch circleci signup to server-side handshake polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the localhost HTTP callback server with a client-initiated polling loop against the auth-svc handshake endpoint. The CLI generates a UUID v4 handshake_id, opens the browser to /cli-auth?handshake_id=..., and polls GET /api/v1/cli-handshake/{id} every 3 seconds until the token arrives (200), the handshake expires (404), or the 10-minute timeout elapses. This eliminates the cross-origin / Private-Network / same-browser / MFA- interrupt failure modes of the prior localhost approach — the browser and CLI never communicate directly, so any device can complete auth. - Remove localhost listener, CORS/PNA middleware, state-nonce machinery, and the cli_port/cli_state/cli_label URL params. - Add UUID-based handshake_id, pollHandshake with retry on transient network errors, ctx-based Ctrl+C cancellation, 10-minute overall deadline, and a CIRCLECI_APP_URL env override for enterprise/testing. - --no-browser now prints the URL for cross-device use instead of prompting for a manual token paste (the handshake makes manual paste unnecessary). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 311 +++++++++++++++++----------------------- cmd/signup_unit_test.go | 303 +++++++++++--------------------------- 2 files changed, 217 insertions(+), 397 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index 54767e638..3ab3d15b6 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -2,27 +2,38 @@ package cmd import ( "context" - "crypto/rand" - "crypto/subtle" - "encoding/hex" "encoding/json" + "errors" "fmt" - "net" "net/http" - "net/url" "os" - "strings" + "os/signal" "time" + "github.com/google/uuid" "github.com/pkg/browser" - "github.com/pkg/errors" "github.com/spf13/cobra" - "github.com/CircleCI-Public/circleci-cli/prompt" "github.com/CircleCI-Public/circleci-cli/settings" "github.com/CircleCI-Public/circleci-cli/telemetry" ) +const ( + // App base URL override for enterprise / testing. Falls back to + // defaultAppBaseURL when unset. + appBaseURLEnv = "CIRCLECI_APP_URL" + defaultAppBaseURL = "https://app.circleci.com" + + handshakeTimeout = 10 * time.Minute + handshakeHTTPTO = 10 * time.Second + // Consecutive network errors tolerated before giving up. + handshakeMaxNetErrs = 3 +) + +// handshakePollWait is the delay between polls. It's a var so tests can +// shorten it; production code should treat it as constant. +var handshakePollWait = 3 * time.Second + type signupOptions struct { cfg *settings.Config noBrowser bool @@ -49,7 +60,7 @@ func newSignupCommand(config *settings.Config) *cobra.Command { }, } - cmd.Flags().BoolVar(&opts.noBrowser, "no-browser", false, "Don't open a browser; print the signup URL and prompt for a token instead") + cmd.Flags().BoolVar(&opts.noBrowser, "no-browser", false, "Don't open a browser — print the signup URL so you can visit it from any device") cmd.Flags().BoolVar(&opts.force, "force", false, "Run signup even if already authenticated") return cmd @@ -70,6 +81,13 @@ func createSignupEvent(noBrowser bool, err error) telemetry.Event { } } +func appBaseURL() string { + if v := os.Getenv(appBaseURLEnv); v != "" { + return v + } + return defaultAppBaseURL +} + func runSignup(cmd *cobra.Command, opts signupOptions) error { if !opts.force && opts.cfg.Token != "" { fmt.Println("You're already authenticated. Your CLI is configured with a personal API token.") @@ -77,211 +95,142 @@ func runSignup(cmd *cobra.Command, opts signupOptions) error { return nil } - state, err := generateState() - if err != nil { - return errors.Wrap(err, "failed to generate cryptographic state") - } + ctx, cancel := context.WithCancel(cmd.Context()) + defer cancel() - if opts.noBrowser { - return signupNoBrowser(opts, state) - } + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, os.Interrupt) + defer signal.Stop(sigCh) + go func() { + select { + case <-sigCh: + cancel() + case <-ctx.Done(): + } + }() - return signupWithBrowser(cmd, opts, state) -} + handshakeID := uuid.NewString() + baseURL := appBaseURL() + signupURL := fmt.Sprintf("%s/cli-auth?handshake_id=%s", baseURL, handshakeID) -func generateState() (string, error) { - b := make([]byte, 16) - if _, err := rand.Read(b); err != nil { - return "", err + if opts.noBrowser { + fmt.Printf("To complete signup, open this URL on any device:\n\n %s\n\n", signupURL) + } else { + trackSignupStep(cmd, "browser_opening", nil) + fmt.Println("Opening your browser to sign up for CircleCI...") + fmt.Printf(" %s\n", signupURL) + if err := browser.OpenURL(signupURL); err != nil { + fmt.Printf("Could not open browser automatically: %v\n", err) + fmt.Println("Please visit the URL above from any device.") + } } - return hex.EncodeToString(b), nil -} -func signupNoBrowser(opts signupOptions, state string) error { - signupURL := "https://app.circleci.com/authentication/login?f=gho&return-to=/settings/user/tokens" - fmt.Printf("Open this URL in your browser to sign up:\n\n %s\n\n", signupURL) + fmt.Println("Waiting for browser authentication...") - token, err := prompt.ReadSecretStringFromUser("Paste your CircleCI API token here") + token, err := pollHandshake(ctx, baseURL, handshakeID, handshakeTimeout) if err != nil { - return errors.Wrap(err, "failed to read token") - } - - if token == "" { - return errors.New("no token provided") + if ctx.Err() != nil { + trackSignupStep(cmd, "canceled", nil) + fmt.Println("\nAuthentication canceled.") + return nil + } + trackSignupStep(cmd, "failed", nil) + return fmt.Errorf("signup failed: %w", err) } + trackSignupStep(cmd, "token_received", nil) return saveToken(opts.cfg, token) } -func signupWithBrowser(cmd *cobra.Command, opts signupOptions, state string) error { - // Start an ephemeral HTTP server on a random available port. - listener, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - return errors.Wrap(err, "failed to start local server") - } - port := listener.Addr().(*net.TCPAddr).Port - - tokenCh := make(chan string, 1) - errCh := make(chan error, 1) - - mux := http.NewServeMux() - mux.HandleFunc("/token", corsMiddleware(handleToken(state, tokenCh, errCh))) +// pollHandshake polls the server-side handshake endpoint until a token appears +// (200), the handshake expires (404), the context is cancelled, or the overall +// timeout elapses. 202 responses mean "still pending"; transient network errors +// are retried up to handshakeMaxNetErrs consecutive times. +func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout time.Duration) (string, error) { + client := &http.Client{Timeout: handshakeHTTPTO} + endpoint := fmt.Sprintf("%s/api/v1/cli-handshake/%s", baseURL, handshakeID) + + deadline := time.NewTimer(timeout) + defer deadline.Stop() + + var netErrs int + for { + token, status, err := handshakePoll(ctx, client, endpoint) + switch { + case err == nil && status == http.StatusOK: + return token, nil + case err == nil && status == http.StatusAccepted: + netErrs = 0 + case err == nil && status == http.StatusNotFound: + return "", errors.New("authentication expired or invalid handshake — run `circleci signup` to try again") + case err == nil: + return "", fmt.Errorf("unexpected response from handshake endpoint: %d", status) + case ctx.Err() != nil: + // Parent context was canceled or hit its deadline — surface it so + // the caller can distinguish from transport-level timeouts. + return "", ctx.Err() + default: + netErrs++ + if netErrs > handshakeMaxNetErrs { + return "", fmt.Errorf("repeated network errors while polling for authentication: %w", err) + } + } - server := &http.Server{ - Handler: mux, - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, - } + fmt.Print(".") - go func() { - if serveErr := server.Serve(listener); serveErr != nil && serveErr != http.ErrServerClosed { - errCh <- serveErr + select { + case <-ctx.Done(): + return "", ctx.Err() + case <-deadline.C: + return "", fmt.Errorf("timed out waiting for browser authentication (%s) — run `circleci signup` to try again", timeout) + case <-time.After(handshakePollWait): } - }() + } +} - // Generate a unique PAT label to avoid 422 duplicate errors when the - // user runs signup multiple times or from different machines. - hostname, err := os.Hostname() +// handshakePoll performs a single GET against the handshake endpoint. +// On 200 it decodes and returns the token; on any other status it returns the +// status code for the caller to dispatch on. Network / transport errors surface +// via the error return so the caller can decide whether to retry. +func handshakePoll(ctx context.Context, client *http.Client, endpoint string) (string, int, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { - hostname = "unknown" + return "", 0, err } - label := fmt.Sprintf("circleci-cli-%s-%d", sanitizeHostname(hostname), time.Now().Unix()) - - // Build the signup URL. Go directly to /cli-auth (Magic Path). - // The frontend checks for an existing session: if authenticated, it creates - // the PAT immediately; if not, it redirects to signup first. - params := url.Values{} - params.Set("cli_port", fmt.Sprintf("%d", port)) - params.Set("cli_state", state) - params.Set("cli_label", label) - signupURL := "https://app.circleci.com/cli-auth?" + params.Encode() - - trackSignupStep(cmd, "browser_opening", nil) - fmt.Println("Opening your browser to sign up for CircleCI...") - decodedURL, _ := url.QueryUnescape(signupURL) - fmt.Printf(" %s\n", decodedURL) - - if err := browser.OpenURL(signupURL); err != nil { - fmt.Printf("⚠️ Could not open browser automatically: %v\n", err) - fmt.Printf(" Please manually visit: %s\n", decodedURL) - } - - fmt.Println("Waiting for authentication...") + req.Header.Set("Accept", "application/json") - // Wait for the token or an error, with a timeout. - select { - case token := <-tokenCh: - _ = server.Shutdown(context.Background()) - trackSignupStep(cmd, "token_received", nil) - return saveToken(opts.cfg, token) - case err := <-errCh: - _ = server.Shutdown(context.Background()) - trackSignupStep(cmd, "failed", nil) - return errors.Wrap(err, "signup failed") - case <-time.After(5 * time.Minute): - _ = server.Shutdown(context.Background()) - trackSignupStep(cmd, "timeout", nil) - return errors.New("timed out waiting for signup to complete. Run `circleci setup` to manually configure your CLI with a personal API token") + resp, err := client.Do(req) + if err != nil { + return "", 0, err } -} - -const allowedOrigin = "https://app.circleci.com" - -// corsMiddleware validates the Origin header and adds CORS headers allowing -// the CircleCI frontend to make cross-origin requests to the CLI's local server. -// Requests with missing or non-matching Origin are rejected with 403. -func corsMiddleware(next http.HandlerFunc) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - origin := r.Header.Get("Origin") - if origin != allowedOrigin { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } - - w.Header().Set("Access-Control-Allow-Origin", allowedOrigin) - w.Header().Set("Access-Control-Allow-Methods", "GET") - w.Header().Set("Access-Control-Allow-Headers", "Content-Type") - w.Header().Set("Access-Control-Allow-Private-Network", "true") - w.Header().Set("Access-Control-Max-Age", "300") + defer resp.Body.Close() - if r.Method == http.MethodOptions { - w.WriteHeader(http.StatusNoContent) - return - } - - next(w, r) + if resp.StatusCode != http.StatusOK { + return "", resp.StatusCode, nil } -} -func stateMatches(a, b string) bool { - return subtle.ConstantTimeCompare([]byte(a), []byte(b)) == 1 -} - -func handleToken(expectedState string, tokenCh chan<- string, errCh chan<- error) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodGet { - http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) - return - } - - query := r.URL.Query() - token := query.Get("token") - state := query.Get("cli_state") - callbackErr := query.Get("error") - - // When an error is present, state validation is best-effort: if state - // is provided it must match, but a missing state is tolerated because - // the frontend may not have had access to it when the failure occurred. - if callbackErr != "" { - if state != "" && !stateMatches(state, expectedState) { - http.Error(w, "Invalid state", http.StatusForbidden) - errCh <- errors.New("state mismatch — possible CSRF attempt") - return - } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{"status": "error"}) - errCh <- fmt.Errorf("authentication failed (%s). Run `circleci setup` to manually configure your CLI with a personal API token", callbackErr) - return - } - - if !stateMatches(state, expectedState) { - http.Error(w, "Invalid state", http.StatusForbidden) - errCh <- errors.New("state mismatch — possible CSRF attempt") - return - } - - if token == "" { - http.Error(w, "Missing token", http.StatusBadRequest) - errCh <- errors.New("callback returned an empty token. Run `circleci setup` to manually configure your CLI with a personal API token") - return - } - - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) - tokenCh <- token + var body struct { + Token string `json:"token"` + CreatedAt string `json:"created_at"` } -} - -func sanitizeHostname(h string) string { - var b strings.Builder - for _, r := range h { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' { - b.WriteRune(r) - } + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + return "", resp.StatusCode, fmt.Errorf("failed to parse handshake response: %w", err) } - s := b.String() - if s == "" { - return "unknown" + if body.Token == "" { + return "", resp.StatusCode, errors.New("handshake response contained no token") } - return s + return body.Token, resp.StatusCode, nil } func saveToken(cfg *settings.Config, token string) error { cfg.Token = token if err := cfg.WriteToDisk(); err != nil { - return errors.Wrap(err, "failed to save token to config") + return fmt.Errorf("failed to save token to config: %w", err) } fmt.Println("\n✅ Welcome to CircleCI! Your CLI is now authenticated.") + fmt.Println("\nNext steps:") + fmt.Println(" circleci init — set up a project in the current directory") + fmt.Println(" circleci help — see all available commands") return nil } diff --git a/cmd/signup_unit_test.go b/cmd/signup_unit_test.go index c429942f8..12df03e56 100644 --- a/cmd/signup_unit_test.go +++ b/cmd/signup_unit_test.go @@ -1,12 +1,13 @@ package cmd import ( + "context" "fmt" "io" "net/http" "net/http/httptest" - "net/url" "os" + "sync/atomic" "time" . "github.com/onsi/ginkgo" @@ -51,245 +52,104 @@ var _ = Describe("Signup", func() { }) }) - Describe("generateState", func() { - It("should return a 32-character hex string", func() { - state, err := generateState() - Expect(err).ShouldNot(HaveOccurred()) - Expect(state).To(HaveLen(32)) - Expect(state).To(MatchRegexp(`^[0-9a-f]{32}$`)) - }) - - It("should generate unique values", func() { - a, _ := generateState() - b, _ := generateState() - Expect(a).ToNot(Equal(b)) - }) - }) - - Describe("corsMiddleware", func() { - var dummyHandler http.HandlerFunc - var handlerCalled bool - - BeforeEach(func() { - handlerCalled = false - dummyHandler = func(w http.ResponseWriter, r *http.Request) { - handlerCalled = true - w.WriteHeader(http.StatusOK) - fmt.Fprint(w, "ok") - } - }) - - It("should set CORS headers on a GET request with valid origin", func() { - wrapped := corsMiddleware(dummyHandler) - req := httptest.NewRequest("GET", "/token", nil) - req.Header.Set("Origin", "https://app.circleci.com") - rec := httptest.NewRecorder() - - wrapped.ServeHTTP(rec, req) - - Expect(rec.Header().Get("Access-Control-Allow-Origin")).To(Equal("https://app.circleci.com")) - Expect(rec.Header().Get("Access-Control-Allow-Methods")).To(Equal("GET")) - Expect(rec.Header().Get("Access-Control-Allow-Headers")).To(Equal("Content-Type")) - Expect(rec.Header().Get("Access-Control-Allow-Private-Network")).To(Equal("true")) - Expect(rec.Header().Get("Access-Control-Max-Age")).To(Equal("300")) - Expect(handlerCalled).To(BeTrue()) - Expect(rec.Code).To(Equal(http.StatusOK)) - }) - - It("should return 204 on OPTIONS preflight without calling the handler", func() { - wrapped := corsMiddleware(dummyHandler) - req := httptest.NewRequest("OPTIONS", "/token", nil) - req.Header.Set("Origin", "https://app.circleci.com") - rec := httptest.NewRecorder() - - wrapped.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusNoContent)) - Expect(rec.Header().Get("Access-Control-Allow-Origin")).To(Equal("https://app.circleci.com")) - Expect(rec.Header().Get("Access-Control-Allow-Private-Network")).To(Equal("true")) - Expect(handlerCalled).To(BeFalse()) + Describe("appBaseURL", func() { + AfterEach(func() { + os.Unsetenv(appBaseURLEnv) }) - It("should reject requests with wrong origin", func() { - wrapped := corsMiddleware(dummyHandler) - req := httptest.NewRequest("GET", "/token", nil) - req.Header.Set("Origin", "https://evil.com") - rec := httptest.NewRecorder() - - wrapped.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusForbidden)) - Expect(handlerCalled).To(BeFalse()) + It("returns the default when the env var is unset", func() { + os.Unsetenv(appBaseURLEnv) + Expect(appBaseURL()).To(Equal(defaultAppBaseURL)) }) - It("should reject requests with missing origin", func() { - wrapped := corsMiddleware(dummyHandler) - req := httptest.NewRequest("GET", "/token", nil) - rec := httptest.NewRecorder() - - wrapped.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusForbidden)) - Expect(handlerCalled).To(BeFalse()) + It("honors the CIRCLECI_APP_URL override", func() { + os.Setenv(appBaseURLEnv, "https://enterprise.example.com") + Expect(appBaseURL()).To(Equal("https://enterprise.example.com")) }) }) - Describe("unique PAT label", func() { - It("should generate a cli_label containing hostname and timestamp", func() { - hostname, _ := os.Hostname() - label := fmt.Sprintf("circleci-cli-%s-%d", hostname, time.Now().Unix()) - - Expect(label).To(ContainSubstring("circleci-cli-")) - Expect(label).To(ContainSubstring(hostname)) - Expect(label).To(MatchRegexp(`-\d+$`)) - }) - }) - - Describe("Magic Path URL", func() { - It("should open browser directly to /cli-auth, not /authentication/", func() { - params := url.Values{} - params.Set("cli_port", "12345") - params.Set("cli_state", "abc123") - params.Set("cli_label", "circleci-cli-test-1234") - signupURL := "https://app.circleci.com/cli-auth?" + params.Encode() - - Expect(signupURL).ToNot(ContainSubstring("/authentication/")) - Expect(signupURL).ToNot(ContainSubstring("/successful-signup")) - Expect(signupURL).To(HavePrefix("https://app.circleci.com/cli-auth?")) - Expect(signupURL).To(ContainSubstring("cli_port=12345")) - Expect(signupURL).To(ContainSubstring("cli_state=abc123")) - Expect(signupURL).To(ContainSubstring("cli_label=circleci-cli-test-1234")) - }) - }) - - Describe("handleToken", func() { - var ( - tokenCh chan string - errCh chan error - state string - ) - - BeforeEach(func() { - tokenCh = make(chan string, 1) - errCh = make(chan error, 1) - state = "abc123" - }) - - It("should accept a valid token and cli_state with JSON response", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?token=mytoken&cli_state=abc123", nil) - rec := httptest.NewRecorder() - - handler.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusOK)) - Expect(rec.Header().Get("Content-Type")).To(Equal("application/json")) - Expect(rec.Body.String()).To(ContainSubstring(`"status":"ok"`)) - Eventually(tokenCh).Should(Receive(Equal("mytoken"))) - }) - - It("should reject a cli_state mismatch with 403", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?token=mytoken&cli_state=wrong", nil) - rec := httptest.NewRecorder() - - handler.ServeHTTP(rec, req) + Describe("pollHandshake", func() { + It("returns the token once the backend responds with 200", func() { + var calls int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&calls, 1) + Expect(r.Method).To(Equal(http.MethodGet)) + Expect(r.URL.Path).To(Equal("/api/v1/cli-handshake/abc-123")) + if n < 2 { + w.WriteHeader(http.StatusAccepted) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"token":"pat-xyz","created_at":"2026-04-16T12:00:00Z"}`) + })) + defer server.Close() - Expect(rec.Code).To(Equal(http.StatusForbidden)) - Expect(rec.Body.String()).To(ContainSubstring("Invalid state")) - Eventually(errCh).Should(Receive()) + token, err := pollHandshakeFast(context.Background(), server.URL, "abc-123", time.Minute) + Expect(err).ShouldNot(HaveOccurred()) + Expect(token).To(Equal("pat-xyz")) + Expect(atomic.LoadInt32(&calls)).To(BeNumerically(">=", 2)) }) - It("should reject non-GET methods", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("POST", "/token?token=mytoken&cli_state=abc123", nil) - rec := httptest.NewRecorder() - - handler.ServeHTTP(rec, req) + It("returns an expired error on 404", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() - Expect(rec.Code).To(Equal(http.StatusMethodNotAllowed)) + _, err := pollHandshakeFast(context.Background(), server.URL, "missing", time.Minute) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("expired")) + Expect(err.Error()).To(ContainSubstring("circleci signup")) }) - It("should reject a missing token when no error param is present", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?cli_state=abc123", nil) - rec := httptest.NewRecorder() + It("fails on unexpected status codes", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() - handler.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusBadRequest)) - Expect(rec.Body.String()).To(ContainSubstring("Missing token")) - var received error - Eventually(errCh).Should(Receive(&received)) - Expect(received.Error()).To(ContainSubstring("circleci setup")) + _, err := pollHandshakeFast(context.Background(), server.URL, "boom", time.Minute) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("unexpected response")) }) - Context("with error param from frontend", func() { - It("should forward the error when cli_state matches", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?error=token_creation_failed&cli_state=abc123", nil) - rec := httptest.NewRecorder() - - handler.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusOK)) - Expect(rec.Header().Get("Content-Type")).To(Equal("application/json")) - Expect(rec.Body.String()).To(ContainSubstring(`"status":"error"`)) - var received error - Eventually(errCh).Should(Receive(&received)) - Expect(received.Error()).To(ContainSubstring("authentication failed")) - Expect(received.Error()).To(ContainSubstring("token_creation_failed")) - Expect(received.Error()).To(ContainSubstring("circleci setup")) - }) - - It("should tolerate a missing cli_state when error is present", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?error=no_token", nil) - rec := httptest.NewRecorder() - - handler.ServeHTTP(rec, req) - - Expect(rec.Code).To(Equal(http.StatusOK)) - var received error - Eventually(errCh).Should(Receive(&received)) - Expect(received.Error()).To(ContainSubstring("no_token")) - }) - - It("should reject error with a mismatched cli_state", func() { - handler := handleToken(state, tokenCh, errCh) - req := httptest.NewRequest("GET", "/token?error=token_creation_failed&cli_state=wrong", nil) - rec := httptest.NewRecorder() + It("surfaces ctx.Err when the context is canceled", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + })) + defer server.Close() - handler.ServeHTTP(rec, req) + ctx, cancel := context.WithCancel(context.Background()) + cancel() - Expect(rec.Code).To(Equal(http.StatusForbidden)) - Expect(rec.Body.String()).To(ContainSubstring("Invalid state")) - Eventually(errCh).Should(Receive()) - }) + _, err := pollHandshakeFast(ctx, server.URL, "id", time.Minute) + Expect(err).To(MatchError(context.Canceled)) }) - }) - Describe("sanitizeHostname", func() { - It("should strip non-alphanumeric characters except hyphens", func() { - Expect(sanitizeHostname("my.local")).To(Equal("myhostlocal")) - Expect(sanitizeHostname("MacBook-Pro")).To(Equal("MacBook-Pro")) - Expect(sanitizeHostname("host name!@#")).To(Equal("hostname")) - Expect(sanitizeHostname("")).To(Equal("unknown")) - Expect(sanitizeHostname("!!!")).To(Equal("unknown")) - }) - }) + It("times out when the backend never completes the handshake", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + })) + defer server.Close() - Describe("stateMatches", func() { - It("should return true for matching states", func() { - Expect(stateMatches("abc123", "abc123")).To(BeTrue()) + _, err := pollHandshakeFast(context.Background(), server.URL, "id", 20*time.Millisecond) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("timed out")) }) - It("should return false for mismatched states", func() { - Expect(stateMatches("abc123", "wrong")).To(BeFalse()) - }) + It("returns an error after repeated network failures", func() { + // Stand up a test server then immediately close it. Subsequent + // requests to the bound port fail fast with "connection refused", + // simulating sustained network errors without the HTTP client's + // 10s timeout kicking in. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + addr := server.URL + server.Close() - It("should return false for empty vs non-empty", func() { - Expect(stateMatches("", "abc123")).To(BeFalse()) + _, err := pollHandshakeFast(context.Background(), addr, "id", time.Minute) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("network")) }) }) @@ -316,6 +176,7 @@ var _ = Describe("Signup", func() { }) Expect(output).To(ContainSubstring("Welcome to CircleCI")) + Expect(output).To(ContainSubstring("Next steps")) Expect(cfg.Token).To(Equal("my-new-token")) // Verify it was persisted to disk @@ -346,3 +207,13 @@ var _ = Describe("Signup", func() { }) }) }) + +// pollHandshakeFast shortens the internal poll interval so the suite doesn't +// sit in 3-second sleeps between requests. The production delay is restored +// after each call. +func pollHandshakeFast(ctx context.Context, baseURL, handshakeID string, timeout time.Duration) (string, error) { + orig := handshakePollWait + handshakePollWait = 5 * time.Millisecond + defer func() { handshakePollWait = orig }() + return pollHandshake(ctx, baseURL, handshakeID, timeout) +} From efca9b92ecb49706cb5cd1dca332287101c77b4b Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Thu, 16 Apr 2026 21:31:35 -0600 Subject: [PATCH 3/7] [WEBXP-747] Drop dead 404 branch from handshake poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auth-svc returns 202 on cache miss, not 404 — the dedicated "authentication expired" branch could never fire in normal operation, and its message was misleading since the server has no 404-for-expiry path. A truly unexpected 404 (e.g. routing misconfiguration) still surfaces via the default "unexpected response: 404" handler, which is honest about what happened without inventing a reason. Timeout is the sole expiry path. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 2 -- cmd/signup_unit_test.go | 12 ------------ 2 files changed, 14 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index 3ab3d15b6..105575a92 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -161,8 +161,6 @@ func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout tim return token, nil case err == nil && status == http.StatusAccepted: netErrs = 0 - case err == nil && status == http.StatusNotFound: - return "", errors.New("authentication expired or invalid handshake — run `circleci signup` to try again") case err == nil: return "", fmt.Errorf("unexpected response from handshake endpoint: %d", status) case ctx.Err() != nil: diff --git a/cmd/signup_unit_test.go b/cmd/signup_unit_test.go index 12df03e56..7d21fd451 100644 --- a/cmd/signup_unit_test.go +++ b/cmd/signup_unit_test.go @@ -91,18 +91,6 @@ var _ = Describe("Signup", func() { Expect(atomic.LoadInt32(&calls)).To(BeNumerically(">=", 2)) }) - It("returns an expired error on 404", func() { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - defer server.Close() - - _, err := pollHandshakeFast(context.Background(), server.URL, "missing", time.Minute) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("expired")) - Expect(err.Error()).To(ContainSubstring("circleci signup")) - }) - It("fails on unexpected status codes", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) From e2c7d139a9f85590763be3c080fbfb7dc2ada54f Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Thu, 16 Apr 2026 21:58:07 -0600 Subject: [PATCH 4/7] fix: remove stale 404 reference from polling comment Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index 105575a92..6a3794053 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -143,9 +143,10 @@ func runSignup(cmd *cobra.Command, opts signupOptions) error { } // pollHandshake polls the server-side handshake endpoint until a token appears -// (200), the handshake expires (404), the context is cancelled, or the overall -// timeout elapses. 202 responses mean "still pending"; transient network errors -// are retried up to handshakeMaxNetErrs consecutive times. +// (200), the context is cancelled, or the overall timeout elapses. The server +// returns 202 for both pending and post-TTL cache-miss cases, so the 10-minute +// deadline is the sole expiry path. Transient network errors are retried up to +// handshakeMaxNetErrs consecutive times. func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout time.Duration) (string, error) { client := &http.Client{Timeout: handshakeHTTPTO} endpoint := fmt.Sprintf("%s/api/v1/cli-handshake/%s", baseURL, handshakeID) From af9ca24a888c846449ddd285c00ceb6e79c40837 Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Thu, 16 Apr 2026 23:52:54 -0600 Subject: [PATCH 5/7] [WEBXP-747] Reuse configured HTTP client and inject poll intervals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three PR-review follow-ups: 1. Reuse cfg.HTTPClient instead of constructing a bare http.Client. The configured client carries the custom CA pool built from cfg.TLSCert plus cfg.TLSInsecure — without it, self-hosted / enterprise users who point CIRCLECI_APP_URL at their own app host would hit TLS verification failures. Per-request deadlines now come from context.WithTimeout(ctx, requestTimeout), so the shared client's Timeout field stays untouched. 2. De-globalize handshakePollWait. The poll interval and per-request timeout are now function parameters on pollHandshake; production passes the package constants, tests pass shortened durations. Removes the var + restore-defer pattern that would have raced under `ginkgo -p` parallel execution. 3. Guard against non-JSON 200 bodies (e.g. Cloudflare / gateway HTML error pages that return 200 on the happy-path URL). handshakePoll now checks Content-Type on a 200 and returns a readable error with a truncated body snippet instead of surfacing the stdlib's "invalid character '<'" JSON error. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 57 +++++++++++++++++++++++++++++------------ cmd/signup_unit_test.go | 46 +++++++++++++++++++-------------- 2 files changed, 67 insertions(+), 36 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index 6a3794053..a6e289813 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -5,9 +5,12 @@ import ( "encoding/json" "errors" "fmt" + "io" + "mime" "net/http" "os" "os/signal" + "strings" "time" "github.com/google/uuid" @@ -24,16 +27,13 @@ const ( appBaseURLEnv = "CIRCLECI_APP_URL" defaultAppBaseURL = "https://app.circleci.com" - handshakeTimeout = 10 * time.Minute - handshakeHTTPTO = 10 * time.Second + handshakeTimeout = 10 * time.Minute + handshakePollWait = 3 * time.Second + handshakeHTTPTO = 10 * time.Second // Consecutive network errors tolerated before giving up. handshakeMaxNetErrs = 3 ) -// handshakePollWait is the delay between polls. It's a var so tests can -// shorten it; production code should treat it as constant. -var handshakePollWait = 3 * time.Second - type signupOptions struct { cfg *settings.Config noBrowser bool @@ -127,7 +127,15 @@ func runSignup(cmd *cobra.Command, opts signupOptions) error { fmt.Println("Waiting for browser authentication...") - token, err := pollHandshake(ctx, baseURL, handshakeID, handshakeTimeout) + // Reuse the configured HTTP client so enterprise installs keep their + // custom CA bundle (cfg.TLSCert) and TLS settings. Per-request deadlines + // are applied via context.WithTimeout so we don't mutate the shared client. + client := opts.cfg.HTTPClient + if client == nil { + client = http.DefaultClient + } + + token, err := pollHandshake(ctx, client, baseURL, handshakeID, handshakeTimeout, handshakePollWait, handshakeHTTPTO) if err != nil { if ctx.Err() != nil { trackSignupStep(cmd, "canceled", nil) @@ -144,11 +152,12 @@ func runSignup(cmd *cobra.Command, opts signupOptions) error { // pollHandshake polls the server-side handshake endpoint until a token appears // (200), the context is cancelled, or the overall timeout elapses. The server -// returns 202 for both pending and post-TTL cache-miss cases, so the 10-minute -// deadline is the sole expiry path. Transient network errors are retried up to -// handshakeMaxNetErrs consecutive times. -func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout time.Duration) (string, error) { - client := &http.Client{Timeout: handshakeHTTPTO} +// returns 202 for both pending and post-TTL cache-miss cases, so the timeout +// is the sole expiry path. Transient network errors are retried up to +// handshakeMaxNetErrs consecutive times. pollWait and requestTimeout are +// passed in so tests can drive the loop deterministically without touching +// package-level state. +func pollHandshake(ctx context.Context, client *http.Client, baseURL, handshakeID string, timeout, pollWait, requestTimeout time.Duration) (string, error) { endpoint := fmt.Sprintf("%s/api/v1/cli-handshake/%s", baseURL, handshakeID) deadline := time.NewTimer(timeout) @@ -156,7 +165,7 @@ func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout tim var netErrs int for { - token, status, err := handshakePoll(ctx, client, endpoint) + token, status, err := handshakePoll(ctx, client, endpoint, requestTimeout) switch { case err == nil && status == http.StatusOK: return token, nil @@ -182,7 +191,7 @@ func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout tim return "", ctx.Err() case <-deadline.C: return "", fmt.Errorf("timed out waiting for browser authentication (%s) — run `circleci signup` to try again", timeout) - case <-time.After(handshakePollWait): + case <-time.After(pollWait): } } } @@ -190,9 +199,14 @@ func pollHandshake(ctx context.Context, baseURL, handshakeID string, timeout tim // handshakePoll performs a single GET against the handshake endpoint. // On 200 it decodes and returns the token; on any other status it returns the // status code for the caller to dispatch on. Network / transport errors surface -// via the error return so the caller can decide whether to retry. -func handshakePoll(ctx context.Context, client *http.Client, endpoint string) (string, int, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) +// via the error return so the caller can decide whether to retry. The +// per-request deadline comes from a derived context so the shared HTTP client +// doesn't need its Timeout field mutated. +func handshakePoll(ctx context.Context, client *http.Client, endpoint string, requestTimeout time.Duration) (string, int, error) { + reqCtx, cancel := context.WithTimeout(ctx, requestTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, endpoint, nil) if err != nil { return "", 0, err } @@ -208,6 +222,15 @@ func handshakePoll(ctx context.Context, client *http.Client, endpoint string) (s return "", resp.StatusCode, nil } + // Guard against proxies or misrouted requests returning a 200 with a + // non-JSON body (e.g. Cloudflare HTML error pages on the happy-path URL). + // Surface a readable error with a short body snippet so users aren't + // debugging from `invalid character '<'`. + if mt, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type")); mt != "" && mt != "application/json" { + snippet, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) + return "", resp.StatusCode, fmt.Errorf("handshake returned non-JSON response (content-type %q): %s", mt, strings.TrimSpace(string(snippet))) + } + var body struct { Token string `json:"token"` CreatedAt string `json:"created_at"` diff --git a/cmd/signup_unit_test.go b/cmd/signup_unit_test.go index 7d21fd451..b6332457c 100644 --- a/cmd/signup_unit_test.go +++ b/cmd/signup_unit_test.go @@ -17,6 +17,13 @@ import ( "github.com/CircleCI-Public/circleci-cli/settings" ) +// testPollWait keeps specs snappy without reaching into package-level state. +const testPollWait = 5 * time.Millisecond + +// testRequestTO is generous enough that httptest servers always reply inside +// it, but short enough that the "unreachable server" spec fails fast. +const testRequestTO = 500 * time.Millisecond + var _ = Describe("Signup", func() { Describe("already authenticated guard", func() { It("should print already authenticated when token exists", func() { @@ -85,7 +92,7 @@ var _ = Describe("Signup", func() { })) defer server.Close() - token, err := pollHandshakeFast(context.Background(), server.URL, "abc-123", time.Minute) + token, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "abc-123", time.Minute, testPollWait, testRequestTO) Expect(err).ShouldNot(HaveOccurred()) Expect(token).To(Equal("pat-xyz")) Expect(atomic.LoadInt32(&calls)).To(BeNumerically(">=", 2)) @@ -97,7 +104,7 @@ var _ = Describe("Signup", func() { })) defer server.Close() - _, err := pollHandshakeFast(context.Background(), server.URL, "boom", time.Minute) + _, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "boom", time.Minute, testPollWait, testRequestTO) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("unexpected response")) }) @@ -111,7 +118,7 @@ var _ = Describe("Signup", func() { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := pollHandshakeFast(ctx, server.URL, "id", time.Minute) + _, err := pollHandshake(ctx, http.DefaultClient, server.URL, "id", time.Minute, testPollWait, testRequestTO) Expect(err).To(MatchError(context.Canceled)) }) @@ -121,24 +128,35 @@ var _ = Describe("Signup", func() { })) defer server.Close() - _, err := pollHandshakeFast(context.Background(), server.URL, "id", 20*time.Millisecond) + _, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "id", 20*time.Millisecond, testPollWait, testRequestTO) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("timed out")) }) It("returns an error after repeated network failures", func() { - // Stand up a test server then immediately close it. Subsequent - // requests to the bound port fail fast with "connection refused", - // simulating sustained network errors without the HTTP client's - // 10s timeout kicking in. server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) addr := server.URL server.Close() - _, err := pollHandshakeFast(context.Background(), addr, "id", time.Minute) + _, err := pollHandshake(context.Background(), http.DefaultClient, addr, "id", time.Minute, testPollWait, testRequestTO) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("network")) }) + + It("surfaces a readable error when a 200 response carries a non-JSON body", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "502 Bad Gateway") + })) + defer server.Close() + + _, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "id", time.Minute, testPollWait, testRequestTO) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("non-JSON")) + Expect(err.Error()).To(ContainSubstring("text/html")) + Expect(err.Error()).To(ContainSubstring("502 Bad Gateway")) + }) }) Describe("saveToken", func() { @@ -195,13 +213,3 @@ var _ = Describe("Signup", func() { }) }) }) - -// pollHandshakeFast shortens the internal poll interval so the suite doesn't -// sit in 3-second sleeps between requests. The production delay is restored -// after each call. -func pollHandshakeFast(ctx context.Context, baseURL, handshakeID string, timeout time.Duration) (string, error) { - orig := handshakePollWait - handshakePollWait = 5 * time.Millisecond - defer func() { handshakePollWait = orig }() - return pollHandshake(ctx, baseURL, handshakeID, timeout) -} From 4c2c77d63ba1ae7f25fcd755bcd41abb5f794530 Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Fri, 17 Apr 2026 00:17:50 -0600 Subject: [PATCH 6/7] [WEBXP-747] Document auth-svc TTL coupling on handshakeTimeout The CLI's 10-minute poll deadline is coupled to auth-svc's handshake cache TTL (CLI_HANDSHAKE_TTL, default 10m, authentication-service PR #1711). Document the constraint so a future change to either value can't silently regress the laptop-sleep / Docker-pause case where the server evicts the entry before the CLI finishes polling. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index a6e289813..ac44a8f26 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -27,11 +27,17 @@ const ( appBaseURLEnv = "CIRCLECI_APP_URL" defaultAppBaseURL = "https://app.circleci.com" - handshakeTimeout = 10 * time.Minute - handshakePollWait = 3 * time.Second - handshakeHTTPTO = 10 * time.Second - // Consecutive network errors tolerated before giving up. - handshakeMaxNetErrs = 3 + // handshakeTimeout is the overall CLI poll deadline. It must stay ≤ the + // auth-svc handshake cache TTL (CLI_HANDSHAKE_TTL, default 10m in + // authentication-service/clihandshake/cache.go) — otherwise the CLI will + // keep polling IDs the server has already evicted, and users whose CLI + // was suspended (laptop sleep, Docker pause, etc.) see a confusing + // "timed out" even though auth completed server-side. If you change + // this value, check the auth-svc default and the WEBXP-745 runbook. + handshakeTimeout = 10 * time.Minute + handshakePollWait = 3 * time.Second + handshakeHTTPTO = 10 * time.Second + handshakeMaxNetErrs = 3 // consecutive network errors tolerated ) type signupOptions struct { From ee650f3b235fc02cc653898eb5eb214680e13116 Mon Sep 17 00:00:00 2001 From: fabio ramirez Date: Fri, 17 Apr 2026 09:38:26 -0600 Subject: [PATCH 7/7] [WEBXP-747] Decouple client timeout from server TTL; retry 429/5xx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes following the auth-svc decision to revert the Redis handshake TTL from 10 min back to 60s (authentication-service PR #1711 commit ddfcd807): 1. Rewrite the handshakeTimeout inline comment. The prior version asserted that the CLI timeout must stay ≤ the auth-svc TTL — that invariant no longer holds and never really did. The only structural requirement is that the server TTL exceed handshakePollWait; the client-side timeout is an independent UX knob. 2. Treat 429 Too Many Requests and 5xx responses as transient rather than hard-failing on the default "unexpected response" branch. Rate limiting is expected once WEBXP-751 lands Gubernator on the unauthenticated GET, and 5xx covers backend blips. They share the existing handshakeMaxNetErrs budget (3 consecutive) and the counter still resets on any 202. Two new specs cover the retry-then-succeed and sustained-429 paths; the existing "unexpected status" spec is switched from 500 → 418 now that 5xx is transient. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/signup.go | 31 +++++++++++++++++++++++-------- cmd/signup_unit_test.go | 39 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/cmd/signup.go b/cmd/signup.go index ac44a8f26..b14c4986c 100644 --- a/cmd/signup.go +++ b/cmd/signup.go @@ -27,17 +27,16 @@ const ( appBaseURLEnv = "CIRCLECI_APP_URL" defaultAppBaseURL = "https://app.circleci.com" - // handshakeTimeout is the overall CLI poll deadline. It must stay ≤ the - // auth-svc handshake cache TTL (CLI_HANDSHAKE_TTL, default 10m in - // authentication-service/clihandshake/cache.go) — otherwise the CLI will - // keep polling IDs the server has already evicted, and users whose CLI - // was suspended (laptop sleep, Docker pause, etc.) see a confusing - // "timed out" even though auth completed server-side. If you change - // this value, check the auth-svc default and the WEBXP-745 runbook. + // handshakeTimeout bounds how long the CLI waits for the browser-side + // authentication to complete before giving up. It's a pure UX knob — + // the auth-svc Redis TTL (sized to cover the POST→next-poll window, + // currently 60s) is an independent server-side concern. The only + // structural requirement is that the server TTL comfortably exceed + // handshakePollWait; the client timeout is decoupled from it. handshakeTimeout = 10 * time.Minute handshakePollWait = 3 * time.Second handshakeHTTPTO = 10 * time.Second - handshakeMaxNetErrs = 3 // consecutive network errors tolerated + handshakeMaxNetErrs = 3 // consecutive transient errors tolerated ) type signupOptions struct { @@ -177,6 +176,16 @@ func pollHandshake(ctx context.Context, client *http.Client, baseURL, handshakeI return token, nil case err == nil && status == http.StatusAccepted: netErrs = 0 + case err == nil && isTransientStatus(status): + // 429 (rate limit) and 5xx are transient server-side conditions — + // rate limiting is expected once WEBXP-751 lands Gubernator on the + // unauthenticated GET, and 5xx covers backend blips. Count them + // under the same budget as transport errors; a later 202 resets + // the counter. + netErrs++ + if netErrs > handshakeMaxNetErrs { + return "", fmt.Errorf("handshake endpoint returned repeated transient errors (last status %d)", status) + } case err == nil: return "", fmt.Errorf("unexpected response from handshake endpoint: %d", status) case ctx.Err() != nil: @@ -202,6 +211,12 @@ func pollHandshake(ctx context.Context, client *http.Client, baseURL, handshakeI } } +// isTransientStatus reports whether a non-200/non-202 response should be +// retried under the network-error budget rather than failing immediately. +func isTransientStatus(status int) bool { + return status == http.StatusTooManyRequests || (status >= 500 && status <= 599) +} + // handshakePoll performs a single GET against the handshake endpoint. // On 200 it decodes and returns the token; on any other status it returns the // status code for the caller to dispatch on. Network / transport errors surface diff --git a/cmd/signup_unit_test.go b/cmd/signup_unit_test.go index b6332457c..960511dae 100644 --- a/cmd/signup_unit_test.go +++ b/cmd/signup_unit_test.go @@ -98,9 +98,9 @@ var _ = Describe("Signup", func() { Expect(atomic.LoadInt32(&calls)).To(BeNumerically(">=", 2)) }) - It("fails on unexpected status codes", func() { + It("fails on truly unexpected status codes", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) + w.WriteHeader(http.StatusTeapot) })) defer server.Close() @@ -109,6 +109,41 @@ var _ = Describe("Signup", func() { Expect(err.Error()).To(ContainSubstring("unexpected response")) }) + It("retries transient 429 / 5xx responses under the network-error budget", func() { + var calls int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&calls, 1) + switch n { + case 1: + w.WriteHeader(http.StatusTooManyRequests) + case 2: + w.WriteHeader(http.StatusServiceUnavailable) + default: + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"token":"pat-xyz"}`) + } + })) + defer server.Close() + + token, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "id", time.Minute, testPollWait, testRequestTO) + Expect(err).ShouldNot(HaveOccurred()) + Expect(token).To(Equal("pat-xyz")) + Expect(atomic.LoadInt32(&calls)).To(Equal(int32(3))) + }) + + It("gives up after sustained 429 responses exceed the budget", func() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + _, err := pollHandshake(context.Background(), http.DefaultClient, server.URL, "id", time.Minute, testPollWait, testRequestTO) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("transient")) + Expect(err.Error()).To(ContainSubstring("429")) + }) + It("surfaces ctx.Err when the context is canceled", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusAccepted)