diff --git a/internal/filtering/filter.go b/internal/filtering/filter.go index 240b623d..ac5e21f1 100644 --- a/internal/filtering/filter.go +++ b/internal/filtering/filter.go @@ -144,21 +144,22 @@ func (d *DNSFilter) filterSetProperties( shouldRestart = true } - if flt.Enabled { - if shouldRestart { - // Download the filter contents. - shouldRestart, err = d.update(flt) - } - } else { + if !flt.Enabled { // TODO(e.burkov): The validation of the contents of the new URL is // currently skipped if the rule list is disabled. This makes it // possible to set a bad rules source, but the validation should still // kick in when the filter is enabled. Consider changing this behavior // to be stricter. flt.unload() + + return shouldRestart, err } - return shouldRestart, err + if !shouldRestart { + return false, nil + } + + return d.update(flt) } // filterExists returns true if a filter with the same url exists in d. It's @@ -315,19 +316,7 @@ func (d *DNSFilter) refreshFiltersArray( return 0, nil, nil, false } - failNum := 0 - for i := range updateFilters { - uf := &updateFilters[i] - updated, err := d.update(uf) - updateFlags = append(updateFlags, updated) - if err != nil { - failNum++ - d.logger.ErrorContext(ctx, "updating filter", "url", uf.URL, slogutil.KeyError, err) - - continue - } - } - + failNum, updateFlags := d.updateFilterList(ctx, updateFilters) if failNum == len(updateFilters) { return 0, nil, nil, true } @@ -335,6 +324,40 @@ func (d *DNSFilter) refreshFiltersArray( d.conf.filtersMu.Lock() defer d.conf.filtersMu.Unlock() + updateCount = d.syncUpdatedFilters(ctx, filters, updateFilters, updateFlags) + + return updateCount, updateFilters, updateFlags, false +} + +// updateFilterList updates each filter in updateFilters and returns the number +// of failures and the updateFlags slice aligned with updateFilters indicating +// whether each filter's data changed. +func (d *DNSFilter) updateFilterList( + ctx context.Context, + updateFilters []FilterYAML, +) (failNum int, updateFlags []bool) { + for i := range updateFilters { + uf := &updateFilters[i] + updated, err := d.update(uf) + updateFlags = append(updateFlags, updated) + if err != nil { + failNum++ + d.logger.ErrorContext(ctx, "updating filter", "url", uf.URL, slogutil.KeyError, err) + } + } + + return failNum, updateFlags +} + +// syncUpdatedFilters syncs updated filters back to the original filters slice +// and returns the updateCount. filters must not be nil. updateFlags must +// align with updateFilters. d.conf.filtersMu must be locked. +func (d *DNSFilter) syncUpdatedFilters( + ctx context.Context, + filters *[]FilterYAML, + updateFilters []FilterYAML, + updateFlags []bool, +) (updateCount int) { for i := range updateFilters { uf := &updateFilters[i] updated := updateFlags[i] @@ -365,7 +388,7 @@ func (d *DNSFilter) refreshFiltersArray( } } - return updateCount, updateFilters, updateFlags, false + return updateCount } // refreshFiltersIntl checks filters and updates them if necessary. If force is @@ -418,21 +441,23 @@ func (d *DNSFilter) refreshFiltersIntl(block, allow, force bool) (int, bool) { return 0, true } - if updNum != 0 { - d.EnableFilters(false) + if updNum == 0 { + return 0, false + } - for i := range lists { - uf := &lists[i] - updated := toUpd[i] - if !updated { - continue - } + d.EnableFilters(false) - p := uf.Path(d.conf.DataDir) - err := os.Remove(p + ".old") - if err != nil { - d.logger.ErrorContext(ctx, "removing old filter", "path", p, slogutil.KeyError, err) - } + for i := range lists { + uf := &lists[i] + updated := toUpd[i] + if !updated { + continue + } + + p := uf.Path(d.conf.DataDir) + err := os.Remove(p + ".old") + if err != nil { + d.logger.ErrorContext(ctx, "removing old filter", "path", p, slogutil.KeyError, err) } } diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go index 41d437c3..874283ec 100644 --- a/internal/filtering/filtering.go +++ b/internal/filtering/filtering.go @@ -766,42 +766,19 @@ func (d *DNSFilter) matchBlockedServicesRules( func newRuleStorage(filters []Filter) (rs *filterlist.RuleStorage, err error) { lists := make([]filterlist.RuleList, 0, len(filters)) for _, f := range filters { - switch id := int(f.ID); { - case len(f.Data) != 0: - lists = append(lists, &filterlist.StringRuleList{ - ID: id, - RulesText: string(f.Data), - IgnoreCosmetic: true, - }) - case f.FilePath == "": + var rl filterlist.RuleList + var skip bool + rl, skip, err = ruleListFromFilter(f) + if skip { continue - case runtime.GOOS == "windows": - // On Windows we don't pass a file to urlfilter because it's - // difficult to update this file while it's being used. - var data []byte - data, err = os.ReadFile(f.FilePath) - if errors.Is(err, fs.ErrNotExist) { - continue - } else if err != nil { - return nil, fmt.Errorf("reading filter content: %w", err) - } - - lists = append(lists, &filterlist.StringRuleList{ - ID: id, - RulesText: string(data), - IgnoreCosmetic: true, - }) - default: - var list *filterlist.FileRuleList - list, err = filterlist.NewFileRuleList(id, f.FilePath, true) - if errors.Is(err, fs.ErrNotExist) { - continue - } else if err != nil { - return nil, fmt.Errorf("creating file rule list with %q: %w", f.FilePath, err) - } - - lists = append(lists, list) } + + if err != nil { + // Don't wrap the error, because it's informative enough as is. + return nil, err + } + + lists = append(lists, rl) } rs, err = filterlist.NewRuleStorage(lists) @@ -812,6 +789,51 @@ func newRuleStorage(filters []Filter) (rs *filterlist.RuleStorage, err error) { return rs, nil } +// ruleListFromFilter returns a rule list from a Filter. +func ruleListFromFilter(f Filter) (rl filterlist.RuleList, skip bool, err error) { + id := int(f.ID) + + if len(f.Data) != 0 { + return &filterlist.StringRuleList{ + ID: id, + RulesText: string(f.Data), + IgnoreCosmetic: true, + }, false, nil + } + + if f.FilePath == "" { + return nil, true, nil + } + + if runtime.GOOS == "windows" { + // On Windows we don't pass a file to urlfilter because it's + // difficult to update this file while it's being used. + var data []byte + data, err = os.ReadFile(f.FilePath) + if errors.Is(err, fs.ErrNotExist) { + return nil, true, nil + } else if err != nil { + return nil, false, fmt.Errorf("reading filter content: %w", err) + } + + return &filterlist.StringRuleList{ + ID: id, + RulesText: string(data), + IgnoreCosmetic: true, + }, false, nil + } + + var list *filterlist.FileRuleList + list, err = filterlist.NewFileRuleList(id, f.FilePath, true) + if errors.Is(err, fs.ErrNotExist) { + return nil, true, nil + } else if err != nil { + return nil, false, fmt.Errorf("creating file rule list with %q: %w", f.FilePath, err) + } + + return list, false, nil +} + // Initialize urlfilter objects. func (d *DNSFilter) initFiltering(ctx context.Context, allowFilters, blockFilters []Filter) (err error) { rulesStorage, err := newRuleStorage(blockFilters) @@ -904,32 +926,37 @@ func (d *DNSFilter) matchHostProcessDNSResult( return makeResult([]rules.Rule{dnsres.NetworkRule}, reason) } - switch qtype { - case dns.TypeA: - if dnsres.HostRulesV4 != nil { - res = makeResult(hostRulesToRules(dnsres.HostRulesV4), FilteredBlockList) - for i, hr := range dnsres.HostRulesV4 { - res.Rules[i].IP = hr.IP - } - - return res - } - case dns.TypeAAAA: - if dnsres.HostRulesV6 != nil { - res = makeResult(hostRulesToRules(dnsres.HostRulesV6), FilteredBlockList) - for i, hr := range dnsres.HostRulesV6 { - res.Rules[i].IP = hr.IP - } - - return res - } - default: - // Go on. + if result, ok := resultFromHostRules(qtype, dnsres); ok { + return result } return hostResultForOtherQType(dnsres) } +// resultFromHostRules handles the HostRulesV4/HostRulesV6 case for +// [matchHostProcessDNSResult]. dnsres must not be nil. +func resultFromHostRules(qtype uint16, dnsres *urlfilter.DNSResult) (res Result, ok bool) { + if qtype == dns.TypeA && dnsres.HostRulesV4 != nil { + res = makeResult(hostRulesToRules(dnsres.HostRulesV4), FilteredBlockList) + for i, hr := range dnsres.HostRulesV4 { + res.Rules[i].IP = hr.IP + } + + return res, true + } + + if qtype == dns.TypeAAAA && dnsres.HostRulesV6 != nil { + res = makeResult(hostRulesToRules(dnsres.HostRulesV6), FilteredBlockList) + for i, hr := range dnsres.HostRulesV6 { + res.Rules[i].IP = hr.IP + } + + return res, true + } + + return Result{}, false +} + // hostResultForOtherQType returns a result based on the host rules in dnsres, // if any. dnsres.HostRulesV4 take precedence over dnsres.HostRulesV6. func hostResultForOtherQType(dnsres *urlfilter.DNSResult) (res Result) { @@ -1051,14 +1078,10 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { confMu: &sync.RWMutex{}, } - for i, p := range c.SafeFSPatterns { - // Use Match to validate the patterns here. - _, err = filepath.Match(p, "test") - if err != nil { - return nil, fmt.Errorf("safe_fs_patterns: at index %d: %w", i, err) - } - - d.safeFSPatterns = append(d.safeFSPatterns, p) + err = d.validateSafeFSPatterns(c.SafeFSPatterns) + if err != nil { + // Don't wrap the error, because it's informative enough as is. + return nil, err } d.hostCheckers = []hostChecker{{ @@ -1126,6 +1149,22 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { return d, nil } +// validateSafeFSPatterns validates and stores patterns for local filtering‑rule +// files. +func (d *DNSFilter) validateSafeFSPatterns(patterns []string) (err error) { + for i, p := range patterns { + // Use Match to validate the patterns here. + _, err = filepath.Match(p, "test") + if err != nil { + return fmt.Errorf("safe_fs_patterns: at index %d: %w", i, err) + } + + d.safeFSPatterns = append(d.safeFSPatterns, p) + } + + return nil +} + // Start registers web handlers and starts filters updates loop. func (d *DNSFilter) Start() { d.filtersInitializerChan = make(chan filtersInitializerParams, 1) diff --git a/internal/filtering/rewrite/storage.go b/internal/filtering/rewrite/storage.go index bab84089..9fea5eeb 100644 --- a/internal/filtering/rewrite/storage.go +++ b/internal/filtering/rewrite/storage.go @@ -97,16 +97,38 @@ func (s *DefaultStorage) MatchRequest(dReq *urlfilter.DNSRequest) (rws []*rules. ctx := context.TODO() - rrules := s.rewriteRulesForReq(dReq) - if len(rrules) == 0 { + rewriteRules := s.rewriteRulesForReq(dReq) + if len(rewriteRules) == 0 { return nil } + resolvedRules, wildcardRewrite := s.resolveCNAMEChain(ctx, dReq, rewriteRules) + if wildcardRewrite != nil { + return []*rules.DNSRewrite{wildcardRewrite} + } + + if resolvedRules == nil { + return nil + } + + return s.collectDNSRewrites(resolvedRules, dReq.DNSType) +} + +// resolveCNAMEChain follows the CNAME chain for a DNS request, handling loops +// and special cases. dReq must not be nil, and rewriteRules must not contain +// nil elements. +func (s *DefaultStorage) resolveCNAMEChain( + ctx context.Context, + dReq *urlfilter.DNSRequest, + rewriteRules []*rules.NetworkRule, +) (resolvedRules []*rules.NetworkRule, wildcardRewrite *rules.DNSRewrite) { // TODO(a.garipov): Check cnames for cycles on initialization. cnames := container.NewMapSet[string]() host := dReq.Hostname - for len(rrules) > 0 && rrules[0].DNSRewrite != nil && rrules[0].DNSRewrite.NewCNAME != "" { - rule := rrules[0] + for len(rewriteRules) > 0 && + rewriteRules[0].DNSRewrite != nil && + rewriteRules[0].DNSRewrite.NewCNAME != "" { + rule := rewriteRules[0] rwAns := rule.DNSRewrite.NewCNAME s.logger.DebugContext(ctx, "cname found", "host", host, "cname", rwAns) @@ -115,37 +137,43 @@ func (s *DefaultStorage) MatchRequest(dReq *urlfilter.DNSRequest) (rws []*rules. // A request for the hostname itself is an exception rule. // TODO(d.kolyshev): Check rewrite of a pattern onto itself. - return nil + return nil, nil } - if host == rwAns && isWildcard(rule.RuleText) { - // An "*.example.com → sub.example.com" rewrite matching in a loop. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/4016. - - return []*rules.DNSRewrite{rule.DNSRewrite} + if isSelfMatchingWildcard(host, rwAns, rule.RuleText) { + return nil, rule.DNSRewrite } if cnames.Has(rwAns) { - s.logger.InfoContext(ctx, "rewrite cname loop", "host", dReq.Hostname, "rewrite", rwAns) + s.logger.WarnContext(ctx, "rewrite cname loop", "host", dReq.Hostname, "rewrite", rwAns) - return nil + return nil, nil } cnames.Add(rwAns) - drules := s.rewriteRulesForReq(&urlfilter.DNSRequest{ + rewriteRulesForReq := s.rewriteRulesForReq(&urlfilter.DNSRequest{ Hostname: rwAns, DNSType: dReq.DNSType, }) - if drules != nil { - rrules = drules + if rewriteRulesForReq != nil { + rewriteRules = rewriteRulesForReq } host = rwAns } - return s.collectDNSRewrites(rrules, dReq.DNSType) + return rewriteRules, nil +} + +// isSelfMatchingWildcard returns true when a wildcard rewrite matches its own +// result. +// +// For example, an "*.example.com → sub.example.com" rewrite matching in a loop. +// +// See https://github.com/AdguardTeam/AdGuardHome/issues/4016. +func isSelfMatchingWildcard(host, rwAns, ruleText string) (ok bool) { + return host == rwAns && isWildcard(ruleText) } // collectDNSRewrites filters DNSRewrite by question type. diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index 62bda9a9..12e19412 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -169,10 +169,6 @@ run_linter gocognit --over='19' \ ./internal/home/ \ ; -run_linter gocognit --over='15' \ - ./internal/filtering/ \ - ; - run_linter gocognit --over='14' \ ./internal/dhcpd \ ; @@ -181,10 +177,6 @@ run_linter gocognit --over='13' \ ./internal/aghnet/ \ ; -run_linter gocognit --over='12' \ - ./internal/filtering/rewrite/ \ - ; - run_linter gocognit --over='10' \ ./internal/aghalg/ \ ./internal/aghhttp/ \ @@ -198,9 +190,7 @@ run_linter gocognit --over='10' \ ./internal/configmigrate/ \ ./internal/dhcpsvc \ ./internal/dnsforward/ \ - ./internal/filtering/hashprefix/ \ - ./internal/filtering/rulelist/ \ - ./internal/filtering/safesearch/ \ + ./internal/filtering/ \ ./internal/ipset \ ./internal/next/ \ ./internal/rdns/ \