TEP-0142: param substitutions not allowed in script

Based on discussions in #7497 and consensus in the API WG, we disallow direct parameter substitution in scripts. While we cannot do this for inlined-steps since it is a major breaking change in `v1`, we can do this in `Step Actions`. In this PR we add validation that params cannot be directly replaced in `scripts` of `StepActions`.
tektoncd · Jan 16, 2024 · 47848a0 · 47848a0
1 parent b4c7250
commit 47848a0
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 31 deletions.
diff --git a/docs/stepactions.md b/docs/stepactions.md
@@ -109,6 +109,9 @@ spec:
   ]
 ```
 
+> :seedling: **`params` cannot be directly used in a `script` in `StepActions`.**
+> Directly substituting `params` in `scripts` makes the workload prone to shell attacks. Therefore, we do not allow direct usage of `params` in `scripts` in `StepActions`. Instead, rely on passing `params` to `env` variables and reference them in `scripts`. We cannot do the same for `inlined-steps` because it breaks `v1 API` compatibility for existing users. 
+
 #### Passing Params to StepAction
 
 A `StepAction` may require [params](#declaring-parameters). In this case, a `Task` needs to ensure that the `StepAction` has access to all the required `params`.

diff --git a/examples/v1/taskruns/alpha/stepaction-params.yaml b/examples/v1/taskruns/alpha/stepaction-params.yaml
@@ -25,10 +25,25 @@ spec:
         key1: "step-action default key1"
         key2: "step-action default key2"
         key3: "step-action default key3"
+  env:
+    - name: arrayparam0
+      value: $(params.array-param[0])
+    - name: arrayparam1
+      value: $(params.array-param[1])
+    - name: arrayparam2
+      value: $(params.array-param[2])
+    - name: stringparam
+      value: $(params.string-param)
+    - name: objectparamkey1
+      value: $(params.object-param.key1)
+    - name: objectparamkey2
+      value: $(params.object-param.key2)
+    - name: objectparamkey3
+      value: $(params.object-param.key3)
   image: ubuntu
   script: |
     #!/bin/bash
-    ARRAYVALUE=("$(params.array-param[0])" "$(params.array-param[1])" "$(params.array-param[2])" "$(params.string-param)" "$(params.object-param.key1)" "$(params.object-param.key2)" "$(params.object-param.key3)")
+    ARRAYVALUE=("${arrayparam0}" "${arrayparam1}" "${arrayparam2}" "${stringparam}" "${objectparamkey1}" "${objectparamkey2}" "${objectparamkey3}")
     ARRAYEXPECTED=("taskrun" "array" "param" "taskrun stringparam" "taskspec default key1" "taskrun key2" "step-action default key3")
     for i in "${!ARRAYVALUE[@]}"; do
         VALUE="${ARRAYVALUE[i]}"

diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go
@@ -62,6 +62,7 @@ func (ss *StepActionSpec) Validate(ctx context.Context) (errs *apis.FieldError)
 		if strings.HasPrefix(cleaned, "#!win") {
 			errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "windows script support", config.AlphaAPIFields).ViaField("script"))
 		}
+		errs = errs.Also(validateNoParamSubstitutionsInScript(ss.Script))
 	}
 	errs = errs.Also(validateUsageOfDeclaredParameters(ctx, *ss))
 	errs = errs.Also(v1.ValidateParameterTypes(ctx, ss.Params).ViaField("params"))
@@ -72,6 +73,18 @@ func (ss *StepActionSpec) Validate(ctx context.Context) (errs *apis.FieldError)
 	return errs
 }
 
+// validateNoParamSubstitutionsInScript validates that param substitutions are not invoked in the script
+func validateNoParamSubstitutionsInScript(script string) *apis.FieldError {
+	_, present, errString := substitution.ExtractVariablesFromString(script, "params")
+	if errString != "" || present {
+		return &apis.FieldError{
+			Message: "param substitution in scripts is not allowed.",
+			Paths:   []string{"script"},
+		}
+	}
+	return nil
+}
+
 // validateUsageOfDeclaredParameters validates that all parameters referenced in the Task are declared by the Task.
 func validateUsageOfDeclaredParameters(ctx context.Context, sas StepActionSpec) *apis.FieldError {
 	params := sas.Params

diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go
@@ -191,19 +191,6 @@ func TestStepActionSpecValidate(t *testing.T) {
 			Command: []string{"$(params.foo-is-baz)"},
 			Args:    []string{"$(params.baz[*])", "middle string", "$(params.foo-is-baz[*])"},
 		},
-	}, {
-		name: "valid step with parameterized script",
-		fields: fields{
-			Params: []v1.ParamSpec{{
-				Name: "baz",
-			}, {
-				Name: "foo-is-baz",
-			}},
-			Image: "my-image",
-			Script: `
-				#!/usr/bin/env bash
-				hello $(params.baz)`,
-		},
 	}, {
 		name: "valid result",
 		fields: fields{
@@ -873,23 +860,6 @@ func TestStepActionSpecValidateError(t *testing.T) {
 			Message: `variable type invalid in "$(params.baz[*])"`,
 			Paths:   []string{"image"},
 		},
-	}, {
-		name: "array star used illegaly in script field",
-		fields: fields{
-			Params: []v1.ParamSpec{{
-				Name: "baz",
-				Type: v1.ParamTypeArray,
-			}, {
-				Name: "foo-is-baz",
-				Type: v1.ParamTypeArray,
-			}},
-			Script: "$(params.baz[*])",
-			Image:  "my-image",
-		},
-		expectedError: apis.FieldError{
-			Message: `variable type invalid in "$(params.baz[*])"`,
-			Paths:   []string{"script"},
-		},
 	}, {
 		name: "array not properly isolated",
 		fields: fields{
@@ -962,6 +932,23 @@ func TestStepActionSpecValidateError(t *testing.T) {
 			Message: `variable is not properly isolated in "not isolated: $(params.baz[*])"`,
 			Paths:   []string{"args[0]"},
 		},
+	}, {
+		name: "params used in script field",
+		fields: fields{
+			Params: []v1.ParamSpec{{
+				Name: "baz",
+				Type: v1.ParamTypeArray,
+			}, {
+				Name: "foo-is-baz",
+				Type: v1.ParamTypeString,
+			}},
+			Script: "$(params.baz[0]), $(params.foo-is-baz)",
+			Image:  "my-image",
+		},
+		expectedError: apis.FieldError{
+			Message: `param substitution in scripts is not allowed.`,
+			Paths:   []string{"script"},
+		},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {