feat: Require SHA for /ok-to-test comments

Introduced a new configuration option `require-ok-to-test-sha` that
enforces the use of a commit SHA when using the `/ok-to-test` command.
This helps to mitigate race conditions where a malicious actor could
push a commit after the `/ok-to-test` comment is made but before the CI
runs.

The regular expression for parsing `/ok-to-test` comments was updated to
capture an optional SHA. New tests were added to cover various
scenarios, including valid and invalid SHAs, and the absence of a SHA
when required. The `ParsePayload` function was modified to handle this
new validation logic, and appropriate error messages were created for
cases where the SHA is missing or does not match the pull request's HEAD
SHA.

Co-authored-by: Cursor
Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

config/302-pac-configmap.yaml

@@ -164,6 +164,13 @@ data:
   # you may want to disable this if ok-to-test should be done on each iteration
   remember-ok-to-test: "false"
 
+  # require-ok-to-test-sha enforces that a pull request's commit SHA must be specified
+  # in an `/ok-to-test` comment. This prevents a race condition where a malicious
+  # user could push a bad commit after the `/ok-to-test` comment is posted but
+  # before the CI runs.
+  # Default: false
+  require-ok-to-test-sha: "false"
+
   # When enabled, this option prevents duplicate pipeline runs when a commit appears in
   # both a push event and a pull request. If a push event comes from a commit that is
   # part of an open pull request, the push event will be skipped as it would create

docs/content/docs/guide/gitops_commands.md

@@ -39,6 +39,41 @@ This will always trigger a new PipelineRun, even if previous runs were successfu
 
 Similar to `/retest`, the `/ok-to-test` command will only trigger new PipelineRuns if no successful PipelineRun already exists for the same commit. This prevents duplicate runs when repository owners repeatedly test the same commit by `/test` and `/retest` command.
 
