From d7e3ccb99f9c6b27ccd4f45fc59df6c7f30cac04 Mon Sep 17 00:00:00 2001 From: James Ridey Date: Wed, 23 Sep 2026 16:10:24 +1000 Subject: [PATCH] fix(auth): [FDE-1873] redeem browser login with callback secret --- internal/auth/login.go | 69 +++----------------- internal/auth/login_callback.go | 49 ++++++++++++++ internal/auth/login_callback_test.go | 98 ++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 61 deletions(-) create mode 100644 internal/auth/login_callback.go create mode 100644 internal/auth/login_callback_test.go diff --git a/internal/auth/login.go b/internal/auth/login.go index aadcdbb..1d6cfb0 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -11,7 +11,6 @@ import ( "os/exec" "runtime" "strings" - "sync" "time" "github.com/nullify-platform/cli/internal/logger" @@ -36,66 +35,23 @@ type cliTokenResponse struct { QueryParameters map[string]string `json:"query_parameters,omitempty"` } -const successHTML = ` -Nullify CLI - -
- -

Authenticated Successfully!

-

You can close this tab and return to your terminal.

-
` - func Login(ctx context.Context, host string) error { - // 1. Start localhost server on random port listener, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return fmt.Errorf("failed to start local server: %w", err) } port := listener.Addr().(*net.TCPAddr).Port - // 2. Create session on backend first so we know the expected session ID sessionResp, err := createCLISession(ctx, host, port) if err != nil { listener.Close() return fmt.Errorf("failed to create auth session: %w", err) } - // 3. Set up callback handler that validates session ID - sessionCh := make(chan string, 1) + sessionCh := make(chan cliCallback, 1) errCh := make(chan error, 1) - var callbackOnce sync.Once - mux := http.NewServeMux() - mux.HandleFunc("/callback", func(w http.ResponseWriter, r *http.Request) { - receivedID := r.URL.Query().Get("session_id") - - // Verify the session ID matches what we requested (CSRF protection) - if receivedID != sessionResp.SessionID { - http.Error(w, "invalid session", http.StatusForbidden) - return - } - - // Only process the first valid callback (guard against duplicates) - processed := false - callbackOnce.Do(func() { - processed = true - w.Header().Set("Content-Type", "text/html; charset=utf-8") - w.WriteHeader(http.StatusOK) - fmt.Fprint(w, successHTML) - sessionCh <- receivedID - }) - if !processed { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - w.WriteHeader(http.StatusOK) - fmt.Fprint(w, successHTML) - } - }) + mux.Handle("/callback", newLoginCallbackHandler(sessionResp.SessionID, sessionCh)) server := &http.Server{Handler: mux} go func() { @@ -105,7 +61,6 @@ func Login(ctx context.Context, host string) error { }() defer server.Close() - // 4. Open browser fmt.Printf("\nOpening browser to authenticate...\n") fmt.Printf("If the browser doesn't open, visit:\n %s\n\n", sessionResp.AuthURL) @@ -116,17 +71,16 @@ func Login(ctx context.Context, host string) error { fmt.Println("Waiting for authentication... (press Ctrl+C to cancel)") - // 5. Wait for callback with context cancellation support ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() timeout := time.After(10 * time.Minute) - var sessionID string + var callback cliCallback waitLoop: for { select { - case sessionID = <-sessionCh: + case callback = <-sessionCh: break waitLoop case err := <-errCh: return fmt.Errorf("local server error: %w", err) @@ -139,8 +93,7 @@ waitLoop: } } - // 6. Fetch tokens from backend - tokenResp, err := fetchCLIToken(ctx, host, sessionID) + tokenResp, err := fetchCLIToken(ctx, host, callback.SessionID, callback.RedemptionSecret) if err != nil { return fmt.Errorf("failed to fetch tokens: %w", err) } @@ -149,7 +102,6 @@ waitLoop: return fmt.Errorf("authentication failed: %s", tokenResp.Error) } - // 7. Store credentials expiresAt := time.Now().Add(time.Duration(tokenResp.ExpiresIn) * time.Second).Unix() err = SaveHostCredentials(host, HostCredentials{ @@ -187,7 +139,6 @@ func GetValidToken(ctx context.Context, host string) (string, error) { return "", fmt.Errorf("not authenticated for %s - run 'nullify auth login --host %s'", host, host) } - // Check if token is expired and refresh if needed if hostCreds.ExpiresAt > 0 && time.Now().Unix() > hostCreds.ExpiresAt { if hostCreds.RefreshToken != "" { logger.L(ctx).Debug("access token expired, attempting refresh") @@ -264,10 +215,10 @@ func createCLISession(ctx context.Context, host string, port int) (*cliSessionRe return &sessionResp, nil } -func fetchCLIToken(ctx context.Context, host string, sessionID string) (*cliTokenResponse, error) { +func fetchCLIToken(ctx context.Context, host string, sessionID string, redemptionSecret string) (*cliTokenResponse, error) { url := authBaseURL(host) + "/auth/cli/token" - bodyData, err := json.Marshal(map[string]string{"session_id": sessionID}) + bodyData, err := json.Marshal(cliCallback{SessionID: sessionID, RedemptionSecret: redemptionSecret}) if err != nil { return nil, err } @@ -328,11 +279,7 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, return "", err } - // GET /auth/refresh_token carries no token in its JSON body -- see - // RefreshTokenOutput in auth/pkg/endpoints/refresh_token.go, which has no - // token field at all. The access token arrives only as a Set-Cookie header, - // so this is the sole source on every refresh, not a fallback. The body - // check is kept in case the endpoint ever starts returning one. + // The refresh endpoint returns its token in Set-Cookie, not the JSON body. if result.AccessToken == "" { for _, c := range resp.Cookies() { if c.Name != "access_token" || c.Value == "" { diff --git a/internal/auth/login_callback.go b/internal/auth/login_callback.go new file mode 100644 index 0000000..ce8dfa7 --- /dev/null +++ b/internal/auth/login_callback.go @@ -0,0 +1,49 @@ +package auth + +import ( + "net/http" + "sync" +) + +type cliCallback struct { + SessionID string `json:"session_id"` + RedemptionSecret string `json:"redemption_secret"` +} + +func newLoginCallbackHandler(sessionID string, callbacks chan<- cliCallback) http.Handler { + var once sync.Once + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "no-store") + w.Header().Set("Referrer-Policy", "no-referrer") + if r.Method != http.MethodGet { + w.Header().Set("Allow", http.MethodGet) + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + query := r.URL.Query() + if sessionID == "" || len(query["session_id"]) != 1 || query.Get("session_id") != sessionID { + http.Error(w, "invalid session", http.StatusForbidden) + return + } + secret := query.Get("redemption_secret") + if len(query["redemption_secret"]) != 1 || !validRedemptionSecret(secret) { + http.Error(w, "invalid redemption secret", http.StatusBadRequest) + return + } + once.Do(func() { callbacks <- cliCallback{SessionID: sessionID, RedemptionSecret: secret} }) + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + _, _ = w.Write([]byte("Callback received. Return to your terminal to check authentication completed. You can close this tab.")) + }) +} + +func validRedemptionSecret(secret string) bool { + if len(secret) != 64 { + return false + } + for _, c := range secret { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} diff --git a/internal/auth/login_callback_test.go b/internal/auth/login_callback_test.go new file mode 100644 index 0000000..8c3d4d0 --- /dev/null +++ b/internal/auth/login_callback_test.go @@ -0,0 +1,98 @@ +package auth + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" +) + +func TestLoginCallbackValidation(t *testing.T) { + secret := strings.Repeat("ab", 32) + tests := []struct { + name, query, method string + status int + }{ + {"valid", "session_id=expected&redemption_secret=" + secret, "GET", 200}, + {"wrong session", "session_id=other&redemption_secret=" + secret, "GET", 403}, + {"missing session", "redemption_secret=" + secret, "GET", 403}, + {"legacy missing secret", "session_id=expected", "GET", 400}, + {"short secret", "session_id=expected&redemption_secret=ab", "GET", 400}, + {"long secret", "session_id=expected&redemption_secret=" + secret + "ab", "GET", 400}, + {"uppercase secret", "session_id=expected&redemption_secret=" + strings.ToUpper(secret), "GET", 400}, + {"nonhex secret", "session_id=expected&redemption_secret=" + strings.Repeat("z", 64), "GET", 400}, + {"duplicate secret", "session_id=expected&redemption_secret=" + secret + "&redemption_secret=" + secret, "GET", 400}, + {"duplicate session", "session_id=expected&session_id=expected&redemption_secret=" + secret, "GET", 403}, + {"wrong method", "session_id=expected&redemption_secret=" + secret, "POST", 405}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callbacks := make(chan cliCallback, 1) + handler := newLoginCallbackHandler("expected", callbacks) + response := httptest.NewRecorder() + handler.ServeHTTP(response, httptest.NewRequest(tt.method, "/callback?"+tt.query, nil)) + if response.Code != tt.status { + t.Fatalf("status = %d, want %d", response.Code, tt.status) + } + if tt.status != 200 { + if len(callbacks) != 0 { + t.Fatal("invalid callback consumed session") + } + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/callback?session_id=expected&redemption_secret="+secret, nil)) + } + select { + case got := <-callbacks: + if got.SessionID != "expected" || got.RedemptionSecret != secret { + t.Fatal("callback did not preserve credentials") + } + default: + t.Fatal("valid callback not delivered") + } + if strings.Contains(response.Body.String(), secret) || strings.Contains(response.Body.String(), "Authenticated Successfully") { + t.Fatal("callback exposed secret or reported premature success") + } + }) + } +} + +func TestLoginCallbackDeliveredOnce(t *testing.T) { + callbacks := make(chan cliCallback, 16) + handler := newLoginCallbackHandler("expected", callbacks) + var wg sync.WaitGroup + for range 16 { + wg.Go(func() { + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/callback?session_id=expected&redemption_secret="+strings.Repeat("a", 64), nil)) + }) + } + wg.Wait() + if len(callbacks) != 1 { + t.Fatalf("delivered %d callbacks", len(callbacks)) + } +} + +func TestFetchCLITokenIncludesRedemptionSecret(t *testing.T) { + secret := strings.Repeat("ab", 32) + newAuthServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" || r.URL.Path != "/auth/cli/token" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + var body map[string]string + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Error(err) + } + if body["session_id"] != "expected" || body["redemption_secret"] != secret { + t.Error("missing redemption credentials") + } + _, _ = w.Write([]byte(`{"access_token":"token","expires_in":60}`)) + }) + result, err := fetchCLIToken(context.Background(), "example.nullify.ai", "expected", secret) + if err != nil { + t.Fatal(err) + } + if result.AccessToken != "token" { + t.Fatal("missing access token") + } +}