🐛 stop an unterminated ${{ from aborting the DangerousWorkflow check

[Sahana2524]

What kind of change does this PR introduce?

Bug fix.

• PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

checkVariablesInScript in checks/raw/dangerous_workflow.go scans a step's run: body for ${{ ... }} expressions. When it finds a ${{ with no closing }}, it returns an internal error. That error bubbles up through validateScriptInjection and the OnMatchingFileContentDo callback, which stops iterating over the remaining workflow files and makes the whole Dangerous-Workflow check error out.

actionlint.Parse treats a run: body as an opaque string and does not validate expression syntax inside it, so an unterminated ${{ parses fine. The practical effect is that a single analyzed workflow file containing one unterminated expression suppresses script-injection detection for every other workflow in the repo, so a genuinely dangerous workflow can go unreported.

What is the new behavior (if this is a feature change)?

When there is no closing }}, the scan loop now breaks instead of returning an error. With no closing delimiter there is no complete expression left to inspect, so scanning stops for that one script and the rest of the check continues normally. Valid expressions in other steps and other files are still evaluated.

• Tests for the changes have been added (for bug fixes/features)

Added github-workflow-dangerous-pattern-untrusted-script-injection-unterminated.yml: its first step has an unterminated ${{ and its second step has a real github.event.issue.title injection. Before this change the check returned an error and reported 0 findings; now it reports the injection.

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

The error return on checkVariablesInScript is now always nil. I kept the signature as-is to keep the diff small and leave room for a future real error condition, but I am happy to drop it if you would rather see it removed.

Does this PR introduce a user-facing change?

NONE

[bilaldeveloper4312]

Reviewed commit 93054a05 with particular attention to whether the new break could hide a later finding.

The change is appropriately scoped to the current run script. An unterminated expression stops scanning only that script; validateScriptInjection continues with later steps and workflow files, which matches the added two-step regression fixture. Removing the now-unused Scorecard error import is also correct. I found no blocking issue in the code change.

Verification note: I ran go test ./checks/raw -run '^TestGithubDangerousWorkflow$' -count=1 on this commit and its parent on Windows. The parent already returns zero findings for several existing fixtures in this environment; the PR branch shows the same baseline limitation plus the new fixture, so I could not treat this local run as a clean pass or as evidence of a PR regression. CI on the supported environment should remain the authoritative test result.

[Sahana2524]

Agreed that CI on the supported runners is the signal to trust here. FWIW the full TestGithubDangerousWorkflow run passes for me on macOS on this branch, so the zero-finding baseline you saw on the parent is likely a Windows-only quirk (line endings in the checked-out testdata would be my first guess) rather than anything this change touches.