fix: Reject URL path segments containing ".." in all request methods by mohammadmseet-hue · Pull Request #4150 · google/go-github

mohammadmseet-hue · 2026-04-13T19:14:01Z

Summary

The existing ErrPathForbidden check in GetContents (added in #2805) correctly blocks path traversal via the path parameter, with the comment:

path must not contain '..' due to auth vulnerability issue

However, the same validation was missing in two places:

NewRequest, NewFormRequest, NewUploadRequest — owner, repo, org, user, enterprise, and team parameters are interpolated into URL paths via fmt.Sprintf before being passed to these methods. A value like x/../../../admin in any of these parameters causes the resolved URL to reach unintended API endpoints, and the client's Authorization header is sent along with the request.
CreateFile, UpdateFile, DeleteFile — these use the same repos/%v/%v/contents/%v URL pattern as GetContents but lacked the .. check on the path parameter.

Fix

Adds a centralized containsDotDotPathSegment check in all three request-building methods (NewRequest, NewFormRequest, NewUploadRequest). This 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.
Adds the same strings.Contains(path, "..") guard to CreateFile, UpdateFile, and DeleteFile for defense in depth (consistent with GetContents).

Example

// owner parameter traversal — before this fix, the auth token is sent to /admin/users
client.Repositories.GetReadme(ctx, "x/../../../admin", "users", nil)
// resolved URL: https://api.github.com/admin/users/readme
// Authorization: Bearer <token> leaked to unintended endpoint

// After this fix: returns ErrPathForbidden

Impact

Any application using go-github that passes user-controlled input as owner, repo, or org parameters could have its authentication token sent to unintended GitHub API endpoints. This affects all API methods that interpolate these parameters into URL paths.

Test plan

Added TestContainsDotDotPathSegment — unit tests for the helper function
Added TestNewRequest_pathTraversal — verifies NewRequest rejects .. path segments
Added TestNewFormRequest_pathTraversal — verifies NewFormRequest rejects .. path segments
Added TestNewUploadRequest_pathTraversal — verifies NewUploadRequest rejects .. path segments
Added TestRepositoriesService_CreateFile_PathWithParent — verifies CreateFile rejects .. in path
Added TestRepositoriesService_UpdateFile_PathWithParent — verifies UpdateFile rejects .. in path
Added TestRepositoriesService_DeleteFile_PathWithParent — verifies DeleteFile rejects .. in path
Full test suite passes (go test ./github/)

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

codecov · 2026-04-13T20:14:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.76%. Comparing base (44908ea) to head (33a356c).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4150   +/-   ##
=======================================
  Coverage   93.76%   93.76%           
=======================================
  Files         211      211           
  Lines       19701    19714   +13     
=======================================
+ Hits        18472    18485   +13     
  Misses       1031     1031           
  Partials      198      198

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gmlewis

Thank you, @mohammadmseet-hue!
One minor tweak, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode

github/github.go

@gmlewis

Apply review suggestion from @gmlewis to simplify the loop into a single slices.Contains call.

stevehipwell · 2026-04-14T08:09:10Z

github/github.go

+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]
+	}
+	return slices.Contains(strings.Split(path, "/"), "..")
+}


Suggested change

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]

}

return slices.Contains(strings.Split(path, "/"), "..")

}

func containsDotDotPathSegment(urlStr string) bool {

if !strings.Contains(urlStr, "..") {

return false

}

u, err := url.Parse(urlStr)

if err != nil {

// Not a valid URL so no path segment

return false

}

return slices.Contains(u.Path, "..")

}

Is there a reason you using url.Parse() (which we use elsewhere in this package)?

I'd also suggest changing the name from containsDotDotPathSegment to urlContainsDotDotPathSegment and changing the signature to return an error if the URL can't be parsed.

stevehipwell · 2026-04-14T08:10:17Z

github/github_test.go

 	}
 }

