From 8d3921fd3b996ef805529af9b4b09e5f60437432 Mon Sep 17 00:00:00 2001 From: Frederick Robinson Date: Mon, 20 Nov 2023 09:41:53 -0800 Subject: [PATCH] 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 | 105 +++++++++++++++++++++----------- internal/domain/filter_test.go | 57 +++++++++++++++++ internal/filter/service.go | 77 +++++++---------------- internal/filter/service_test.go | 41 ------------- 4 files changed, 149 insertions(+), 131 deletions(-) delete mode 100644 internal/filter/service_test.go diff --git a/internal/domain/filter.go b/internal/domain/filter.go index de7718c..e04009b 100644 --- a/internal/domain/filter.go +++ b/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 -// for indexers that doesn't announce size, like some gazelle based -// 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 +// checkSizeFilter compares the filter size limits to a release's size if it is +// known from the announce line. 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 - // handle both min and max - if minSize != "" { - // string to bytes - 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 - } - + sizeErr, err := f.CheckReleaseSize(r.Size) + if err != nil { + r.addRejectionF("size: error checking release size against filter: %+v", err) + return false } - - if maxSize != "" { - // string to bytes - 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 - } + if sizeErr != nil { + r.addRejectionF("%+v", sizeErr) + 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 +} diff --git a/internal/domain/filter_test.go b/internal/domain/filter_test.go index b534956..994e5d9 100644 --- a/internal/domain/filter_test.go +++ b/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) + } + }) + } +} diff --git a/internal/filter/service.go b/internal/filter/service.go index d8510a6..52470c4 100644 --- a/internal/filter/service.go +++ b/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 - err := s.repo.Store(ctx, filter) - if err != nil { + if err := s.repo.Store(ctx, filter); 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 filter.Name == "" { - return errors.New("validation: name can't be empty") + if err := filter.Validate(); err != nil { + s.log.Error().Err(err).Msgf("invalid filter: %v", filter) + 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 - // additional size check. Some indexers have api implemented to fetch this data and for the others - // it will download the torrent file to parse and make the size check. This is all to minimize the amount of downloads. - - // do additional size check against indexer api or download torrent for size check + // If size constraints are set in a filter and the indexer did not + // announce the size, we need to do an additional out of band 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 -// Some indexers do not announce the size and if size (min,max) is set in a filter then it will need -// additional size check. Some indexers have api implemented to fetch this data and for the others -// it will download the torrent file to parse and make the size check. This is all to minimize the amount of downloads. +// AdditionalSizeCheck performs additional out of band checks to determine the +// size of a torrent. Some indexers do not announce torrent size, so it is +// necessary to determine the size of the torrent in some other way. Some +// 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) } diff --git a/internal/filter/service_test.go b/internal/filter/service_test.go deleted file mode 100644 index c6a17d3..0000000 --- a/internal/filter/service_test.go +++ /dev/null @@ -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) - } - }) - } -}