diff --git a/operator/pkg/s3.go b/operator/pkg/s3.go index 317146008..300670be6 100644 --- a/operator/pkg/s3.go +++ b/operator/pkg/s3.go @@ -18,7 +18,7 @@ const ( // getBatchFromDataServiceWithMultipleURLs tries multiple comma-separated URLs until first successful response func (o *Operator) getBatchFromDataServiceWithMultipleURLs(ctx context.Context, batchURLs string, expectedMerkleRoot [32]byte, maxRetries int, retryDelay time.Duration) ([]VerificationData, error) { - // Parse comma-separated URLs and limit to max 5 + // Parse comma-separated URLs and limit them to MaxBatchURLs urls := parseBatchURLs(batchURLs) o.Logger.Infof("Getting batch from data service with %d URLs: %v", len(urls), urls) @@ -42,16 +42,16 @@ func (o *Operator) getBatchFromDataServiceWithMultipleURLs(ctx context.Context, return nil, fmt.Errorf("failed to get batch from all URLs, errors: %s", strings.Join(errors, "; ")) } -// parseBatchURLs parses comma-separated URLs and limits to max 5 +// parseBatchURLs parses comma-separated URLs and limits them to MaxBatchURLs func parseBatchURLs(batchURLs string) []string { urls := make([]string, 0) for _, url := range strings.Split(batchURLs, ",") { + if len(urls) >= MaxBatchURLs { + break + } trimmedURL := strings.TrimSpace(url) if trimmedURL != "" { urls = append(urls, trimmedURL) - if len(urls) > MaxBatchURLs { - break - } } } @@ -72,6 +72,12 @@ func (o *Operator) getBatchFromDataService(ctx context.Context, batchURL string, Transport: transport, } + // A non-positive retry count would skip the loop entirely and leave resp nil, + // which would then be dereferenced below. Always make at least one attempt. + if maxRetries < 1 { + maxRetries = 1 + } + for attempt := 0; attempt < maxRetries; attempt++ { if attempt > 0 { o.Logger.Infof("Waiting for %s before retrying data fetch (attempt %d of %d)", retryDelay, attempt+1, maxRetries) @@ -122,16 +128,26 @@ func (o *Operator) getBatchFromDataService(ctx context.Context, batchURL string, return nil, fmt.Errorf("error getting batch from data service: %s", resp.Status) } + maxBatchSize := o.Config.Operator.MaxBatchSize + contentLength := resp.ContentLength - if contentLength > o.Config.Operator.MaxBatchSize { + if contentLength > maxBatchSize { return nil, fmt.Errorf("proof size %d exceeds max batch size %d", - contentLength, o.Config.Operator.MaxBatchSize) + contentLength, maxBatchSize) + } + + // resp.ContentLength is -1 when the server does not announce a length, which is + // the case for chunked transfer encoding. Fall back to the configured maximum so + // those responses are still readable while remaining bounded. + readLimit := maxBatchSize + if contentLength >= 0 { + readLimit = contentLength } // Use io.LimitReader to limit the size of the response body // This is to prevent the operator from downloading a larger than expected file - // + 1 is added to the contentLength to check if the response body is larger than expected - reader := io.LimitedReader{R: resp.Body, N: contentLength + 1} + // + 1 is added to the limit to check if the response body is larger than expected + reader := io.LimitedReader{R: resp.Body, N: readLimit + 1} batchBytes, err := io.ReadAll(&reader) if err != nil { return nil, err @@ -139,7 +155,7 @@ func (o *Operator) getBatchFromDataService(ctx context.Context, batchURL string, // Check if the response body is larger than expected if reader.N <= 0 { - return nil, fmt.Errorf("batch size exceeds max batch size %d", o.Config.Operator.MaxBatchSize) + return nil, fmt.Errorf("batch size exceeds max batch size %d", maxBatchSize) } // Checks if downloaded merkle root is the same as the expected one diff --git a/operator/pkg/s3_test.go b/operator/pkg/s3_test.go new file mode 100644 index 000000000..723cd8fbb --- /dev/null +++ b/operator/pkg/s3_test.go @@ -0,0 +1,54 @@ +package operator + +import "testing" + +func TestParseBatchURLs(t *testing.T) { + tests := []struct { + name string + input string + want []string + }{ + { + name: "empty string yields no URLs", + input: "", + want: []string{}, + }, + { + name: "single URL", + input: "https://a.example/batch", + want: []string{"https://a.example/batch"}, + }, + { + name: "surrounding whitespace is trimmed and blanks skipped", + input: " https://a.example/batch ,, https://b.example/batch ", + want: []string{"https://a.example/batch", "https://b.example/batch"}, + }, + { + name: "exactly MaxBatchURLs are kept", + input: "u1,u2,u3,u4,u5", + want: []string{"u1", "u2", "u3", "u4", "u5"}, + }, + { + name: "more than MaxBatchURLs are truncated to the limit", + input: "u1,u2,u3,u4,u5,u6,u7", + want: []string{"u1", "u2", "u3", "u4", "u5"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseBatchURLs(tt.input) + if len(got) > MaxBatchURLs { + t.Fatalf("parseBatchURLs returned %d URLs, which exceeds MaxBatchURLs (%d)", len(got), MaxBatchURLs) + } + if len(got) != len(tt.want) { + t.Fatalf("parseBatchURLs(%q) = %v, want %v", tt.input, got, tt.want) + } + for i := range tt.want { + if got[i] != tt.want[i] { + t.Errorf("parseBatchURLs(%q)[%d] = %q, want %q", tt.input, i, got[i], tt.want[i]) + } + } + }) + } +}