From 133a5ec9e11bbca9b63b43188fb51ac4840c7e49 Mon Sep 17 00:00:00 2001
From: Chuang Wang <chuangw@google.com>
Date: Tue, 26 Apr 2022 16:18:36 -0700
Subject: [PATCH] Validate parameter name format

`tasks.md` mentioned that the parameter names should follow the
following rules. But we didn't enforce it.
- Must only contain alphanumeric characters, hyphens (-),
  underscores (_), and dots (.).
- Must begin with a letter or an underscore (_).

In this commit, we enforce the rules. If invalid name format is found,
validation webhook will complain.
---
 pkg/apis/pipeline/v1beta1/task_validation.go  | 25 ++++++++++++++++
 .../pipeline/v1beta1/task_validation_test.go  | 29 +++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
index 35383626013..708173a91da 100644
--- a/pkg/apis/pipeline/v1beta1/task_validation.go
+++ b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 
@@ -34,6 +35,8 @@ import (
 
 var _ apis.Validatable = (*Task)(nil)
 
+const variableNameFormat = "^[_a-zA-Z][_a-zA-Z0-9.-]*$"
+
 // Validate implements apis.Validatable
 func (t *Task) Validate(ctx context.Context) *apis.FieldError {
 	errs := validate.ObjectMetadata(t.GetObjectMeta()).ViaField("metadata")
@@ -351,6 +354,28 @@ func validateStepArrayUsage(step Step, prefix string, vars sets.String) *apis.Fi
 }
 
 func validateVariables(steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) {
+	// validate that the variable name format follows the rules
+	// - Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)
+	// - Must begin with a letter or an underscore (_)
+	re := regexp.MustCompile(variableNameFormat)
+	invalidNames := []string{}
+	// Converting to sorted list here rather than just looping map keys
+	// because we want the order of items in vars to be deterministic for purpose of unit testing
+	for _, name := range vars.List() {
+		if !re.MatchString(name) {
+			invalidNames = append(invalidNames, name)
+		}
+	}
+
+	if len(invalidNames) != 0 {
+		return &apis.FieldError{
+			Message: fmt.Sprintf("The format of following variable names is invalid. %s", invalidNames),
+			Paths:   []string{"params"},
+			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+		}
+	}
+
+	// We've checked param name format. Now, we want to check if param names are referenced correctly in each step
 	for idx, step := range steps {
 		errs = errs.Also(validateStepVariables(step, prefix, vars).ViaFieldIndex("steps", idx))
 	}
diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
index bd50010f604..3e2458f0781 100644
--- a/pkg/apis/pipeline/v1beta1/task_validation_test.go
+++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -480,6 +480,35 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: `expected exactly one, got both`,
 			Paths:   []string{"resources.outputs.name"},
 		},
+	}, {
+		name: "invalid param name format",
+		fields: fields{
+			Params: []v1beta1.ParamSpec{{
+				Name:        "_validparam1",
+				Description: "valid param name format",
+			}, {
+				Name:        "valid_param2",
+				Description: "valid param name format",
+			}, {
+				Name:        "",
+				Description: "invalid param name format",
+			}, {
+				Name:        "a^b",
+				Description: "invalid param name format",
+			}, {
+				Name:        "0ab",
+				Description: "invalid param name format",
+			}, {
+				Name:        "f oo",
+				Description: "invalid param name format",
+			}},
+			Steps: validSteps,
+		},
+		expectedError: apis.FieldError{
+			Message: fmt.Sprintf("The format of following variable names is invalid. %s", []string{"", "0ab", "a^b", "f oo"}),
+			Paths:   []string{"params"},
+			Details: "Names: \nMust only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.)\nMust begin with a letter or an underscore (_)",
+		},
 	}, {
 		name: "invalid param type",
 		fields: fields{