✨ Add machine-readable patch to fix script injections in workflows

cmd/internal/scdiff/app/runner/runner_test.go

@@ -46,6 +46,7 @@ func TestRunner_Run(t *testing.T) {
 	mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil)
 	mockRepo.EXPECT().Close().Return(nil)
 	mockRepo.EXPECT().GetFileReader(gomock.Any()).Return(nil, errors.New("reading files unsupported for this test")).AnyTimes()
+	mockRepo.EXPECT().LocalPath().Return(".", nil)
 	r := Runner{
 		ctx: context.Background(),
 		// use a check which works locally, but we declare no files above so no-op

docs/probes.md

@@ -194,7 +194,7 @@ If the probe finds no binary files, it returns a single OutcomeFalse.
 
 **Implementation**: The probe analyzes the repository's workflows for known dangerous patterns.
 
-**Outcomes**: The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected.
+**Outcomes**: The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected. Each finding may include a suggested patch to fix the respective script injection.
 If no dangerous patterns are found, the probe returns one finding with OutcomeFalse.
 
 

go.mod

@@ -150,7 +150,7 @@ require (
 	github.com/emirpasic/gods v1.18.1 // indirect
 	github.com/fatih/color v1.17.0 // indirect
 	github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect
-	github.com/go-git/go-billy/v5 v5.5.0 // indirect
+	github.com/go-git/go-billy/v5 v5.5.0
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang-jwt/jwt/v4 v4.5.1 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect

pkg/scorecard/scorecard.go

@@ -138,6 +138,11 @@
 
 	resultsCh := make(chan checker.CheckResult)
 
+	localPath, err := repoClient.LocalPath()
+	if err != nil {
+		return Result{}, fmt.Errorf("RepoClient.LocalPath: %w", err)
+	}
+
 	// Set metadata for all checks to use. This is necessary
 	// to create remediations from the probe yaml files.
 	ret.RawResults.Metadata.Metadata = map[string]string{
@@ -146,6 +151,7 @@
 		"repository.uri":           repo.URI(),
 		"repository.sha1":          commitSHA,
 		"repository.defaultBranch": defaultBranch,
+		"localPath":                localPath,
 	}
 
 	request := &checker.CheckRequest{

pkg/scorecard/scorecard_test.go

@@ -242,6 +242,7 @@ func TestRun_WithProbes(t *testing.T) {
 							"repository.name":          "ossf/scorecard",
 							"repository.sha1":          "1a17bb812fb2ac23e9d09e86e122f8b67563aed7",
 							"repository.uri":           "github.com/ossf/scorecard",
+							"localPath":                "test_path",
 						},
 					},
 				},
@@ -279,6 +280,9 @@ func TestRun_WithProbes(t *testing.T) {
 			t.Parallel()
 			ctrl := gomock.NewController(t)
 			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
+			mockRepoClient.EXPECT().LocalPath().DoAndReturn(func() (string, error) {
+				return "test_path", nil
+			}).AnyTimes()
 			repo := mockrepo.NewMockRepo(ctrl)
 
 			repo.EXPECT().URI().Return(tt.args.uri).AnyTimes()

probes/hasDangerousWorkflowScriptInjection/def.yml

@@ -20,7 +20,7 @@ motivation: >
 implementation: >
   The probe analyzes the repository's workflows for known dangerous patterns.
 outcome:
-  - The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected.
+  - The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected. Each finding may include a suggested patch to fix the respective script injection.
   - If no dangerous patterns are found, the probe returns one finding with OutcomeFalse.
 remediation:
   onOutcome: True
@@ -30,6 +30,11 @@ remediation:
   markdown:
     - Avoid the dangerous workflow patterns.
     - See [this document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) for information on avoiding and mitigating the risk of script injections.
+    - |
+      Here is a proposed patch to eliminate this risk:
+      ```yml
+      ${{ metadata.patch }}
+      ```
 ecosystem:
   languages:
     - all

probes/hasDangerousWorkflowScriptInjection/impl.go

@@ -18,11 +18,16 @@
 import (
 	"embed"
 	"fmt"
+	"os"
+	"path"
+
+	"github.com/rhysd/actionlint"
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
 	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
+	"github.com/ossf/scorecard/v5/probes/hasDangerousWorkflowScriptInjection/internal/patch"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
@@ -53,23 +58,37 @@
 	}
 
 	var findings []finding.Finding
-	for _, e := range r.Workflows {
-		e := e
-		if e.Type == checker.DangerousWorkflowScriptInjection {
-			f, err := finding.NewWith(fs, Probe,
-				fmt.Sprintf("script injection with untrusted input '%v'", e.File.Snippet),
-				nil, finding.OutcomeTrue)
-			if err != nil {
-				return nil, Probe, fmt.Errorf("create finding: %w", err)
-			}
-			f = f.WithLocation(&finding.Location{
-				Path:      e.File.Path,
-				Type:      e.File.Type,
-				LineStart: &e.File.Offset,
-				Snippet:   &e.File.Snippet,
-			})
-			findings = append(findings, *f)
+	var currWorkflow string
+	var workflow *actionlint.Workflow
+	var content []byte
+	var errs []*actionlint.Error
+	localPath := raw.Metadata.Metadata["localPath"]
+	for _, w := range r.Workflows {
+		w := w
+		if w.Type != checker.DangerousWorkflowScriptInjection {
+			continue
+		}
+
+		f, err := finding.NewWith(fs, Probe,
+			fmt.Sprintf("script injection with untrusted input '%v'", w.File.Snippet),
+			nil, finding.OutcomeTrue)
+		if err != nil {
+			return nil, Probe, fmt.Errorf("create finding: %w", err)
 		}
+
+		f = f.WithLocation(&finding.Location{
+			Path:      w.File.Path,
+			Type:      w.File.Type,
+			LineStart: &w.File.Offset,
+			Snippet:   &w.File.Snippet,
+		})
+
+		err = parseWorkflow(localPath, &w, &currWorkflow, &content, &workflow, &errs)
+		if err == nil {
+			generatePatch(&w, content, workflow, errs, f)
+		}
+
+		findings = append(findings, *f)
 	}
 
 	if len(findings) == 0 {
@@ -79,6 +98,50 @@
 	return findings, Probe, nil
 }
 
+func parseWorkflow(
+	localPath string,
+	e *checker.DangerousWorkflow,
+	currWorkflow *string,
+	content *[]byte,
+	workflow **actionlint.Workflow,
+	errs *[]*actionlint.Error,
+) error {
+	var err error
+	wp := path.Join(localPath, e.File.Path)
+	if *currWorkflow != wp {
+		// update current open file if injection in different file
+		*currWorkflow = wp
+		*content, err = os.ReadFile(wp)
+		if err != nil {
+			return err //nolint:wrapcheck // we only care about the error's existence
+		}
+
+		*workflow, *errs = actionlint.Parse(*content)
+		if len(*errs) > 0 && *workflow == nil {
+			// the workflow contains unrecoverable parsing errors, skip.
+			return err //nolint:wrapcheck // we only care about the error's existence
+		}
+	}
+	return nil
+}
+
+func generatePatch(
+	e *checker.DangerousWorkflow,
+	content []byte,
+	workflow *actionlint.Workflow,
+	errs []*actionlint.Error,
+	f *finding.Finding,
+) {
+	findingPatch, err := patch.GeneratePatch(e.File, content, workflow, errs)
+	if err != nil {
+		return
+	}
+	f.WithPatch(&findingPatch)
+	f.WithRemediationMetadata(map[string]string{
+		"patch": findingPatch,
+	})
+}
+
 func falseOutcome() ([]finding.Finding, string, error) {
 	f, err := finding.NewWith(fs, Probe,
 		"Project does not have dangerous workflow(s) with possibility of script injection.", nil,

probes/hasDangerousWorkflowScriptInjection/impl_test.go

@@ -43,6 +43,9 @@ func Test_Run(t *testing.T) {
 					Workflows: []checker.DangerousWorkflow{
 						{
 							Type: checker.DangerousWorkflowScriptInjection,
+							File: checker.File{
+								Path: "patch/testdata/userInputAssignedToVariable.yaml",
+							},
 						},
 					},
 				},