Skip to content

Commit

Permalink
Validate rule guidance strictly. (#4304)
Browse files Browse the repository at this point in the history
This change uses `bluemonday` library to perform strict validation of
`guidance` field of rule types. In case sanitized input is different
from the input itself, rule type creation/update is rejected returning
a meaningful error message.

The way input is sanitized is in line with the way it is rendered in
the CLI by the `glamour` library.
  • Loading branch information
blkt authored Aug 30, 2024
1 parent 0630091 commit d3523eb
Show file tree
Hide file tree
Showing 4 changed files with 322 additions and 17 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ require (
github.com/hashicorp/go-version v1.7.0
github.com/itchyny/gojq v0.12.16
github.com/lib/pq v1.10.9
github.com/microcosm-cc/bluemonday v1.0.27
github.com/motemen/go-loghttp v0.0.0-20231107055348-29ae44b293f4
github.com/nats-io/nats-server/v2 v2.10.20
github.com/nats-io/nats.go v1.37.0
Expand Down Expand Up @@ -155,7 +156,6 @@ require (
github.com/lestrrat-go/option v1.0.1 // indirect
github.com/mattn/go-localereader v0.0.1 // indirect
github.com/mfridman/interpolate v0.0.2 // indirect
github.com/microcosm-cc/bluemonday v1.0.27 // indirect
github.com/minio/highwayhash v1.0.3 // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/moby/buildkit v0.15.2 // indirect
Expand Down
85 changes: 69 additions & 16 deletions internal/controlplane/handlers_ruletype.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import (
"errors"
"fmt"
"strings"
"unicode/utf8"

"github.com/google/uuid"
"github.com/microcosm-cc/bluemonday"
"github.com/yuin/goldmark"
"github.com/yuin/goldmark/extension"
"github.com/yuin/goldmark/parser"
Expand All @@ -38,6 +40,10 @@ import (
minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)

var (
maxReadableStringSize = 3 * 1 << 10 // 3kB
)

var (
errInvalidRuleType = errors.New("invalid rule type")
)
Expand Down Expand Up @@ -168,7 +174,14 @@ func (s *Server) CreateRuleType(
}

projectID := entityCtx.Project.ID
if err := validateGuidance(crt.RuleType.Guidance); err != nil {

if err := validateSizeAndUTF8(crt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}
if err := sanitizeMarkdown(crt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}
if err := validateMarkdown(crt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}

Expand Down Expand Up @@ -201,7 +214,14 @@ func (s *Server) UpdateRuleType(
}

projectID := entityCtx.Project.ID
if err := validateGuidance(urt.RuleType.Guidance); err != nil {

if err := validateSizeAndUTF8(urt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}
if err := sanitizeMarkdown(urt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}
if err := validateMarkdown(urt.RuleType.Guidance); err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err)
}

Expand Down Expand Up @@ -284,22 +304,55 @@ func (s *Server) DeleteRuleType(
return &minderv1.DeleteRuleTypeResponse{}, nil
}

func validateGuidance(guidance string) error {
var (
allowedPlainChars = []string{"'", "\""}
allowedEncodings = []string{"&#39;", "&#34;"}
)

func validateSizeAndUTF8(s string) error {
// As of the time of this writing, Minder profiles and rules
// have a guidance that's less than 10kB long.
if len(guidance) > 10*1<<10 {
return fmt.Errorf(
"%w: guidance too long",
errInvalidRuleType,
)
// have a guidance that's less the maximum allowed size for
// human-readable strings.
if len(s) > maxReadableStringSize {
return errors.New("too long")
}

if !utf8.ValidString(s) {
return errors.New("not valid utf-8")
}

return nil
}

func sanitizeMarkdown(md string) error {
p := bluemonday.StrictPolicy()

// The following two for loops remove characters that we want
// to allow from both the source string and the sanitized
// version, so that we can compare the two to verify that no
// other HTML content is there.
sanitized := p.Sanitize(md)
for _, c := range allowedEncodings {
sanitized = strings.ReplaceAll(sanitized, c, "")
}
for _, c := range allowedPlainChars {
md = strings.ReplaceAll(md, c, "")
}

if sanitized != md {
return fmt.Errorf("%w: value contains html", errInvalidRuleType)
}

// The following lines validate that guidance is valid,
// parseable markdown. Be mindful that any UTF-8 string is
// valid markdown, so this is redundant at the moment. Should
// the definition of `guidance` change to bytes, this check
// would become much more relevant.
md := goldmark.New(
return nil
}

func validateMarkdown(md string) error {
// The following lines validate that `md` is valid, parseable
// markdown. Be mindful that any UTF-8 string is valid
// markdown, so this is redundant at the moment. Should we
// accept byte slices in place of strings, this check would
// become much more relevant.
gm := goldmark.New(
// GitHub Flavored Markdown
goldmark.WithExtensions(extension.GFM),
goldmark.WithParserOptions(
Expand All @@ -310,7 +363,7 @@ func validateGuidance(guidance string) error {
html.WithXHTML(),
),
)
if err := md.Convert([]byte(guidance), &bytes.Buffer{}); err != nil {
if err := gm.Convert([]byte(md), &bytes.Buffer{}); err != nil {
return fmt.Errorf(
"%w: %s",
errInvalidRuleType,
Expand Down
240 changes: 240 additions & 0 deletions internal/controlplane/handlers_ruletype_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
//
// Copyright 2024 Stacklok, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package controlplane

import (
"context"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"

mockdb "github.com/stacklok/minder/database/mock"
df "github.com/stacklok/minder/database/mock/fixtures"
db "github.com/stacklok/minder/internal/db"
sf "github.com/stacklok/minder/internal/ruletypes/mock/fixtures"
minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)

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

tests := []struct {
name string
mockStoreFunc df.MockStoreBuilder
ruleTypeServiceFunc sf.RuleTypeSvcMockBuilder
request *minderv1.CreateRuleTypeRequest
error bool
}{
{
name: "happy path",
mockStoreFunc: df.NewMockStore(
df.WithTransaction(),
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(
sf.WithSuccessfulCreateRuleType,
),
request: &minderv1.CreateRuleTypeRequest{
RuleType: &minderv1.RuleType{},
},
},
{
name: "guidance sanitize error",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.CreateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: "<div>foo</div>",
},
},
error: true,
},
{
name: "guidance not utf-8",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.CreateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: string([]byte{0xff, 0xfe, 0xfd}),
},
},
error: true,
},
{
name: "guidance too long",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.CreateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: strings.Repeat("a", 4*1<<10),
},
},
error: true,
},
}

for _, tt := range tests {
tt := tt

t.Run(tt.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()

var mockStore *mockdb.MockStore
if tt.mockStoreFunc != nil {
mockStore = tt.mockStoreFunc(ctrl)
} else {
mockStore = mockdb.NewMockStore(ctrl)
}

var mockSvc sf.RuleTypeSvcMock
if tt.ruleTypeServiceFunc != nil {
mockSvc = tt.ruleTypeServiceFunc(ctrl)
}

srv, _ := newDefaultServer(t, mockStore, nil, nil)
srv.ruleTypes = mockSvc

resp, err := srv.CreateRuleType(context.Background(), tt.request)
if tt.error {
require.Error(t, err)
require.Nil(t, resp)
return
}

require.NoError(t, err)
require.NotNil(t, resp)
})
}
}

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

tests := []struct {
name string
mockStoreFunc df.MockStoreBuilder
ruleTypeServiceFunc sf.RuleTypeSvcMockBuilder
request *minderv1.UpdateRuleTypeRequest
error bool
}{
{
name: "happy path",
mockStoreFunc: df.NewMockStore(
df.WithTransaction(),
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(
sf.WithSuccessfulUpdateRuleType,
),
request: &minderv1.UpdateRuleTypeRequest{
RuleType: &minderv1.RuleType{},
},
},
{
name: "guidance sanitize error",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.UpdateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: "<div>foo</div>",
},
},
error: true,
},
{
name: "guidance not utf-8",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.UpdateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: string([]byte{0xff, 0xfe, 0xfd}),
},
},
error: true,
},
{
name: "guidance too long",
mockStoreFunc: df.NewMockStore(
WithSuccessfulGetProjectByID(uuid.Nil),
),
ruleTypeServiceFunc: sf.NewRuleTypeServiceMock(),
request: &minderv1.UpdateRuleTypeRequest{
RuleType: &minderv1.RuleType{
Guidance: strings.Repeat("a", 4*1<<10),
},
},
error: true,
},
}

for _, tt := range tests {
tt := tt

t.Run(tt.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()

var mockStore *mockdb.MockStore
if tt.mockStoreFunc != nil {
mockStore = tt.mockStoreFunc(ctrl)
} else {
mockStore = mockdb.NewMockStore(ctrl)
}

var mockSvc sf.RuleTypeSvcMock
if tt.ruleTypeServiceFunc != nil {
mockSvc = tt.ruleTypeServiceFunc(ctrl)
}

srv, _ := newDefaultServer(t, mockStore, nil, nil)
srv.ruleTypes = mockSvc

resp, err := srv.UpdateRuleType(context.Background(), tt.request)
if tt.error {
require.Error(t, err)
require.Nil(t, resp)
return
}

require.NoError(t, err)
require.NotNil(t, resp)
})
}
}

func WithSuccessfulGetProjectByID(projectID uuid.UUID) func(*mockdb.MockStore) {
return func(mockStore *mockdb.MockStore) {
mockStore.EXPECT().
GetProjectByID(gomock.Any(), gomock.Any()).
Return(db.Project{ID: projectID}, nil)
}
}
Loading

0 comments on commit d3523eb

Please sign in to comment.