+func TestContainsDotDotPathSegment(t *testing.T) {


Is it intentional to not have any full URLs in here with a scheme, user or fragment?

mohammadmseet-hue · 2026-04-14T13:34:06Z

Thanks for the review @stevehipwell!

Good call on using url.Parse() — cleaner and consistent with the rest of the package.

Small note on the code suggestion: slices.Contains(u.Path, "..") won't compile since u.Path is a string, not a slice — used slices.Contains(strings.Split(u.Path, "/"), "..") instead.

Applied all your suggestions:

Switched to url.Parse() for path extraction
Renamed to urlContainsDotDotPathSegment
Changed signature to return (bool, error)
Added full URL test cases (scheme, userinfo, fragment)

alexandear · 2026-04-14T13:39:02Z

github/github.go

+	if hasDotDot, err := urlContainsDotDotPathSegment(urlStr); err != nil {
+		return nil, err
+	} else if hasDotDot {
+		return nil, ErrPathForbidden


Please move the declaration ErrPathForbidden to this file.

alexandear · 2026-04-14T13:40:08Z

github/github.go

This logic duplicates in NewRequest and NewFormRequest. We should extract into a helper.

alexandear · 2026-04-14T13:41:22Z

github/repos_contents.go

+	if strings.Contains(path, "..") {
+		return nil, nil, ErrPathForbidden
+	}


This is already handled by s.client.NewRequest. Do we need it here? The same for other RepositoriesService functions: GetContents, UpdateFile, DeleteFile.

alexandear · 2026-04-14T13:42:51Z

github/github.go

+// 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) {


Let's declare this unexported function after the NewFormRequest for readability.

mohammadmseet-hue · 2026-04-14T13:47:59Z

Thanks for the review @alexandear! Applied all 4 changes:

Moved ErrPathForbidden to github.go
Simplified to checkURLPathTraversal(urlStr string) error — eliminates the duplicated 4-line calling pattern across NewRequest/NewFormRequest/NewUploadRequest
Moved the function after NewFormRequest for readability
Removed redundant .. checks from GetContents, CreateFile, UpdateFile, DeleteFile — NewRequest already handles it centrally

Net result: -101 lines added, +47 lines — cleaner and less duplication.

@stevehipwell

…rn error 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)

- 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

alexandear · 2026-04-14T14:38:47Z

github/github.go

+	if err := checkURLPathTraversal(urlStr); err != nil {
+		return nil, err
+	}
+
 	u, err := c.BaseURL.Parse(urlStr)


Here we are calling url.Parse twice. Could we eliminate the redundant parse?

I believe the double url.Parse is actually necessary here. The fast path (strings.Contains) exits without any parsing for 99%+ of calls — url.Parse inside checkURLPathTraversal only runs when .. is present, and in that case we almost always return ErrPathForbidden before reaching c.BaseURL.Parse.

We also can't move the check after c.BaseURL.Parse — it resolves .. segments during URL resolution (e.g. repos/x/../../../admin → /admin), so the .. would be gone and the check would never trigger. The traversal detection has to happen on the raw input before resolution.

The only case where both parses run is something like repos/owner/file..txt (contains .. substring but not as a path segment) — edge case with negligible cost.

stevehipwell

LGTM

gmlewis changed the title ~~fix: reject URL path segments containing ".." in all request methods~~ fix: Reject URL path segments containing ".." in all request methods Apr 13, 2026

gmlewis added the NeedsReview PR is awaiting a review before merging. label Apr 13, 2026

gmlewis requested changes Apr 13, 2026

View reviewed changes

github/github.go Outdated Show resolved Hide resolved

refactor: use slices.Contains in containsDotDotPathSegment

cb706ec

Apply review suggestion from @gmlewis to simplify the loop into a single slices.Contains call.

gmlewis approved these changes Apr 13, 2026

View reviewed changes

stevehipwell suggested changes Apr 14, 2026

View reviewed changes

alexandear suggested changes Apr 14, 2026

View reviewed changes

gmlewis added DO NOT MERGE Do not merge this PR. waiting for signed CLA labels Apr 14, 2026

mohammadmseet-hue added 2 commits April 14, 2026 15:53

mohammadmseet-hue force-pushed the fix/path-traversal-validation branch from 8a102a0 to 33a356c Compare April 14, 2026 13:54

gmlewis removed DO NOT MERGE Do not merge this PR. waiting for signed CLA labels Apr 14, 2026

alexandear reviewed Apr 14, 2026

View reviewed changes

alexandear approved these changes Apr 15, 2026

View reviewed changes

stevehipwell approved these changes Apr 15, 2026

View reviewed changes

munlicode approved these changes Apr 15, 2026

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Apr 15, 2026

Conversation

mohammadmseet-hue commented Apr 13, 2026

Summary

Fix

Example

Impact

Test plan

Uh oh!

codecov bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohammadmseet-hue commented Apr 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohammadmseet-hue commented Apr 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 13, 2026 •

edited

Loading