feat(github): implement PRReader interface #102

Merged
aweiker merged 5 commits from issue-80-b-pr-reader into feature/github-support 2026-05-13 05:30:37 +00:00
2 changed files with 75 additions and 2 deletions
Showing only changes of commit 289b400bfd - Show all commits
+7 -2
View File
@@ -1,5 +1,10 @@
package github_test
import "gitea.weiker.me/rodin/review-bot/vcs"
import (
"gitea.weiker.me/rodin/review-bot/github"
"gitea.weiker.me/rodin/review-bot/vcs"
)
var _ vcs.PRReader = (*Client)(nil)
// Compile-time interface conformance assertion.
// Verifies github.Client satisfies vcs.PRReader.
var _ vcs.PRReader = (*github.Client)(nil)
+68
View File
@@ -0,0 +1,68 @@
package github
import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"net/url"
"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) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
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)
Review

[NIT] User-controlled path is interpolated into error messages without sanitization. If callers log these errors verbatim, an attacker-controlled path containing newlines could enable log injection. Low practical risk given typical GitHub path constraints.

**[NIT]** User-controlled path is interpolated into error messages without sanitization. If callers log these errors verbatim, an attacker-controlled path containing newlines could enable log injection. Low practical risk given typical GitHub path constraints.
}
var resp struct {
Content string `json:"content"`
Encoding string `json:"encoding"`
}
if err := json.Unmarshal(body, &resp); err != nil {
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)
Review

[NIT] Same as above: error includes raw path value. Consider escaping or sanitizing control characters before embedding in error strings.

**[NIT]** Same as above: error includes raw path value. Consider escaping or sanitizing control characters before embedding in error strings.
}
decoded, err := decodeBase64Content(resp.Content)
if err != nil {
return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
Outdated
Review

[NIT] Same as above: error message includes raw path. Defense-in-depth to avoid potential log injection if higher layers log errors without escaping.

**[NIT]** Same as above: error message includes raw path. Defense-in-depth to avoid potential log injection if higher layers log errors without escaping.
}
Review

[MINOR] escapePath silently strips dot-segments ("." and ".."), causing paths like "foo/../bar" to resolve to "foo/bar" instead of canonical "bar". This can lead to surprising behavior or 404s; consider using path.Clean for canonicalization and rejecting traversal (e.g., resulting paths starting with "../"), or returning an error on dot-segments rather than altering the path.

**[MINOR]** escapePath silently strips dot-segments ("." and ".."), causing paths like "foo/../bar" to resolve to "foo/bar" instead of canonical "bar". This can lead to surprising behavior or 404s; consider using path.Clean for canonicalization and rejecting traversal (e.g., resulting paths starting with "../"), or returning an error on dot-segments rather than altering the path.
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
for _, part := range parts {
if part == "." || part == ".." || part == "" {
continue
}
clean = append(clean, url.PathEscape(part))
}
return strings.Join(clean, "/")
}
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
// GitHub returns base64 content with line breaks for formatting; we strip \r and \n before decoding.
Outdated
Review

[NIT] decodeBase64Content recreates a strings.Replacer on every call. For very hot paths, you could optimize by defining a package-level replacer or using a more lightweight approach; not critical here.

**[NIT]** decodeBase64Content recreates a strings.Replacer on every call. For very hot paths, you could optimize by defining a package-level replacer or using a more lightweight approach; not critical here.
func decodeBase64Content(encoded string) (string, error) {
cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
decoded, err := base64.StdEncoding.DecodeString(cleaned)
if err != nil {
return "", err
}
return string(decoded), nil
}