+### Requiring a SHA with `/ok-to-test`
+
+{{< tech_preview "Requiring a SHA argument to `/ok-to-test`" >}}
+{{< support_matrix github_app="true" github_webhook="false" gitea="false" gitlab="false" bitbucket_cloud="false" bitbucket_datacenter="false" >}}
+
+Cluster administrators can enforce SHA validation on `/ok-to-test` by setting
+`require-ok-to-test-sha: "true"` in the Pipelines-as-Code ConfigMap. This
+feature currently applies only to GitHub, as its `issue_comment` event does not
+include the pull request’s HEAD SHA (unlike other providers that do).
+
+Without this SHA, a small timing window exists where an attacker could push a
+new commit immediately after an owner comments `/ok-to-test`, causing CI to run
+on unintended code. Requiring the reviewer to include the commit ID eliminates
+this risk until GitHub includes the SHA in its webhook payload.
+
+When enabled, repository owners and collaborators must append a 7–40 character
+Git SHA (in lowercase or uppercase hexadecimal) to the command, for example:
+
+```text
+/ok-to-test 1A2B3C4
+```
+
+Pipelines-as-Code verifies the provided SHA against the pull request’s current HEAD:
+
+- Short SHAs must match the HEAD commit’s prefix.
+- Full SHAs must match exactly.
+
+If the SHA is missing or invalid, the comment is rejected, and the bot replies
+with instructions to retry using the correct value. This mechanism protects
+GitHub repositories from the time-of-check/time-of-use vulnerability,
+a risk that other providers avoid by including the commit SHA directly in the
+webhook payload.
+
+### Targeting Specific PipelineRuns
+
 If you have multiple `PipelineRun` and you want to target a specific `PipelineRun`, you can use the `/test` command followed by the specific PipelineRun name to restart it. Example:
 
 ```text

pkg/acl/regexp.go

@@ -4,7 +4,7 @@ import (
 	"regexp"
 )
 
-const OKToTestCommentRegexp = `(^|\n)\/ok-to-test(\r\n|\r|\n|$)`
+const OKToTestCommentRegexp = `(^|\n)\/ok-to-test(?:\s+([a-fA-F0-9]{7,40}))?\s*(\r\n|\r|\n|$)`
 
 // MatchRegexp Match a regexp to a string.
 func MatchRegexp(reg, comment string) bool {

pkg/acl/regexp_test.go

@@ -22,6 +22,26 @@ func TestRegexp(t *testing.T) {
 			text:    "/ok-to-test",
 			matched: true,
 		},
+		{
+			name:    "good/match regexp with short sha",
+			text:    "/ok-to-test 1234567",
+			matched: true,
+		},
+		{
+			name:    "good/match regexp with uppercase sha",
+			text:    "/ok-to-test ABCDEF1",
+			matched: true,
+		},
+		{
+			name:    "good/match regexp with full sha",
+			text:    "/ok-to-test 1234567890123456789012345678901234567890",
+			matched: true,
+		},
+		{
+			name:    "bad/match regexp with invalid sha",
+			text:    "/ok-to-test GGGGGGG",
+			matched: false,
+		},
 		{
 			name:    "good/match regexp newline",
 			text:    "\n/ok-to-test",
@@ -32,6 +52,11 @@ func TestRegexp(t *testing.T) {
 			text:    "\n /ok-to-test",
 			matched: false,
 		},
+		{
+			name:    "good/match regexp trailing spaces",
+			text:    "/ok-to-test   \n",
+			matched: true,
+		},
 		{
 			name:    "good/in the middle",
 			text:    "foo bar\n/ok-to-test\nhello moto",

pkg/opscomments/comments.go

@@ -7,6 +7,7 @@ import (
 
 	"go.uber.org/zap"
 
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/acl"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
@@ -18,7 +19,7 @@ var (
 	retestAllRegex    = regexp.MustCompile(`(?m)^/retest\s*$`)
 	testSingleRegex   = regexp.MustCompile(`(?m)^/test[ \t]+\S+`)
 	retestSingleRegex = regexp.MustCompile(`(?m)^/retest[ \t]+\S+`)
-	oktotestRegex     = regexp.MustCompile(`(?m)^/ok-to-test\s*$`)
+	oktotestRegex     = regexp.MustCompile(acl.OKToTestCommentRegexp)
 	cancelAllRegex    = regexp.MustCompile(`(?m)^(/cancel)\s*$`)
 	cancelSingleRegex = regexp.MustCompile(`(?m)^(/cancel)[ \t]+\S+`)
 )
@@ -88,6 +89,15 @@ func IsOkToTestComment(comment string) bool {
 	return oktotestRegex.MatchString(comment)
 }
 
+// GetSHAFromOkToTestComment extracts the optional SHA from an /ok-to-test comment.
+func GetSHAFromOkToTestComment(comment string) string {
+	matches := oktotestRegex.FindStringSubmatch(comment)
+	if len(matches) > 2 {
+		return strings.TrimSpace(matches[2])
+	}
+	return ""
+}
+
 // EventTypeBackwardCompat handle the backward compatibility we need to keep until
 // we have done the deprecated notice
 //

pkg/opscomments/comments_test.go

@@ -306,9 +306,9 @@ func TestIsOkToTestComment(t *testing.T) {
 			want:    false,
 		},
 		{
-			name:    "invalid comment",
-			comment: "/ok-to-test abc",
-			want:    false,
+			name:    "valid comment with sha",
+			comment: "/ok-to-test 1234567",
+			want:    true,
 		},
 	}
 
@@ -679,6 +679,41 @@ func TestGetPipelineRunAndBranchNameFromCancelComment(t *testing.T) {
 	}
 }
 
+func TestGetSHAFromOkToTestComment(t *testing.T) {
+	tests := []struct {
+		name    string
+		comment string
+		want    string
+	}{
+		{
+			name:    "no sha",
+			comment: "/ok-to-test",
+			want:    "",
+		},
+		{
+			name:    "short sha",
+			comment: "/ok-to-test 1234567",
+			want:    "1234567",
+		},
+		{
+			name:    "full sha",
+			comment: "/ok-to-test 1234567890123456789012345678901234567890",
+			want:    "1234567890123456789012345678901234567890",
+		},
+		{
+			name:    "sha with surrounding text",
+			comment: "lgtm\n/ok-to-test 1234567\napproved",
+			want:    "1234567",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := GetSHAFromOkToTestComment(tt.comment)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestAnyOpsKubeLabelInSelector(t *testing.T) {
 	assert.Assert(t, strings.Contains(AnyOpsKubeLabelInSelector(), RetestSingleCommentEventType.String()))
 }

pkg/params/settings/config.go

@@ -81,7 +81,8 @@ type Settings struct {
 	CustomConsolePRTaskLog    string `json:"custom-console-url-pr-tasklog"`
 	CustomConsoleNamespaceURL string `json:"custom-console-url-namespace"`
 
-	RememberOKToTest bool `json:"remember-ok-to-test"`
+	RememberOKToTest   bool `json:"remember-ok-to-test"`
+	RequireOkToTestSHA bool `json:"require-ok-to-test-sha"`
 }
 
 func (s *Settings) DeepCopy(out *Settings) {

pkg/params/settings/config_test.go

@@ -77,6 +77,7 @@ func TestSyncConfig(t *testing.T) {
 				"custom-console-url-namespace":            "https://custom-console-namespace",
 				"remember-ok-to-test":                     "false",
 				"skip-push-event-for-pr-commits":          "true",
+				"require-ok-to-test-sha":                  "true",
 			},
 			expectedStruct: Settings{
 				ApplicationName:                      "pac-pac",
@@ -107,6 +108,7 @@ func TestSyncConfig(t *testing.T) {
 				CustomConsolePRTaskLog:               "https://custom-console-pr-tasklog",
 				CustomConsoleNamespaceURL:            "https://custom-console-namespace",
 				RememberOKToTest:                     false,
+				RequireOkToTestSHA:                   true,
 			},
 		},
 		{

pkg/provider/github/acl_test.go

@@ -346,6 +346,157 @@ func TestOkToTestComment(t *testing.T) {
 	}
 }
 
+func TestOkToTestCommentSHA(t *testing.T) {
+	tests := []struct {
+		name                   string
+		commentsReply          string
+		commentBody            string
+		runevent               info.Event
+		allowed                bool
+		wantErr                bool
+		requireOkToTestSHA     bool
+		pullRequestReply       string
+		pullRequestListComment string
+	}{
+		{
+			name:               "good issue comment event with sha",
+			commentsReply:      `[{"body": "/ok-to-test ABCDEF1", "user": {"login": "owner"}}]`,
+			commentBody:        "/ok-to-test ABCDEF1",
+			pullRequestReply:   `{"head": {"sha": "abcdef1234567890"}}`,
+			requireOkToTestSHA: true,
+			runevent: info.Event{
+				Organization: "owner",
+				Sender:       "nonowner",
+				EventType:    "issue_comment",
+				Event: &github.IssueCommentEvent{
+					Issue: &github.Issue{
+						PullRequestLinks: &github.PullRequestLinks{
+							HTMLURL: github.Ptr("http://url.com/owner/repo/1"),
+						},
+					},
+				},
+			},
+			allowed: true,
+			wantErr: false,
+		},
+		{
+			name:               "bad issue comment event with sha",
+			commentsReply:      `[{"body": "/ok-to-test 1234567", "user": {"login": "owner"}}]`,
+			commentBody:        "/ok-to-test 1234567",
+			pullRequestReply:   `{"head": {"sha": "7654321"}}`,
+			requireOkToTestSHA: true,
+			runevent: info.Event{
+				Organization: "owner",
+				Sender:       "nonowner",
+				EventType:    "issue_comment",
+				Event: &github.IssueCommentEvent{
+					Issue: &github.Issue{
+						PullRequestLinks: &github.PullRequestLinks{
+							HTMLURL: github.Ptr("http://url.com/owner/repo/1"),
+						},
+					},
+				},
+			},
+			allowed: false,
+			wantErr: true,
+		},
+		{
+			name:               "good issue comment event without sha",
+			commentsReply:      `[{"body": "/ok-to-test", "user": {"login": "owner"}}]`,
+			commentBody:        "/ok-to-test",
+			pullRequestReply:   `{"head": {"sha": "1234567890"}}`,
+			requireOkToTestSHA: false,
+			runevent: info.Event{
+				Organization: "owner",
+				Sender:       "nonowner",
+				EventType:    "issue_comment",
+				Event: &github.IssueCommentEvent{
+					Issue: &github.Issue{
+						PullRequestLinks: &github.PullRequestLinks{
+							HTMLURL: github.Ptr("http://url.com/owner/repo/1"),
+						},
+					},
+				},
+			},
+			allowed: true,
+			wantErr: false,
+		},
+		{
+			name:               "bad issue comment event without sha when required",
+			commentsReply:      `[{"body": "/ok-to-test", "user": {"login": "owner"}}]`,
+			commentBody:        "/ok-to-test",
+			pullRequestReply:   `{"head": {"sha": "1234567890"}}`,
+			requireOkToTestSHA: true,
+			runevent: info.Event{
+				Organization: "owner",
+				Sender:       "nonowner",
+				EventType:    "issue_comment",
+				Event: &github.IssueCommentEvent{
+					Issue: &github.Issue{
+						PullRequestLinks: &github.PullRequestLinks{
+							HTMLURL: github.Ptr("http://url.com/owner/repo/1"),
+						},
+					},
+				},
+			},
+			allowed: false,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.runevent.TriggerTarget = "ok-to-test-comment"
+			fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
+			defer teardown()
+			mux.HandleFunc("/repos/owner/issues/comments/0", func(rw http.ResponseWriter, _ *http.Request) {
+				fmt.Fprint(rw, tt.commentsReply)
+			})
+			mux.HandleFunc("/repos/owner/repo/pulls/1", func(rw http.ResponseWriter, _ *http.Request) {
+				fmt.Fprint(rw, tt.pullRequestReply)
+			})
+			mux.HandleFunc("/repos/owner/repo/issues/1/comments", func(rw http.ResponseWriter, r *http.Request) {
+				// this will test if pagination works okay
+				if r.URL.Query().Get("page") == "" || r.URL.Query().Get("page") == "1" {
+					rw.Header().Add("Link", `<https://api.github.com/owner/repo/issues/1/comments?page=2&per_page=1>; rel="next"`)
+					fmt.Fprint(rw, `[{"body": "Foo Bar", "user": {"login": "notallowed"}}]`, tt.commentsReply)
+				} else {
+					fmt.Fprint(rw, tt.commentsReply)
+				}
+			})
+			mux.HandleFunc("/repos/owner/collaborators", func(rw http.ResponseWriter, _ *http.Request) {
+				fmt.Fprint(rw, "[]")
+			})
+			ctx, _ := rtesting.SetupFakeContext(t)
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
+			repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{
+				Settings: &v1alpha1.Settings{},
+			}}
+			pacopts := &info.PacOpts{
+				Settings: settings.Settings{
+					RequireOkToTestSHA: tt.requireOkToTestSHA,
+				},
+			}
+			gprovider := Provider{
+				ghClient:      fakeclient,
+				repo:          repo,
+				Logger:        logger,
+				PaginedNumber: 1,
+				Run:           &params.Run{},
+				pacInfo:       pacopts,
+			}
+
+			payload := fmt.Sprintf(`{"action": "created", "repository": {"name": "repo", "owner": {"login": "owner"}}, "sender": {"login": %q}, "issue": {"pull_request": {"html_url": "https://github.com/owner/repo/pull/1"}}, "comment": {"body": %q}}`,
+				tt.runevent.Sender, tt.commentBody)
+			_, err := gprovider.ParsePayload(ctx, gprovider.Run, &http.Request{Header: http.Header{"X-Github-Event": []string{"issue_comment"}}}, payload)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("aclCheck() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+		})
+	}
+}
+
 func TestAclCheckAll(t *testing.T) {
 	fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
 	defer teardown()