feat(filters): validate filter size (#1263)

* update comments

* refactor, fail on malformed size constraints

* refactor validation, add failing test

* unify in ReleaseSizeOkay, refactor test

* validate filter limit parseability

* logging improvement

* refactor. more clear, explicit parsing step

* inline, add log

* comment tweak

* pass error with more info

* tweak parsedSizeLimits interface

internal/domain/filter.go

@@ -5,6 +5,8 @@ package domain
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"regexp"
 	"strconv"
 	"strings"
@@ -243,6 +245,18 @@ type FilterUpdate struct {
 	Indexers                         []Indexer               `json:"indexers,omitempty"`
 }
 
+func (f Filter) Validate() error {
+	if f.Name == "" {
+		return errors.New("validation: name can't be empty")
+	}
+
+	if _, _, err := f.parsedSizeLimits(); err != nil {
+		return fmt.Errorf("error validating filter size limits: %w", err)
+	}
+
+	return nil
+}
+
 func (f Filter) CheckFilter(r *Release) ([]string, bool) {
 	// reset rejections first to clean previous checks
 	r.resetRejections()
@@ -557,50 +571,24 @@ func (f Filter) isPerfectFLAC(r *Release) bool {
 	return true
 }
 
-// checkSizeFilter additional size check
+// checkSizeFilter compares the filter size limits to a release's size if it is
-// for indexers that doesn't announce size, like some gazelle based
+// known from the announce line.
-// set flag r.AdditionalSizeCheckRequired if there's a size in the filter, otherwise go a head
-// implement API for ptp,btn,ggn to check for size if needed
-// for others pull down torrent and do check
 func (f Filter) checkSizeFilter(r *Release, minSize string, maxSize string) bool {
-
 	if r.Size == 0 {
 		r.AdditionalSizeCheckRequired = true
-
 		return true
 	} else {
 		r.AdditionalSizeCheckRequired = false
 	}
 
-	// if r.Size parse filter to bytes and compare
+	sizeErr, err := f.CheckReleaseSize(r.Size)
-	// handle both min and max
+	if err != nil {
-	if minSize != "" {
+		r.addRejectionF("size: error checking release size against filter: %+v", err)
-		// string to bytes
+		return false
-		minSizeBytes, err := humanize.ParseBytes(minSize)
-		if err != nil {
-			r.addRejectionF("size: invalid minSize set: %s err: %q", minSize, err)
-			return false
-		}
-
-		if r.Size <= minSizeBytes {
-			r.addRejection("size: smaller than min size")
-			return false
-		}
-
 	}
-
+	if sizeErr != nil {
-	if maxSize != "" {
+		r.addRejectionF("%+v", sizeErr)
-		// string to bytes
+		return false
-		maxSizeBytes, err := humanize.ParseBytes(maxSize)
-		if err != nil {
-			r.addRejectionF("size: invalid maxSize set: %s err: %q", maxSize, err)
-			return false
-		}
-
-		if r.Size >= maxSizeBytes {
-			r.addRejection("size: larger than max size")
-			return false
-		}
 	}
 
 	return true
@@ -934,3 +922,50 @@ func matchHDR(releaseValues []string, filterValues []string) bool {
 
 	return false
 }
+
+func (f Filter) CheckReleaseSize(releaseSize uint64) (sizeErr, err error) {
+	min, max, err := f.parsedSizeLimits()
+	if err != nil {
+		return err, err
+	}
+
+	if min != nil && releaseSize <= *min {
+		return fmt.Errorf("release size %d bytes <= min size %d bytes", releaseSize, *min), nil
+	}
+
+	if max != nil && releaseSize >= *max {
+		return fmt.Errorf("release size %d bytes <= max size %d bytes", releaseSize, *max), nil
+	}
+
+	return nil, nil
+}
+
+// parsedSizeLimits parses filter bytes limits (expressed as a string) into a
+// uint64 number of bytes. The bounds are returned as *uint64 number of bytes,
+// with "nil" representing "no limit". We break out filter size limit parsing
+// into a discrete step so that we can more easily check parsability at filter
+// creation time.
+func (f Filter) parsedSizeLimits() (*uint64, *uint64, error) {
+	min, err := parseBytes(f.MinSize)
+	if err != nil {
+		return nil, nil, fmt.Errorf("trouble parsing min size: %w", err)
+	}
+
+	max, err := parseBytes(f.MaxSize)
+	if err != nil {
+		return nil, nil, fmt.Errorf("trouble parsing max size: %w", err)
+	}
+
+	return min, max, nil
+}
+
+// parseBytes parses a string representation of a file size into a number of
+// bytes. It returns a *uint64 where "nil" represents "none" (corresponding to
+// the empty string)
+func parseBytes(s string) (*uint64, error) {
+	if s == "" {
+		return nil, nil
+	}
+	b, err := humanize.ParseBytes(s)
+	return &b, err
+}

internal/domain/filter_test.go

@@ -2114,3 +2114,60 @@ func Test_matchRegex(t *testing.T) {
 		})
 	}
 }
+
+func Test_validation(t *testing.T) {
+	tests := []struct {
+		name   string
+		filter Filter
+		valid  bool
+	}{
+		{name: "empty name", filter: Filter{}, valid: false},
+		{name: "empty filter, with name", filter: Filter{Name: "test"}, valid: true},
+		{name: "valid size limit", filter: Filter{Name: "test", MaxSize: "12MB"}, valid: true},
+		{name: "gibberish max size limit", filter: Filter{Name: "test", MaxSize: "asdf"}, valid: false},
+		{name: "gibberish min size limit", filter: Filter{Name: "test", MinSize: "qwerty"}, valid: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.valid, tt.filter.Validate() == nil, "validation error \"%+v\" in test case %s", tt.filter.Validate(), tt.filter.Name)
+		})
+	}
+}
+
+func Test_checkSizeFilter(t *testing.T) {
+	type args struct {
+		minSize     string
+		maxSize     string
+		releaseSize uint64
+	}
+	tests := []struct {
+		name        string
+		filter      Filter
+		releaseSize uint64
+		want        bool
+		wantErr     bool
+	}{
+		{name: "test_1", filter: Filter{MinSize: "1GB", MaxSize: ""}, releaseSize: 100, want: false, wantErr: false},
+		{name: "test_2", filter: Filter{MinSize: "1GB", MaxSize: ""}, releaseSize: 2000000000, want: true, wantErr: false},
+		{name: "test_3", filter: Filter{MinSize: "1GB", MaxSize: "2.2GB"}, releaseSize: 2000000000, want: true, wantErr: false},
+		{name: "test_4", filter: Filter{MinSize: "1GB", MaxSize: "2GIB"}, releaseSize: 2000000000, want: true, wantErr: false},
+		{name: "test_5", filter: Filter{MinSize: "1GB", MaxSize: "2GB"}, releaseSize: 2000000010, want: false, wantErr: false},
+		{name: "test_6", filter: Filter{MinSize: "1GB", MaxSize: "2GB"}, releaseSize: 2000000000, want: false, wantErr: false},
+		{name: "test_7", filter: Filter{MaxSize: "2GB"}, releaseSize: 2500000000, want: false, wantErr: false},
+		{name: "test_8", filter: Filter{MaxSize: "20GB"}, releaseSize: 2500000000, want: true, wantErr: false},
+		{name: "test_9", filter: Filter{MinSize: "unparseable", MaxSize: "20GB"}, releaseSize: 2500000000, want: false, wantErr: true},
+	}
+	for _, tt := range tests {
+
+		t.Run(tt.name, func(t *testing.T) {
+			checkErr, err := tt.filter.CheckReleaseSize(tt.releaseSize)
+			if err != nil != tt.wantErr {
+				t.Errorf("checkSizeFilter() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if checkErr == nil != tt.want {
+				t.Errorf("checkSizeFilter() got = %v, want %v", checkErr, tt.want)
+			}
+		})
+	}
+}

internal/filter/service.go

@@ -24,7 +24,6 @@ import (
 	"github.com/autobrr/autobrr/pkg/errors"
 
 	"github.com/avast/retry-go/v4"
-	"github.com/dustin/go-humanize"
 	"github.com/mattn/go-shellwords"
 	"github.com/rs/zerolog"
 )
@@ -113,20 +112,18 @@ func (s *service) ListFilters(ctx context.Context) ([]domain.Filter, error) {
 }
 
 func (s *service) FindByID(ctx context.Context, filterID int) (*domain.Filter, error) {
-	// find filter
 	filter, err := s.repo.FindByID(ctx, filterID)
 	if err != nil {
+		s.log.Error().Err(err).Msgf("could not find filter for id: %v", filterID)
 		return nil, err
 	}
 
-	// find actions and attach
 	actions, err := s.actionRepo.FindByFilterID(ctx, filter.ID)
 	if err != nil {
-		s.log.Error().Msgf("could not find filter actions for filter id: %v", filter.ID)
+		s.log.Error().Err(err).Msgf("could not find filter actions for filter id: %v", filter.ID)
 	}
 	filter.Actions = actions
 
-	// find indexers and attach
 	indexers, err := s.indexerSvc.FindByFilterID(ctx, filter.ID)
 	if err != nil {
 		s.log.Error().Err(err).Msgf("could not find indexers for filter: %v", filter.Name)
@@ -149,11 +146,12 @@ func (s *service) GetDownloadsByFilterId(ctx context.Context, filterID int) (*do
 }
 
 func (s *service) Store(ctx context.Context, filter *domain.Filter) error {
-	// validate data
+	if err := filter.Validate(); err != nil {
+		s.log.Error().Err(err).Msgf("invalid filter: %v", filter)
+		return err
+	}
 
-	// store
+	if err := s.repo.Store(ctx, filter); err != nil {
-	err := s.repo.Store(ctx, filter)
-	if err != nil {
 		s.log.Error().Err(err).Msgf("could not store filter: %v", filter)
 		return err
 	}
@@ -162,9 +160,9 @@ func (s *service) Store(ctx context.Context, filter *domain.Filter) error {
 }
 
 func (s *service) Update(ctx context.Context, filter *domain.Filter) error {
-	// validate data
+	if err := filter.Validate(); err != nil {
-	if filter.Name == "" {
+		s.log.Error().Err(err).Msgf("invalid filter: %v", filter)
-		return errors.New("validation: name can't be empty")
+		return err
 	}
 
 	// replace newline with comma
@@ -375,11 +373,9 @@ func (s *service) CheckFilter(ctx context.Context, f domain.Filter, release *dom
 
 		s.log.Debug().Msgf("filter.Service.CheckFilter: found and matched filter: %s", f.Name)
 
-		// Some indexers do not announce the size and if size (min,max) is set in a filter then it will need
+		// If size constraints are set in a filter and the indexer did not
-		// additional size check. Some indexers have api implemented to fetch this data and for the others
+		// announce the size, we need to do an additional out of band size
-		// it will download the torrent file to parse and make the size check. This is all to minimize the amount of downloads.
+		// check.
-
-		// do additional size check against indexer api or download torrent for size check
 		if release.AdditionalSizeCheckRequired {
 			s.log.Debug().Msgf("filter.Service.CheckFilter: (%s) additional size check required", f.Name)
 
@@ -416,10 +412,13 @@ func (s *service) CheckFilter(ctx context.Context, f domain.Filter, release *dom
 	return false, nil
 }
 
-// AdditionalSizeCheck
+// AdditionalSizeCheck performs additional out of band checks to determine the
-// Some indexers do not announce the size and if size (min,max) is set in a filter then it will need
+// size of a torrent. Some indexers do not announce torrent size, so it is
-// additional size check. Some indexers have api implemented to fetch this data and for the others
+// necessary to determine the size of the torrent in some other way. Some
-// it will download the torrent file to parse and make the size check. This is all to minimize the amount of downloads.
+// indexers have an API implemented to fetch this data. For those which don't,
+// it is necessary to download the torrent file and parse it to make the size
+// check. We use the API where available to minimize the number of torrents we
+// need to download.
 func (s *service) AdditionalSizeCheck(ctx context.Context, f domain.Filter, release *domain.Release) (bool, error) {
 	var err error
 	defer func() {
@@ -457,13 +456,13 @@ func (s *service) AdditionalSizeCheck(ctx context.Context, f domain.Filter, rele
 	}
 
 	// compare size against filter
-	match, err := checkSizeFilter(f.MinSize, f.MaxSize, release.Size)
+	sizeErr, err := f.CheckReleaseSize(release.Size)
 	if err != nil {
 		s.log.Error().Stack().Err(err).Msgf("filter.Service.AdditionalSizeCheck: (%s) error checking extra size filter", f.Name)
 		return false, err
 	}
 	//no match, lets continue to next filter
-	if !match {
+	if sizeErr != nil {
 		s.log.Debug().Msgf("filter.Service.AdditionalSizeCheck: (%s) filter did not match after additional size check, trying next", f.Name)
 		return false, nil
 	}
@@ -471,38 +470,6 @@ func (s *service) AdditionalSizeCheck(ctx context.Context, f domain.Filter, rele
 	return true, nil
 }
 
-func checkSizeFilter(minSize string, maxSize string, releaseSize uint64) (bool, error) {
-	// handle both min and max
-	if minSize != "" {
-		// string to bytes
-		minSizeBytes, err := humanize.ParseBytes(minSize)
-		if err != nil {
-			// log could not parse into bytes
-		}
-
-		if releaseSize <= minSizeBytes {
-			//r.addRejection("size: smaller than min size")
-			return false, nil
-		}
-
-	}
-
-	if maxSize != "" {
-		// string to bytes
-		maxSizeBytes, err := humanize.ParseBytes(maxSize)
-		if err != nil {
-			// log could not parse into bytes
-		}
-
-		if releaseSize >= maxSizeBytes {
-			//r.addRejection("size: larger than max size")
-			return false, nil
-		}
-	}
-
-	return true, nil
-}
-
 func (s *service) CanDownloadShow(ctx context.Context, release *domain.Release) (bool, error) {
 	return s.releaseRepo.CanDownloadShow(ctx, release.Title, release.Season, release.Episode)
 }

internal/filter/service_test.go

@@ -1,41 +0,0 @@
-// Copyright (c) 2021 - 2023, Ludvig Lundgren and the autobrr contributors.
-// SPDX-License-Identifier: GPL-2.0-or-later
-
-package filter
-
-import "testing"
-
-func Test_checkSizeFilter(t *testing.T) {
-	type args struct {
-		minSize     string
-		maxSize     string
-		releaseSize uint64
-	}
-	tests := []struct {
-		name    string
-		args    args
-		want    bool
-		wantErr bool
-	}{
-		{name: "test_1", args: args{minSize: "1GB", maxSize: "", releaseSize: 100}, want: false, wantErr: false},
-		{name: "test_2", args: args{minSize: "1GB", maxSize: "", releaseSize: 2000000000}, want: true, wantErr: false},
-		{name: "test_3", args: args{minSize: "1GB", maxSize: "2.2GB", releaseSize: 2000000000}, want: true, wantErr: false},
-		{name: "test_4", args: args{minSize: "1GB", maxSize: "2GIB", releaseSize: 2000000000}, want: true, wantErr: false},
-		{name: "test_5", args: args{minSize: "1GB", maxSize: "2GB", releaseSize: 2000000010}, want: false, wantErr: false},
-		{name: "test_6", args: args{minSize: "1GB", maxSize: "2GB", releaseSize: 2000000000}, want: false, wantErr: false},
-		{name: "test_7", args: args{minSize: "", maxSize: "2GB", releaseSize: 2500000000}, want: false, wantErr: false},
-		{name: "test_8", args: args{minSize: "", maxSize: "20GB", releaseSize: 2500000000}, want: true, wantErr: false},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := checkSizeFilter(tt.args.minSize, tt.args.maxSize, tt.args.releaseSize)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("checkSizeFilter() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.want {
-				t.Errorf("checkSizeFilter() got = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}