feat(github): implement PRReader interface #102
@@ -75,13 +75,26 @@ func escapePath(p string) (string, error) {
|
||||
return strings.Join(encoded, "/"), nil
|
||||
}
|
||||
|
||||
// maxFileContentSize is the maximum decoded file size (10 MB) to prevent
|
||||
// resource exhaustion when decoding base64 content from the API.
|
||||
|
|
||||
const maxFileContentSize = 10 * 1024 * 1024
|
||||
|
||||
// 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.
|
||||
// Returns an error if the decoded content exceeds maxFileContentSize.
|
||||
func decodeBase64Content(encoded string) (string, error) {
|
||||
cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
|
||||
// Check estimated decoded size before allocating.
|
||||
// Base64 encodes 3 bytes into 4 chars, so decoded ~ len*3/4.
|
||||
if len(cleaned)*3/4 > maxFileContentSize {
|
||||
return "", fmt.Errorf("file content too large: estimated %d bytes exceeds limit of %d", len(cleaned)*3/4, maxFileContentSize)
|
||||
}
|
||||
decoded, err := base64.StdEncoding.DecodeString(cleaned)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if len(decoded) > maxFileContentSize {
|
||||
return "", fmt.Errorf("file content too large: %d bytes exceeds limit of %d", len(decoded), maxFileContentSize)
|
||||
}
|
||||
return string(decoded), nil
|
||||
}
|
||||
|
||||
@@ -68,7 +68,7 @@ func TestGetFileContentAtRef_DotSegmentError(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `TestGetFileContentAtRef_DotSegmentError` does not call `AllowInsecureHTTP()` unlike all other tests that create an httptest server and use `NewClient`. This may still work if `NewClient` defaults to allowing HTTP for test servers, but it is inconsistent and may cause issues if the client enforces HTTPS by default for non-test usage. All tests that spin up an httptest server should use `AllowInsecureHTTP()` for consistency.
|
||||
c := NewClient(srv.URL, "token")
|
||||
c := NewClient("token", srv.URL)
|
||||
|
gpt-review-bot
commented
[MINOR] NewClient is called with arguments in an order inconsistent with other tests (srv.URL, "token"). While this test never performs a request, it’s confusing and could break if behavior changes. Use the same argument order as elsewhere (e.g., NewClient("token", srv.URL, ...)). **[MINOR]** NewClient is called with arguments in an order inconsistent with other tests (srv.URL, "token"). While this test never performs a request, it’s confusing and could break if behavior changes. Use the same argument order as elsewhere (e.g., NewClient("token", srv.URL, ...)).
|
||||
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
|
||||
|
sonnet-review-bot
commented
[NIT] TestGetFileContentAtRef_DotSegmentError constructs the client with **[NIT]** TestGetFileContentAtRef_DotSegmentError constructs the client with `NewClient(srv.URL, "token")` — argument order is reversed compared to all other tests in pr_test.go which use `NewClient("token", srv.URL, AllowInsecureHTTP())`. If the constructor signature is `NewClient(token, baseURL string, opts...)`, this test may pass the URL as the token and vice versa, which would be a latent correctness bug. The test happens to work only because it expects the server to never be called (the error is caught before the HTTP request), so the wrong baseURL is never used. Verify the argument order is intentional or align with the rest of the test suite.
|
||||
if err == nil {
|
||||
t.Fatal("expected error for path with dot-segments")
|
||||
@@ -77,3 +77,20 @@ func TestGetFileContentAtRef_DotSegmentError(t *testing.T) {
|
||||
t.Errorf("expected 'invalid file path' error, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDecodeBase64Content_SizeLimit(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Create base64 content that would decode to > maxFileContentSize.
|
||||
// maxFileContentSize is 10MB. Base64 of 11MB worth of zeros.
|
||||
// We just need something big enough to trigger the estimated size check.
|
||||
// 14MB of base64 chars (decodes to ~10.5MB).
|
||||
huge := strings.Repeat("A", 14*1024*1024)
|
||||
_, err := decodeBase64Content(huge)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for oversized content")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "too large") {
|
||||
t.Errorf("expected 'too large' error, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -199,7 +199,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
|
||||
// - "success" → "success"
|
||||
// - "failure", "action_required", "timed_out" → "failure"
|
||||
// - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics)
|
||||
// - "stale", "waiting" → "pending"
|
||||
// - "stale" → "pending" (check run became stale before completing)
|
||||
// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
|
||||
func mapCheckRunStatus(conclusion *string) string {
|
||||
if conclusion == nil {
|
||||
@@ -213,7 +213,7 @@ func mapCheckRunStatus(conclusion *string) string {
|
||||
return "failure"
|
||||
case "cancelled", "skipped", "neutral":
|
||||
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
|
||||
case "stale", "waiting":
|
||||
case "stale":
|
||||
return "pending"
|
||||
default:
|
||||
return "pending"
|
||||
|
||||
[MINOR] decodeBase64Content decodes the entire file content into memory without checking size limits. If an attacker can influence which file is fetched, this could be used for resource exhaustion (DoS). Consider enforcing a maximum allowed content size and ensure HTTP client timeouts and response body limits are in place.