PR upload validates comment file extension

Pullrequest upload now validates the extension of each comment file:
the only valid extensions are none and .json. This allows the upload to
fail if a .json comment file cannot be parsed, rather than posting all
the contents as the comment. Fixes #2168

This behaviour can be disabled by setting a parameter called
`disable-strict-json-comments`: setting this parameter will disable strict
json parsing on comment files with json extension.

Signed-off-by: Arash Deshmeh <[email protected]>

cmd/pullrequest-init/main.go

@@ -26,11 +26,12 @@ import (
 )
 
 var (
- prURL = flag.String("url", "", "The url of the pull request to initialize.")
- path = flag.String("path", "", "Path of directory under which PR will be copied")
- mode = flag.String("mode", "download", "Whether to operate in download or upload mode")
- provider = flag.String("provider", "", "The SCM provider to use. Optional")
- skipTLSVerify = flag.Bool("insecure-skip-tls-verify", false, "Enable skipping TLS certificate verification in the git client. Defaults to false")
+ prURL = flag.String("url", "", "The url of the pull request to initialize.")
+ path = flag.String("path", "", "Path of directory under which PR will be copied")
+ mode = flag.String("mode", "download", "Whether to operate in download or upload mode")
+ provider = flag.String("provider", "", "The SCM provider to use. Optional")
+ skipTLSVerify = flag.Bool("insecure-skip-tls-verify", false, "Enable skipping TLS certificate verification in the git client. Defaults to false")
+ disableStrictJSONComments = flag.Bool("disable-strict-json-comments", false, "Disable strict json parsing for comment files with .json extension")
 )
 
 func main() {
@@ -65,7 +66,7 @@ func main() {
 
  case "upload":
  logger.Info("RUNNING UPLOAD!")
- r, err := pullrequest.FromDisk(*path)
+ r, err := pullrequest.FromDisk(*path, *disableStrictJSONComments)
  if err != nil {
  logger.Fatal(err)
  }

docs/resources.md

@@ -506,7 +506,10 @@ References (head and base) describe Git references. They are represented as a
 set of json files.
 
 Comments describe a pull request comment. They are represented as a set of json
-files.
+files. Add a file or modify the `Body` field in an existing json comment file to
+interact with the PR. Files with json extension will be parsed as such.
+The content of any comments file(s) with other/no extensions will be treated as
+body field of the comment.
 
 Other pull request information can be found in `pr.json`. This is a read-only
 resource. Users should use other subresources (labels, comments, etc) to

pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource.go

@@ -49,8 +49,9 @@ type Resource struct {
  // Secrets holds a struct to indicate a field name and corresponding secret name to populate it.
  Secrets []resourcev1alpha1.SecretParam `json:"secrets"`
 
- PRImage string `json:"-"`
- InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"`
+ PRImage string `json:"-"`
+ InsecureSkipTLSVerify bool `json:"insecure-skip-tls-verify"`
+ DisableStrictJSONComments bool `json:"disable-strict-json-comments"`
 }
 
 // NewResource create a new git resource to pass to a Task
@@ -59,11 +60,12 @@ func NewResource(name, prImage string, r *resourcev1alpha1.PipelineResource) (*R
  return nil, fmt.Errorf("cannot create a PR resource from a %s Pipeline Resource", r.Spec.Type)
  }
  prResource := Resource{
- Name: name,
- Type: r.Spec.Type,
- Secrets: r.Spec.SecretParams,
- PRImage: prImage,
- InsecureSkipTLSVerify: false,
+ Name: name,
+ Type: r.Spec.Type,
+ Secrets: r.Spec.SecretParams,
+ PRImage: prImage,
+ InsecureSkipTLSVerify: false,
+ DisableStrictJSONComments: false,
  }
  for _, param := range r.Spec.Params {
  switch {
@@ -77,6 +79,12 @@ func NewResource(name, prImage string, r *resourcev1alpha1.PipelineResource) (*R
  return nil, fmt.Errorf("error occurred converting %q to boolean in Pipeline Resource %s", param.Value, r.Name)
  }
  prResource.InsecureSkipTLSVerify = verify
+ case strings.EqualFold(param.Name, "disable-strict-json-comments"):
+ strict, err := strconv.ParseBool(param.Value)
+ if err != nil {
+ return nil, fmt.Errorf("error occurred converting %q to boolean in Pipeline Resource %s", param.Value, r.Name)
+ }
+ prResource.DisableStrictJSONComments = strict
  }
  }
 
@@ -101,11 +109,12 @@ func (s *Resource) GetURL() string {
 // Replacements is used for template replacement on a PullRequestResource inside of a Taskrun.
 func (s *Resource) Replacements() map[string]string {
  return map[string]string{
- "name": s.Name,
- "type": s.Type,
- "url": s.URL,
- "provider": s.Provider,
- "insecure-skip-tls-verify": strconv.FormatBool(s.InsecureSkipTLSVerify),
+ "name": s.Name,
+ "type": s.Type,
+ "url": s.URL,
+ "provider": s.Provider,
+ "insecure-skip-tls-verify": strconv.FormatBool(s.InsecureSkipTLSVerify),
+ "disable-strict-json-comments": strconv.FormatBool(s.DisableStrictJSONComments),
  }
 }
 
@@ -131,6 +140,9 @@ func (s *Resource) getSteps(mode string, sourcePath string) []pipelinev1beta1.St
  if s.InsecureSkipTLSVerify {
  args = append(args, "-insecure-skip-tls-verify=true")
  }
+ if s.DisableStrictJSONComments {
+ args = append(args, "-disable-strict-json-comments=true")
+ }
 
  evs := []corev1.EnvVar{}
  for _, sec := range s.Secrets {

pkg/apis/resource/v1alpha1/pullrequest/pull_request_resource_test.go

@@ -37,20 +37,22 @@ func TestPullRequest_NewResource(t *testing.T) {
  tb.PipelineResourceSpecParam("url", url),
  tb.PipelineResourceSpecParam("provider", "github"),
  tb.PipelineResourceSpecSecretParam("authToken", "test-secret-key", "test-secret-name"),
+ tb.PipelineResourceSpecParam("disable-strict-json-comments", "true"),
  ))
  got, err := pullrequest.NewResource("test-resource", "override-with-pr:latest", pr)
  if err != nil {
  t.Fatalf("Error creating storage resource: %s", err.Error())
  }
 
  want := &pullrequest.Resource{
- Name: "test-resource",
- Type: resourcev1alpha1.PipelineResourceTypePullRequest,
- URL: url,
- Provider: "github",
- Secrets: pr.Spec.SecretParams,
- PRImage: "override-with-pr:latest",
- InsecureSkipTLSVerify: false,
+ Name: "test-resource",
+ Type: resourcev1alpha1.PipelineResourceTypePullRequest,
+ URL: url,
+ Provider: "github",
+ Secrets: pr.Spec.SecretParams,
+ PRImage: "override-with-pr:latest",
+ InsecureSkipTLSVerify: false,
+ DisableStrictJSONComments: true,
  }
  if d := cmp.Diff(want, got); d != "" {
  t.Error(diff.PrintWantGot(d))
@@ -133,6 +135,21 @@ func containerTestCases(mode string) []testcase {
  Args: []string{"-url", "https://example.com", "-path", workspace, "-mode", mode, "-insecure-skip-tls-verify=true"},
  Env: []corev1.EnvVar{},
  }}},
+ }, {
+ in: &pullrequest.Resource{
+ Name: "strict-json-comments",
+ URL: "https://example.com",
+ PRImage: "override-with-pr:latest",
+ DisableStrictJSONComments: true,
+ },
+ out: []v1beta1.Step{{Container: corev1.Container{
+ Name: "pr-source-strict-json-comments-78c5n",
+ Image: "override-with-pr:latest",
+ WorkingDir: pipeline.WorkspaceDir,
+ Command: []string{"/ko-app/pullrequest-init"},
+ Args: []string{"-url", "https://example.com", "-path", workspace, "-mode", mode, "-disable-strict-json-comments=true"},
+ Env: []corev1.EnvVar{},
+ }}},
  }}
 }
 

