Skip to content

Commit de692ba

Browse files
theakshaypantzakisk
theakshaypant
authored and
zakisk
committed
fix(github): detect GHE instances for URL validation
Reject Repository CRs with malformed GitHub URLs that include extra path segments (e.g., https://github.com/org/repo/extra). These URLs previously passed admission and were truncated during token scoping, allowing bypass of namespace guards. Add GitHub Enterprise detection to accurately validate repository URLs. Detects GHE via Server header and /api/v3/meta endpoint, then enforces org/repo format without additional path segments. This prevents malformed URLs during admission and token scoping. Fixes: #2395 Jira: https://issues.redhat.com/browse/SRVKP-10943 Signed-off-by: Akshay Pant <akpant@redhat.com>
1 parent c6397d3 commit de692ba

File tree

6 files changed

+237
-9
lines changed

6 files changed

+237
-9
lines changed

‎pkg/provider/github/github.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -661,6 +661,12 @@ func (v *Provider) CreateToken(ctx context.Context, repository []string, event *
661661
}
662662

663663
split := strings.Split(r, "/")
664+
// Validate the URLs do not include additional path segments (like https://github.com/org/repo/extra).
665+
// This validation is not required for glob as a pattern like "org/*/*" would not be matched.
666+
if len(split) > 2 {
667+
return "", fmt.Errorf("github repository URL must follow org/repo format without subgroups (found %d path segments, expected 2): %s", len(split), r)
668+
}
669+
664670
infoData, _, err := wrapAPI(v, "get_repository", func() (*github.Repository, *github.Response, error) {
665671
return v.Client().Repositories.Get(ctx, split[0], split[1])
666672
})

‎pkg/provider/github/scope.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,5 +115,11 @@ func getURLPathData(urlInfo string) ([]string, error) {
115115
if err != nil {
116116
return []string{}, err
117117
}
118-
return strings.Split(urlData.Path, "/"), nil
118+
119+
split := strings.Split(urlData.Path, "/")
120+
if len(split) != 3 {
121+
return []string{}, fmt.Errorf("github repository URL must follow https://github.com/org/repo format without subgroups (found %d path segments, expected 2): %s", len(split)-1, urlInfo)
122+
}
123+
124+
return split, nil
119125
}

‎pkg/provider/github/scope_test.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,33 @@ func TestScopeTokenToListOfRepos(t *testing.T) {
140140
},
141141
}
142142

