Validate Repository URL

When creating the repository, the URL must be set and must have a valid scheme (http or https). Let's add a check on the webhook to make sure the user has provider a proper URL. We make the global repository URL mandatory compared to before, but since we are TP for global repository, we can make still change that behaviour. Fixes #1852 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
openshift-pipelines · Feb 12, 2025 · 64e83d5 · 64e83d5
1 parent 022f348
commit 64e83d5
Show file tree

Hide file tree

Showing 5 changed files with 182 additions and 56 deletions.
diff --git a/docs/content/docs/guide/repositorycrd.md b/docs/content/docs/guide/repositorycrd.md
@@ -15,14 +15,19 @@ The Repository CR serves the following purposes:
 - Letting you declare [custom parameters]({{< relref "/docs/guide/customparams" >}})
   within the `PipelineRun` that can be expanded based on certain filters.
 
-The process involves creating a Repository CR inside the target namespace
-my-pipeline-ci, using the tkn pac CLI or another method.
+To configure Pipelines-as-Code, a Repository CR must be created within the
+user's namespace, for example `project-repository`, where their CI will run.
 
-For example, this will create a Repo CR for the GitHub repository
-<https://github.com/linda/project>
+Keep in mind that creating a Repository CR in the namespace where
+Pipelines-as-Code is deployed is not supported (for example
+`openshift-pipelines` or `pipelines-as-code` namespace).
+
+You can create the Repository CR using the [tkn pac]({{< relref
+"/docs/guide/cli.md" >}}) CLI and its `tkn pac create repository` command or by
+applying a YAML file with kubectl:
 
 ```yaml
-cat <<EOF|kubectl create -n my-pipeline-ci -f-
+cat <<EOF|kubectl create -n project-repository -f-
 apiVersion: "pipelinesascode.tekton.dev/v1alpha1"
 kind: Repository
 metadata:
@@ -42,6 +47,20 @@ specific branch and event like a `push` or `pull_request`, it will start the
 `PipelineRun` where the `Repository` CR has been created. You can only start the
 `PipelineRun` in the namespace where the Repository CR is located.
 
+{{< hint info >}}
+Pipelines-as-Code uses a Kubernetes Mutating Admission Webhook to enforce a
+single Repository CRD per URL in the cluster and to ensure that URLs are valid
+and non-empty.
+
+Disabling this webhook is not supported and may pose a security risk in
+clusters with untrusted users, as it could allow one user to hijack another's
+private repository and gain unauthorized control over it.
+
+If the webhook were disabled, multiple Repository CRDs could be created for the
+same URL. In this case, only the first created CRD would be recognized unless
+the user specifies the `target-namespace` annotation in their PipelineRun.
+{{< /hint >}}
+
 ## Setting PipelineRun definition source
 
 An additional layer of security can be added by using a PipelineRun annotation
@@ -63,15 +82,6 @@ Pipelines-as-Code will then only match the repository in the mynamespace
 namespace instead of trying to match it from all available repositories on the
 cluster.
 
-{{< hint info >}}
-Pipelines-as-Code installs a Kubernetes Mutating Admission Webhook to ensure
-that only one Repository CRD is created per URL on a cluster.
-
-If you disable this webhook, multiple Repository CRDs can be created for the
-same URL. However, only the oldest created Repository CRD will be matched,
-unless you use the `target-namespace` annotation.
-{{< /hint >}}
-
 ### PipelineRun definition provenance
 
 By default, on a `Push` or a `Pull Request`, Pipelines-as-Code will fetch the

diff --git a/docs/content/docs/install/global_repositories_setting.md b/docs/content/docs/install/global_repositories_setting.md
@@ -1,49 +1,49 @@
 ---
-title: Global Repository settings
+title: Global Repository Settings
 weight: 4
 ---
 {{< tech_preview "Global repository settings" >}}
 
-## Pipelines-as-Code global repository settings
+## Pipelines-as-Code Global Repository Settings
 
-Pipelines-as-Code let you have a global repository for all your Repo settings.
-This allows you to define settings that will be applied to all repositories on
-your cluster.
+Pipelines-as-Code lets you have a global repository for settings of all your
+local repositories. This enables you to define settings that will be applied to
+all local repositories on your cluster.
 
-The global repository setting are set as a fallback for all repositories, if
-the local repository on the namespace don't override it.
+The global repository settings serve as a fallback for all repositories if the
+local repository settings in the namespace do not override them.
 
-The global repository have to be created in the namespace where the
+The global repository must be created in the namespace where the
 `pipelines-as-code` controller is installed (usually `pipelines-as-code` or
 `openshift-pipelines`).
 
-The global repository CR should not have a `spec.url` defined.
+The global repository Custom Resource (CR) does not need a `spec.url` field. The
+field can either be blank or point to an unknown destination, such as:
 
-By default the repository needs to be named `pipelines-as-code` but you can
-redefine it by defining the environment variable
-`PAC_CONTROLLER_GLOBAL_REPOSITORY` on the controller Deployment.
+<https://pac.global.repo>
+
+By default, the global repository should be named `pipelines-as-code` unless
+you redefine it by setting the environment variable
+`PAC_CONTROLLER_GLOBAL_REPOSITORY` in the controller and watcher Deployment.
 
 The settings that can be defined in the global repository are:
 
 - [Concurrency Limit]({{< relref "/docs/guide/repositorycrd.md#concurrency" >}}).
 - [PipelineRun Provenance]({{< relref "/docs/guide/repositorycrd.md#pipelinerun-definition-provenance" >}}).
 - [Repository Policy]({{< relref "/docs/guide/policy" >}}).
-- [Repository GitHub App Token scope.]({{< relref "/docs/guide/repositorycrd.md#scoping-the-github-token-using-global-configuration" >}}).
-- The git provider auth settings like user, token, url etc...
-  The `type` needs to be defined in the namespace repository settings and need to match the `type` of the global repository (see below for an example).
+- [Repository GitHub App Token Scope]({{< relref "/docs/guide/repositorycrd.md#scoping-the-github-token-using-global-configuration" >}}).
+- Git provider auth settings such as user, token, URL, etc.
+  - The `type` must be defined in the namespace repository settings and must match the `type` of the global repository (see below for an example).
 - [Custom Parameters]({{< relref "/docs/guide/customparams.md" >}}).
-- [The incoming webhooks rules]({{< relref "/docs/guide/incoming_webhook.md" >}}).
-
-Note that the custom parameters and the incoming rules don't get merged with the
-namespace repository settings, they are only used if none is defined in the namespace.
+- [Incoming Webhooks Rules]({{< relref "/docs/guide/incoming_webhook.md" >}}).
 
 {{< hint info >}}
-global settings only gets applied at "runtime", they are not used by the tkn pac create repo command.
+Global settings are only applied when running via a Git provider event; they are not applied when for example using the `tkn pac` cli.
 {{< /hint >}}
 
-### Example of how the global repository settings are applied
+### Example of How Global Repository Settings Are Applied
 
-- if you have a Repository CR in the user namespace
+- If you have a Repository CR in the namespace named `user-namespace`:
 
 ```yaml
 apiVersion: pipelinesascode.tekton.dev/v1alpha1