pkg/pullrequest/api_test.go

@@ -135,7 +135,7 @@ func TestUploadFromDisk(t *testing.T) {
  t.Fatal(err)
  }
 
- r, err = FromDisk(dir)
+ r, err = FromDisk(dir, false)
  if err != nil {
  t.Fatal(err)
  }

pkg/pullrequest/disk.go

@@ -19,11 +19,13 @@ package pullrequest
 import (
  "encoding/gob"
  "encoding/json"
+ "fmt"
  "io/ioutil"
  "net/url"
  "os"
  "path/filepath"
  "strconv"
+ "strings"
 
  "github.com/jenkins-x/go-scm/scm"
 )
@@ -160,7 +162,7 @@ func toDisk(path string, r interface{}, perm os.FileMode) error {
 }
 
 // FromDisk outputs a PullRequest object from an on-disk representation at the specified path.
-func FromDisk(path string) (*Resource, error) {
+func FromDisk(path string, disableJSONComments bool) (*Resource, error) {
  r := &Resource{
  PR: &scm.PullRequest{},
  Manifests: make(map[string]Manifest),
@@ -182,7 +184,7 @@ func FromDisk(path string) (*Resource, error) {
  }
 
  commentsPath := filepath.Join(path, "comments")
- r.Comments, manifest, err = commentsFromDisk(commentsPath)
+ r.Comments, manifest, err = commentsFromDisk(commentsPath, disableJSONComments)
  if err != nil {
  return nil, err
  }
@@ -246,7 +248,7 @@ func manifestFromDisk(path string) (Manifest, error) {
  return m, nil
 }
 
-func commentsFromDisk(path string) ([]*scm.Comment, Manifest, error) {
+func commentsFromDisk(path string, disableStrictJSON bool) ([]*scm.Comment, Manifest, error) {
  fis, err := ioutil.ReadDir(path)
  if isNotExistError(err) {
  return nil, nil, nil
@@ -263,9 +265,16 @@ func commentsFromDisk(path string) ([]*scm.Comment, Manifest, error) {
  if err != nil {
  return nil, nil, err
  }
- comment := scm.Comment{}
- if err := json.Unmarshal(b, &comment); err != nil {
- // The comment might be plain text.
+ var comment scm.Comment
+ if strings.EqualFold(filepath.Ext(fi.Name()), ".json") {
+ if err := json.Unmarshal(b, &comment); err != nil {
+ if disableStrictJSON {
+ comment.Body = string(b)
+ } else {
+ return nil, nil, fmt.Errorf("error parsing comment file %q: %w", fi.Name(), err)
+ }
+ }
+ } else {
  comment.Body = string(b)
  }
  comments = append(comments, &comment)

pkg/pullrequest/disk_test.go

@@ -232,7 +232,7 @@ func TestFromDiskWithoutComments(t *testing.T) {
  writeFile(filepath.Join(d, "base.json"), &base)
  writeFile(filepath.Join(d, "head.json"), &head)
 
- rsrc, err := FromDisk(d)
+ rsrc, err := FromDisk(d, false)
  if err != nil {
  t.Fatal(err)
  }
@@ -341,7 +341,7 @@ func TestFromDisk(t *testing.T) {
  t.Fatal(err)
  }
 
- rsrc, err := FromDisk(d)
+ rsrc, err := FromDisk(d, false)
  if err != nil {
  t.Fatal(err)
  }
@@ -420,6 +420,131 @@ func readAndUnmarshal(t *testing.T, p string, v interface{}) {
  }
 }
 
+func TestCommentsFromDisk(t *testing.T) {
+ tests := []struct {
+ name string
+ files map[string][]byte
+ disableStrictJSON bool
+ expectErr bool
+ expectedComments []*scm.Comment
+ }{
+ {
+ name: "comments from .json and plain text files",
+ files: map[string][]byte{
+ "comment_foo.json": []byte(`
+{
+ "Body": "foo",
+ "Author": {
+ "Login": "author"
+ }
+}`),
+ "comment_bar": []byte("bar"),
+ },
+ expectedComments: []*scm.Comment{
+ {
+ Body: "bar",
+ },
+ {
+ Body: "foo",
+ Author: scm.User{Login: "author"},
+ },
+ },
+ },
+ {
+ name: "any non-json extension is considered text",
+ files: map[string][]byte{
+ "comment.ANY": []byte("comment_any"),
+ },
+ expectedComments: []*scm.Comment{
+ {
+ Body: "comment_any",
+ },
+ },
+ },
+ {
+ name: "invalid content in .json file",
+ files: map[string][]byte{
+ "comment.json": []byte("invalid_json"),
+ },
+ expectErr: true,
+ },
+ {
+ name: "JSON extension is case-insensitive",
+ files: map[string][]byte{
+ "comment.JSON": []byte("invalid_json"),
+ },
+ expectErr: true,
+ },
+ {
+ name: "JSON extension parsing can be disabled using `disable-json-comments`",
+ files: map[string][]byte{
+ "comment.JSON": []byte("comment"),
+ },
+ disableStrictJSON: true,
+ expectedComments: []*scm.Comment{
+ {
+ Body: "comment",
+ },
+ },
+ },
+ {
+ name: "valid JSON files are parsed as such even with `disable-json-comments`",
+ files: map[string][]byte{
+ "comment_foo.json": []byte(`
+{
+ "Body": "foo",
+ "Author": {
+ "Login": "author"
+ }
+}`),
+ },
+ disableStrictJSON: true,
+ expectedComments: []*scm.Comment{
+ {
+ Body: "foo",
+ Author: scm.User{Login: "author"},
+ },
+ },
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ d, err := ioutil.TempDir("", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer os.RemoveAll(d)
+ if err := os.MkdirAll(filepath.Join(d, "comments"), 0750); err != nil {
+ t.Fatal(err)
+ }
+ fileNames := []string{}
+ for fileName, contents := range tt.files {
+ fileNames = append(fileNames, fileName)
+ if err := ioutil.WriteFile(filepath.Join(d, "comments", fileName), contents, 0700); err != nil {
+ t.Fatal(err)
+ }
+ }
+ writeManifest(t, fileNames, filepath.Join(d, "comments", manifestPath))
+
+ comments, _, err := commentsFromDisk(filepath.Join(d, "comments"), tt.disableStrictJSON)
+ if err == nil && tt.expectErr {
+ t.Fatal("expected error, got nil")
+ }
+ if err != nil && !tt.expectErr {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if len(comments) != len(tt.expectedComments) {
+ t.Fatalf("Comments length does not match expected length. expected %d, got %d: %+v", len(tt.expectedComments), len(comments), comments)
+ }
+ for i, c := range tt.expectedComments {
+ if d := cmp.Diff(c, comments[i]); d != "" {
+ t.Errorf("Get comments: %s", diff.PrintWantGot(d))
+ }
+ }
+ })
+ }
+}
+
 func TestLabelsToDisk(t *testing.T) {
  type args struct {
  labels []*scm.Label
@@ -682,7 +807,7 @@ func TestFromDiskPRShaWithNullHeadAndBase(t *testing.T) {
  writeFile(filepath.Join(d, "head.json"), &head)
  writeFile(filepath.Join(d, "pr.json"), &pr)
 
- rsrc, err := FromDisk(d)
+ rsrc, err := FromDisk(d, false)
  if err != nil {
  t.Fatal(err)
  }