From a3da71594369507cf5af0c3eae7a3aaae25a4325 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 14:11:19 +0200 Subject: [PATCH 1/2] fix(raw): reject traversal segments when constructing raw content URLs url.URL.JoinPath normalizes ".." segments before producing the final URL. A path containing enough parent-directory segments could therefore consume the owner, repo, and ref components already joined onto the base URL, rebinding the raw.githubusercontent.com request to a different owner/repository/ref than the caller specified. Reject any owner, repo, ref/sha, or path component whose "/"-separated segments are, or percent-decode to, ".." before building the URL. Benign filenames such as "file..txt" or "..hidden" are unaffected. URLFromOpts, refURL, and commitURL now return an error alongside the URL string so this can be enforced at construction time; GetRawContent propagates it. Adds table-driven tests covering normal, nested, and benign double-dot paths as well as literal and percent-encoded traversal attempts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/raw/raw.go | 58 +++++++++++++++++++---- pkg/raw/raw_test.go | 109 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 148 insertions(+), 19 deletions(-) diff --git a/pkg/raw/raw.go b/pkg/raw/raw.go index 7234dc69df..9b63cb0de5 100644 --- a/pkg/raw/raw.go +++ b/pkg/raw/raw.go @@ -3,12 +3,45 @@ 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 segment == "" { + continue + } + if segment == ".." { + return errPathTraversal + } + // Guard against percent-encoded traversal (e.g. "%2e%2e") in case + // the segment is later decoded before being treated as a path + // component. + if decoded, err := url.PathUnescape(segment); err == nil && decoded == ".." { + return errPathTraversal + } + } + } + return nil +} + // GetRawClientFn is a function type that returns a RawClient instance. type GetRawClientFn func(context.Context) (*Client, error) @@ -34,14 +67,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" + } + if err := rejectPathTraversal(owner, repo, ref, path); err != nil { + return "", err } - return c.url.JoinPath(owner, repo, ref, path).String() + 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 +88,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 +102,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..c326768032 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,76 @@ 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, + }, } 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) } From 65fe088c2ab22b0bca21fee2397c413d24d20bad Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 14:48:34 +0200 Subject: [PATCH 2/2] fix(raw): reject dot-dot segments revealed by decoding encoded separators rejectPathTraversal split components on literal "/" before checking each segment, so a segment containing an encoded separator (e.g. "%2e%2e%2fsecret.txt") decoded to "../secret.txt" instead of "..", and the check never caught it. Percent-decoding a segment can therefore introduce new "/"-separated subsegments that were invisible to the original literal split. Recursively re-split and re-check the decoded form whenever decoding changes a segment, so a ".." revealed by one or more layers of percent-decoding (including through an encoded separator, or double-encoding) is rejected regardless of where it appears. Add regression tests for encoded-separator traversal, encoded separators in other components, and double percent-encoded dot-dot segments, plus a benign percent-encoded filename case to confirm non-traversal decodes still pass through. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/raw/raw.go | 40 +++++++++++++++++++++++++++++----------- pkg/raw/raw_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/pkg/raw/raw.go b/pkg/raw/raw.go index 9b63cb0de5..fcaa5c6af2 100644 --- a/pkg/raw/raw.go +++ b/pkg/raw/raw.go @@ -25,23 +25,41 @@ var errPathTraversal = errors.New(`raw: path segment ".." is not allowed`) func rejectPathTraversal(components ...string) error { for _, component := range components { for segment := range strings.SplitSeq(component, "/") { - if segment == "" { - continue - } - if segment == ".." { - return errPathTraversal - } - // Guard against percent-encoded traversal (e.g. "%2e%2e") in case - // the segment is later decoded before being treated as a path - // component. - if decoded, err := url.PathUnescape(segment); err == nil && decoded == ".." { - return errPathTraversal + 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) diff --git a/pkg/raw/raw_test.go b/pkg/raw/raw_test.go index c326768032..8f6f54f4c7 100644 --- a/pkg/raw/raw_test.go +++ b/pkg/raw/raw_test.go @@ -254,6 +254,42 @@ func TestUrlFromOpts(t *testing.T) { 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 {