Validate YAML on Bitbucket Server & Refactor the Validation

pkg/provider/bitbucketcloud/bitbucket.go

@@ -17,7 +17,6 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud/types"
 	"go.uber.org/zap"
-	"gopkg.in/yaml.v2"
 )
 
 var _ provider.Interface = (*Provider)(nil)
@@ -262,9 +261,8 @@ func (v *Provider) concatAllYamlFiles(objects []bitbucket.RepositoryFile, event
 			if err != nil {
 				return "", err
 			}
-			var i any
-			if err := yaml.Unmarshal([]byte(data), &i); err != nil {
-				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			if err := provider.ValidateYaml([]byte(data), value.Path); err != nil {
+				return "", err
 			}
 
 			if allTemplates != "" && !strings.HasPrefix(data, "---") {

pkg/provider/bitbucketcloud/bitbucket_test.go

@@ -80,7 +80,7 @@ func TestGetTektonDir(t *testing.T) {
 			content, err := v.GetTektonDir(ctx, tt.event, ".tekton", tt.provenance)
 			if tt.wantErr != "" {
 				assert.Assert(t, err != nil, "expected error %s, got %v", tt.wantErr, err)
-				assert.Equal(t, err.Error(), tt.wantErr)
+				assert.ErrorContains(t, err, tt.wantErr)
 				return
 			}
 			if tt.contentContains == "" {

pkg/provider/bitbucketserver/bitbucketserver.go

@@ -150,6 +150,10 @@ func (v *Provider) concatAllYamlFiles(objects []string, runevent *info.Event) (s
 				return "", err
 			}
 
+			if err := provider.ValidateYaml([]byte(data), value); err != nil {
+				return "", err
+			}
+
 			if allTemplates != "" && !strings.HasPrefix(data, "---") {
 				allTemplates += "---"
 			}

pkg/provider/bitbucketserver/bitbucketserver_test.go

@@ -32,7 +32,7 @@ func TestGetTektonDir(t *testing.T) {
 		path            string
 		testDirPath     string
 		contentContains string
-		wantErr         bool
+		wantErr         string
 		removeSuffix    bool
 	}{
 		{
@@ -49,6 +49,13 @@ func TestGetTektonDir(t *testing.T) {
 			testDirPath:     "./",
 			contentContains: "",
 		},
+		{
+			name:        "Badly formatted yaml",
+			event:       bbtest.MakeEvent(nil),
+			path:        ".tekton",
+			testDirPath: "../../pipelineascode/testdata/bad_yaml/.tekton",
+			wantErr:     "error unmarshalling yaml file .tekton/badyaml.yaml: yaml: line 2: did not find expected key",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -60,15 +67,12 @@ func TestGetTektonDir(t *testing.T) {
 			v := &Provider{Logger: logger, baseURL: tURL, Client: client, projectKey: tt.event.Organization}
 			bbtest.MuxDirContent(t, mux, tt.event, tt.testDirPath, tt.path)
 			content, err := v.GetTektonDir(ctx, tt.event, tt.path, "")
-			if tt.wantErr {
-				assert.Assert(t, err != nil,
-					"GetTektonDir() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if tt.contentContains == "" {
-				assert.Equal(t, content, "")
+			if tt.wantErr != "" {
+				assert.Assert(t, err != nil, "we should have get an error here")
+				assert.ErrorContains(t, err, tt.wantErr)
 				return
 			}
+			assert.NilError(t, err)
 			assert.Assert(t, strings.Contains(content, tt.contentContains), "content %s doesn't have %s", content, tt.contentContains)
 		})
 	}

pkg/provider/gitea/gitea.go

@@ -20,7 +20,6 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"go.uber.org/zap"
-	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -258,10 +257,8 @@ func (v *Provider) concatAllYamlFiles(objects []gitea.GitEntry, event *info.Even
 			if err != nil {
 				return "", err
 			}
-			// validate yaml
-			var i any
-			if err := yaml.Unmarshal(data, &i); err != nil {
-				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			if err := provider.ValidateYaml(data, value.Path); err != nil {
+				return "", err
 			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"

pkg/provider/gitea/gitea_test.go

@@ -2,11 +2,13 @@ package gitea
 
 import (
 	"context"
+	"crypto/sha256"
 	"fmt"
 	"io"
 	"net/http"
 	"reflect"
 	"sort"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -21,6 +23,7 @@ import (
 	tgitea "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitea/test"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
+	"gotest.tools/v3/assert"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
@@ -470,3 +473,55 @@ func TestProvider_CreateStatusCommit(t *testing.T) {
 		})
 	}
 }
+
+func TestGetTektonDir(t *testing.T) {
+	testGetTektonDir := []struct {
+		treepath             string
+		event                *info.Event
+		name                 string
+		expectedString       string
+		provenance           string
+		filterMessageSnippet string
+		wantErr              string
+	}{
+		{
+			name: "test with badly formatted yaml",
+			event: &info.Event{
+				Organization: "tekton",
+				Repository:   "cat",
+				SHA:          "123",
+			},
+			treepath: "testdata/tree/badyaml",
+			wantErr:  "error unmarshalling yaml file badyaml.yaml: yaml: line 2: did not find expected key",
+		},
+	}
+	for _, tt := range testGetTektonDir {
+		t.Run(tt.name, func(t *testing.T) {
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			fakelogger := zap.New(observer).Sugar()
+			ctx, _ := rtesting.SetupFakeContext(t)
+			fakeclient, mux, teardown := tgitea.Setup(t)
+			defer teardown()
+			gvcs := Provider{
+				Client: fakeclient,
+				Logger: fakelogger,
+			}
+			if tt.provenance == "default_branch" {
+				tt.event.SHA = tt.event.DefaultBranch
+			} else {
+				shaDir := fmt.Sprintf("%x", sha256.Sum256([]byte(tt.treepath)))
+				tt.event.SHA = shaDir
+			}
+
+			tgitea.SetupGitTree(t, mux, tt.treepath, tt.event, false)
+			got, err := gvcs.GetTektonDir(ctx, tt.event, ".tekton", tt.provenance)
+			if tt.wantErr != "" {
+				assert.Assert(t, err != nil, "we should have get an error here")
+				assert.ErrorContains(t, err, tt.wantErr)
+				return
+			}
+			assert.NilError(t, err)
+			assert.Assert(t, strings.Contains(got, tt.expectedString), "expected %s, got %s", tt.expectedString, got)
+		})
+	}
+}

pkg/provider/gitea/test/setup.go

@@ -1,13 +1,19 @@
 package test
 
 import (
+	"crypto/sha256"
+	"encoding/base64"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"code.gitea.io/sdk/gitea"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"gotest.tools/v3/assert"
 )
 
@@ -43,3 +49,90 @@ func Setup(t *testing.T) (*gitea.Client, *http.ServeMux, func()) {
 	assert.NilError(t, err)
 	return client, mux, tearDown
 }
+
+// SetupGitTree Take a dir and fake a full GitTree Gitea api calls reply recursively over a muxer.
+func SetupGitTree(t *testing.T, mux *http.ServeMux, dir string, event *info.Event, recursive bool) {
+	entries := []gitea.GitEntry{}
+	type file struct {
+		sha, name string
+		isdir     bool
+	}
+	files := []file{}
+	if recursive {
+		err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+			sha := fmt.Sprintf("%x", sha256.Sum256([]byte(path)))
+			if err == nil && path != dir {
+				files = append(files, file{name: path, isdir: info.IsDir(), sha: sha})
+			}
+			return nil
+		})
+		assert.NilError(t, err)
+	} else {
+		dfiles, err := os.ReadDir(dir)
+		assert.NilError(t, err)
+
+		for _, f := range dfiles {
+			sha := fmt.Sprintf("%x", sha256.Sum256([]byte(f.Name())))
+			files = append(files, file{name: filepath.Join(dir, f.Name()), sha: sha, isdir: f.IsDir()})
+		}
+	}
+	for _, f := range files {
+		etype := "blob"
+		mode := "100644"
+		if f.isdir {
+			etype = "tree"
+			mode = "040000"
+			if !recursive {
+				SetupGitTree(t, mux, f.name,
+					&info.Event{
+						Organization: event.Organization,
+						Repository:   event.Repository,
+						SHA:          f.sha,
+					},
+					true)
+			}
+		} else {
+			mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/git/blobs/%v", event.Organization, event.Repository, f.sha),
+				func(w http.ResponseWriter, r *http.Request) {
+					// go over all files and match the sha to the name we want
+					sha := filepath.Base(r.URL.Path)
+					chosenf := file{}
+					for _, f := range files {
+						if f.sha == sha {
+							chosenf = f
+							break
+						}
+					}
+					assert.Assert(t, chosenf.name != "", "sha %s not found", sha)
+
+					s, err := os.ReadFile(chosenf.name)
+					assert.NilError(t, err)
+					// encode content as base64
+					blob := &gitea.GitBlobResponse{
+						SHA:     chosenf.sha,
+						Content: base64.StdEncoding.EncodeToString(s),
+					}
+					b, err := json.Marshal(blob)
+					assert.NilError(t, err)
+					fmt.Fprint(w, string(b))
+				})
+		}
+		entries = append(entries, gitea.GitEntry{
+			Path: strings.TrimPrefix(f.name, dir+"/"),
+			Mode: mode,
+			Type: etype,
+			SHA:  f.sha,
+		})
+	}
+	u := fmt.Sprintf("/repos/%v/%v/git/trees/%v", event.Organization, event.Repository, event.SHA)
+	mux.HandleFunc(u, func(rw http.ResponseWriter, _ *http.Request) {
+		tree := &gitea.GitTreeResponse{
+			SHA:     event.SHA,
+			Entries: entries,
+		}
+		// encode tree as json
+		b, err := json.Marshal(tree)
+		assert.NilError(t, err)
+		fmt.Fprint(rw, string(b))
+	})
+}

pkg/provider/gitea/testdata/tree/badyaml/.tekton/badyaml.yaml

@@ -0,0 +1,3 @@
+foo:
+    bar:
+   xlxlxl

pkg/provider/github/github.go

@@ -22,7 +22,6 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"go.uber.org/zap"
 	"golang.org/x/oauth2"
-	"k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -410,10 +409,8 @@ func (v *Provider) concatAllYamlFiles(ctx context.Context, objects []*github.Tre
 			if err != nil {
 				return "", err
 			}
-			// validate yaml
-			var i any
-			if err := yaml.Unmarshal(data, &i); err != nil {
-				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.GetPath(), err)
+			if err := provider.ValidateYaml(data, value.GetPath()); err != nil {
+				return "", err
 			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"

pkg/provider/github/github_test.go

@@ -275,7 +275,7 @@ func TestGetTektonDir(t *testing.T) {
 			},
 			expectedString: "FROMSUBTREE",
 			treepath:       "testdata/tree/badyaml",
-			wantErr:        "error unmarshalling yaml file badyaml.yaml: error converting YAML to JSON: yaml: line 2: did not find expected key",
+			wantErr:        "error unmarshalling yaml file badyaml.yaml: yaml: line 2: did not find expected key",
 		},
 	}
 	for _, tt := range testGetTektonDir {
@@ -300,7 +300,7 @@ func TestGetTektonDir(t *testing.T) {
 			got, err := gvcs.GetTektonDir(ctx, tt.event, ".tekton", tt.provenance)
 			if tt.wantErr != "" {
 				assert.Assert(t, err != nil, "we should have get an error here")
-				assert.Equal(t, tt.wantErr, err.Error())
+				assert.ErrorContains(t, err, tt.wantErr)
 				return
 			}
 			assert.NilError(t, err)

pkg/provider/gitlab/gitlab.go

@@ -19,7 +19,6 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"github.com/xanzy/go-gitlab"
 	"go.uber.org/zap"
-	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -291,10 +290,8 @@ func (v *Provider) concatAllYamlFiles(objects []*gitlab.TreeNode, runevent *info
 			if err != nil {
 				return "", err
 			}
-			// validate yaml
-			var i any
-			if err := yaml.Unmarshal(data, &i); err != nil {
-				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			if err := provider.ValidateYaml(data, value.Path); err != nil {
+				return "", err
 			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"

pkg/provider/gitlab/gitlab_test.go

@@ -392,7 +392,7 @@ func TestGetTektonDir(t *testing.T) {
 			got, err := v.GetTektonDir(ctx, tt.args.event, tt.args.path, tt.args.provenance)
 			if tt.wantErr != "" {
 				assert.Assert(t, err != nil, "expected error %s, got %v", tt.wantErr, err)
-				assert.Equal(t, err.Error(), tt.wantErr)
+				assert.ErrorContains(t, err, tt.wantErr)
 				return
 			}
 			if tt.wantStr != "" {

pkg/provider/provider.go

@@ -5,6 +5,8 @@ import (
 	"net/url"
 	"regexp"
 	"strings"
+
+	"gopkg.in/yaml.v2"
 )
 
 var (
@@ -123,3 +125,11 @@ func CompareHostOfURLS(uri1, uri2 string) bool {
 	}
 	return u1.Host == u2.Host
 }
+
+func ValidateYaml(content []byte, filename string) error {
+	var validYaml any
+	if err := yaml.Unmarshal(content, &validYaml); err != nil {
+		return fmt.Errorf("error unmarshalling yaml file %s: %w", filename, err)
+	}
+	return nil
+}

test/testdata/TestGithubPullRequestSecondBadYaml.golden

@@ -1 +1 @@
-There was an issue validating the commit: "error while parsing the yaml file bad-yaml.yaml: line 3: could not find expected ':'"
+There was an issue validating the commit: "error while parsing the yaml file bad-yaml.yaml: yaml: line 3: could not find expected ':'"