Skip to content

Commit

Permalink
fix: ensure websocket close messages are truncated to 123 bytes (#779)
Browse files Browse the repository at this point in the history
It's possible for websocket close messages to be too long, which cause
them to silently fail without a proper close message. See error below:

```
2022-03-31 17:08:34.862 [INFO]	(stdlib)	<close_notjs.go:72>	"2022/03/31 17:08:34 websocket: failed to marshal close frame: reason string max is 123 but got \"insert provisioner daemon:Cannot encode []database.ProvisionerType into oid 19098 - []database.ProvisionerType must implement Encoder or be converted to a string\" with length 161"
```
  • Loading branch information
coadler committed Apr 1, 2022
1 parent 4601a35 commit dc46ff4
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 12 deletions.
3 changes: 2 additions & 1 deletion cli/configssh_test.go
Expand Up @@ -4,10 +4,11 @@ import (
"os"
"testing"

"github.com/stretchr/testify/require"

"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/coderd/coderdtest"
"github.com/coder/coder/pty/ptytest"
"github.com/stretchr/testify/require"
)

func TestConfigSSH(t *testing.T) {
Expand Down
3 changes: 2 additions & 1 deletion cli/ssh.go
Expand Up @@ -13,11 +13,12 @@ import (
gossh "golang.org/x/crypto/ssh"
"golang.org/x/xerrors"

"golang.org/x/crypto/ssh/terminal"

"github.com/coder/coder/cli/cliflag"
"github.com/coder/coder/cli/cliui"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/codersdk"
"golang.org/x/crypto/ssh/terminal"
)

func ssh() *cobra.Command {
Expand Down
18 changes: 18 additions & 0 deletions coderd/httpapi/httpapi.go
Expand Up @@ -115,3 +115,21 @@ func Read(rw http.ResponseWriter, r *http.Request, value interface{}) bool {
}
return true
}

const websocketCloseMaxLen = 123

// WebsocketCloseSprintf formats a websocket close message and ensures it is
// truncated to the maximum allowed length.
func WebsocketCloseSprintf(format string, vars ...any) string {
msg := fmt.Sprintf(format, vars...)

// Cap msg length at 123 bytes. nhooyr/websocket only allows close messages
// of this length.
if len(msg) > websocketCloseMaxLen {
// Trim the string to 123 bytes. If we accidentally cut in the middle of
// a UTF-8 character, remove it from the string.
return strings.ToValidUTF8(string(msg[123]), "")
}

return msg
}
22 changes: 22 additions & 0 deletions coderd/httpapi/httpapi_test.go
Expand Up @@ -5,8 +5,10 @@ import (
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/coderd/httpapi"
Expand Down Expand Up @@ -142,3 +144,23 @@ func TestReadUsername(t *testing.T) {
})
}
}

func WebsocketCloseMsg(t *testing.T) {
t.Parallel()

t.Run("TruncateSingleByteCharacters", func(t *testing.T) {
t.Parallel()

msg := strings.Repeat("d", 255)
trunc := httpapi.WebsocketCloseSprintf(msg)
assert.LessOrEqual(t, len(trunc), 123)
})

t.Run("TruncateMultiByteCharacters", func(t *testing.T) {
t.Parallel()

msg := strings.Repeat("こんにちは", 10)
trunc := httpapi.WebsocketCloseSprintf(msg)
assert.LessOrEqual(t, len(trunc), 123)
})
}
8 changes: 4 additions & 4 deletions coderd/provisionerdaemons.go
Expand Up @@ -56,7 +56,7 @@ func (api *api) provisionerDaemonsListen(rw http.ResponseWriter, r *http.Request
Provisioners: []database.ProvisionerType{database.ProvisionerTypeEcho, database.ProvisionerTypeTerraform},
})
if err != nil {
_ = conn.Close(websocket.StatusInternalError, fmt.Sprintf("insert provisioner daemon:% s", err))
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("insert provisioner daemon: %s", err))
return
}

Expand All @@ -67,7 +67,7 @@ func (api *api) provisionerDaemonsListen(rw http.ResponseWriter, r *http.Request
config.LogOutput = io.Discard
session, err := yamux.Server(websocket.NetConn(r.Context(), conn, websocket.MessageBinary), config)
if err != nil {
_ = conn.Close(websocket.StatusInternalError, fmt.Sprintf("multiplex server: %s", err))
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("multiplex server: %s", err))
return
}
mux := drpcmux.New()
Expand All @@ -80,13 +80,13 @@ func (api *api) provisionerDaemonsListen(rw http.ResponseWriter, r *http.Request
Logger: api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)),
})
if err != nil {
_ = conn.Close(websocket.StatusInternalError, fmt.Sprintf("drpc register provisioner daemon: %s", err))
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("drpc register provisioner daemon: %s", err))
return
}
server := drpcserver.New(mux)
err = server.Serve(r.Context(), session)
if err != nil {
_ = conn.Close(websocket.StatusInternalError, fmt.Sprintf("serve: %s", err))
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("serve: %s", err))
return
}
_ = conn.Close(websocket.StatusGoingAway, "")
Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaceresources.go
Expand Up @@ -108,7 +108,7 @@ func (api *api) workspaceResourceDial(rw http.ResponseWriter, r *http.Request) {
Pubsub: api.Pubsub,
})
if err != nil {
_ = conn.Close(websocket.StatusInternalError, fmt.Sprintf("serve: %s", err))
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("serve: %s", err))
return
}
}
Expand Down
8 changes: 3 additions & 5 deletions provisioner/terraform/serve.go
Expand Up @@ -4,16 +4,14 @@ import (
"context"
"path/filepath"

"github.com/cli/safeexec"
"github.com/hashicorp/go-version"
"github.com/hashicorp/hc-install/product"
"github.com/hashicorp/hc-install/releases"
"golang.org/x/xerrors"

"cdr.dev/slog"

"github.com/cli/safeexec"
"github.com/coder/coder/provisionersdk"

"github.com/hashicorp/hc-install/product"
"github.com/hashicorp/hc-install/releases"
)

var (
Expand Down

0 comments on commit dc46ff4

Please sign in to comment.