fix(github): escapePath returns error on dot-segments, fix Description semantics
- escapePath now returns an error when paths contain dot-segments
(".", "..") instead of silently rewriting them. This prevents
subtle API misses where callers pass "foo/../bar" expecting to
hit "bar" but the old code produced "foo/bar".
- Uses path.Clean for canonical form after validation.
- CommitStatus.Description for check runs is now empty string
instead of the raw conclusion enum. The conclusion is already
captured in the Status field via mapCheckRunStatus; storing it
again in Description was semantically inconsistent with commit
statuses where Description carries a human-readable narrative.
- Removed unused derefString helper.
- Added tests for escapePath valid paths, dot-segment rejection,
and GetFileContentAtRef dot-segment error propagation.
This commit is contained in:
+35
-16
@@ -6,24 +6,28 @@ import (
|
|||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
"net/url"
|
"net/url"
|
||||||
|
"path"
|
||||||
"strings"
|
"strings"
|
||||||
)
|
)
|
||||||
|
|
||||||
// GetFileContentAtRef fetches a file at a specific ref from a repo.
|
// GetFileContentAtRef fetches a file at a specific ref from a repo.
|
||||||
// If ref is empty, the query parameter is omitted (uses default branch).
|
// If ref is empty, the query parameter is omitted (uses default branch).
|
||||||
//
|
//
|
||||||
// Note: dot-segments ("." and "..") in the path are silently removed to
|
// Returns an error if the path contains dot-segments (".", "..") or
|
||||||
// prevent path traversal. This means a path like "foo/../bar" resolves
|
// attempts to traverse above the repository root.
|
||||||
// to "foo/bar" rather than "bar".
|
func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
|
||||||
func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, 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",
|
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 != "" {
|
if ref != "" {
|
||||||
reqURL += "?ref=" + url.QueryEscape(ref)
|
reqURL += "?ref=" + url.QueryEscape(ref)
|
||||||
}
|
}
|
||||||
body, err := c.doGet(ctx, reqURL)
|
body, err := c.doGet(ctx, reqURL)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "", fmt.Errorf("fetch file %s: %w", path, err)
|
return "", fmt.Errorf("fetch file %s: %w", filePath, err)
|
||||||
}
|
}
|
||||||
var resp struct {
|
var resp struct {
|
||||||
Content string `json:"content"`
|
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)
|
return "", fmt.Errorf("parse file content JSON: %w", err)
|
||||||
}
|
}
|
||||||
if resp.Encoding != "base64" {
|
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)
|
decoded, err := decodeBase64Content(resp.Content)
|
||||||
if err != nil {
|
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
|
return decoded, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// escapePath encodes each segment of a slash-separated path, stripping
|
// escapePath validates and encodes a slash-separated file path for use in
|
||||||
// dot-segments to prevent path traversal.
|
// GitHub API URLs. Returns an error if the path contains dot-segments ("."
|
||||||
func escapePath(p string) string {
|
// or "..") or resolves to a path outside the repository root.
|
||||||
parts := strings.Split(p, "/")
|
func escapePath(p string) (string, error) {
|
||||||
var clean []string
|
// 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 {
|
for _, part := range parts {
|
||||||
if part == "." || part == ".." || part == "" {
|
if part == "" {
|
||||||
continue
|
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.
|
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
+1
-8
@@ -178,7 +178,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
|
|||||||
result = append(result, vcs.CommitStatus{
|
result = append(result, vcs.CommitStatus{
|
||||||
Context: cr.Name,
|
Context: cr.Name,
|
||||||
Status: mapCheckRunStatus(cr.Conclusion),
|
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,
|
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
|
|
||||||
}
|
|
||||||
|
|||||||
Reference in New Issue
Block a user