fix(raw): reject traversal segments when constructing raw content URLs

pkg/raw/raw.go

@@ -3,12 +3,63 @@ package raw
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/url"
+	"strings"
 
 	gogithub "github.com/google/go-github/v89/github"
 )
 
+// errPathTraversal is returned when an owner, repo, ref/sha, or path
+// component used to build a raw content URL contains a ".." path segment,
+// either literally or via percent-decoding.
+var errPathTraversal = errors.New(`raw: path segment ".." is not allowed`)
+
+// rejectPathTraversal reports an error if any "/"-separated segment of the
+// given components is, or decodes to, "..". url.URL.JoinPath cleans the
+// joined path (resolving ".." segments) before producing the final URL, so a
+// ".." segment anywhere in owner, repo, ref/sha, or path could otherwise
+// rebind the resulting raw.githubusercontent.com URL to a different owner,
+// repository, or ref than the one requested.
+func rejectPathTraversal(components ...string) error {
+	for _, component := range components {
+		for segment := range strings.SplitSeq(component, "/") {
+			if err := rejectSegment(segment); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// rejectSegment reports an error if segment is, or decodes to, "..". A
+// percent-encoded separator (e.g. "%2f") can appear inside a single
+// "/"-separated segment and only becomes a "/" once decoded, revealing new
+// subsegments (e.g. "%2e%2e%2fsecret.txt" decodes to "../secret.txt"). To
+// catch that, whenever decoding changes the segment, the decoded form is
+// split on "/" again and each subsegment is checked recursively, so
+// traversal segments introduced by one or more layers of percent-decoding
+// are rejected regardless of where the encoded separator falls.
+func rejectSegment(segment string) error {
+	if segment == "" {
+		return nil
+	}
+	if segment == ".." {
+		return errPathTraversal
+	}
+	decoded, err := url.PathUnescape(segment)
+	if err != nil || decoded == segment {
+		return nil
+	}
+	for subsegment := range strings.SplitSeq(decoded, "/") {
+		if err := rejectSegment(subsegment); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // GetRawClientFn is a function type that returns a RawClient instance.
 type GetRawClientFn func(context.Context) (*Client, error)
 
@@ -34,14 +85,17 @@ func (c *Client) newRequest(ctx context.Context, method string, urlStr string, b
 	return c.client.NewRequest(ctx, method, urlStr, body, opts...)
 }
 
-func (c *Client) refURL(owner, repo, ref, path string) string {
+func (c *Client) refURL(owner, repo, ref, path string) (string, error) {
 	if ref == "" {
-		return c.url.JoinPath(owner, repo, "HEAD", path).String()
+		ref = "HEAD"
 	}
-	return c.url.JoinPath(owner, repo, ref, path).String()
+	if err := rejectPathTraversal(owner, repo, ref, path); err != nil {
+		return "", err
+	}
+	return c.url.JoinPath(owner, repo, ref, path).String(), nil
 }
 
-func (c *Client) URLFromOpts(opts *ContentOpts, owner, repo, path string) string {
+func (c *Client) URLFromOpts(opts *ContentOpts, owner, repo, path string) (string, error) {
 	if opts == nil {
 		opts = &ContentOpts{}
 	}
@@ -52,8 +106,11 @@ func (c *Client) URLFromOpts(opts *ContentOpts, owner, repo, path string) string
 }
 
 // BlobURL returns the URL for a blob in the raw content API.
-func (c *Client) commitURL(owner, repo, sha, path string) string {
-	return c.url.JoinPath(owner, repo, sha, path).String()
+func (c *Client) commitURL(owner, repo, sha, path string) (string, error) {
+	if err := rejectPathTraversal(owner, repo, sha, path); err != nil {
+		return "", err
+	}
+	return c.url.JoinPath(owner, repo, sha, path).String(), nil
 }
 
 type ContentOpts struct {
@@ -63,8 +120,11 @@ type ContentOpts struct {
 
 // GetRawContent fetches the raw content of a file from a GitHub repository.
 func (c *Client) GetRawContent(ctx context.Context, owner, repo, path string, opts *ContentOpts) (*http.Response, error) {
-	url := c.URLFromOpts(opts, owner, repo, path)
-	req, err := c.newRequest(ctx, "GET", url, nil)
+	rawURL, err := c.URLFromOpts(opts, owner, repo, path)
+	if err != nil {
+		return nil, err
+	}
+	req, err := c.newRequest(ctx, "GET", rawURL, nil)
 	if err != nil {
 		return nil, err
 	}

pkg/raw/raw_test.go

@@ -96,6 +96,24 @@ func TestGetRawContent(t *testing.T) {
 			contentType: "application/json",
 			body:        `{"message": "Not Found"}`,
 		},
+		{
+			name:        "benign double-dot filename",
+			opts:        nil,
+			owner:       "octocat",
+			repo:        "hello",
+			path:        "file..txt",
+			statusCode:  200,
+			contentType: "text/plain",
+			body:        "# Test file",
+		},
+		{
+			name:        "literal path traversal rejected",
+			opts:        nil,
+			owner:       "octocat",
+			repo:        "hello",
+			path:        "../evilowner/evilrepo/HEAD/secret.txt",
+			expectError: "path traversal",
+		},
 	}
 
 	for _, tc := range tests {
@@ -113,9 +131,11 @@ func TestGetRawContent(t *testing.T) {
 			client, err := NewClient(ghClient, base)
 			require.NoError(t, err)
 			resp, err := client.GetRawContent(context.Background(), tc.owner, tc.repo, tc.path, tc.opts)
-			defer func() {
-				_ = resp.Body.Close()
-			}()
+			if resp != nil {
+				defer func() {
+					_ = resp.Body.Close()
+				}()
+			}
 
 			if tc.expectError != "" {
 				require.Error(t, err)
@@ -125,7 +145,8 @@ func TestGetRawContent(t *testing.T) {
 			require.Equal(t, tc.statusCode, resp.StatusCode)
 
 			// Verify the URL was constructed correctly
-			actualURL := client.URLFromOpts(tc.opts, tc.owner, tc.repo, tc.path)
+			actualURL, err := client.URLFromOpts(tc.opts, tc.owner, tc.repo, tc.path)
+			require.NoError(t, err)
 			require.True(t, strings.Contains(actualURL, tc.owner))
 			require.True(t, strings.Contains(actualURL, tc.repo))
 			require.True(t, strings.Contains(actualURL, tc.path))
@@ -141,12 +162,13 @@ func TestUrlFromOpts(t *testing.T) {
 	require.NoError(t, err)
 
 	tests := []struct {
-		name  string
-		opts  *ContentOpts
-		owner string
-		repo  string
-		path  string
-		want  string
+		name    string
+		opts    *ContentOpts
+		owner   string
+		repo    string
+		path    string
+		want    string
+		wantErr bool
 	}{
 		{
 			name:  "no opts (HEAD)",
@@ -172,11 +194,112 @@ func TestUrlFromOpts(t *testing.T) {
 			owner: "octocat", repo: "hello", path: "README.md",
 			want: "https://raw.example.com/octocat/hello/abc123/README.md",
 		},
+		{
+			name:  "nested path",
+			opts:  &ContentOpts{Ref: "refs/heads/main"},
+			owner: "octocat", repo: "hello", path: "src/pkg/deep/file.go",
+			want: "https://raw.example.com/octocat/hello/refs/heads/main/src/pkg/deep/file.go",
+		},
+		{
+			name:  "benign double-dot filename",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "file..txt",
+			want: "https://raw.example.com/octocat/hello/HEAD/file..txt",
+		},
+		{
+			name:  "benign leading double-dot filename",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "..hidden",
+			want: "https://raw.example.com/octocat/hello/HEAD/..hidden",
+		},
+		{
+			name:  "literal dot-dot segment rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "../secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "nested literal dot-dot segment rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "docs/../../secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "rebinding traversal rejected",
+			opts:  &ContentOpts{Ref: "main"},
+			owner: "octocat", repo: "hello", path: "../../evilowner/evilrepo/main/secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "percent-encoded dot-dot segment rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%2e%2e/secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "uppercase percent-encoded dot-dot segment rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%2E%2E/secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "partially percent-encoded dot-dot segment rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%2e./secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "dot-dot in ref rejected",
+			opts:  &ContentOpts{Ref: "../evilref"},
+			owner: "octocat", repo: "hello", path: "README.md",
+			wantErr: true,
+		},
+		{
+			name:  "encoded separator revealing dot-dot rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%2e%2e%2fsecret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "encoded separator revealing dot-dot mid-path rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "docs%2f..%2f..%2fsecret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "encoded separator in owner rejected",
+			opts:  nil,
+			owner: "octocat%2f..", repo: "hello", path: "README.md",
+			wantErr: true,
+		},
+		{
+			name:  "double percent-encoded dot-dot rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%252e%252e/secret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "double percent-encoded separator revealing dot-dot rejected",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%252e%252e%252fsecret.txt",
+			wantErr: true,
+		},
+		{
+			name:  "benign percent-encoded filename allowed",
+			opts:  nil,
+			owner: "octocat", repo: "hello", path: "%2ehidden",
+			want: "https://raw.example.com/octocat/hello/HEAD/%2ehidden",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := client.URLFromOpts(tt.opts, tt.owner, tt.repo, tt.path)
+			got, err := client.URLFromOpts(tt.opts, tt.owner, tt.repo, tt.path)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
 			if got != tt.want {
 				t.Errorf("UrlFromOpts() = %q, want %q", got, tt.want)
 			}