diff --git a/docs/stepactions.md b/docs/stepactions.md index 235aeaf36d8..834b8e70490 100644 --- 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 index ef55e24e271..52402bbdfe6 100644 --- 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 index f2d2ccfb7f2..a9c6ebed0e6 100644 --- 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 index cd85acdd132..e1b3c0f3b8d 100644 --- 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) {