diff --git a/pkg/raw/raw.go b/pkg/raw/raw.go index 7234dc69df..fcaa5c6af2 100644 --- a/pkg/raw/raw.go +++ b/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 } diff --git a/pkg/raw/raw_test.go b/pkg/raw/raw_test.go index c921d15149..8f6f54f4c7 100644 --- a/pkg/raw/raw_test.go +++ b/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) }