TEP-0142: param substitutions not allowed in StepAction's script

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`.

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]}"

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

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) {