143+
// Malformed repository URLs for testing validation
144+
malformedRepoURL := "https://org.com/owner/repo/subgroup"
145+
repoDataMalformed := &v1alpha1.Repository{
146+
TypeMeta: metav1.TypeMeta{},
147+
ObjectMeta: metav1.ObjectMeta{
148+
Name: "malformedrepo",
149+
Namespace: testNamespace.Name,
150+
},
151+
Spec: v1alpha1.RepositorySpec{
152+
URL: malformedRepoURL,
153+
},
154+
}
155+
156+
repoDataMalformedPattern := &v1alpha1.Repository{
157+
TypeMeta: metav1.TypeMeta{},
158+
ObjectMeta: metav1.ObjectMeta{
159+
Name: "publicrepo-malformed-pattern",
160+
Namespace: testNamespace.Name,
161+
},
162+
Spec: v1alpha1.RepositorySpec{
163+
URL: repoFromWhichEventComes,
164+
Settings: &v1alpha1.Settings{
165+
GithubAppTokenScopeRepos: []string{"owner/repo/extra"},
166+
},
167+
},
168+
}
169+
143170
tests := []struct {
144171
name string
145172
tData testclient.Data
@@ -310,6 +337,66 @@ func TestScopeTokenToListOfRepos(t *testing.T) {
310337
wantError: "invalid repo glob specified",
311338
wantToken: "",
312339
},
340+
{
341+
name: "malformed repository URL with extra path segments returns error",
342+
tData: testclient.Data{
343+
Namespaces: []*corev1.Namespace{testNamespace},
344+
Secret: []*corev1.Secret{validSecret},
345+
Repositories: []*v1alpha1.Repository{
346+
repoData, repoDataMalformed,
347+
},
348+
},
349+
repository: &v1alpha1.Repository{
350+
TypeMeta: metav1.TypeMeta{},
351+
ObjectMeta: metav1.ObjectMeta{
352+
Name: "publicrepo",
353+
Namespace: testNamespace.Name,
354+
},
355+
Spec: v1alpha1.RepositorySpec{
356+
URL: repoFromWhichEventComes,
357+
Settings: &v1alpha1.Settings{
358+
GithubAppTokenScopeRepos: []string{"owner/repo/subgroup"},
359+
},
360+
},
361+
},
362+
repoListsByGlobalConf: "",
363+
wantError: "github repository URL must follow https://github.com/org/repo format without subgroups",
364+
wantToken: "",
365+
},
366+
{
367+
name: "malformed pattern with extra path segments returns error",
368+
tData: testclient.Data{
369+
Namespaces: []*corev1.Namespace{testNamespace},
370+
Secret: []*corev1.Secret{validSecret},
371+
Repositories: []*v1alpha1.Repository{
372+
repoDataMalformedPattern, repoData1,
373+
},
374+
},
375+
repository: repoDataMalformedPattern,
376+
repoListsByGlobalConf: "",
377+
wantError: "failed to scope GitHub token as repo with pattern owner/repo/extra does not exist in namespace",
378+
wantToken: "",
379+
},
380+
{
381+
name: "malformed pattern in global config with extra path segments returns error",
382+
tData: testclient.Data{
383+
Namespaces: []*corev1.Namespace{testNamespace},
384+
Secret: []*corev1.Secret{validSecret},
385+
},
386+
repository: &v1alpha1.Repository{
387+
TypeMeta: metav1.TypeMeta{},
388+
ObjectMeta: metav1.ObjectMeta{
389+
Name: "publicrepo",
390+
Namespace: testNamespace.Name,
391+
},
392+
Spec: v1alpha1.RepositorySpec{
393+
URL: repoFromWhichEventComes,
394+
},
395+
},
396+
repoListsByGlobalConf: "owner/repo/extra",
397+
wantError: "github repository URL must follow",
398+
wantToken: "",
399+
},
313400
}
314401
for _, tt := range tests {
315402
t.Run(tt.name, func(t *testing.T) {

‎pkg/test/repository/repository.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ type RepoTestcreationOpts struct {
1616
SecretName string
1717
WebhookSecretName string
1818
ProviderURL string
19+
GitProviderType string
1920
CreateTime metav1.Time
2021
RepoStatus []v1alpha1.RepositoryRunStatus
2122
ConcurrencyLimit int
@@ -78,7 +79,7 @@ func NewRepo(opts RepoTestcreationOpts) *v1alpha1.Repository {
7879
repo.Spec.ConcurrencyLimit = &opts.ConcurrencyLimit
7980
}
8081

81-
if opts.SecretName != "" || opts.ProviderURL != "" || opts.WebhookSecretName != "" {
82+
if opts.SecretName != "" || opts.ProviderURL != "" || opts.WebhookSecretName != "" || opts.GitProviderType != "" {
8283
repo.Spec.GitProvider = &v1alpha1.GitProvider{
8384
Secret: &v1alpha1.Secret{},
8485
}
@@ -99,6 +100,10 @@ func NewRepo(opts RepoTestcreationOpts) *v1alpha1.Repository {
99100
}
100101
}
101102

103+
if opts.GitProviderType != "" {
104+
repo.Spec.GitProvider.Type = opts.GitProviderType
105+
}
106+
102107
if opts.Params != nil {
103108
repo.Spec.Params = opts.Params
104109
}

‎pkg/webhook/validation.go‎

Lines changed: 64 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,12 @@ package webhook
22

33
import (
44
"context"
5+
"fmt"
6+
"net/http"
57
"net/url"
68
"os"
9+
"strings"
10+
"time"
711

812
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
913
pac "github.com/openshift-pipelines/pipelines-as-code/pkg/generated/listers/pipelinesascode/v1alpha1"
@@ -42,13 +46,8 @@ func (ac *reconciler) Admit(_ context.Context, request *v1.AdmissionRequest) *v1
4246
return webhook.MakeErrorStatus("URL must be set")
4347
}
4448

45-
parsed, err := url.Parse(repo.Spec.URL)
46-
if err != nil {
47-
return webhook.MakeErrorStatus("invalid URL format: %v", err)
48-
}
49-
50-
if parsed.Scheme != "http" && parsed.Scheme != "https" {
51-
return webhook.MakeErrorStatus("URL scheme must be http or https")
49+
if err := validateRepositoryURL(repo.Spec.URL); err != nil {
50+
return webhook.MakeErrorStatus("%s", err.Error())
5251
}
5352
}
5453

@@ -94,3 +93,61 @@ func checkIfRepoExist(pac pac.RepositoryLister, repo *v1alpha1.Repository, ns st
9493
}
9594
return false, nil
9695
}
96+
97+
func validateRepositoryURL(repoURL string) error {
98+
parsedURL, err := url.Parse(repoURL)
99+
if err != nil {
100+
return fmt.Errorf("invalid URL format: %w", err)
101+
}
102+
103+
if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
104+
return fmt.Errorf("URL scheme must be http or https")
105+
}
106+
107+
// Validate github repository URL does not include additional path segments
108+
// (like https://github.com/org/repo/extra).
109+
// Detect if this is a GitHub instance (github.com or GHE) by checking headers
110+
// and API endpoints.
111+
if isGitHubInstance(parsedURL.Host, parsedURL.Scheme) {
112+
// Remove leading and trailing "/"
113+
repoPath := strings.Trim(parsedURL.Path, "/")
114+
115+
split := strings.Split(repoPath, "/")
116+
if len(split) != 2 {
117+
return fmt.Errorf("github repository URL must follow https://github.com/org/repo format without subgroups (found %d path segments, expected 2): %s", len(split), repoURL)
118+
}
119+
}
120+
121+
return nil
122+
}
123+
124+
// isGitHubInstance detects if a host is github.com or a GitHub Enterprise instance.
125+
// It checks the server header and /api/v3/meta endpoint.
126+
func isGitHubInstance(host, scheme string) bool {
127+
if host == "github.com" {
128+
return true
129+
}
130+
131+
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
132+
defer cancel()
133+
134+
client := &http.Client{}
135+
136+
// Try HEAD request to check the server header.
137+
url := fmt.Sprintf("%s://%s", scheme, host)
138+
req, err := http.NewRequestWithContext(ctx, http.MethodHead, url, nil)
139+
if err != nil {
140+
return false
141+
}
142+
143+
resp, err := client.Do(req)
144+
if err == nil {
145+
defer resp.Body.Close()
146+
server := resp.Header.Get("Server")
147+
if strings.Contains(strings.ToLower(server), "github.com") {
148+
return true
149+
}
150+
}
151+
152+
return false
153+
}

‎pkg/webhook/validation_test.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,73 @@ func TestReconciler_Admit(t *testing.T) {
103103
allowed: false,
104104
result: "repository already exists with URL: https://pac.test/already/installed",
105105
},
106+
{
107+
name: "reject github.com URL with subgroup, GitHub auto-detected",
108+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
109+
Name: "test-run",
110+
InstallNamespace: "namespace",
111+
URL: "https://github.com/owner/repo/subgroup",
112+
}),
113+
allowed: false,
114+
result: "github repository URL must follow https://github.com/org/repo format without subgroups (found 3 path segments, expected 2): https://github.com/owner/repo/subgroup",
115+
},
116+
{
117+
name: "allow github.com URL with correct format, GitHub auto-detected",
118+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
119+
Name: "test-run",
120+
InstallNamespace: "namespace",
121+
URL: "https://github.com/owner/repo",
122+
}),
123+
allowed: true,
124+
},
125+
{
126+
name: "reject GitHub repository URL with subgroup",
127+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
128+
Name: "test-run",
129+
InstallNamespace: "namespace",
130+
URL: "https://ghe.pipelinesascode.com/owner/repo/subgroup",
131+
}),
132+
allowed: false,
133+
result: "github repository URL must follow https://github.com/org/repo format without subgroups (found 3 path segments, expected 2): https://ghe.pipelinesascode.com/owner/repo/subgroup",
134+
},
135+
{
136+
name: "reject GitHub repository URL with multiple subgroups",
137+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
138+
Name: "test-run",
139+
InstallNamespace: "namespace",
140+
URL: "https://github.com/owner/repo/subgroup/extra",
141+
}),
142+
allowed: false,
143+
result: "github repository URL must follow https://github.com/org/repo format without subgroups (found 4 path segments, expected 2): https://github.com/owner/repo/subgroup/extra",
144+
},
145+
{
146+
name: "allow GitLab repository URL with subgroups",
147+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
148+
Name: "test-run",
149+
InstallNamespace: "namespace",
150+
URL: "https://gitlab.com/owner/group/repo",
151+
}),
152+
allowed: true,
153+
},
154+
{
155+
name: "allow Bitbucket repository URL with subgroups",
156+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
157+
Name: "test-run",
158+
InstallNamespace: "namespace",
159+
URL: "https://bitbucket.org/workspace/project/repo",
160+
}),
161+
allowed: true,
162+
},
163+
{
164+
name: "allow GitHub URL with correct format",
165+
repo: testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{
166+
Name: "test-run",
167+
InstallNamespace: "namespace",
168+
URL: "https://ghe.company.com/owner/repo",
169+
GitProviderType: "github",
170+
}),
171+
allowed: true,
172+
},
106173
}
107174
for _, tt := range tests {
108175
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)
⚡