fix: address review feedback on PRReader implementation
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m55s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m55s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m55s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m55s
- Add maxFileContentSize (10 MB) limit to decodeBase64Content to prevent resource exhaustion from oversized file content (security MINOR) - Fix reversed NewClient arg order in TestGetFileContentAtRef_DotSegmentError (GPT MINOR + Sonnet NIT) - Remove 'waiting' from mapCheckRunStatus conclusion cases since it is a status value not a conclusion, update comment (GPT NIT) - Add TestDecodeBase64Content_SizeLimit test
This commit is contained in:
@@ -75,13 +75,26 @@ func escapePath(p string) (string, error) {
|
|||||||
return strings.Join(encoded, "/"), nil
|
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.
|
// 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.
|
// 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) {
|
func decodeBase64Content(encoded string) (string, error) {
|
||||||
cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
|
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)
|
decoded, err := base64.StdEncoding.DecodeString(cleaned)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "", err
|
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
|
return string(decoded), nil
|
||||||
}
|
}
|
||||||
|
|||||||
+18
-1
@@ -68,7 +68,7 @@ func TestGetFileContentAtRef_DotSegmentError(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient(srv.URL, "token")
|
c := NewClient("token", srv.URL)
|
||||||
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
|
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal("expected error for path with dot-segments")
|
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)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
|||||||
+2
-2
@@ -199,7 +199,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
|
|||||||
// - "success" → "success"
|
// - "success" → "success"
|
||||||
// - "failure", "action_required", "timed_out" → "failure"
|
// - "failure", "action_required", "timed_out" → "failure"
|
||||||
// - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics)
|
// - "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)
|
// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
|
||||||
func mapCheckRunStatus(conclusion *string) string {
|
func mapCheckRunStatus(conclusion *string) string {
|
||||||
if conclusion == nil {
|
if conclusion == nil {
|
||||||
@@ -213,7 +213,7 @@ func mapCheckRunStatus(conclusion *string) string {
|
|||||||
return "failure"
|
return "failure"
|
||||||
case "cancelled", "skipped", "neutral":
|
case "cancelled", "skipped", "neutral":
|
||||||
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
|
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
|
||||||
case "stale", "waiting":
|
case "stale":
|
||||||
return "pending"
|
return "pending"
|
||||||
default:
|
default:
|
||||||
return "pending"
|
return "pending"
|
||||||
|
|||||||
Reference in New Issue
Block a user