Pull request: 4916 Editing filter

Merge in DNS/adguard-home from 4916-fix-filter-edit to master Closes #4916. Squashed commit of the following: commit c31be58abf73ed6047edc04ee606bceeb698f1bb Merge: c9f3e337 67d89660 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Fri Oct 21 19:58:16 2022 +0300 Merge branch 'master' into 4916-fix-filter-edit commit c9f3e337be8f005cc667d1cfd505f8cbca97cf20 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Fri Oct 21 14:49:53 2022 +0300 filtering: imp docs commit ef8228fd51772fb4c1876864a1d8e41caec45a70 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Fri Oct 21 12:40:00 2022 +0300 filtering: imp code commit 57fdbfca882537e50685b514f89bd9cf8a4cf5da Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Oct 20 11:54:39 2022 +0300 filtering: imp docs commit 670ac9aa009f4d6b021c37992182492f943a5005 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Wed Oct 19 21:03:26 2022 +0300 home: unexport close of clients container commit f5b29166ede4c89966740bee8d09b443fde9e475 Merge: 2e57624e 2de42284 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Wed Oct 19 21:02:33 2022 +0300 Merge branch 'master' into 4916-fix-filter-edit commit 2e57624e00ff702ef469ec0aa129eae9b627e41f Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Wed Oct 19 21:01:19 2022 +0300 filtering: imp code, tests commit be56df7cef9b0548de3ac6bb9ced7705d7f31783 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Tue Oct 18 15:31:30 2022 +0300 filtering: fix url edit
2024-08-16 22:20:30 +03:00 · 2022-10-21 20:14:43 +03:00 · 2022-10-21 20:14:43 +03:00 · a149d816d9
commit a149d816d9
parent 67d89660ca
8 changed files with 264 additions and 116 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -30,6 +30,8 @@ and this project adheres to

 ### Fixed

