Skip to content

Commit 8a102a0

Browse files
mohammadmseet-hueclaude
mohammadmseet-hue
and
claude
committed
refactor: address alexandear's review feedback
- 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 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9a3a8e4 commit 8a102a0

4 files changed

Lines changed: 47 additions & 101 deletions

File tree

‎github/github.go‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,10 @@ const (
158158

159159
var errNonNilContext = errors.New("context must be non-nil")
160160

161+
// ErrPathForbidden is returned when a URL path contains ".." as a path
162+
// segment, which could allow path traversal attacks.
163+
var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulnerability issue")
164+
161165
// A Client manages communication with the GitHub API.
162166
type Client struct {
163167
clientMu sync.Mutex // clientMu protects the client during calls that modify the CheckRedirect func.
@@ -552,21 +556,6 @@ func WithVersion(version string) RequestOption {
552556
}
553557
}
554558

555-
// urlContainsDotDotPathSegment reports whether urlStr contains ".." as a path
556-
// segment (e.g. "a/../b"). It does not match ".." embedded within a segment
557-
// (e.g. "file..txt"). The check is performed only on the path portion of the
558-
// URL, ignoring any query string or fragment.
559-
func urlContainsDotDotPathSegment(urlStr string) (bool, error) {
560-
if !strings.Contains(urlStr, "..") {
561-
return false, nil
562-
}
563-
u, err := url.Parse(urlStr)
564-
if err != nil {
565-
return false, err
566-
}
567-
return slices.Contains(strings.Split(u.Path, "/"), ".."), nil
568-
}
569-
570559
// NewRequest creates an API request. A relative URL can be provided in urlStr,
571560
// in which case it is resolved relative to the BaseURL of the Client.
572561
// 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
577566
return nil, fmt.Errorf("baseURL must have a trailing slash, but %q does not", c.BaseURL)
578567
}
579568

