From f592ad35d128ff1b1868ec665a375e8025eb3b34 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 3 Oct 2024 15:31:24 +0300 Subject: [PATCH 1/2] gitlab: Handle query parameters in REST trait This allows us to reliably pass query parameters in the gitlab REST trait. Else, we'd get everything URL-encoded and it would not work. Signed-off-by: Juan Antonio Osorio --- internal/providers/gitlab/gitlab_rest.go | 29 +++++- internal/providers/gitlab/gitlab_rest_test.go | 94 +++++++++++++++++++ 2 files changed, 119 insertions(+), 4 deletions(-) diff --git a/internal/providers/gitlab/gitlab_rest.go b/internal/providers/gitlab/gitlab_rest.go index b822a22e29..8a67f97e70 100644 --- a/internal/providers/gitlab/gitlab_rest.go +++ b/internal/providers/gitlab/gitlab_rest.go @@ -40,13 +40,11 @@ func (c *gitlabClient) GetBaseURL() string { // NewRequest implements the REST provider interface func (c *gitlabClient) NewRequest(method, requestPath string, body any) (*http.Request, error) { - base, err := url.Parse(c.glcfg.Endpoint) + u, err := getParsedURL(c.glcfg.Endpoint, requestPath) if err != nil { - return nil, fmt.Errorf("failed to parse URL: %w", err) + return nil, err } - u := base.JoinPath(requestPath) - var buf io.ReadWriter if body != nil { buf = &bytes.Buffer{} @@ -118,3 +116,26 @@ func glRESTGet[T any](ctx context.Context, cli genericRESTClient, path string, o return nil } + +func getParsedURL(endpoint, path string) (*url.URL, error) { + base, err := url.Parse(endpoint) + if err != nil { + return nil, fmt.Errorf("failed to parse URL: %w", err) + } + + // Explicitly parse path and query parameters. This is to ensure that + // the path is properly escaped and that the query parameters are + // properly encoded. + parsedPathAndQuery, err := url.Parse(path) + if err != nil { + return nil, fmt.Errorf("failed to parse path: %w", err) + } + + u := base.JoinPath(parsedPathAndQuery.Path) + + // These have already been escaped by the URL parser + u.RawQuery = parsedPathAndQuery.RawQuery + u.Fragment = parsedPathAndQuery.Fragment + + return u, nil +} diff --git a/internal/providers/gitlab/gitlab_rest_test.go b/internal/providers/gitlab/gitlab_rest_test.go index a3a0c55e80..2b3ac2a2f2 100644 --- a/internal/providers/gitlab/gitlab_rest_test.go +++ b/internal/providers/gitlab/gitlab_rest_test.go @@ -273,3 +273,97 @@ func Test_glRESTGet(t *testing.T) { }) } } + +func Test_getParsedURL(t *testing.T) { + t.Parallel() + + type args struct { + endpoint string + // This may include query parameters + path string + } + tests := []struct { + name string + args args + want *url.URL + wantErr bool + }{ + { + name: "valid URL", + args: args{ + endpoint: "http://example.com/v1/api", + path: "/test", + }, + want: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/v1/api/test", + }, + }, + { + name: "valid URL with query parameters", + args: args{ + endpoint: "http://example.com/v1/api", + path: "/test?param1=value1¶m2=value2", + }, + want: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/v1/api/test", + RawQuery: "param1=value1¶m2=value2", + }, + }, + { + name: "valid URL with fragment identifier", + args: args{ + endpoint: "http://example.com/v1/api", + path: "/test#section", + }, + want: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/v1/api/test", + Fragment: "section", + }, + }, + { + name: "invalid URL", + args: args{ + endpoint: "://example.com:80:80", + path: "/test", + }, + wantErr: true, + }, + { + name: "query parameter that needs escaping", + args: args{ + endpoint: "http://example.com", + path: "/test?param1=va lue1", + }, + want: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "test", + RawQuery: "param1=va%20lue1", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := getParsedURL(tt.args.endpoint, tt.args.path) + if tt.wantErr { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + assert.Equal(t, tt.want.Scheme, got.Scheme, "Expected scheme to be equal") + assert.Equal(t, tt.want.Host, got.Host, "Expected host to be equal") + assert.Equal(t, tt.want.Path, got.Path, "Expected path to be equal") + assert.Equal(t, tt.want.Query(), got.Query(), "Expected query to be equal") + assert.Equal(t, tt.want.Fragment, got.Fragment, "Expected fragment to be equal") + }) + } +} From aba1fd6b3013647e935085521505f5d0bb3477bd Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 3 Oct 2024 16:34:40 +0300 Subject: [PATCH 2/2] Fix unit test Signed-off-by: Juan Antonio Osorio --- internal/providers/gitlab/gitlab_rest_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/providers/gitlab/gitlab_rest_test.go b/internal/providers/gitlab/gitlab_rest_test.go index 2b3ac2a2f2..540bab2e09 100644 --- a/internal/providers/gitlab/gitlab_rest_test.go +++ b/internal/providers/gitlab/gitlab_rest_test.go @@ -162,7 +162,7 @@ func Test_gitlabClient_NewRequest(t *testing.T) { name: "invalid URL gets cleaned up", method: http.MethodGet, requestPath: "..:/invalid-url", - wantErr: false, + wantErr: true, }, { name: "invalid URL parsing error",