feat: Add JSON body support for incoming webhook parameters

* Implemented support for passing incoming webhook parameters in the JSON
request body.
* Marked the legacy method using URL query parameters for secrets as insecure.
* Added a warning log when the legacy method is detected.
* Updated documentation to describe the new recommended method and the
deprecated legacy method.
* Updated end-to-end tests to cover both legacy and new methods.

Fixes #1093
JIRA: https://issues.redhat.com/browse/SRVKP-7678

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

Author: chmouel

docs/content/docs/guide/incoming_webhook.md

@@ -87,19 +87,28 @@ stringData:
 
 After setting this up, you will be able to start the PipelineRun with a POST
 request sent to the controller URL appended with /incoming. The request
-includes the very-secure-shared-secret, the repository name (repo), the target
-branch (main), and the PipelineRun name.
+can include the very-secure-shared-secret, the repository name (repo), the
+target branch (main), and the PipelineRun name either as URL query parameters
+(legacy, insecure, and deprecated) or in the POST JSON body (recommended).
 
-You can use the `generateName` field as the PipelineRun name but you will need to make sure to specify the hyphen (-) at the end.
+You can use the `generateName` field as the PipelineRun name but you will need
+to make sure to specify the hyphen (-) at the end.
 
-As an example here is a curl snippet starting the PipelineRun:
+#### Legacy (URL query) method (deprecated)
 
 ```shell
 curl -X POST 'https://control.pac.url/incoming?secret=very-secure-shared-secret&repository=repo&branch=main&pipelinerun=target-pipelinerun'
 ```
 
-in this snippet, note two things the `"/incoming"` path to the controller URL
-and the `"POST"` method to the URL rather than a simple `"GET"`.
+**Warning:** Passing secrets in the URL is insecure and will be deprecated. Please use the POST body method below.
+
+#### Recommended (POST JSON body) method
+
+```shell
+curl -H "Content-Type: application/json" -X POST "https://control.pac.url/incoming" -d '{"repository":"repo","branch":"main","pipelinerun":"target-pipelinerun","secret":"very-secure-shared-secret"}'
+```
+
+In both cases, the `"/incoming"` path to the controller URL and the `"POST"` method will remain unchanged.
 
 It is important to note that when the PipelineRun is triggered, Pipelines as
 Code will treat it as a push event and will have the capability to report the
@@ -110,17 +119,17 @@ provides guidance on how to achieve this.
 
 ### Passing dynamic parameter value to incoming webhook
 
