Pull request 1881: 5913-fix-safesearch-ipv6-better

Updates #5913.

Squashed commit of the following:

commit 6bff5ee
Merge: 0a6f490 2902f03
Author: Ainar Garipov <[email protected]>
Date:   Tue Jun 20 14:14:47 2023 +0300

    Merge branch 'master' into 5913-fix-safesearch-ipv6-better

commit 0a6f490
Author: Ainar Garipov <[email protected]>
Date:   Tue Jun 20 14:11:18 2023 +0300

    safesearch: imp tests

commit 3f9056d
Author: Ainar Garipov <[email protected]>
Date:   Mon Jun 19 23:01:03 2023 +0300

    safesearch: fix ipv6 more

CHANGELOG.md

@@ -14,11 +14,11 @@ and this project adheres to
 <!--
 ## [v0.108.0] - TBA
 
-## [v0.107.32] - 2023-06-28 (APPROX.)
+## [v0.107.33] - 2023-06-28 (APPROX.)
 
-See also the [v0.107.32 GitHub milestone][ms-v0.107.32].
+See also the [v0.107.33 GitHub milestone][ms-v0.107.33].
 
-[ms-v0.107.32]: https://github.com/AdguardTeam/AdGuardHome/milestone/68?closed=1
+[ms-v0.107.33]: https://github.com/AdguardTeam/AdGuardHome/milestone/68?closed=1
 
 NOTE: Add new changes BELOW THIS COMMENT.
 -->
@@ -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

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)
-
-		return res, nil
-	}
+	res = *fltRes
+	ss.setCacheResult(host, qtype, res)
 
-	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,21 +252,14 @@ 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
 	}
 
 	ss.log(log.DEBUG, "resolving %q", host)
 
 	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].

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)

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