fix(listen): stop when Hookdeck ends the key's access, and show the MFA setup link

pkg/cmd/mfa_setup_link.go

@@ -0,0 +1,51 @@
+package cmd
+
+import (
+	"errors"
+	"net/http"
+	"net/url"
+	"strings"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+)
+
+// withMFASetupLink returns the error's message, followed by the dashboard link
+// to set up MFA when the API refused the request because the organization
+// requires it.
+func withMFASetupLink(err error, dashboardBaseURL string) string {
+	message := err.Error()
+	if link := mfaSetupLink(err, dashboardBaseURL); link != "" {
+		message += "\nSet it up here: " + link
+	}
+	return message
+}
+
+// mfaSetupLink returns the link to set up MFA, or "" when err isn't the API's
+// MFA refusal. The API sends a dashboard path such as
+// "/setup-mfa?organization_id=org_123". Only that path is used, joined to the
+// CLI's own dashboard address, so an error response can't point the user to
+// another site.
+func mfaSetupLink(err error, dashboardBaseURL string) string {
+	var apiErr *hookdeck.APIError
+	if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusForbidden {
+		return ""
+	}
+	if !strings.HasPrefix(apiErr.Redirect, "/setup-mfa") {
+		return ""
+	}
+
+	base, parseErr := url.Parse(dashboardBaseURL)
+	if parseErr != nil || base.Scheme == "" || base.Host == "" {
+		return ""
+	}
+	ref, parseErr := url.Parse(apiErr.Redirect)
+	if parseErr != nil || ref.IsAbs() || ref.Host != "" {
+		return ""
+	}
+
+	resolved := base.ResolveReference(ref)
+	if resolved.Scheme != base.Scheme || resolved.Host != base.Host {
+		return ""
+	}
+	return resolved.String()
+}

pkg/cmd/mfa_setup_link_test.go

@@ -0,0 +1,61 @@
+package cmd
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+)
+
+const testDashboard = "https://dashboard.hookdeck.com"
+
+func mfaRefusal(redirect string) error {
+	return &hookdeck.APIError{
+		StatusCode: http.StatusForbidden,
+		Message:    "Acme requires multi-factor authentication. Please set up MFA to continue.",
+		Redirect:   redirect,
+	}
+}
+
+func TestWithMFASetupLinkAddsTheDashboardLink(t *testing.T) {
+	got := withMFASetupLink(mfaRefusal("/setup-mfa?organization_id=org_1"), testDashboard)
+
+	assert.Equal(t, "error: Acme requires multi-factor authentication. Please set up MFA to continue.\n"+
+		"Set it up here: https://dashboard.hookdeck.com/setup-mfa?organization_id=org_1", got)
+}
+
+func TestMFASetupLinkFindsAWrappedRefusal(t *testing.T) {
+	err := fmt.Errorf("listing sources: %w", mfaRefusal("/setup-mfa?organization_id=org_1"))
+
+	assert.Equal(t, "https://dashboard.hookdeck.com/setup-mfa?organization_id=org_1", mfaSetupLink(err, testDashboard))
+}
+
+func TestMFASetupLinkUsesTheCLIsDashboard(t *testing.T) {
+	assert.Equal(t, "http://tokens.localhost/setup-mfa?organization_id=org_1",
+		mfaSetupLink(mfaRefusal("/setup-mfa?organization_id=org_1"), "http://tokens.localhost"))
+}
+
+// The CLI only ever links to its own dashboard, whatever the response says.
+func TestMFASetupLinkIgnoresAnythingButASetupPath(t *testing.T) {
+	cases := map[string]error{
+		"another site":          mfaRefusal("https://evil.example.com/setup-mfa"),
+		"another site, no host": mfaRefusal("//evil.example.com/setup-mfa"),
+		"another page":          mfaRefusal("/billing"),
+		"no redirect":           mfaRefusal(""),
+		"not a 403": &hookdeck.APIError{
+			StatusCode: http.StatusUnauthorized,
+			Redirect:   "/setup-mfa?organization_id=org_1",
+		},
+		"not an API error": errors.New("boom"),
+	}
+	for name, err := range cases {
+		t.Run(name, func(t *testing.T) {
+			assert.Empty(t, mfaSetupLink(err, testDashboard))
+			assert.Equal(t, err.Error(), withMFASetupLink(err, testDashboard))
+		})
+	}
+}

pkg/cmd/root.go

@@ -249,9 +249,9 @@ func Execute() {
 					fmt.Println(msg)
 				}
 			} else if isMCP {
-				fmt.Fprintln(os.Stderr, err)
+				fmt.Fprintln(os.Stderr, withMFASetupLink(err, Config.DashboardBaseURL))
 			} else {
-				fmt.Println(err)
+				fmt.Println(withMFASetupLink(err, Config.DashboardBaseURL))
 			}
 		}
 

