diff --git a/CHANGELOG.md b/CHANGELOG.md index aef7317156e..54b88a13225 100644 --- 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 diff --git a/internal/filtering/filter.go b/internal/filtering/filter.go index fcba11aa215..070ce73ce12 100644 --- 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, + ) + + 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) - log.Debug("filter: set properties: %s: {%s %s %v}", filt.URL, newf.Name, newf.URL, newf.Enabled) - filt.Name = newf.Name + filt.Name = newList.Name - if filt.URL != newf.URL { - r |= statusURLChanged | statusUpdateRequired - if d.filterExistsNoLock(newf.URL) { - return statusURLExists + 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 - } - - 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) - - filt.LastUpdated = time.Time{} - filt.checksum = 0 - filt.RulesCount = 0 - r |= statusUpdateRequired - } - } - } else { - filt.unload() + filt.unload() + } + + if filt.Enabled != newList.Enabled { + filt.Enabled = newList.Enabled + shouldRestart = true + } + + 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 diff --git a/internal/filtering/filter_test.go b/internal/filtering/filter_test.go index b37dd10e57c..99014bd0fa7 100644 --- 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) }) diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go index ab884056d83..a1de94cd4a5 100644 --- 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 } diff --git a/internal/filtering/http.go b/internal/filtering/http.go index 5c311c4318e..3f6880da90d 100644 --- 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) } diff --git a/internal/filtering/http_test.go b/internal/filtering/http_test.go new file mode 100644 index 00000000000..987b8a2c918 --- /dev/null +++ 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(``), + }} { + 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) + }) + } +} diff --git a/internal/home/clients.go b/internal/home/clients.go index ef1a11a7880..e4267ab6d29 100644 --- 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 }) diff --git a/internal/home/dns.go b/internal/home/dns.go index b5367b7df85..7d40ed351fe 100644 --- 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) }