From 17df63f12d6dda962da884208185f7580e933bfa Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Mon, 13 Apr 2026 21:13:28 +0200 Subject: [PATCH 1/4] fix: reject URL path segments containing ".." in all request methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing ErrPathForbidden check in GetContents correctly blocks path traversal via the "path" parameter, but the same validation was missing from: 1. NewRequest, NewFormRequest, NewUploadRequest — allowing traversal via owner, repo, org, user, enterprise, and team parameters across all API methods. 2. CreateFile, UpdateFile, DeleteFile — which use the same URL pattern as GetContents but lacked the ".." check on the path parameter. This adds a centralized containsDotDotPathSegment check in the three request-building methods, which protects all current and future API methods from path traversal via any URL-interpolated parameter. The check only matches ".." as a complete path segment (not substrings like "file..txt") and ignores query strings. Also adds the same strings.Contains(path, "..") guard that GetContents has to CreateFile, UpdateFile, and DeleteFile for defense in depth. Fixes google/go-github#XXXX --- github/github.go | 34 ++++++++++++++++++ github/github_test.go | 66 +++++++++++++++++++++++++++++++++++ github/repos_contents.go | 12 +++++++ github/repos_contents_test.go | 30 ++++++++++++++++ 4 files changed, 142 insertions(+) diff --git a/github/github.go b/github/github.go index acc9bc3c29f..0c174e3c45b 100644 --- a/github/github.go +++ b/github/github.go @@ -551,6 +551,27 @@ func WithVersion(version string) RequestOption { } } +// containsDotDotPathSegment reports whether urlStr contains ".." as a path +// segment (e.g. "a/../b"). 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. +func containsDotDotPathSegment(urlStr string) bool { + if !strings.Contains(urlStr, "..") { + return false + } + // Only check the path portion, ignore query string. + path := urlStr + if i := strings.IndexByte(path, '?'); i >= 0 { + path = path[:i] + } + for _, seg := range strings.Split(path, "/") { + if seg == ".." { + return true + } + } + return false +} + // NewRequest creates an API request. A relative URL can be provided in urlStr, // in which case it is resolved relative to the BaseURL of the Client. // Relative URLs should always be specified without a preceding slash. If @@ -561,6 +582,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 containsDotDotPathSegment(urlStr) { + return nil, ErrPathForbidden + } + u, err := c.BaseURL.Parse(urlStr) if err != nil { return nil, err @@ -607,6 +632,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 containsDotDotPathSegment(urlStr) { + return nil, ErrPathForbidden + } + u, err := c.BaseURL.Parse(urlStr) if err != nil { return nil, err @@ -638,6 +667,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 containsDotDotPathSegment(urlStr) { + return nil, ErrPathForbidden + } + 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..b15c7dd1909 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -786,6 +786,72 @@ func TestNewRequest_errorForNoTrailingSlash(t *testing.T) { } } +func TestContainsDotDotPathSegment(t *testing.T) { + t.Parallel() + tests := []struct { + input string + want bool + }{ + {"repos/o/r/contents/file.txt", false}, + {"repos/o/r/contents/dir/file.txt", false}, + {"repos/o/r/contents/file..txt", false}, + {"repos/o/r?q=a..b", false}, + {"repos/../admin/users", true}, + {"repos/x/../../../admin", true}, + {"../admin", true}, + {"repos/o/r/contents/..", true}, + {"repos/o/r/contents/../secrets", true}, + } + for _, tt := range tests { + if got := containsDotDotPathSegment(tt.input); got != tt.want { + t.Errorf("containsDotDotPathSegment(%q) = %v, want %v", tt.input, got, tt.want) + } + } +} + +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..78e83f688d8 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -278,6 +278,10 @@ func (s *RepositoriesService) GetContents(ctx context.Context, owner, repo, path // //meta:operation PUT /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { + if strings.Contains(path, "..") { + return nil, nil, ErrPathForbidden + } + u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("PUT", u, opts) if err != nil { @@ -300,6 +304,10 @@ func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path // //meta:operation PUT /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { + if strings.Contains(path, "..") { + return nil, nil, ErrPathForbidden + } + u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("PUT", u, opts) if err != nil { @@ -322,6 +330,10 @@ func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path // //meta:operation DELETE /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) DeleteFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { + if strings.Contains(path, "..") { + return nil, nil, ErrPathForbidden + } + u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("DELETE", u, opts) if err != nil { diff --git a/github/repos_contents_test.go b/github/repos_contents_test.go index 113aad6ebc4..50f71ecdd96 100644 --- a/github/repos_contents_test.go +++ b/github/repos_contents_test.go @@ -702,6 +702,36 @@ func TestRepositoriesService_GetContents_Directory(t *testing.T) { } } +func TestRepositoriesService_CreateFile_PathWithParent(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + ctx := t.Context() + _, _, err := client.Repositories.CreateFile(ctx, "o", "r", "some/../other", nil) + if err == nil { + t.Fatal("Repositories.CreateFile expected error for path with '..' but got none") + } +} + +func TestRepositoriesService_UpdateFile_PathWithParent(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + ctx := t.Context() + _, _, err := client.Repositories.UpdateFile(ctx, "o", "r", "some/../other", nil) + if err == nil { + t.Fatal("Repositories.UpdateFile expected error for path with '..' but got none") + } +} + +func TestRepositoriesService_DeleteFile_PathWithParent(t *testing.T) { + t.Parallel() + client, _, _ := setup(t) + ctx := t.Context() + _, _, err := client.Repositories.DeleteFile(ctx, "o", "r", "some/../other", nil) + if err == nil { + t.Fatal("Repositories.DeleteFile expected error for path with '..' but got none") + } +} + func TestRepositoriesService_CreateFile(t *testing.T) { t.Parallel() client, mux, _ := setup(t) From cb706ecaf4bdeb349e2875b35b8f64245f962054 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Mon, 13 Apr 2026 22:43:14 +0200 Subject: [PATCH 2/4] refactor: use slices.Contains in containsDotDotPathSegment Apply review suggestion from @gmlewis to simplify the loop into a single slices.Contains call. --- github/github.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/github/github.go b/github/github.go index 0c174e3c45b..24207b62614 100644 --- a/github/github.go +++ b/github/github.go @@ -20,6 +20,7 @@ import ( "net/http" "net/url" "regexp" + "slices" "strconv" "strings" "sync" @@ -564,12 +565,7 @@ func containsDotDotPathSegment(urlStr string) bool { if i := strings.IndexByte(path, '?'); i >= 0 { path = path[:i] } - for _, seg := range strings.Split(path, "/") { - if seg == ".." { - return true - } - } - return false + return slices.Contains(strings.Split(path, "/"), "..") } // NewRequest creates an API request. A relative URL can be provided in urlStr, From 4b63f60ffb22482223dc01cc4a8720a0718f9759 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Tue, 14 Apr 2026 15:33:54 +0200 Subject: [PATCH 3/4] refactor: use url.Parse, rename to urlContainsDotDotPathSegment, return error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback from @stevehipwell: - Use url.Parse() for path extraction instead of manual string splitting - Rename containsDotDotPathSegment → urlContainsDotDotPathSegment - Change signature to return (bool, error) - Add full URL test cases (scheme, userinfo, fragment) --- github/github.go | 29 ++++++++++++++++------------ github/github_test.go | 44 +++++++++++++++++++++++++++++-------------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/github/github.go b/github/github.go index 24207b62614..7ef9d298659 100644 --- a/github/github.go +++ b/github/github.go @@ -552,20 +552,19 @@ func WithVersion(version string) RequestOption { } } -// containsDotDotPathSegment reports whether urlStr contains ".." as a path +// urlContainsDotDotPathSegment reports whether urlStr contains ".." as a path // segment (e.g. "a/../b"). 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. -func containsDotDotPathSegment(urlStr string) bool { +// URL, ignoring any query string or fragment. +func urlContainsDotDotPathSegment(urlStr string) (bool, error) { if !strings.Contains(urlStr, "..") { - return false + return false, nil } - // Only check the path portion, ignore query string. - path := urlStr - if i := strings.IndexByte(path, '?'); i >= 0 { - path = path[:i] + u, err := url.Parse(urlStr) + if err != nil { + return false, err } - return slices.Contains(strings.Split(path, "/"), "..") + return slices.Contains(strings.Split(u.Path, "/"), ".."), nil } // NewRequest creates an API request. A relative URL can be provided in urlStr, @@ -578,7 +577,9 @@ 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 containsDotDotPathSegment(urlStr) { + if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + return nil, err + } else if hasDotDot { return nil, ErrPathForbidden } @@ -628,7 +629,9 @@ 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 containsDotDotPathSegment(urlStr) { + if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + return nil, err + } else if hasDotDot { return nil, ErrPathForbidden } @@ -664,7 +667,9 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m return nil, fmt.Errorf("uploadURL must have a trailing slash, but %q does not", c.UploadURL) } - if containsDotDotPathSegment(urlStr) { + if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + return nil, err + } else if hasDotDot { return nil, ErrPathForbidden } diff --git a/github/github_test.go b/github/github_test.go index b15c7dd1909..99cbb8d3596 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -786,25 +786,41 @@ func TestNewRequest_errorForNoTrailingSlash(t *testing.T) { } } -func TestContainsDotDotPathSegment(t *testing.T) { +func TestUrlContainsDotDotPathSegment(t *testing.T) { t.Parallel() tests := []struct { - input string - want bool + input string + want bool + wantErr bool }{ - {"repos/o/r/contents/file.txt", false}, - {"repos/o/r/contents/dir/file.txt", false}, - {"repos/o/r/contents/file..txt", false}, - {"repos/o/r?q=a..b", false}, - {"repos/../admin/users", true}, - {"repos/x/../../../admin", true}, - {"../admin", true}, - {"repos/o/r/contents/..", true}, - {"repos/o/r/contents/../secrets", true}, + {"repos/o/r/contents/file.txt", false, false}, + {"repos/o/r/contents/dir/file.txt", false, false}, + {"repos/o/r/contents/file..txt", false, false}, + {"repos/o/r?q=a..b", false, false}, + {"repos/../admin/users", true, false}, + {"repos/x/../../../admin", true, false}, + {"../admin", true, false}, + {"repos/o/r/contents/..", true, false}, + {"repos/o/r/contents/../secrets", true, false}, + // Full URLs with scheme. + {"https://api.github.com/repos/../admin", true, false}, + {"https://api.github.com/repos/o/r/contents/file.txt", false, false}, + {"https://api.github.com/repos/o/r/contents/file..txt", false, false}, + // URL with fragment. + {"repos/o/r/contents/file.txt#section", false, false}, + {"repos/../admin#frag", true, false}, + // URL with userinfo. + {"https://user:pass@api.github.com/repos/../admin", true, false}, + {"https://user:pass@api.github.com/repos/o/r", false, false}, } for _, tt := range tests { - if got := containsDotDotPathSegment(tt.input); got != tt.want { - t.Errorf("containsDotDotPathSegment(%q) = %v, want %v", tt.input, got, tt.want) + got, err := urlContainsDotDotPathSegment(tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("urlContainsDotDotPathSegment(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) + continue + } + if got != tt.want { + t.Errorf("urlContainsDotDotPathSegment(%q) = %v, want %v", tt.input, got, tt.want) } } } From 33a356c6776b92993c4809c4dbb5f48173bcb7dd Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Tue, 14 Apr 2026 15:47:47 +0200 Subject: [PATCH 4/4] refactor: address alexandear's review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move ErrPathForbidden declaration to github.go - Simplify urlContainsDotDotPathSegment → checkURLPathTraversal returning error directly, eliminating duplicated calling pattern - Move checkURLPathTraversal after NewFormRequest for readability - Remove redundant ".." checks from GetContents, CreateFile, UpdateFile, DeleteFile — NewRequest already handles path traversal validation --- github/github.go | 50 ++++++++++++++++++----------------- github/github_test.go | 47 +++++++++++++++----------------- github/repos_contents.go | 21 --------------- github/repos_contents_test.go | 30 --------------------- 4 files changed, 47 insertions(+), 101 deletions(-) diff --git a/github/github.go b/github/github.go index 7ef9d298659..7ab237f5520 100644 --- a/github/github.go +++ b/github/github.go @@ -158,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. @@ -552,21 +556,6 @@ func WithVersion(version string) RequestOption { } } -// urlContainsDotDotPathSegment reports whether urlStr contains ".." as a path -// segment (e.g. "a/../b"). 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 urlContainsDotDotPathSegment(urlStr string) (bool, error) { - if !strings.Contains(urlStr, "..") { - return false, nil - } - u, err := url.Parse(urlStr) - if err != nil { - return false, err - } - return slices.Contains(strings.Split(u.Path, "/"), ".."), nil -} - // NewRequest creates an API request. A relative URL can be provided in urlStr, // in which case it is resolved relative to the BaseURL of the Client. // Relative URLs should always be specified without a preceding slash. If @@ -577,10 +566,8 @@ 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 hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + if err := checkURLPathTraversal(urlStr); err != nil { return nil, err - } else if hasDotDot { - return nil, ErrPathForbidden } u, err := c.BaseURL.Parse(urlStr) @@ -629,10 +616,8 @@ 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 hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + if err := checkURLPathTraversal(urlStr); err != nil { return nil, err - } else if hasDotDot { - return nil, ErrPathForbidden } u, err := c.BaseURL.Parse(urlStr) @@ -659,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. @@ -667,10 +671,8 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m return nil, fmt.Errorf("uploadURL must have a trailing slash, but %q does not", c.UploadURL) } - if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil { + if err := checkURLPathTraversal(urlStr); err != nil { return nil, err - } else if hasDotDot { - return nil, ErrPathForbidden } u, err := c.UploadURL.Parse(urlStr) diff --git a/github/github_test.go b/github/github_test.go index 99cbb8d3596..f1584b2794c 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -786,41 +786,36 @@ func TestNewRequest_errorForNoTrailingSlash(t *testing.T) { } } -func TestUrlContainsDotDotPathSegment(t *testing.T) { +func TestCheckURLPathTraversal(t *testing.T) { t.Parallel() tests := []struct { input string - want bool - wantErr bool + wantErr error }{ - {"repos/o/r/contents/file.txt", false, false}, - {"repos/o/r/contents/dir/file.txt", false, false}, - {"repos/o/r/contents/file..txt", false, false}, - {"repos/o/r?q=a..b", false, false}, - {"repos/../admin/users", true, false}, - {"repos/x/../../../admin", true, false}, - {"../admin", true, false}, - {"repos/o/r/contents/..", true, false}, - {"repos/o/r/contents/../secrets", true, false}, + {"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", true, false}, - {"https://api.github.com/repos/o/r/contents/file.txt", false, false}, - {"https://api.github.com/repos/o/r/contents/file..txt", false, false}, + {"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", false, false}, - {"repos/../admin#frag", true, false}, + {"repos/o/r/contents/file.txt#section", nil}, + {"repos/../admin#frag", ErrPathForbidden}, // URL with userinfo. - {"https://user:pass@api.github.com/repos/../admin", true, false}, - {"https://user:pass@api.github.com/repos/o/r", false, false}, + {"https://user:pass@api.github.com/repos/../admin", ErrPathForbidden}, + {"https://user:pass@api.github.com/repos/o/r", nil}, } for _, tt := range tests { - got, err := urlContainsDotDotPathSegment(tt.input) - if (err != nil) != tt.wantErr { - t.Errorf("urlContainsDotDotPathSegment(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) - continue - } - if got != tt.want { - t.Errorf("urlContainsDotDotPathSegment(%q) = %v, want %v", tt.input, got, tt.want) + err := checkURLPathTraversal(tt.input) + if !errors.Is(err, tt.wantErr) { + t.Errorf("checkURLPathTraversal(%q) = %v, want %v", tt.input, err, tt.wantErr) } } } diff --git a/github/repos_contents.go b/github/repos_contents.go index 78e83f688d8..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) @@ -278,10 +269,6 @@ func (s *RepositoriesService) GetContents(ctx context.Context, owner, repo, path // //meta:operation PUT /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { - if strings.Contains(path, "..") { - return nil, nil, ErrPathForbidden - } - u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("PUT", u, opts) if err != nil { @@ -304,10 +291,6 @@ func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path // //meta:operation PUT /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { - if strings.Contains(path, "..") { - return nil, nil, ErrPathForbidden - } - u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("PUT", u, opts) if err != nil { @@ -330,10 +313,6 @@ func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path // //meta:operation DELETE /repos/{owner}/{repo}/contents/{path} func (s *RepositoriesService) DeleteFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) { - if strings.Contains(path, "..") { - return nil, nil, ErrPathForbidden - } - u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path) req, err := s.client.NewRequest("DELETE", u, opts) if err != nil { diff --git a/github/repos_contents_test.go b/github/repos_contents_test.go index 50f71ecdd96..113aad6ebc4 100644 --- a/github/repos_contents_test.go +++ b/github/repos_contents_test.go @@ -702,36 +702,6 @@ func TestRepositoriesService_GetContents_Directory(t *testing.T) { } } -func TestRepositoriesService_CreateFile_PathWithParent(t *testing.T) { - t.Parallel() - client, _, _ := setup(t) - ctx := t.Context() - _, _, err := client.Repositories.CreateFile(ctx, "o", "r", "some/../other", nil) - if err == nil { - t.Fatal("Repositories.CreateFile expected error for path with '..' but got none") - } -} - -func TestRepositoriesService_UpdateFile_PathWithParent(t *testing.T) { - t.Parallel() - client, _, _ := setup(t) - ctx := t.Context() - _, _, err := client.Repositories.UpdateFile(ctx, "o", "r", "some/../other", nil) - if err == nil { - t.Fatal("Repositories.UpdateFile expected error for path with '..' but got none") - } -} - -func TestRepositoriesService_DeleteFile_PathWithParent(t *testing.T) { - t.Parallel() - client, _, _ := setup(t) - ctx := t.Context() - _, _, err := client.Repositories.DeleteFile(ctx, "o", "r", "some/../other", nil) - if err == nil { - t.Fatal("Repositories.DeleteFile expected error for path with '..' but got none") - } -} - func TestRepositoriesService_CreateFile(t *testing.T) { t.Parallel() client, mux, _ := setup(t)