From 352eff906213908d73b16448e17c0f2d8c446e5d Mon Sep 17 00:00:00 2001 From: Eoin McAfee <83226740+eoinmcafee00@users.noreply.github.com> Date: Tue, 31 Aug 2021 15:51:39 +0100 Subject: [PATCH] add check on template extension type - throw error if invalid (#3128) * add check on template extension type - throw error if invalid --- handler/api/template/all_test.go | 2 +- handler/api/template/create.go | 23 +++++++-- handler/api/template/create_test.go | 8 ++-- handler/api/template/delete_test.go | 6 +-- handler/api/template/find_test.go | 4 +- handler/api/template/update_test.go | 12 ++--- plugin/converter/template.go | 18 +++---- plugin/converter/template_test.go | 48 +++++++++++++++++++ .../testdata/yaml.template.invalid.yml | 6 +++ 9 files changed, 99 insertions(+), 28 deletions(-) create mode 100644 plugin/converter/testdata/yaml.template.invalid.yml diff --git a/handler/api/template/all_test.go b/handler/api/template/all_test.go index b6b86838e..c50aae24d 100644 --- a/handler/api/template/all_test.go +++ b/handler/api/template/all_test.go @@ -24,7 +24,7 @@ import ( var ( dummyTemplate = &core.Template{ - Name: "my_template", + Name: "my_template.yml", Data: "my_data", Created: 1, Updated: 2, diff --git a/handler/api/template/create.go b/handler/api/template/create.go index 231e2d53f..1a5cbaa1d 100644 --- a/handler/api/template/create.go +++ b/handler/api/template/create.go @@ -8,16 +8,23 @@ package template import ( "encoding/json" - "github.com/go-chi/chi" "net/http" + "path/filepath" "github.com/drone/drone/core" + "github.com/drone/drone/handler/api/errors" "github.com/drone/drone/handler/api/render" + + "github.com/go-chi/chi" +) + +var ( + errTemplateExtensionInvalid = errors.New("Template extension invalid. Must be yaml, starlark or jsonnet") ) type templateInput struct { - Name string `json:"name"` - Data string `json:"data"` + Name string `json:"name"` + Data string `json:"data"` } // HandleCreate returns an http.HandlerFunc that processes http @@ -32,6 +39,16 @@ func HandleCreate(templateStore core.TemplateStore) http.HandlerFunc { return } + // check valid template extension type + switch filepath.Ext(in.Name) { + case ".yml", ".yaml": + case ".star", ".starlark", ".script": + case ".jsonnet": + default: + render.BadRequest(w, errTemplateExtensionInvalid) + return + } + t := &core.Template{ Name: in.Name, Data: in.Data, diff --git a/handler/api/template/create_test.go b/handler/api/template/create_test.go index 4c43dd923..3df7230ba 100644 --- a/handler/api/template/create_test.go +++ b/handler/api/template/create_test.go @@ -48,13 +48,13 @@ func TestHandleCreate(t *testing.T) { } } -func TestHandleCreate_ValidationErrorName(t *testing.T) { +func TestHandleCreate_NotValidTemplateExtensionName(t *testing.T) { controller := gomock.NewController(t) defer controller.Finish() c := new(chi.Context) in := new(bytes.Buffer) - json.NewEncoder(in).Encode(&core.Template{Name: "", Data: "my_data"}) + json.NewEncoder(in).Encode(&core.Template{Name: "my_template", Data: "my_data"}) w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/", in) @@ -67,7 +67,7 @@ func TestHandleCreate_ValidationErrorName(t *testing.T) { t.Errorf("Want response code %d, got %d", want, got) } - got, want := &errors.Error{}, &errors.Error{Message: "No Template Name Provided"} + got, want := &errors.Error{}, &errors.Error{Message: "Template extension invalid. Must be yaml, starlark or jsonnet"} json.NewDecoder(w.Body).Decode(got) if diff := cmp.Diff(got, want); len(diff) != 0 { t.Errorf(diff) @@ -80,7 +80,7 @@ func TestHandleCreate_ValidationErrorData(t *testing.T) { c := new(chi.Context) in := new(bytes.Buffer) - json.NewEncoder(in).Encode(&core.Template{Name: "my_template", Data: ""}) + json.NewEncoder(in).Encode(&core.Template{Name: "my_template.yml", Data: ""}) w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/", in) diff --git a/handler/api/template/delete_test.go b/handler/api/template/delete_test.go index 304d90dbb..aae3c1617 100644 --- a/handler/api/template/delete_test.go +++ b/handler/api/template/delete_test.go @@ -30,7 +30,7 @@ func TestHandleDelete(t *testing.T) { template.EXPECT().Delete(gomock.Any(), dummyTemplate).Return(nil) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") w := httptest.NewRecorder() @@ -53,7 +53,7 @@ func TestHandleDelete_TemplateNotFound(t *testing.T) { template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(nil, errors.ErrNotFound) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") w := httptest.NewRecorder() @@ -83,7 +83,7 @@ func TestHandleDelete_DeleteError(t *testing.T) { template.EXPECT().Delete(gomock.Any(), dummyTemplate).Return(errors.ErrNotFound) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") w := httptest.NewRecorder() diff --git a/handler/api/template/find_test.go b/handler/api/template/find_test.go index 09bad84d2..a5687b04a 100644 --- a/handler/api/template/find_test.go +++ b/handler/api/template/find_test.go @@ -29,7 +29,7 @@ func TestHandleFind(t *testing.T) { template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(dummyTemplate, nil) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") w := httptest.NewRecorder() @@ -52,7 +52,7 @@ func TestHandleFind_TemplateNotFound(t *testing.T) { template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(nil, errors.ErrNotFound) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") w := httptest.NewRecorder() diff --git a/handler/api/template/update_test.go b/handler/api/template/update_test.go index 570b97176..7ec96bbab 100644 --- a/handler/api/template/update_test.go +++ b/handler/api/template/update_test.go @@ -32,7 +32,7 @@ func TestHandleUpdate(t *testing.T) { template.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") in := new(bytes.Buffer) @@ -55,10 +55,10 @@ func TestHandleUpdate_ValidationErrorData(t *testing.T) { defer controller.Finish() template := mock.NewMockTemplateStore(controller) - template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(&core.Template{Name: "my_template"}, nil) + template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(&core.Template{Name: "my_template.yml"}, nil) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") in := new(bytes.Buffer) @@ -90,7 +90,7 @@ func TestHandleUpdate_TemplateNotFound(t *testing.T) { template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(nil, errors.ErrNotFound) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") in := new(bytes.Buffer) @@ -119,11 +119,11 @@ func TestHandleUpdate_UpdateError(t *testing.T) { defer controller.Finish() template := mock.NewMockTemplateStore(controller) - template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(&core.Template{Name: "my_template"}, nil) + template.EXPECT().FindName(gomock.Any(), dummyTemplate.Name, dummyTemplate.Namespace).Return(&core.Template{Name: "my_template.yml"}, nil) template.EXPECT().Update(gomock.Any(), gomock.Any()).Return(errors.ErrNotFound) c := new(chi.Context) - c.URLParams.Add("name", "my_template") + c.URLParams.Add("name", "my_template.yml") c.URLParams.Add("namespace", "my_org") in := new(bytes.Buffer) diff --git a/plugin/converter/template.go b/plugin/converter/template.go index 6ac012423..5840e9c41 100644 --- a/plugin/converter/template.go +++ b/plugin/converter/template.go @@ -35,10 +35,11 @@ import ( ) var ( - // TemplateFileRE regex to verifying kind is template. - TemplateFileRE = regexp.MustCompile("^kind:\\s+template+\\n") - ErrTemplateNotFound = errors.New("template converter: template name given not found") - ErrTemplateSyntaxErrors = errors.New("template converter: there is a problem with the yaml file provided") + // templateFileRE regex to verifying kind is template. + templateFileRE = regexp.MustCompile("^kind:\\s+template+\\n") + errTemplateNotFound = errors.New("template converter: template name given not found") + errTemplateSyntaxErrors = errors.New("template converter: there is a problem with the yaml file provided") + errTemplateExtensionInvalid = errors.New("template extension invalid. must be yaml, starlark or jsonnet") ) func Template(templateStore core.TemplateStore) core.ConvertService { @@ -58,19 +59,19 @@ func (p *templatePlugin) Convert(ctx context.Context, req *core.ConvertArgs) (*c } // check kind is template - if TemplateFileRE.MatchString(req.Config.Data) == false { + if templateFileRE.MatchString(req.Config.Data) == false { return nil, nil } // map to templateArgs var templateArgs core.TemplateArgs err := yaml.Unmarshal([]byte(req.Config.Data), &templateArgs) if err != nil { - return nil, ErrTemplateSyntaxErrors + return nil, errTemplateSyntaxErrors } // get template from db template, err := p.templateStore.FindName(ctx, templateArgs.Load, req.Repo.Namespace) if err == sql.ErrNoRows { - return nil, ErrTemplateNotFound + return nil, errTemplateNotFound } if err != nil { return nil, err @@ -84,9 +85,8 @@ func (p *templatePlugin) Convert(ctx context.Context, req *core.ConvertArgs) (*c case ".jsonnet": return parseJsonnet(req, template, templateArgs) default: + return nil, errTemplateExtensionInvalid } - - return nil, nil } func parseYaml(req *core.ConvertArgs, template *core.Template, templateArgs core.TemplateArgs) (*core.Config, error) { diff --git a/plugin/converter/template_test.go b/plugin/converter/template_test.go index 3b2a8f534..820522ece 100644 --- a/plugin/converter/template_test.go +++ b/plugin/converter/template_test.go @@ -388,3 +388,51 @@ func TestTemplatePluginConvertYaml(t *testing.T) { t.Errorf("Want %q got %q", want, got) } } +// tests to check error is thrown if user has already loaded a template file of invalid extension +// and refers to it in the drone.yml file +func TestTemplatePluginConvertInvalidTemplateExtension(t *testing.T) { + // reads yml input file which refers to a template file of invalid extensions + templateArgs, err := ioutil.ReadFile("testdata/yaml.template.invalid.yml") + if err != nil { + t.Error(err) + return + } + + req := &core.ConvertArgs{ + Build: &core.Build{ + After: "3d21ec53a331a6f037a91c368710b99387d012c1", + }, + Repo: &core.Repository{ + Slug: "octocat/hello-world", + Config: ".drone.yml", + Namespace: "octocat", + }, + Config: &core.Config{ + Data: string(templateArgs), + }, + } + // reads the template drone.yml + beforeInput, err := ioutil.ReadFile("testdata/yaml.input.yml") + if err != nil { + t.Error(err) + return + } + + template := &core.Template{ + Name: "plugin.txt", + Data: string(beforeInput), + Namespace: "octocat", + } + + controller := gomock.NewController(t) + defer controller.Finish() + + templates := mock.NewMockTemplateStore(controller) + templates.EXPECT().FindName(gomock.Any(), template.Name, req.Repo.Namespace).Return(template, nil) + + plugin := Template(templates) + config, err := plugin.Convert(noContext, req) + if config != nil { + t.Errorf("template extension invalid. must be yaml, starlark or jsonnet") + } +} \ No newline at end of file diff --git a/plugin/converter/testdata/yaml.template.invalid.yml b/plugin/converter/testdata/yaml.template.invalid.yml new file mode 100644 index 000000000..fe0649468 --- /dev/null +++ b/plugin/converter/testdata/yaml.template.invalid.yml @@ -0,0 +1,6 @@ +kind: template +load: plugin.txt +data: + stepName: my_step + image: my_image + commands: my_command