fix: Reject URL path segments containing ".." in all request methods (#4150)

Author: mohammadmseet-hue

github/github.go

@@ -20,6 +20,7 @@ import (
 	"net/http"
 	"net/url"
 	"regexp"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -157,6 +158,10 @@ const (
 
 var errNonNilContext = errors.New("context must be non-nil")
 
+// ErrPathForbidden is returned when a URL path contains ".." as a path
+// segment, which could allow path traversal attacks.
+var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulnerability issue")
+
 // A Client manages communication with the GitHub API.
 type Client struct {
 	clientMu              sync.Mutex   // clientMu protects the client during calls that modify the CheckRedirect func.
@@ -561,6 +566,10 @@ func (c *Client) NewRequest(method, urlStr string, body any, opts ...RequestOpti
 		return nil, fmt.Errorf("baseURL must have a trailing slash, but %q does not", c.BaseURL)
 	}
 
+	if err := checkURLPathTraversal(urlStr); err != nil {
+		return nil, err
+	}
+
 	u, err := c.BaseURL.Parse(urlStr)
 	if err != nil {
 		return nil, err
@@ -607,6 +616,10 @@ func (c *Client) NewFormRequest(urlStr string, body io.Reader, opts ...RequestOp
 		return nil, fmt.Errorf("baseURL must have a trailing slash, but %q does not", c.BaseURL)
 	}
 
+	if err := checkURLPathTraversal(urlStr); err != nil {
+		return nil, err
+	}
+
 	u, err := c.BaseURL.Parse(urlStr)
 	if err != nil {
 		return nil, err
@@ -631,13 +644,37 @@ func (c *Client) NewFormRequest(urlStr string, body io.Reader, opts ...RequestOp
 	return req, nil
 }
 
+// checkURLPathTraversal returns ErrPathForbidden if urlStr contains ".." as a
+// path segment (e.g. "a/../b"), preventing path traversal attacks. It does not
+// match ".." embedded within a segment (e.g. "file..txt"). The check is
+// performed only on the path portion of the URL, ignoring any query string or
+// fragment.
+func checkURLPathTraversal(urlStr string) error {
+	if !strings.Contains(urlStr, "..") {
+		return nil
+	}
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		return err
+	}
+	if slices.Contains(strings.Split(u.Path, "/"), "..") {
+		return ErrPathForbidden
+	}
+	return nil
+}
+
 // NewUploadRequest creates an upload request. A relative URL can be provided in
 // urlStr, in which case it is resolved relative to the UploadURL of the Client.
 // Relative URLs should always be specified without a preceding slash.
 func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, mediaType string, opts ...RequestOption) (*http.Request, error) {
 	if !strings.HasSuffix(c.UploadURL.Path, "/") {
 		return nil, fmt.Errorf("uploadURL must have a trailing slash, but %q does not", c.UploadURL)
 	}
+
+	if err := checkURLPathTraversal(urlStr); err != nil {
+		return nil, err
+	}
+
 	u, err := c.UploadURL.Parse(urlStr)
 	if err != nil {
 		return nil, err

github/github_test.go

@@ -786,6 +786,83 @@ func TestNewRequest_errorForNoTrailingSlash(t *testing.T) {
 	}
 }
 
+func TestCheckURLPathTraversal(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		input   string
+		wantErr error
+	}{
+		{"repos/o/r/contents/file.txt", nil},
+		{"repos/o/r/contents/dir/file.txt", nil},
+		{"repos/o/r/contents/file..txt", nil},
+		{"repos/o/r?q=a..b", nil},
+		{"repos/../admin/users", ErrPathForbidden},
+		{"repos/x/../../../admin", ErrPathForbidden},
+		{"../admin", ErrPathForbidden},
+		{"repos/o/r/contents/..", ErrPathForbidden},
+		{"repos/o/r/contents/../secrets", ErrPathForbidden},
+		// Full URLs with scheme.
+		{"https://api.github.com/repos/../admin", ErrPathForbidden},
+		{"https://api.github.com/repos/o/r/contents/file.txt", nil},
+		{"https://api.github.com/repos/o/r/contents/file..txt", nil},
+		// URL with fragment.
+		{"repos/o/r/contents/file.txt#section", nil},
+		{"repos/../admin#frag", ErrPathForbidden},
+		// URL with userinfo.
+		{"https://user:pass@api.github.com/repos/../admin", ErrPathForbidden},
+		{"https://user:pass@api.github.com/repos/o/r", nil},
+	}
+	for _, tt := range tests {
+		err := checkURLPathTraversal(tt.input)
+		if !errors.Is(err, tt.wantErr) {
+			t.Errorf("checkURLPathTraversal(%q) = %v, want %v", tt.input, err, tt.wantErr)
+		}
+	}
+}
+
+func TestNewRequest_pathTraversal(t *testing.T) {
+	t.Parallel()
+	c := NewClient(nil)
+
+	tests := []struct {
+		urlStr    string
+		wantError bool
+	}{
+		{"repos/o/r/readme", false},
+		{"repos/o/r/contents/file..txt", false},
+		{"repos/x/../../../admin/users", true},
+		{"repos/../admin", true},
+	}
+	for _, tt := range tests {
+		_, err := c.NewRequest("GET", tt.urlStr, nil)
+		if tt.wantError && !errors.Is(err, ErrPathForbidden) {
+			t.Errorf("NewRequest(%q): want ErrPathForbidden, got %v", tt.urlStr, err)
+		} else if !tt.wantError && err != nil {
+			t.Errorf("NewRequest(%q): unexpected error: %v", tt.urlStr, err)
+		}
+	}
+}
+
+func TestNewFormRequest_pathTraversal(t *testing.T) {
+	t.Parallel()
+	c := NewClient(nil)
+
+	_, err := c.NewFormRequest("repos/x/../../../admin", nil)
+	if !errors.Is(err, ErrPathForbidden) {
+		t.Fatalf("NewFormRequest with path traversal: want ErrPathForbidden, got %v", err)
+	}
+}
+
+func TestNewUploadRequest_pathTraversal(t *testing.T) {
+	t.Parallel()
+	c := NewClient(nil)
+
+	_, err := c.NewUploadRequest("repos/x/../../../admin", nil, 0, "")
+	if !errors.Is(err, ErrPathForbidden) {
+		t.Fatalf("NewUploadRequest with path traversal: want ErrPathForbidden, got %v", err)
+	}
+}
+
 func TestNewFormRequest(t *testing.T) {
 	t.Parallel()
 	c := NewClient(nil)

github/repos_contents.go

@@ -20,8 +20,6 @@ import (
 	"strings"
 )
 
-var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulnerability issue")
-
 // RepositoryContent represents a file or directory in a github repository.
 type RepositoryContent struct {
 	Type *string `json:"type,omitempty"`
@@ -192,17 +190,10 @@ func (s *RepositoriesService) DownloadContentsWithMeta(ctx context.Context, owne
 // as possible, both result types will be returned but only one will contain a
 // value and the other will be nil.
 //
-// Due to an auth vulnerability issue in the GitHub v3 API, ".." is not allowed
-// to appear anywhere in the "path" or this method will return an error.
-//
 // GitHub API docs: https://docs.github.com/rest/repos/contents?apiVersion=2022-11-28#get-repository-content
 //
 //meta:operation GET /repos/{owner}/{repo}/contents/{path}
 func (s *RepositoriesService) GetContents(ctx context.Context, owner, repo, path string, opts *RepositoryContentGetOptions) (fileContent *RepositoryContent, directoryContent []*RepositoryContent, resp *Response, err error) {
-	if strings.Contains(path, "..") {
-		return nil, nil, nil, ErrPathForbidden
-	}
-
 	escapedPath := (&url.URL{Path: strings.TrimSuffix(path, "/")}).String()
 	u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, escapedPath)
 	u, err = addOptions(u, opts)