Updates from review

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

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.
 
 

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. Each finding includes a suggested patch to fix the respective script injection.
+  - 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

probes/hasDangerousWorkflowScriptInjection/impl.go

@@ -58,7 +58,7 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	}
 
 	var findings []finding.Finding
-	var curr string
+	var currWorkflow string
 	var workflow *actionlint.Workflow
 	var content []byte
 	var errs []*actionlint.Error
@@ -83,8 +83,9 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		findings = append(findings, *f)
 
 		wp := path.Join(localPath, e.File.Path)
-		if curr != wp {
-			curr = wp
+		if currWorkflow != wp {
+			// update current open file if injection in different file
+			currWorkflow = wp
 			content, err = os.ReadFile(wp)
 			if err != nil {
 				continue

probes/hasDangerousWorkflowScriptInjection/internal/patch/impl.go

@@ -61,10 +61,11 @@ func patchWorkflow(f checker.File, content []byte, workflow *actionlint.Workflow
 	unsafeVar := strings.TrimSpace(f.Snippet)
 
 	lines := bytes.Split(content, []byte("\n"))
-	if f.Offset == 0 || int(f.Offset) >= len(lines) {
+	runCmdIndex := int(f.Offset - 1)
+
+	if runCmdIndex < 0 || runCmdIndex >= len(lines) {
 		return []byte(""), sce.WithMessage(sce.ErrScorecardInternal, "Invalid dangerous workflow offset")
 	}
-	runCmdIndex := f.Offset - 1
 
 	unsafePattern, err := getUnsafePattern(unsafeVar)
 	if err != nil {
@@ -209,10 +210,10 @@ func useExistingEnvvars(
 }
 
 // Replaces all instances of the given script injection variable with the safe environment variable.
-func replaceUnsafeVarWithEnvvar(lines [][]byte, pattern unsafePattern, runIndex uint) {
+func replaceUnsafeVarWithEnvvar(lines [][]byte, pattern unsafePattern, runIndex int) {
 	runIndent := getIndent(lines[runIndex])
 	for i, line := range lines[runIndex:] {
-		currLine := int(runIndex) + i
+		currLine := runIndex + i
 		if i > 0 && isParentLevelIndent(lines[currLine], runIndent) {
 			// anything at the same indent as the first line of the  `- run:` block will mean the end of the run block.
 			break