🐛 detect dangerous patterns in toJSON() in dangerous workflows

[AdamKorcz]

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

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

What is the current behavior?

Scorecard misses dangerous patterns when they are wrapped in toJSON().

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

Scorecard detects dangerous patterns when they are wrapped in toJSON().

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

Which issue(s) this PR fixes

#3554

Special notes for your reviewer

Does this PR introduce a user-facing change?

Yes

Scorecard detects dangerous workflow patterns when wrapped in `toJSON()`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.97%. Comparing base (353ed60) to head (6b9ba75).
⚠️ Report is 260 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4717      +/-   ##
==========================================
+ Coverage   66.80%   69.97%   +3.17%     
==========================================
  Files         230      249      +19     
  Lines       16602    20274    +3672     
==========================================
+ Hits        11091    14187    +3096     
- Misses       4808     5214     +406     
- Partials      703      873     +170     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kailealee]

Hey @AdamKorcz , we attempted to run your code via Codespaces, however it still passes on this test repo that we made which we think contains dangerous workflow. We were interested in working on Issue #3554, but we noticed that you recently submitted a PR so we came to see your approach on solving it. Let us know if you want us to test again.

My team includes @purpleskates123, @jakbrownbytes, @dcaine125, @smrghsh and @devon3583.

[AdamKorcz]

Hi @kailealee Thank you for reviewing. I believe the reason for that is unrelated to the current problem. Scorecard makes the following check return strings.Contains(variable, "github.event.") which your test file will return false against since you print out github.event (without the trailing dot). Could you test it with any of the sub fields from here such as github.event.issue.title.

I will be happy to modify the workflow patterns so it also includes github.event.

[kailealee]

Hey @AdamKorcz, my team and I retested our test repo using one of your sub fields. Our first test failed because we used a sub field that wasn’t included in your code. After that, we ran the test again using the correct sub field, "pull_request.head.label", it ran correctly. Without the right sub field, we got a 10/10 score, but now with the correct one, it ran as expected and gave us a 0/10 score since we only have a dangerous workflow in our test repo.

My team includes @purpleskates123, @smrghsh, and @devon3583.

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity

[AdamKorcz]

Please reopen.

[spencerschrock]

this doesn't work as expected for the small example Pedro showed in his issue. I was curious about toJSON(github), which also ends up being vulnerable. But not every toJSON call would be vulnerable, only examples that are less specific. For example
toJSON(github.event.pull_request.state) should be declared as safe (though this is contrived)

Here's a patch with a test I was trying. As well as the example in my test repo (workflow now disabled):
https://github.com/spencerschrock/actions-test/actions/runs/17951418605/job/51051442328

diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go
index 15befe75..58e4fb04 100644
--- a/checks/dangerous_workflow_test.go
+++ b/checks/dangerous_workflow_test.go
@@ -72,6 +72,15 @@ func TestDangerousWorkflow(t *testing.T) {
 				Score: checker.MaxResultScore,
 			},
 		},
+		{
+			name:          "toJSON injection is a failing score",
+			workflowPaths: []string{".github/workflows/github-workflow-dangerous-toJSON.yml"},
+			err:           nil,
+			expected: scut.TestReturn{
+				Score:        checker.MinResultScore,
+				NumberOfWarn: 2,
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-toJSON.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-toJSON.yml
new file mode 100644
index 00000000..c986aa08
--- /dev/null
+++ b/checks/testdata/.github/workflows/github-workflow-dangerous-toJSON.yml
@@ -0,0 +1,11 @@
+on:
+  pull_request_target:
+
+jobs:
+  get-pwned:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Log event and get pwned
+        run: |
+          echo '${{ toJSON(github.event) }}' | jq
+          echo '${{ toJSON(github) }}' | jq

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity