diff --git a/CHANGELOG.md b/CHANGELOG.md index 941c3ed6..305fc5e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,11 +14,11 @@ and this project adheres to @@ -84,7 +84,8 @@ In this release, the schema version has changed from 20 to 21. - Queries with the question-section target `.`, for example `NS .`, are now counted in the statistics and correctly shown in the query log ([#5910]). -- Safe Search not working with `AAAA` queries for Yandex domains ([#5913]). +- Safe Search not working with `AAAA` queries for domains that don't have `AAAA` + records ([#5913]). [#951]: https://github.com/AdguardTeam/AdGuardHome/issues/951 [#1577]: https://github.com/AdguardTeam/AdGuardHome/issues/1577 diff --git a/internal/filtering/safesearch/safesearch.go b/internal/filtering/safesearch/safesearch.go index a1ac6404..9d5b5121 100644 --- a/internal/filtering/safesearch/safesearch.go +++ b/internal/filtering/safesearch/safesearch.go @@ -161,12 +161,8 @@ func (ss *Default) resetEngine( // type check var _ filtering.SafeSearch = (*Default)(nil) -// CheckHost implements the [filtering.SafeSearch] interface for -// *DefaultSafeSearch. -func (ss *Default) CheckHost( - host string, - qtype rules.RRType, -) (res filtering.Result, err error) { +// CheckHost implements the [filtering.SafeSearch] interface for *Default. +func (ss *Default) CheckHost(host string, qtype rules.RRType) (res filtering.Result, err error) { start := time.Now() defer func() { ss.log(log.DEBUG, "lookup for %q finished in %s", host, time.Since(start)) @@ -196,14 +192,10 @@ func (ss *Default) CheckHost( return filtering.Result{}, err } - if fltRes != nil { - res = *fltRes - ss.setCacheResult(host, qtype, res) + res = *fltRes + ss.setCacheResult(host, qtype, res) - return res, nil - } - - return filtering.Result{}, fmt.Errorf("no ip addresses for %q", host) + return res, nil } // searchHost looks up DNS rewrites in the internal DNS filtering engine. @@ -229,7 +221,11 @@ func (ss *Default) searchHost(host string, qtype rules.RRType) (res *rules.DNSRe } // newResult creates Result object from rewrite rule. qtype must be either -// [dns.TypeA] or [dns.TypeAAAA]. +// [dns.TypeA] or [dns.TypeAAAA]. If err is nil, res is never nil, so that the +// empty result is converted into a NODATA response. +// +// TODO(a.garipov): Use the main rewrite result mechanism used in +// [dnsforward.Server.filterDNSRequest]. func (ss *Default) newResult( rewrite *rules.DNSRewrite, qtype rules.RRType, @@ -243,9 +239,10 @@ func (ss *Default) newResult( } if rewrite.RRType == qtype { - ip, ok := rewrite.Value.(net.IP) + v := rewrite.Value + ip, ok := v.(net.IP) if !ok || ip == nil { - return nil, nil + return nil, fmt.Errorf("expected ip rewrite value, got %T(%[1]v)", v) } res.Rules[0].IP = ip @@ -255,13 +252,6 @@ func (ss *Default) newResult( host := rewrite.NewCNAME if host == "" { - // If there is a rewrite, but it's neither a CNAME one nor one matching - // the IP version, then it's a service that only has one type of IP - // record but not the other. Return the empty result to be converted - // into a NODATA response. - // - // TODO(a.garipov): Use the main rewrite result mechanism used in - // [dnsforward.Server.filterDNSRequest]. return res, nil } @@ -269,7 +259,7 @@ func (ss *Default) newResult( ips, err := ss.resolver.LookupIP(context.Background(), qtypeToProto(qtype), host) if err != nil { - return nil, err + return nil, fmt.Errorf("resolving cname: %w", err) } ss.log(log.DEBUG, "resolved %s", ips) @@ -283,11 +273,9 @@ func (ss *Default) newResult( } res.Rules[0].IP = ip - - return res, nil } - return nil, nil + return res, nil } // qtypeToProto returns "ip4" for [dns.TypeA] and "ip6" for [dns.TypeAAAA]. diff --git a/internal/filtering/safesearch/safesearch_test.go b/internal/filtering/safesearch/safesearch_test.go index b0775c60..12860c5d 100644 --- a/internal/filtering/safesearch/safesearch_test.go +++ b/internal/filtering/safesearch/safesearch_test.go @@ -1,6 +1,7 @@ package safesearch_test import ( + "context" "net" "testing" "time" @@ -80,6 +81,14 @@ func TestDefault_CheckHost_yandexAAAA(t *testing.T) { require.NoError(t, err) assert.True(t, res.IsFiltered) + + // TODO(a.garipov): Currently, the safe-search filter returns a single rule + // with a nil IP address. This isn't really necessary and should be changed + // once the TODO in [safesearch.Default.newResult] is resolved. + require.Len(t, res.Rules, 1) + + assert.Nil(t, res.Rules[0].IP) + assert.EqualValues(t, filtering.SafeSearchListID, res.Rules[0].FilterListID) } func TestDefault_CheckHost_google(t *testing.T) { @@ -116,6 +125,56 @@ func TestDefault_CheckHost_google(t *testing.T) { } } +// testResolver is a [filtering.Resolver] for tests. +// +// TODO(a.garipov): Move to aghtest and use everywhere. +type testResolver struct { + OnLookupIP func(ctx context.Context, network, host string) (ips []net.IP, err error) +} + +// type check +var _ filtering.Resolver = (*testResolver)(nil) + +// LookupIP implements the [filtering.Resolver] interface for *testResolver. +func (r *testResolver) LookupIP( + ctx context.Context, + network string, + host string, +) (ips []net.IP, err error) { + return r.OnLookupIP(ctx, network, host) +} + +func TestDefault_CheckHost_duckduckgoAAAA(t *testing.T) { + conf := testConf + conf.CustomResolver = &testResolver{ + OnLookupIP: func(_ context.Context, network, host string) (ips []net.IP, err error) { + assert.Equal(t, "ip6", network) + assert.Equal(t, "safe.duckduckgo.com", host) + + return nil, nil + }, + } + + ss, err := safesearch.NewDefault(conf, "", testCacheSize, testCacheTTL) + require.NoError(t, err) + + // The DuckDuckGo safe-search addresses are resolved through CNAMEs, but + // DuckDuckGo doesn't have a safe-search IPv6 address. The result should be + // the same as the one for Yandex IPv6. That is, a NODATA response. + res, err := ss.CheckHost("www.duckduckgo.com", dns.TypeAAAA) + require.NoError(t, err) + + assert.True(t, res.IsFiltered) + + // TODO(a.garipov): Currently, the safe-search filter returns a single rule + // with a nil IP address. This isn't really necessary and should be changed + // once the TODO in [safesearch.Default.newResult] is resolved. + require.Len(t, res.Rules, 1) + + assert.Nil(t, res.Rules[0].IP) + assert.EqualValues(t, filtering.SafeSearchListID, res.Rules[0].FilterListID) +} + func TestDefault_Update(t *testing.T) { conf := testConf ss, err := safesearch.NewDefault(conf, "", testCacheSize, testCacheTTL) diff --git a/internal/next/changelog.md b/internal/next/changelog.md index 39e1aff0..ac304796 100644 --- a/internal/next/changelog.md +++ b/internal/next/changelog.md @@ -24,7 +24,7 @@ enough. ### Fixed -- Inconsistent application of `--work-dir/-w` ([#2902]). +- Inconsistent application of `--work-dir/-w` ([#2598], [#2902]). - The order of `-v/--verbose` and `--version` being significant ([#2893]). ### Removed @@ -33,6 +33,7 @@ enough. - `--host` and `-p/--port` flags. Use `--web-addr=host:port` to set an address on which to serve the Web UI. `-h` is now an alias for `--help`, see above. +[#2598]: https://github.com/AdguardTeam/AdGuardHome/issues/2598 [#2893]: https://github.com/AdguardTeam/AdGuardHome/issues/2893 [#2902]: https://github.com/AdguardTeam/AdGuardHome/issues/2902 [#5676]: https://github.com/AdguardTeam/AdGuardHome/issues/5676