@@ -58,7 +58,7 @@ spec:
     type: gitlab
 ```
 
-- and a have a global Repository CR on the controller namespace:
+- And a global Repository CR in the namespace where the controller and the watcher is located:
 
 ```yaml
 apiVersion: pipelinesascode.tekton.dev/v1alpha1
@@ -67,45 +67,44 @@ metadata:
   name: pipelines-as-code
   namespace: pipelines-as-code
 spec:
+  url: "https://paac.repo"
   concurrency_limit: 1
   params:
     - name: custom
       value: "value"
-
   git_provider:
     type: gitlab
     secret:
       name: "gitlab-token"
     webhook_secret:
-      name: gitlba-webhook-secret
+      name: gitlab-webhook-secret
 ```
 
-On this example the Repository `repo` will have a concurrency limit of 2 since
-the setting comes from the user namespace and ignored from the global repository. The
-parameter `custom` will be set to `value` and ready to be used to every
-repository that don't define any other custom parameters.
+In this example, the Repository `repo` will have a concurrency limit of 2 since
+the setting comes from the user namespace and is ignored from the global
+repository. The parameter `custom` will be set to `value` and will be available
+for every repository that does not define other custom parameters.
 
-Since the local repository CR has the git_provider.type `gitlab` like the
-global repository CR the git provider settings for the
-[GitLab]({{< relref "/docs/install/gitlab.md#create-a-repository-and-configure-webhook-manually" >}})
+Since the local Repository CR has the `git_provider.type` set to `gitlab`, like
+the global Repository CR, the Git provider settings for [GitLab]({{< relref "/docs/install/gitlab.md#create-a-repository-and-configure-webhook-manually" >}})
 will be taken from the global repository. The secret referenced will be fetched
 from where the global repository is defined.
 
-### Types when git provider settings gets applied
+### Webhook Based provider global settings
 
-These are the types you can setup for the git provider settings. Those are
-only used when doing incoming webhooks or global repository settings. They are
-only used for webhook based git providers (i.e: everything except GitHub Apps
-installation), in this case the type github means repository configured using
-[github webhooks]({{< relref "/docs/install/github_webhook.md" >}}):
+These are the `spec.git_provider.type` you can set up for the Git provider
+settings. They are only used when handling incoming webhooks or global
+repository settings. They are used for webhook-based Git providers (i.e.,
+everything except GitHub Apps installations). In this case, the type `github`
+means a repository configured using [GitHub webhooks]({{< relref "/docs/install/github_webhook.md" >}}):
 
 - github
 - gitlab
 - gitea
 - bitbucket-cloud
 - bitbucket-server
 
-The global repository settings for git provider can currently only reference one
-type of provider on a cluster. The user would need to specify their own provider
-info in their own Repository CR if they don't want to use the global settings or
-want to target another repository.
+The global repository settings for the Git provider can currently only
+reference one type of provider on a cluster. The user would need to specify
+their own provider information in their own Repository CR if they do not want
+to use the global settings or want to target another provider.
diff --git a/pkg/webhook/validation.go b/pkg/webhook/validation.go
@@ -2,6 +2,8 @@ package webhook
 
 import (
 	"context"
+	"net/url"
+	"os"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	pac "github.com/openshift-pipelines/pipelines-as-code/pkg/generated/listers/pipelinesascode/v1alpha1"
@@ -27,6 +29,22 @@ func (ac *reconciler) Admit(_ context.Context, request *v1.AdmissionRequest) *v1
 		return webhook.MakeErrorStatus("validation failed: %v", err)
 	}
 
+	// Check that if we have a URL set only for non global repository which can be set as empty.
+	if repo.GetNamespace() != os.Getenv("SYSTEM_NAMESPACE") {
+		if repo.Spec.URL == "" {
+			return webhook.MakeErrorStatus("URL must be set")
+		}
+
+		parsed, err := url.Parse(repo.Spec.URL)
+		if err != nil {
+			return webhook.MakeErrorStatus("invalid URL format: %v", err)
+		}
+
+		if parsed.Scheme != "http" && parsed.Scheme != "https" {
+			return webhook.MakeErrorStatus("URL scheme must be http or https")
+		}
+	}
+
 	exist, err := checkIfRepoExist(ac.pacLister, &repo, "")
 	if err != nil {
 		return webhook.MakeErrorStatus("validation failed: %v", err)

diff --git a/pkg/webhook/validation_test.go b/pkg/webhook/validation_test.go
@@ -8,12 +8,16 @@ import (
 	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
 	testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository"
 	"gotest.tools/v3/assert"
+	"gotest.tools/v3/env"
 	v1 "k8s.io/api/admission/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
 func TestReconciler_Admit(t *testing.T) {
+	globalNamespace := "globalNamespace"
+	envRemove := env.PatchAll(t, map[string]string{"SYSTEM_NAMESPACE": globalNamespace})
+	defer envRemove()
 	tests := []struct {
 		name    string
 		repo    *v1alpha1.Repository
@@ -30,6 +34,46 @@ func TestReconciler_Admit(t *testing.T) {
 			allowed: true,
 			result:  "",
 		},
+		{
+			name: "no http or https",
+			repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
+				Name:             "test-run",
+				InstallNamespace: "namespace",
+				URL:              "foobar",
+			}),
+			allowed: false,
+			result:  "URL scheme must be http or https",
+		},
+		{
+			name: "no http or https for global namespace allowed",
+			repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
+				Name:             "test-run",
+				InstallNamespace: globalNamespace,
+				URL:              "foobar",
+			}),
+			allowed: true,
+			result:  "URL scheme must be http or https",
+		},
+		{
+			name: "bad url",
+			repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
+				Name:             "test-run",
+				InstallNamespace: "namespace",
+				URL:              "h t t p s://github.com/openshift-pipelines/pipelines-as-code", // nolint: dupword
+			}),
+			allowed: false,
+			result:  `invalid URL format: parse "h t t p s://github.com/openshift-pipelines/pipelines-as-code": first path segment in URL cannot contain colon`, // nolint: dupword
+		},
+		{
+			name: "bad url for global namespace allowed",
+			repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
+				Name:             "test-run",
+				InstallNamespace: globalNamespace,
+				URL:              "h t t p s://github.com/openshift-pipelines/pipelines-as-code", // nolint: dupword
+			}),
+			allowed: true,
+			result:  `invalid URL format: parse "h t t p s://github.com/openshift-pipelines/pipelines-as-code": first path segment in URL cannot contain colon`, // nolint: dupword
+		},
 		{
 			name: "reject",
 			repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{

diff --git a/test/repo_validation_test.go b/test/repo_validation_test.go
@@ -0,0 +1,55 @@
+//go:build e2e
+// +build e2e
+
+package test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
+	pacrepo "github.com/openshift-pipelines/pipelines-as-code/test/pkg/repository"
+	"github.com/tektoncd/pipeline/pkg/names"
+	"gotest.tools/v3/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestRepoValidation(t *testing.T) {
+	ctx := context.TODO()
+	run := params.New()
+	assert.NilError(t, run.Clients.NewClients(ctx, &run.Info))
+	targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
+	assert.NilError(t, pacrepo.CreateNS(ctx, targetNS, run))
+
+	tests := []struct {
+		name        string
+		url         string
+		expectedErr string
+	}{
+		{
+			name:        "not http or https",
+			url:         "foobar",
+			expectedErr: "URL scheme must be http or https",
+		},
+		{
+			name:        "invalid URL",
+			url:         "http://   ",
+			expectedErr: "invalid URL format",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			repository := &v1alpha1.Repository{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: targetNS,
+				},
+				Spec: v1alpha1.RepositorySpec{
+					URL: tt.url,
+				},
+			}
+			err := pacrepo.CreateRepo(ctx, targetNS, run, repository)
+			assert.ErrorContains(t, err, tt.expectedErr)
+		})
+	}
+}