-You can define the value of a any Pipelines-as Code Parameters (including
-redefining the [builtin ones](../authoringprs#default-parameters).
+You can define the value of any Pipelines-as-Code Parameters (including
+redefining the [builtin ones](../authoringprs#default-parameters)).
 
 You need to list the overridden or added params in the params section of the
-Repo CR configuration and pass the value in the json body of the incoming webhook
+Repo CR configuration and pass the value in the JSON body of the incoming webhook
 request.
 
-You will need to pass the `content-type` as `application/json` in the header of
+You will need to pass the `Content-Type` as `application/json` in the header of
 your URL request.
 
-Here is a Repository CR letting passing the `pull_request_number` dynamic variable:
+Here is a Repository CR allowing passing the `pull_request_number` dynamic variable:
 
 ```yaml
 ---
@@ -144,10 +153,11 @@ spec:
 and here is a curl snippet passing the `pull_request_number` value:
 
 ```shell
-curl -H "Content-Type: application/json" -X POST "https://control.pac.url/incoming?repository=repo&branch=main&secret=very-secure-shared-secret&pipelinerun=target-pipelinerun" -d '{"params": {"pull_request_number": "12345"}}'
+curl -H "Content-Type: application/json" -X POST "https://control.pac.url/incoming" -d '{"repository":"repo","branch":"main","pipelinerun":"target-pipelinerun","secret":"very-secure-shared-secret","params": {"pull_request_number": "12345"}}'
 ```
 
-The parameter value of `pull_request_number` will be set to `12345` when using the variable `{{pull_request_number}}` in your PipelineRun.
+The parameter value of `pull_request_number` will be set to `12345` when using
+the variable `{{pull_request_number}}` in your PipelineRun.
 
 ### Using incoming webhook with GitHub Enterprise application
 

pkg/adapter/incoming.go

@@ -3,6 +3,7 @@ package adapter
 import (
 	"context"
 	"crypto/subtle"
+	"encoding/json"
 	"fmt"
 	"net/http"
 
@@ -50,14 +51,46 @@ func applyIncomingParams(req *http.Request, payloadBody []byte, params []string)
 }
 
 func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloadBody []byte) (bool, *v1alpha1.Repository, error) {
+	// Support both legacy (URL query) and new (POST body) secret passing
 	repository := req.URL.Query().Get("repository")
-	querySecret := req.URL.Query().Get("secret")
-	pipelineRun := req.URL.Query().Get("pipelinerun")
 	branch := req.URL.Query().Get("branch")
+	pipelineRun := req.URL.Query().Get("pipelinerun")
+	querySecret := req.URL.Query().Get("secret")
+	legacyMode := false
 
 	if req.URL.Path != "/incoming" {
 		return false, nil, nil
 	}
+
+	// If not all required query params are present, try to parse from JSON body
+	if repository == "" || branch == "" || pipelineRun == "" || querySecret == "" {
+		if req.Method == http.MethodPost && req.Header.Get("Content-Type") == "application/json" && len(payloadBody) > 0 {
+			var body struct {
+				Repository  string         `json:"repository"`
+				Branch      string         `json:"branch"`
+				PipelineRun string         `json:"pipelinerun"`
+				Secret      string         `json:"secret"`
+				Params      map[string]any `json:"params"`
+			}
+			if err := json.Unmarshal(payloadBody, &body); err == nil {
+				repository = body.Repository
+				branch = body.Branch
+				pipelineRun = body.PipelineRun
+				querySecret = body.Secret
+			} else {
+				return false, nil, fmt.Errorf("invalid JSON body for incoming webhook: %w", err)
+			}
+		} else {
+			return false, nil, fmt.Errorf("missing query URL argument: pipelinerun, branch, repository, secret: '%s' '%s' '%s' '%s'", pipelineRun, branch, repository, querySecret)
+		}
+	} else {
+		legacyMode = true
+	}
+
+	if legacyMode {
+		l.logger.Warnf("[SECURITY] Incoming webhook used legacy URL-based secret passing. This is insecure and will be deprecated. Please use POST body instead.")
+	}
+
 	l.logger.Infof("incoming request has been requested: %v", req.URL)
 	if pipelineRun == "" || repository == "" || querySecret == "" || branch == "" {
 		err := fmt.Errorf("missing query URL argument: pipelinerun, branch, repository, secret: '%s' '%s' '%s' '%s'", pipelineRun, branch, repository, querySecret)

pkg/adapter/incoming_test.go

@@ -182,7 +182,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 						},
 					},
 				},
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -223,7 +223,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 						},
 					},
 				},
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -235,7 +235,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 		{
 			name: "invalid incoming body",
 			args: args{
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -563,7 +563,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 						},
 					},
 				},
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -600,7 +600,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 						},
 					},
 				},
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -641,7 +641,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 						},
 					},
 				},
-				method:           "POST",
+				method:           http.MethodPost,
 				queryURL:         "/incoming",
 				queryRepository:  "test-good",
 				querySecret:      "verysecrete",
@@ -913,3 +913,159 @@ func TestApplyIncomingParams(t *testing.T) {
 		})
 	}
 }
+
+func Test_detectIncoming_legacy_warning(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	testNamespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "pipelinesascode",
+		},
+	}
+	ctx = info.StoreCurrentControllerName(ctx, "default")
+	ctx = info.StoreNS(ctx, testNamespace.GetName())
+	cs, _ := testclient.SeedTestData(t, ctx, testclient.Data{
+		Repositories: []*v1alpha1.Repository{
+			{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-good"},
+				Spec: v1alpha1.RepositorySpec{
+					URL: "https://matched/by/incoming",
+					Incomings: &[]v1alpha1.Incoming{{
+						Targets: []string{"main"},
+						Secret:  v1alpha1.Secret{Name: "good-secret"},
+						Params:  []string{"foo", "bar"},
+					}},
+					GitProvider: &v1alpha1.GitProvider{Type: "github"},
+				},
+			},
+		},
+	})
+	client := &params.Run{
+		Clients: clients.Clients{
+			PipelineAsCode: cs.PipelineAsCode,
+			Kube:           cs.Kube,
+		},
+		Info: info.Info{
+			Controller: &info.ControllerInfo{Secret: info.DefaultPipelinesAscodeSecretName},
+		},
+	}
+	tests := []struct {
+		name          string
+		req           *http.Request
+		body          []byte
+		expectWarning bool
+	}{
+		{
+			name: "legacy mode - params in URL",
+			req: httptest.NewRequest(http.MethodPost,
+				"http://localhost/incoming?repository=test-good&secret=verysecrete&pipelinerun=pipelinerun1&branch=main",
+				strings.NewReader("")),
+			body:          nil,
+			expectWarning: true,
+		},
+		{
+			name: "new mode - params in JSON body",
+			req: func() *http.Request {
+				payload := `{
+					"repository": "test-good",
+					"branch": "main",
+					"pipelinerun": "pipelinerun2",
+					"secret": "verysecrete",
+					"params": {"foo": "bar"}
+				}`
+				r := httptest.NewRequest(http.MethodPost,
+					"http://localhost/incoming",
+					strings.NewReader(payload))
+				r.Header.Set("Content-Type", "application/json")
+				return r
+			}(),
+			body:          []byte(`{"repository":"test-good","branch":"main","pipelinerun":"pipelinerun2","secret":"verysecrete","params":{"foo":"bar"}}`),
+			expectWarning: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			observer, observedLogs := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
+			kint := &kubernetestint.KinterfaceTest{GetSecretResult: map[string]string{"good-secret": "verysecrete"}}
+			l := &listener{
+				run:    client,
+				logger: logger,
+				kint:   kint,
+				event:  info.NewEvent(),
+			}
+			got, _, err := l.detectIncoming(ctx, tt.req, tt.body)
+			assert.NilError(t, err)
+			assert.Assert(t, got)
+			found := false
+			for _, entry := range observedLogs.All() {
+				if strings.Contains(entry.Message, "[SECURITY] Incoming webhook used legacy URL-based secret passing") {
+					found = true
+					break
+				}
+			}
+			if tt.expectWarning {
+				assert.Assert(t, found, "expected security warning log for legacy URL-based secret passing")
+			} else {
+				assert.Assert(t, !found, "did not expect security warning log for new mode")
+			}
+		})
+	}
+}
+
+func Test_detectIncoming_body_params_are_parsed(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	testNamespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "pipelinesascode",
+		},
+	}
+	ctx = info.StoreCurrentControllerName(ctx, "default")
+	ctx = info.StoreNS(ctx, testNamespace.GetName())
+	cs, _ := testclient.SeedTestData(t, ctx, testclient.Data{
+		Repositories: []*v1alpha1.Repository{
+			{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-good"},
+				Spec: v1alpha1.RepositorySpec{
+					URL: "https://matched/by/incoming",
+					Incomings: &[]v1alpha1.Incoming{{
+						Targets: []string{"main"},
+						Secret:  v1alpha1.Secret{Name: "good-secret"},
+						Params:  []string{"foo", "bar"},
+					}},
+					GitProvider: &v1alpha1.GitProvider{Type: "github"},
+				},
+			},
+		},
+	})
+	client := &params.Run{
+		Clients: clients.Clients{
+			PipelineAsCode: cs.PipelineAsCode,
+			Kube:           cs.Kube,
+		},
+		Info: info.Info{
+			Controller: &info.ControllerInfo{Secret: info.DefaultPipelinesAscodeSecretName},
+		},
+	}
+	payload := `{
+		"repository": "test-good",
+		"branch": "main",
+		"pipelinerun": "pipelinerun2",
+		"secret": "verysecrete",
+		"params": {"foo": "bar", "bar": "baz"}
+	}`
+	req := httptest.NewRequest(http.MethodPost,
+		"http://localhost/incoming",
+		strings.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+	kint := &kubernetestint.KinterfaceTest{GetSecretResult: map[string]string{"good-secret": "verysecrete"}}
+	l := &listener{
+		run:    client,
+		logger: zap.NewNop().Sugar(),
+		kint:   kint,
+		event:  info.NewEvent(),
+	}
+	got, _, err := l.detectIncoming(ctx, req, []byte(payload))
+	assert.NilError(t, err)
+	assert.Assert(t, got)
+}

test/github_incoming_test.go

@@ -5,6 +5,7 @@ package test
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -119,12 +120,35 @@ func verifyIncomingWebhook(t *testing.T, randomedString, pipelinerunName string,
 	assert.NilError(t, err)
 	runcnx.Clients.Log.Infof("Commit %s has been created and pushed to branch %s", sha, vref.GetURL())
 
-	incomingURL := fmt.Sprintf("%s/incoming?repository=%s&branch=%s&pipelinerun=%s&secret=%s", opts.ControllerURL,
-		randomedString, randomedString, pipelinerunName, incomingSecreteValue)
-	body := `{"params":{"the_best_superhero_is":"Superman"}}`
+	var req *http.Request
+	var incomingURL string
 	client := &http.Client{}
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, incomingURL, strings.NewReader(body))
-	req.Header.Add("Content-Type", "application/json")
+
+	if onWebhook {
+		// Legacy URL query parameters method
+		incomingURL = fmt.Sprintf("%s/incoming?repository=%s&branch=%s&pipelinerun=%s&secret=%s",
+			opts.ControllerURL, randomedString, randomedString, pipelinerunName, incomingSecreteValue)
+		req, err = http.NewRequestWithContext(ctx, http.MethodPost, incomingURL, strings.NewReader(`{"params":{"the_best_superhero_is":"Superman"}}`))
+		assert.NilError(t, err)
+		req.Header.Add("Content-Type", "application/json")
+	} else {
+		// JSON body method
+		incomingURL = fmt.Sprintf("%s/incoming", opts.ControllerURL)
+		jsonBody := map[string]interface{}{
+			"repository":  randomedString,
+			"branch":      randomedString,
+			"pipelinerun": pipelinerunName,
+			"secret":      incomingSecreteValue,
+			"params": map[string]string{
+				"the_best_superhero_is": "Superman",
+			},
+		}
+		jsonData, err := json.Marshal(jsonBody)
+		assert.NilError(t, err)
+		req, err = http.NewRequestWithContext(ctx, http.MethodPost, incomingURL, strings.NewReader(string(jsonData)))
+		assert.NilError(t, err)
+		req.Header.Add("Content-Type", "application/json")
+	}
 	if onSecondController {
 		urlParse, _ := url.Parse(*ghprovider.APIURL)
 		req.Header.Add("X-GitHub-Enterprise-Host", urlParse.Host)