diff --git a/pkg/platform/api/buildlogstream/streamer.go b/pkg/platform/api/buildlogstream/streamer.go index 619a13508c..cc5288a6db 100644 --- a/pkg/platform/api/buildlogstream/streamer.go +++ b/pkg/platform/api/buildlogstream/streamer.go @@ -1,6 +1,7 @@ package buildlogstream import ( + "errors" "net/http" "github.com/gorilla/websocket" @@ -18,6 +19,16 @@ import ( // keeping the token out of proxy/browser response logs. const wsSubprotocol = "build-log-streamer.activestate.com.v1" +// StreamDeniedError indicates the server refused the build-log stream. +type StreamDeniedError struct { + *errs.WrapperError +} + +func IsStreamDenied(err error) bool { + var e *StreamDeniedError + return errors.As(err, &e) +} + // Connect opens the build-log-streamer WebSocket. When jwt is non-empty it is // offered via Sec-WebSocket-Protocol as `bearer.` (alongside // wsSubprotocol, which the server echoes back) so the server can authorize the @@ -40,8 +51,11 @@ func Connect(ctx context.Context, jwt string) (*websocket.Conn, error) { } logging.Debug("Creating websocket for %s (origin: %s)", url.String(), header.Get("Origin")) - conn, _, err := dialer.DialContext(ctx, url.String(), header) + conn, resp, err := dialer.DialContext(ctx, url.String(), header) if err != nil { + if resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) { + return nil, &StreamDeniedError{errs.Wrap(err, "build-log-streamer WebSocket Upgrade denied with status %d", resp.StatusCode)} + } return nil, errs.Wrap(err, "Could not create websocket dialer") } return conn, nil diff --git a/pkg/platform/api/buildlogstream/streamer_test.go b/pkg/platform/api/buildlogstream/streamer_test.go index 66e72545fa..599c835156 100644 --- a/pkg/platform/api/buildlogstream/streamer_test.go +++ b/pkg/platform/api/buildlogstream/streamer_test.go @@ -2,6 +2,7 @@ package buildlogstream import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -9,11 +10,20 @@ import ( "time" "github.com/ActiveState/cli/internal/constants" + "github.com/ActiveState/cli/internal/errs" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestIsStreamDenied(t *testing.T) { + denied := &StreamDeniedError{errs.New("denied")} + assert.True(t, IsStreamDenied(denied), "a StreamDeniedError must be recognized") + assert.True(t, IsStreamDenied(errs.Wrap(denied, "wrapped")), "denial must be recognized through wrapping") + assert.False(t, IsStreamDenied(errs.New("some other failure")), "an unrelated error must not be a denial") + assert.False(t, IsStreamDenied(nil), "nil must not be a denial") +} + // upgradeRequest captures the headers the build-log-streamer server saw on the // WS Upgrade. The mock handler writes the fields from the server goroutine and // closes recorded; callers must await() before reading the fields so there's a @@ -93,6 +103,46 @@ func TestConnect_ForwardsJWTViaSubprotocol(t *testing.T) { "client must send the versioned State Tool User-Agent so the server can monitor versions") } +// startDenyingBLS stands up a server that refuses the WS Upgrade with the given +// HTTP status, and redirects Connect's resolved service URL at it. +func startDenyingBLS(t *testing.T, status int) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + })) + t.Cleanup(srv.Close) + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + t.Setenv(constants.APIServiceOverrideEnvVarName+"BUILDLOG_STREAMER", wsURL) +} + +func TestConnect_UpgradeDeniedReturnsTypedError(t *testing.T) { + for _, status := range []int{http.StatusUnauthorized, http.StatusForbidden} { + status := status + t.Run(http.StatusText(status), func(t *testing.T) { + startDenyingBLS(t, status) + + _, err := Connect(context.Background(), "header.payload.signature") + require.Error(t, err) + var denied *StreamDeniedError + assert.Truef(t, errors.As(err, &denied), + "a %d Upgrade response must be classified as denial, got: %v", status, err) + }) + } +} + +func TestConnect_NonAuthDialErrorNotDenied(t *testing.T) { + // A handshake failure that isn't an auth rejection is a genuine error and + // must not be mistaken for a denial (the run should still surface it). + startDenyingBLS(t, http.StatusInternalServerError) + + _, err := Connect(context.Background(), "") + require.Error(t, err) + var denied *StreamDeniedError + assert.False(t, errors.As(err, &denied), + "a non-auth handshake failure must not be classified as denial") +} + func TestConnect_AnonymousOffersNoBearer(t *testing.T) { got := startMockBLS(t) diff --git a/pkg/runtime/internal/buildlog/buildlog.go b/pkg/runtime/internal/buildlog/buildlog.go index 94a7c29501..f28d92ca67 100644 --- a/pkg/runtime/internal/buildlog/buildlog.go +++ b/pkg/runtime/internal/buildlog/buildlog.go @@ -2,6 +2,7 @@ package buildlog import ( "context" + "errors" "fmt" "os" "strings" @@ -102,6 +103,10 @@ func (b *BuildLog) OnArtifactReady(id strfmt.UUID, cb func()) { func (b *BuildLog) Wait(ctx context.Context) error { conn, err := buildlogstream.Connect(ctx, b.authToken) if err != nil { + var denied *buildlogstream.StreamDeniedError + if errors.As(err, &denied) { + return denied + } return errs.Wrap(err, "Could not connect to build-log streamer build updates") } @@ -124,6 +129,10 @@ func (b *BuildLog) Wait(ctx context.Context) error { if err == nil { continue } + var denied *buildlogstream.StreamDeniedError + if errors.As(err, &denied) { + return denied // this is a singular error that arrives alone + } if rerr == nil { rerr = errs.New("failed build") } @@ -223,16 +232,24 @@ func (b *BuildLog) waitForBuildLog(ctx context.Context, conn *websocket.Conn, er } } + receivedFrame := false var artifactErr error for { var msg Message err := conn.ReadJSON(&msg) if err != nil { + // At this time, the server can either deny the websocket connect, or accept it and close it + // without sending any frames. We handle the latter here. + if !receivedFrame && websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseGoingAway) { + errCh <- &buildlogstream.StreamDeniedError{errs.Wrap(err, "build-log stream soft-closed with no frames")} + return + } // This should bubble up and logging it is just an extra measure to help with debugging logging.Debug("Encountered error: %s", errs.JoinMessage(err)) errCh <- err return } + receivedFrame = true if verboseLogging { logging.Debug("Received response: %s", msg.MessageTypeValue()) } diff --git a/pkg/runtime/internal/buildlog/buildlog_denial_test.go b/pkg/runtime/internal/buildlog/buildlog_denial_test.go new file mode 100644 index 0000000000..ec0d2e6c5e --- /dev/null +++ b/pkg/runtime/internal/buildlog/buildlog_denial_test.go @@ -0,0 +1,49 @@ +package buildlog + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/ActiveState/cli/internal/constants" + "github.com/ActiveState/cli/pkg/buildplan" + "github.com/ActiveState/cli/pkg/platform/api/buildlogstream" + "github.com/go-openapi/strfmt" + "github.com/gorilla/websocket" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWait_SoftCloseIsDenial drives Wait against a server that accepts the +// Upgrade then soft-closes with no frames (one of the two deny shapes), and +// asserts the run degrades to a denial instead of surfacing a build failure. +func TestWait_SoftCloseIsDenial(t *testing.T) { + upgrader := websocket.Upgrader{CheckOrigin: func(*http.Request) bool { return true }} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + t.Errorf("mock build-log-streamer failed to upgrade: %v", err) + return + } + // Drain the client's recipe request, then close normally with no build + // frames -- the server-side soft-close denial shape. + _, _, _ = conn.ReadMessage() + _ = conn.WriteControl(websocket.CloseMessage, + websocket.FormatCloseMessage(websocket.CloseNormalClosure, ""), + time.Now().Add(5*time.Second)) + _ = conn.Close() + })) + t.Cleanup(srv.Close) + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + t.Setenv(constants.APIServiceOverrideEnvVarName+"BUILDLOG_STREAMER", wsURL) + + blog := New(strfmt.UUID("00000000-0000-0000-0000-000000000000"), buildplan.ArtifactIDMap{}, "") + + err := blog.Wait(context.Background()) + require.Error(t, err) + assert.Truef(t, buildlogstream.IsStreamDenied(err), "a soft-close with no frames must be recognized as a denial, got: %v", err) +} diff --git a/pkg/runtime/setup.go b/pkg/runtime/setup.go index 322fad1203..ded39d5324 100644 --- a/pkg/runtime/setup.go +++ b/pkg/runtime/setup.go @@ -29,6 +29,7 @@ import ( "github.com/ActiveState/cli/internal/svcctl" "github.com/ActiveState/cli/internal/unarchiver" "github.com/ActiveState/cli/pkg/buildplan" + "github.com/ActiveState/cli/pkg/platform/api/buildlogstream" "github.com/ActiveState/cli/pkg/platform/api/buildplanner/types" "github.com/ActiveState/cli/pkg/platform/model" "github.com/ActiveState/cli/pkg/runtime/events" @@ -303,6 +304,14 @@ func (s *setup) update() error { // Wait for build to finish if !s.buildplan.IsBuildReady() && len(s.toBuild) > 0 { if err := blog.Wait(context.Background()); err != nil { + if buildlogstream.IsStreamDenied(err) { + if s.opts.AuthToken == "" { + return locale.WrapExternalError(err, "err_buildlog_stream_denied_unauthenticated", + "Could not monitor in-progress build. Please authenticate by running '[ACTIONABLE]state auth[/RESET]' and try again.") + } + return locale.WrapExternalError(err, "err_buildlog_stream_denied_unauthorized", + "Could not monitor in-progress build. If this is a private project, make sure your account has access to it.") + } return errs.Wrap(err, "errors occurred during buildlog streaming") } }