pkg/hookdeck/client.go

@@ -144,6 +144,10 @@ type ErrorResponse struct {
 	// change — the Outpost API, for instance, returns
 	// {"message":"validation error","data":["topic is invalid"]}.
 	Data []string `json:"data,omitempty"`
+
+	// Redirect is the dashboard path that fixes the error, e.g.
+	// "/setup-mfa?organization_id=org_123" when the organization requires MFA.
+	Redirect string `json:"redirect,omitempty"`
 }
 
 // UnmarshalJSON decodes an error body, accepting every shape "data" is returned
@@ -157,9 +161,10 @@ type ErrorResponse struct {
 func (e *ErrorResponse) UnmarshalJSON(data []byte) error {
 	// A distinct type avoids recursing into this method.
 	var raw struct {
-		Handled bool            `json:"Handled"`
-		Message string          `json:"message"`
-		Data    json.RawMessage `json:"data,omitempty"`
+		Handled  bool            `json:"Handled"`
+		Message  string          `json:"message"`
+		Data     json.RawMessage `json:"data,omitempty"`
+		Redirect json.RawMessage `json:"redirect,omitempty"`
 	}
 	if err := json.Unmarshal(data, &raw); err != nil {
 		return err
@@ -168,6 +173,12 @@ func (e *ErrorResponse) UnmarshalJSON(data []byte) error {
 	e.Handled = raw.Handled
 	e.Message = raw.Message
 	e.Data = flattenErrorData(raw.Data)
+	// Only a string is a path. Anything else is ignored rather than failing the
+	// decode, which would dump the raw body instead of the message.
+	var redirect string
+	if json.Unmarshal(raw.Redirect, &redirect) == nil {
+		e.Redirect = redirect
+	}
 
 	return nil
 }
@@ -261,6 +272,8 @@ func (e *ErrorResponse) Detail() string {
 type APIError struct {
 	StatusCode int
 	Message    string
+	// Redirect is the dashboard path the API pointed to, if any (see ErrorResponse).
+	Redirect string
 }
 
 func (e *APIError) Error() string {
@@ -573,6 +586,7 @@ func checkAndPrintError(res *http.Response) error {
 			return &APIError{
 				StatusCode: res.StatusCode,
 				Message:    detail,
+				Redirect:   response.Redirect,
 			}
 		}
 		return &APIError{

pkg/hookdeck/error_response_test.go

@@ -2,6 +2,10 @@ package hookdeck
 
 import (
 	"encoding/json"
+	"errors"
+	"io"
+	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -140,3 +144,29 @@ func TestErrorResponseDropsInternalFields(t *testing.T) {
 	}
 	assert.Equal(t, "dimensions[0] must be [destination_id]", got)
 }
+
+// The MFA refusal names the dashboard page that fixes it. Keeping the path lets
+// the CLI show the link.
+func TestCheckAndPrintErrorKeepsTheRedirect(t *testing.T) {
+	res := &http.Response{
+		StatusCode: http.StatusForbidden,
+		Body: io.NopCloser(strings.NewReader(`{"error":"mfa_setup_required",` +
+			`"message":"Acme requires multi-factor authentication. Please set up MFA to continue.",` +
+			`"blocking_organization":{"id":"org_1","name":"Acme"},` +
+			`"redirect":"/setup-mfa?organization_id=org_1"}`)),
+	}
+
+	var apiErr *APIError
+	require.True(t, errors.As(checkAndPrintError(res), &apiErr))
+	assert.Equal(t, http.StatusForbidden, apiErr.StatusCode)
+	assert.Equal(t, "Acme requires multi-factor authentication. Please set up MFA to continue.", apiErr.Message)
+	assert.Equal(t, "/setup-mfa?organization_id=org_1", apiErr.Redirect)
+}
+
+// A redirect that isn't a string must not turn a readable message into a raw dump.
+func TestErrorResponseIgnoresARedirectThatIsNotAPath(t *testing.T) {
+	var response ErrorResponse
+	require.NoError(t, json.Unmarshal([]byte(`{"message":"nope","redirect":{"to":"x"}}`), &response))
+	assert.Equal(t, "nope", response.Detail())
+	assert.Empty(t, response.Redirect)
+}

pkg/listen/proxy/proxy.go

@@ -272,6 +272,14 @@ func (p *Proxy) Run(parentCtx context.Context) error {
 				return nil
 			}
 
+			// Hookdeck won't let this key connect again (revoked, no access to the
+			// project, MFA required). Retrying would loop forever, so stop and say why.
+			if accessErr := wsClient.AccessEndedErr(); accessErr != nil {
+				p.renderer.OnConnectionFailed(accessErr)
+				p.renderer.Cleanup()
+				return accessErr
+			}
+
 			p.renderer.OnDisconnected()
 			// If this attempt connected successfully before dropping (e.g. a
 			// routine server deploy closing with 1001), reset the counter so

pkg/listen/proxy/proxy_access_ended_test.go

@@ -0,0 +1,120 @@
+package proxy
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	ws "github.com/gorilla/websocket"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+	"github.com/hookdeck/hookdeck-cli/pkg/websocket"
+)
+
+// runAgainst runs `listen` against a websocket server and returns what Run
+// returned, how many times the CLI connected, and the renderer.
+func runAgainst(t *testing.T, wsHandler http.HandlerFunc) (error, int32, *recordingRenderer) {
+	t.Helper()
+
+	// No backoff, so a CLI that keeps retrying shows up as many connects.
+	restoreBackoff := fixedConnectBackoffMS
+	fixedConnectBackoffMS = 1
+	t.Cleanup(func() { fixedConnectBackoffMS = restoreBackoff })
+
+	api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == hookdeck.APIPathPrefix+"/cli-sessions" {
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"id":"cli_sess_1"}`))
+			return
+		}
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	t.Cleanup(api.Close)
+
+	var connects atomic.Int32
+	wsServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		connects.Add(1)
+		wsHandler(w, r)
+	}))
+	t.Cleanup(wsServer.Close)
+
+	apiURL, err := url.Parse(api.URL)
+	require.NoError(t, err)
+	targetURL, err := url.Parse("http://127.0.0.1:1")
+	require.NoError(t, err)
+
+	renderer := newRecordingRenderer()
+	p := New(&Config{
+		DeviceName:    "test-device",
+		Key:           "sk_test_123456789012",
+		URL:           targetURL,
+		APIBaseURL:    api.URL,
+		WSBaseURL:     "ws://" + strings.TrimPrefix(wsServer.URL, "http://"),
+		NoWSS:         true,
+		NoHealthcheck: true,
+		APIClient:     &hookdeck.Client{BaseURL: apiURL, APIKey: "sk_test_123456789012"},
+	}, nil, renderer)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	runErr := p.Run(ctx)
+	require.NoError(t, ctx.Err(), "listen must stop by itself instead of retrying until the test times out")
+	return runErr, connects.Load(), renderer
+}
+
+func assertStoppedWith(t *testing.T, runErr error, renderer *recordingRenderer, message string) {
+	t.Helper()
+
+	var accessErr *websocket.AccessEndedError
+	require.True(t, errors.As(runErr, &accessErr), "listen must fail with why access ended, got %v", runErr)
+	assert.Equal(t, message, runErr.Error())
+
+	calls := renderer.recorded()
+	failedAt := indexOfCall(calls, "OnConnectionFailed")
+	cleanupAt := indexOfCall(calls, "Cleanup")
+	require.GreaterOrEqual(t, failedAt, 0, "the renderer must be told why listen stopped: %v", calls)
+	assert.Less(t, failedAt, cleanupAt, "the reason has to be shown before the renderer is torn down: %v", calls)
+	assert.Equal(t, -1, indexOfCall(calls, "OnDisconnected"),
+		"listen must not say it is reconnecting when it won't: %v", calls)
+	assert.Equal(t, message, renderer.connectionFailure().Error())
+}
+
+func TestRunStopsWhenTheConnectIsRefused(t *testing.T) {
+	message := "This CLI key can't reach this project. Run `hookdeck project use` to pick another one."
+	runErr, connects, renderer := runAgainst(t, func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusForbidden)
+		_, _ = w.Write([]byte(`{"error":{"code":"NO_PROJECT_ACCESS","message":"` + message + `"}}`))
+	})
+
+	assertStoppedWith(t, runErr, renderer, message)
+	assert.Equal(t, int32(1), connects, "a refused key must not be retried")
+}
+
+// Once connected, listen otherwise retries forever, so a revoke must stop it.
+func TestRunStopsWhenTheKeyIsRevokedWhileConnected(t *testing.T) {
+	upgrader := ws.Upgrader{}
+	runErr, connects, renderer := runAgainst(t, func(w http.ResponseWriter, r *http.Request) {
+		conn, err := upgrader.Upgrade(w, r, nil)
+		if err != nil {
+			return
+		}
+		_ = conn.WriteControl(
+			ws.CloseMessage,
+			ws.FormatCloseMessage(4003, "CLI_KEY_REVOKED"),
+			time.Now().Add(time.Second),
+		)
+	})
+
+	assertStoppedWith(t, runErr, renderer, "Your CLI key was revoked. Run `hookdeck login` to log in again.")
+	assert.Equal(t, int32(1), connects, "a revoked key must not reconnect")
+}