diff --git a/github/github.go b/github/github.go index acc9bc3c29f..7ab237f5520 100644 --- a/github/github.go +++ b/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,6 +644,25 @@ 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. @@ -638,6 +670,11 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m 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 diff --git a/github/github_test.go b/github/github_test.go index a341ba6b882..f1584b2794c 100644 --- a/github/github_test.go +++ b/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) diff --git a/github/repos_contents.go b/github/repos_contents.go index 0c2ac3bf25f..5b6c5c81cbc 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -21,8 +21,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"` @@ -229,17 +227,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)