diff --git a/pkg/cmd/mfa_setup_link.go b/pkg/cmd/mfa_setup_link.go new file mode 100644 index 00000000..3565ff7d --- /dev/null +++ b/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() +} diff --git a/pkg/cmd/mfa_setup_link_test.go b/pkg/cmd/mfa_setup_link_test.go new file mode 100644 index 00000000..529bbc8e --- /dev/null +++ b/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)) + }) + } +} diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 09ba2acf..9813ec76 100644 --- a/pkg/cmd/root.go +++ b/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)) } } diff --git a/pkg/hookdeck/client.go b/pkg/hookdeck/client.go index 8133d145..c804f55f 100644 --- a/pkg/hookdeck/client.go +++ b/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{ diff --git a/pkg/hookdeck/error_response_test.go b/pkg/hookdeck/error_response_test.go index 3a841cdc..5818c397 100644 --- a/pkg/hookdeck/error_response_test.go +++ b/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) +} diff --git a/pkg/listen/proxy/proxy.go b/pkg/listen/proxy/proxy.go index d47b5756..70865cba 100644 --- a/pkg/listen/proxy/proxy.go +++ b/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 diff --git a/pkg/listen/proxy/proxy_access_ended_test.go b/pkg/listen/proxy/proxy_access_ended_test.go new file mode 100644 index 00000000..58e77efb --- /dev/null +++ b/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") +} diff --git a/pkg/websocket/access_ended_test.go b/pkg/websocket/access_ended_test.go new file mode 100644 index 00000000..e66c9066 --- /dev/null +++ b/pkg/websocket/access_ended_test.go @@ -0,0 +1,138 @@ +package websocket + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + ws "github.com/gorilla/websocket" + "github.com/sirupsen/logrus" + logtest "github.com/sirupsen/logrus/hooks/test" +) + +// refusingServer answers every websocket connect with status and body, the way +// Hookdeck refuses a key. +func refusingServer(t *testing.T, status int, body string) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if body != "" { + w.Header().Set("Content-Type", "application/json") + } + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(server.Close) + return server +} + +// runOnce makes one connect attempt and waits for the client to report it lost. +func runOnce(t *testing.T, client *Client) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + go client.Run(ctx) + select { + case <-client.NotifyExpired: + case <-ctx.Done(): + t.Fatal("client did not report the connection as lost") + } +} + +func TestRefusedConnectEndsAccess(t *testing.T) { + cases := []struct { + name string + status int + body string + message string + }{ + { + name: "revoked key with the server's reason", + status: http.StatusUnauthorized, + body: `{"error":{"code":"UNAUTHORIZED","message":"This CLI key is no longer valid."}}`, + message: "This CLI key is no longer valid.", + }, + { + name: "no access to the project with the server's reason", + status: http.StatusForbidden, + body: `{"error":{"code":"NO_PROJECT_ACCESS","message":"This CLI key can't reach this project."}}`, + message: "This CLI key can't reach this project.", + }, + { + name: "revoked key from a server that gives no reason", + status: http.StatusUnauthorized, + message: "This CLI key is no longer valid. Run `hookdeck login` to log in again.", + }, + { + name: "refused key from a server that gives no reason", + status: http.StatusForbidden, + message: "Hookdeck refused the connection: this CLI key can't use this project. Run `hookdeck project use` to pick another one, or `hookdeck login`.", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + server := refusingServer(t, tc.status, tc.body) + client := NewClient(wsURL(server), "cses_test", "cli-key", "tm_test", nil, "", &Config{Log: logrus.New()}) + + runOnce(t, client) + + accessErr := client.AccessEndedErr() + if accessErr == nil { + t.Fatalf("a %d on connect must end access", tc.status) + } + if accessErr.StatusCode != tc.status { + t.Errorf("StatusCode = %d, want %d", accessErr.StatusCode, tc.status) + } + if accessErr.Error() != tc.message { + t.Errorf("Error() = %q, want %q", accessErr.Error(), tc.message) + } + }) + } +} + +// 503 means Hookdeck is busy or restarting, so the CLI must keep retrying. +func TestUnavailableConnectKeepsRetrying(t *testing.T) { + server := refusingServer(t, http.StatusServiceUnavailable, "") + client := NewClient(wsURL(server), "cses_test", "cli-key", "tm_test", nil, "", &Config{Log: logrus.New()}) + + runOnce(t, client) + + if accessErr := client.AccessEndedErr(); accessErr != nil { + t.Fatalf("a 503 must not end access, got %v", accessErr) + } + if client.LastConnectErr() == nil { + t.Error("the failed connect must still be recorded") + } +} + +func TestRevokedWhileConnectedEndsAccess(t *testing.T) { + var captured http.Header + server := upgradeTestServer(t, &captured, func(conn *ws.Conn) { + _ = conn.WriteControl( + ws.CloseMessage, + ws.FormatCloseMessage(closeCodeCLIKeyRevoked, "CLI_KEY_REVOKED"), + time.Now().Add(time.Second), + ) + }) + defer server.Close() + + logger, hook := logtest.NewNullLogger() + client := NewClient(wsURL(server), "cses_test", "cli-key", "tm_test", nil, "", &Config{Log: logger}) + + runOnce(t, client) + + accessErr := client.AccessEndedErr() + if accessErr == nil { + t.Fatal("a 4003 close must end access") + } + if want := "Your CLI key was revoked. Run `hookdeck login` to log in again."; accessErr.Error() != want { + t.Errorf("Error() = %q, want %q", accessErr.Error(), want) + } + // A revoke is expected, so it must not tell the user to file a bug report. + for _, entry := range hook.AllEntries() { + if entry.Level <= logrus.ErrorLevel { + t.Errorf("4003 logged at %s level: %s", entry.Level, entry.Message) + } + } +} diff --git a/pkg/websocket/client.go b/pkg/websocket/client.go index 6d7b6157..d9c4a75f 100644 --- a/pkg/websocket/client.go +++ b/pkg/websocket/client.go @@ -104,6 +104,11 @@ type Client struct { // stateMu: written by Run, read by LastConnectErr from the proxy goroutine. lastConnectErr error + // accessEndedErr is set when Hookdeck refused this key for good: a connect + // answered 401 or 403, or a live connection closed with 4003. Guarded by + // stateMu, like lastConnectErr. + accessEndedErr *AccessEndedError + NotifyExpired chan struct{} notifyClose chan error send chan *OutgoingMessage @@ -148,6 +153,20 @@ func (c *Client) setLastConnectErr(err error) { c.stateMu.Unlock() } +// AccessEndedErr returns why Hookdeck won't let this key connect again, or nil. +// Retrying can't change the answer, so the caller should stop and show it. +func (c *Client) AccessEndedErr() *AccessEndedError { + c.stateMu.Lock() + defer c.stateMu.Unlock() + return c.accessEndedErr +} + +func (c *Client) setAccessEndedErr(err *AccessEndedError) { + c.stateMu.Lock() + c.accessEndedErr = err + c.stateMu.Unlock() +} + // HasConnected reports whether this client successfully established its // websocket connection at some point. It stays true after a disconnect, so // callers can distinguish "connected then dropped" from "never connected". @@ -285,6 +304,31 @@ func readWSConnectErrorMessage(resp *http.Response) string { return se.InnerError.Message } +// AccessEndedError means Hookdeck won't let this CLI key connect again, so +// retrying can't help: the key was revoked or expired, it can't reach the +// project any more, or the organization requires MFA the user hasn't set up. +type AccessEndedError struct { + // StatusCode is the HTTP status of the refused connect, or 0 when the server + // closed a live connection because the key was revoked. + StatusCode int + // Message is Hookdeck's explanation, when it sent one. + Message string +} + +func (e *AccessEndedError) Error() string { + if e.Message != "" { + return e.Message + } + switch e.StatusCode { + case 0: + return "Your CLI key was revoked. Run `hookdeck login` to log in again." + case http.StatusUnauthorized: + return "This CLI key is no longer valid. Run `hookdeck login` to log in again." + default: + return "Hookdeck refused the connection: this CLI key can't use this project. Run `hookdeck project use` to pick another one, or `hookdeck login`." + } +} + var unknownIDMessage string = "Unknown WebSocket ID." // ErrUnknownID can occur when the websocket session is expired or invalid @@ -340,6 +384,14 @@ func (c *Client) connect(ctx context.Context) error { if message == unknownIDMessage { return ErrUnknownID } + // 401 and 403 mean the key can't connect: revoked, expired, no access to + // the project, or MFA required. Hookdeck answers 503 for anything a retry + // could fix. + if resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) { + accessErr := &AccessEndedError{StatusCode: resp.StatusCode, Message: message} + c.setAccessEndedErr(accessErr) + return accessErr + } return err } @@ -433,6 +485,13 @@ func (c *Client) readPump() { c.cfg.Log.WithFields(log.Fields{ "prefix": "websocket.Client.Close", }).Debug("session expired on server, reconnecting to recreate it: ", err) + case closeErr.Code == closeCodeCLIKeyRevoked: + // 4003 CLI_KEY_REVOKED: the key was revoked while connected. + // Reconnecting would be refused, so the caller stops and says so. + c.setAccessEndedErr(&AccessEndedError{}) + c.cfg.Log.WithFields(log.Fields{ + "prefix": "websocket.Client.Close", + }).Debug("CLI key revoked, not reconnecting: ", err) case closeErr.Code == ws.CloseAbnormalClosure: // 1006: the connection dropped without a close handshake — a network // blip, laptop sleep, load balancer idle timeout, or a pod killed @@ -620,6 +679,10 @@ const ( // a ping). Reconnecting with the X-Webhook-Ids / X-Session-Filters headers // recreates the session, so this close code is expected, not an error. closeCodeSessionExpired = 4001 + + // closeCodeCLIKeyRevoked (4003) is sent by the server when the CLI key was + // revoked while connected. A reconnect would be refused. + closeCodeCLIKeyRevoked = 4003 ) //