Add printable checks to request path and name/desc (#740)

internal/servers/controller/handler.go

@@ -20,6 +20,7 @@ import (
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers/sessions"
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers/targets"
 	"github.com/hashicorp/boundary/sdk/strutil"
+	"github.com/hashicorp/go-cleanhttp"
 	"github.com/hashicorp/shared-secure-libs/configutil"
 
 	"github.com/hashicorp/boundary/internal/servers/controller/handlers"
@@ -53,8 +54,9 @@ func (c *Controller) handler(props HandlerProperties) (http.Handler, error) {
 
 	corsWrappedHandler := wrapHandlerWithCors(mux, props)
 	commonWrappedHandler := wrapHandlerWithCommonFuncs(corsWrappedHandler, c, props)
+	printablePathCheckHandler := cleanhttp.PrintablePathCheckHandler(commonWrappedHandler, nil)
 
-	return commonWrappedHandler, nil
+	return printablePathCheckHandler, nil
 }
 
 func handleGrpcGateway(c *Controller, props HandlerProperties) (http.Handler, error) {

internal/servers/controller/handler_test.go

@@ -110,7 +110,8 @@ func TestHandleImplementedPaths(t *testing.T) {
 			"v1/hosts/someid",
 			"v1/roles",
 			"v1/roles/someid",
-			"v1/scopes",
+			"400_v1/sc\u200Bopes",
+			"200_v1/scopes",
 			"v1/scopes/someid",
 			"v1/sessions/",
 			"v1/sessions/someid",
@@ -185,11 +186,28 @@ func TestHandleImplementedPaths(t *testing.T) {
 	} {
 		for _, p := range paths {
 			t.Run(fmt.Sprintf("%s/%s", verb, p), func(t *testing.T) {
+
+				var expCode int
+				if !strings.HasPrefix(p, "v1/") {
+					sp := strings.Split(p, "_")
+					require.Len(t, sp, 2)
+					switch sp[0] {
+					case "400":
+						expCode = http.StatusBadRequest
+					case "200":
+						expCode = http.StatusOK
+					}
+					p = sp[1]
+				}
+
 				url := fmt.Sprintf("%s/%s", c.ApiAddrs()[0], p)
 				req, err := http.NewRequest(verb, url, nil)
 				require.NoError(t, err)
 				resp, err := http.DefaultClient.Do(req)
 				require.NoError(t, err)
+				if expCode != 0 {
+					assert.Equal(t, expCode, resp.StatusCode)
+				}
 				assert.NotEqualf(t, resp.StatusCode, http.StatusNotFound, "Got response %v, wanted not 404", resp.StatusCode)
 			})
 		}

internal/servers/controller/handlers/verifiers.go

@@ -3,6 +3,7 @@ package handlers
 import (
 	"regexp"
 	"strings"
+	"unicode"
 
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/golang/protobuf/ptypes/wrappers"
@@ -30,10 +31,26 @@ func ValidateCreateRequest(i ApiResource, fn CustomValidatorFunc) error {
 		badFields["id"] = "This is a read only field."
 	}
 	if i.GetName() != nil {
-		i.GetName().Value = strings.TrimSpace(i.GetName().GetValue())
+		trimmed := strings.TrimSpace(i.GetName().GetValue())
+		switch {
+		case trimmed == "":
+			badFields["name"] = "Cannot set empty string as name"
+		case !ValidNameDescription(trimmed):
+			badFields["name"] = "Name contains unprintable characters"
+		default:
+			i.GetName().Value = trimmed
+		}
 	}
 	if i.GetDescription() != nil {
-		i.GetDescription().Value = strings.TrimSpace(i.GetDescription().GetValue())
+		trimmed := strings.TrimSpace(i.GetDescription().GetValue())
+		switch {
+		case trimmed == "":
+			badFields["description"] = "Cannot set empty string as description"
+		case !ValidNameDescription(trimmed):
+			badFields["description"] = "Description contains unprintable characters"
+		default:
+			i.GetDescription().Value = trimmed
+		}
 	}
 	if i.GetCreatedTime() != nil {
 		badFields["created_time"] = "This is a read only field."
@@ -74,17 +91,23 @@ func ValidateUpdateRequest(prefix string, r UpdateRequest, i ApiResource, fn Cus
 	}
 	if i.GetName() != nil {
 		trimmed := strings.TrimSpace(i.GetName().GetValue())
-		if trimmed == "" {
+		switch {
+		case trimmed == "":
 			badFields["name"] = "Cannot set empty string as name"
-		} else {
+		case !ValidNameDescription(trimmed):
+			badFields["name"] = "Name contains unprintable characters"
+		default:
 			i.GetName().Value = trimmed
 		}
 	}
 	if i.GetDescription() != nil {
 		trimmed := strings.TrimSpace(i.GetDescription().GetValue())
-		if trimmed == "" {
+		switch {
+		case trimmed == "":
 			badFields["description"] = "Cannot set empty string as description"
-		} else {
+		case !ValidNameDescription(trimmed):
+			badFields["description"] = "Description contains unprintable characters"
+		default:
 			i.GetDescription().Value = trimmed
 		}
 	}
@@ -157,3 +180,15 @@ func ValidId(prefix, id string) bool {
 	id = strings.TrimPrefix(id, prefix)
 	return !reInvalidID.Match([]byte(id))
 }
+
+func ValidNameDescription(in string) bool {
+	idx := strings.IndexFunc(in, func(c rune) bool {
+		return !unicode.IsPrint(c)
+	})
+
+	if idx != -1 {
+		return false
+	}
+
+	return true
+}

internal/servers/controller/handlers/verifiers_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
 	"google.golang.org/protobuf/types/known/timestamppb"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 )
 
 func errorIncludesFields(t *testing.T, err error, wantFields []string) {
@@ -38,6 +39,11 @@ func TestValidId(t *testing.T) {
 	assert.False(t, ValidId("short", "prefix_short"))
 }
 
+func TestValidNameDescription(t *testing.T) {
+	assert.True(t, ValidNameDescription("foobar"))
+	assert.False(t, ValidNameDescription("foo\u200Bbar"))
+}
+
 func TestValidateGetRequest(t *testing.T) {
 	cases := []struct {
 		name      string
@@ -228,6 +234,36 @@ func TestValidateCreateRequest(t *testing.T) {
 			},
 			badFields: []string{"id", "created_time", "version", "test"},
 		},
+		{
+			name: "good name",
+			item: &pb.User{
+				Name: wrapperspb.String("foobar"),
+			},
+			valFn: NoopValidatorFn,
+		},
+		{
+			name: "bad name",
+			item: &pb.User{
+				Name: wrapperspb.String("foo\u200Bbar"),
+			},
+			valFn:     NoopValidatorFn,
+			badFields: []string{"name"},
+		},
+		{
+			name: "good description",
+			item: &pb.User{
+				Description: wrapperspb.String("foobar"),
+			},
+			valFn: NoopValidatorFn,
+		},
+		{
+			name: "bad description",
+			item: &pb.User{
+				Description: wrapperspb.String("foo\u200Bbar"),
+			},
+			valFn:     NoopValidatorFn,
+			badFields: []string{"description"},
+		},
 	}
 
 	for _, tc := range cases {
@@ -290,6 +326,60 @@ func TestValidateUpdateRequest(t *testing.T) {
 			valFn:     NoopValidatorFn,
 			badFields: []string{"id"},
 		},
+		{
+			name:   "good name",
+			prefix: "prefix",
+			req: &pbs.UpdateUserRequest{
+				Id:         "prefix_something",
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"anything"}},
+			},
+			item: &pb.User{
+				Version: 1,
+				Name:    wrapperspb.String("foobar"),
+			},
+			valFn: NoopValidatorFn,
+		},
+		{
+			name:   "bad name",
+			prefix: "prefix",
+			req: &pbs.UpdateUserRequest{
+				Id:         "prefix_something",
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"anything"}},
+			},
+			item: &pb.User{
+				Version: 1,
+				Name:    wrapperspb.String("foo\u200Bbar"),
+			},
+			valFn:     NoopValidatorFn,
+			badFields: []string{"name"},
+		},
+		{
+			name:   "good description",
+			prefix: "prefix",
+			req: &pbs.UpdateUserRequest{
+				Id:         "prefix_something",
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"anything"}},
+			},
+			item: &pb.User{
+				Version:     1,
+				Description: wrapperspb.String("foobar"),
+			},
+			valFn: NoopValidatorFn,
+		},
+		{
+			name:   "bad description",
+			prefix: "prefix",
+			req: &pbs.UpdateUserRequest{
+				Id:         "prefix_something",
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"anything"}},
+			},
+			item: &pb.User{
+				Version:     1,
+				Description: wrapperspb.String("foo\u200Bbar"),
+			},
+			valFn:     NoopValidatorFn,
+			badFields: []string{"description"},
+		},
 		{
 			name:   "missing version",
 			prefix: "prefix",