[public-api] Validate Workspace ID is a UUID

easyCZ · easyCZ · commit fca202c117fb · 2022-12-20T15:52:35.000Z
diff --git a/components/common-go/namegen/workspaceid.go b/components/common-go/namegen/workspaceid.go
@@ -6,13 +6,16 @@ package namegen
 
 import (
 	"crypto/rand"
+	"errors"
+	"fmt"
 	"math/big"
 	"regexp"
 	"strings"
 )
 
-// WorkspaceIDPattern generates a new workspace ID by randomly choosing
-var WorkspaceIDPattern = regexp.MustCompile(`^[a-z]{3,12}-[a-z]{2,16}-[a-z0-9]{8}$`)
+// WorkspaceIDPattern is the expected Worksapce ID pattern
+// gitpod-protocol/src/util/generate-workspace-id.ts is authoritative over the generation
+var WorkspaceIDPattern = regexp.MustCompile(`^[a-z]{3,12}-[a-z]{2,16}-[a-z0-9]{11}$`)
 
 func GenerateWorkspaceID() (string, error) {
 	s1, err := chooseRandomly(colors, 1)
@@ -23,14 +26,26 @@ func GenerateWorkspaceID() (string, error) {
 	if err != nil {
 		return "", err
 	}
-	s3, err := chooseRandomly(characters, 8)
+	s3, err := chooseRandomly(characters, 11)
 	if err != nil {
 		return "", err
 	}
 
 	return strings.Join([]string{s1, s2, s3}, "-"), nil
 }
 
+var (
+	InvalidWorkspaceID = errors.New("workspace id does not match required format")
+)
+
+func ValidateWorkspaceID(id string) error {
+	if !WorkspaceIDPattern.MatchString(id) {
+		return fmt.Errorf("id '%s' does not match workspace ID regex '%s': %w", id, WorkspaceIDPattern.String(), InvalidWorkspaceID)
+	}
+
+	return nil
+}
+
 func chooseRandomly(options []string, length int) (res string, err error) {
 	l := big.NewInt(int64(len(options)))
 	for i := 0; i < length; i++ {
diff --git a/components/common-go/namegen/workspaceid_test.go b/components/common-go/namegen/workspaceid_test.go
@@ -5,20 +5,45 @@
 package namegen_test
 
 import (
+	"github.com/stretchr/testify/require"
 	"testing"
 
 	"github.com/gitpod-io/gitpod/common-go/namegen"
 )
 
 func TestGenerateWorkspaceID(t *testing.T) {
-
 	for i := 0; i < 1000; i++ {
 		name, err := namegen.GenerateWorkspaceID()
 		if err != nil {
 			t.Error(err)
 		}
-		if !namegen.WorkspaceIDPattern.MatchString(name) {
+
+		err = namegen.ValidateWorkspaceID(name)
+		if err != nil {
 			t.Errorf("The workspace id \"%s\" didn't met the expectation.", name)
 		}
 	}
 }
+
+func TestValidateWorkspaceID(t *testing.T) {
+	valid := []string{
+		"gitpodio-gitpod-65k8jqq6up4",
+	}
+	for _, v := range valid {
+		require.NoError(t, namegen.ValidateWorkspaceID(v))
+	}
+
+	invalid := []string{
+		"",
+		"foo",
+		"foo-bar",
+		"fo-bo",
+		"foo-bar-12",
+		"foo--",
+		"---",
+	}
+	for _, i := range invalid {
+		require.Error(t, namegen.ValidateWorkspaceID(i))
+	}
+
+}
diff --git a/components/public-api-server/pkg/apiv1/workspace.go b/components/public-api-server/pkg/apiv1/workspace.go
@@ -7,8 +7,9 @@ package apiv1
 import (
 	"context"
 	"fmt"
-
 	connect "github.com/bufbuild/connect-go"
+	"github.com/gitpod-io/gitpod/common-go/log"
+	"github.com/gitpod-io/gitpod/common-go/namegen"
 	v1 "github.com/gitpod-io/gitpod/components/public-api/go/experimental/v1"
 	"github.com/gitpod-io/gitpod/components/public-api/go/experimental/v1/v1connect"
 	protocol "github.com/gitpod-io/gitpod/gitpod-protocol"
@@ -31,14 +32,19 @@ type WorkspaceService struct {
 }
 
 func (s *WorkspaceService) GetWorkspace(ctx context.Context, req *connect.Request[v1.GetWorkspaceRequest]) (*connect.Response[v1.GetWorkspaceResponse], error) {
-	logger := ctxlogrus.Extract(ctx)
+	workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId())
+	if err != nil {
+		return nil, err
+	}
+
+	logger := ctxlogrus.Extract(ctx).WithField("workspace_id", workspaceID)
 
 	conn, err := getConnection(ctx, s.connectionPool)
 	if err != nil {
 		return nil, err
 	}
 
-	workspace, err := conn.GetWorkspace(ctx, req.Msg.GetWorkspaceId())
+	workspace, err := conn.GetWorkspace(ctx, workspaceID)
 	if err != nil {
 		logger.WithError(err).Error("Failed to get workspace.")
 		return nil, proxy.ConvertError(err)
@@ -71,13 +77,18 @@ func (s *WorkspaceService) GetWorkspace(ctx context.Context, req *connect.Reques
 }
 
 func (s *WorkspaceService) GetOwnerToken(ctx context.Context, req *connect.Request[v1.GetOwnerTokenRequest]) (*connect.Response[v1.GetOwnerTokenResponse], error) {
-	logger := ctxlogrus.Extract(ctx)
+	workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId())
+	if err != nil {
+		return nil, err
+	}
+
+	logger := ctxlogrus.Extract(ctx).WithField("workspace_id", workspaceID)
 	conn, err := getConnection(ctx, s.connectionPool)
 	if err != nil {
 		return nil, err
 	}
 
-	ownerToken, err := conn.GetOwnerToken(ctx, req.Msg.GetWorkspaceId())
+	ownerToken, err := conn.GetOwnerToken(ctx, workspaceID)
 
 	if err != nil {
 		logger.WithError(err).Error("Failed to get owner token.")
@@ -123,24 +134,35 @@ func (s *WorkspaceService) ListWorkspaces(ctx context.Context, req *connect.Requ
 }
 
 func (s *WorkspaceService) UpdatePort(ctx context.Context, req *connect.Request[v1.UpdatePortRequest]) (*connect.Response[v1.UpdatePortResponse], error) {
+	workspaceID, err := validateWorkspaceID(req.Msg.GetWorkspaceId())
+	if err != nil {
+		return nil, err
+	}
+
 	conn, err := getConnection(ctx, s.connectionPool)
 	if err != nil {
 		return nil, err
 	}
-	if req.Msg.Port.Policy == v1.PortPolicy_PORT_POLICY_PRIVATE {
-		_, err = conn.OpenPort(ctx, req.Msg.GetWorkspaceId(), &protocol.WorkspaceInstancePort{
+
+	switch req.Msg.GetPort().GetPolicy() {
+	case v1.PortPolicy_PORT_POLICY_PRIVATE:
+		_, err = conn.OpenPort(ctx, workspaceID, &protocol.WorkspaceInstancePort{
 			Port:       float64(req.Msg.Port.Port),
 			Visibility: protocol.PortVisibilityPrivate,
 		})
-	} else if req.Msg.Port.Policy == v1.PortPolicy_PORT_POLICY_PUBLIC {
-		_, err = conn.OpenPort(ctx, req.Msg.GetWorkspaceId(), &protocol.WorkspaceInstancePort{
+	case v1.PortPolicy_PORT_POLICY_PUBLIC:
+		_, err = conn.OpenPort(ctx, workspaceID, &protocol.WorkspaceInstancePort{
 			Port:       float64(req.Msg.Port.Port),
 			Visibility: protocol.PortVisibilityPublic,
 		})
+	default:
+		return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Unknown port policy specified."))
 	}
 	if err != nil {
+		log.WithField("workspace_id", workspaceID).Error("Failed to update port")
 		return nil, proxy.ConvertError(err)
 	}
+
 	return connect.NewResponse(
 		&v1.UpdatePortResponse{},
 	), nil
@@ -281,3 +303,16 @@ func parseGitpodTimestamp(input string) (*timestamppb.Timestamp, error) {
 	}
 	return timestamppb.New(parsed), nil
 }
+
+func validateWorkspaceID(id string) (string, error) {
+	if id == "" {
+		return "", connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Empty workspace id specified"))
+	}
+
+	err := namegen.ValidateWorkspaceID(id)
+	if err != nil {
+		return "", connect.NewError(connect.CodeInvalidArgument, err)
+	}
+
+	return id, nil
+}
diff --git a/components/public-api-server/pkg/apiv1/workspace_test.go b/components/public-api-server/pkg/apiv1/workspace_test.go
@@ -6,6 +6,7 @@ package apiv1
 
 import (
 	"context"
+	"github.com/gitpod-io/gitpod/common-go/namegen"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -26,72 +27,60 @@ import (
 )
 
 func TestWorkspaceService_GetWorkspace(t *testing.T) {
-	const (
-		bearerToken      = "bearer-token-for-tests"
-		foundWorkspaceID = "easycz-seer-xl8o1zacpyw"
-	)
 
-	type Expectation struct {
-		Code     connect.Code
-		Response *v1.GetWorkspaceResponse
-	}
+	workspaceID := workspaceTestData[0].Protocol.Workspace.ID
 
-	scenarios := []struct {
-		name        string
-		WorkspaceID string
-		Workspaces  map[string]protocol.WorkspaceInfo
-		Expect      Expectation
-	}{
-		{
-			name:        "returns a workspace when workspace is found by ID",
-			WorkspaceID: foundWorkspaceID,
-			Workspaces: map[string]protocol.WorkspaceInfo{
-				foundWorkspaceID: workspaceTestData[0].Protocol,
-			},
-			Expect: Expectation{
-				Response: &v1.GetWorkspaceResponse{
-					Result: workspaceTestData[0].API,
-				},
-			},
-		},
-		{
-			name:        "not found when workspace is not found by ID",
-			WorkspaceID: "some-not-found-workspace-id",
-			Expect: Expectation{
-				Code: connect.CodeNotFound,
-			},
-		},
-	}
+	t.Run("invalid argument when workspace ID is missing", func(t *testing.T) {
+		_, client := setupWorkspacesService(t)
 
-	for _, test := range scenarios {
-		t.Run(test.name, func(t *testing.T) {
-			serverMock, client := setupWorkspacesService(t)
+		_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{
+			WorkspaceId: "",
+		}))
+		require.Error(t, err)
+		require.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err))
+	})
 
-			serverMock.EXPECT().GetWorkspace(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, id string) (res *protocol.WorkspaceInfo, err error) {
-				w, ok := test.Workspaces[id]
-				if !ok {
-					return nil, &jsonrpc2.Error{
-						Code:    404,
-						Message: "not found",
-					}
-				}
-				return &w, nil
-			})
+	t.Run("invalid argument when workspace ID does not validate", func(t *testing.T) {
+		_, client := setupWorkspacesService(t)
 
-			resp, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{
-				WorkspaceId: test.WorkspaceID,
-			}))
-			requireErrorCode(t, test.Expect.Code, err)
-			if test.Expect.Response != nil {
-				requireEqualProto(t, test.Expect.Response, resp.Msg)
-			}
+		_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{
+			WorkspaceId: "some-random-not-valid-workspace-id",
+		}))
+		require.Error(t, err)
+		require.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err))
+	})
+
+	t.Run("not found when workspace does not exist", func(t *testing.T) {
+		serverMock, client := setupWorkspacesService(t)
+
+		serverMock.EXPECT().GetWorkspace(gomock.Any(), workspaceID).Return(nil, &jsonrpc2.Error{
+			Code:    404,
+			Message: "not found",
 		})
-	}
+
+		_, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{
+			WorkspaceId: workspaceID,
+		}))
+		require.Error(t, err)
+		require.Equal(t, connect.CodeNotFound, connect.CodeOf(err))
+	})
+
+	t.Run("returns a workspace when it exists", func(t *testing.T) {
+		serverMock, client := setupWorkspacesService(t)
+
+		serverMock.EXPECT().GetWorkspace(gomock.Any(), workspaceID).Return(&workspaceTestData[0].Protocol, nil)
+
+		resp, err := client.GetWorkspace(context.Background(), connect.NewRequest(&v1.GetWorkspaceRequest{
+			WorkspaceId: workspaceID,
+		}))
+		require.NoError(t, err)
+
+		requireEqualProto(t, workspaceTestData[0].API, resp.Msg.GetResult())
+	})
 }
 
 func TestWorkspaceService_GetOwnerToken(t *testing.T) {
 	const (
-		bearerToken      = "bearer-token-for-tests"
 		foundWorkspaceID = "easycz-seer-xl8o1zacpyw"
 		ownerToken       = "some-owner-token"
 	)
@@ -118,7 +107,7 @@ func TestWorkspaceService_GetOwnerToken(t *testing.T) {
 		},
 		{
 			name:        "not found when workspace is not found by ID",
-			WorkspaceID: "some-not-found-workspace-id",
+			WorkspaceID: mustGenerateWorkspaceID(t),
 			Expect: Expectation{
 				Code: connect.CodeNotFound,
 			},
@@ -446,3 +435,12 @@ func requireErrorCode(t *testing.T, expected connect.Code, err error) {
 	actual := connect.CodeOf(err)
 	require.Equal(t, expected, actual, "expected code %s, but got %s from error %v", expected.String(), actual.String(), err)
 }
+
+func mustGenerateWorkspaceID(t *testing.T) string {
+	t.Helper()
+
+	wsid, err := namegen.GenerateWorkspaceID()
+	require.NoError(t, err)
+
+	return wsid
+}