580-
if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil {
569+
if err := checkURLPathTraversal(urlStr); err != nil {
581570
return nil, err
582-
} else if hasDotDot {
583-
return nil, ErrPathForbidden
584571
}
585572

586573
u, err := c.BaseURL.Parse(urlStr)
@@ -629,10 +616,8 @@ func (c *Client) NewFormRequest(urlStr string, body io.Reader, opts ...RequestOp
629616
return nil, fmt.Errorf("baseURL must have a trailing slash, but %q does not", c.BaseURL)
630617
}
631618

632-
if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil {
619+
if err := checkURLPathTraversal(urlStr); err != nil {
633620
return nil, err
634-
} else if hasDotDot {
635-
return nil, ErrPathForbidden
636621
}
637622

638623
u, err := c.BaseURL.Parse(urlStr)
@@ -659,6 +644,25 @@ func (c *Client) NewFormRequest(urlStr string, body io.Reader, opts ...RequestOp
659644
return req, nil
660645
}
661646

647+
// checkURLPathTraversal returns ErrPathForbidden if urlStr contains ".." as a
648+
// path segment (e.g. "a/../b"), preventing path traversal attacks. It does not
649+
// match ".." embedded within a segment (e.g. "file..txt"). The check is
650+
// performed only on the path portion of the URL, ignoring any query string or
651+
// fragment.
652+
func checkURLPathTraversal(urlStr string) error {
653+
if !strings.Contains(urlStr, "..") {
654+
return nil
655+
}
656+
u, err := url.Parse(urlStr)
657+
if err != nil {
658+
return err
659+
}
660+
if slices.Contains(strings.Split(u.Path, "/"), "..") {
661+
return ErrPathForbidden
662+
}
663+
return nil
664+
}
665+
662666
// NewUploadRequest creates an upload request. A relative URL can be provided in
663667
// urlStr, in which case it is resolved relative to the UploadURL of the Client.
664668
// 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
667671
return nil, fmt.Errorf("uploadURL must have a trailing slash, but %q does not", c.UploadURL)
668672
}
669673

670-
if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil {
674+
if err := checkURLPathTraversal(urlStr); err != nil {
671675
return nil, err
672-
} else if hasDotDot {
673-
return nil, ErrPathForbidden
674676
}
675677

676678
u, err := c.UploadURL.Parse(urlStr)

‎github/github_test.go‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -786,41 +786,36 @@ func TestNewRequest_errorForNoTrailingSlash(t *testing.T) {
786786
}
787787
}
788788

789-
func TestUrlContainsDotDotPathSegment(t *testing.T) {
789+
func TestCheckURLPathTraversal(t *testing.T) {
790790
t.Parallel()
791791
tests := []struct {
792792
input string
793-
want bool
794-
wantErr bool
793+
wantErr error
795794
}{
796-
{"repos/o/r/contents/file.txt", false, false},
797-
{"repos/o/r/contents/dir/file.txt", false, false},
798-
{"repos/o/r/contents/file..txt", false, false},
799-
{"repos/o/r?q=a..b", false, false},
800-
{"repos/../admin/users", true, false},
801-
{"repos/x/../../../admin", true, false},
802-
{"../admin", true, false},
803-
{"repos/o/r/contents/..", true, false},
804-
{"repos/o/r/contents/../secrets", true, false},
795+
{"repos/o/r/contents/file.txt", nil},
796+
{"repos/o/r/contents/dir/file.txt", nil},
797+
{"repos/o/r/contents/file..txt", nil},
798+
{"repos/o/r?q=a..b", nil},
799+
{"repos/../admin/users", ErrPathForbidden},
800+
{"repos/x/../../../admin", ErrPathForbidden},
801+
{"../admin", ErrPathForbidden},
802+
{"repos/o/r/contents/..", ErrPathForbidden},
803+
{"repos/o/r/contents/../secrets", ErrPathForbidden},
805804
// Full URLs with scheme.
806-
{"https://api.github.com/repos/../admin", true, false},
807-
{"https://api.github.com/repos/o/r/contents/file.txt", false, false},
808-
{"https://api.github.com/repos/o/r/contents/file..txt", false, false},
805+
{"https://api.github.com/repos/../admin", ErrPathForbidden},
806+
{"https://api.github.com/repos/o/r/contents/file.txt", nil},
807+
{"https://api.github.com/repos/o/r/contents/file..txt", nil},
809808
// URL with fragment.
810-
{"repos/o/r/contents/file.txt#section", false, false},
811-
{"repos/../admin#frag", true, false},
809+
{"repos/o/r/contents/file.txt#section", nil},
810+
{"repos/../admin#frag", ErrPathForbidden},
812811
// URL with userinfo.
813-
{"https://user:pass@api.github.com/repos/../admin", true, false},
814-
{"https://user:pass@api.github.com/repos/o/r", false, false},
812+
{"https://user:pass@api.github.com/repos/../admin", ErrPathForbidden},
813+
{"https://user:pass@api.github.com/repos/o/r", nil},
815814
}
816815
for _, tt := range tests {
817-
got, err := urlContainsDotDotPathSegment(tt.input)
818-
if (err != nil) != tt.wantErr {
819-
t.Errorf("urlContainsDotDotPathSegment(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr)
820-
continue
821-
}
822-
if got != tt.want {
823-
t.Errorf("urlContainsDotDotPathSegment(%q) = %v, want %v", tt.input, got, tt.want)
816+
err := checkURLPathTraversal(tt.input)
817+
if !errors.Is(err, tt.wantErr) {
818+
t.Errorf("checkURLPathTraversal(%q) = %v, want %v", tt.input, err, tt.wantErr)
824819
}
825820
}
826821
}

‎github/repos_contents.go‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import (
2121
"strings"
2222
)
2323

24-
var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulnerability issue")
25-
2624
// RepositoryContent represents a file or directory in a github repository.
2725
type RepositoryContent struct {
2826
Type *string `json:"type,omitempty"`
@@ -229,17 +227,10 @@ func (s *RepositoriesService) DownloadContentsWithMeta(ctx context.Context, owne
229227
// as possible, both result types will be returned but only one will contain a
230228
// value and the other will be nil.
231229
//
232-
// Due to an auth vulnerability issue in the GitHub v3 API, ".." is not allowed
233-
// to appear anywhere in the "path" or this method will return an error.
234-
//
235230
// GitHub API docs: https://docs.github.com/rest/repos/contents?apiVersion=2022-11-28#get-repository-content
236231
//
237232
//meta:operation GET /repos/{owner}/{repo}/contents/{path}
238233
func (s *RepositoriesService) GetContents(ctx context.Context, owner, repo, path string, opts *RepositoryContentGetOptions) (fileContent *RepositoryContent, directoryContent []*RepositoryContent, resp *Response, err error) {
239-
if strings.Contains(path, "..") {
240-
return nil, nil, nil, ErrPathForbidden
241-
}
242-
243234
escapedPath := (&url.URL{Path: strings.TrimSuffix(path, "/")}).String()
244235
u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, escapedPath)
245236
u, err = addOptions(u, opts)
@@ -278,10 +269,6 @@ func (s *RepositoriesService) GetContents(ctx context.Context, owner, repo, path
278269
//
279270
//meta:operation PUT /repos/{owner}/{repo}/contents/{path}
280271
func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) {
281-
if strings.Contains(path, "..") {
282-
return nil, nil, ErrPathForbidden
283-
}
284-
285272
u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path)
286273
req, err := s.client.NewRequest("PUT", u, opts)
287274
if err != nil {
@@ -304,10 +291,6 @@ func (s *RepositoriesService) CreateFile(ctx context.Context, owner, repo, path
304291
//
305292
//meta:operation PUT /repos/{owner}/{repo}/contents/{path}
306293
func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) {
307-
if strings.Contains(path, "..") {
308-
return nil, nil, ErrPathForbidden
309-
}
310-
311294
u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path)
312295
req, err := s.client.NewRequest("PUT", u, opts)
313296
if err != nil {
@@ -330,10 +313,6 @@ func (s *RepositoriesService) UpdateFile(ctx context.Context, owner, repo, path
330313
//
331314
//meta:operation DELETE /repos/{owner}/{repo}/contents/{path}
332315
func (s *RepositoriesService) DeleteFile(ctx context.Context, owner, repo, path string, opts *RepositoryContentFileOptions) (*RepositoryContentResponse, *Response, error) {
333-
if strings.Contains(path, "..") {
334-
return nil, nil, ErrPathForbidden
335-
}
336-
337316
u := fmt.Sprintf("repos/%v/%v/contents/%v", owner, repo, path)
338317
req, err := s.client.NewRequest("DELETE", u, opts)
339318
if err != nil {

‎github/repos_contents_test.go‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -702,36 +702,6 @@ func TestRepositoriesService_GetContents_Directory(t *testing.T) {
702702
}
703703
}
704704

705-
func TestRepositoriesService_CreateFile_PathWithParent(t *testing.T) {
706-
t.Parallel()
707-
client, _, _ := setup(t)
708-
ctx := t.Context()
709-
_, _, err := client.Repositories.CreateFile(ctx, "o", "r", "some/../other", nil)
710-
if err == nil {
711-
t.Fatal("Repositories.CreateFile expected error for path with '..' but got none")
712-
}
713-
}
714-
715-
func TestRepositoriesService_UpdateFile_PathWithParent(t *testing.T) {
716-
t.Parallel()
717-
client, _, _ := setup(t)
718-
ctx := t.Context()
719-
_, _, err := client.Repositories.UpdateFile(ctx, "o", "r", "some/../other", nil)
720-
if err == nil {
721-
t.Fatal("Repositories.UpdateFile expected error for path with '..' but got none")
722-
}
723-
}
724-
725-
func TestRepositoriesService_DeleteFile_PathWithParent(t *testing.T) {
726-
t.Parallel()
727-
client, _, _ := setup(t)
728-
ctx := t.Context()
729-
_, _, err := client.Repositories.DeleteFile(ctx, "o", "r", "some/../other", nil)
730-
if err == nil {
731-
t.Fatal("Repositories.DeleteFile expected error for path with '..' but got none")
732-
}
733-
}
734-
735705
func TestRepositoriesService_CreateFile(t *testing.T) {
736706
t.Parallel()
737707
client, mux, _ := setup(t)

0 commit comments

Comments
 (0)