+- Editing an enabled rule list's URL now also includes validation of the filter
+  contents preventing from saving a bad one ([#4916]).
 - The default value of `dns.cache_size` accidentally set to 0 has now been
  reverted to 4 MiB ([#5010]).
 - Responses for which the DNSSEC validation had explicitly been omitted aren't
@ -38,6 +40,7 @@ and this project adheres to

 [#2926]: https://github.com/AdguardTeam/AdGuardHome/issues/2926
 [#3418]: https://github.com/AdguardTeam/AdGuardHome/issues/3418
+[#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
 [#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
 [#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942
 [#4986]: https://github.com/AdguardTeam/AdGuardHome/issues/4986
--- a/internal/filtering/filter.go
+++ b/internal/filtering/filter.go
@ -54,97 +54,120 @@ func (filter *FilterYAML) Path(dataDir string) string {
 }

 const (
-	statusFound = 1 << iota
-	statusEnabledChanged
-	statusURLChanged
-	statusURLExists
-	statusUpdateRequired
+	// errFilterNotExist is returned from [filterSetProperties] when there are
+	// no lists with the desired URL to update.
+	//
+	// TODO(e.burkov):  Use wherever the same error is needed.
+	errFilterNotExist errors.Error = "url doesn't exist"
+
+	// errFilterExists is returned from [filterSetProperties] when there is
+	// another filter having the same URL as the one updated.
+	//
+	// TODO(e.burkov):  Use wherever the same error is needed.
+	errFilterExists errors.Error = "url already exists"
 )

-// Update properties for a filter specified by its URL
-// Return status* flags.
-func (d *DNSFilter) filterSetProperties(url string, newf FilterYAML, whitelist bool) int {
-	r := 0
+// filterSetProperties searches for the particular filter list by url and sets
+// the values of newList to it, updating afterwards if needed.  It returns true
+// if the update was performed and the filtering engine restart is required.
+func (d *DNSFilter) filterSetProperties(
+	listURL string,
+	newList FilterYAML,
+	isAllowlist bool,
+) (shouldRestart bool, err error) {
 	d.filtersMu.Lock()
 	defer d.filtersMu.Unlock()

 	filters := d.Filters
-	if whitelist {
+	if isAllowlist {
 		filters = d.WhitelistFilters
 	}

-	i := slices.IndexFunc(filters, func(filt FilterYAML) bool {
-		return filt.URL == url
-	})
+	i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == listURL })
 	if i == -1 {
-		return 0
+		return false, errFilterNotExist
 	}

 	filt := &filters[i]
+	log.Debug(
+		"filtering: set name to %q, url to %s, enabled to %t for filter %s",
+		newList.Name,
+		newList.URL,
+		newList.Enabled,
+		filt.URL,
+	)

-	log.Debug("filter: set properties: %s: {%s %s %v}", filt.URL, newf.Name, newf.URL, newf.Enabled)
-	filt.Name = newf.Name
+	defer func(oldURL, oldName string, oldEnabled bool, oldUpdated time.Time) {
+		if err != nil {
+			filt.URL = oldURL
+			filt.Name = oldName
+			filt.Enabled = oldEnabled
+			filt.LastUpdated = oldUpdated
+		}
+	}(filt.URL, filt.Name, filt.Enabled, filt.LastUpdated)

-	if filt.URL != newf.URL {
-		r |= statusURLChanged | statusUpdateRequired
-		if d.filterExistsNoLock(newf.URL) {
-			return statusURLExists
+	filt.Name = newList.Name
+
+	if filt.URL != newList.URL {
+		if d.filterExistsLocked(newList.URL) {
+			return false, errFilterExists
 		}

-		filt.URL = newf.URL
-		filt.unload()
+		shouldRestart = true
+
+		filt.URL = newList.URL
 		filt.LastUpdated = time.Time{}
-		filt.checksum = 0
-		filt.RulesCount = 0
+		filt.unload()
 	}

-	if filt.Enabled != newf.Enabled {
-		r |= statusEnabledChanged
-		filt.Enabled = newf.Enabled
-		if filt.Enabled {
-			if (r & statusURLChanged) == 0 {
-				err := d.load(filt)
-				if err != nil {
-					// TODO(e.burkov):  It seems the error is only returned when
-					// the file exists and couldn't be open.  Investigate and
-					// improve.
-					log.Error("loading filter %d: %s", filt.ID, err)
+	if filt.Enabled != newList.Enabled {
+		filt.Enabled = newList.Enabled
+		shouldRestart = true
+	}

-					filt.LastUpdated = time.Time{}
-					filt.checksum = 0
-					filt.RulesCount = 0
-					r |= statusUpdateRequired
-				}
-			}
-		} else {
-			filt.unload()
+	if filt.Enabled {
+		if shouldRestart {
+			// Download the filter contents.
+			shouldRestart, err = d.update(filt)
 		}
+	} else {
+		// 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 making changing this
+		// behavior to be stricter.
+		filt.unload()
 	}

-	return r | statusFound
+	return shouldRestart, err
 }

-// Return TRUE if a filter with this URL exists
-func (d *DNSFilter) filterExists(url string) bool {
+// filterExists returns true if a filter with the same url exists in d.  It's
+// safe for concurrent use.
+func (d *DNSFilter) filterExists(url string) (ok bool) {
 	d.filtersMu.RLock()
 	defer d.filtersMu.RUnlock()

-	r := d.filterExistsNoLock(url)
+	r := d.filterExistsLocked(url)

 	return r
 }

-func (d *DNSFilter) filterExistsNoLock(url string) bool {
+// filterExistsLocked returns true if d contains the filter with the same url.
+// d.filtersMu is expected to be locked.
+func (d *DNSFilter) filterExistsLocked(url string) (ok bool) {
 	for _, f := range d.Filters {
 		if f.URL == url {
 			return true
 		}
 	}
+
 	for _, f := range d.WhitelistFilters {
 		if f.URL == url {
 			return true
 		}
 	}
+
 	return false
 }

@ -155,7 +178,7 @@ func (d *DNSFilter) filterAdd(flt FilterYAML) bool {
 	defer d.filtersMu.Unlock()

 	// Check for duplicates
-	if d.filterExistsNoLock(flt.URL) {
+	if d.filterExistsLocked(flt.URL) {
 		return false
 	}

@ -258,18 +281,6 @@ func (d *DNSFilter) tryRefreshFilters(block, allow, force bool) (updated int, is
 	return updated, isNetworkErr, ok
 }

-// refreshFilters updates the lists and returns the number of updated ones.
-// It's safe for concurrent use, but blocks at least until the previous
-// refreshing is finished.
-func (d *DNSFilter) refreshFilters(block, allow, force bool) (updated int) {
-	d.refreshLock.Lock()
-	defer d.refreshLock.Unlock()
-
-	updated, _ = d.refreshFiltersIntl(block, allow, force)
-
-	return updated
-}
-
 // listsToUpdate returns the slice of filter lists that could be updated.
 func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []FilterYAML) {
 	now := time.Now()
@ -279,7 +290,6 @@ func (d *DNSFilter) listsToUpdate(filters *[]FilterYAML, force bool) (toUpd []Fi

 	for i := range *filters {
 		flt := &(*filters)[i] // otherwise we will be operating on a copy
-		log.Debug("checking list at index %d: %v", i, flt)

 		if !flt.Enabled {
 			continue
--- a/internal/filtering/filter_test.go
+++ b/internal/filtering/filter_test.go
@ -4,40 +4,43 @@ import (
 	"io/fs"
 	"net"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"testing"
 	"time"

-	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

-const testFltsFileName = "1.txt"
-
-func testStartFilterListener(t *testing.T, fltContent *[]byte) (l net.Listener) {
+// serveFiltersLocally is a helper that concurrently listens on a free port to
+// respond with fltContent.  It also gracefully closes the listener when the
+// test under t finishes.
+func serveFiltersLocally(t *testing.T, fltContent []byte) (ipp netip.AddrPort) {
 	t.Helper()

 	h := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-		n, werr := w.Write(*fltContent)
-		require.NoError(t, werr)
-		require.Equal(t, len(*fltContent), n)
+		pt := testutil.PanicT{}
+
+		n, werr := w.Write(fltContent)
+		require.NoError(pt, werr)
+		require.Equal(pt, len(fltContent), n)
 	})

-	var err error
-	l, err = net.Listen("tcp", ":0")
+	l, err := net.Listen("tcp", ":0")
 	require.NoError(t, err)

-	go func() {
-		_ = http.Serve(l, h)
-	}()
+	go func() { _ = http.Serve(l, h) }()
 	testutil.CleanupAndRequireSuccess(t, l.Close)

-	return l
+	addr := l.Addr()
+	require.IsType(t, new(net.TCPAddr), addr)
+
+	return netip.AddrPortFrom(aghnet.IPv4Localhost(), uint16(addr.(*net.TCPAddr).Port))
 }

 func TestFilters(t *testing.T) {
@ -49,7 +52,7 @@ func TestFilters(t *testing.T) {

 	fltContent := []byte(content)

-	l := testStartFilterListener(t, &fltContent)
+	addr := serveFiltersLocally(t, fltContent)

 	tempDir := t.TempDir()

@ -64,11 +67,7 @@ func TestFilters(t *testing.T) {
 	f := &FilterYAML{
 		URL: (&url.URL{
 			Scheme: "http",
-			Host: (&netutil.IPPort{
-				IP:   net.IP{127, 0, 0, 1},
-				Port: l.Addr().(*net.TCPAddr).Port,
-			}).String(),
-			Path: path.Join(filterDir, testFltsFileName),
+			Host:   addr.String(),
 		}).String(),
 	}

@ -101,8 +100,15 @@ func TestFilters(t *testing.T) {
 	})

 	t.Run("refresh_actually", func(t *testing.T) {
-		fltContent = []byte(`||example.com^`)
-		t.Cleanup(func() { fltContent = []byte(content) })
+		anotherContent := []byte(`||example.com^`)
+		oldURL := f.URL
+
+		ipp := serveFiltersLocally(t, anotherContent)
+		f.URL = (&url.URL{
+			Scheme: "http",
+			Host:   ipp.String(),
+		}).String()
+		t.Cleanup(func() { f.URL = oldURL })

 		updateAndAssert(t, require.True, 1)
 	})
--- a/internal/filtering/filtering.go
+++ b/internal/filtering/filtering.go
@ -345,27 +345,29 @@ func (d *DNSFilter) SetFilters(blockFilters, allowFilters []Filter, async bool)
 			blockFilters: blockFilters,
 		}

-		d.filtersInitializerLock.Lock() // prevent multiple writers from adding more than 1 task
+		d.filtersInitializerLock.Lock()
 		defer d.filtersInitializerLock.Unlock()

-		// remove all pending tasks
-		stop := false
-		for !stop {
+		// Remove all pending tasks.
+	removeLoop:
+		for {
 			select {
 			case <-d.filtersInitializerChan:
-				//
+				// Continue removing.
 			default:
-				stop = true
+				break removeLoop
 			}
 		}

 		d.filtersInitializerChan <- params
+
 		return nil
 	}

 	err := d.initFiltering(allowFilters, blockFilters)
 	if err != nil {
-		log.Error("Can't initialize filtering subsystem: %s", err)
+		log.Error("filtering: can't initialize filtering subsystem: %s", err)
+
 		return err
 	}

--- a/internal/filtering/http.go
+++ b/internal/filtering/http.go
@ -56,7 +56,6 @@ func (d *DNSFilter) handleFilteringAddURL(w http.ResponseWriter, r *http.Request

 	err = validateFilterURL(fj.URL)
 	if err != nil {
-		err = fmt.Errorf("invalid url: %s", err)
 		aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)

 		return
@ -75,8 +74,10 @@ func (d *DNSFilter) handleFilteringAddURL(w http.ResponseWriter, r *http.Request
 		URL:     fj.URL,
 		Name:    fj.Name,
 		white:   fj.Whitelist,
+		Filter: Filter{
+			ID: assignUniqueFilterID(),
+		},
 	}
-	filt.ID = assignUniqueFilterID()

 	// Download the filter contents
 	ok, err := d.update(&filt)
@ -216,32 +217,15 @@ func (d *DNSFilter) handleFilteringSetURL(w http.ResponseWriter, r *http.Request
 		Name:    fj.Data.Name,
 		URL:     fj.Data.URL,
 	}
-	status := d.filterSetProperties(fj.URL, filt, fj.Whitelist)
-	if (status & statusFound) == 0 {
-		aghhttp.Error(r, w, http.StatusBadRequest, "URL doesn't exist")

-		return
-	}
-	if (status & statusURLExists) != 0 {
-		aghhttp.Error(r, w, http.StatusBadRequest, "URL already exists")
+	restart, err := d.filterSetProperties(fj.URL, filt, fj.Whitelist)
+	if err != nil {
+		aghhttp.Error(r, w, http.StatusBadRequest, err.Error())

 		return
 	}

 	d.ConfigModified()
-
-	restart := (status & statusEnabledChanged) != 0
-	if (status&statusUpdateRequired) != 0 && fj.Data.Enabled {
-		// download new filter and apply its rules.
-		nUpdated := d.refreshFilters(!fj.Whitelist, fj.Whitelist, false)
-		// if at least 1 filter has been updated, refreshFilters() restarts the filtering automatically
-		// if not - we restart the filtering ourselves
-		restart = false
-		if nUpdated == 0 {
-			restart = true
-		}
-	}
-
 	if restart {
 		d.EnableFilters(true)
 	}
--- a/internal/filtering/http_test.go
+++ b/internal/filtering/http_test.go
@ -0,0 +1,143 @@
+package filtering
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDNSFilter_handleFilteringSetURL(t *testing.T) {
+	filtersDir := t.TempDir()
+
+	var goodRulesEndpoint, anotherGoodRulesEndpoint, badRulesEndpoint string
+	for _, rulesSource := range []struct {
+		endpoint *string
+		content  []byte
+	}{{
+		endpoint: &goodRulesEndpoint,
+		content:  []byte(`||example.org^`),
+	}, {
+		endpoint: &anotherGoodRulesEndpoint,
+		content:  []byte(`||example.com^`),
+	}, {
+		endpoint: &badRulesEndpoint,
+		content:  []byte(`<html></html>`),
+	}} {
+		ipp := serveFiltersLocally(t, rulesSource.content)
+		*rulesSource.endpoint = (&url.URL{
+			Scheme: "http",
+			Host:   ipp.String(),
+		}).String()
+	}
+
+	testCases := []struct {
+		name     string
+		wantBody string
+		oldURL   string
+		newName  string
+		newURL   string
+		initial  []FilterYAML
+	}{{
+		name:     "success",
+		wantBody: "",
+		oldURL:   goodRulesEndpoint,
+		newName:  "default_one",
+		newURL:   anotherGoodRulesEndpoint,
+		initial: []FilterYAML{{
+			Enabled: true,
+			URL:     goodRulesEndpoint,
+			Name:    "default_one",
+			white:   false,
+		}},
+	}, {
+		name:     "non-existing",
+		wantBody: "url doesn't exist\n",
+		oldURL:   anotherGoodRulesEndpoint,
+		newName:  "default_one",
+		newURL:   goodRulesEndpoint,
+		initial: []FilterYAML{{
+			Enabled: true,
+			URL:     goodRulesEndpoint,
+			Name:    "default_one",
+			white:   false,
+		}},
+	}, {
+		name:     "existing",
+		wantBody: "url already exists\n",
+		oldURL:   goodRulesEndpoint,
+		newName:  "default_one",
+		newURL:   anotherGoodRulesEndpoint,
+		initial: []FilterYAML{{
+			Enabled: true,
+			URL:     goodRulesEndpoint,
+			Name:    "default_one",
+			white:   false,
+		}, {
+			Enabled: true,
+			URL:     anotherGoodRulesEndpoint,
+			Name:    "another_default_one",
+			white:   false,
+		}},
+	}, {
+		name:     "bad_rules",
+		wantBody: "data is HTML, not plain text\n",
+		oldURL:   goodRulesEndpoint,
+		newName:  "default_one",
+		newURL:   badRulesEndpoint,
+		initial: []FilterYAML{{
+			Enabled: true,
+			URL:     goodRulesEndpoint,
+			Name:    "default_one",
+			white:   false,
+		}},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			confModifiedCalled := false
+			d, err := New(&Config{
+				FilteringEnabled: true,
+				Filters:          tc.initial,
+				HTTPClient: &http.Client{
+					Timeout: 5 * time.Second,
+				},
+				ConfigModified: func() { confModifiedCalled = true },
+				DataDir:        filtersDir,
+			}, nil)
+			require.NoError(t, err)
+			t.Cleanup(d.Close)
+
+			d.Start()
+
+			reqData := &filterURLReq{
+				Data: &filterURLReqData{
+					// Leave the name of an existing list.
+					Name:    tc.newName,
+					URL:     tc.newURL,
+					Enabled: true,
+				},
+				URL:       tc.oldURL,
+				Whitelist: false,
+			}
+			data, err := json.Marshal(reqData)
+			require.NoError(t, err)
+
+			r := httptest.NewRequest(http.MethodPost, "http://example.org", bytes.NewReader(data))
+			w := httptest.NewRecorder()
+
+			d.handleFilteringSetURL(w, r)
+			assert.Equal(t, tc.wantBody, w.Body.String())
+
+			// For the moment the non-empty response body only contains occurred
+			// error, so the configuration shouldn't be written.
+			assert.Equal(t, tc.wantBody == "", confModifiedCalled)
+		})
+	}
+}
--- a/internal/home/clients.go
+++ b/internal/home/clients.go
@ -932,9 +932,9 @@ func (clients *clientsContainer) updateFromDHCP(add bool) {
 	log.Debug("clients: added %d client aliases from dhcp", n)
 }

-// Close gracefully closes all the client-specific upstream configurations of
+// close gracefully closes all the client-specific upstream configurations of
 // the persistent clients.
-func (clients *clientsContainer) Close() (err error) {
+func (clients *clientsContainer) close() (err error) {
 	persistent := maps.Values(clients.list)
 	slices.SortFunc(persistent, func(a, b *Client) (less bool) { return a.Name < b.Name })

--- a/internal/home/dns.go
+++ b/internal/home/dns.go
@ -441,7 +441,7 @@ func stopDNSServer() (err error) {
 		return fmt.Errorf("stopping forwarding dns server: %w", err)
 	}

-	err = Context.clients.Close()
+	err = Context.clients.close()
 	if err != nil {
 		return fmt.Errorf("closing clients container: %w", err)
 	}