throw when invalid uses key is provided (#1804)

* throw if `uses` is invalid * update JobType to return error * lint * put //nolint:dupl on wrong test * update error message to remove end punctuation * lint * update remote job type check * move if statement * rm nolint:dupl ... we'll see how that goes --------- Co-authored-by: Casey Lee <cplee@nektos.com>
2023-07-11 00:27:43 -04:00 · 2023-07-11 00:27:43 -04:00 · 808cf5a2c3
commit 808cf5a2c3
parent 8c7c0f53c1
4 changed files with 123 additions and 30 deletions
--- a/pkg/model/workflow.go
+++ b/pkg/model/workflow.go
@ -445,14 +445,17 @@ func commonKeysMatch2(a map[string]interface{}, b map[string]interface{}, m map[
 type JobType int

 const (
-	// StepTypeRun is all steps that have a `run` attribute
+	// JobTypeDefault is all jobs that have a `run` attribute
 	JobTypeDefault JobType = iota

-	// StepTypeReusableWorkflowLocal is all steps that have a `uses` that is a local workflow in the .github/workflows directory
+	// JobTypeReusableWorkflowLocal is all jobs that have a `uses` that is a local workflow in the .github/workflows directory
 	JobTypeReusableWorkflowLocal

-	// JobTypeReusableWorkflowRemote is all steps that have a `uses` that references a workflow file in a github repo
+	// JobTypeReusableWorkflowRemote is all jobs that have a `uses` that references a workflow file in a github repo
 	JobTypeReusableWorkflowRemote
+
+	// JobTypeInvalid represents a job which is not configured correctly
+	JobTypeInvalid
 )

 func (j JobType) String() string {
@ -468,13 +471,28 @@ func (j JobType) String() string {
 }

 // Type returns the type of the job
-func (j *Job) Type() JobType {
-	if strings.HasPrefix(j.Uses, "./.github/workflows") && (strings.HasSuffix(j.Uses, ".yml") || strings.HasSuffix(j.Uses, ".yaml")) {
-		return JobTypeReusableWorkflowLocal
-	} else if !strings.HasPrefix(j.Uses, "./") && strings.Contains(j.Uses, ".github/workflows") && (strings.Contains(j.Uses, ".yml@") || strings.Contains(j.Uses, ".yaml@")) {
-		return JobTypeReusableWorkflowRemote
+func (j *Job) Type() (JobType, error) {
+	isReusable := j.Uses != ""
+
+	if isReusable {
+		isYaml, _ := regexp.MatchString(`\.(ya?ml)(?:$|@)`, j.Uses)
+
+		if isYaml {
+			isLocalPath := strings.HasPrefix(j.Uses, "./")
+			isRemotePath, _ := regexp.MatchString(`^[^.](.+?/){2,}.+\.ya?ml@`, j.Uses)
+			hasVersion, _ := regexp.MatchString(`\.ya?ml@`, j.Uses)
+
+			if isLocalPath {
+				return JobTypeReusableWorkflowLocal, nil
+			} else if isRemotePath && hasVersion {
+				return JobTypeReusableWorkflowRemote, nil
 			}
-	return JobTypeDefault
+		}
+
+		return JobTypeInvalid, fmt.Errorf("`uses` key references invalid workflow path '%s'. Must start with './' if it's a local workflow, or must start with '<org>/<repo>/' and include an '@' if it's a remote workflow", j.Uses)
+	}
+
+	return JobTypeDefault, nil
 }

 // ContainerSpec is the specification of the container to use for the job
--- a/pkg/model/workflow_test.go
+++ b/pkg/model/workflow_test.go
@ -147,20 +147,81 @@ jobs:
    runs-on: ubuntu-latest
    steps:
      - run: echo
-  remote-reusable-workflow:
-    runs-on: ubuntu-latest
-    uses: remote/repo/.github/workflows/workflow.yml@main
-  local-reusable-workflow:
-    runs-on: ubuntu-latest
-    uses: ./.github/workflows/workflow.yml
+  remote-reusable-workflow-yml:
+    uses: remote/repo/some/path/to/workflow.yml@main
+  remote-reusable-workflow-yaml:
+    uses: remote/repo/some/path/to/workflow.yaml@main
+  remote-reusable-workflow-custom-path:
+    uses: remote/repo/path/to/workflow.yml@main
+  local-reusable-workflow-yml:
+    uses: ./some/path/to/workflow.yml
+  local-reusable-workflow-yaml:
+    uses: ./some/path/to/workflow.yaml
 `

 	workflow, err := ReadWorkflow(strings.NewReader(yaml))
 	assert.NoError(t, err, "read workflow should succeed")
-	assert.Len(t, workflow.Jobs, 3)
-	assert.Equal(t, workflow.Jobs["default-job"].Type(), JobTypeDefault)
-	assert.Equal(t, workflow.Jobs["remote-reusable-workflow"].Type(), JobTypeReusableWorkflowRemote)
-	assert.Equal(t, workflow.Jobs["local-reusable-workflow"].Type(), JobTypeReusableWorkflowLocal)
+	assert.Len(t, workflow.Jobs, 6)
+
+	jobType, err := workflow.Jobs["default-job"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeDefault, jobType)
+
+	jobType, err = workflow.Jobs["remote-reusable-workflow-yml"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeReusableWorkflowRemote, jobType)
+
+	jobType, err = workflow.Jobs["remote-reusable-workflow-yaml"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeReusableWorkflowRemote, jobType)
+
+	jobType, err = workflow.Jobs["remote-reusable-workflow-custom-path"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeReusableWorkflowRemote, jobType)
+
+	jobType, err = workflow.Jobs["local-reusable-workflow-yml"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeReusableWorkflowLocal, jobType)
+
+	jobType, err = workflow.Jobs["local-reusable-workflow-yaml"].Type()
+	assert.Equal(t, nil, err)
+	assert.Equal(t, JobTypeReusableWorkflowLocal, jobType)
+}
+
+func TestReadWorkflow_JobTypes_InvalidPath(t *testing.T) {
+	yaml := `
+name: invalid job definition
+
+jobs:
+  remote-reusable-workflow-missing-version:
+    uses: remote/repo/some/path/to/workflow.yml
+  remote-reusable-workflow-bad-extension:
+    uses: remote/repo/some/path/to/workflow.json
+  local-reusable-workflow-bad-extension:
+    uses: ./some/path/to/workflow.json
+  local-reusable-workflow-bad-path:
+    uses: some/path/to/workflow.yaml
+`
+
+	workflow, err := ReadWorkflow(strings.NewReader(yaml))
+	assert.NoError(t, err, "read workflow should succeed")
+	assert.Len(t, workflow.Jobs, 4)
+
+	jobType, err := workflow.Jobs["remote-reusable-workflow-missing-version"].Type()
+	assert.Equal(t, JobTypeInvalid, jobType)
+	assert.NotEqual(t, nil, err)
+
+	jobType, err = workflow.Jobs["remote-reusable-workflow-bad-extension"].Type()
+	assert.Equal(t, JobTypeInvalid, jobType)
+	assert.NotEqual(t, nil, err)
+
+	jobType, err = workflow.Jobs["local-reusable-workflow-bad-extension"].Type()
+	assert.Equal(t, JobTypeInvalid, jobType)
+	assert.NotEqual(t, nil, err)
+
+	jobType, err = workflow.Jobs["local-reusable-workflow-bad-path"].Type()
+	assert.Equal(t, JobTypeInvalid, jobType)
+	assert.NotEqual(t, nil, err)
 }

 func TestReadWorkflow_StepsTypes(t *testing.T) {
--- a/pkg/runner/run_context.go
+++ b/pkg/runner/run_context.go
@ -454,16 +454,19 @@ func (rc *RunContext) steps() []*model.Step {
 }

 // Executor returns a pipeline executor for all the steps in the job
-func (rc *RunContext) Executor() common.Executor {
+func (rc *RunContext) Executor() (common.Executor, error) {
 	var executor common.Executor
+	var jobType, err = rc.Run.Job().Type()

-	switch rc.Run.Job().Type() {
+	switch jobType {
 	case model.JobTypeDefault:
 		executor = newJobExecutor(rc, &stepFactoryImpl{}, rc)
 	case model.JobTypeReusableWorkflowLocal:
 		executor = newLocalReusableWorkflowExecutor(rc)
 	case model.JobTypeReusableWorkflowRemote:
 		executor = newRemoteReusableWorkflowExecutor(rc)
+	case model.JobTypeInvalid:
+		return nil, err
 	}

 	return func(ctx context.Context) error {
@ -475,7 +478,7 @@ func (rc *RunContext) Executor() common.Executor {
 			return executor(ctx)
 		}
 		return nil
-	}
+	}, nil
 }

 func (rc *RunContext) containerImage(ctx context.Context) string {
@ -528,19 +531,24 @@ func (rc *RunContext) options(_ context.Context) string {
 func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
 	job := rc.Run.Job()
 	l := common.Logger(ctx)
-	runJob, err := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess)
-	if err != nil {
-		return false, fmt.Errorf("  \u274C  Error in if-expression: \"if: %s\" (%s)", job.If.Value, err)
+	runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess)
+	jobType, jobTypeErr := job.Type()
+
+	if runJobErr != nil {
+		return false, fmt.Errorf("  \u274C  Error in if-expression: \"if: %s\" (%s)", job.If.Value, runJobErr)
 	}
+
+	if jobType == model.JobTypeInvalid {
+		return false, jobTypeErr
+	} else if jobType != model.JobTypeDefault {
+		return true, nil
+	}
+
 	if !runJob {
 		l.WithField("jobResult", "skipped").Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value)
 		return false, nil
 	}

-	if job.Type() != model.JobTypeDefault {
-		return true, nil
-	}
-
 	img := rc.platformImage(ctx)
 	if img == "" {
 		if job.RunsOn() == nil {
--- a/pkg/runner/runner.go
+++ b/pkg/runner/runner.go
@ -181,7 +181,13 @@ func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor {
 					}
 					stageExecutor = append(stageExecutor, func(ctx context.Context) error {
 						jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String())
-						return rc.Executor()(common.WithJobErrorContainer(WithJobLogger(ctx, rc.Run.JobID, jobName, rc.Config, &rc.Masks, matrix)))
+						executor, err := rc.Executor()
+
+						if err != nil {
+							return err
+						}
+
+						return executor(common.WithJobErrorContainer(WithJobLogger(ctx, rc.Run.JobID, jobName, rc.Config, &rc.Masks, matrix)))
 					})
 				}
 				pipeline = append(pipeline, common.NewParallelExecutor(maxParallel, stageExecutor...))