Escape label names when writing to disk.

Github/other providers allow labels to contain special characters like /. These
cannot easily be represented as filenames. This commit URL encodes these label
keys, which should provide a safe mechanism for writing these names as files.

I've been unable to find documentation containing an exhaustive list of characters
allowed in labels, but this change works with everything in the Tekton repos.

This PR also includes a sample YAML that ensures a Tekton PR has been approved, which
caught this issue.

cmd/pullrequest-init/disk.go

@@ -19,6 +19,7 @@ package main
 import (
 	"encoding/json"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -37,6 +38,8 @@ import (
 // /workspace/<resource>/head.json
 // /workspace/<resource>/base.json
 
+// Filenames for labels and statuses are URL encoded for safety.
+
 // ToDisk converts a PullRequest object to an on-disk representation at the specified path.
 func ToDisk(pr *PullRequest, path string) error {
 	labelsPath := filepath.Join(path, "labels")
@@ -92,7 +95,8 @@ func commentsToDisk(path string, comments []*Comment) error {
 
 func labelsToDisk(path string, labels []*Label) error {
 	for _, l := range labels {
-		labelPath := filepath.Join(path, l.Text)
+		name := url.QueryEscape(l.Text)
+		labelPath := filepath.Join(path, name)
 		if err := ioutil.WriteFile(labelPath, []byte{}, 0700); err != nil {
 			return err
 		}
@@ -102,7 +106,8 @@ func labelsToDisk(path string, labels []*Label) error {
 
 func statusToDisk(path string, statuses []*Status) error {
 	for _, s := range statuses {
-		statusPath := filepath.Join(path, s.ID+".json")
+		statusName := url.QueryEscape(s.ID) + ".json"
+		statusPath := filepath.Join(path, statusName)
 		b, err := json.Marshal(s)
 		if err != nil {
 			return err
@@ -191,7 +196,11 @@ func labelsFromDisk(path string) ([]*Label, error) {
 	}
 	labels := []*Label{}
 	for _, fi := range fis {
-		labels = append(labels, &Label{fi.Name()})
+		text, err := url.QueryUnescape(fi.Name())
+		if err != nil {
+			return nil, err
+		}
+		labels = append(labels, &Label{Text: text})
 	}
 	return labels, nil
 }

cmd/pullrequest-init/disk_test.go

@@ -19,8 +19,11 @@ package main
 import (
 	"encoding/json"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"path/filepath"
+	"reflect"
+	"sort"
 	"strconv"
 	"testing"
 
@@ -48,6 +51,12 @@ func TestToDisk(t *testing.T) {
 				Description: "foobar",
 				URL:         "https://foo.bar",
 			},
+			{
+				ID:          "cla/foo",
+				Code:        Success,
+				Description: "bazbat",
+				URL:         "https://baz.bat",
+			},
 		},
 		Comments: []*Comment{
 			{
@@ -59,6 +68,7 @@ func TestToDisk(t *testing.T) {
 		Labels: []*Label{
 			{Text: "help"},
 			{Text: "me"},
+			{Text: "foo/bar"},
 		},
 	}
 
@@ -111,7 +121,11 @@ func TestToDisk(t *testing.T) {
 	}
 	labels := map[string]struct{}{}
 	for _, fi := range fis {
-		labels[fi.Name()] = struct{}{}
+		text, err := url.QueryUnescape(fi.Name())
+		if err != nil {
+			t.Errorf("Error decoding label text: %s", fi.Name())
+		}
+		labels[text] = struct{}{}
 	}
 
 	for _, l := range tektonPr.Labels {
@@ -198,7 +212,7 @@ func TestFromDisk(t *testing.T) {
 	if err := os.MkdirAll(filepath.Join(d, "labels"), 0750); err != nil {
 		t.Fatal(err)
 	}
-	labels := []string{"hey", "you"}
+	labels := []string{"hey", "you", "size%2Flgtm"}
 	for _, l := range labels {
 		if err := ioutil.WriteFile(filepath.Join(d, l), []byte{}, 0700); err != nil {
 			t.Fatal(err)
@@ -264,7 +278,8 @@ func TestFromDisk(t *testing.T) {
 		labelsMap[l] = struct{}{}
 	}
 	for _, l := range pr.Labels {
-		if diff := cmp.Diff(labelsMap[l.Text], &l); diff != "" {
+		key := url.QueryEscape(l.Text)
+		if diff := cmp.Diff(labelsMap[key], &l); diff != "" {
 			t.Errorf("Get labels: -want +got: %s", diff)
 		}
 	}
@@ -291,3 +306,215 @@ func readAndUnmarshal(t *testing.T, p string, v interface{}) {
 		t.Fatal(err)
 	}
 }
+
+func Test_labelsToDisk(t *testing.T) {
+	type args struct {
+		labels []*Label
+	}
+	tests := []struct {
+		name      string
+		args      args
+		wantFiles []string
+	}{
+		{
+			name: "single label",
+			args: args{
+				labels: []*Label{
+					{Text: "foo"},
+				},
+			},
+			wantFiles: []string{
+				"foo",
+			},
+		},
+		{
+			name: "multiple labels",
+			args: args{
+				labels: []*Label{
+					{Text: "foo"},
+					{Text: "bar"},
+				},
+			},
+			wantFiles: []string{
+				"foo",
+				"bar",
+			},
+		},
+		{
+			name: "complex labels",
+			args: args{
+				labels: []*Label{
+					{Text: "foo/bar"},
+					{Text: "help wanted"},
+					{Text: "simple"},
+				},
+			},
+			wantFiles: []string{
+				"foo%2Fbar",
+				"help+wanted",
+				"simple",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d, err := ioutil.TempDir("", "")
+			if err != nil {
+				t.Fatalf("Error creating temp dir: %s", err)
+			}
+			defer os.RemoveAll(d)
+			if err := labelsToDisk(d, tt.args.labels); err != nil {
+				t.Errorf("labelsToDisk() error = %v", err)
+			}
+			for _, f := range tt.wantFiles {
+				if _, err := os.Stat(filepath.Join(d, f)); err != nil {
+					t.Errorf("expected file %s to exist", f)
+				}
+			}
+		})
+	}
+}
+
+func Test_statusToDisk(t *testing.T) {
+	type args struct {
+		statuses []*Status
+	}
+	tests := []struct {
+		name      string
+		args      args
+		wantFiles []string
+	}{
+		{
+			name: "single status",
+			args: args{
+				statuses: []*Status{
+					{ID: "foo"},
+				},
+			},
+			wantFiles: []string{
+				"foo.json",
+			},
+		},
+		{
+			name: "multiple statuses",
+			args: args{
+				statuses: []*Status{
+					{ID: "foo"},
+					{ID: "bar"},
+				},
+			},
+			wantFiles: []string{
+				"foo.json",
+				"bar.json",
+			},
+		},
+		{
+			name: "complex statuses",
+			args: args{
+				statuses: []*Status{
+					{ID: "foo/bar"},
+					{ID: "help wanted"},
+					{ID: "simple"},
+				},
+			},
+			wantFiles: []string{
+				"foo%2Fbar.json",
+				"help+wanted.json",
+				"simple.json",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d, err := ioutil.TempDir("", "")
+			if err != nil {
+				t.Fatalf("Error creating temp dir: %s", err)
+			}
+			defer os.RemoveAll(d)
+			if err := statusToDisk(d, tt.args.statuses); err != nil {
+				t.Errorf("statusToDisk() error = %v", err)
+			}
+			for _, f := range tt.wantFiles {
+				if _, err := os.Stat(filepath.Join(d, f)); err != nil {
+					t.Errorf("expected file %s to exist", f)
+				}
+			}
+		})
+	}
+}
+
+func Test_labelsFromDisk(t *testing.T) {
+	type args struct {
+		fileNames []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want []Label
+	}{
+		{
+			name: "single label",
+			args: args{
+				fileNames: []string{"foo"},
+			},
+			want: []Label{
+				{Text: "foo"},
+			},
+		},
+		{
+			name: "multiple labels",
+			args: args{
+				fileNames: []string{"foo", "bar"},
+			},
+			want: []Label{
+				{Text: "foo"},
+				{Text: "bar"},
+			},
+		},
+		{
+			name: "complex labels",
+			args: args{
+				fileNames: []string{"foo%2Fbar", "bar+bat"},
+			},
+			want: []Label{
+				{Text: "foo/bar"},
+				{Text: "bar bat"},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d, err := ioutil.TempDir("", "")
+			if err != nil {
+				t.Fatalf("Error creating temp dir: %s", err)
+			}
+			defer os.RemoveAll(d)
+
+			for _, l := range tt.args.fileNames {
+				if err := ioutil.WriteFile(filepath.Join(d, l), []byte{}, 0700); err != nil {
+					t.Errorf("Error creating label: %s", err)
+				}
+			}
+			got, err := labelsFromDisk(d)
+			if err != nil {
+				t.Errorf("labelsFromDisk() error = %v", err)
+			}
+
+			derefed := []Label{}
+			for _, l := range got {
+				derefed = append(derefed, *l)
+			}
+
+			sort.Slice(derefed, func(i, j int) bool {
+				return derefed[i].Text < derefed[j].Text
+			})
+			sort.Slice(tt.want, func(i, j int) bool {
+				return tt.want[i].Text < tt.want[j].Text
+			})
+
+			if !reflect.DeepEqual(derefed, tt.want) {
+				t.Errorf("labelsFromDisk() = %v, want %v", derefed, tt.want)
+			}
+		})
+	}
+}

examples/taskruns/taskrun-github-pr-yaml

@@ -0,0 +1,51 @@
+apiVersion: tekton.dev/v1alpha1
+kind: PipelineResource
+metadata:
+  name: tekton-pr
+spec:
+  type: pullRequest
+  params:
+  - name: url
+    # I just picked a random PR. The first couple didn't have any interesting comments or labels.
+    value: https://github.com/tektoncd/pipeline/pull/100
+---
+apiVersion: tekton.dev/v1alpha1
+kind: Task
+metadata:
+  name: dump-pr-task
+spec:
+  inputs:
+    resources:
+    - name: pr
+      type: pullRequest
+  steps:
+  - name: dump-workspace
+    image: ubuntu
+    command: ["sh"]
+    args: ["-c", "find $(inputs.resources.pr.path)/* -type f | xargs tail -n +1"]
+  - name: ensure-approved
+    image: ubuntu
+    command: ["/bin/bash"]
+    args:
+    - -c
+    - |
+      if [ -f "$(inputs.resources.pr.path)/labels/approved" ]; then
+        echo "PR is approved!"
+      else
+        echo "PR is not approved!"
+        exit 1
+      fi
+---
+apiVersion: tekton.dev/v1alpha1
+kind: TaskRun
+metadata:
+  name: dump-pr-task-run
+spec:
+  inputs:
+    resources:
+    - name: pr
+      resourceRef:
+        name: tekton-pr
+  taskRef:
+    name: dump-pr-task
+---