diff --git a/github/files.go b/github/files.go index f7d415d..d0f7ecc 100644 --- a/github/files.go +++ b/github/files.go @@ -6,24 +6,28 @@ import ( "encoding/json" "fmt" "net/url" + "path" "strings" ) // GetFileContentAtRef fetches a file at a specific ref from a repo. // If ref is empty, the query parameter is omitted (uses default branch). // -// Note: dot-segments ("." and "..") in the path are silently removed to -// prevent path traversal. This means a path like "foo/../bar" resolves -// to "foo/bar" rather than "bar". -func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) { +// Returns an error if the path contains dot-segments (".", "..") or +// attempts to traverse above the repository root. +func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, ref string) (string, error) { + escaped, err := escapePath(filePath) + if err != nil { + return "", fmt.Errorf("invalid file path: %w", err) + } reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path)) + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escaped) if ref != "" { reqURL += "?ref=" + url.QueryEscape(ref) } body, err := c.doGet(ctx, reqURL) if err != nil { - return "", fmt.Errorf("fetch file %s: %w", path, err) + return "", fmt.Errorf("fetch file %s: %w", filePath, err) } var resp struct { Content string `json:"content"` @@ -33,27 +37,42 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref return "", fmt.Errorf("parse file content JSON: %w", err) } if resp.Encoding != "base64" { - return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path) + return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, filePath) } decoded, err := decodeBase64Content(resp.Content) if err != nil { - return "", fmt.Errorf("decode base64 content for %s: %w", path, err) + return "", fmt.Errorf("decode base64 content for %s: %w", filePath, err) } return decoded, nil } -// escapePath encodes each segment of a slash-separated path, stripping -// dot-segments to prevent path traversal. -func escapePath(p string) string { - parts := strings.Split(p, "/") - var clean []string +// escapePath validates and encodes a slash-separated file path for use in +// GitHub API URLs. Returns an error if the path contains dot-segments ("." +// or "..") or resolves to a path outside the repository root. +func escapePath(p string) (string, error) { + // Reject paths containing dot-segments rather than silently rewriting them. + for _, seg := range strings.Split(p, "/") { + if seg == "." || seg == ".." { + return "", fmt.Errorf("path contains dot-segment %q: %s", seg, p) + } + } + + // Use path.Clean for canonical form, then verify it doesn't escape root. + cleaned := path.Clean(p) + if cleaned == "." || strings.HasPrefix(cleaned, "..") { + return "", fmt.Errorf("path resolves outside repository root: %s", p) + } + + // Encode each segment individually. + parts := strings.Split(cleaned, "/") + var encoded []string for _, part := range parts { - if part == "." || part == ".." || part == "" { + if part == "" { continue } - clean = append(clean, url.PathEscape(part)) + encoded = append(encoded, url.PathEscape(part)) } - return strings.Join(clean, "/") + return strings.Join(encoded, "/"), nil } // decodeBase64Content decodes base64-encoded content from the GitHub contents API. diff --git a/github/files_test.go b/github/files_test.go new file mode 100644 index 0000000..62c5412 --- /dev/null +++ b/github/files_test.go @@ -0,0 +1,79 @@ +package github + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestEscapePath_ValidPaths(t *testing.T) { + t.Parallel() + tests := []struct { + name string + path string + want string + }{ + {"simple file", "file.go", "file.go"}, + {"nested path", "path/to/file.go", "path/to/file.go"}, + {"special chars", "path/to/my file.go", "path/to/my%20file.go"}, + {"leading slash stripped", "/path/to/file.go", "path/to/file.go"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := escapePath(tt.path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("escapePath(%q) = %q, want %q", tt.path, got, tt.want) + } + }) + } +} + +func TestEscapePath_DotSegments(t *testing.T) { + t.Parallel() + tests := []struct { + name string + path string + }{ + {"single dot", "./file.go"}, + {"double dot", "../file.go"}, + {"dot in middle", "path/./file.go"}, + {"parent traversal", "path/../file.go"}, + {"only dots", ".."}, + {"nested parent traversal", "a/b/../../c"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := escapePath(tt.path) + if err == nil { + t.Fatalf("expected error for path %q, got nil", tt.path) + } + if !strings.Contains(err.Error(), "dot-segment") { + t.Errorf("expected error about dot-segment, got: %v", err) + } + }) + } +} + +func TestGetFileContentAtRef_DotSegmentError(t *testing.T) { + // Server should never be called — the error is caught before the request. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("server should not have been called") + })) + defer srv.Close() + + c := NewClient(srv.URL, "token") + _, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main") + if err == nil { + t.Fatal("expected error for path with dot-segments") + } + if !strings.Contains(err.Error(), "invalid file path") { + t.Errorf("expected 'invalid file path' error, got: %v", err) + } +} diff --git a/github/pr.go b/github/pr.go index c028506..2aa4c79 100644 --- a/github/pr.go +++ b/github/pr.go @@ -178,7 +178,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) result = append(result, vcs.CommitStatus{ Context: cr.Name, Status: mapCheckRunStatus(cr.Conclusion), - Description: derefString(cr.Conclusion), // raw conclusion value (e.g. "success", "failure", "skipped") + Description: "", // check runs have no human-readable description; conclusion is captured in Status TargetURL: cr.HTMLURL, }) } @@ -220,10 +220,3 @@ func mapCheckRunStatus(conclusion *string) string { } } -// derefString safely dereferences a string pointer, returning empty string if nil. -func derefString(s *string) string { - if s == nil { - return "" - } - return *s -}